feat(bond): recognize pay-bond-invoice and add taker bond payment flow - #213
Open
AndreaDiazCorreia wants to merge 29 commits into
Open
feat(bond): recognize pay-bond-invoice and add taker bond payment flow#213AndreaDiazCorreia wants to merge 29 commits into
AndreaDiazCorreia wants to merge 29 commits into
Conversation
Add PayBondInvoiceScreen to handle taker bond payment when taking orders on bond-requiring nodes. The daemon replies with pay-bond-invoice instead of progressing the trade, placing the order at waitingTakerBond status. The screen displays the bond hold invoice as QR code with copy/share/lightning-URI actions, supports NWC auto-payment with manual fallback, and watches order status to forward the taker to the next step (add-invoice for buyers, pay-invoice for sellers, or trade-detail)
Move taker anti-abuse bond hold invoice from `holdInvoice` to dedicated `bondInvoice` field in TradeInfo to prevent collision with escrow hold invoice when trade advances past bond payment. Update classify_take_reply to use action (not payload status) as authoritative for pay-bond-invoice replies since daemon stamps embedded SmallOrder with wire-mapped Pending.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds end-to-end taker bond invoice support: Rust recognizes and persists bond invoices, Flutter receives the new status, routes takers to a payment screen, supports multiple Lightning payment methods, updates trade filtering and countdowns, handles bond cancellation, and adds localized UI strings. ChangesTaker bond invoice flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TakeOrderScreen
participant OrdersAPI
participant PayBondInvoiceScreen
participant LightningWallet
TakeOrderScreen->>OrdersAPI: takeOrder
OrdersAPI-->>TakeOrderScreen: WaitingTakerBond and bond invoice
TakeOrderScreen->>PayBondInvoiceScreen: Open payment route
PayBondInvoiceScreen->>LightningWallet: Pay invoice
OrdersAPI-->>PayBondInvoiceScreen: Trade status updates
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 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: 315412b9fa
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rust/src/api/orders.rs (1)
1931-1996: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCorrect, but lacks dedicated unit coverage.
The amount-extraction/status-sync logic here is new business logic (mirrors what
classify_take_replydoes) but, unlike that function, isn't exercised by any unit test inmod tests— it can only be verified indirectly throughdispatch_mostro_message, which requires heavy nostr/gift-wrap scaffolding to test. Consider extracting the small "deriveamountfrom theOrderpayload" step into a pure helper (similar tostatus_for_action/map_core_status) so it can be unit-tested directly.🤖 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 `@rust/src/api/orders.rs` around lines 1931 - 1996, Extract the AddInvoice amount derivation from the Action::AddInvoice arm into a small pure helper that accepts the payload and returns the optional u64 amount, preserving the existing positive-amount and non-Order behavior. Add focused unit tests in mod tests covering valid, non-positive, missing, and non-Order payloads, then reuse the helper in the status-sync flow.
🤖 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/features/order/screens/pay_bond_invoice_screen.dart`:
- Around line 131-167: Remove the trade amount lookup from the bond invoice
payment flow in the screen’s data builder and stop passing
trade.order.amountSats to NwcPaymentWidget or the manual payInvoiceAmount
fallback. Use the bond invoice’s own amount parsed from the invoice, or the
dedicated bond amount field if one is available in the Rust/contracts model, so
both payment paths charge the bond hold amount.
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 153-154: Update the status mapping around
OrderStatus.waitingTakerBond so it remains distinct from
OrderStatus.waitingPayment, using a dedicated TradeStatus and UI action for
taker bond payment. Ensure the trade detail screen routes this state to the
bond-payment flow rather than payInvoicePath, with appropriate bond-specific
copy and timer handling; leave ordinary waitingPayment behavior unchanged.
In `@rust/src/api/orders.rs`:
- Around line 259-273: Update the documentation for the bond invoice response
near the order reply classification to match the implementation: state that bond
replies use the separate bond_invoice field, leave hold_invoice unset, and are
distinguished by the action rather than a WaitingTakerBond payload status.
Remove the outdated claim that bond_invoice shares the hold_invoice slot or that
Dart identifies it through WaitingTakerBond.
---
Nitpick comments:
In `@rust/src/api/orders.rs`:
- Around line 1931-1996: Extract the AddInvoice amount derivation from the
Action::AddInvoice arm into a small pure helper that accepts the payload and
returns the optional u64 amount, preserving the existing positive-amount and
non-Order behavior. Add focused unit tests in mod tests covering valid,
non-positive, missing, and non-Order payloads, then reuse the helper in the
status-sync flow.
🪄 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 Plus
Run ID: 122530d1-1461-40e3-9681-449e35e6d158
📒 Files selected for processing (21)
lib/core/app_routes.dartlib/features/order/providers/trade_state_provider.dartlib/features/order/screens/pay_bond_invoice_screen.dartlib/features/order/screens/take_order_screen.dartlib/features/trades/providers/trades_providers.dartlib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartrust/src/api/orders.rsrust/src/api/types.rsrust/src/frb_generated.rstest/features/trades/filtered_trades_provider_test.dart
…ut retry Gate add-invoice state advancement on order being WaitingTakerBond to handle only the post-bond resume case. When a buyer's payout exhausts retries the daemon re-sends add-invoice while the order remains SettledHoldInvoice (sats locked in escrow). Advancing that to WaitingBuyerInvoice would misrender the trade as pre-escrow with cancellation controls.
…oc comment Update documentation to reflect that pay-bond-invoice replies route the bond bolt11 into the dedicated bond_invoice field rather than hold_invoice, preventing collision with the trade's escrow hold invoice. Clarify that Dart discriminates bond payment by WaitingTakerBond status to route takers to the bond payment screen.
…ing trade amount Add dedicated bond_amount_sats field to TradeInfo and DaemonReply::TakeAccepted to carry the bond hold invoice amount separately from the trade's amount_sats. Update classify_take_reply to route bond amounts into bond_amount_sats (never amount_sats) when action is PayBondInvoice, preventing the bond figure from overwriting the order's trade amount.
…yment Split waitingTakerBond from waitingPayment in TradeStatus enum to distinguish taker bond payment from escrow hold invoice flow. Map OrderStatus.waitingTakerBond to new TradeStatus.waitingBond state, render dedicated headline/button routing to pay-bond-invoice screen instead of pay-invoice. Add tradeStatusWaitingBond and tradeHeadlineWaitingBond localization strings across all locales.
Add WaitingTakerBond to OrderStatus enum documentation across contracts/types.md and data-model.md. Document bond_invoice and bond_amount_sats fields in TradeInfo type, clarifying they remain distinct from hold_invoice/amount_sats to prevent collision when trade advances past bond payment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/features/trades/screens/trade_detail_screen.dart`:
- Line 159: Update _getInstructionText and _timerContext to explicitly handle
TradeStatus.waitingBond, using the localized bond-payment instruction and
corresponding bond deadline context so reopened trades retain the correct
payment state.
🪄 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 Plus
Run ID: a85deed4-1dd8-4809-8727-cd2ea2eaa1bc
📒 Files selected for processing (19)
lib/features/order/screens/pay_bond_invoice_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartrust/src/api/orders.rsrust/src/api/types.rsrust/src/frb_generated.rsspecs/004-mostro-p2p-client/contracts/orders.mdspecs/004-mostro-p2p-client/contracts/types.mdspecs/004-mostro-p2p-client/data-model.md
🚧 Files skipped from review as they are similar to previous changes (11)
- lib/l10n/app_it.arb
- lib/l10n/app_de.arb
- lib/l10n/app_localizations_it.dart
- lib/l10n/app_en.arb
- lib/l10n/app_localizations_de.dart
- lib/l10n/app_localizations_es.dart
- lib/l10n/app_es.arb
- lib/l10n/app_localizations_fr.dart
- rust/src/api/types.rs
- lib/features/order/screens/pay_bond_invoice_screen.dart
- rust/src/api/orders.rs
… detail screen Add tradeInstructionWaitingBond to render bond payment instruction when status is waitingBond. Add tradeTimerWaitingBondLabel and map waitingBond to same consequence as waitingInvoice/waitingPayment (order returns to book on timeout). Add localization strings across all locales.
…tatus Add public_status_supersedes guard to prevent coarse NIP-69 buckets (Pending/InProgress) from overwriting fine-grained gift-wrap statuses (WaitingTakerBond/WaitingBuyerInvoice/WaitingPayment) in subscribe_single_order. Terminal statuses always supersede. Flip tradeStatusProvider to prefer persisted trade row over in-memory book since book carries daemon's coarse public status while trade row holds authoritative gift-wrap state.
Strip verbose explanations from bond invoice code comments and docstrings across Rust and Dart. Reduce multi-sentence rationales to concise statements while preserving essential context about bond_invoice/bond_amount_sats separation from hold_invoice/amount_sats, WaitingTakerBond routing, and wire-mapped Pending status handling.
…vances correctly Add integration test over real SQLite store confirming WaitingTakerBond resists coarse Pending/InProgress buckets from public order book sync, advances to WaitingBuyerInvoice/WaitingPayment with per-role amounts, and yields only to terminal statuses. Update tradeAmountProvider to prefer persisted trade row over order book to surface daemon's calculated per-role sats rather than coarse public 38383 figure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/features/order/providers/trade_state_provider.dart`:
- Around line 20-26: Update the polling loop in tradeStatusProvider to catch
transient exceptions from listTrades and getOrder, preserve the stream, and
continue polling using the existing retry behavior established by
tradeBondInfoProvider. Ensure successful sats results still yield and return as
before, and add targeted tests covering failures from both API calls followed by
a successful retry.
- Around line 21-26: Update the trade polling logic around the `trade` lookup so
`getOrder` is called only when no matching trade exists; when a trade row is
present with null `amountSats`, yield null and continue polling instead of using
the public order amount. Add a regression test covering that case with a
non-null public amount and verify polling does not stop.
🪄 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 Plus
Run ID: a4c8d372-a586-4389-81ba-9d5499398935
📒 Files selected for processing (17)
lib/features/order/providers/trade_state_provider.dartlib/features/order/screens/pay_bond_invoice_screen.dartlib/features/order/screens/take_order_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartrust/src/api/orders.rsrust/src/api/types.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- lib/l10n/app_fr.arb
- lib/l10n/app_it.arb
- lib/l10n/app_de.arb
- lib/l10n/app_es.arb
- lib/l10n/app_localizations_de.dart
- lib/l10n/app_localizations_es.dart
- lib/l10n/app_localizations_it.dart
- lib/l10n/app_localizations_en.dart
- rust/src/api/types.rs
- lib/l10n/app_en.arb
- lib/features/order/screens/take_order_screen.dart
- lib/features/trades/screens/trade_detail_screen.dart
- lib/l10n/app_localizations_fr.dart
- lib/features/order/screens/pay_bond_invoice_screen.dart
… and return order to book Add _confirmAndCancel method to pay_bond_invoice_screen that sends real cancel request to daemon when taker backs out before paying bond, releasing bond without slash and returning order to book instead of stranding at WaitingTakerBond. Wire cancel button to confirmation dialog and add waitingBond to cancellable statuses set in trade_detail_screen.
…ng and book handling Add cancelBondBackoutDialogContent localization string to clarify that cancelling before bond payment is unilateral and returns order to book without affecting counterparty. Update pay_bond_invoice_screen and trade_detail_screen to detect WaitingTakerBond status and render bond-specific messaging/snackbar. Modify cancel_order to classify pre-commit cancels (WaitingTakerBond/Pending) and reset book entry to Pending instead of removing it
…y_bond_invoice_screen Extract _waitingIndicator method to deduplicate CircularProgressIndicator/waitingForPaymentConfirmation text block used in both NWC and manual payment modes. Replace inline Column widgets with method call in both branches.
…nd_invoice_screen Extract _cancelButton method to deduplicate OutlinedButton/destructiveRed styling used in both NWC and manual payment modes. Replace inline SizedBox/OutlinedButton blocks with method call in both branches. Restructure NWC body from Center to Column with Spacer to position cancel button at bottom when not waiting.
Wrap tradeAmountProvider and tradeStatusProvider poll bodies in try-catch to prevent exceptions from breaking the polling loop. Log failures with debugPrint and continue polling after delay instead of terminating stream on transient API errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/src/api/orders.rs (1)
3326-3364: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert bond fields remain empty for non-bond replies.
The classification contract requires normal
PayInvoice,AddInvoice, and action-only replies to keepbond_invoiceandbond_amount_satsunset, but these test arms do not assert that invariant. Add those assertions to prevent accidental invoice-field leakage.As per coding guidelines, add targeted tests when expanding protocol handling.
🤖 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 `@rust/src/api/orders.rs` around lines 3326 - 3364, Extend the relevant `classify_take_reply` test arms for normal `PayInvoice`, `AddInvoice`, and action-only replies to assert that `bond_invoice` and `bond_amount_sats` remain unset. Update the `DaemonReply::TakeAccepted` pattern bindings as needed while preserving the existing status, amount, and hold-invoice assertions, and add targeted coverage for any action-only reply path.Source: Coding guidelines
🤖 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 `@rust/src/api/orders.rs`:
- Around line 1340-1350: Implement the IndexedDB-backed trade lookup used by the
is_precommit check and the Action::AddInvoice flow so it returns the persisted
trade and observes WaitingTakerBond. Replace .ok().flatten() with explicit error
handling that does not treat database failures as a missing trade, while
preserving the existing transition behavior for valid statuses; update both
affected paths.
---
Nitpick comments:
In `@rust/src/api/orders.rs`:
- Around line 3326-3364: Extend the relevant `classify_take_reply` test arms for
normal `PayInvoice`, `AddInvoice`, and action-only replies to assert that
`bond_invoice` and `bond_amount_sats` remain unset. Update the
`DaemonReply::TakeAccepted` pattern bindings as needed while preserving the
existing status, amount, and hold-invoice assertions, and add targeted coverage
for any action-only reply path.
🪄 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 Plus
Run ID: 63aa3ee0-7253-4cba-bfc1-9ee518451be5
📒 Files selected for processing (15)
lib/features/order/providers/trade_state_provider.dartlib/features/order/screens/pay_bond_invoice_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartrust/src/api/orders.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/l10n/app_fr.arb
- lib/l10n/app_localizations_it.dart
- lib/l10n/app_localizations_es.dart
- lib/l10n/app_localizations_de.dart
- lib/l10n/app_de.arb
- lib/l10n/app_localizations_fr.dart
- lib/l10n/app_en.arb
- lib/l10n/app_it.arb
- lib/l10n/app_localizations_en.dart
- lib/features/order/screens/pay_bond_invoice_screen.dart
- lib/features/order/providers/trade_state_provider.dart
- lib/features/trades/screens/trade_detail_screen.dart
grunch
requested changes
Jul 29, 2026
grunch
left a comment
grunch
left a comment
Member
There was a problem hiding this comment.
Requesting changes on cdd737f2129792e5d48626b08ced0db52d18db75. Three correctness blockers remain:
- The unresolved IndexedDB finding is valid: the new post-bond
AddInvoiceand pre-commit cancel gates depend onget_trade_by_order_id, but the WASM backend always returnsNone. The paid-bond flow therefore cannot advance on web. - The bond screen's implicit back paths bypass the only protocol-aware cancellation path and can strand an unpaid take at
WaitingTakerBonduntil timeout (inline). tradeAmountProviderstill falls back to the coarse public order amount when an authoritative persisted trade exists with a temporarily-null per-role amount, so it can stop polling with the wrong invoice amount (inline).
The exact-head Rust tests pass (113 passed, 6 ignored), and all GitHub checks are green, but those checks do not exercise the affected Flutter navigation/provider paths or the WASM persistence transition.
grunch
requested changes
Jul 29, 2026
grunch
left a comment
grunch
left a comment
Member
There was a problem hiding this comment.
Supplemental exact-head review after completing the full parallel audit. These are additional blockers discovered after the first review was submitted; they are consolidated here to keep the remediation checklist stable:
- The cancellation state machine conflates maker cancellation with taker bond back-out, and the later
Action::Canceledhandling reverses the intended taker book restoration (inline). - The coarse-status guard protects only the persisted trade; both public ingestion paths can still overwrite the in-memory order book with
Pending/InProgress(inline). - Payment launch and destructive cancellation can race while the Lightning outcome is unknown (inline).
- The newly presented bond countdown uses the public listing expiration rather than
TradeInfo.timeoutAt(inline).
Please also replace the mirrored bond-flow test with coverage that exercises the real dispatcher/order-book path. As a non-blocking documentation follow-up, specs/004-mostro-p2p-client/contracts/orders.md still describes bonds as unsupported and cancel_order as maker-only.
…n addition to trade row Extract followed_trade_state helper to retrieve authoritative (status, amount_sats) from persisted trade row. Add merge_followed_state to overlay trade state onto incoming public order before upserting to book cache. Apply guard in both subscribe_single_order and ingest_order_event so book cache never contradicts trade row with coarse public status downgrades.
…r taker back-out Add is_mine check to cancel_order so only taker pre-commit back-outs reset book entry to Pending; maker withdrawals and cooperative cancels drop the order. Mirror logic in dispatch_mostro_message Canceled handler to preserve non-owned orders and let public 38383 events govern them.
…creen Track bond payment lifecycle through idle/launched/confirming phases to prevent cancellation races when payment outcome is unknown. Add PopScope handler that blocks navigation during in-flight payments and presents three-way dialog (keep paying/leave/release) when bond is unpaid. Mark payment as launched on wallet open, invoice copy, share, and NWC dispatch so settling bonds cannot be cancelled.
…ade status Add _resolveDeadline helper that returns trade's timeoutAt while WaitingTakerBond, otherwise falls back to public listing expiresAt. Extract _epochSeconds to handle PlatformInt64 conversion. Add listener to tradeStatusProvider that reloads deadline when status changes so countdown reflects correct timeout window instead of stale listing expiry.
…cel_order contract docs Rewrite tradeAmountProvider comment to explain that persisted trade row is authoritative for per-role sats and order book fallback only applies when no trade is followed, preventing premature polling termination with coarse public amount. Restructure ternary to explicit if-else for clarity. Update cancel_order contract to document ownership-based book handling: taker pre-commit back-outs reset to Pending, maker withdraw
…and unify public order sync Extract classify_cancel and cancel_drops_listing helpers to encapsulate ownership-based cancel decision logic, replacing inline is_taker_backout and owned checks. Introduce CancelEffect enum to make cancel_order's book handling explicit. Extract sync_public_order_update to deduplicate d-tag event processing between subscribe_single_order and future ingest_order_event paths. Add comprehensive tests covering
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/l10n/app_en.arb (1)
179-180: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse natural status wording.
Waiting Bondis ungrammatical and unclear. UseWaiting for bondor, preferably,Bond payment requiredto make the required action explicit.🤖 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 `@lib/l10n/app_en.arb` around lines 179 - 180, Update the tradeStatusWaitingBond localization value to use clear, natural status wording, preferably “Bond payment required,” while preserving its existing description metadata.
🧹 Nitpick comments (2)
test/features/order/screens/pay_bond_invoice_screen_test.dart (1)
198-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for tapping the NWC pay button.
This group verifies the idle-state render and back-dialog behavior, but nothing taps
payWithWalletButtonto exerciseonPaymentStarted/onPaymentSuccess/onFallbackToManual. Adding this (once the underlying bug flagged inpay_bond_invoice_screen.dartis fixed) would have caught the widget being disposed before the payment outcome resolves. As per coding guidelines, add targeted tests when expanding complex logic, asynchronous workflows, or protocol handling.🤖 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/screens/pay_bond_invoice_screen_test.dart` around lines 198 - 215, Add a widget test in the “PayBondInvoiceScreen with an NWC wallet connected” group that taps l10n.payWithWalletButton and awaits the payment outcome, covering onPaymentStarted, onPaymentSuccess, and onFallbackToManual without disposing the screen prematurely. Reuse _pumpBondScreen and _settle, and assert the resulting UI for the resolved payment or manual fallback path.Source: Coding guidelines
lib/features/order/providers/trade_state_provider.dart (1)
63-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wintradeStatusProvider's
getOrderfallback bypasses the new bridge seam.Line 68 still calls
orders_api.getOrder(orderId: orderId)directly, whiletradeAmountProviderabove was refactored to go throughref.read(bridgeGetOrderProvider). This is the exact seam this PR introduced specifically so bridge calls are substitutable "since the bridge is uninitialised underflutter test" (per the doc comment on lines 6-8) — leaving this call untouched means the getOrder-fallback branch oftradeStatusProvidercan't be unit-tested the same way, and it's an easy-to-miss inconsistency for future readers.♻️ Proposed fix
- final info = await orders_api.getOrder(orderId: orderId); + final info = await getOrder(orderId);(assuming
getOrderis bound fromref.read(bridgeGetOrderProvider)the same way as intradeAmountProvider)🤖 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 `@lib/features/order/providers/trade_state_provider.dart` around lines 63 - 72, Update the getOrder fallback in tradeStatusProvider to obtain and invoke the bridge-backed callable from ref.read(bridgeGetOrderProvider), matching tradeAmountProvider. Remove the direct orders_api.getOrder call while preserving the existing null check, status yield, and terminal-status return behavior.
🤖 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/features/order/screens/pay_bond_invoice_screen.dart`:
- Around line 188-204: The _PaymentPhase.confirming branch in _footer needs a
recovery action instead of rendering only _waitingIndicator. Provide the same
cancel or “not paid yet” escape available during _PaymentPhase.launched, and
update _handleBackIntent/PopScope handling as needed so confirming users can
exit or recover when daemon confirmation never arrives, while preserving the
waiting indicator.
In `@lib/l10n/app_localizations_en.dart`:
- Around line 245-255: Update the leave-bond-payment localization text so the
action clearly returns the order to the book rather than releasing the bond: in
lib/l10n/app_localizations_en.dart lines 245-255, replace “release it now” with
explicit order-return wording; apply the equivalent order/commande wording in
lib/l10n/app_localizations_fr.dart lines 251-261 and order/ordine wording in
lib/l10n/app_localizations_it.dart lines 249-259. Keep the button labels and
other text unchanged.
---
Outside diff comments:
In `@lib/l10n/app_en.arb`:
- Around line 179-180: Update the tradeStatusWaitingBond localization value to
use clear, natural status wording, preferably “Bond payment required,” while
preserving its existing description metadata.
---
Nitpick comments:
In `@lib/features/order/providers/trade_state_provider.dart`:
- Around line 63-72: Update the getOrder fallback in tradeStatusProvider to
obtain and invoke the bridge-backed callable from
ref.read(bridgeGetOrderProvider), matching tradeAmountProvider. Remove the
direct orders_api.getOrder call while preserving the existing null check, status
yield, and terminal-status return behavior.
In `@test/features/order/screens/pay_bond_invoice_screen_test.dart`:
- Around line 198-215: Add a widget test in the “PayBondInvoiceScreen with an
NWC wallet connected” group that taps l10n.payWithWalletButton and awaits the
payment outcome, covering onPaymentStarted, onPaymentSuccess, and
onFallbackToManual without disposing the screen prematurely. Reuse
_pumpBondScreen and _settle, and assert the resulting UI for the resolved
payment or manual fallback path.
🪄 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 Plus
Run ID: 66024996-b136-4753-b4dc-fa3aa59902ac
📒 Files selected for processing (21)
lib/features/order/providers/trade_state_provider.dartlib/features/order/screens/pay_bond_invoice_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartlib/shared/widgets/nwc_payment_widget.dartrust/src/api/orders.rsspecs/004-mostro-p2p-client/contracts/orders.mdtest/features/order/providers/trade_amount_provider_test.darttest/features/order/screens/pay_bond_invoice_screen_test.darttest/features/trades/trade_detail_countdown_test.darttest/support/fake_trades.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/l10n/app_it.arb
- lib/l10n/app_es.arb
- lib/l10n/app_localizations_es.dart
- specs/004-mostro-p2p-client/contracts/orders.md
…utcome is unknown
… leave-bond dialog copy Replace direct orders_api calls with injected bridgeListTradesProvider and bridgeGetOrderProvider to enable widget testing. Add tests verifying tradeStatusProvider prefers trade row status over coarse public bucket and falls back to book when no trade exists. Update leaveBondPaymentContent and releaseOrderButton strings across all locales to clarify that releasing "puts the order back on the book" / "returns
Add bond_invoice, bond_amount_sats fields to Rust TradeRow test fixture and rating, totalReviews, daysActive fields to Dart OrderInfo test fixture to match updated data structures.
4 tasks
Catrya
requested changes
Jul 31, 2026
Catrya
left a comment
Catrya
left a comment
Member
There was a problem hiding this comment.
When an order is in the waiting-buyer-invoice or waiting-payment status and the taker doesn't respond, the order returns to the Pending status. However, the maker's app doesn't update to Pending, it stays on the previous status. This doesn't happen on main.
Steps to reproduce:
- User A creates a sell order.
- User B takes the order and pays the bond.
- Mostro asks User B to provide a Lightning invoice.
- User A sees the order in the
waiting-buyer-invoicestatus. - User B does not provide the invoice and lets the timeout expire.
- The order returns to the
Pendingstatus, but User A continues to seewaiting-buyer-invoiceinstead ofPending.
The daemon returns an unanswered WaitingBuyerInvoice or WaitingPayment to the book as Pending; the maker must follow instead of staying stranded on the stale fine-grained status. public_status_supersedes now rejects only the bucket that already corresponds to what we hold (no information), so every other bucket — including a return to Pending — is a real transition.
Catrya
reviewed
Jul 31, 2026
Catrya
left a comment
Catrya
left a comment
Member
There was a problem hiding this comment.
It’s weird because sometimes it works fine and other times it doesn't. In these cases, all the orders should appear as "pending" since the timeout is 3 minutes and the new "pending" message has already been sent, but not all of them are updating.
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
3 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.
Closes #208. Part of #145 (anti-abuse bond).
Summary
Phase 1 (taker) of the anti-abuse bond feature. When a taker takes an order on a bond-requiring node, the daemon replies with
pay-bond-invoice(a Lightning hold invoice for the taker's anti-abuse bond) instead of progressing the trade. Previously the client rejected that reply with a stableBondRequiredmarker; this PR replaces the rejection with the real taker bond flow: the bond hold invoice is shown to the taker, and once it is paid the trade advances into the normal take flow.What changed
Rust (
rust/src/api/)BondRequiredrejection branch inclassify_take_reply.OrderStatus::WaitingTakerBondand mapped it inmap_core_status/status_for_action(maker-sideWaitingMakerBondstays unmapped — that is Phase 5: maker anti-abuse bond on order creation #191).bond_invoicefield toTradeInfoandDaemonReply::TakeAccepted, kept distinct fromhold_invoice(see "Verified against the daemon" below).Action::AddInvoicedispatcher arm so the buyer's trade advances correctly after the bond is paid.Dart (
lib/)PayBondInvoiceScreen(route/pay_bond_invoice/:orderId) reusing the QR / NWC / external-wallet payment pattern of the seller hold-invoice screen. It watches the order status and forwards the taker to the next step once the bond is paid: add-invoice (buyer), pay-invoice (seller), or trade detail.take_order_screenroutes to the bond screen when the take reply comes back asWaitingTakerBond, and theBondRequirederror branch was removed.tradeBondInfoProviderand thewaitingTakerBondcases to the exhaustiveOrderStatusswitches.l10n: new bond-screen strings across all 5 locales; retired
bondRequired.Verified against the daemon
Cross-checked against
mostro(daemon),mostro-cli, and v1mobile, which surfaced two issues fixed here:pay-bond-invoicepayload'sSmallOrderwith the wire-mappedPendingstatus (WaitingTakerBondis never exposed on the NIP-69 wire), soclassify_take_replynow derives the bond status from the action, not the payload. Without this the client would never route to the bond screen.WaitingPaymentand sends thepay-invoicegift wrap. Reusing thehold_invoiceslot for the bond raced these two: the pay-invoice screen could latch the stale bond bolt11. The dedicatedbond_invoicefield removes the collision (hold_invoicestays null until the escrow arrives).Testing
cargo test(110 passing) andcargo clippyclean.flutter analyzeclean andflutter test(120 passing).flutter_rust_bridgebindings regenerated and verified in sync.Out of scope / follow-ups
Summary by CodeRabbit