Include dispute initiator in Nostr dispute events #563
Conversation
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin handler (cancel/settle/take)
participant Publish as publish_dispute_event
participant Nostr as Nostr client/relay
Admin->>Publish: call publish_dispute_event(dispute, my_keys, is_buyer_dispute)
Publish->>Publish: compute initiator ("buyer" / "seller"), build tags (s, y, z, initiator)
Publish->>Nostr: send Nostr event with tags
Nostr-->>Publish: ack / result
Publish-->>Admin: return Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/dispute.rs (1)
16-29: Update function documentation to reflect the new parameter.The function docstring should mention the new
is_buyer_disputeparameter and the initiator tag that's now included in the published event. This helps maintainability and makes the function's behavior clearer to other developers.📝 Proposed documentation update
/// Publishes a dispute event to the Nostr network. /// /// Creates and publishes a NIP-33 replaceable event containing dispute details -/// including status and application metadata. +/// including status, initiator (buyer or seller), and application metadata. +/// +/// # Arguments +/// * `dispute` - The dispute to publish +/// * `my_keys` - The keys to sign the event with +/// * `is_buyer_dispute` - True if the buyer initiated the dispute, false if the seller did async fn publish_dispute_event( dispute: &Dispute, my_keys: &Keys, is_buyer_dispute: bool, ) -> Result<(), MostroError> {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/dispute.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/dispute.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/dispute.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/dispute.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/dispute.rs
🔇 Additional comments (2)
src/app/dispute.rs (2)
38-41: LGTM: Initiator tag correctly added.The new initiator tag follows the same pattern as existing custom tags and is properly constructed. The tag will make dispute events easier to parse and reason about for clients.
235-237: LGTM: Call site correctly updated.The function call correctly passes the
is_buyer_disputeparameter, which was already validated earlier in the function throughget_counterpart_info. The error handling remains properly structured.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/dispute.rs (1)
25-29: Consider simplifying the boolean-to-string conversion.The match expression is correct, but a more idiomatic Rust approach would be a simple if-expression.
♻️ Suggested simplification
- // Create initiator string - let initiator = match is_buyer_dispute { - true => "buyer", - false => "seller", - }; + // Create initiator string + let initiator = if is_buyer_dispute { "buyer" } else { "seller" };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/dispute.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/dispute.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/dispute.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/dispute.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/dispute.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
src/app/dispute.rs (2)
18-24: LGTM! Clear function signature enhancement.The updated documentation and function signature clearly convey the new initiator functionality. The boolean parameter approach is type-safe and integrates well with the existing validation logic upstream.
235-235: LGTM! Correct parameter propagation.The call site correctly passes the validated
is_buyer_disputeparameter, which was properly derived fromget_counterpart_infoearlier in the function.
There was a problem hiding this comment.
Hi @arkanoider The initiator is shown only in the dispute event in the initiated status, but not in the others.
It's important that the initiator appears in all dispute events with all status Because if the user restores their sessions while the dispute is in status in-progress settled or seller-refunded the app won't know who the initiator was if the initiator isn't listed.
Ah ah! Got it, is supposed the fix that the guy made in the issue was what we needed. Ok we need initiator in every event update of dispute right? Will do it! |
|
Now dispute creator is show in the events of:
So also with restore user can request events of dispute with initiator always inclued, let me know @Catrya if this is ok. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/app/admin_cancel.rs:
- Around line 75-79: The current logic sets dispute_initiator based only on
order.buyer_dispute which can misidentify the initiator if both or neither flags
are set; update admin_cancel.rs to validate that exactly one of
order.buyer_dispute and order.seller_dispute is true (same pattern used in
admin_take_dispute.rs), return an error if both or neither are true, and then
set the dispute_initiator variable to "buyer" when order.buyer_dispute is true
or "seller" when order.seller_dispute is true.
In @src/app/admin_settle.rs:
- Line 119: Fix the typo in the inline comment "// Print event dispute with
upidate" by changing "upidate" to "update" so it reads "// Print event dispute
with update"; locate and update that comment in src/app/admin_settle.rs (near
the code handling event dispute printing).
- Around line 81-85: The current dispute_initiator assignment only inspects
order.buyer_dispute and can yield incorrect results when both or neither dispute
flags are set; replicate the validation used in admin_take_dispute.rs (lines
58–71): explicitly check order.buyer_dispute and order.seller_dispute for three
invalid cases (both false -> return an error, both true -> return an error) and
only then set dispute_initiator = "buyer" if buyer_dispute is true or "seller"
if seller_dispute is true, using the same error type/response pattern as
admin_take_dispute.rs to fail fast on invalid dispute states.
In @src/app/admin_take_dispute.rs:
- Around line 214-218: The dispute_initiator determination currently only checks
order.buyer_dispute and can misidentify initiator when flags are inconsistent;
replace that logic in the admin_take_dispute handler by matching on both
(order.seller_dispute, order.buyer_dispute) like prepare_solver_info_message
does: return "seller" for (true, false), "buyer" for (false, true), and return
an Err (or appropriate error path) for any other combination so
dispute_initiator is only set when exactly one flag is true.
🧹 Nitpick comments (1)
src/app/dispute.rs (1)
1-241: Consider extracting initiator derivation to a helper function.The logic to derive
dispute_initiatorfrom order flags is duplicated acrossadmin_take_dispute.rs,admin_cancel.rs, andadmin_settle.rs. Extracting this into a shared helper function would reduce duplication and ensure consistency.Example helper function that could be added to a utility module:
/// Derives the dispute initiator from order flags. /// Returns "buyer" if buyer initiated, "seller" if seller initiated. /// Returns error if dispute state is invalid. fn get_dispute_initiator(order: &Order) -> Result<&'static str, MostroError> { match (order.seller_dispute, order.buyer_dispute) { (true, false) => Ok("seller"), (false, true) => Ok("buyer"), (_, _) => Err(MostroInternalErr(ServiceError::DisputeEventError)), } }Then use it as:
let dispute_initiator = get_dispute_initiator(&order)?;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/admin_cancel.rssrc/app/admin_settle.rssrc/app/admin_take_dispute.rssrc/app/dispute.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/admin_cancel.rssrc/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/admin_settle.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/admin_cancel.rssrc/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/admin_settle.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/admin_cancel.rssrc/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/admin_settle.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/admin_cancel.rssrc/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/admin_settle.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/admin_take_dispute.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/app/admin_cancel.rs (1)
112-113: Good addition for observability.The info log before publishing the dispute event improves debugging and auditability.
src/app/admin_settle.rs (1)
120-120: Good addition for observability.The tracing log before publishing improves debugging and auditability.
src/app/dispute.rs (3)
16-30: LGTM! Clean implementation of initiator tracking.The function signature extension with
is_buyer_dispute: booland the initiator derivation logic are clear and correct. The docstring accurately reflects the changes.
38-42: LGTM! Correct addition of initiator tag.The custom "initiator" tag follows the NIP-33 pattern and correctly includes the derived initiator value in the event tags.
236-236: LGTM! Correct propagation of initiator information.The call site correctly passes the validated
is_buyer_disputevalue topublish_dispute_event.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/admin_settle.rs (1)
81-86: Consider moving initiator determination inside the dispute check.The
dispute_initiatoris computed before verifying that a dispute exists (line 88). While not incorrect, moving this logic inside theif let Ok(mut d) = disputeblock would:
- Only compute the initiator when it's actually needed
- Make the error case (invalid flags) only trigger when there's a real dispute with corrupted state
- Improve code locality by grouping dispute-related logic together
♻️ Proposed refactor
// we check if there is a dispute let dispute = find_dispute_by_order_id(pool, order.id).await; -// Get the creator of the dispute -let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), -}; - if let Ok(mut d) = dispute { + // Get the creator of the dispute + let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { + (true, false) => "seller", + (false, true) => "buyer", + (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), + }; + let dispute_id = d.id;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/admin_cancel.rssrc/app/admin_settle.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/admin_cancel.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/admin_settle.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/admin_settle.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/admin_settle.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/admin_settle.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/app/admin_settle.rs (2)
101-105: LGTM!The initiator tag is correctly constructed and follows the same pattern as the other custom tags in the dispute event. The tag name and value align with the PR objectives to track who initiated the dispute.
120-122: LGTM!Good addition of logging before publishing the dispute event. Using
tracing::info!with pretty-print debug format provides valuable observability for the dispute settlement flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/admin_take_dispute.rs (1)
214-219: Consider extracting the initiator derivation logic to eliminate duplication.This match pattern on
(order.seller_dispute, order.buyer_dispute)is duplicated from lines 58-71 inprepare_solver_info_message. Extracting this logic into a helper method (e.g.,Order::get_dispute_initiator()or a standalone function) would improve maintainability and ensure consistency if the validation logic ever changes.♻️ Proposed refactor: extract to Order method
Add a method to the
Ordertype (likely in the appropriate module):impl Order { /// Returns "buyer" if buyer initiated dispute, "seller" if seller initiated, /// or error if dispute state is invalid. pub fn get_dispute_initiator(&self) -> Result<&'static str, MostroError> { match (self.seller_dispute, self.buyer_dispute) { (true, false) => Ok("seller"), (false, true) => Ok("buyer"), (_, _) => Err(MostroInternalErr(ServiceError::DisputeEventError)), } } }Then replace both occurrences (lines 58-71 and lines 214-219) with:
- let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), - }; + let dispute_initiator = order.get_dispute_initiator()?;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/admin_take_dispute.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/admin_take_dispute.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/admin_take_dispute.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/admin_take_dispute.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/admin_take_dispute.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/app/admin_take_dispute.rs (1)
227-231: LGTM! Initiator tag correctly implements the PR objective.The addition of the "initiator" custom tag with values "buyer" or "seller" correctly implements the PR requirement to include who initiated the dispute in all dispute event updates. This enables clients that restore sessions to determine the dispute initiator.
|
Moved to a safer placer the initiator getter in admin settle file, now we know that a dispute exist for real. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/admin_settle.rs (1)
51-51: Fix typo in comment."orde" should be "order".
📝 Proposed fix
- // Was orde cooperatively cancelled? + // Was order cooperatively cancelled?
🤖 Fix all issues with AI agents
In @src/app/admin_settle.rs:
- Around line 89-94: The code constructs a new String from the &str
dispute_initiator (e.g., via dispute_initiator.to_string()), causing an
unnecessary allocation; change call sites to use the &str directly (pass
dispute_initiator) or wrap as std::borrow::Cow::Borrowed(dispute_initiator) or
call to_owned() only if a String is truly required, locating usages around the
dispute_initiator variable set in the match (seller/buyer) and where it is
converted to a String.
🧹 Nitpick comments (2)
src/app/admin_settle.rs (2)
102-106: Avoid unnecessary allocation in tag value.
dispute_initiatoris already a&str, so calling.to_string()creates an unnecessary heap allocation. You can pass the string slice directly:♻️ Proposed fix
// Who is the dispute creator Tag::custom( TagKind::Custom(std::borrow::Cow::Borrowed("initiator")), - vec![dispute_initiator.to_string()], + vec![dispute_initiator], ),
121-122: Consider using tracing span instead of ad-hoc logging.As per coding guidelines: "Prefer
tracingspans over ad-hoc logging in Rust code." Consider wrapping the dispute settlement logic in a span for better structured observability. Also, the comment says "Print" when it actually logs, and using{:#?}on the entire event may expose sensitive information or be overly verbose.♻️ Suggested improvement
+ let _span = tracing::info_span!("publish_dispute_settlement", + dispute_id = %dispute_id, + order_id = %order.id, + initiator = %dispute_initiator + ).entered(); + - // Print event dispute with update - tracing::info!("Dispute event to be published: {event:#?}"); + tracing::info!("Publishing dispute settlement event");This provides structured data without logging the entire event object.
Based on coding guidelines, prefer tracing spans over ad-hoc logging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_settle.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/admin_settle.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/admin_settle.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/admin_settle.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/admin_settle.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/admin_settle.rs (1)
88-94: Extract the duplicate initiator computation into a shared helper function.The dispute initiator derivation logic (matching on
order.seller_disputeandorder.buyer_dispute) is duplicated insrc/app/admin_take_dispute.rs(lines 215-218) andsrc/app/admin_cancel.rs. Consider extracting this into a helper function (e.g.,get_dispute_initiator(&order) -> Result<&str, MostroError>) to eliminate duplication and ensure consistent behavior across all dispute event handlers.♻️ Proposed shared helper function
Add to a common utility module (e.g.,
src/app/dispute.rsorsrc/util.rs):/// Determines the dispute initiator from order flags. /// /// # Returns /// - `Ok("seller")` if seller initiated the dispute /// - `Ok("buyer")` if buyer initiated the dispute /// - `Err(DisputeEventError)` if flags are ambiguous or invalid pub fn get_dispute_initiator(order: &Order) -> Result<&'static str, MostroError> { match (order.seller_dispute, order.buyer_dispute) { (true, false) => Ok("seller"), (false, true) => Ok("buyer"), (_, _) => Err(MostroInternalErr(ServiceError::DisputeEventError)), } }Then replace the inline match in this file:
- // Get the creator of the dispute - let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), - }; + // Get the creator of the dispute + let dispute_initiator = get_dispute_initiator(&order)?;Apply the same change in
src/app/admin_take_dispute.rsandsrc/app/admin_cancel.rs.src/app/admin_take_dispute.rs (1)
214-219: Extract the duplicate initiator computation into a shared helper function.This logic is identical to the code in
src/app/admin_settle.rs(lines 90-93) andsrc/app/admin_cancel.rs. As noted in the PR discussion, consolidating this into a shared helper function would improve maintainability and ensure consistent error handling across all dispute handlers.Refer to the proposed refactor in the
src/app/admin_settle.rsreview for a concrete implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/admin_cancel.rssrc/app/admin_settle.rssrc/app/admin_take_dispute.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/admin_cancel.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/admin_take_dispute.rssrc/app/admin_settle.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/app/admin_take_dispute.rssrc/app/admin_settle.rs
src/app/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/directory where applicable
Files:
src/app/admin_take_dispute.rssrc/app/admin_settle.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/app/admin_take_dispute.rssrc/app/admin_settle.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/admin_take_dispute.rs
📚 Learning: 2025-12-23T17:33:03.588Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T17:33:03.588Z
Learning: Link the motivating issue in pull requests, include `cargo test` output in PR descriptions, and call out schema or config changes
Applied to files:
src/app/admin_settle.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
src/app/admin_settle.rs (3)
51-51: LGTM! Typo corrected.The comment typo has been fixed appropriately.
102-106: LGTM! Initiator tag added correctly.The custom
initiatortag is properly constructed and inserted into the dispute event tags, ensuring clients can identify the dispute creator during session restoration.
121-122: LGTM! Helpful observability logging added.The debug-formatted event log will aid in troubleshooting dispute settlement flows.
src/app/admin_take_dispute.rs (1)
227-231: LGTM! Initiator tag added correctly.The
initiatortag is properly included in the dispute event, consistent with the implementation in other dispute handlers and aligned with the PR objective to expose the dispute creator in all dispute status events.
Updated PR Description
Summary
This PR fixes GitHub issue #548 by ensuring Nostr dispute events explicitly include who initiated the dispute (buyer or seller).
Details
initiatorcustom tag to NIP-33 dispute events with value"buyer"or"seller".Motivation
Having the dispute initiator in the event metadata makes it easier for clients and tooling to reason about disputes, improves auditability, and directly addresses the requirements described in issue #548.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.