fix(restore): show new-invoice prompt for failed payout in settled-hold-invoice#623
fix(restore): show new-invoice prompt for failed payout in settled-hold-invoice#623AndreaDiazCorreia wants to merge 1 commit into
Conversation
…old-invoice buyers
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR fixes a session-restore regression for buyers in the ChangesFailed Payout Restore Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 498ec58e05
ℹ️ 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".
| id: orderDetail.id, | ||
| action: replayAction, | ||
| payload: order, | ||
| timestamp: baseTimestamp + i, |
There was a problem hiding this comment.
When historical messages were saved during the 10-second restore window, these synthetic payment-failed/add-invoice messages are stored with the order creation time, so the next OrderNotifier.sync() (which replays storage sorted by timestamp) can replay older historical actions like released after the failed-payout pair and move the buyer back to settled-hold-invoice. This makes the fix non-persistent across provider recreation/app restart; the replayed actions need timestamps after the restored history, e.g. after the max stored timestamp.
Useful? React with 👍 / 👎.
| for (var i = 0; i < sorted.length; i++) { | ||
| final action = sorted[i].action; | ||
| if (action == Action.paymentFailed) return true; | ||
| if (action == Action.addInvoice && i > releaseIndex) return true; |
There was a problem hiding this comment.
With releaseIndex initialized to -1, any lone add-invoice satisfies i > releaseIndex, even though add-invoice is also the normal early buyer-invoice request. During restore the code explicitly waits for historical messages but relays can deliver only that early event before the later release within the fixed window, so a happy-path settled-hold-invoice buyer can be restored as a failed payout. Treat add-invoice as a failed-payout signal only when a release/settle marker was actually seen before it, or require payment-failed for the single-message case.
Useful? React with 👍 / 👎.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the current head and I do see one real blocking issue before merge.
Blocking issue:
restoreHasFailedPayoutSignal([addInvoice])currently returnstrue, treating a loneadd-invoiceas a definitive failed-payout signal.- But
add-invoiceis not unique to the failed-payout substate; it also exists in the normal buyer flow earlier in the trade. - The helper comments themselves acknowledge that
add-invoiceis only diagnostic when it appears after hold release/settlement, yet the implementation special-cases the storage-cleared restore path by making a bareadd-invoiceenough to force the failed-payout rebuild.
Why this matters:
- Restore logic is reconstructing durable state, so ambiguous evidence should not be upgraded into a deterministic substate.
- A partial or incomplete message history that happens to contain only
add-invoicewould now push the order intopayment-failed+add-invoice, even though that same single action can also belong to the normal happy-path buyer flow. - In other words, the heuristic is currently biased toward the failed-payout interpretation in a case where the evidence is not actually unique.
What I would want:
- Only treat
payment-failedas definitive on its own. - Treat
add-invoiceas a failed-payout signal only when it is ordered after a release/settled marker or when some other unambiguous failed-payout evidence exists. - If the restore path truly relies on storage being empty as an invariant, I would want that invariant enforced much more explicitly than “a lone
add-invoicemeans failed payout”.
Because this can reconstruct the wrong buyer substate from ambiguous history, I think it is a real blocker for merge.
1 participant
Closes #615
When Mostro's Lightning payout to the buyer fails, the order stays at
settled-hold-invoiceand the daemon re-sendsadd-invoice. On restore, the state was rebuilt purely from the snapshot status, regressing this substate into the "released / paying sats" screen instead of the new-invoice prompt.settled-hold-invoiceis overloaded for the buyer — only the action distinguishes "sats in flight" from "payout failed". This change detects the failed-payout substate from the re-sentpayment-failed/add-invoicemessages and replays them so the order lands onpayment-failed+add-invoice(the manual invoice prompt), matching the pre-restore state.Changes
restoreHasFailedPayoutSignal()to detect the failed-payout substate (treatspayment-failedas definitive;add-invoiceonly when it follows the release).settled-hold-invoice+ buyer by replayingpayment-failedthenadd-invoice.Acceptance criteria
settled-hold-invoicewith no failed payout) still shows "released / paying sats".settled-hold-invoice+payment-failed/add-invoicein storage → final statepayment-failed.Verification
fvm flutter analyze: no issuesfvm flutter test: all tests passingSummary by CodeRabbit
Bug Fixes
Tests