feat(cashu): C1a — detect the node's escrow mode from its 38385 tags - #230
Conversation
First half of phase C1. Rust-only and inert: it learns what the active node advertises and stores it. Nothing reads the result yet, so behaviour is unchanged — the Dart provider, About surface and dev override are C1b. Split because the override needs a generic k/v accessor on the storage trait, which means touching both the sqlite and IndexedDB backends. That is a different kind of change from tag parsing and does not belong in the same review. EscrowMode is tri-state like BondPolicy on the Dart side, and the distinction carries weight: a daemon that predates the tags is Unknown, not Lightning, so the UI can say "not advertised" instead of claiming the node confirmed anything. is_cashu() is the only way to ask and answers false for both Unknown and Lightning, so every Cashu path stays shut unless a node positively said otherwise. An unrecognised backend reads as Lightning for the same reason: one we cannot trade Cashu with is one whose Cashu paths must stay closed. fetch_and_set_pow becomes fetch_and_set_node_capabilities and now parses both PoW and the escrow tags from the same event — one fetch, since a second relay query would double the traffic for no new information. The mode is also cleared at the start of a node switch, beside the order-book clear. The capability re-fetch is a network round trip, and without this the old node's mode stays cached until it answers; clearing first makes that window read as Unknown rather than carrying one node's Cashu mode onto another. 14 tests over parsing, the fail-safe defaults, override precedence and the node-switch clear. Unrelated, found while verifying: api::messages::tests::mark_as_read_updates_count is flaky on unmodified main — 2 failures in 6 runs. Not touched here; filed separately.
|
Warning Review limit reached
Next review available in: 22 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 (4)
✨ 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 |
![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: ec140bf8b9
ℹ️ 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".
| Err(e) => { | ||
| log::warn!("[nostr] failed to fetch Kind 38385 for PoW: {e}"); | ||
| log::warn!("[nostr] failed to fetch Kind 38385 for node capabilities: {e}"); | ||
| } |
There was a problem hiding this comment.
When the active node was previously resolved as Cashu, a later capability refresh that hits this error branch leaves the global RESOLVED cache untouched. For a reconnect where the 38385 fetch times out or otherwise fails, is_cashu_mode() can keep returning true from stale data even though the module treats unfetched/unreachable node info as Unknown, so the Cashu gate does not fail closed until a successful Ok(None) or node-switch clear occurs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed in 2a57773.
The Err branch now calls escrow_mode::clear(). The module documents unreachable as identical to unfetched — both are Unknown — but the code was only honouring that for Ok(None), so a reconnect whose 38385 query timed out kept answering cashu from the previous successful fetch. Clearing makes that window fail closed.
PoW is deliberately left untouched in the same branch: a stale difficulty still gets messages accepted by the daemon, whereas a stale escrow mode opens a path. The comment in the code says so, so the asymmetry does not read as an oversight later.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…less nodes Two P2 findings from the Codex review, both cases where the gate stayed open when the module's own contract says it must not. Stale mode after a failed fetch: the Err branch of fetch_and_set_node_capabilities left RESOLVED untouched, so a reconnect whose 38385 query timed out kept answering "cashu" from the last successful fetch. escrow_mode treats unreachable exactly like unfetched, so clear it here too. PoW is deliberately left alone — a stale difficulty still gets messages accepted, whereas a stale escrow mode opens a path. Cashu without a usable mint: a node advertising escrow_mode=cashu but no cashu_mint_url produced is_cashu_mode() == true with nothing to connect to. The global gate now also requires CashuNodeConfig::is_usable(), so the failure lands at the gate instead of at the first mint call. EscrowMode::is_cashu() keeps reporting the mode alone — that is what the About screen (C1b) reads to say "cashu, no mint advertised" rather than silently showing Lightning. The mint override (§4.3) is what makes a forced Cashu mode usable against a daemon publishing no mint. Tests that touch the RESOLVED global now share a mutex, so the new case does not race the node-switch test.
1 participant
Phase C1, first half of
docs/cashu/README.md.Depends on: nothing — works on
mostro-core0.13.1, so it is independent of #229 (C0) and the two can merge in any order. Blocks: C1b, then C5.Why this is split
C1 as specified covers tag parsing, a persisted developer override, the FRB surface and the Dart About/settings UI. The override needs a generic k/v accessor on the storage trait, which means touching both the sqlite and IndexedDB backends — a different kind of change from tag parsing, and not one that belongs in the same review.
escrowModeProvider, About section, dev-only toggle, l10n ×5.Behaviour is unchanged either way. Against every daemon that exists today the mode resolves to
Unknown, which keeps all Cashu paths shut.The design point that matters
EscrowModeis tri-state, mirroringBondPolicyon the Dart side — and the third state earns its place:is_cashu()UnknownfalseLightning"lightning", or anything unrecognisedfalseCashu"cashu"trueUnknown≠Lightning: it lets the About screen (C1b) say "not advertised" rather than claim the node confirmed Lightning. Andis_cashu()is the only way to ask, answeringfalsefor both non-Cashu states — so the gate fails safe by construction, per design principle 1 ("off by default, inert until detected").An unrecognised backend (say a future
fedimint) reads as Lightning deliberately: a node we cannot trade Cashu with is one whose Cashu paths must stay closed.Wiring
fetch_and_set_pow→fetch_and_set_node_capabilities, parsing PoW and the escrow tags from the same event. One fetch: they come from the same 38385 event, and a second relay query would double the traffic for no new information.The mode is also cleared at the start of a node switch, beside the existing order-book clear. The capability re-fetch is a network round trip; without this the old node's mode stays cached until it answers, and that window would carry one node's Cashu mode onto another. Clearing first makes the window read
Unknown.Test plan
cargo test— 124 pass (110 pre-existing unmodified, +14 new)cargo clippy -- -D warnings— cleancargo check --target wasm32-unknown-unknown— cleanrust/src/api/surface change (the renamed fn ispub(crate)), so nofrb-generate.shrun requiredThe 14 tests cover: a today's-daemon tag set resolving to
Unknown; theis_cashu()fail-safe; a full Cashu tag set; explicitlightning; an unrecognised backend; case/whitespace tolerance; a malformed day count not costing us the mint URL; a blank mint URL not counting as one; override precedence both ways; a blank override not erasing the node's value; a corrupted stored override falling back toauto; and the node-switch clear.While verifying I hit
api::messages::tests::mark_as_read_updates_countfailing intermittently. I checked it on unmodifiedmain: 2 failures in 6 runs (~33%). It is not caused by this change and is not touched here — filed separately so it does not ride along in a Cashu review.Series
C0 #229 ✅ · C1a (this) · C1b · C2 wallet core (cdk 0.17.3) · C3 wallet UI · C4 escrow primitives · C5 Track A · C6–C8 · C9 wasm · C10 polish