Feat: antiabuse bond enabled and bond slash activation in admin finalization#79
Merged
Merged
Conversation
…nvoice. Parse bond_enabled from kind-38385 instance info to hide the finalize Bond button when bonds are off. Wire trader AddBondInvoice notifications with invoice input, execute_add_bond_invoice, and updated docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Admin settle/cancel now use request_id, wait_for_dm, and handle_mostro_response so CantDo errors surface before DB updates. Finalize popup shows Admin settle/cancel inside action buttons; success popup uses a structured multi-line message with word wrap. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
WalkthroughThis PR implements complete anti-abuse bond support: instance-level bond capability detection, admin dispute finalization with conditional bond-slash UI and DM request/response execution, trader-side bond-payout invoice popups with async submission, and dynamic operation-result message rendering with proper text wrapping. ChangesAnti-abuse bond finalization and payout support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
src/ui/orders.rs (1)
365-370: ⚡ Quick winAdd focused unit tests for the new
AddBondInvoicegating branches.These branches decide popup eligibility and acting-party routing. A couple of targeted tests for
invoice_popup_allowed_for_order_statusandlocal_user_must_act_on_invoice_popupwould lock this behavior and prevent regressions.Also applies to: 392-392
🤖 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 `@src/ui/orders.rs` around lines 365 - 370, Add focused unit tests for the AddBondInvoice gating logic: write tests for invoice_popup_allowed_for_order_status and local_user_must_act_on_invoice_popup that assert the popup is allowed for statuses other than Canceled and CanceledByAdmin and disallowed for those two, and verify routing/acting-party behavior (local user must act vs remote) for each relevant Status variant; reference the Action::AddBondInvoice branch and the functions invoice_popup_allowed_for_order_status and local_user_must_act_on_invoice_popup when adding these targeted tests to prevent regressions.
🤖 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 `@src/util/order_utils/execute_add_invoice.rs`:
- Around line 87-99: The call to wait_for_dm currently returns an error on
timeout which causes AddBondInvoice flows to fail; change the logic around
wait_for_dm(&order_trade_keys, FETCH_EVENTS_TIMEOUT, sent_message).await so that
if it returns a timeout-style error you treat it like an acceptable no-response
for Action::AddBondInvoice (return Ok(())) and only propagate non-timeout
errors; keep the existing parse_dm_events(...) and messages.first() handling for
non-timeout successful receipts. Ensure you identify the timeout by matching the
error variant or message from wait_for_dm and reference wait_for_dm,
FETCH_EVENTS_TIMEOUT, sent_message, Action::AddBondInvoice, parse_dm_events, and
messages.first() in your change.
In `@src/util/order_utils/execute_admin_cancel.rs`:
- Around line 63-65: Remove the secret key derivation and logging: stop calling
admin_keys.secret_key() and delete the admin_keys_log variable and
log::info!("admin_keys dispute: {}", admin_keys_log); instead, if logging is
needed use a non-sensitive identifier such as
admin_keys.public_key().to_bech32() or an admin ID/alias; update any references
to admin_keys_log in this scope (in the execute_admin_cancel flow) accordingly
so no private key material is emitted to logs.
In `@src/util/order_utils/execute_take_dispute.rs`:
- Around line 63-65: Remove the secret-key exposure: do not call
admin_keys.secret_key() or log admin_keys_log in execute_take_dispute.rs;
instead log a non-sensitive identifier (e.g.,
admin_keys.public_key().to_bech32() or an admin_keys.id() / redacted
placeholder) and delete the log::info!("admin_keys dispute: {}", admin_keys_log)
line so secret material is never read or printed from the admin_keys value.
---
Nitpick comments:
In `@src/ui/orders.rs`:
- Around line 365-370: Add focused unit tests for the AddBondInvoice gating
logic: write tests for invoice_popup_allowed_for_order_status and
local_user_must_act_on_invoice_popup that assert the popup is allowed for
statuses other than Canceled and CanceledByAdmin and disallowed for those two,
and verify routing/acting-party behavior (local user must act vs remote) for
each relevant Status variant; reference the Action::AddBondInvoice branch and
the functions invoice_popup_allowed_for_order_status and
local_user_must_act_on_invoice_popup when adding these targeted tests to prevent
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea7fd460-1d85-460c-9d8d-9c285c798aea
📒 Files selected for processing (30)
README.mddocs/ADMIN_DISPUTES.mddocs/FINALIZE_DISPUTES.mddocs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/TUI_INTERFACE.mdsrc/main.rssrc/ui/dispute_finalization_confirm.rssrc/ui/dispute_finalization_popup.rssrc/ui/key_handler/admin_handlers.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/message_notification.rssrc/ui/operation_result.rssrc/ui/orders.rssrc/ui/tabs/mostro_info_tab.rssrc/util/dm_utils/mod.rssrc/util/dm_utils/notifications_ch_mng.rssrc/util/dm_utils/order_ch_mng.rssrc/util/mod.rssrc/util/mostro_info.rssrc/util/order_utils/bond_resolution.rssrc/util/order_utils/execute_add_invoice.rssrc/util/order_utils/execute_admin_cancel.rssrc/util/order_utils/execute_admin_settle.rssrc/util/order_utils/execute_take_dispute.rssrc/util/order_utils/helper.rssrc/util/order_utils/mod.rs
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hermes Agent Review
Verdict: Request changes
Critical
admin_keys.secret_key()is being converted to bech32 and written to application logs inexecute_take_dispute.rsandexecute_admin_cancel.rs. That leaks the admin private key into logs. Please remove those log lines and log only non-sensitive identifiers.
Warning
- The
AddBondInvoicebranch inexecute_add_invoice.rsreturns success even when Mostro sends no follow-up DM, and it also returnsOk(())for any response that passeshandle_mostro_responsewithout checking the expected success action. That can report "Bond payout invoice sent successfully" for a silent failure or a wrong-action reply. Please keep timeout as an error and validate the expected success action before returning success.
There was a problem hiding this comment.
Hermes Agent Review
Verdict: Approve
I re-checked the latest head (ccff795) and the earlier blockers are resolved:
- The admin private key is no longer being logged.
AdminSettle/AdminCancelnow wait for and validate the Mostro response before updating local state.- The bond payout flow is wired through the UI and DM handlers cleanly.
I also ran the test suite locally: 90 passed, 0 failed.
Nice cleanup.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found a blocker in the new AddBondInvoice execution path.
execute_payment_request_reply() is still strict for AddInvoice, but for AddBondInvoice it now returns Ok(()) in all of these cases:
wait_for_dm(...)times out- no parsed response message is found
- any parsed response passes
handle_mostro_response(...), regardless of the returned action
That last part is the real problem:
let inner_message = handle_mostro_response(response_message, request_id)?;
if action == Action::AddBondInvoice {
return Ok(());
}So AddBondInvoice currently treats every request-id-matching non-CantDo reply as success, without checking that Mostro actually acknowledged the bond payout invoice with the expected action.
This is looser than the existing AddInvoice path and can produce a false local success state if:
- Mostro replies with an unexpected action
- the wrong phase message is matched first
- or the daemon behavior changes and still sends a request-id-matching response that is not the intended confirmation
I am also not fully comfortable with treating both timeout and empty-response as unconditional success for this path, but even setting that aside, the missing action validation is enough to block for me.
I think AddBondInvoice needs the same kind of explicit response contract that AddInvoice and the updated admin finalize flows now use: accept only the expected acknowledgement action(s), and fail otherwise.
Because this can incorrectly report success for a bond payout invoice that was not actually confirmed the way the client expects, I am marking this as request changes.
After bond payout bolt11, wait_for_dm and map Mostro follow-ups (e.g. WaitingBuyerInvoice, PayBondInvoice) to OpenInvoicePopup or PaymentRequestRequired instead of stopping at a success toast. Treat DM timeout as acceptance for AddBondInvoice only. Document the flow; box large OperationResult payloads to satisfy clippy. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Reviewed the new commit. The blocker is addressed.
The AddBondInvoice path is now fully separated from the generic execute_payment_request_reply and lives in its own execute_bond_payment_request_reply. The timeout and empty-response cases now return Ok(None) with a descriptive log instead of a silent Ok(()), and any real response from Mostro goes through handle_mostro_response first and then through the action match in operation_result_from_bond_invoice_reply. So the path is no longer unconditionally successful regardless of what Mostro replies.
The response contract is explicit now: known follow-up actions chain into the next UI step; anything else logs and returns None rather than failing hard, which is a reasonable choice while the protocol is still being defined. That is exactly the kind of defensiveness I was asking for.
No remaining blocker from my side.
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)
src/ui/operation_result.rs (1)
21-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle overlong single tokens in wrapper to avoid clipped content.
Current wrapping only breaks on whitespace. Long tokens (e.g., UUIDs / bolt11) can exceed width, get visually clipped, and make
info_popup_heightunderestimate space.💡 Proposed fix
fn wrap_message_lines(message: &str, width: usize) -> Vec<Line<'static>> { let wrap_width = width.max(10); let mut lines = Vec::new(); @@ let mut current = String::new(); for word in paragraph.split_whitespace() { + // Fallback for overlong single tokens (UUIDs, invoices, etc.). + if word.len() > wrap_width { + if !current.is_empty() { + lines.push(Line::from(std::mem::take(&mut current))); + } + let mut chunk = String::new(); + for ch in word.chars() { + chunk.push(ch); + if chunk.len() >= wrap_width { + lines.push(Line::from(std::mem::take(&mut chunk))); + } + } + if !chunk.is_empty() { + current = chunk; + } + continue; + } if current.is_empty() { current = word.to_string(); } else if current.len() + 1 + word.len() <= wrap_width { current.push(' '); current.push_str(word);Also applies to: 42-44, 193-194, 216-217
🤖 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 `@src/ui/operation_result.rs` around lines 21 - 33, The current word-wrapping loop in src/ui/operation_result.rs (uses variables paragraph, current, wrap_width, lines and constructs Line::from) only breaks at whitespace, so single tokens longer than wrap_width get clipped; modify the loop to detect when a token's length > wrap_width and split that token into wrap_width-sized chunks (appending the first chunk to current or as its own Line::from if current is empty, and pushing subsequent chunks as separate lines), and when adding a chunk to a non-empty current ensure proper spacing/transition using std::mem::take(&mut current); apply the same chunking logic to the other wrap sites referenced (the similar blocks around lines noted in the comment) so overlong tokens are split rather than overflowing.
🧹 Nitpick comments (1)
src/util/order_utils/execute_add_invoice.rs (1)
19-22: ⚡ Quick winMake timeout detection less brittle than full-string equality.
Matching the entire error string risks breaking timeout tolerance on tiny message wording changes.
💡 Proposed fix
fn is_wait_for_dm_timeout(err: &anyhow::Error) -> bool { - err.to_string() == "Timeout waiting for DM or gift wrap event" + err.to_string().contains("Timeout waiting for DM") }🤖 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 `@src/util/order_utils/execute_add_invoice.rs` around lines 19 - 22, The current is_wait_for_dm_timeout function is brittle because it compares the whole error string; change it to robustly detect timeout by checking the error chain for a matching substring or cause instead — e.g., update is_wait_for_dm_timeout(err: &anyhow::Error) to return true if err.chain().any(|e| e.to_string().contains("Timeout waiting for DM")) (or use starts_with/contains for a stable prefix like "Timeout waiting for DM") so small wording changes won't break detection.
🤖 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 `@src/ui/operation_result.rs`:
- Around line 21-33: The current word-wrapping loop in
src/ui/operation_result.rs (uses variables paragraph, current, wrap_width, lines
and constructs Line::from) only breaks at whitespace, so single tokens longer
than wrap_width get clipped; modify the loop to detect when a token's length >
wrap_width and split that token into wrap_width-sized chunks (appending the
first chunk to current or as its own Line::from if current is empty, and pushing
subsequent chunks as separate lines), and when adding a chunk to a non-empty
current ensure proper spacing/transition using std::mem::take(&mut current);
apply the same chunking logic to the other wrap sites referenced (the similar
blocks around lines noted in the comment) so overlong tokens are split rather
than overflowing.
---
Nitpick comments:
In `@src/util/order_utils/execute_add_invoice.rs`:
- Around line 19-22: The current is_wait_for_dm_timeout function is brittle
because it compares the whole error string; change it to robustly detect timeout
by checking the error chain for a matching substring or cause instead — e.g.,
update is_wait_for_dm_timeout(err: &anyhow::Error) to return true if
err.chain().any(|e| e.to_string().contains("Timeout waiting for DM")) (or use
starts_with/contains for a stable prefix like "Timeout waiting for DM") so small
wording changes won't break detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e90bbd03-75e8-4ff8-92c1-dbd637e0c6e5
📒 Files selected for processing (19)
docs/FINALIZE_DISPUTES.mddocs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/TUI_INTERFACE.mdsrc/main.rssrc/ui/app_state.rssrc/ui/key_handler/admin_handlers.rssrc/ui/key_handler/async_tasks.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/operation_result.rssrc/ui/orders.rssrc/util/dm_utils/notifications_ch_mng.rssrc/util/dm_utils/order_ch_mng.rssrc/util/order_utils/execute_add_invoice.rssrc/util/order_utils/execute_admin_cancel.rs
💤 Files with no reviewable changes (1)
- src/util/order_utils/execute_admin_cancel.rs
✅ Files skipped from review due to trivial changes (3)
- docs/README.md
- docs/TUI_INTERFACE.md
- docs/MESSAGE_FLOW_AND_PROTOCOL.md
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.
Summary
bond_enabled(kind 38385): Bond slash button and confirm recap only when Mostro advertisesbond_enabled: true(mostro_info.rs).AddBondInvoice: Bond payout invoice popup andexecute_add_bond_invoiceflow (aligned with existingAddInvoicepatterns).execute_admin_settle/execute_admin_cancelsend withrequest_id,wait_for_dm, andhandle_mostro_response—CantDo(e.g. invalid bond slash) returns an error before updatingadmin_disputes.BondSlashChoice::finalize_success_messagewith word-boundary wrapping inoperation_result.rs.Motivation
Mostro can reject admin finalize with
CantDo; previously Mostrix updated local state after send-only. Long UUIDs also broke the success toast mid-word.Summary by CodeRabbit
New Features
Bug Fixes
Documentation