Skip to content

Add canonical payment method actions - #73

Closed
a16i wants to merge 6 commits into
masterfrom
a16i/payment-method-actions
Closed

Add canonical payment method actions#73
a16i wants to merge 6 commits into
masterfrom
a16i/payment-method-actions

Conversation

@a16i

@a16i a16i commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • introduce stable PaymentMethodId, derived PaymentMethodCategory, Money, and PaymentAction contracts
  • create hosted/onramp/exchange methods with one ID-discriminated request shape
  • replace provider-shaped modal navigation with a generic PaymentMethod node and action-driven renderer
  • keep current exchange, Cash App, and Stripe request/response/nav shapes as deprecated compatibility adapters
  • add public custom-UI documentation and characterization tests

Why

Payment method identity, product grouping, and client execution had become conflated. Cash App was modeled as an exchange, while hosted onramps were heading toward a generic external bucket. This foundation gives each concept one durable responsibility before Revolut is added:

  • identity: PaymentMethodId
  • grouping: PaymentMethodCategory
  • execution: PaymentAction
  • implementation/provider: server-private

This PR intentionally does not add Revolut.

Validation

  • cd packages/sdk && pnpm test
    • 34 test files, 210 tests
    • ESLint
    • web and native TypeScript builds
    • stylesheet verification

Open in Devin Review

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 3 new potential issues.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment thread packages/sdk/src/web/components/PaymentActionPage.tsx Outdated
Comment on lines +51 to +58
message={
openUrl?.waitingMessage ??
(embeddedWidget
? t.depositExactlyWith(formattedSourceAmount, node.title)
: usesDesktopQR
? t.scanWithPhone
: `${t.continueTo} ${node.title} ${t.toCompleteYourDeposit}`)
}

@devin-ai-integration devin-ai-integration Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Payment instructions can render blank when the server sends an empty waiting message

The waiting text is taken from the server even when it is an empty string (openUrl?.waitingMessage ?? at packages/sdk/src/web/components/PaymentActionPage.tsx:52), so the instruction area renders empty instead of falling back to the built-in "Continue to …" copy.
Impact: A user can land on a payment handoff screen with no instructions at all.

`||` was replaced by `??` during the migration

The deleted ExchangePage used waitingMessage || (usesDesktopQR ? t.scanWithPhone : …), so an empty waitingMessage fell back to localized copy. The legacy adapter copies the field verbatim (waitingMessage: result.exchange.waitingMessage in packages/sdk/src/web/hooks/useSessionNav.ts:182) and zPaymentAction accepts any string including "" (packages/sdk/src/common/paymentMethod.ts:71), so an empty server message now yields a blank message block.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +64 to +66
popupName={
openUrl?.popupName ?? embeddedWidget?.sdk ?? node.id.toLowerCase()
}

@devin-ai-integration devin-ai-integration Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Desktop payment popups get a window name derived from the menu entry instead of the payment method

When a server returns a next step without an explicit popup name, the desktop popup is named after the internal menu entry (node.id.toLowerCase() at packages/sdk/src/web/components/PaymentActionPage.tsx:50) even though the correct method-based name was already computed, so reopening the same method can open a second window instead of reusing the first.
Impact: A user can end up with duplicate provider windows for the same payment method on desktop.

Mechanism: legacyHandoff.popupName is computed but never used as a fallback

legacyHandoff (packages/sdk/src/web/components/PaymentActionPage.tsx:34-37) comes from getNavExternalHandoff, which returns both desktopBehavior and popupName (packages/sdk/src/web/api/navTree.ts:252-259, 278-295). desktopBehavior is used as a fallback at line 39, but popupName is ignored: the fallback chain goes openUrl?.popupName ?? (Stripe ? "stripe" : embeddedWidget?.sdk ?? node.id.toLowerCase()).

Nav node ids are menu identifiers such as Exchange-Coinbase or PaymentMethod-MtPelerin, so the popup name becomes exchange-coinbase, whereas the deleted ExchangePage used exchangeId.toLowerCase() (i.e. coinbase). Today's legacy responses always populate popupName via getPaymentAction (packages/sdk/src/web/hooks/useSessionNav.ts:176-185), so this only bites for canonical PaymentMethod nodes or actions returned without popupName; window.open then uses a different target name for the same method (packages/sdk/src/web/components/HostedPaymentPage.tsx:66-72).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 6 additional findings in Devin Review.

Open in Devin Review

Comment on lines +43 to +44
const sourceUnits = formatNavSourceAmountUnits(sourceAmount, sourcePolicy);
const formattedSourceAmount = `${sourcePolicy.currencySymbol}${sourceUnits} ${sourcePolicy.currency}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Deposit amount shown to non-English users is formatted with the wrong decimal separator

The amount shown in the "deposit exactly …" instruction is built with a raw wire-format helper (formatNavSourceAmountUnits at packages/sdk/src/web/components/PaymentActionPage.tsx:43) instead of the locale-aware formatter used before, so users in Spanish, Portuguese and other locales see a number punctuated the wrong way.
Impact: Non-English users can be shown an amount such as "1000.00" where their locale expects "1.000,00", making the exact-amount instruction confusing.

Mechanism: toFixed replaces Intl-based formatFixedAmount in the Stripe hand-off copy

The removed StripeOnrampPage built its message with formatFixedAmount(amountUsd) (packages/sdk/src/web/formatAmount.ts:59-64), which uses Intl.NumberFormat with the active number locale, producing grouped, locale-punctuated output.

PaymentActionPage now derives display text from formatNavSourceAmountUnits (packages/sdk/src/web/api/navTree.ts:262-275), which is amount.toFixed(decimals) — the correct format for the request body sent in getPaymentMethodRequest (packages/sdk/src/web/hooks/useSessionNav.ts:128-136), but not for display. formattedSourceAmount at packages/sdk/src/web/components/PaymentActionPage.tsx:44 is only consumed by t.depositExactlyWith(...), so the fix is to keep the wire string separate from a locale-formatted display string.

Prompt for agents
In packages/sdk/src/web/components/PaymentActionPage.tsx, the user-visible amount in the embedded-widget message is produced by formatNavSourceAmountUnits, which is a wire-format helper (amount.toFixed(decimals)) with no locale awareness. The page it replaced (StripeOnrampPage) used formatFixedAmount from packages/sdk/src/web/formatAmount.ts, which formats via Intl.NumberFormat with the active number locale. Keep the wire format for request bodies (useSessionNav) but use the locale-aware formatter, passing the backend-provided decimals, for the displayed string; also decide whether appending the currency code after a currency symbol is desired.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@a16i a16i closed this Aug 1, 2026
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