fix(payments): present all keep-open send codes as pending, never re-sendable (double-pay #440/#441)#442
Conversation
…sendable (double-pay) Two send paths could re-offer a Send/Pay button after a possibly-committed on-chain spend, letting the user double-pay: - #440 useTransfer only mapped SEND_SYNC_PENDING and CERTIFICATION_UNCONFIRMED to a synthetic pending-success; the other keep-open certification errors the SDK re-throws RAW (CHECKPOINT_PERSIST_FAILED and the E.4 split-checkpoint pair SPLIT_CHECKPOINT_LOST / CHECKPOINT_TRUSTBASE_MISMATCH) fell through to `throw e`, reaching SendModal as a re-sendable failure. Each keeps the intent OPEN and resume completes the ORIGINAL payment under the same transferId, so a fresh send() would spend a DIFFERENT source and double-pay. - #441 useIncomingPaymentRequests.pay called payPaymentRequest with no pending-code handling. payPaymentRequest routes through the same send(), and its catch REVERTS the request to 'pending' before re-throwing — so the finally-refresh re-listed the request as payable → one-click double-pay. Fix: extract ONE shared classifier (PENDING_COMMIT_CODES / isPendingCommitCode in src/sdk/errors.ts) for the possibly-committed keep-open code set, so the two paths can never drift. useTransfer returns the synthetic pending-success for any of them (CERTIFICATION_UNCONFIRMED keeps its extra 429/401 annotation). The pay path treats them as a pending SUCCESS: it swallows the throw and pins a local 'settling' override so refresh presents the request as sent-and-settling (non-payable) even though the SDK reverted it to 'pending'; the override is dropped once the SDK's own list resolves the request. Tests: new tests/unit/sdk/pendingCommitCodes.test.ts pins the exact code set + classifier; useTransfer and useIncomingPaymentRequests tests cover every code resolving to pending / non-payable. 368 tests pass, tsc + lint clean. Closes #440 Closes #441
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98074cfa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (PENDING) state until the SDK's own list resolves it (paid/expired). A ref, | ||
| // not state: refresh() reads it synchronously and it must survive re-renders | ||
| // without itself triggering one. | ||
| const settlingIdsRef = useRef<Set<string>>(new Set()); |
There was a problem hiding this comment.
When payPaymentRequest hits one of these keep-open errors and the user reloads, signs back in, or otherwise remounts WalletPanel before the SDK moves the request off pending, this hook-local Set is recreated empty. The next refresh() then maps the SDK's still-pending request back to PENDING, showing the Pay button again even though the original spend may already be on-chain, which reopens the double-pay path this change is meant to close. Store this settling marker somewhere that survives remount/session resume, or have the SDK request state reflect the settling payment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR prevents “double-pay” UI paths by treating all SDK “keep-open / possibly committed” send errors as pending (non-actionable) outcomes across both normal sends and paying incoming payment requests, using a single shared classifier to avoid drift.
Changes:
- Introduces
PENDING_COMMIT_CODES+isPendingCommitCode()insrc/sdk/errors.tsas the single source of truth for “possibly committed” keep-open send codes. - Updates
useTransferto convert all pending-commit codes to a synthetic delivery-pending success (while keeping the CERTIFICATION_UNCONFIRMED-specific 429/401 annotation). - Updates
useIncomingPaymentRequests.payto swallow pending-commit rejections and locally pin requests as non-payable (“Payment Sent”) until the SDK list resolves them, with new unit tests covering the full code set.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/sdk/pendingCommitCodes.test.ts | Pins the exact pending-commit code set and validates the classifier behavior. |
| tests/unit/hooks/useTransfer.test.tsx | Verifies E.4 keep-open codes resolve as delivery-pending success (no re-send). |
| tests/unit/hooks/useIncomingPaymentRequests.test.tsx | Verifies pay-on-request treats pending-commit codes as non-payable and the override lifecycle works. |
| src/sdk/hooks/payments/useTransfer.ts | Routes keep-open pending-commit errors through isPendingCommitCode for consistent pending-success behavior. |
| src/sdk/errors.ts | Adds the shared PENDING_COMMIT_CODES and isPendingCommitCode helper. |
| src/components/wallet/L3/hooks/useIncomingPaymentRequests.ts | Adds settling override + pending-commit swallow logic to prevent re-payable request reappearance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…LLY_COMPLETED guard; bump sdk 0.11.14 (#441/#442) With sdk 0.11.14 (#682), payPaymentRequest holds a possibly-committed pay in a DURABLE 'settling' status (survives reload) instead of reverting to 'pending'. So the app's in-memory settlingIdsRef override (which never survived reload — the Codex/Copilot P1 on this branch) is removed and replaced by mapping the SDK's 'settling' status to a non-payable state: - STATUS_MAP: 'settling' -> ACCEPTED ('Payment Sent', non-payable). Verified load-bearing (a fresh mount over a 'settling' request stays non-payable; the old override was empty on reload -> the reopened double-pay). - bridgeRequest: an UNKNOWN/unmapped status now defaults NON-payable (ACCEPTED), never PENDING. sdk.status widens to string in this repo (no compile check on the union — verified), so a future unmapped status would otherwise fall through to payable and risk a double-pay. - pay(): swallow a possibly-committed reject (no re-payable error); the SDK's durable 'settling' now handles non-payability across reload. Also: add SEND_PARTIALLY_COMPLETED (sdk #681) to PENDING_COMMIT_CODES. It was missing, so a normal send ending in PartialSendConflictError fell through useTransfer to a re-sendable failure -> double-pay of the delivered legs. Now presented as pending, never re-sendable. Dedicated load-bearing test added. tsc + lint clean, 371 tests pass.
| **Repos:** `sphere` (React app), `sphere-sdk` (client SDK, published 0.11.12), `wallet-api` | ||
| (TypeScript server). They operate as one unit. |
There was a problem hiding this comment.
Good catch — the real issue is that MAINNET-RISK-REGISTER.md should not be in this PR at all; it's a local session working doc that got swept in by a git add -A. Removed from the PR in 5c1fd57 (untracked + gitignored, kept locally). The stale version wording goes away with it.
| // #441: payPaymentRequest routes through the same send() as a normal | ||
| // transfer, so it can reject with a possibly-committed keep-open code | ||
| // (PENDING_COMMIT_CODES). On those the spend is (or may be) on-chain and | ||
| // the SDK holds the request in a DURABLE 'settling' state (non-payable, | ||
| // survives reload) until its transfer resolves — so a second pay can't | ||
| // double-pay. Swallow the throw so the modal shows no re-payable error; | ||
| // the finally→refresh reads the SDK's 'settling' status (→ ACCEPTED, | ||
| // non-payable). Any OTHER error is a genuine failure — re-throw so | ||
| // PaymentRequestModal surfaces it and the request stays actionable | ||
| // (safe to retry; nothing committed). |
There was a problem hiding this comment.
Valid — and the PR description was stale (it described the removed override). Both fixed in 5c1fd57: the pay catch now explicitly documents the CONTRACT DEPENDENCY — non-payability is the SDK's durable 'settling' status (sphere-sdk >= 0.11.14, exact-pinned, covered by the sdk deferred-paid tests), not this hook's job.
On reintroducing the local override as a fallback: I deliberately did not. The in-memory override was the source of the original P1 on this branch — it did NOT survive reload, so it reopened the double-pay after a refresh. The SDK's durable 'settling' (journaled, reload-safe) is the correct mechanism, and it's the exact-pinned version. A same-session-only local fallback wouldn't close the reload window against a hypothetical SDK that failed to mark 'settling' (that SDK also wouldn't journal it, so reload re-surfaces payable regardless) — so it'd add back the removed complexity for partial protection. I chose to depend on the SDK contract explicitly and document it. Happy to add a belt-and-suspenders guard if you'd prefer it — just flag it.
| // the exact set: dropping a code here silently re-opens the double-pay path. | ||
|
|
||
| describe('PENDING_COMMIT_CODES', () => { | ||
| it('is exactly the possibly-committed keep-open set (SDK PaymentsModule send-failure catch)', () => { |
There was a problem hiding this comment.
Renamed in 5c1fd57 to 'is exactly the possibly-committed, non-re-sendable set (keep-open certification codes + the partial-send outcome)' — reflects that the set now includes SEND_PARTIALLY_COMPLETED (a partial-send outcome, not a keep-open error).
…ment SDK 'settling' dependency, rename set test - Remove MAINNET-RISK-REGISTER.md from the PR (a local session working doc swept in by git add -A; now untracked + gitignored, kept locally). - useIncomingPaymentRequests: make the SDK-'settling' contract dependency explicit in the pay catch — non-payability is the SDK's durable 'settling' status (>= 0.11.14, pinned), not this hook's; documents why the in-memory override was removed (it didn't survive reload). - pendingCommitCodes test: rename from 'keep-open set' to 'possibly-committed, non-re-sendable set' since it now includes SEND_PARTIALLY_COMPLETED (a partial- send outcome, not a keep-open error).
…ards # Conflicts: # package-lock.json # package.json
Closes #440, closes #441.
App-side double-pay guards, finalized against sphere-sdk 0.11.14 (which contains the SDK-side deferred-paid fix #682).
Changes
useTransfermaps everyPENDING_COMMIT_CODESerror to a synthetic delivery-pending success (via the sharedisPendingCommitCode), so a possibly-committed send is never presented on a re-sendable confirm screen.payPaymentRequestroutes throughsend(), so it can throw a possibly-committed code. Rather than an in-memory override (an earlier iteration usedsettlingIdsRef, which did not survive reload — the reopened double-pay Codex/Copilot flagged), the app now consumes the SDK's durablesettlingstatus (sphere-sdk 0.11.14 marks the requestsettlingbefore throwing; it survives reload via the SDK journal):STATUS_MAP:settling → ACCEPTED("Payment Sent", non-payable).bridgeRequest: an unknown/unmapped status now defaults non-payable (neverPENDING).sdk.statuswidens tostringin this repo (no compile-time union check — verified), so a future unmapped status would otherwise fall through to payable and risk a double-pay.pay(): swallows a possibly-committed reject (no re-payable error); non-payability is the SDK's durablesettlingstatus, documented as an explicit contract dependency in-code.settlingIdsRefoverride is removed.PENDING_COMMIT_CODES. It was missing, so a normal send ending inPartialSendConflictErrorfell throughuseTransferto a re-sendable failure → double-pay of the delivered legs. Now presented as pending, never re-sendable.Verification
tsc --noEmitclean, ESLint 0 errors, full suite 371 tests pass.settlingrequest stays non-payable across a remount/reload; aSEND_PARTIALLY_COMPLETEDsend resolves delivery-pending (not re-sendable).Note: this depends on the SDK's
settlingcontract (sphere-sdk >= 0.11.14, exact-pinned). Not verified via a full live e2e drive — verification is load-bearing unit tests + typecheck.