Skip to content

fix(pay): handle nullable options for terminal payment states#474

Draft
ignaciosantise wants to merge 5 commits into
mainfrom
succeeded-payment-options
Draft

fix(pay): handle nullable options for terminal payment states#474
ignaciosantise wants to merge 5 commits into
mainfrom
succeeded-payment-options

Conversation

@ignaciosantise
Copy link
Copy Markdown
Collaborator

@ignaciosantise ignaciosantise commented Apr 6, 2026

Summary

  • Fixes "Invalid Response Payload" error when calling get_payment_options on payments in terminal states (succeeded, failed, expired, cancelled) — the API returns "options": null in that case, which progenitor previously failed to deserialize into Vec<PaymentOption>
  • Marks the options field as nullable in the OpenAPI spec for GetPaymentOptionsResponse
  • Exposes options as Option<Vec<PaymentOption>> end-to-end so callers can distinguish null (terminal state — check info.status) from [] (no viable options, e.g. insufficient balance)
  • Added unit test covering the null options terminal-state path

Breaking change

  • PaymentOptionsResponse.options is now Option<Vec<PaymentOption>> (nullable in FFI/JSON consumers). Swift/Kotlin wallets will need to handle the optional.

Caveats

  • The OpenAPI spec edit is currently a manual hand-tweak. The backend team will publish an updated spec with nullable: true on options; once that's released, this spec file should be replaced wholesale with the official version before merging.

Test plan

  • All pay module tests pass (84 passed)
  • Unit test exercises the "options": null path with a succeeded payment response
  • Replace openapi.json with the official spec once released by the backend team
  • Verify with a real succeeded payment that the SDK returns options: None + correct info.status

🤖 Generated with Claude Code

The API returns `options: null` for payments in terminal states (succeeded,
failed, expired, cancelled). Mark the field as nullable in the OpenAPI spec
and coerce null to an empty vec so callers can check `info.status` instead
of hitting an "Invalid Response Payload" deserialization error.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 6, 2026 17:57
@claude
Copy link
Copy Markdown

claude Bot commented Apr 6, 2026

Claude finished @ignaciosantise's task in 1m 6s —— View job


Review complete

  • Gather context
  • Read changed files
  • Analyze changes
  • Post review
Found 1 issue(s)

Issue 1: No unit test for null options (terminal state) case

ID: mod-null-options-test-a3f1
File: crates/yttrium/src/pay/mod.rs
Severity: LOW
Category: code_quality

Context:

  • Pattern: The fix handles options: null via unwrap_or_default() (line 986), but there's no unit test exercising this path. Existing tests cover options: [...] (success) and HTTP error codes (404, 410) but not {"options": null}.
  • Risk: Silent regression if the codegen or serde behavior changes (e.g., if options stops being Option<Vec<...>>).
  • Impact: Future refactors could break terminal-state handling without failing tests.
  • Trigger: Payment in succeeded/failed/cancelled/expired state returning null options.

Recommendation: Add a test alongside the existing ones:

#[tokio::test]
async fn test_get_payment_options_null_options_terminal_state() {
    // mock returns {"options": null, "info": {...}}
    // assert response.options.is_empty()
    // assert response.info.is_some() // status shows terminal state
}

Fix this →

The core fix is correct — marking options as nullable: true in the OpenAPI spec (while keeping it in required, meaning the key is always present but may be null) and using .unwrap_or_default() is the right approach for OpenAPI 3.0 semantics.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Pay SDK’s get_payment_options handling and the embedded OpenAPI spec to support payments in terminal states where the backend may return options: null, avoiding “Invalid Response Payload” decoding failures.

Changes:

  • Mark GetPaymentOptionsResponse.options as nullable in the OpenAPI spec.
  • Coerce null options into an empty vector in WalletConnectPay::get_payment_options and use that for logging/caching/response mapping.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
crates/yttrium/src/pay/openapi.json Marks options as nullable in GetPaymentOptionsResponse to reflect backend behavior in terminal states.
crates/yttrium/src/pay/mod.rs Adjusts get_payment_options to handle nullable options by defaulting to an empty list for downstream logic.
Comments suppressed due to low confidence (1)

crates/yttrium/src/pay/mod.rs:1001

  • This change introduces new behavior for when the API returns options: null, but the existing get_payment_options tests only cover non-null arrays. Add a test case that mocks a 200 response with "options": null (and any other required fields) and asserts the SDK returns an empty options vec and clears the cache accordingly.
        let api_response = response.into_inner();
        let options = api_response.options.unwrap_or_default();
        pay_debug!(
            "get_payment_options: success, {} options",
            options.len()
        );

        // Cache the options with their raw actions
        let cached: Vec<CachedPaymentOption> = options
            .iter()
            .map(|o| CachedPaymentOption {
                option_id: o.id.clone(),
                actions: o.actions.clone(),
            })
            .collect();
        let mut cache = self.cached_options.write();
        *cache = cached;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/yttrium/src/pay/mod.rs
ignaciosantise and others added 2 commits April 6, 2026 15:01
Verifies that get_payment_options handles options: null correctly when
the API returns a 200 for a succeeded payment with includePaymentInfo.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ignaciosantise ignaciosantise marked this pull request as draft April 7, 2026 14:14
Comment thread crates/yttrium/src/pay/mod.rs Outdated
"get_payment_options: success, {} options",
api_response.options.len()
);
let options = api_response.options.unwrap_or_default();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of defaulting to empty set of options, I think you should early return w/ the status indicating that the payment is no longer available? Idk anything other than defaulting to empty

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i will just return the null value, and on wallet side check the payment status

ignaciosantise and others added 2 commits April 20, 2026 14:40
Preserve the distinction between null (terminal payment state) and empty
array (no viable options for the provided accounts) instead of coercing
null to an empty vec. Callers can now branch on info.status when options
is None.

BREAKING: PaymentOptionsResponse.options is now Option<Vec<PaymentOption>>.

Co-Authored-By: Claude Opus 4.6 <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.

3 participants