fix: take_order and send_invoice wait for daemon confirmation#178
Merged
Conversation
…uests - rename PendingCreate/DaemonConfirmation to PendingRequest/DaemonReply and move the create-specific fields (local_uuid, content_key) into a PendingRequestKind::Create variant - no behavior change: same correlation invariant, same call sites, tests adapted mechanically - prepares the take/add-invoice confirmation work, which reuses this same machinery with new request kinds
- send a request_id nonce in take-buy/take-sell and register a pending Take record; only the daemon reply echoing the nonce can resolve the caller - resolve take waiters before the per-action arms: a take's first reply varies (add-invoice, pay-invoice, pay-bond-invoice), so replies classify by payload shape and the caller applies their effects itself - return Err on CantDo, internal failures, publish failure and the 10s timeout — nothing is persisted until the daemon acknowledges the take, removing the fake-trade path - pay-bond-invoice maps to a stable BondRequired rejection (bonds are not supported yet); localized in the take screen along with NoDaemonResponse and OrderAlreadyTaken
- send a request_id nonce in add-invoice and register a pending AddInvoice record; only the daemon reply echoing the nonce unblocks the caller - the reply doubles as a status update, so the dispatcher acknowledges the waiter and falls through to the per-action arms (unlike takes, whose caller applies the reply itself) - return Err on a correlated CantDo (e.g. invalid invoice) or the 10s timeout — the UI stays on the invoice step instead of advancing on a publish the daemon errored on
- add-invoice: display the sats the buyer's invoice must be for (from the daemon's AddInvoice reply) in the manual entry card - pay-invoice: display the hold invoice amount (from the PayInvoice reply) above the QR in the manual view - take_order now syncs the reply's amount_sats into the order book too, so tradeAmountProvider resolves on its first poll instead of waiting for the 38383 amt tag - new addInvoiceAmount/payInvoiceAmount l10n keys in the 5 locales
- document the request_id correlation invariant shared by create/take/ add-invoice in the orders contract - take_order: real signature, reply-driven trade construction, and the BondRequired/NoDaemonResponse error surface; drop the unimplemented ActiveTradeExists precondition - rename submit_buyer_invoice to the real send_invoice API and describe the gated acknowledgement
Contributor
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughOrder-taking and invoice submission now wait for nonce-correlated daemon replies before completing. Pending requests share unified matching and timeout handling, while Flutter screens use localized errors and display invoice amounts in sats. ChangesOrder confirmation and correlation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FlutterScreens
participant RustOrderAPI
participant MostroDaemon
participant TradeSession
User->>FlutterScreens: Take order or submit invoice
FlutterScreens->>RustOrderAPI: Invoke correlated order action
RustOrderAPI->>MostroDaemon: Publish request_id-bearing message
MostroDaemon-->>RustOrderAPI: Confirm or reject echoed request_id
RustOrderAPI->>TradeSession: Persist accepted trade and create session
RustOrderAPI-->>FlutterScreens: Return success or localized error context
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…ayment - navigate to trade detail on waitingBuyerInvoice: on this screen it can only mean the hold invoice was paid and mostrod now waits for the buyer's invoice — the seller was left staring at an already-paid QR - inProgress deliberately stays put: the public 38383 status flips at take time, before payment
![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: 2
🧹 Nitpick comments (1)
lib/features/order/screens/add_lightning_invoice_screen.dart (1)
93-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared AnyhowException parsing to avoid duplication.
The regex
r'^.*?AnyhowException\((.+)\)$'and the extraction logic are duplicated verbatim intake_order_screen.dart(lines 156-157). If the Rust error format changes, both call sites must be updated in lockstep — a missed update would cause inconsistent error display across screens.Consider extracting to a shared utility, e.g.:
♻️ Proposed refactor
// In a shared utility, e.g. lib/core/error_utils.dart /// Strips the Rust `AnyhowException(...)` wrapper and returns the inner /// message, or the raw string if the pattern doesn't match. String extractAnyhowMessage(String raw) { final match = RegExp(r'^.*?AnyhowException\((.+)\)$').firstMatch(raw); return match != null ? match.group(1)! : raw; }Then in both screens:
- final raw = e.toString(); - final anyhowMatch = RegExp(r'^.*?AnyhowException\((.+)\)$').firstMatch(raw); - final msg = anyhowMatch != null ? anyhowMatch.group(1)! : raw; + final msg = extractAnyhowMessage(e.toString());🤖 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/screens/add_lightning_invoice_screen.dart` around lines 93 - 95, Extract the duplicated AnyhowException parsing into a shared utility function, such as extractAnyhowMessage, preserving the existing regex and fallback behavior. Update the error handling in both add_lightning_invoice_screen.dart and take_order_screen.dart to call this utility instead of duplicating the extraction logic.
🤖 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 164-182: Same-key concurrent send_invoice attempts can detach or
delete a newer attempt’s pending waiter. Update detach_request_waiter and
remove_pending_request to accept and validate a per-attempt request_id (or
equivalent identity) before mutating the map entry, and pass that identity
through send_invoice timeout, receive-error, and publish-failure paths. Add a
regression test covering overlapping same-key attempts, ensuring the newer
waiter remains active and receives the daemon reply.
In `@specs/004-mostro-p2p-client/contracts/orders.md`:
- Around line 110-122: Update send_invoice to return the documented
TradeNotFound marker when the persisted trade lookup fails, replacing the
free-form “no persisted trade key...” error while preserving the existing
handling for InvalidInvoice and NoDaemonResponse.
---
Nitpick comments:
In `@lib/features/order/screens/add_lightning_invoice_screen.dart`:
- Around line 93-95: Extract the duplicated AnyhowException parsing into a
shared utility function, such as extractAnyhowMessage, preserving the existing
regex and fallback behavior. Update the error handling in both
add_lightning_invoice_screen.dart and take_order_screen.dart to call this
utility instead of duplicating the extraction logic.
🪄 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: 26f2f3ff-1c8d-4a9d-8f5d-06c235f7837f
📒 Files selected for processing (17)
lib/features/order/screens/add_lightning_invoice_screen.dartlib/features/order/screens/pay_lightning_invoice_screen.dartlib/features/order/screens/take_order_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/mostro/actions.rsspecs/004-mostro-p2p-client/contracts/orders.md
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Blocking
- rust/src/api/orders.rs:1638 — The early return for correlated take replies bypasses the status-sync arms below for action-only progression replies. When
classify_take_reply()has no payload to extract a status from,take_order()can persist the trade asPendingeven though Mostro already advanced it (for exampleWaitingBuyerInvoice/WaitingSellerToPay).
Looks Good
- The request-id correlation and timeout handling for take/add-invoice are a solid step up from the previous optimistic flow.
Verdict: Request changes
…t_id - classify_take_reply falls back to the action-implied status (shared status_for_action helper, also used by the status-sync arm) so a take whose first reply is action-only no longer persists the trade as Pending - detach_request_waiter/remove_pending_request now require the attempt's request_id: overlapping same-key attempts (send_invoice reuses the take's trade key) can no longer cross-detach or delete each other's records; subscription-end cleanup stays unconditional as purge_pending_request - send_invoice returns the documented TradeNotFound marker
There was a problem hiding this comment.
Code Review Summary
Looks Good
- The earlier take-order status-sync blocker is fixed on the current head.
take_ordernow waits for the daemon reply, classifies action-only progress replies correctly, and persists the real status/amount instead of a phantom pending trade.send_invoicekeeps the request-id correlation and status propagation intact, and the focused Rust tests pass on this head.
Verdict: Approve
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
1 participant
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 #170
Problem
Taking an order and submitting an invoice were fire-and-forget:
take_orderreturned
Ok(trade)unconditionally (even on internal errors and publishfailures) and persisted a local trade before any daemon response. When the
daemon rejected the take (
CantDo) or never answered, the app still walkedthe whole happy path — add-invoice screen,
AddInvoicepublish the daemonerrored on, "Waiting for a counterpart" — for a trade that never existed.
Daemon rejections found no waiter and were dropped as "stale".
Fix
Extends the request-correlation machinery from #172 (create_order) to the
take and add-invoice flows, matching how mostro-cli and MostriX correlate
every action:
refactor(orders): generalizePendingCreate→PendingRequestwith akind per request type (
Create/Take/AddInvoice). No behaviorchange; same correlation invariant.
fix(orders) take_order: sends arequest_idnonce and waits up to 10 sfor the daemon's first reply. A take's success reply varies by role and
daemon config, so replies are classified by payload shape (the MostriX
pattern):
add-invoice→ calculated sats,pay-invoice→ hold invoice,pay-bond-invoice→ stableBondRequiredrejection (bonds unsupported).The trade is built from the reply's real data and nothing is persisted
on rejection, timeout, internal error or publish failure. All error
paths now return
Err(localized in the take screen).fix(orders) send_invoice: same nonce + 10 s wait. A correlatedCantDo(e.g.
InvalidInvoice) or timeout surfaces as an error and the UI stayson the invoice step. The reply doubles as a status update, so the
dispatcher acknowledges the waiter and falls through to the normal arms.
feat(ui): the add-invoice and pay-invoice screens now display thedaemon-calculated sats amount (data this fix captures from the replies);
take_orderalso syncs it into the order book so the amount resolves onthe provider's first poll.
docs(specs): orders contract updated — correlation invariant, realtake_order/send_invoicesignatures and error surfaces.fix(ui): the pay-invoice screen now navigates to the trade once thedaemon registers the payment (
waiting-buyer-invoice), not only when theorder goes straight to
active.The correlation invariant is unchanged from #172: only a reply echoing
the exact nonce may resolve or consume a pending request; stale relay
replays touch nothing.
Verification
cargo test(105) +cargo clippyclean,flutter analyzeclean(3 pre-existing infos). Manual end-to-end run against the test daemon
covered every scenario:
AddInvoicereply correlated →trade built with the daemon's calculated sats → invoice acknowledged via
WaitingSellerToPay→ trade completed throughReleased/PurchaseCompleted/RateReceived.PayInvoicereply with the holdinvoice (amount extracted from the embedded order — the fallback path) →
trade completed through
HoldInvoicePaymentSettled/RateReceived.NoDaemonResponse, no phantom trade persisted, immediate retrysucceeded. This exact case produced the fake trade before this PR.
CantDo(InvalidInvoice)echoing the nonce →send_invoicereturned the error and the UI stayed on the invoice step.NewOrder/CantDoignored("no matching pending request").
Summary by CodeRabbit