feat(payments): idempotency on the two send paths that bypassed it - #494
Open
bobodread876 wants to merge 3 commits into
Open
feat(payments): idempotency on the two send paths that bypassed it#494bobodread876 wants to merge 3 commits into
bobodread876 wants to merge 3 commits into
Conversation
ENG-533 discovery: the ENG-530 exactly-once wrapper covers the four send functions in @app/payments — but two mutations never call those functions. lnNoAmountUsdInvoicePaymentSend and lnurlPaymentSend are FLASH-FORK resolvers that execute IBEX directly, so they had no idempotencyKey input, no lock, no dedupe. A double-fire on the most common USD send path double-paid, exactly the 2026-07-23 incident class (intraLedgerPaymentSend fired twice ~1.5s apart, $280 debited for a $140 send). Both resolvers now accept the same optional idempotencyKey and route ONLY the money-moving IBEX call through withPaymentIdempotency: - scoped to the ROUTED wallet (the one actually debited), so the same key behaves identically across the cash-wallet compat redirect - fingerprinted on the REQUEST AS SENT (invoice/lnurl + input amount). Deliberately not amountMsat on the lnurl path: the msat figure moves with the dealer rate, and a legitimate same-key retry must not be rejected as a different payment because the price ticked - decode, metadata fetch, routing and amount conversion stay OUTSIDE execute(), so a cached replay touches neither IBEX nor the lnurl server - no key = passthrough, existing clients unaffected Onchain send mutations audited too: their resolvers are stubbed (onchain moved client-side to Breez), so there is nothing to cover there. Tests pin the WIRING in both resolver specs — key, wallet scope, fingerprint shape, cached-replay-skips-IBEX, wrapper-error mapping, absent-key passthrough — while the wrapper's own dedupe/lock behavior stays covered by app/payments/idempotency.spec.ts. The wrapper is mocked in resolver specs because it constructs Redis/Lock clients at import. SDL + supergraph regenerated. 205 suites / 2228 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…e lnurl server Review fixes on PR #494: - Move decodeLnurl, the metadata fetch, the msat conversion and validateLnurlPayAmountMsat inside withPaymentIdempotency's execute(). They previously ran on every replay, so a cache hit was unreachable unless the flaky lnurl server (or the dealer rate) cooperated on the retry — the retry could report failure while the cached success sat unreachable, prompting a fresh-key re-send: the double-pay class this PR exists to close. The fingerprint only needs lnurl + amount + routed wallet, all available before the wrapper. - Catch axios.get rejection in the metadata fetch and return InvalidLnurlError so it maps to the typed failed payload instead of escaping the redlock callback as a bare GraphQL error. - Strengthen the cached-replay spec to assert decodeLnurl and axios.get are also untouched (it previously only checked payToLnurl, certifying a property the code did not have). - Add the wrapper-error → failed payload and absent-key passthrough specs the PR body claimed, mirroring the noamount spec. - Add a spec for the metadata-fetch rejection path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…pper; pin exact fingerprint
- usdWalletAmountFromWalletId (a Mongo read + three failure branches) ran
after routing but outside execute(), contradicting the comment claiming
everything after routing sits inside the wrapper. Moved it into execute()
before the msat conversion — its errors are ApplicationErrors the wrapper
never caches, so behavior is preserved — and the cached-replay spec now
asserts it is never called on a replay.
- Replaced the weak fingerprint assertions (toMatch(/^lnurl|/) +
not.toContain("Msat"), which could not fail under the regression they
guard) with the exact string match, matching the sibling noamount spec.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
islandbitcoin
approved these changes
Aug 25, 2026
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
2 participants
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.
Backend half of ENG-533, and a discovery that upgrades it from "attach a key" to "close a live double-pay hole."
The audit
Every payment input in the schema, checked for the key:
@app/payments(ENG-530 wrapper)Ibex.payInvoicedirectlyIbex.payToLnurldirectlyThe two fork resolvers never call the wrapped functions, so ENG-530's exactly-once machinery never ran on the most common USD send paths. A double-fire there today double-pays — the exact 2026-07-23 incident class ($140 fired twice, $280 debited).
The change
Both resolvers accept the same optional
idempotencyKeyand route only the money-moving IBEX call throughwithPaymentIdempotency:amountMsaton the LNURL path: that figure moves with the dealer rate, and a legitimate same-key retry must not be rejected as a different payment because the price ticked.execute()— a cached replay touches neither IBEX nor the lnurl server.Tests
Resolver specs pin the wiring — key, wallet scope, fingerprint shape, cached-replay-skips-IBEX, wrapper-error mapping (
IdempotencyKeyReuseError→ failed payload), absent-key passthrough. The wrapper's own dedupe/lock/fingerprint behavior stays covered byapp/payments/idempotency.spec.ts; it's mocked in resolver specs because it constructs Redis/Lock clients at import.205 suites / 2228 tests green; SDL + supergraph regenerated; tsc + eslint clean.
What follows
The mobile half (in-flight button disable + key generation, reusing the key on retry) lands as a flash-mobile PR against these inputs — it needs this schema in the checked-in SDL first.