Skip to content

fix: return already-completed send without re-paying on the node [AMB-2985] - #39

Merged
bufo24 merged 1 commit into
mainfrom
jesseva/amb-2985-short-circuit-completed-send
Aug 18, 2026
Merged

fix: return already-completed send without re-paying on the node [AMB-2985]#39
bufo24 merged 1 commit into
mainfrom
jesseva/amb-2985-short-circuit-completed-send

Conversation

@bufo24

@bufo24 bufo24 commented Aug 18, 2026

Copy link
Copy Markdown
Member

Summary

Transactions.send() now checks the transaction create_send returns: if its status is already COMPLETED, the SDK short-circuits and resolves the send as a success instead of executing the node payment.

This covers two cases where the backend returns an existing settled transaction instead of creating a new one:

  • a genuine duplicate send (same payment hash, already paid)
  • an idempotency-key replay

Without this change, the SDK would take that already-settled transaction and try to pay the invoice again on the node, which fails.

Return shape

{
  transaction, // the existing COMPLETED transaction, unchanged
  payment: {
    status: 'SUCCEEDED',
    paymentHash: transaction.payment_hash,
    feeSat: transaction.fee,       // already sats today — see note below
    paymentPreimage: undefined,    // see note below
  },
}
  • paymentPreimage is always undefined on this path. Checked packages/core/schema/rails.graphqlPaymentsTransaction has no preimage field (there's an unmerged amboss-nodify branch, feat/AMB-2698-paymentstransaction-preimage-adjacent work, that would add one, but it isn't live). Only a payment actually executed on the node (the existing path below the new check) can return a preimage.
  • feeSat maps directly from transaction.fee with no numeric conversion — both are sats. Verified against amboss-nodify's src/modules/lnd/handlers/offchain.ts on main, which writes payments_transaction.fee from LND's safe_fee (sats).
    • This is a contract, not a coincidence. An earlier attempt to switch that column to msat (amboss-nodify#826) was closed unmerged — it would have broken amboss-cron's routing-fee reimbursement math by 1000x. It has since been decided that the SDK will not handle msat: NodePaymentResult.feeSat stays sats-only and will never carry a /1000 conversion, so whatever the ledger stores underneath must still present sats to the SDK. Tracked in AMB-2990.

Sequencing (important)

This PR must merge and be released before the sibling amboss-rails-api PR (fix: return completed transaction instead of throwing on duplicate payment hash [AMB-2985]).

If the rails-api change lands first while consumers are still on an older SDK version, the SDK will receive the completed transaction from create_send and attempt to re-pay it on the node — producing a confusing node-side error where users previously got a clean "already paid" style result. Ship this SDK change and let it propagate to consumers first.

Test plan

  • pnpm run typecheck
  • pnpm run build
  • pnpm run test (36/36 passing, including 2 new cases)
    • COMPLETED transaction from create_send → resolves success without invoking the node
    • non-COMPLETED transaction → still executes the node payment exactly as before (regression check)
  • pnpm run format:check
  • Sandbox path is untouched (existing test unchanged, still passing)

🤖 Generated with Claude Code

…-2985]

When create_send returns an existing COMPLETED transaction (a duplicate
payment hash or an idempotency-key replay), short-circuit and resolve
the send as a success instead of executing the node payment again,
which would otherwise fail or double-pay.

Depends on amboss-rails-api PR: fix: return completed transaction
instead of throwing on duplicate payment hash [AMB-2985]. Must merge
and release before that PR lands, or old SDK consumers will re-pay a
settled invoice on the node once the backend starts returning it
instead of throwing.
@bufo24
bufo24 force-pushed the jesseva/amb-2985-short-circuit-completed-send branch from f5826f3 to 4a9ddd1 Compare August 18, 2026 11:41
@bufo24
bufo24 marked this pull request as ready for review August 18, 2026 11:41
@bufo24
bufo24 merged commit 55e637e into main Aug 18, 2026
2 checks passed
@apotdevin apotdevin mentioned this pull request Aug 18, 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.

2 participants