fix(#202): wait for the daemon reply before persisting a dispute - #275
Open
AndreaDiazCorreia wants to merge 9 commits into
Open
fix(#202): wait for the daemon reply before persisting a dispute#275AndreaDiazCorreia wants to merge 9 commits into
AndreaDiazCorreia wants to merge 9 commits into
Conversation
…ecord Send Dispute action with correlation nonce, wait for daemon's DisputeInitiatedByYou or CantDo reply, and persist local record only on acceptance. Previously persisted on publish, leaving unreconciled Open disputes when daemon rejected with CantDo (#202). Add 10s timeout with late-reply handling, use daemon-assigned dispute UUID, and update error documentation.
…quest handling Add tests for dispute_id_from_payload helper, verify take_matching_dispute only consumes Dispute records with exact nonce, and confirm CantDo rejections reach waiting open_dispute callers to prevent unreconciled Open disputes (#202).
Update dispute contract to explain request_id nonce correlation, 10s timeout, daemon UUID assignment, and no-persistence-on-rejection behavior. Note CantDo vs internal-error distinction and cross-reference orders.md nonce gate. Add id field note distinguishing daemon-assigned UUIDs (our disputes) from local UUIDs (peer disputes). Include open_dispute in orders.md correlation list.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesDispute correlation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant open_dispute
participant orders_dispatch
participant Daemon
participant dispute_store
Client->>open_dispute: Open dispute
open_dispute->>orders_dispatch: Register request nonce
open_dispute->>Daemon: Publish correlated Dispute action
Daemon-->>orders_dispatch: DisputeInitiatedByYou or CantDo
orders_dispatch-->>open_dispute: Matching response
alt accepted with daemon dispute ID
open_dispute->>dispute_store: Persist dispute and update status
else rejected or timed out
open_dispute-->>Client: Return error
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![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
🤖 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/disputes.rs`:
- Around line 297-302: Require DisputeAccepted handling in
rust/src/api/disputes.rs at lines 297-302 to extract dispute_id and return
ProtocolError before constructing or persisting Dispute when it is absent;
remove the local UUID fallback while preserving daemon-provided IDs. In
rust/src/api/orders.rs at lines 3308-3311, remove the fallback expectation and
add coverage verifying that an acceptance without a daemon dispute UUID is
rejected and persists nothing.
In `@rust/src/api/orders.rs`:
- Around line 1781-1814: The DisputeInitiatedByYou handling must not allow a
late acceptance with tx: None to fall through and persist only the Dispute trade
status. Update the take_matching_dispute branch in the reply handler to either
return immediately after logging late acceptances or create the
daemon-identified Dispute record before continuing; preserve normal processing
for active requests, and add a test covering timeout followed by acceptance.
- Around line 302-317: Update register_dispute_request to atomically reserve
each trade key before inserting its PendingRequest, rejecting a second
concurrent open_dispute attempt instead of replacing the existing waiter.
Release the reservation when the first attempt completes, including success,
failure, and timeout paths, and preserve the original sender/request
association. Add a concurrent-open test verifying the second attempt is rejected
while the first remains registered.
🪄 Autofix
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: 50278ee6-694c-4360-8291-8dfaf3f04651
📒 Files selected for processing (6)
rust/src/api/disputes.rsrust/src/api/orders.rsrust/src/mostro/actions.rsspecs/004-mostro-p2p-client/contracts/disputes.mdspecs/004-mostro-p2p-client/contracts/orders.mdspecs/004-mostro-p2p-client/data-model.md
![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: 44944d2129
ℹ️ 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".
Prevent second open_dispute from replacing first attempt's pending record by checking insert freshness. Previously both concurrent calls would derive same trade key, second would overwrite first's pending entry, stranding first waiter on NoDaemonResponse. Add test coverage for in-flight guard rejection (#275).
…-reply Two conflicts, both additive: - rust/src/api/disputes.rs: the status gate from #203 and this branch's single-flight guard are adjacent in open_dispute and independent, so both stay, gate first — it is the cheap local check. Their tests likewise. - contracts/disputes.md: main's stale "creates local Dispute record" side effect is the one this branch replaces; main's local-precondition paragraph is kept and the two layers are now documented together. cargo test 246 pass, clippy clean, flutter analyze clean, flutter test 197 pass, frb-generate produces no drift.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specs/004-mostro-p2p-client/contracts/orders.md (1)
255-264: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope deletion to taker-side rows.
These lines state that every
Pending/WaitingBuyerInvoice/WaitingPaymentrow is deleted onCanceled. Line 279 requires a maker row to be resynced toPendingafter a timeout republish. Keep the maker row and define the maker/taker condition explicitly. Otherwise, the cancellation handler can delete data that the republished order still needs.🤖 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 `@specs/004-mostro-p2p-client/contracts/orders.md` around lines 255 - 264, Update the cancellation semantics around the Canceled path to delete only taker-side never-active rows, not every Pending/WaitingBuyerInvoice/WaitingPayment row. In the orders contract text and any matching cancellation handler logic, make the maker/taker distinction explicit using the existing Canceled/TradeUpdate semantics so a maker row can still be resynced to Pending after timeout republish, while taker-side rows are deleted and active/history trades keep status Canceled.
🤖 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.
Outside diff comments:
In `@specs/004-mostro-p2p-client/contracts/orders.md`:
- Around line 255-264: Update the cancellation semantics around the Canceled
path to delete only taker-side never-active rows, not every
Pending/WaitingBuyerInvoice/WaitingPayment row. In the orders contract text and
any matching cancellation handler logic, make the maker/taker distinction
explicit using the existing Canceled/TradeUpdate semantics so a maker row can
still be resynced to Pending after timeout republish, while taker-side rows are
deleted and active/history trades keep status Canceled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea4b91eb-6250-448d-9ad8-333689627d02
📒 Files selected for processing (5)
rust/src/api/disputes.rsrust/src/api/orders.rsspecs/004-mostro-p2p-client/contracts/disputes.mdspecs/004-mostro-p2p-client/contracts/orders.mdspecs/004-mostro-p2p-client/data-model.md
🚧 Files skipped from review as they are similar to previous changes (4)
- specs/004-mostro-p2p-client/data-model.md
- specs/004-mostro-p2p-client/contracts/disputes.md
- rust/src/api/disputes.rs
- rust/src/api/orders.rs
… placeholder When claiming a peer-initiated placeholder during the open_dispute race window, adopt the daemon's dispute ID instead of keeping the locally-minted placeholder ID. The peer path never sees the daemon's ID, while an accepted open carries it; discarding it would lose the authoritative identifier. Add test coverage for ID preservation during claim (#275 review).
…meout When the daemon accepts open_dispute after the 10s timeout, the acceptance is genuine — the daemon opened the dispute and notified the counterparty — and its reply moves the trade to Disputed either way. Suppressing the record would leave a disputed trade with no dispute to open and no solver to reach. Record it now (unread, without the reason that went with the timed-out call).
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review: changes requested
I would not approve this yet.
Blocking issue:
open_disputestill persists a locally generated dispute id when the accepted daemon reply does not carry aPayload::Disputeid. That contradicts the new daemon-id contract this PR introduces: the solver and Kind 38386 dispute events use the daemon UUID, so a locally generated fallback can recreate an unjoinable local dispute record. The late-acceptance reconciliation has the same fallback. Please fail closed instead: treat an acceptance without a daemon dispute UUID as a protocol error / unreconcilable malformed acceptance and persist nothing, and add a focused test for that case.
I verified the rest of the previously raised concurrency/late-acceptance concerns on the current head: the in-flight guard, placeholder id adoption, and late-acceptance reconciliation tests are present and pass locally.
Local verification run:
cargo test --lib→ 248 passed, 8 ignored.
Reject malformed acceptances that carry no dispute_id in both open_dispute and record_late_acceptance. Dispute.id is contractually the daemon's identifier; minting a local fallback would create a record indistinguishable from a genuine one while being wrong. A conforming daemon always sends it (mostro src/app/dispute.rs notify_dispute_to_users), so this is a protocol-violation guard. Add test coverage for late acceptance rejection (#275
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 `@rust/src/api/disputes.rs`:
- Around line 475-491: The upsert update closure around upsert_or_update must
claim only an existing InReview, non-initiator, reason-less placeholder with a
solver, preserving its solver and status while replacing its local ID with the
daemon dispute ID and setting initiated_by_me to true. Leave all other records
unchanged, and update a_late_acceptance_leaves_an_existing_record_alone to cover
this race while retaining a separate completed-retry preservation test.
🪄 Autofix
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: 81782226-691d-4391-b2c1-4eb5a5d2c3d3
📒 Files selected for processing (3)
rust/src/api/disputes.rsrust/src/api/orders.rsspecs/004-mostro-p2p-client/contracts/disputes.md
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/src/api/orders.rs
- specs/004-mostro-p2p-client/contracts/disputes.md
There was a problem hiding this comment.
Strict re-review: changes requested
The previous daemon-id fallback blocker is fixed on this head, but I would still not approve yet.
Blocking issue:
- The late-acceptance reconciliation still leaves an admin-created placeholder unchanged. If
open_disputetimes out, thenadmin-took-disputecreates the peer-style placeholder before the correlatedDisputeInitiatedByYouacceptance is processed,record_late_acceptancecallsupsert_or_update(..., |_| Ok(()))and keeps the local placeholder id plusinitiated_by_me: false. That loses the daemon dispute id and ownership proof from the correlated acceptance, so the local record still violates the daemon-id contract in this race.
Please make record_late_acceptance claim only the placeholder shape that proves this race (InReview, non-initiator, no reason, solver present): preserve the solver/status, replace the local id with the daemon dispute id, and set initiated_by_me = true. Leave completed retry records unchanged. Add a focused test for that interleaving; the current a_late_acceptance_leaves_an_existing_record_alone test actually codifies the buggy behavior.
Verification:
cargo test --libpasses on the current head: 249 passed, 8 ignored.- A temporary focused test for the placeholder interleaving fails on this head: the record keeps the placeholder UUID instead of adopting the daemon UUID.
…uard Both the post-acceptance insert and the late-acceptance reconciliation test for the exact same placeholder shape before claiming it (InReview, not ours, no reason, solver known). Extract the predicate into a single function with a doc comment explaining what it guards. Update the late-acceptance reconciliation to claim the placeholder the same way the insert does, and add test coverage for the claim path.
There was a problem hiding this comment.
Strict re-review: approved
The latest head fixes the remaining daemon-id reconciliation blocker. I re-checked the prior concerns on the current commit:
- Missing daemon dispute ID now fails closed and persists nothing.
- Late acceptance with no local record creates a daemon-ID-backed dispute.
- Late acceptance racing an admin-created placeholder now claims only the placeholder shape, preserves solver/status, and adopts the daemon dispute ID plus initiator ownership.
- A successful retry record is left untouched.
Local verification:
cargo test --lib -- --nocapture→ 250 passed, 8 ignored.cargo clippy --locked -- -D warnings→ passed.
No blocking issues remain from my review.
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 #202
Problem
open_disputetreated a successful relay publish as a successful dispute: it persistedDispute { status: Open }right after publishing and never registered a pending-request waiter. TheDisputemessage also carried norequest_id, so the daemon'sCantDorejection matched nothing in the dispatcher and was dropped (no matching pending request, ignoring event). The dispute stayed locally Open and the UI navigated to the dispute detail — a false success.Fix
The Dispute message now carries a
request_idnonce, which the daemon echoes in bothDisputeInitiatedByYouandCantDo(verified in mostrosrc/app/dispute.rsandsrc/app.rs).open_disputeregisters a pending record before publishing and waits up to 10 s, exactly likecreate_order/take_order/send_invoice. It persists only on a correlated acceptance; rejection and timeout return an error and write nothing.The acceptance also carries the daemon's dispute UUID — the id its Kind 38386 event and the solver refer to — so the record is stored under it instead of a locally minted one.
No Dart changes:
_openDisputealready showed a localized SnackBar and rethrew without navigating, so propagating the error was enough.Notes
The daemon replies
CantDoonly forMostroCantDocauses. A duplicate dispute or a daemon-side DB failure is an internal error it merely logs, so those surface asNoDaemonResponserather than a precise reason.Verification
cargo build,cargo clippy --locked -- -D warnings,cargo test(238 pass),flutter analyze,flutter test(195 pass).frb-generate.shproduces no drift — the public bridge surface is unchanged.Tests cover the correlation layer, where the bug was: the kind/nonce gates of
take_matching_dispute, aCantDoreaching the waiting caller through the dispatcher's lookup, and the dispute-id extraction. The end-to-end path needs a live daemon — pending manual check with the #203 repro.Follow-ups (not in this PR)
data-model.md.fiat_sent,releaseandcancelstill send no nonce and await no reply — the same false-success shape as this issue, in other actions.Summary by CodeRabbit
New Features
Bug Fixes
Documentation