Skip to content

feat(cashu): C1b — persist escrow-mode overrides and surface the backend in About - #234

Merged
grunch merged 5 commits into
mainfrom
feat/cashu-c1b-escrow-mode-persistence
Jul 29, 2026
Merged

feat(cashu): C1b — persist escrow-mode overrides and surface the backend in About#234
grunch merged 5 commits into
mainfrom
feat/cashu-c1b-escrow-mode-persistence

Conversation

@grunch

@grunch grunch commented Jul 25, 2026

Copy link
Copy Markdown
Member

Closes the C1 phase of docs/cashu/README.md. C1a (#230) added the tri-state
detection and the is_cashu_mode() gate; this half adds persistence, the bridge
surface, and the About screen.

Zero behaviour change on Lightning. With no escrow_mode tag — every daemon
in the wild today — the resolution stays Unknown, every Cashu path stays shut,
and About renders exactly what it rendered before this PR.

Rust

Settings k/v store. Storage gains get_setting / set_setting /
delete_setting over the settings table that already existed in the schema.
SQLite implements it for real, and the two named active-node accessors become
thin wrappers over it rather than a second copy of the SQL. IndexedDB follows
its existing stub contract (warn + no-op), so on web the overrides apply for the
session only — the whole IndexedDB backend is a stub, now tracked in #233.

Resolution on read. mostro/escrow_mode.rs previously cached the resolved
value. It now keeps what the node advertised and the developer overrides in
separate globals and runs resolve() on read. Two consequences worth the
change:

  • flipping an override takes effect immediately instead of waiting for the next
    relay fetch, and there is no second copy of the answer that can go stale;
  • a node switch clears the tags but keeps the overrides. Forcing Cashu is a
    statement about this build, not about a particular node.

Every mutator emits on a broadcast channel, so "changed" and "notified" cannot
drift apart.

api/escrow.rs (new): get_escrow_mode, set_escrow_mode_override,
set_cashu_mint_url_override, rehydrate_escrow_overrides, and an
on_escrow_mode_changed stream. Mint URLs must be http(s) with a host —
validated on write and again on rehydrate, so a stored value that no longer
passes tightened rules is dropped rather than silently used.
EscrowModeInfo.mode is a stable marker (unknown / lightning / cashu);
Rust does not translate.

The gate stays is_cashu_available, not mode == "cashu": a node can advertise
Cashu and publish no usable mint, and that combination must not open anything.

Dart

  • MostroInstance.fromTags parses escrow_mode and the three cashu_* tags,
    tri-state and parameter-gated exactly like BondPolicy — a Lightning node
    carrying a stale cashu_mint_url never surfaces it as live.
  • About reports what the node advertises, so the backends are mutually
    exclusive on screen: a Cashu node gets a "Cashu escrow" section (mint,
    locktime, settlement margin) instead of the Lightning Network section. A
    Cashu node that published no mint says "Not advertised" rather than rendering
    an empty section. The mint renders through the plain info row, not the
    copyable one, whose 20-character abbreviation would hide the host — the one
    part of a mint URL that matters.
  • The client-side override never changes what About says. It is surfaced only in
    a kDebugMode-only settings card, which shows the effective resolution next
    to the toggle, so a tester is never fooled about which is which.
  • escrowModeProvider / isCashuAvailableProvider expose the resolution that
    gates behaviour, for the phases stacked on top of this one.
  • Strings translated in all five locales.

Tests

Rust — k/v round-trip and key independence; override re-resolves without a
fetch; a node switch keeps the override; every mutator notifies subscribers; a
rejected mint URL leaves the previous value in place; forcing Cashu without a
mint does not open the gate.

Dart — tag parsing including gating, case/whitespace, unrecognised backends
and malformed day counts; widget tests asserting no trace of Cashu on a
Lightning or silent node (including one carrying stale cashu tags), and no
Lightning section on a Cashu one.

Verification

cargo test                                  145 passed, 6 ignored
cargo clippy --locked -- -D warnings        clean
cargo check --target wasm32-unknown-unknown clean
flutter analyze                             no issues
flutter test                                155 passed

./scripts/frb-generate.sh was run after the rust/src/api/ change.

Test plan

  • Against any current daemon: About is identical to main, no Cashu strings
    anywhere, trades behave as before.
  • Debug build → Settings → "Escrow backend (developer)": toggling "Force
    Cashu escrow" flips the effective mode instantly, with no relay round trip.
  • Entering a garbage mint URL is rejected with a localized message and the
    previous value survives.
  • Restart the app: the override and mint URL are still in force (native).
  • Switch Mostro nodes: the override survives, the node's advertised tags do
    not.
  • Release build: the developer card is absent.

Follow-ups: #233 (IndexedDB persistence on web). Next in the plan is the cdk
spike, before C2.

Summary by CodeRabbit

  • New Features
    • Updated the About screen to show the escrow backend as Lightning or Cashu (including mint URL, locktime, and settlement margin) and hide the non-matching section.
    • Added a debug-only settings card to preview the effective escrow backend, force Cashu, override the Cashu mint URL, and display availability/warnings (with “not advertised” messaging when applicable).
  • Documentation
    • Updated Cashu escrow docs to clarify escrow-mode reporting and override persistence (persisted on desktop databases; session-only on web).
  • Localization / Tests
    • Added Cashu escrow translations across supported languages and expanded UI/widget coverage to match the new rendering.

Manual verification

Two things a reviewer needs to be sure of: nothing changed for a Lightning
user
, and the override does what it claims without leaking into About.
No public daemon runs Cashu yet, so the Cashu-side rendering is covered by the
widget tests rather than by hand.

Already run on this branch

cargo test                                  149 passed, 6 ignored
cargo clippy --locked -- -D warnings        clean
cargo check --target wasm32-unknown-unknown clean
flutter analyze                             no issues
flutter test                                155 passed

A · Lightning regression — the part that must not move

Run against the default node (flutter run -d linux, or your usual device).

  1. About screen — open it and compare against main:
    • the Lightning Network section is present with the lnd_* values;
    • there is no "Cashu escrow" section and no row containing the word
      Cashu, a mint URL, a locktime or a settlement margin;
    • every other section (General info, Anti-abuse bond, Technical details) is
      byte-identical to main.
  2. Take and complete a trade as you normally would. Nothing in this PR
    touches the order or trade flow; if anything behaves differently, that is a
    bug in this PR.
  3. Logs (Settings → Log report) should show
    [escrow-mode] active node advertises unknown at most once per node,
    not once per reconnect.

B · The developer card

The card is kDebugMode-only, so it only exists in a debug build.

  1. Debug build: Settings → scroll to the bottom → "Escrow backend
    (developer)"
    is there, below "Mostro node".
  2. Release build: flutter build linux --release (or your platform) and
    open Settings → the card is absent. This is the check that matters most
    — it is the one thing flutter test cannot verify, because kDebugMode is
    true under the test harness.

C · The override behaves

In the debug build, on the developer card:

  1. Toggle Force Cashu escrow on. "Effective backend" flips to Cashu
    immediately — no relay round trip, no app restart. This is the whole point
    of resolving on read.
  2. With the toggle on and no mint set, the card shows the red
    "Cashu cannot run without a mint" line. That is the gate refusing to open,
    which is correct.
  3. Open About again. It must still say Lightning and show the Lightning
    Network section. About reports what the node advertised; the override is a
    client-side statement and must never change it. If About flips, that is the
    regression this contract exists to prevent.
  4. Type not a url in Mint URL override and apply → a localized error, and
    the previous value is still in the field.
  5. Type http://localhost:3338 and apply → accepted, and "Effective mint"
    updates.
  6. Kill the app and relaunch → both the toggle and the mint URL are still
    set. (Native only; on web they last the session — Web: IndexedDB storage backend is a stub — nothing persists across a reload #233.)
  7. Switch to a different Mostro node in Settings → the override survives
    (it is a statement about this build, not about a node), while the node's own
    advertised mode is re-fetched.
  8. Turn the toggle off → "Effective backend" returns to what the node says.

What is not covered by hand

The Cashu-mode About rendering — the "Cashu escrow" section replacing the
Lightning one — has no daemon to produce it yet. It is covered by
test/features/about/screens/about_screen_test.dart, which asserts both
directions: no Cashu string on a Lightning or silent node, and no Lightning
section on a Cashu one.

…end in About

Closes the C1 phase of docs/cashu/README.md. Behaviour on a Lightning node is
unchanged: with no `escrow_mode` tag the resolution stays Unknown, every Cashu
path stays shut, and the About screen renders exactly what it did before.

Rust
- `Storage` gains a generic k/v accessor (`get_setting`/`set_setting`/
  `delete_setting`) over the `settings` table that already existed. SQLite
  implements it for real and the two named active-node accessors become thin
  wrappers over it; IndexedDB follows its existing stub contract, so on web the
  overrides apply for the session only (tracked in #233).
- `escrow_mode` now stores what the node advertised and the overrides
  separately, and resolves on read. Flipping an override therefore takes effect
  immediately instead of waiting for the next relay fetch, and there is no
  second copy of the answer that can go stale. A node switch clears the tags but
  keeps the overrides — forcing Cashu is a statement about this build, not about
  a node. Every mutator emits on a broadcast channel.
- New `api/escrow.rs`: `get_escrow_mode`, `set_escrow_mode_override`,
  `set_cashu_mint_url_override`, `rehydrate_escrow_overrides` and an
  `on_escrow_mode_changed` stream. Mint URLs are validated as http(s) with a
  host, on write and again on rehydrate. `EscrowModeInfo.mode` is a stable
  marker; Dart maps it to a localized string.

Dart
- `MostroInstance.fromTags` parses `escrow_mode` and the three `cashu_*` tags,
  tri-state and parameter-gated exactly like `BondPolicy`.
- About reports what the node advertises, so the backends are mutually exclusive
  on screen: a Cashu node gets a "Cashu escrow" section instead of the Lightning
  Network one. The mint renders in full rather than through the copyable row,
  whose 20-character abbreviation would hide the host.
- `escrowModeProvider` / `isCashuAvailableProvider` expose the resolution that
  gates behaviour; the override controls live in a `kDebugMode`-only settings
  card that shows the effective resolution next to the toggle.
- Strings translated in all five locales.

Tests
- Rust: k/v round-trip, override re-resolution without a fetch, node switch
  keeps the override, every mutator notifies, mint-URL rejection leaves the
  previous value in place.
- Dart: tag parsing incl. gating and malformed values; widget tests asserting no
  trace of Cashu on a Lightning or silent node, and no Lightning section on a
  Cashu one.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 22877152-189b-4772-96ef-ece5eaacd48e

📥 Commits

Reviewing files that changed from the base of the PR and between 077a4d5 and 8286c36.

📒 Files selected for processing (15)
  • 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
  • lib/main.dart
  • rust/src/api/types.rs
  • rust/src/frb_generated.rs
  • test/features/settings/widgets/escrow_mode_dev_card_test.dart
🚧 Files skipped from review as they are similar to previous changes (15)
  • lib/main.dart
  • lib/l10n/app_it.arb
  • lib/l10n/app_localizations_es.dart
  • lib/l10n/app_localizations_it.dart
  • lib/l10n/app_de.arb
  • lib/l10n/app_fr.arb
  • lib/l10n/app_localizations.dart
  • lib/l10n/app_en.arb
  • lib/l10n/app_localizations_en.dart
  • lib/l10n/app_localizations_fr.dart
  • lib/l10n/app_localizations_de.dart
  • test/features/settings/widgets/escrow_mode_dev_card_test.dart
  • rust/src/api/types.rs
  • lib/l10n/app_es.arb
  • rust/src/frb_generated.rs

Walkthrough

The PR adds Cashu escrow-mode parsing and About-screen support, separates advertised state from developer overrides, persists overrides through the settings store, exposes Rust state changes to Flutter, adds debug controls, and localizes the new UI across five languages.

Changes

Cashu escrow mode

Layer / File(s) Summary
Escrow state and UI API
rust/src/mostro/escrow_mode.rs, rust/src/api/escrow.rs, rust/src/api/types.rs, rust/src/api/nostr.rs
Advertised escrow tags and developer overrides resolve into streamed EscrowModeInfo snapshots with Cashu availability and mint validation.
Settings persistence contract
rust/src/db/*
Generic settings operations support escrow override keys, with SQLite persistence and IndexedDB no-op behavior.
Flutter bridge and debug controls
rust/src/frb_generated.rs, lib/main.dart, lib/features/settings/*
Generated bridge endpoints, startup rehydration, Riverpod providers, and a debug-only override card expose effective escrow state and controls.
About escrow model and rendering
lib/features/about/*, test/features/about/*
Node tags populate gated Cashu fields, and About displays either Cashu escrow details or Lightning details with corresponding tests.
Localized escrow copy and documentation
lib/l10n/*, docs/cashu/README.md
New Cashu, escrow-mode, override, availability, and pluralized-day strings are added in five locales, with C1 documentation describing advertised versus effective state and persistence.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Possibly related PRs

  • MostroP2P/app#235 — Its Cashu wallet enablement is coupled to the escrow-mode usability logic changed here.

Suggested reviewers: arkanoider

Poem

A bunny found Cashu tags in a node,
And watched escrow details unfold.
Overrides hopped through a stream,
Mint URLs joined the debug dream,
While five tongues made the new story bold.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the two main changes: persisted escrow-mode overrides and exposing backend details in About.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/cashu-c1b-escrow-mode-persistence

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@grunch

grunch commented Jul 25, 2026

Copy link
Copy Markdown
Member Author

Review — C1b (strict pass)

Note: GitHub does not allow Request changes on your own PR, so this is posted as a comment. Treat it as 🔴 Request changes — one finding is a real Flutter anti-pattern in a screen that writes persisted state, and one is a concurrency bug that will bite the moment two overrides are written close together.

Scope reviewed: rust/src/db/{mod,sqlite,indexeddb}.rs, rust/src/mostro/escrow_mode.rs, rust/src/api/escrow.rs, rust/src/api/types.rs, lib/features/about/**, lib/features/settings/**, l10n ×5.


🔴 Major

1. Read-modify-write race on the overridesrust/src/api/escrow.rs

Both setters do the same thing:

escrow_mode::set_overrides(EscrowOverrides {
    mode,
    ..escrow_mode::get_overrides()   // ← read
});                                   // ← write

get_overrides() releases its lock before set_overrides() takes the write lock. Two concurrent calls — the dev card can produce exactly this by toggling the switch while the mint-URL field is submitted — interleave as read/read/write/write and the second write silently discards the first. The persisted value and the in-memory value then disagree until the next rehydrate.

Fix: give escrow_mode a update_overrides(impl FnOnce(&mut EscrowOverrides)) that mutates under a single write lock, and have both setters go through it. The persist call should stay outside the lock.

2. TextEditingController mutated during build()lib/features/settings/widgets/escrow_mode_dev_card.dart

final info = ref.watch(escrowModeProvider).valueOrNull;
if (info != null) {
  _syncMintField(info.mintUrlOverride);   // assigns _mintController.text
}

Assigning .text notifies listeners, which marks the TextField dirty during the build that is currently laying it out. It happens to work today, but it is the standard cause of "setState() or markNeedsBuild() called during build" once anything else in the subtree listens to the controller — and it will also stomp on the user's in-progress typing whenever an unrelated escrow event fires (a node switch, a capability re-fetch), because the guard only compares against the last stored value.

Fix: move it to a listener —

ref.listen(escrowModeProvider, (_, next) {
  final stored = next.valueOrNull?.mintUrlOverride;
  if (stored != _syncedMintOverride) { ... }
});

🟡 Minor

3. is_overridden conflates "forced" with "genuinely Cashu"escrow_mode.rs, resolve()

is_overridden is true whenever the override is ForceCashu, even when the node independently advertises Cashu. The dev card then labels a perfectly genuine Cashu node as forced, which is precisely the confusion the flag exists to prevent. Consider overridden && from_tags != EscrowMode::Cashu.

4. The broadcast payload is computed and thrown awayescrow_mode.rs / api/escrow.rs

notify() builds a full ResolvedEscrowMode on every event, and EscrowModeStream::next() discards it (Ok(_) => Ok(snapshot())). The rebuild is deliberate and well-argued in the comment, but then the channel should be broadcast::Sender<()> — as written, every notification does the resolution work twice.

5. clear() now emits on every failed capability fetchapi/nostr.rs

clear() notifies, and it is called on both the Ok(None) and Err arms of fetch_and_set_node_capabilities. On a flaky relay this produces a stream of identical "still unknown" events. Harmless today (the UI is idempotent), but worth suppressing when the state did not actually change.


🔵 Nit

6. Inconsistent stubsrust/src/db/indexeddb.rs: set_setting warns, delete_setting returns Ok(()) silently. Both are no-ops; either both should warn or the comment should say why removal is different.


✅ What is right

  • The move from caching the resolved value to resolving on read is the correct call, and the reasoning in the doc comment (no second copy that can go stale) is the right one to record.
  • Keeping the override across a node switch while clearing the tags is subtle and correctly justified.
  • The About decision — report what the node advertises, never what the override forces — is the honest one, and the widget tests actually assert the absence of every Cashu string rather than just the presence of the right ones.
  • EscrowModeOverride::from_stored failing closed on garbage is the right default for a flag that unlocks a money path.

Test gaps

  • Nothing covers the persistence path end to end (rehydrate_escrow_overrides after a real set_*), only the pieces.
  • No test asserts the dev card is absent from a release build. kDebugMode is true under flutter test, so this is untestable as written — worth a comment saying so, rather than leaving the reader to assume it is covered.

… write

Addresses the strict review on #234.

Major
- `set_escrow_mode_override` / `set_cashu_mint_url_override` read the overrides
  and wrote them back as a whole struct, so two writes from the same screen
  interleaved and the second silently discarded the first. Both now go through
  `escrow_mode::update_overrides`, which mutates under one write lock. Covered
  by `setting_one_override_never_clobbers_the_other`.
- The dev card assigned `TextEditingController.text` during `build()`, which
  notifies listeners in the middle of laying the field out and also wiped
  in-progress typing on any unrelated escrow event. Seeding now happens in a
  post-frame callback and updates in `ref.listen`.

Minor
- `is_overridden` was true whenever the override was on, including when the
  node genuinely advertises Cashu — so the dev card labelled a real Cashu node
  as forced, which is the confusion the flag exists to prevent. Now
  `forcing && from_tags != Cashu`.
- The broadcast carried a `ResolvedEscrowMode` that the stream discarded and
  rebuilt, resolving twice per change. The channel is now `Sender<()>`: a bare
  wake-up, with subscribers reading the globals as before.
- Every mutator emitted unconditionally, so a node whose capability fetch keeps
  failing produced a stream of identical "still unknown" events. Each now
  compares before/after and only notifies on a real change.
- `indexeddb::delete_setting` returned Ok silently while its siblings warned. A
  silent no-op in a storage layer reads like working storage in a log.

Tests
- `overrides_survive_a_restart_through_the_settings_store` — the persist and
  rehydrate halves end to end against a real store, which nothing covered.
- `a_change_that_changes_nothing_does_not_wake_subscribers`.
- `forcing_cashu_on_a_cashu_node_is_not_flagged_as_an_override`.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (3)
rust/src/mostro/escrow_mode.rs (1)

284-301: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Log line fires on every capability fetch, including the no-change case.

The changed guard suppresses the notification but not the info!, so a flaky relay that re-fetches the same event logs identical lines indefinitely. Moving it into the changed branch keeps the log a record of actual transitions.

♻️ Proposed refactor
 pub fn set_from_tags(mode: EscrowMode, config: CashuNodeConfig) {
-    log::info!(
-        "[escrow-mode] active node advertises {} (mint={:?})",
-        mode.as_marker(),
-        config.mint_url,
-    );
     let changed = {
         let mut guard = TAGS.write().unwrap_or_else(|e| e.into_inner());
-        let next = Some((mode, config));
+        let next = Some((mode, config.clone()));
         let changed = *guard != next;
         *guard = next;
         changed
     };
     // A re-fetch that confirms what we already knew is the common case on a
     // reconnect; emitting for it would wake every listener for nothing.
     if changed {
+        log::info!(
+            "[escrow-mode] active node advertises {} (mint={:?})",
+            mode.as_marker(),
+            config.mint_url,
+        );
         notify();
     }
 }
🤖 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 `@rust/src/mostro/escrow_mode.rs` around lines 284 - 301, Move the escrow-mode
info log into the existing `if changed` branch alongside `notify()`, so it runs
only when the stored capability changes. Keep the TAGS update and
unchanged-fetch suppression behavior intact.
rust/src/api/escrow.rs (2)

23-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

snapshot() resolves the globals three times, so the fields it promises are consistent can still disagree.

get_resolved(), get_overrides() and is_cashu_mode() each take the locks independently, and is_cashu_mode() re-resolves from scratch. A concurrent update_overrides between them yields e.g. mode == "lightning" with is_cashu_available == true — exactly the inconsistency the stream comment at Line 172 says cannot happen. Deriving every field from one resolved value removes the window.

♻️ Proposed refactor
 fn snapshot() -> EscrowModeInfo {
     let resolved = escrow_mode::get_resolved();
     let overrides = escrow_mode::get_overrides();
 
     EscrowModeInfo {
         mode: resolved.mode.as_marker().to_string(),
         mint_url: resolved.config.mint_url.clone(),
         escrow_locktime_days: resolved.config.escrow_locktime_days,
         settlement_margin_days: resolved.config.settlement_margin_days,
         is_overridden: resolved.is_overridden,
-        is_cashu_available: escrow_mode::is_cashu_mode(),
+        is_cashu_available: resolved.mode.is_cashu() && resolved.config.is_usable(),
         force_cashu_override: matches!(overrides.mode, EscrowModeOverride::ForceCashu),
         mint_url_override: overrides.mint_url,
     }
 }

An accessor in escrow_mode returning (ResolvedEscrowMode, EscrowOverrides) under one pass would close the remaining get_overrides() gap too.

🤖 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 `@rust/src/api/escrow.rs` around lines 23 - 37, Update snapshot() to obtain the
resolved mode and overrides through a single escrow_mode accessor/pass, rather
than independently calling get_resolved(), get_overrides(), and is_cashu_mode().
Derive is_cashu_available and all other fields from that one consistent resolved
value, while retaining the existing override fields from the same snapshot.

199-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Two independent mutexes guard the same process globals, so these tests can race the escrow_mode tests.

escrow_lock() uses a private LOCK here, while rust/src/mostro/escrow_mode.rs (Line 411) serializes its own tests on a separate GLOBAL mutex. Both modules compile into the same test binary and mutate TAGS/OVERRIDES, so a test from each module can interleave — the clear() in one resetting state the other just arranged. Expose one shared test lock (e.g. a pub(crate) helper in escrow_mode) and use it from both.

🤖 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 `@rust/src/api/escrow.rs` around lines 199 - 207, Replace the private LOCK used
by escrow_lock() with the shared test mutex exposed by escrow_mode, and make the
existing GLOBAL lock in escrow_mode accessible through a pub(crate) helper.
Ensure both escrow.rs and escrow_mode tests acquire that same lock before
clearing or modifying TAGS and OVERRIDES, while preserving the current state
reset behavior.
🤖 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 `@docs/cashu/README.md`:
- Around line 369-375: Update the documentation around the About-screen contract
and its “Done when” criteria to state that client-side escrow overrides affect
only effective/debug state, not the About section. Ensure About remains
determined solely by the node’s advertised capabilities, with Cashu and
Lightning sections rendered according to the existing advertisement logic.

In `@lib/features/about/models/mostro_instance.dart`:
- Around line 241-245: Update parseEscrowMode so it uses get('escrow_mode') and
checks for null before trimming the value; return EscrowMode.unknown only when
the tag is missing, while blank and other unrecognized values resolve to
EscrowMode.lightning. Add a regression test covering a present blank
escrow_mode.

In `@lib/features/settings/widgets/escrow_mode_dev_card.dart`:
- Around line 141-147: Replace the hard-coded hintText in the escrow mode
development card with a generated AppLocalizations accessor, add the mint URL
hint string to the supported ARB localization files, and run flutter gen-l10n to
regenerate accessors. Keep the existing settingsCashuMintOverrideLabel and
settingsCashuMintOverrideApply usage unchanged.
- Around line 87-91: Update the post-frame callback in the initialization block
guarded by _seeded and info so it reads the latest provider value when the
callback executes instead of capturing the stale seed from the current build;
preserve the existing _syncMintField behavior. Add a widget regression test
covering an escrow update delivered before the first post-frame callback and
verify the newer override remains applied.

In `@lib/l10n/app_localizations_fr.dart`:
- Around line 2317-2323: Update the source localization entry for aboutDaysValue
so zero maps to the plural “jours” while exactly one remains singular, then
regenerate localizations with flutter gen-l10n to propagate the corrected plural
behavior across generated files.

---

Nitpick comments:
In `@rust/src/api/escrow.rs`:
- Around line 23-37: Update snapshot() to obtain the resolved mode and overrides
through a single escrow_mode accessor/pass, rather than independently calling
get_resolved(), get_overrides(), and is_cashu_mode(). Derive is_cashu_available
and all other fields from that one consistent resolved value, while retaining
the existing override fields from the same snapshot.
- Around line 199-207: Replace the private LOCK used by escrow_lock() with the
shared test mutex exposed by escrow_mode, and make the existing GLOBAL lock in
escrow_mode accessible through a pub(crate) helper. Ensure both escrow.rs and
escrow_mode tests acquire that same lock before clearing or modifying TAGS and
OVERRIDES, while preserving the current state reset behavior.

In `@rust/src/mostro/escrow_mode.rs`:
- Around line 284-301: Move the escrow-mode info log into the existing `if
changed` branch alongside `notify()`, so it runs only when the stored capability
changes. Keep the TAGS update and unchanged-fetch suppression behavior intact.
🪄 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 Plus

Run ID: 431f6111-9570-44c3-a260-bc6779248de9

📥 Commits

Reviewing files that changed from the base of the PR and between 1931ac0 and 983bd48.

📒 Files selected for processing (29)
  • docs/cashu/README.md
  • lib/features/about/models/mostro_instance.dart
  • lib/features/about/screens/about_screen.dart
  • lib/features/settings/providers/escrow_mode_provider.dart
  • lib/features/settings/screens/settings_screen.dart
  • lib/features/settings/widgets/escrow_mode_dev_card.dart
  • 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
  • lib/main.dart
  • rust/src/api/escrow.rs
  • rust/src/api/mod.rs
  • rust/src/api/nostr.rs
  • rust/src/api/types.rs
  • rust/src/db/indexeddb.rs
  • rust/src/db/mod.rs
  • rust/src/db/sqlite.rs
  • rust/src/frb_generated.rs
  • rust/src/mostro/escrow_mode.rs
  • test/features/about/models/mostro_instance_test.dart
  • test/features/about/screens/about_screen_test.dart

Comment thread docs/cashu/README.md
Comment thread lib/features/about/models/mostro_instance.dart
Comment thread lib/features/settings/widgets/escrow_mode_dev_card.dart Outdated
Comment on lines +141 to +147
decoration: InputDecoration(
labelText: l10n.settingsCashuMintOverrideLabel,
hintText: 'http://localhost:3338',
suffixIcon: IconButton(
icon: const Icon(Icons.check),
tooltip: l10n.settingsCashuMintOverrideApply,
onPressed: info == null ? null : _applyMintUrl,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Localize the visible mint URL hint.

Line 143 is user-facing copy. Define it in the supported ARB files, use the generated localization accessor, then run flutter gen-l10n.

Proposed fix
-              hintText: 'http://localhost:3338',
+              hintText: l10n.settingsCashuMintOverrideHint,

As per coding guidelines, “Localize all user-facing strings through ARB files and access them with AppLocalizations.of(context) instead of hard-coded literals.”

🤖 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 `@lib/features/settings/widgets/escrow_mode_dev_card.dart` around lines 141 -
147, Replace the hard-coded hintText in the escrow mode development card with a
generated AppLocalizations accessor, add the mint URL hint string to the
supported ARB localization files, and run flutter gen-l10n to regenerate
accessors. Keep the existing settingsCashuMintOverrideLabel and
settingsCashuMintOverrideApply usage unchanged.

Source: Coding guidelines

Comment thread lib/l10n/app_localizations_fr.dart
… doc

CodeRabbit raised these against #234, where the files live. They had been fixed
one branch too far down the stack (on the C5 branch) and so never reached this
PR; applied here instead, and the stack will carry them forward.

- `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 — About and
  the Cashu gate could disagree about the same daemon. Only an absent (or
  value-less) tag is unknown now.
- The dev card's post-frame seed captured `mintUrlOverride` during build. 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.
- The C1 "Done when" 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.

Two more from the same pass, not raised inline but the same class:
- `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()` expresses the gate against a value the
  caller already holds.
- `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.

Tests: blank / whitespace / value-less / absent `escrow_mode` 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@test/features/settings/widgets/escrow_mode_dev_card_test.dart`:
- Around line 65-80: Update the test “the newest override wins over an earlier
one” to register a post-frame callback before the widget’s seed callback, emit
the newer override synchronously before the frame runs, then pump twice. Ensure
the timing exercises the first post-frame race so the assertion verifies the
latest value rather than merely testing a second pumpAndSettle cycle.
🪄 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 Plus

Run ID: 0914a954-915b-4751-9fa8-80b8a82914b4

📥 Commits

Reviewing files that changed from the base of the PR and between 983bd48 and 077a4d5.

📒 Files selected for processing (7)
  • docs/cashu/README.md
  • lib/features/about/models/mostro_instance.dart
  • lib/features/settings/widgets/escrow_mode_dev_card.dart
  • rust/src/api/escrow.rs
  • rust/src/mostro/escrow_mode.rs
  • test/features/about/models/mostro_instance_test.dart
  • test/features/settings/widgets/escrow_mode_dev_card_test.dart
🚧 Files skipped from review as they are similar to previous changes (6)
  • lib/features/settings/widgets/escrow_mode_dev_card.dart
  • test/features/about/models/mostro_instance_test.dart
  • docs/cashu/README.md
  • lib/features/about/models/mostro_instance.dart
  • rust/src/mostro/escrow_mode.rs
  • rust/src/api/escrow.rs

Comment thread test/features/settings/widgets/escrow_mode_dev_card_test.dart Outdated
CodeRabbit's second pass on #234 pointed out that the regression test added in
the previous commit did not reproduce what it claimed to: it emitted the newer
override after the seed callback had already run, so it passed against the
buggy code too. That matches what I had flagged — the interleaving looked
unreachable from a widget test. It is not, with two changes:

- a **synchronous** broadcast controller, so the provider update and its
  listener land inside the frame rather than in a later microtask;
- a post-frame callback registered **before** the widget's, which emits the
  newer value while the widget's seed callback is still queued behind it.

Verified to discriminate: with the build-time capture restored, the test fails
with `Actual: 'http://old.example'` — the exact stale value. With the fix it
passes.
codaMW
codaMW previously approved these changes Jul 26, 2026

@codaMW codaMW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed C1b independently, full pass over escrow_mode.rs, api/escrow.rs,
db/*, the About rendering, and the dev card, on top of your strict round and
the CodeRabbit rounds. Approving.

Both Majors verified fixed:

  • Override race, both setters now go through update_overrides(impl FnOnce)
    under a single write lock; no read-then-write window.
  • Controller-during-build _syncMintField moved out of build() into
    ref.listen.

Money-path safety (the part that must not move), fail-safe by construction:

  • No escrow_mode tag -> Unknown -> About keeps the Lightning section it always
    showed.
  • The gate is is_cashu_usable (mode Cashu and a non-empty mint), not
    mode == cashu a Cashu node with no usable mint opens nothing.
  • About branches on node.escrowMode (node-advertised), never the client
    override; the widget test asserts the absence of every Cashu string on a
    legacy/silent node, not just the presence of the right ones.
  • validate_mint_url requires http(s)+host on write and drops non-conforming
    stored values on rehydrate fails closed.
  • The Err/Ok(None) fetch arms both clear() to fail closed on an unreachable
    node, with PoW deliberately left alone the reasoning in those comments is
    the right call.

Your earlier minors all confirmed addressed: #3 (is_overridden no longer
conflates forced with genuinely-Cashu), #4 (broadcast is now Sender<()>), #5
(clear() only notifies when the cached state actually changed), #6 (both
IndexedDB stubs warn).

One heads-up (non-blocking): the 3 escrow_mode_dev_card_test cases fail on
my local Flutter 3.44 with a ListTile background color or ink splashes may be invisible assertion the ListTile is inside a DecoratedBox with a bg color.
CI passes them on the pinned 3.38.2, so it's a version-strictness gap today, but
that pattern will start failing once FLUTTER_VERSION is bumped. Wrapping that
ListTile in its own Material (or dropping the intermediate DecoratedBox
background) now would future-proof it. Flagging so it isn't a surprise later.

cargo test 149 passed / clippy -D warnings clean / cargo check --target wasm32-unknown-unknown clean / flutter analyze clean. The
resolve-on-read design and the honest About contract (report what the node
advertises, never what the override forces) are the right calls.

@AndreaDiazCorreia AndreaDiazCorreia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

BraCR10
BraCR10 previously approved these changes Jul 29, 2026

@BraCR10 BraCR10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

Only conflict was the generated rust/src/frb_generated.rs — resolved by
regenerating the bindings from the merged sources with the pinned
codegen (2.11.1) instead of hand-merging generated code.
@grunch
grunch dismissed stale reviews from BraCR10, AndreaDiazCorreia, and codaMW via 8286c36 July 29, 2026 19:15
@grunch
grunch merged commit b538041 into main Jul 29, 2026
4 checks passed
@grunch
grunch deleted the feat/cashu-c1b-escrow-mode-persistence branch July 29, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants