feat(disputes): route admin-took-dispute so the app learns the solver - #253
Conversation
First half of taking the dispute chat off gift wrap (#138). `handle_admin_took_dispute` existed and `Dispute.admin_pubkey` was documented as "populated when adminTookDispute is received", but nothing ever called it: `grep -rn AdminTookDispute rust/src` found no dispatch, and no Dart caller either. The action fell through the catch-all in `dispatch_mostro_message`, so the solver's pubkey — the only thing both sides can ECDH against to establish the dispute chat — was discarded on arrival. The dispatch now routes it, reading the pubkey from the `Peer` payload the daemon sends (https://mostro.network/protocol/dispute_chat.html). Any other payload shape is reported rather than guessed at: deriving keys from the wrong field would silently address the wrong party. `handle_admin_took_dispute` also creates the dispute record when none exists. The daemon notifies BOTH parties, and the side that did not open the dispute has no local record, so `update_conditional` failed with DisputeNotFound and the pubkey was lost exactly for the party that never chose to be in a dispute. The shared-key derivation moved into its own function so both paths use it. Tests: the solver pubkey is read from a Peer payload; a message without one yields nothing (no guessing); and a peer-opened dispute records the solver, lands InReview, and is marked unread. 201 Rust tests, clippy unchanged at the existing 24 warnings, wasm32 check passes, bridge regen yields no diff. Second half, separately: the channel itself — sending and receiving over the kind-14 envelope from dispute_chat.md instead of `gift_wrap::wrap`, which is the last NIP-59 user left in the app.
|
Warning Review limit reached
Next review available in: 12 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 Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesAdmin-taken dispute flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Daemon
participant MessageDispatcher
participant DisputeHandler
participant DisputeStore
participant SharedKeyDerivation
Daemon->>MessageDispatcher: AdminTookDispute action
MessageDispatcher->>MessageDispatcher: Extract order_id and Peer pubkey
MessageDispatcher->>DisputeHandler: Handle dispute and admin pubkey
DisputeHandler->>DisputeStore: Create or update dispute record
DisputeHandler->>SharedKeyDerivation: Derive admin shared key
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 |
![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: efe942213a
ℹ️ 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 252-266: The missing-dispute check in the dispute creation flow
must be atomic to prevent an arriving AdminTookDispute record from being
overwritten or causing a duplicate insert. Replace the separate
dispute_store().get and upsert calls with a store-level insert-or-merge
operation, or persist the pending local dispute before dispatch, ensuring any
existing dispute is preserved and not replaced by initiated_by_me: false data.
🪄 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: 61651d64-2341-4e82-af71-f2e5297ec9d2
📒 Files selected for processing (2)
rust/src/api/disputes.rsrust/src/api/orders.rs
…dmin-took Codex P2 and CodeRabbit both flagged the same TOCTOU: an incoming admin-took-dispute raced open_dispute's post-publish insert in both directions. The handler's separate get + upsert could overwrite the initiator's fresh record with a peer-side placeholder (initiated_by_me: false, reason: None), and the opposite interleaving made open_dispute fail DisputeAlreadyOpen with its metadata lost. - DisputeStore::upsert_or_update: create-or-update under one write lock; handle_admin_took_dispute now uses it, so the missing-record check and the insert can no longer interleave with anything. - try_insert_if_absent_or_resolved learns to claim the admin-took placeholder (InReview, not ours, no reason, solver known): the initiator's insert restores initiated_by_me and its reason while keeping the solver and the InReview status the handler already learned, instead of failing. Resolved disputes still get replaced, live ones still reject duplicates. - Targeted tests for both interleavings: open_dispute_insert_claims_the_admin_took_placeholder and admin_took_preserves_the_initiators_metadata.
Resolves the disputes.rs conflict by combining both review rounds: handle_admin_took_dispute keeps #253's atomic upsert_or_update (one write lock, placeholder created when the record is missing) and folds in #254's idempotent same-solver replay arm, so a replayed assignment for the same solver falls through to re-derive and re-arm the listener instead of failing InvalidState. Tests from both rounds pass together.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review of exact head 57c7def0e82c32001a90cea7a34bfc2871177190.
I read the full conversation before reviewing and did not repeat the resolved check-then-act race. The atomic store operation fixes that interleaving, but three distinct integration/state issues remain on the current head; details are inline.
Verification performed:
cargo test --locked: 203 passed, 0 failed, 8 ignored.cargo clippy --locked --all-targets: completed with the repository's existing warnings.- Current Rust, Flutter, and Web checks are green on this exact SHA.
- Payload shape and solver-takeover behavior were cross-checked against the current Mostro daemon and public protocol.
The tests do not cover solver reassignment, a peer-owned placeholder being mistaken for a local in-flight open, or delivery after the per-trade receiver expires. Requesting changes for those cases.
…on, DM coverage Addresses ermeme's three P1s on 57c7def: - Solver reassignment + idempotent replay: the update closure accepts an InReview record — same solver falls through as a retry of the best-effort key derivation, a different pubkey is a genuinely newer assignment (the daemon lets a write-capable solver take over from a read-only one and resends admin-took-dispute) and replaces the stored key. Exact-event replays never reach the handler — the receive path dedups by event id — so ordering is handled upstream. - Placeholder claim correlation: open_dispute now fails DisputeAlreadyOpen at entry when any non-Resolved record exists (no duplicate publish), and registers an in-flight pending-open marker before publishing; try_insert_if_absent_or_resolved claims the admin-took placeholder ONLY while that marker is held. A placeholder with no owned in-flight attempt is a genuinely peer-opened dispute and stays peer-owned. - Refreshable bulk Kind-14 coverage: the global subscription's decryption map is now a refreshable global seeded at startup; create/take/restore call ensure_global_dm_coverage after deriving a fresh key, which adds it to the map and re-issues the bulk DM filter under the same stable id — so a solver assignment arriving after the 30-minute per-trade receiver expires is still decrypted. Tests: reassignment replaces the pubkey, same-solver replay is idempotent, an unowned placeholder is preserved and rejected, the claim test now models the in-flight open via the marker, and a late-derived key joins the coverage map idempotently.
Resolves the handler conflict by keeping the base's broader InReview arm (same-solver idempotent replay + solver reassignment), which subsumes #254's same-solver-only arm, and drops the now-duplicate same-solver replay test in favor of the base's copy. 212 tests pass.
…lity Conflict in `handle_admin_took_dispute`: main replaced the check-then-act create path with a single `upsert_or_update` under one store write lock (PR #253 review — a separate check races `open_dispute`'s post-publish insert in both directions). Took main's version; the persistence this branch adds needed no special case there, since the single `persist_admin_pubkey` call after the store write already covers both the create and the update path.
Second half of MostroP2P#138, on top of MostroP2P#253. The dispute channel used NIP-59: `submit_evidence` gift-wrapped a hand-rolled `{"type":"evidence", …}` JSON to the admin, and nothing ever read the other direction — there was no inbound path at all, so a solver's reply was invisible to the app. It now uses the same envelope as the peer chat, which is what https://mostro.network/protocol/dispute_chat.html specifies: inner kind 1 signed by our trade key, NIP-44 under `K_conv`, inside a kind 14 signed with `K_sign` and p-tagged to `pub(K_conv)`. No gift wrap, no ephemeral key, and nothing addressed to a pubkey a relay could correlate. Key derivation is `derive_chat_keys` unchanged: the only difference is that the ECDH peer is the solver's pubkey from `admin-took-dispute` instead of the counterparty's trade key, exactly as the spec prescribes. The inbound side is the existing subscriber, generalized by a `ChatChannel`. Its accepted-signers slice was already `[my trade key, other party]`, which is the right rule for both channels — the spec makes the admin a legitimate writer here, unlike the peer chat. The channel decides three things: the single-owner guard key and subscription id (so the two conversations of one order do not collide and the dispute chat is not silently swallowed by the peer chat's guard), the stored `MessageType`, and whether to dual-read the pre-migration gift wrap — only the peer chat ever had one. Also drops the `admin_shared_key` derivation, which computed a NIP-04-style key that was written to the session and never read by anything. NIP-59 is not gone from the app yet: `gift_wrap::unwrap` still serves the time-boxed legacy read of the peer chat until LEGACY_CHAT_DEPRECATION_TS. This removes the last *new* NIP-59 traffic. 205 Rust tests (four new on channel separation, wire-identity stability, legacy gating and message typing), clippy back at the baseline 24 warnings, wasm32 check passes, bridge regen yields no diff.
1 participant
First of two PRs taking the dispute chat off gift wrap. Part of #138.
Problem
handle_admin_took_disputealready existed, andDispute.admin_pubkeywas documented as "populated whenadminTookDisputeis received" — but nothing ever called it.grep -rn AdminTookDispute rust/srcfound no dispatch arm, and no Dart caller either, so the action fell through the catch-all indispatch_mostro_message.That pubkey is the whole basis of the dispute chat: per dispute_chat.md both sides ECDH their trade key against the solver's pubkey and split the result into
K_conv/K_sign. Discarding the message on arrival means there is no way to reach the solver at all.Change
dispatch_mostro_messageroutesAction::AdminTookDispute, reading the pubkey from thePeerpayload the daemon sends. Any other payload shape is logged rather than guessed at — deriving keys from the wrong field would silently address the wrong party.handle_admin_took_disputenow creates the dispute record when none exists. The daemon notifies both parties, and the side that did not open the dispute has no local record, soupdate_conditionalfailed withDisputeNotFound: the pubkey was lost precisely for the party that never chose to be in a dispute. The record landsInReview,initiated_by_me: false, unread.Test plan
cargo test— 201 passing. New: the solver pubkey is read from aPeerpayload; a message without one yieldsNone(no guessing); a peer-opened dispute records the solver, landsInReviewand is marked unread.cargo clippy --all-targets— 24 warnings, identical tomain(verified by stashing); the one near the new test pre-exists.cargo check --locked --target wasm32-unknown-unknown— passes./scripts/frb-generate.sh— no diff; the bridge surface is unchangedNot in this PR
The channel itself: sending and receiving dispute messages over the kind-14 envelope from
dispute_chat.mdinstead ofgift_wrap::wrap(api/disputes.rs:217), plus inbound routing into the message store. That is the second PR, and it removes the last NIP-59 user in the app.Also left for it: durability. The dispute store is in-memory by design, so the solver pubkey does not survive a restart yet — the chat is what makes that matter, so it is fixed where it is needed.
Summary by CodeRabbit