fix: close active dispute when seller releases funds#606
Merged
grunch merged 5 commits intoMostroP2P:mainfrom Feb 23, 2026
Merged
fix: close active dispute when seller releases funds#606grunch merged 5 commits intoMostroP2P:mainfrom
grunch merged 5 commits intoMostroP2P:mainfrom
Conversation
When an order with an active dispute is released by the seller, the dispute status should be updated to 'settled' and a Nostr event should be published so admin clients see it as resolved. This is similar to the fix in cancel.rs for cooperative cancellation (which sets status to 'seller-refunded'), but for the release flow where sats go to the buyer. Closes MostroP2P#605
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:
WalkthroughAdds a centralized best-effort function Changes
Sequence Diagram(s)sequenceDiagram
participant ReleaseFlow as "release_action"
participant CancelFlow as "cancel_cooperative_execution_step_2"
participant DisputeFn as "close_dispute_after_user_resolution"
participant DB as "SQLite Pool"
participant Nostr as "NostrClient"
ReleaseFlow->>DisputeFn: call(order, DisputeStatus::Settled, my_keys, "release")
CancelFlow->>DisputeFn: call(order, DisputeStatus::SellerRefunded, my_keys, "cooperative cancel")
DisputeFn->>DB: find_dispute_by_order_id(order.id)
DB-->>DisputeFn: dispute (or not found)
alt dispute found
DisputeFn->>DB: update dispute.status = new_status
DB-->>DisputeFn: update result
DisputeFn->>Nostr: build & publish dispute event (tags: status, initiator, y="mostro", z="dispute")
Nostr-->>DisputeFn: publish result
else no dispute
DisputeFn-->>DisputeFn: log "no dispute found"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![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)
src/app/release.rs (1)
246-246:eventshadows the&UnwrappedGiftfunction parameter.The binding
if let Ok(event) = new_dispute_event(...)at line 246 shadows theevent: &UnwrappedGiftparameter declared at line 165. The originaleventis no longer live by this point (last used at line 197), so there is no functional impact, but the shadowing is confusing. Renaming todispute_eventorclose_eventis cleaner.♻️ Proposed rename
- if let Ok(event) = new_dispute_event(my_keys, "", dispute_id.to_string(), tags) { + if let Ok(dispute_event) = new_dispute_event(my_keys, "", dispute_id.to_string(), tags) { match get_nostr_client() { Ok(client) => { - if let Err(e) = client.send_event(&event).await { + if let Err(e) = client.send_event(&dispute_event).await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/release.rs` at line 246, The binding if let Ok(event) = new_dispute_event(...) shadows the function parameter event: &UnwrappedGift; rename the local binding to a non-conflicting name (e.g., dispute_event or close_event) where new_dispute_event is called so the parameter event remains unshadowed and the intent is clearer — update any subsequent uses of that local binding accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/release.rs`:
- Line 205: The code calls find_dispute_by_order_id(...) and then mutates the
returned dispute unconditionally; add a guard to skip updating if the dispute is
already in a terminal state (e.g., SellerRefunded, Settled, Closed) so you don't
overwrite prior resolutions. Locate the block that matches if let Ok(mut
dispute) = find_dispute_by_order_id(pool, order.id).await and before mutating or
setting dispute.status = Settled, check dispute.status against your enum's
terminal variants (or only proceed when status == Open/Pending) and
return/continue when it's terminal; ensure any logging reflects the skipped
update.
- Around line 246-257: The code currently swallows errors from new_dispute_event
by using if let Ok(event) and never logging Err; change the guard to match the
Result from new_dispute_event (or use if let Err(e) => { ... } ) so that when
new_dispute_event(...) returns Err you log the error (including the error
details) before returning/continuing; keep the existing flow that calls
get_nostr_client() and client.send_event(&event).await on success, but ensure
new_dispute_event failures are explicitly logged (referencing new_dispute_event,
get_nostr_client, and the client.send_event call so you update the correct
block).
---
Nitpick comments:
In `@src/app/release.rs`:
- Line 246: The binding if let Ok(event) = new_dispute_event(...) shadows the
function parameter event: &UnwrappedGift; rename the local binding to a
non-conflicting name (e.g., dispute_event or close_event) where
new_dispute_event is called so the parameter event remains unshadowed and the
intent is clearer — update any subsequent uses of that local binding
accordingly.
Renamed DISPUTE_AUTO_CLOSE_ON_COOPERATIVE_CANCEL.md to DISPUTE_AUTO_CLOSE_ON_USER_RESOLUTION.md and expanded it to cover both scenarios where disputes are automatically closed: 1. Cooperative Cancellation → SellerRefunded (existing) 2. Seller Release → Settled (new) Added comparison tables and code flow diagrams for both cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/DISPUTE_AUTO_CLOSE_ON_USER_RESOLUTION.md`:
- Around line 46-54: The two fenced ASCII diagram blocks containing the flows
for cancel_action() → cancel_cooperative_execution_step_2() (showing Cancel hold
invoice, Set order status to CooperativelyCanceled, Notify both parties, Close
dispute → SellerRefunded) and release_action() (Settle hold invoice, Set order
status to SettledHoldInvoice, Close dispute → Settled, Notify both parties,
Initiate payment to buyer) are missing language tags; update each opening
triple-backtick to include the language specifier text (i.e., change ``` to
```text) for both diagram blocks (the
cancel_action()/cancel_cooperative_execution_step_2() block and the
release_action() block, and the additional occurrence noted at lines 70-77) to
satisfy the project's fenced code block guideline.
- Log error when new_dispute_event() fails (was silently ignored) - Rename 'event' to 'dispute_event' to avoid shadowing function parameter - Add 'text' language specifier to markdown code blocks
Collaborator
|
Ok @Mostrica and @CodeRabbit, this idea is valid, but please try do thing in more nice way. You are duplicating code for nothing, the same logic in Line 118 in 48396a1 is replicated here, please extract a function that you can call like this afted db fetch: if let Ok(mut dispute) = find_dispute_by_order_id(pool, order.id).await {
update_dispute_event( dispute.id, new_status);
....
} |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3959 Task: Extract duplicate dispute closure logic into reusable helper function
🏁 Script executed: Length of output: 212 🏁 Script executed: Length of output: 2748 🏁 Script executed: Length of output: 2578 🏁 Script executed: Length of output: 2185
Task: Extract duplicate dispute closure logic into shared helper functionThe same ~50-line dispute closure block appears in both
🏁 Script executed: Length of output: 841 🧠 Learnings used💡 Use |
Contributor
Author
|
@arkanoider You're right, I apologize for the code duplication. I'll extract the dispute closure logic into a shared helper function in I'll push the refactored version shortly. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/dispute.rs (3)
262-262: Add a trace/debug log for the no-dispute early exit.When
find_dispute_by_order_idreturnsErr, the function silently returns. For a best-effort helper called on every cooperative-cancel and release path, atracing::debug!log would distinguish "order had no dispute" from a quiet DB failure, aiding production diagnostics.🔍 Suggested trace log
- if let Ok(mut dispute) = find_dispute_by_order_id(pool, order.id).await { + let Ok(mut dispute) = find_dispute_by_order_id(pool, order.id).await else { + tracing::debug!("No dispute found for order {} during {}; nothing to close", order.id, context); + return; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dispute.rs` at line 262, The early-return when find_dispute_by_order_id(pool, order.id).await yields Err is silent; add a tracing::debug! (or trace!) in the else branch to log that no dispute was found for the order (include the order id and any minimal context like order.id or order identifier) so callers can distinguish "no dispute" from a DB error; update the match around find_dispute_by_order_id to log this message before returning from the function.
255-329: Add unit tests forclose_dispute_after_user_resolution.The coding guidelines require co-located tests under
mod testswith descriptive names (e.g.,handles_missing_dispute,logs_error_on_db_update_failure,publishes_event_on_success). The new public function has multiple distinct code paths (no dispute found, DB update error, event publish error, full success) and currently has no test coverage.As per coding guidelines: "Co-locate tests in their modules under
mod testswith descriptive names likehandles_expired_hold_invoice."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dispute.rs` around lines 255 - 329, Add co-located unit tests under mod tests for the public function close_dispute_after_user_resolution to cover its distinct code paths: (1) when no dispute is found (handles_missing_dispute), (2) when updating the DB fails and an error is logged (logs_error_on_db_update_failure), (3) when creating or publishing the Nostr event fails and appropriate errors are logged (publishes_event_error_cases), and (4) full success where the dispute is updated and client.send_event is called (publishes_event_on_success). Use mocks or test doubles for find_dispute_by_order_id, dispute.update, new_dispute_event and get_nostr_client/client.send_event to simulate each scenario, assert expected log messages and side effects (DB update called or not, event send attempted), and place tests in the same module as close_dispute_after_user_resolution with descriptive test names as required.
289-326: Consider reusingpublish_dispute_eventto avoid tag-building duplication.The tag construction and event-publishing block here (lines 289–326) is essentially a duplicate of
publish_dispute_event(lines 32–82). Both live in the same module, so a small refactor—either wideningpublish_dispute_eventto accept an&strinitiator instead ofbool, or extractingbuild_dispute_tags(status, initiator)as a private helper—would eliminate the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dispute.rs` around lines 289 - 326, The code duplicates tag construction and event publishing; refactor to reuse publish_dispute_event or extract a shared helper: create a private function build_dispute_tags(status: &str, initiator: &str) that returns Tags (used by new_dispute_event) and replace the inline Tags::from_list block in the current code with a call to build_dispute_tags; alternatively broaden publish_dispute_event to accept initiator: &str (instead of bool) and call it here to handle tag creation and sending (reusing get_nostr_client/new_dispute_event inside that function) so the duplicate Tag::custom and client.send_event logic is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/dispute.rs`:
- Around line 282-286: The match on (order.seller_dispute, order.buyer_dispute)
in dispute_initiator silently falls back to "unknown" for inconsistent DB
states; add a tracing::warn! in the fallback branch that logs the actual flag
values (order.seller_dispute and order.buyer_dispute) and contextual info (e.g.,
order id or dispute id if available) before returning "unknown" so the
inconsistency is surfaced while keeping the current best-effort behavior.
---
Nitpick comments:
In `@src/app/dispute.rs`:
- Line 262: The early-return when find_dispute_by_order_id(pool, order.id).await
yields Err is silent; add a tracing::debug! (or trace!) in the else branch to
log that no dispute was found for the order (include the order id and any
minimal context like order.id or order identifier) so callers can distinguish
"no dispute" from a DB error; update the match around find_dispute_by_order_id
to log this message before returning from the function.
- Around line 255-329: Add co-located unit tests under mod tests for the public
function close_dispute_after_user_resolution to cover its distinct code paths:
(1) when no dispute is found (handles_missing_dispute), (2) when updating the DB
fails and an error is logged (logs_error_on_db_update_failure), (3) when
creating or publishing the Nostr event fails and appropriate errors are logged
(publishes_event_error_cases), and (4) full success where the dispute is updated
and client.send_event is called (publishes_event_on_success). Use mocks or test
doubles for find_dispute_by_order_id, dispute.update, new_dispute_event and
get_nostr_client/client.send_event to simulate each scenario, assert expected
log messages and side effects (DB update called or not, event send attempted),
and place tests in the same module as close_dispute_after_user_resolution with
descriptive test names as required.
- Around line 289-326: The code duplicates tag construction and event
publishing; refactor to reuse publish_dispute_event or extract a shared helper:
create a private function build_dispute_tags(status: &str, initiator: &str) that
returns Tags (used by new_dispute_event) and replace the inline Tags::from_list
block in the current code with a call to build_dispute_tags; alternatively
broaden publish_dispute_event to accept initiator: &str (instead of bool) and
call it here to handle tag creation and sending (reusing
get_nostr_client/new_dispute_event inside that function) so the duplicate
Tag::custom and client.send_event logic is removed.
As suggested by @arkanoider, extracted the duplicate dispute closure logic from cancel.rs and release.rs into a new shared function close_dispute_after_user_resolution() in dispute.rs. This eliminates ~100 lines of duplicated code while maintaining the same best-effort error handling behavior.
Mostrica
force-pushed
the
fix/close-dispute-on-release
branch
from
d61b55e to
574c363
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/release.rs (1)
1-1:close_dispute_after_user_resolution(insrc/app/dispute.rs) is missing a///doc comment.The function is
pub async fnwith non-obvious behaviour: best-effort DB update, conditional Nostr event publishing, and silent skip when no dispute exists. Per coding guidelines, non-obvious public APIs should carry///doc comments.📝 Suggested doc comment
+/// Closes an active dispute tied to `order` after the users themselves resolved the +/// trade (cooperative cancel or seller release). +/// +/// Updates the dispute's status to `new_status` in the database and publishes an +/// updated replaceable Nostr dispute event so admin clients reflect the resolution. +/// +/// This is a **best-effort** operation: failures are logged but never propagated, +/// so the primary order flow is never blocked by a dispute-closure error. +/// If no dispute is associated with the order the function is a no-op. +/// +/// # Arguments +/// * `context` – Short label used in log messages (e.g. `"release"`, `"cooperative cancel"`). pub async fn close_dispute_after_user_resolution(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/release.rs` at line 1, Add a triple-slash doc comment above the public async function close_dispute_after_user_resolution in src/app/dispute.rs that documents its non-obvious behavior: describe that it performs a best-effort database update (may fail without panicking), conditionally publishes a Nostr event when applicable, and silently returns/does nothing if the dispute does not exist; also mention the function is async, its parameters' role, and any error/return semantics so callers know it won't always produce an error on missing disputes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/release.rs`:
- Line 1: Add a triple-slash doc comment above the public async function
close_dispute_after_user_resolution in src/app/dispute.rs that documents its
non-obvious behavior: describe that it performs a best-effort database update
(may fail without panicking), conditionally publishes a Nostr event when
applicable, and silently returns/does nothing if the dispute does not exist;
also mention the function is async, its parameters' role, and any error/return
semantics so callers know it won't always produce an error on missing disputes.
Collaborator
|
Ok @Mostrica! Tested and works, can you just add a warn message in case of match missing and an info print of dispute event? pub async fn close_dispute_after_user_resolution(
pool: &Pool<Sqlite>,
order: &Order,
new_status: DisputeStatus,
my_keys: &Keys,
context: &str,
) {
if let Ok(mut dispute) = find_dispute_by_order_id(pool, order.id).await {
let dispute_id = dispute.id;
dispute.status = new_status.to_string();
if let Err(e) = dispute.update(pool).await {
tracing::error!(
"Failed to update dispute {} status after {}: {}",
dispute_id,
context,
e
);
} else {
tracing::info!(
"Dispute {} closed automatically after {} of order {}",
dispute_id,
context,
order.id
);
// Determine who initiated the dispute for the event tag
let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) {
(true, false) => "seller",
(false, true) => "buyer",
_ => {
tracing::warn!(
"Dispute {} for order {} has inconsistent dispute flags (seller={}, buyer={}); \
publishing initiator as 'unknown'",
dispute_id, order.id,
order.seller_dispute, order.buyer_dispute,
);
"unknown"
}
};
// Publish updated dispute event to Nostr so admin clients see it as resolved
let tags = Tags::from_list(vec![
Tag::custom(
TagKind::Custom(Cow::Borrowed("s")),
vec![new_status.to_string()],
),
Tag::custom(
TagKind::Custom(Cow::Borrowed("initiator")),
vec![dispute_initiator.to_string()],
),
Tag::custom(
TagKind::Custom(Cow::Borrowed("y")),
vec!["mostro".to_string()],
),
Tag::custom(
TagKind::Custom(Cow::Borrowed("z")),
vec!["dispute".to_string()],
),
]);
match new_dispute_event(my_keys, "", dispute_id.to_string(), tags) {
Ok(event) => match get_nostr_client() {
Ok(client) => {
tracing::info!("Publishing dispute close event: {:#?}", event);
if let Err(e) = client.send_event(&event).await {
tracing::error!("Failed to publish dispute close event: {}", e);
}
}
Err(e) => {
tracing::error!("Failed to get Nostr client for dispute event: {}", e);
}
},
Err(e) => {
tracing::error!(
"Failed to create dispute close event for dispute {}: {}",
dispute_id,
e
);
}
}
}
}
} |
As requested by @arkanoider: - Added tracing::warn! when dispute flags are inconsistent - Added tracing::info! to log the dispute close event before publishing
Contributor
Author
|
@arkanoider Done! Added both:
Commit: ff25d51 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/dispute.rs (1)
298-337: Consider reusingpublish_dispute_eventto reduce intra-file duplication.Lines 299-337 rebuild the same four Nostr tags (
s,initiator,y,z) and repeat thenew_dispute_event→get_nostr_client→send_eventchain thatpublish_dispute_event(lines 20-82) already encapsulates. The only differences are (1) how the initiator string is determined and (2) error handling (Result vs. best-effort).A lightweight refactor: change
publish_dispute_eventto acceptinitiator: &strinstead ofis_buyer_dispute: bool, then call it from here and swallow the error.♻️ Sketch
-async fn publish_dispute_event( - dispute: &Dispute, - my_keys: &Keys, - is_buyer_dispute: bool, -) -> Result<(), MostroError> { - let initiator = match is_buyer_dispute { - true => "buyer", - false => "seller", - }; +async fn publish_dispute_event( + dispute: &Dispute, + my_keys: &Keys, + initiator: &str, +) -> Result<(), MostroError> { // ... rest unchanged ...In
dispute_action, derive the string before calling:- publish_dispute_event(&dispute, my_keys, is_buyer_dispute) + let initiator = if is_buyer_dispute { "buyer" } else { "seller" }; + publish_dispute_event(&dispute, my_keys, initiator)Then in
close_dispute_after_user_resolution, replace lines 298-337 with:+ if let Err(e) = publish_dispute_event(&dispute, my_keys, dispute_initiator).await { + tracing::error!("Failed to publish dispute close event: {}", e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dispute.rs` around lines 298 - 337, This block rebuilds Nostr tags and duplicates the new_dispute_event → get_nostr_client → send_event flow already implemented by publish_dispute_event; change publish_dispute_event signature to accept initiator: &str (instead of is_buyer_dispute: bool), update its internal tag construction to use that initiator string, then in close_dispute_after_user_resolution derive the initiator string the same way dispute_action does and call publish_dispute_event(&my_keys, dispute_id.to_string(), &initiator). Call it in a best-effort way (ignore/log the Result) instead of reimplementing new_dispute_event/get_nostr_client/send_event here to eliminate duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/dispute.rs`:
- Around line 298-337: This block rebuilds Nostr tags and duplicates the
new_dispute_event → get_nostr_client → send_event flow already implemented by
publish_dispute_event; change publish_dispute_event signature to accept
initiator: &str (instead of is_buyer_dispute: bool), update its internal tag
construction to use that initiator string, then in
close_dispute_after_user_resolution derive the initiator string the same way
dispute_action does and call publish_dispute_event(&my_keys,
dispute_id.to_string(), &initiator). Call it in a best-effort way (ignore/log
the Result) instead of reimplementing
new_dispute_event/get_nostr_client/send_event here to eliminate duplication.
Collaborator
|
last detail @Mostrica : remove unused error import |
Contributor
Author
|
@arkanoider Already fixed in the current head ( You might be looking at an older commit. Current state: use tracing::info; // line 11 - no error import |
arkanoider
approved these changes
Feb 20, 2026
Collaborator
arkanoider
left a comment
arkanoider
left a comment
There was a problem hiding this comment.
Missing update on dispute status worka, dispute event is sent correctly with new state and mostro db ia updated too.
tACK.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Clean refactor, LGTM ✅
The extraction of close_dispute_after_user_resolution() is a solid DRY improvement — centralizes the dispute close logic that was previously duplicated (and now needed in two places). The status mapping is semantically correct (SellerRefunded for cooperative cancel, Settled for release), and the best-effort error handling is the right call since the primary operation must not be blocked by dispute bookkeeping.
Good additions:
- The
tracing::warn!on inconsistent dispute flags is a nice improvement over the existing silent"unknown"pattern - Updated doc consolidating both scenarios is clear and well-structured
- All CodeRabbit feedback was properly addressed
CI passes, no conflicts. Ready to merge.
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
4 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.
Summary
When an order with an active dispute is released by the seller, the dispute status should be updated to
settledand a Nostr event should be published so admin clients see it as resolved.Problem
Currently, if a seller releases funds while a dispute is active:
settled-hold-invoice→successinitiatedorin-progressThis is the same issue fixed in #577 for cooperative cancellation, but for the release flow.
Solution
Add dispute closure logic to
release_action()insrc/app/release.rs, similar to what was done incancel.rs:Settled(since sats went to buyer)Changes
find_dispute_by_order_idimport fromcrate::dbnew_dispute_eventimport fromcrate::nip33settle_seller_hold_invoice()succeedsCloses #605
Summary by CodeRabbit
New Features
Documentation