Claude/orchestra cash app onramp - #2588
Merged
Merged
Conversation
Adds a USD deposit rail paid from Cash App. Selecting USD under "Deposit with cash" now opens a chooser — "Wire transfer, ACH" (the virtual account) or "Cash App" — instead of going straight to the bank rail. Outside the US the chooser is skipped, because one option is a question with one answer. The flow is amount → invoice → status, with the invoice screen acting as the review step: it shows the sats, fee and delivery Orchestra actually quoted, and nothing is charged until it is paid. There is no live quote on the amount screen by design — /estimate prices in sats only, with no fiat parameter at any spelling, and the app has no spot source, so a dollar figure there could only be quoted against a rate Orchestra never agreed to. Everything goes through our own backend at /accounts/v1/orchestra, which holds the Orchestra server key. The app carries no Orchestra credential: the recipient is the Safe the server resolves from the session, and availability — region plus allowlist — is the server's verdict, which it enforces again on order creation rather than trusting a hidden row. Status is streamed over SSE where EventSource exists and polled at the documented three seconds everywhere else, which is every native build. The poll keeps running underneath a healthy stream: frames carry only the status, and a reconnect does not replay what was missed. Amounts are formatted from smallest units with BigInt and grouped by hand, because Hermes and the React Native Web Intl shim throw on a BigInt passed to Intl.NumberFormat — Node's does not, so the tests alone would not have caught it. There is now a test that makes Intl behave the way the app's runtime does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The modal header is a three-part row: a 50px back button, the title, and the close button. The title's container had no flex constraint and the title itself no line limit, so a long one took its natural width and shoved both controls out of the header. They were still mounted, just off-screen — which on the Cash App amount step left a modal with no visible way back. The no-title branch already used `flex-1`; the title branch never did. It now takes the space between the two controls and truncates to one line rather than growing past them. Also shortens that step's own title from "Deposit with Cash App" to "Cash App". It is reached by tapping a row already labelled Cash App, from a screen titled "Deposit US Dollars", so the longer version was repeating context to earn a truncation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+66
to
+73
| useEffect(() => { | ||
| if (!config || config.isAvailable) return; | ||
| setError( | ||
| orchestraErrorFromCode(ORCHESTRA_ERROR_CODE.NOT_IN_AUDIENCE), | ||
| DEPOSIT_MODAL.OPEN_ORCHESTRA_AMOUNT, | ||
| ); | ||
| setModal(DEPOSIT_MODAL.OPEN_ORCHESTRA_ERROR); | ||
| }, [config, setError, setModal]); |
Contributor
There was a problem hiding this comment.
Bug: The useOrchestraConfig hook fires with an undefined country code before geo-detection completes, causing a premature navigation to an error screen for eligible users.
Severity: HIGH
Suggested Fix
Use the isResolved boolean returned from the useCashAppDepositAvailability hook to conditionally enable the useOrchestraConfig query. The query should only be enabled when isResolved is true.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: components/Orchestra/OrchestraAmount.tsx#L66-L73
Potential issue: The `useOrchestraConfig` hook is called immediately upon component
mount, even when the `countryCode` from `useCashAppDepositAvailability` is still
`undefined`. This triggers a network request to the `/config` endpoint without a country
parameter. The backend conservatively returns `isAvailable: false` for this request. An
effect hook then incorrectly interprets this as the user being ineligible and navigates
them to an error screen. This race condition occurs before the asynchronous
geo-detection can complete and provide a valid country code, causing US-based users to
be unnecessarily redirected to an error page.
Did we get this right? 👍 / 👎 to inform future reviews.
| const setModal = useOrchestraNavigation(); | ||
| const order = useOrchestraStore(state => state.order); | ||
| const amountUsd = useOrchestraStore(state => state.amountUsd); | ||
| const { data: config } = useOrchestraConfig(); |
Contributor
There was a problem hiding this comment.
Bug: The useOrchestraConfig hook is called without a countryCode, causing unnecessary API calls and a potential bug where the transaction amount is displayed incorrectly.
Severity: MEDIUM
Suggested Fix
The countryCode should be stored in a shared state, like useOrchestraStore, after it's determined in OrchestraAmount. Downstream components like OrchestraInvoice and OrchestraOrderStatus should then retrieve the countryCode from the store and pass it to useOrchestraConfig to ensure cache reuse and data consistency.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: components/Orchestra/OrchestraInvoice.tsx#L60
Potential issue: The `OrchestraInvoice` and `OrchestraOrderStatus` components call
`useOrchestraConfig()` without a `countryCode`. This results in a separate cache key
(`['orchestraConfig', undefined]`) from the one used in the previous screen
(`['orchestraConfig', 'US']`), causing an unnecessary API request. If the backend
returns different data for a request without a country code, such as missing `decimals`,
the `formatSmallestUnits` function could return `undefined`. This would lead to an
incorrect amount being displayed on the success screen, showing "undefined" or "Not
available" instead of the actual amount.
Also affects:
components/Orchestra/OrchestraOrderStatus.tsx:61
Did we get this right? 👍 / 👎 to inform future reviews.
The config query fired on mount with countryCode still undefined. The server correctly answers "not available" to a request that names no region — but the amount screen read that as a verdict on the user and bounced them to the error screen, a beat before the real answer arrived. The hook kept its detected country in local state rather than the country store, so every mount started at undefined and each screen re-ran the race independently. The cause was that "we don't know yet" and "we looked and couldn't tell" were the same value. They are different answers: the first must not be acted on, the second is final. The hook now reports isResolving separately from isResolved, and every /config call waits for it. This only ever hit users who are not on the allowlist — an allowlisted username short-circuits the region check server-side, which is why it survived testing on an allowlisted account. The invoice and status screens also now pass the country, so all four call sites share one cache entry instead of firing a second request under ['orchestraConfig', undefined]. Their amounts were never wrong — decimals come from /v2/routes and do not depend on the country — but that entry cached isAvailable: false, which was a trap for anyone adding a gate to those screens later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gating /config on the geo lookup fixed the false "not available" bounce, but left each screen restarting the lookup from scratch: the hook kept its result in component state, so arriving at the invoice screen held its config call off for a frame and flashed empty amounts. detectGeo already memoises the request; what was missing was the settled result. Holding it at module scope lets a later mount start with the answer in hand — its first render is already settled, which is what the new test asserts rather than whatever state the mount eventually reaches. Exports a reset for tests, because module state would otherwise leak between cases. Also replaces the reassign-an-outer-binding pattern in this spec with collection into an array: the react-compiler rule counts the former as a render side-effect, and the file had been failing that rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.