Skip to content

fix(payin): fail closed on ambiguous non-EVM pay-in sends#4263

Merged
TaprootFreak merged 2 commits into
developfrom
fix/payin-nonevm-send-fail-closed
Jul 18, 2026
Merged

fix(payin): fail closed on ambiguous non-EVM pay-in sends#4263
TaprootFreak merged 2 commits into
developfrom
fix/payin-nonevm-send-fail-closed

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

What

Ports the fail-closed designate-before-broadcast boundary from #4249 (EVM) to all non-EVM pay-in send strategies (Bitcoin/Firo, Monero, Zano, Cardano, Lightning, ICP, Solana, Tron), completing the non-EVM part of #4192.

Why

The non-EVM send loops dispatched the transfer and only afterwards persisted the result; their catch block only logged. Any failure at or after the broadcast — an ambiguous TxBroadcastError, or a persistence failure once the transaction was already in flight — left the pay-in in a re-selectable status, so the minute cron re-broadcast a brand-new transfer: a double payment of customer funds from the pooled hot wallet. The Lightning return path was the worst case: a completion lookup running after the broadcast could throw and mask a completed send, and each retry fetches a fresh invoice that LND dedup cannot catch.

The service-layer machinery for this — the SENDING/SEND_UNCERTAIN statuses, designateSending, the processStrandedSendingPayIns escalation, the cron-selection exclusion, getPendingPayIns, and the return guards — already landed in #4249 and is reused unchanged. No DB migration is needed (the enum values already exist; status is a nullable varchar).

How

  • New shared sendWithBroadcastBoundary helper in the base SendStrategy: persist the in-flight SENDING marker before broadcasting, then classify failures at the boundary — a plain error before a tx id is obtained is provably pre-broadcast and restores the captured status for the next cron retry; a TxBroadcastError, an empty tx id, or a persistence failure after the broadcast keeps SENDING (the cron escalates it to SEND_UNCERTAIN), never re-broadcasting.
  • The non-EVM doSend loops (bitcoin-based, Monero, Zano, Cardano, Lightning, ICP, Solana, Tron) route their send through the helper; every other line (fee estimation, verifyForwardFee, the outer catch and its No maximum fee / FeeLimitExceededException handling) is unchanged.
  • PayInLightningService.sendTransfer makes the post-broadcast completion lookup fail-safe: a lookup error is logged and the completed outTxId is still returned, so it can no longer be misclassified as pre-broadcast and re-sent with a fresh invoice.

Out of scope

EVM send strategies (already covered by #4249). No client, entity, cron query or migration is touched.

Test plan

  • New unit tests pin the helper contract (designate-before-broadcast order; TxBroadcastError / empty-id / post-broadcast-persistence-failure keep SENDING; pre-broadcast plain error restores the captured status; happy-path forward persists) and the Lightning completion fail-safe.
  • Verified on a build machine (Node 20): type-check, lint and prettier --check green; the focused jest suites pass (63/63 across the send-strategy and Lightning-service specs, including the untouched EVM specs as a regression check); and the two new suites were confirmed to fail against the pre-change production code (red-proof), so they genuinely pin the new behaviour.
  • CI Build and test on the PR

…dano, Lightning)

Ports the designate-before-broadcast boundary from #4249 (EVM) to the non-EVM pay-in send strategies. A failure at or after the broadcast now keeps the pay-in in the non-reselectable SENDING status (escalated to SEND_UNCERTAIN by the cron) instead of re-broadcasting a fresh transfer on the next run, which could pay a customer twice.

Adds a shared sendWithBroadcastBoundary helper in SendStrategy used by the bitcoin-based, Monero, Zano, Cardano and Lightning send loops, and makes the Lightning completion lookup fail-safe so a post-broadcast lookup error cannot mask a completed send.

Completes the non-EVM part of #4192.
…ends

The ICP, Solana and Tron pay-in send strategies share Cardano's staged PREPARED send loop and carried the same pre-broadcast double-send window; route them through sendWithBroadcastBoundary like the other non-EVM chains. Also sort the PayInRepository import.
@TaprootFreak TaprootFreak changed the title fix(payin): fail closed on ambiguous non-EVM sends (Monero, Zano, Cardano, Lightning) fix(payin): fail closed on ambiguous non-EVM pay-in sends Jul 17, 2026
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Completed 2 review passes (conformance + logic). Pass 1 surfaced that the ICP, Solana and Tron send strategies shared the same un-guarded staged send path as the other non-EVM strategies, and flagged an import-ordering nit. Both were addressed — all eight non-EVM pay-in send strategies now route through the fail-closed boundary, and the import is sorted — and pass 2 came back clean (0 findings). Verified locally: type-check, lint, prettier and the focused jest suites pass (63/63), and the new tests fail against the pre-change code (red-proof).

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 17, 2026 23:02
@TaprootFreak
TaprootFreak merged commit 520ee85 into develop Jul 18, 2026
7 checks passed
@TaprootFreak
TaprootFreak deleted the fix/payin-nonevm-send-fail-closed branch July 18, 2026 07:23
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