feat(cashu): Track C — cooperative cancel in Cashu mode#764
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCashu-mode cancel processing is now supported through a mode-aware escrow abstraction: CashuBackend provides a cancel no-op, cancel paths use return_escrow_to_seller, and run_cashu dispatches Action::Cancel to cancel_action_cashu which injects CashuBackend (no LND). ChangesCashu-mode cancel routing with escrow abstraction
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 review |
✅ Actions performedReview triggered.
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
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.rs (1)
398-403:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
run_cashu's contract comment.This branch now handles
Action::Cancel, but therun_cashudoc block above still says cancel falls through and is ignored. Please refresh it so the public API description matches the dispatch table. As per coding guidelines "Document non-obvious public APIs with///doc comments".Also applies to: 460-471
🤖 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/app.rs` around lines 398 - 403, Update the doc comment for the public function run_cashu so it accurately reflects the current dispatch behavior: state that Action::Cancel is now handled (via handle_message_action_no_ln) instead of falling through/being ignored, and clarify which actions still fall through to the `_` arm (e.g., Release, AdminCancel, AdminSettle) until EscrowBackend is implemented; make the same wording update in the other similar doc block that describes the cashu-mode event loop so the public API docs match the actual dispatch table.
🤖 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/app/cancel.rs`:
- Around line 521-528: Add an upfront mode guard in cancel_action_cashu to fail
fast when the app is not running in Cashu-only escrow mode: before calling
cancel_action_generic, check Settings::escrow_mode() (or the app config
accessor) and return an appropriate MostroError (or Err variant) if the mode is
not the Cashu variant; this prevents injecting CashuBackend and proceeding when
Lightning mode is configured and ensures you don’t noop cancel_hold_invoice
while marking an order canceled.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 398-403: Update the doc comment for the public function run_cashu
so it accurately reflects the current dispatch behavior: state that
Action::Cancel is now handled (via handle_message_action_no_ln) instead of
falling through/being ignored, and clarify which actions still fall through to
the `_` arm (e.g., Release, AdminCancel, AdminSettle) until EscrowBackend is
implemented; make the same wording update in the other similar doc block that
describes the cashu-mode event loop so the public API docs match the actual
dispatch table.
🪄 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: 1bcf5c0e-16bb-4dee-8c6e-4a1d8b883e0d
📒 Files selected for processing (2)
src/app.rssrc/app/cancel.rs
|
@codex review |
4d27848 to
998b88d
Compare
There was a problem hiding this comment.
I reviewed the current head of this PR and I am requesting changes.
There are two blocking issues.
- CI / quality bar: clippy is red on this branch.
cargo clippy --all-targets --all-features -- -D warnings fails in src/cashu/mod.rs, including:
- dead code on
CashuClient::mint_url - useless same-type conversions like
let err: cdk::error::Error = e.into(); Error::Client(cdk::error::Error::from(e))
That alone makes the branch not ready to merge.
- The PR claims Cashu mode means "LND not required", but the codebase still has important Lightning-specific paths instantiated directly outside a fully enforced backend seam.
Examples from this head:
src/main.rslogsStarting in Cashu escrow mode (LND not required)- but
src/util.rsstill has directLndConnector::new().await?paths inshow_hold_invoiceandinvoice_subscribe src/app/bond/flow.rsstill createsLndConnectordirectly- scheduler jobs are partially gated, but the broader codebase is still in a mixed mode state rather than a fully enforced backend boundary
So even though this PR is much closer than the earlier Cashu groundwork, I still do not think the current state cleanly matches the operational claim that Cashu mode does not require LND. The mode split and the backend seam are still not applied consistently enough for me to approve this yet.
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
Blocking
src/cashu/mod.rsstill has clippy failures that will trip the branch quality gate (cargo clippy --all-targets --all-features -- -D warnings). The concrete offenders reported by the existing review are the unusedCashuClient::mint_urlfield and redundant error conversions inconnect(),check_state(), andverify_token_dleq(). Please fix those before merge.
Non-blocking
- The earlier Cashu-mode guard concern appears resolved; I’m not re-flagging it.
src/app.rsstill has a stale doc comment forrun_cashuthat saysAction::Cancelfalls through, even though the branch now handles it explicitly.
998b88d to
0fecce2
Compare
|
Addressed in the latest push:
On the LND seam: agreed it isn't fully enforced ( |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Track C · Cooperative cancel (Cashu 2-of-3 escrow)
Wires cooperative cancel for Cashu escrow mode, per
docs/CASHU_ESCROW_ARCHITECTURE.md. In Cashu mode Mostro is only a coordinator and never takes custody, so there is no hold invoice to cancel: the daemon records the cancel and transitions state, while the buyer hands their signature to the seller P2P (NIP-59 DM) so the seller reconstructs aP_S + P_B2-of-3 swap to reclaim the locked ecash. Lightning behavior is unchanged.Depends on F1 (mostro-core protocol) and F3 (
EscrowBackendseam). Independent of Tracks A/B/D.What's in this PR
src/app/cancel.rsreturn_escrow_to_sellerhelper: the single place that returns escrowed funds on a cancel. Lightning cancels the seller's hold invoice (custodial); Cashu is a no-op (non-custodial, P2P reclaim). All three cancel sites (cooperative, taker-cancel, maker-cancel) route through it, so fund-return follows one consistent mode rule.cancel_action_cashuentry point: drives the same cancel state machine without an LND client, using a no-opCashuBackendas theCancelLightningimpl.src/app.rs—run_cashunow routesAction::Canceltocancel_action_cashuinstead of rejecting it withInvalidAction. Other trade actions (NewOrder/TakeSell/TakeBuy/AddInvoice/Release/Admin*) stay rejected until their tracks land.Notes
mostro-coreactions/payloads: the cancel signature is exchanged client↔client over NIP-59, never relayed through Mostro.cancel/releaseonto the F3EscrowBackendtrait (todaycancel.rsuses the older localCancelLightningtrait) once Tracks A/B/D are in and the full pattern is visible.Tests
cargo test --bin mostrod cancelgreen, incl. newcashu_backend_cancel_hold_invoice_is_noop.Summary by CodeRabbit
Bug Fixes
Refactor