feat(cashu): C5 — seller escrow lock flow (Track A) - #238
Conversation
Phase C5 of docs/cashu/README.md: the Cashu replacement for "the seller pays the hold invoice". Verified against the daemon's own Track A branches (feat/cashu-ta2-take-flow, feat/cashu-ta1f-fee-token) rather than inferred. What the daemon actually does, confirmed by reading it: - the escrow request is `Action::WaitingSellerToPay` + `Payload::Order` with status WaitingPayment, both trade pubkeys and no buyer invoice; - the escrow token must be worth *exactly* order.amount; - the fee token is `2 * order.fee`, where order.fee is `round(fee * amount / 2)` — the daemon rounds the half, so doubling the rounded half is the only expression that agrees with it, and a satoshi of disagreement is a rejection; - the proof's stated pubkeys must equal the order's trade keys, which the daemon re-derives rather than trusting. Rust - `mostro/node_fee.rs`: the node's advertised fee, read from the same 38385 fetch as PoW and the escrow mode, cleared on node switch. `total_fee_sats` reproduces the daemon's rounding exactly; a malformed fee is discarded rather than stored, because a wrong fee is an unexplained lock failure later. - `mostro/actions.rs`: `add_cashu_escrow` builder beside `add_invoice`. - `api/cashu.rs`: `cashu_escrow_quote` (amount, fee, total, balance, mint, locktime — shown before the seller commits) and `lock_escrow`, which refuses below `amount + fee`, builds the escrow and fee tokens, verifies the escrow it just built with the same check the daemon runs, publishes, and persists. Persistence deliberately follows the mint swap and survives a failed publish: the ecash is already committed by then, and a token we did not record is money we cannot find again. - `TradeInfo` gains `cashu_mint_url` / `cashu_escrow_token` / `cashu_locked_at`, `#[serde(default)]` so older rows still load. - A node switch now also drops the fee and disconnects the wallet, which was bound to the previous node's mint. Dart - `lock_escrow_screen.dart`: the Cashu sibling of the pay-invoice screen — escrow, fee, total against the balance, mint, and when the seller can reclaim unilaterally. Short balance offers "fund your wallet" instead of a failure. - The take flow branches on `isCashuAvailable`: buyer straight to the trade (there is no invoice step in Cashu mode), seller to the lock screen. - Markers mapped to localized strings in all five locales, unknown ones falling back so no internal string reaches a user. Also fixes a real test-isolation bug found here: `api::escrow` and `api::cashu` serialized the same globals with two different mutexes, which fails only under parallel execution. One global, one lock (`escrow_mode::test_lock`). Stacked on C1b (#234), C2 (#235), C4 (#236) and C3 (#237).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
✨ 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 |
Review — C5 (strict pass)
Scope reviewed: 🛑 Critical1. The buyer's key is taken from the wrong field — the lock is rejected on both paths — let buyer_hex = trade.counterparty_pubkey.clone();
The second case is worse than a failed lock: the funds are committed at the mint before the rejection arrives, and the escrow is unspendable by the intended buyer. The right source is the escrow request itself. The daemon's new_order.buyer_trade_pubkey = Some(buyer_pubkey.to_string());
new_order.seller_trade_pubkey = Some(seller_pubkey.to_string());
🔴 Major2. The locktime is computed at exactly the daemon's floor, and evaluated earlier than the daemon evaluates it — let locktime = now_secs() + locktime_days * SECONDS_PER_DAY;The daemon's check is Add a margin (an hour is invisible to the user and larger than any plausible propagation delay), or derive the locktime from a timestamp the daemon supplied. 3. The fee is computed from the node's current advertised rate, not the order's stored fee
The arithmetic itself is right and the rounding note is the good part of this PR — but the input is the wrong one. If the order does not carry its fee to the client today, that is worth raising upstream; until then the failure mode deserves an explicit error rather than a generic 4. The escrow is committed before the fee token is built —
5.
6.
🟡 Minor7. No re-submit path after a failed publish. The code correctly persists the token even when the publish fails, and correctly notes the daemon's handler is idempotent — but nothing in the UI ever calls 8. 9. ✅ What is right
Test gapsThe Rust tests cover the gate (
|
…-flow # Conflicts: # lib/l10n/app_de.arb # lib/l10n/app_en.arb # lib/l10n/app_es.arb # lib/l10n/app_fr.arb # lib/l10n/app_it.arb # lib/l10n/app_localizations.dart # lib/l10n/app_localizations_de.dart # lib/l10n/app_localizations_en.dart # lib/l10n/app_localizations_es.dart # lib/l10n/app_localizations_fr.dart # lib/l10n/app_localizations_it.dart
…yer key Addresses the strict review on #238. Critical - `lock_escrow` took the buyer key from `TradeInfo.counterparty_pubkey`, which is written once at construction and never updated: empty for a maker seller (so a user who *created* the sell order could never fund the escrow) and the maker's order-book key for a taker seller — not the per-order trade key the daemon validates against. The taker case was the worse one: the daemon rejects, but only after the ecash has been swapped into a token locked to a key the buyer does not hold. The daemon states both keys in the escrow request (`SmallOrder`'s `buyer_trade_pubkey` / `seller_trade_pubkey`) and the client was discarding them. They are now carried through `classify_take_reply` for the taker path, captured from the status-sync arm for the maker path (where the request arrives as an ordinary message), persisted on `TradeInfo`, and read from there. `lock_escrow` also refuses when the stored seller key is not the one this device holds, rather than building an escrow nobody can spend. Major - The locktime matched the daemon's floor exactly, computed with *our* clock while the daemon evaluates its own later — every lock was a race against the publish delay, lost with the funds already swapped. A one-hour margin is invisible to a seller and larger than any plausible propagation. - `cashu_escrow_quote` read the balance without connecting, so an unconnected wallet reported zero and a fully funded seller was told they had insufficient funds. It connects first, and an unreadable balance is an error rather than a zero. - The escrow token was built before the fee token, so a fee failure stranded the whole escrow. The fee — the smaller and cheaper of the two — is built first. - `take_order_screen` read `isCashuAvailableProvider` while it was still loading, routing a Cashu seller to a hold-invoice screen that would never fill. It awaits the provider now. Minor - A lock that failed *after* the mint swap now offers a retry, with the reason stated: the token is persisted and the daemon's handler is idempotent, so retrying is the only way out of a lost publish. Failures raised before any funds move deliberately do not offer it. - `node_fee::set_fee` caps the fraction at 1.0; a malformed `2.0` tag would have produced a fee token twice the escrow. - `now_secs` returns an error instead of 0 for a pre-epoch clock, which would have built a 1970 locktime and surfaced as an unexplained condition error. - The marker→message mapping is shared with the wallet screen. Note on the fee input: `SmallOrder` carries no fee, so the client must still derive it from the node's advertised rate. An operator who changes the fee between order creation and the lock will invalidate in-flight orders; that is recorded as a known limitation rather than silently accepted. Tests: the buyer key must come from the escrow request and not the order book; the locktime clears a floor evaluated a minute later; and a widget suite for the lock screen — quote shown before committing, funding offered on a short balance, a marker explained rather than printed, and retry offered only after the swap.
… doc Four findings were valid, three were not. Fixed - `MostroInstance.parseEscrowMode` used `getOptional`, so a *present but blank* `escrow_mode` tag read as `unknown` while Rust's `parse_tags` reads the same event as `Lightning`. Two parsers, one event, different answers — the About screen and the Cashu gate could disagree about the same daemon. Now only an absent (or value-less) tag is unknown, matching Rust exactly. - The dev card's post-frame seed captured `mintUrlOverride` during build and applied it after the frame. An override arriving in that gap is applied by `ref.listen` first, and the captured copy then overwrote it with the older value. The callback reads the current provider value instead. - `api::escrow::snapshot` derived `mode` from one `get_resolved()` and `is_cashu_available` from `is_cashu_mode()`, which reads the globals again — a node switch between the two produced a snapshot whose mode and gate disagreed. `ResolvedEscrowMode::is_cashu_usable()` now expresses the gate against a value the caller already holds, and `is_cashu_mode()` is that applied to the current globals. - `set_from_tags` logged unconditionally while only notifying on a change, so a reconnect that confirmed what we already knew still wrote an info line. Moved inside the `changed` branch. - The C1 "Done when" in the plan still said flipping the override flips "the About section", contradicting the bullet directly above it. About reports what the node advertised; the override moves the resolved mode and the dev card's effective state, nothing else. Not fixed, with reasons - The shared test lock is already in place (`escrow_mode::test_lock`, used by both `api::escrow` and `api::cashu`); the finding describes the pre-fix state. - The `hintText` is the literal `http://localhost:3338`, identical in every locale, on a `kDebugMode`-only card. Five ARB entries that translate nothing is maintenance without a reader. - French `aboutDaysValue`: CLDR puts 0 in the `one` category for French, so "0 jour" is correct and the suggested change would introduce an error. The generated file is also not the source of truth and must not be hand-edited. Tests: blank / whitespace / value-less / absent `escrow_mode` all pinned against the Rust behaviour, and a widget suite for the dev card covering seeding, a newer override winning, and in-progress typing surviving a no-op event.
Running the `#[ignore]`d suite against a real nutshell for the first time found that it could not pass at all, and then that it could not pass twice. - Every funds-using test asserted "fund the wallet first" and returned. There was no way for a reviewer to satisfy that, so five tests were documentation rather than verification. `CashuWallet::mint_for_test` mints from the mint itself, which settles instantly against a FakeWallet backend. - Fixed seeds plus a fresh DB per run replay the same NUT-13 blinding secrets, and the mint answers "Blinded Message is already signed" on the second run. Seeds are now unique per process and per call. - Two assertions pinned amounts the mint is free to reduce: nutshell's default keyset charges a swap fee, so an 8 sat token redeems for 7 and locking 16 sat costs the seller more than 16. The face value is what the daemon validates, so the assertions bound the redeemed amount instead of pinning it. - `one_signature_is_not_enough_to_move_an_escrow` expected the mint to refuse a premature reclaim. Since the C4 round the client refuses first, with the time remaining — a better message for the same property. Accepts either. Verified: 8/8 against nutshell 0.20.3, twice in a row. Note that nutshell rate limits by default; the suite needs MINT_RATE_LIMIT=FALSE to run back to back. Worth flagging for review rather than fixing here: a mint that charges swap fees means a seller needs slightly more than `amount + mostro_fee` to lock an escrow, and `cashu_escrow_quote` does not account for that.
…-flow # Conflicts: # rust/src/cashu/escrow.rs # rust/src/cashu/wallet.rs
|
@coderabbitai Review |
✅ Action performedReview finished.
|
1 participant
Phase C5 of
docs/cashu/README.md— the Cashu replacement for "the sellerpays the hold invoice", and the first end-to-end flow.
Read the daemon, didn't guess it
The plan flagged the escrow request's exact shape as unconfirmed. It is now
confirmed against the daemon's own branches (
feat/cashu-ta2-take-flow,feat/cashu-ta1f-fee-token):Action::WaitingSellerToPay+Payload::Order, statusWaitingPayment, both trade pubkeys, no buyer invoice — so it isclassified by payload shape, not by a new action;
order.amount;2 * order.fee, whereorder.fee = round(fee * amount / 2).The daemon rounds the half, so doubling the rounded half is the only
expression that agrees with it —
round(fee * amount)differs by a satoshi onhalf of all amounts, and every one of those would be rejected;
re-derives rather than trusting.
Rust
mostro/node_fee.rs— the node's advertised fee, read from the same 38385fetch that already yields PoW and the escrow mode, cleared on node switch. A
malformed fee is discarded rather than stored: a wrong fee surfaces much later
as an unexplained lock failure.
mostro/actions.rs—add_cashu_escrow, besideadd_invoice.api/cashu.rs—cashu_escrow_quote(amount, fee, total, balance, mint,locktime, all shown before the seller commits) and
lock_escrow:amount + fee;runs — catching a mistake here means the seller learns before publishing,
not after;
Persistence deliberately follows the mint swap and happens even if the
publish fails: the ecash is already committed by then, and a token we did
not record is money we cannot find again. The daemon's handler is idempotent
on re-submission, which is what makes that safe.
TradeInfogainscashu_mint_url/cashu_escrow_token/cashu_locked_at,#[serde(default)]so rows written by older builds load.A node switch now also drops the fee and disconnects the wallet, which was
bound to the previous node's mint.
Dart
lock_escrow_screen.dart— the Cashu sibling of the pay-invoice screen: escrow,fee and total against the balance, the mint, and when the seller can reclaim
unilaterally if Mostro vanishes. A short balance offers "fund your wallet"
rather than a failure. The take flow branches on
isCashuAvailable: the buyergoes straight to the trade (there is no invoice step in Cashu mode), the seller
to the lock screen. Strings in all five locales; unknown markers fall back.
A real bug this phase surfaced
api::escrowandapi::cashuserialized the same globals with two differentmutexes. That only fails under parallel execution and reads as flakiness. Fixed
properly: one global, one lock (
escrow_mode::test_lock).Verification
./scripts/frb-generate.shrun after therust/src/api/changes. The wasm stubgrew the escrow surface so the bridge stays one codebase; it still refuses with
CashuUnsupportedOnWeb.Test plan
lock_escrowreturns
CashuNotEnabled.screen shows amount/fee/total; confirm; daemon accepts and the buyer is
told to send fiat.
order.feeon an amountwhere the halves round differently (e.g. 500 sat at 0.6%) — this is the
single most likely thing to be wrong.
recorded against the trade, and re-submitting is accepted (the daemon's
replay path).
Remaining: C6 (release), C7 (cooperative cancel), C8 (disputes), C9 (web
storage), C10 (resilience/backup).
Manual verification
Be honest about the ceiling here: the escrow lock cannot be exercised
end to end without a
mostrodrunning Cashu mode, and no public node does. Sothis PR splits into what can be proven today (the crypto, the arithmetic, the
gate, the Lightning regression) and what must wait for the daemon (an accepted
AddCashuEscrow). Both are listed, and the second is the reviewer's real risk.Already run on this branch
Plus the integration suite against a real mint — 8/8, twice in a row
(nutshell 0.20.3), covering the escrow primitives this flow calls into.
A · The fee arithmetic — check this first
This is the single most likely thing to be wrong in production, because a
one-satoshi disagreement with the daemon is a rejection with no useful message.
The rule is
2 * round(fee * amount / 2)— the daemon rounds the half, sodoubling the rounded half is the only expression that agrees with it.
the_total_fee_is_twice_the_rounded_halfpins 500 sat at 0.6%, where the naiveround(fee * amount)gives 3 and the correct answer is 4.Cross-check against a daemon if you can: create an order for 500 sat on a
node charging 0.6%, read
order.feefrom its database, and confirm the app'squote shows exactly
2 × order.fee.B · The critical fix from review — party derivation
The first version took the buyer key from
counterparty_pubkey, which is emptyfor a maker seller and the order-book key for a taker. Both produce an escrow
the buyer cannot spend, and the taker case swaps the funds before the daemon
rejects it.
To convince yourself the plumbing is real rather than just the test: in
rust/src/api/cashu.rs, temporarily changetrade.buyer_trade_pubkeyback totrade.counterparty_pubkeyand run the lock flow (section D) — it must failwith
CashuEscrowRequestMissingon a maker, rather than building a token.C · Lightning regression — the part that must not move
Run against the default (Lightning) node, with the developer override off.
exactly as on
main. Not the escrow screen.unchanged.
regression the review found: the routing gate used to be read before it had
resolved, which could send a seller to the wrong screen. It must land on the
hold invoice every time.
D · The escrow screen (override + local mint)
Setup is the same as #237 (nutshell + the developer override + mint URL), and
you will need a funded app wallet — fund it through the C3 wallet screen with a
token from
docker exec nutshell poetry run cashu --host http://localhost:3338 send 64.Lock the escrow instead of the hold invoice screen.
the mint and when you can reclaim it. Check the total is
amount + feeand that the fee matches section A.to the C3 wallet screen — not a failure message.
Lightning-only daemon the submission will be rejected — that is expected,
and the interesting part is what the screen does: it must show a Retry
sending button and the "locked but the node has not confirmed" note,
because the funds have already moved and the daemon's handler is idempotent.
amount + fee(slightly more — the mint charges a swap fee, see below).and open the escrow screen for a trade: you get a localized message and
no retry button, because nothing moved.
E · Still needs a Cashu daemon
Everything below is unverifiable today and is where the remaining risk sits:
AddCashuEscrowthe daemon accepts;cashu-escrow-lockedand being told to send fiat;and unit-tested, not observed);
order.feeon a live order.When testing against the daemon's Track A branch, watch for
InvalidCashuTokenand
InvalidMintUrl— those are the two rejections this flow can still earn.Known limitation
A mint that charges swap fees makes locking cost slightly more than
amount + mostro_fee, andcashu_escrow_quotedoes not account for that. Aseller holding exactly the total can fail at the mint. Flagged rather than
fixed here; it wants a decision about whether the quote should pad or the UI
should warn.