feat(cashu): Track A — Cashu 2-of-3 escrow lock end-to-end#765
Conversation
Implements Track A from docs/CASHU_ESCROW_ARCHITECTURE.md: the Cashu
analogue of paying the Lightning hold invoice. Mostro stays a pure
coordinator (1 of 3 keys, never custodial) — it only validates the
seller-submitted token and advances the order.
- add_cashu_escrow.rs: `AddCashuEscrow` handler. Requires WaitingPayment +
sender==seller, derives the authoritative {P_B,P_S,P_M} from the order's
trade pubkeys and Mostro's identity key (never trusting the pubkeys the
seller states in the proof), validates the token via
CashuClient::verify_escrow_token (2-of-3 over the right keys, bound to the
configured mint, exact amount, all proofs unspent), then atomically
CAS-advances WaitingPayment→Active and notifies both parties
(CashuEscrowLocked) so the buyer can send fiat.
- take_sell/take_buy: branch on is_cashu_enabled() before the hold-invoice
path and call util::show_cashu_escrow_request, which sets WaitingPayment,
persists, publishes the order event, and prompts the seller to fund the
token (reusing WaitingSellerToPay + SmallOrder carrying the trade pubkeys).
- app.rs run_cashu: route NewOrder/TakeSell/TakeBuy through the no-LN
dispatcher (they are now Cashu-aware); keep AddInvoice/Release/Cancel/
AdminCancel/AdminSettle rejected (AddInvoice would hit the unimplemented
CashuBackend::lock; release/cancel/dispute are Tracks B/C/D).
- escrow seam plumbing: AppContext carries Arc<dyn EscrowBackend> + optional
CashuClient; show_hold_invoice / settle_seller_hold_invoice go through the
backend; main/rpc build the Lightning backend, main connects the mint and
attaches the Cashu client in Cashu mode.
- cashu/mod.rs: verify_escrow_token + cashu_pubkey_from_xonly_hex helpers.
The escrow lock value is exactly order.amount (no Mostro fee added): Mostro
holds only 1 of 3 keys and cannot split a fee out of the redeemed ecash, so
Cashu-mode fee collection is left as an open question per the spec.
Track A lock is implemented as a dedicated handler outside the EscrowBackend
seam (seam stays LN-shaped for Tracks B/C/D), per design decision.
cargo fmt + clippy --all-targets --all-features -D warnings clean; 388 unit
tests + cashu mint integration test pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThis PR redesigns the escrow handling architecture by replacing mutable connector-based primitives with an immutable, context-driven ChangesCashu Escrow & Escrow Backend Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: f3de52b138
ℹ️ 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".
| if Settings::is_cashu_enabled() { | ||
| tracing::info!("Starting in Cashu escrow mode (LND not required)"); | ||
| let escrow: Arc<dyn escrow::EscrowBackend> = Arc::new(cashu::CashuBackend::new()); | ||
| let ctx = AppContext::new( |
There was a problem hiding this comment.
When [cashu].enabled = true, this branch constructs AppContext and immediately calls run_cashu, but AppContext::new initializes cashu_client to None. A repo-wide search shows that CashuClient::connect and with_cashu_client are never called. Consequently, every seller submission reaches add_cashu_escrow_action and fails with AddCashuEscrow dispatched without a Cashu client, so no Cashu order can advance from WaitingPayment to Active. Connect to the configured mint URL and attach the resulting client before entering run_cashu.
Useful? React with 👍 / 👎.
| .map(|s| cdk::dhke::hash_to_curve(s.as_bytes())) | ||
| .collect::<Result<Vec<PublicKey>, _>>() | ||
| .map_err(|e| Error::Token(format!("proof Y: {e}")))?; | ||
| let states = self.check_state(ys).await?; |
There was a problem hiding this comment.
Once the startup wiring is fixed, a malicious seller can submit a fabricated token and make the buyer send fiat without locking mint-backed funds. This path checks only the declared amount, P2PK secret structure, configured mint URL, and /v1/checkstate; as the comment on check_state itself notes, that endpoint proves only that secrets are unspent and does not authenticate mint signatures. An attacker can choose previously unseen secrets with the expected 2-of-3 condition, arbitrary proof points, and the required amount; an honest mint reports those unknown secrets as unspent, and the order is accepted. Require DLEQ-backed issuance verification or another mint-authenticating validation before persisting the escrow and transitioning to Active.
Useful? React with 👍 / 👎.
Two P1 issues from the Codex review of #765: 1. Attach the Cashu client before the event loop. The Cashu boot branch built AppContext but never called CashuClient::connect / with_cashu_client, so cashu_client stayed None and every AddCashuEscrow failed with "dispatched without a Cashu client" — no order could reach Active. main now connects the configured mint and attaches the client, panicking if the mint is unreachable (mirroring how Lightning mode refuses to boot without LND). 2. Authenticate proofs as mint-issued before activating the order. verify_escrow_token relied on /v1/checkstate, which reports any unknown secret as Unspent — a seller could fabricate proofs with the right 2-of-3 condition and amount but no mint signature, pass every check, and trick the buyer into sending fiat against worthless ecash. Now verify the NUT-12 DLEQ proof of every proof (verify_token_dleq) before the unspent check. cargo fmt + clippy --all-targets --all-features -D warnings clean; 388 unit tests + cashu mint integration test pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f532ef062c
ℹ️ 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".
| // 3: the locked amount must equal the order amount exactly. | ||
| let value = token | ||
| .value() | ||
| .map_err(|e| Error::Token(format!("token value: {e}")))? | ||
| .to_u64(); | ||
| if value != expected_amount { |
There was a problem hiding this comment.
When the configured mint exposes more than one currency unit, this accepts a token whose numeric value matches order.amount without checking that its unit is sat. Cashu tokens carry an explicit unit, and keysets may use units such as msat; for example, a 100,000-sat order can be advanced to Active with a valid 100,000-msat token worth only 100 sats. Verify the parsed token unit before persisting the lock.
Useful? React with 👍 / 👎.
| @@ -59,19 +61,29 @@ impl CashuClient { | |||
| Ok(info) => { | |||
| if !info.nuts.nut11.supported { | |||
There was a problem hiding this comment.
When a mint advertises NUT-11 but not NUT-07 or NUT-12, Cashu mode boots successfully even though every AddCashuEscrow request later fails: verify_escrow_token unconditionally calls /v1/checkstate and requires a DLEQ proof. These are separate optional Cashu extensions, so accepting a NUT-11-only mint leaves all taken orders stuck in WaitingPayment. Reject startup unless the mint also advertises the capabilities required by the lock path.
Useful? React with 👍 / 👎.
| pub async fn verify_token_dleq(&self, token: &Token) -> Result<(), Error> { | ||
| let keysets = self.client.get_mint_keys().await.map_err(|e| Error::Client(cdk::error::Error::from(e)))?; | ||
| let keysets = self.client.get_mint_keys().await.map_err(Error::Client)?; | ||
|
|
||
| match token { | ||
| Token::TokenV3(token_v3) => { | ||
| let proofs = token_v3.token.iter().flat_map(|t| t.proofs.clone()).collect::<Vec<_>>(); | ||
| let proofs = token_v3 | ||
| .token | ||
| .iter() | ||
| .flat_map(|t| t.proofs.clone()) | ||
| .collect::<Vec<_>>(); | ||
| for proof in proofs { | ||
| let keyset = keysets.iter().find(|k| ShortKeysetId::from(k.id) == proof.keyset_id) | ||
| let keyset = keysets | ||
| .iter() | ||
| .find(|k| ShortKeysetId::from(k.id) == proof.keyset_id) | ||
| .ok_or_else(|| Error::Token("Unknown keyset".into()))?; |
There was a problem hiding this comment.
When the mint rotates keysets after an escrow token is created, this fetches only /v1/keys and reports Unknown keyset for the still-valid token. The Cashu NUT-01 specification states that /v1/keys returns only active keysets while proofs from inactive keysets remain accepted as inputs; keys for a specific active or inactive keyset must be requested via /v1/keys/{keyset_id}. Resolve each proof's keyset ID through that endpoint so a rotation does not prevent a seller from locking valid funds.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app.rs (1)
204-265:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
AddCashuEscrowexplicitly outside Cashu mode.In the normal
run()path this action falls into the default arm and returnsOk(()), so unsupported Cashu-lock requests get silently ignored instead of returningInvalidAction.Suggested fix
Action::PayInvoice => Err(MostroError::MostroCantDo(CantDoReason::InvalidAction).into()), + Action::AddCashuEscrow => { + Err(MostroError::MostroCantDo(CantDoReason::InvalidAction).into()) + } Action::LastTradeIndex => last_trade_index(ctx, msg, event, my_keys) .await .map_err(|e| e.into()),🤖 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 204 - 265, handle_message_action_no_ln currently falls through Cashu-specific requests like Action::AddCashuEscrow to the wildcard and returns Ok(()), silently ignoring them; update handle_message_action_no_ln to explicitly match Action::AddCashuEscrow and return Err(MostroError::MostroCantDo(CantDoReason::InvalidAction).into()) so Cashu-lock requests are rejected when not in Cashu mode. Locate the match in handle_message_action_no_ln and add a branch for Action::AddCashuEscrow that returns the InvalidAction error (mirroring how Action::PayInvoice is handled).src/app/take_sell.rs (1)
166-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip LN invoice validation on the Cashu path.
validate_invoice()still runs before the Cashu-mode branch, soTakeSellcan fail on an irrelevant Lightning invoice even though the flow immediately switches toshow_cashu_escrow_requestand never usespayment_request.Suggested fix
- // Validate invoice and get payment request if present - // NOW dev_fee is set correctly for proper validation - let payment_request = validate_invoice(&msg, &order).await?; - let trade_index = match msg.get_inner_message_kind().trade_index { Some(trade_index) => trade_index, None => { @@ if Settings::is_cashu_enabled() { show_cashu_escrow_request(my_keys, &event.sender, &seller_pubkey, order, request_id) .await?; return Ok(()); } + + // Validate invoice only on the LN path. + let payment_request = validate_invoice(&msg, &order).await?; // If payment request is not present, update order status to waiting buyer invoice if payment_request.is_none() { update_order_status(&mut order, my_keys, pool, request_id).await?;🤖 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/take_sell.rs` around lines 166 - 222, The code currently calls validate_invoice(&msg, &order).await? before checking Settings::is_cashu_enabled(), causing Cashu-mode takes to fail on irrelevant LN invoice validation; change the flow to skip validate_invoice when cashu is enabled by either: (a) moving the call to validate_invoice so it runs only after the cashu/bond branches (i.e. after checking bond_required and Settings::is_cashu_enabled()), or (b) wrapping it in if !Settings::is_cashu_enabled() { let payment_request = validate_invoice(&msg, &order).await?; } and ensuring payment_request (used by bond::request_taker_bond and later non-bond logic) is conditionally available when needed; update references to payment_request accordingly and keep show_cashu_escrow_request, update_user_trade_index, and bond::request_taker_bond logic unchanged.
🧹 Nitpick comments (1)
src/util.rs (1)
886-968: 💤 Low valueUnused variable
locked_amount.
locked_amountis assigned on line 918 but never used afterward. The comment explains the intent (the valueadd_cashu_escrow_actionvalidates against), but the variable itself is dead code in this function—the assignment tonew_order.amounton line 941 could useorder.amountdirectly.♻️ Suggested fix
- let locked_amount = order.amount; - order.status = Status::WaitingPayment.to_string(); ... - new_order.amount = locked_amount; + new_order.amount = order.amount;🤖 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.rs` around lines 886 - 968, The function show_cashu_escrow_request declares locked_amount but never uses it; remove the dead variable and instead set new_order.amount directly from order.amount (or use locked_amount consistently if you prefer) so the value validated by add_cashu_escrow_action is preserved; update the assignment at new_order.amount to use order.amount and delete the locked_amount binding to eliminate the unused-variable lint.
🤖 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/cashu/mod.rs`:
- Around line 208-219: The code currently only checks that returned states are
Unspent but doesn't verify the mint returned as many state entries as requests;
after creating ys and calling self.check_state(ys) ensure that
states.states.len() equals the number of requested proofs (e.g., secrets.len()
or ys.len()) and return an Err (e.g., Error::Token with a clear message like
"mismatched states length" or "missing state entries") if it is shorter, before
iterating to check State::Unspent; reference the variables/functions ys,
self.check_state, and states.states/State::Unspent to locate where to add this
length check.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 204-265: handle_message_action_no_ln currently falls through
Cashu-specific requests like Action::AddCashuEscrow to the wildcard and returns
Ok(()), silently ignoring them; update handle_message_action_no_ln to explicitly
match Action::AddCashuEscrow and return
Err(MostroError::MostroCantDo(CantDoReason::InvalidAction).into()) so Cashu-lock
requests are rejected when not in Cashu mode. Locate the match in
handle_message_action_no_ln and add a branch for Action::AddCashuEscrow that
returns the InvalidAction error (mirroring how Action::PayInvoice is handled).
In `@src/app/take_sell.rs`:
- Around line 166-222: The code currently calls validate_invoice(&msg,
&order).await? before checking Settings::is_cashu_enabled(), causing Cashu-mode
takes to fail on irrelevant LN invoice validation; change the flow to skip
validate_invoice when cashu is enabled by either: (a) moving the call to
validate_invoice so it runs only after the cashu/bond branches (i.e. after
checking bond_required and Settings::is_cashu_enabled()), or (b) wrapping it in
if !Settings::is_cashu_enabled() { let payment_request = validate_invoice(&msg,
&order).await?; } and ensuring payment_request (used by bond::request_taker_bond
and later non-bond logic) is conditionally available when needed; update
references to payment_request accordingly and keep show_cashu_escrow_request,
update_user_trade_index, and bond::request_taker_bond logic unchanged.
---
Nitpick comments:
In `@src/util.rs`:
- Around line 886-968: The function show_cashu_escrow_request declares
locked_amount but never uses it; remove the dead variable and instead set
new_order.amount directly from order.amount (or use locked_amount consistently
if you prefer) so the value validated by add_cashu_escrow_action is preserved;
update the assignment at new_order.amount to use order.amount and delete the
locked_amount binding to eliminate the unused-variable lint.
🪄 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: 8ce906e2-0fe4-42ce-b004-f45aecc40591
📒 Files selected for processing (14)
src/app.rssrc/app/add_cashu_escrow.rssrc/app/add_invoice.rssrc/app/admin_settle.rssrc/app/bond/flow.rssrc/app/context.rssrc/app/release.rssrc/app/take_buy.rssrc/app/take_sell.rssrc/cashu/mod.rssrc/escrow.rssrc/main.rssrc/rpc/service.rssrc/util.rs
| let ys = secrets | ||
| .iter() | ||
| .map(|s| cdk::dhke::hash_to_curve(s.as_bytes())) | ||
| .collect::<Result<Vec<PublicKey>, _>>() | ||
| .map_err(|e| Error::Token(format!("proof Y: {e}")))?; | ||
| let states = self.check_state(ys).await?; | ||
| if states | ||
| .states | ||
| .iter() | ||
| .any(|s| s.state != cdk::nuts::State::Unspent) | ||
| { | ||
| return Err(Error::Token("one or more proofs are not unspent".into())); |
There was a problem hiding this comment.
Fail closed if /checkstate returns fewer entries than requested.
This only rejects when a returned state is not Unspent. If the mint responds with a shorter states vector, the missing proofs are treated as implicitly valid and the escrow can be accepted with some proofs never checked.
Suggested fix
- let states = self.check_state(ys).await?;
+ let expected_states = ys.len();
+ let states = self.check_state(ys).await?;
+ if states.states.len() != expected_states {
+ return Err(Error::Token(format!(
+ "checkstate returned {} states for {expected_states} proofs",
+ states.states.len()
+ )));
+ }
if states
.states
.iter()
.any(|s| s.state != cdk::nuts::State::Unspent)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ys = secrets | |
| .iter() | |
| .map(|s| cdk::dhke::hash_to_curve(s.as_bytes())) | |
| .collect::<Result<Vec<PublicKey>, _>>() | |
| .map_err(|e| Error::Token(format!("proof Y: {e}")))?; | |
| let states = self.check_state(ys).await?; | |
| if states | |
| .states | |
| .iter() | |
| .any(|s| s.state != cdk::nuts::State::Unspent) | |
| { | |
| return Err(Error::Token("one or more proofs are not unspent".into())); | |
| let ys = secrets | |
| .iter() | |
| .map(|s| cdk::dhke::hash_to_curve(s.as_bytes())) | |
| .collect::<Result<Vec<PublicKey>, _>>() | |
| .map_err(|e| Error::Token(format!("proof Y: {e}")))?; | |
| let expected_states = ys.len(); | |
| let states = self.check_state(ys).await?; | |
| if states.states.len() != expected_states { | |
| return Err(Error::Token(format!( | |
| "checkstate returned {} states for {expected_states} proofs", | |
| states.states.len() | |
| ))); | |
| } | |
| if states | |
| .states | |
| .iter() | |
| .any(|s| s.state != cdk::nuts::State::Unspent) | |
| { | |
| return Err(Error::Token("one or more proofs are not unspent".into())); |
🤖 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/cashu/mod.rs` around lines 208 - 219, The code currently only checks that
returned states are Unspent but doesn't verify the mint returned as many state
entries as requests; after creating ys and calling self.check_state(ys) ensure
that states.states.len() equals the number of requested proofs (e.g.,
secrets.len() or ys.len()) and return an Err (e.g., Error::Token with a clear
message like "mismatched states length" or "missing state entries") if it is
shorter, before iterating to check State::Unspent; reference the
variables/functions ys, self.check_state, and states.states/State::Unspent to
locate where to add this length check.
1 participant
Track A — Cashu 2-of-3 escrow lock
Implements Track A from
docs/CASHU_ESCROW_ARCHITECTURE.md: the Cashu analogue of the seller paying the Lightning hold invoice. Mostro stays a pure coordinator — it holds only 1 of the 3 keys, never takes custody — and only validates the seller-submitted token before advancing the order.Flow
take (cashu mode) → WaitingPayment → [seller builds & submits 2-of-3 token] → AddCashuEscrow → Active → buyer notified to send fiatWhat's in this PR
src/app/add_cashu_escrow.rs—Action::AddCashuEscrowhandler. RequiresWaitingPayment+sender == seller, derives the authoritative{P_B, P_S, P_M}from the order's trade pubkeys and Mostro's identity key (never trusting the pubkeys the seller states in the proof), validates viaCashuClient::verify_escrow_token(2-of-3 over the right keys, bound to the configured mint, exact amount, all proofs unspent), then atomically CAS-advancesWaitingPayment → Activeand emitsCashuEscrowLockedto both parties.take_sell/take_buy— branch onis_cashu_enabled()before the hold-invoice path and callutil::show_cashu_escrow_request, which setsWaitingPayment, persists, publishes the order event, and prompts the seller to fund the token (reusingWaitingSellerToPay+SmallOrdercarrying the trade pubkeys).app.rsrun_cashu— routesNewOrder/TakeSell/TakeBuythrough the no-LN dispatcher (now Cashu-aware); keepsAddInvoice/Release/Cancel/AdminCancel/AdminSettlerejected (AddInvoicewould hit the unimplementedCashuBackend::lock; the rest belong to Tracks B/C/D).AppContextcarriesArc<dyn EscrowBackend>+ optionalCashuClient;show_hold_invoice/settle_seller_hold_invoicego through the backend;main/rpcbuild the Lightning backend, andmainconnects the mint + attaches the Cashu client in Cashu mode.cashu/mod.rs—verify_escrow_token+cashu_pubkey_from_xonly_hexhelpers.Notable fixes during review
run_cashupreviously rejectedNewOrder/TakeSell/TakeBuy, so in Cashu mode no order could be created or taken — thetake_*Cashu branches were dead code. Now routed correctly.amount + fee, but the lock handler validatestoken value == order.amountexactly — every lock would have failed. The escrow now locks exactlyorder.amount.Design notes
EscrowBackendseam (the seam stays LN-shaped for Tracks B/C/D), per design decision.Out of scope (follow-ups)
restore_session/ scheduler timeout for in-flight Cashu orders (the F5find_locked_cashu_ordershelper exists but isn't wired) — Integration & Hardening.Verification
cargo fmt+cargo clippy --all-targets --all-features -D warningsclean; 388 unit tests + Cashu mint integration test pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements