feat(bond): maker anti-abuse bond on order creation (Phase 5)#608
Conversation
- handle pay-bond-invoice in AddOrderNotifier so a bond-enabled node prompts the maker for the deposit instead of timing out the create flow - add SessionNotifier.registerSessionInMemory to keep the unconfirmed order ephemeral (no disk persist) while reachable by orderId for the pay-bond screen - cancel the 10s create timeout on the bond message and keep the notifier alive so the post-bond new-order ack still confirms via _confirmOrder - no change to bond-disabled nodes: the new-order path is untouched
- add a transient Session.bondPending flag (never serialized) set while a maker order awaits its bond - skip saveSession in the shared pay-bond handler when bondPending, so the screen's OrderNotifier cannot persist the uncommitted order - clear the flag and persist for real in _confirmOrder once the bond locks and the order is published - an abandoned maker bond now leaves nothing on disk and never reappears after a restart
- the daemon rejects an explicit cancel while an order sits at WaitingMakerBond, so cancelOrder now detects the uncommitted maker bond (session.bondPending) and tears down the local session and stored pay-bond message instead of sending a cancel that would be refused - the bond hold invoice expires server-side and the stranded order is deleted, so no HTLC or ghost order is left behind - taker bonds and published orders keep the existing cancel flow unchanged
- session_bond_pending_test: bondPending defaults to false, is absent from toJson, and is never restored by fromJson — locks the ephemerality invariant so an abandoned order cannot survive a restart - order_state_maker_bond_test: pending → pay-bond-invoice captures the bolt11 and moves to the bond-waiting state, then the post-bond new-order confirms back to pending
- add bondExplanationMaker string (en/es/it/de/fr) warning the maker to keep the screen open and pay, or the order won't be created - pay-bond screen shows the maker copy when session.bondPending is set, taker copy otherwise - regenerate localizations
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a transient Session.bondPending flag, an in-memory session registration API, conditional session persistence and cancellation behavior for maker bond flows, payBondInvoice event handling and navigation, UI selection for maker/taker explanations, localization updates across locales, and tests validating transient session and order-state transitions. ChangesMaker anti-abuse bond flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/features/order/models/order_state_maker_bond_test.dart (2)
11-11: ⚡ Quick winUpdate comment to match actual enum value.
The comment mentions "WaitingMakerBond" but the test checks for
Status.waitingTakerBond(line 56). Either update the comment to reference the actual enum value or explain why the status is named "waitingTakerBond" when it's used for maker bonds.📝 Suggested clarification
- // The daemon parks the order at WaitingMakerBond and asks the maker to pay a + // The daemon parks the order at Status.waitingTakerBond and asks the maker to pay a // bond first; the bolt11 arrives as a PaymentRequest with the bond amount.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/order/models/order_state_maker_bond_test.dart` at line 11, Update the misleading comment so it matches the enum used in the test: replace "WaitingMakerBond" with "waitingTakerBond" or otherwise explain why Status.waitingTakerBond is used for maker bonds; reference the test name/order_state_maker_bond_test.dart and the enum symbol Status.waitingTakerBond (and the comment text near the daemon description) so the comment accurately reflects the status value being asserted in the test.
38-38: ⚡ Quick winClarify why
publishedAckusesamount: 0in maker bond flow (test/features/order/models/order_state_maker_bond_test.dart:38)
publishedAck(Action.newOrder) setsOrder.amountto0, even though the earlierbondInvoice(Action.payBondInvoice) usesamount: 300. This matches the protocol examples fornew-orderconfirmations indocs/architecture/ORDER_CREATION_PROCESS.md(which also showamount: 0). Add a short test comment next topublishedAckexplaining thatnew-orderacks intentionally carryamount: 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/order/models/order_state_maker_bond_test.dart` at line 38, Add a short inline test comment next to the publishedAck variable (the Action.newOrder instance) explaining that Order.amount is intentionally set to 0 for new-order acknowledgements as per the protocol examples; reference the bondInvoice (Action.payBondInvoice) using amount: 300 to contrast the values and mention that this matches docs/architecture/ORDER_CREATION_PROCESS.md showing new-order confirmations carry amount: 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/l10n/intl_de.arb`:
- Line 709: Replace the English value for the ARB key "bondExplanationMaker"
with a German translation so German users see localized copy; update the value
of bondExplanationMaker in lib/l10n/intl_de.arb to something like: "Bevor Ihre
Bestellung erstellt werden kann, müssen Sie eine Anti-Missbrauchs-Kaution
zahlen. Lassen Sie diesen Bildschirm geöffnet, bis die Zahlung abgeschlossen ist
– wenn Sie ihn schließen oder nicht zahlen, wird die Bestellung nicht erstellt.
So funktioniert’s:\n\n• Ihre Sats werden in Ihrer Wallet gehalten, sie werden
nicht ausgegeben.\n• Wenn der Austausch erfolgreich abgeschlossen wird, erhalten
Sie die Kaution automatisch zurück.\n• Wenn es einen Streitfall zu dieser
Bestellung gibt und Sie den Streit verlieren, verlieren Sie die Kaution.\n•
Dieser Mechanismus schützt alle Nutzer vor Betrügern." and ensure proper
escaping/formatting for ARB newlines and punctuation.
---
Nitpick comments:
In `@test/features/order/models/order_state_maker_bond_test.dart`:
- Line 11: Update the misleading comment so it matches the enum used in the
test: replace "WaitingMakerBond" with "waitingTakerBond" or otherwise explain
why Status.waitingTakerBond is used for maker bonds; reference the test
name/order_state_maker_bond_test.dart and the enum symbol
Status.waitingTakerBond (and the comment text near the daemon description) so
the comment accurately reflects the status value being asserted in the test.
- Line 38: Add a short inline test comment next to the publishedAck variable
(the Action.newOrder instance) explaining that Order.amount is intentionally set
to 0 for new-order acknowledgements as per the protocol examples; reference the
bondInvoice (Action.payBondInvoice) using amount: 300 to contrast the values and
mention that this matches docs/architecture/ORDER_CREATION_PROCESS.md showing
new-order confirmations carry amount: 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d97ef52b-0977-4d48-9078-3ae7be81fff5
📒 Files selected for processing (13)
lib/data/models/session.dartlib/features/order/notifiers/abstract_mostro_notifier.dartlib/features/order/notifiers/add_order_notifier.dartlib/features/order/notifiers/order_notifier.dartlib/features/order/screens/pay_bond_invoice_screen.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arblib/shared/notifiers/session_notifier.darttest/data/models/session_bond_pending_test.darttest/features/order/models/order_state_maker_bond_test.dart
There was a problem hiding this comment.
I like the direction here, but I do see one remaining correctness issue that I would block on before merge.
Blocking issue:
- In the maker-bond abandon path,
OrderNotifier.cancelOrder()now deletes the local session and stored pay-bond messages whensession.bondPending == true, but it does not also cancel the outstanding create-order timeout cleanup that was originally started for this requestId. - This PR already correctly cancels that timeout when
payBondInvoicearrives in_handleMakerBondInvoice(), which means the timeout is still considered part of the maker-bond flow lifecycle. If the user abandons locally while that timer is still armed, it can still fire later against a session/flow that you already tore down. At best that is redundant cleanup; at worst it can interfere with a new flow reusing nearby state and make the behavior timing-dependent.
I think the local-abandon branch should explicitly cancel the same create-timeout/session-timeout machinery before deleting messages/session, so the maker-bond flow is fully torn down in one place.
The rest of the approach makes sense to me: keeping the pre-confirmation maker order ephemeral is the right model, bondPending being transient is good, and the localized maker copy is useful. But I would still request changes until the timeout lifecycle is closed cleanly on manual abandon as well.
- add @VisibleForTesting AbstractMostroNotifier.hasRequestTimeout - regression test: cancelSessionTimeoutCleanupForRequestId disarms the request timer and a repeated cancel is a safe no-op - documents that the create timeout is torn down when pay-bond-invoice is handled, before the abandon path is reachable
|
Added a regression test (maker_bond_timeout_test.dart) for this. I don't think the issue is reachable: the create-order timeout is cancelled on the first line of _handleMakerBondInvoice, the same method that sets bondPending and navigates to the pay-bond screen. Since the abandon branch in cancelOrder() is only reachable from that screen, the timer is always already disarmed before abandon runs — the two states can't coexist. Also, OrderNotifier doesn't hold the requestId (it's keyed by orderId), so cancelling that timer from the abandon branch isn't even possible, nor needed. The test confirms the mechanism it relies on: cancelSessionTimeoutCleanupForRequestId disarms the timer and a second cancel is a safe no-op. |
There was a problem hiding this comment.
I re-checked the new commit and your explanation, and I agree the blocker I raised does not hold under the actual state flow here.
What changed my mind:
bondPendingis only set inside_handleMakerBondInvoice().- That same method cancels the create-order timeout on its first line, before it marks the session as bond-pending and before navigation reaches the pay-bond screen.
- The local-abandon branch in
OrderNotifier.cancelOrder()is only reachable from that post-payBondInvoicestate, so the supposedly outstanding create timeout andbondPending == truedo not actually coexist. - The new regression test documenting that
cancelSessionTimeoutCleanupForRequestId()disarms the timer and that a second cancel is a safe no-op is a good lock on that invariant.
So I’m retracting the timeout-lifecycle blocker.
I re-checked the rest of the PR and still think the main model is sound: the unconfirmed maker order stays ephemeral, bondPending is transient and never serialized, the maker-specific bond copy is useful, and the local abandon path avoids sending a cancel the daemon would reject.
Approved on the current revision.
Implements the mobile side of anti-abuse bond Phase 5: when a node has the bond enabled for makers (
apply_to = make | both), creating an order now requires paying a bond before the order is published.What this does
AddOrderNotifiernow handlespay-bond-invoiceduring creation: cancels the create timeout, shows the pay-bondscreen, and stays alive so the post-bond
new-orderack still confirms via_confirmOrder.registerSessionInMemory) and atransient, non-serialized
Session.bondPendingflag keeps the shared pay-bond handler from persisting it. Anabandoned order leaves nothing on disk and never reappears after a restart; it is also never shown in My Trades (no
published NIP-33 event).
cancelthe daemon rejects duringWaitingMakerBond; the bond hold invoice expires server-side. Taker bonds andpublished orders keep the existing cancel flow.
How to test manually
Requires a Mostro node with the anti-abuse bond enabled for makers (
[anti_abuse_bond] enabled = true,apply_to = makeorboth). The info event should showbond_enabled: true.Bond-enabled node (maker)
cancelis sent to Mostro (the bond invoice just expires server-side ~5 min later).not in My Trades.
Nothing is waiting and nothing shows in My Trades; you must create the order again.
Bond-disabled node (regression)
apply_to = take). Create an order → it publishes immediately withno pay-bond screen, exactly as before.
Taker (regression)
Nodes without the bond are unaffected: the
new-orderpath is untouched, so creation behaves exactly as before.Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests