Skip to content

feat(pay): upgrade OpenAPI spec to 3.1 and progenitor to 0.13#497

Open
chris13524 wants to merge 5 commits into
mainfrom
upgrade-progenitor-3.1
Open

feat(pay): upgrade OpenAPI spec to 3.1 and progenitor to 0.13#497
chris13524 wants to merge 5 commits into
mainfrom
upgrade-progenitor-3.1

Conversation

@chris13524
Copy link
Copy Markdown
Member

Summary

Type changes consumed

  • PaymentId is now a strict newtype that only impls From<String>, so .id(&payment_id) becomes .id(payment_id.clone()) (or .to_string() when starting from &str).
  • CollectData.url is non-optional String; CollectData.schema is serde_json::Value (not Option<Value>).
  • Amount.unit is now the generated types::String enum (Iso4217/Caip19 variants); flattened back to the prefixed string form ("iso4217/..." / "caip19/...").
  • GetPaymentOptionsResponse.options is now Option<Vec<...>>; unwrapped with .unwrap_or_default().

Known issue

The new openapi.json defines a top-level String schema as a oneOf of two object variants (Iso4217/Caip19), but the runtime format (per the spec example) is a plain prefixed string like "iso4217/USD". Serde deserialization will likely fail at runtime on the unit field. This is a schema authoring issue rather than a progenitor or code issue, but worth flagging — the schema may need to revert unit to a plain string type with a description.

Test plan

  • cargo check --features=full --lib --bins -p yttrium passes
  • cargo check --features=full --tests passes
  • Pay e2e tests (just test-pay-e2e) — requires REOWN_PROJECT_ID
  • Verify Kotlin/Swift bindings still generate (just kotlin, just swift)
  • Verify runtime behavior of the unit field once the schema issue is resolved

🤖 Generated with Claude Code

- Update openapi.json from 3.0.3 to 3.1.0 (version 2026-02-18).
- Replace progenitor 0.11.2 (crates.io) with WalletConnect/progenitor
  fork at oas3-migration, which adds OpenAPI 3.1 support.
- Adapt pay/mod.rs to the new generated types:
  - `.id(&payment_id)` -> `.id(payment_id.clone())` (PaymentId is now a
    newtype that only impls `From<String>`).
  - `CollectData.url` is non-optional `String`; `CollectData.schema` is
    `serde_json::Value` (not Option<Value>).
  - `Amount.unit` is now the `types::String` enum (Iso4217/Caip19);
    flatten back to the prefixed string form.
  - `GetPaymentOptionsResponse.options` is now `Option<Vec<...>>`;
    unwrap with `.unwrap_or_default()`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your contribution! We ask that you please read and sign our CTA Document before we can accept your contribution. You can sign the CTA simply by posting a Pull Request Comment with the following text:


I have read the CTA Document and I hereby sign the CTA


Chris Smith seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

Claude finished @chris13524's task in 3m 44s —— View job


PR Review: feat(pay): upgrade OpenAPI spec to 3.1 and progenitor to 0.13

  • Gather context
  • Read changed files
  • Run license compliance check
  • Run breaking changes check
  • Analyze and post review

License compliance: No new license issues. progenitor was already MPL-2.0 at 0.11.2; only truly new dep is oas3 (MIT). ✅

Breaking changes: Public/FFI surface is unchanged — CollectDataAction, PayAmount, PaymentOptionsResponse all keep the same types via the adapter layer. ✅


Found 3 issue(s)

Issue 1: Runtime deserialization will fail for Amount.unit field

ID: mod-amount-unit-deser-fail-9c2a
File: crates/yttrium/src/pay/mod.rs:677
Severity: CRITICAL
Category: bug

Context:

  • Pattern: types::String is a progenitor-generated enum expecting {"Iso4217": "USD"} or {"Caip19": "..."} on the wire, but the API sends the plain prefixed string "iso4217/USD".
  • Risk: Every call to get_payment_options / get_required_payment_actions that returns an Amount will panic/error at the serde deserialization step, not the conversion step. The From<types::Amount> conversion at line 677 is correct — the bug is upstream of it, when progenitor deserializes the HTTP response body.
  • Impact: All payment option fetching is broken at runtime. The PR itself flags this as a "Known issue", but it should be resolved before merging.
  • Trigger: Any live API call returning an Amount object.

Recommendation: Fix the String schema in openapi.json to use a plain string type (matching the actual wire format) rather than the oneOf object variants. The description and example can document the prefix convention:

"String": {
  "type": "string",
  "description": "Currency unit: 'iso4217/USD' for fiat, 'caip19/eip155:1/erc20:0x...' for tokens",
  "example": "iso4217/USD"
}

Then types::String becomes a plain String and the match in mod.rs:677 simplifies back to unit: a.unit.


Issue 2: Git-pinned fork dependency for progenitor

ID: cargo-progenitor-git-pin-b3f1
File: Cargo.toml:53
Severity: HIGH
Category: maintainability

Context:

  • Pattern: progenitor and progenitor-client are sourced from a WalletConnect GitHub fork pinned to commit 664b34bb321d47559752059b7fe0e07fd96419ee instead of a crates.io release.
  • Risk: If that commit is force-pushed or the fork is deleted, the build breaks with no recoverable path. The fork's oas3-migration branch also depends on a PR (WalletConnect/progenitor#1) that may not be merged upstream.
  • Impact: Build instability for all contributors and CI; cannot be audited by cargo audit.
  • Trigger: Any cargo build after the fork's git history is modified.

Recommendation: Either publish the fork to crates.io, or add a comment documenting the expected lifetime of this dependency and a tracking issue to upstream the OAS 3.1 support to the official progenitor crate.


Issue 3: Silent error suppression in CollectDataAction schema serialization

ID: mod-collectdata-schema-silent-err-d4e8
File: crates/yttrium/src/pay/mod.rs:616
Severity: LOW
Category: code_quality

Context:

  • Pattern: serde_json::to_string(&c.schema).ok() silently drops a serialization error, yielding schema: None.
  • Risk: serde_json::Value serialization is practically infallible, but the inconsistency with line 552 (unwrap_or_else(|e| { ... log error ... })) means if it ever does fail, the schema is silently dropped without any log signal.
  • Impact: Hard-to-diagnose data loss in the webview collect flow.

Recommendation: Match the pattern used at line 552:

schema: serde_json::to_string(&c.schema)
    .map_err(|e| pay_debug!("schema serialize error: {e}"))
    .ok(),

Chris Smith and others added 3 commits April 29, 2026 11:20
Picks up the upstream merge from oxidecomputer/progenitor (typify 0.6.2,
tokio/uuid/clap/assert_cmd/regress version bumps, and the v0.14.0
release prep). Also bumps semver 1.0.27 -> 1.0.28 in the lockfile to
satisfy typify 0.6.2's MSRV on semver.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copies external-latest.json from pay-core. Net semantic changes:
- Add `PaymentFailureCode` schema (currently `declined_user`).
- Add optional `failureCode` field on payment response objects.
- Extend GetPaymentOptionsError enum: `payment_not_succeeded`,
  `already_refunded`.

Compiles cleanly with no code changes — additions are all optional or
enum-additive on the response side.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The OpenAPI spec defines a `String` schema as a oneOf of two object
variants (`Iso4217`/`Caip19`), but the runtime wire format is the plain
prefixed string described by the example (e.g. `"iso4217/USD"`). The
default progenitor codegen mismatched the wire and broke deserialization
of the `Amount.unit` field — visible in
`test_json_get_required_payment_actions_success`.

Use progenitor's `replace = { ... }` directive to skip generation of the
schema's `String` and use `std::string::String` directly. Also drops the
manual variant-flatten that's no longer needed in `From<types::Amount>`.

Also collapses a multi-line `pay_debug!` to satisfy `cargo +nightly fmt
--all -- --check`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@chris13524
Copy link
Copy Markdown
Member Author

I have read the CTA Document and I hereby sign the CTA

Pay-core's openapi correctly describes `Amount.unit` (and the other two
`AmountUnit`-typed fields) as a plain `string` schema once
WalletConnect/pay-core#603 lands. Pre-applies the equivalent
transformation to yttrium's vendored copy of openapi.json:

- Drops the bogus `String` component schema (a tagged oneOf of
  `Iso4217`/`Caip19` object variants that never matched the wire form).
- Inlines a `{type: "string", example: "iso4217/USD"}` schema at each
  `unit` field site that previously `$ref`-ed the bad schema.

With the spec corrected, the `replace = { String = std::string::String }`
workaround in `progenitor::generate_api!` is no longer needed and is
dropped. The 82 pay tests still pass; the test that originally regressed
(`test_json_get_required_payment_actions_success`) deserializes the
plain prefixed-string `unit` field via the standard generated types.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant