feat(#141): migrate backup_confirmed from SharedPreferences to the Rust identity record - #266
feat(#141): migrate backup_confirmed from SharedPreferences to the Rust identity record#266codaMW wants to merge 1 commit into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughBackup confirmation now persists in Rust identity state. The Dart notifier performs native migration from SharedPreferences and uses platform-specific persistence. Screens save confirmation before dismissing the reminder. ChangesBackup confirmation persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BackupScreen
participant BackupCompletedNotifier
participant RustIdentityAPI
participant IdentityStorage
BackupScreen->>BackupCompletedNotifier: markCompleted()
BackupCompletedNotifier->>RustIdentityAPI: set_backup_confirmed(true)
RustIdentityAPI->>IdentityStorage: Persist backup_confirmed
IdentityStorage-->>RustIdentityAPI: Return success
RustIdentityAPI-->>BackupCompletedNotifier: Complete
BackupCompletedNotifier-->>BackupScreen: Dismiss reminder
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@rust/src/api/identity.rs`:
- Around line 314-325: Update set_backup_confirmed so it clones the identity
record, applies the new backup_confirmed value to the clone, and persists the
clone before assigning it to state.identity_info; only commit the in-memory
change after save_identity succeeds. Preserve the direct assignment path when no
database exists, and add a test that forces save_identity to fail and verifies
the flag remains unconfirmed.
In `@test/features/account/backup_reminder_provider_test.dart`:
- Around line 173-190: Update the markCompleted() and reset() tests to retain
access to the fake bridge backing value and assert its effect directly:
markCompleted() must set it to true, while reset() must set it to false. Keep
the existing notifier state assertions and use the test’s existing fake bridge
setup rather than introducing unrelated changes.
🪄 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: 3e727952-ff62-4aaa-9c83-5ab2d8f62142
📒 Files selected for processing (7)
lib/features/account/providers/backup_reminder_provider.dartrust/src/api/identity.rsrust/src/api/types.rsrust/src/db/sqlite.rsrust/src/frb_generated.rstest/features/account/backup_reminder_provider_test.darttest/features/account/backup_ritual_screen_test.dart
|
Two clarifications on the linked-issue check: Backend persistence (SQLite + IndexedDB): identity is stored as a single JSON blob (identity (id, data)), not columns, so backup_confirmed serializes into that blob for both backends automatically no per-backend column needed. The SQLite round-trip test asserts it persists; IndexedDB uses the same serialized struct. |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Adversarial review — feat(#141): migrate backup_confirmed to the Rust identity record
The Rust side is clean and the direction is right. The problems are all on the seam: what happens when the store the flag now lives in is less durable than the one it left.
Verified working (ran, not assumed)
./scripts/frb-generate.shreproduces the committedrust/src/frb_generated.rsbyte-for-byte. Regeneration was done correctly, andlib/src/rust/is gitignored with CI regenerating it, so nothing is missing there.cargo test --lib→ 239 passed, 0 failed;cargo clippy --lib -- -D warningsclean;flutter analyzeclean;flutter test test/features/account/→ 16 passed (after generating bindings locally).#[serde(default)]on a JSON blob is the right call — no schema migration, and the legacy-blob deserialization test pins it.restore_backup_confirmed's public-key guard is correct and genuinely well tested (same-identity, absent, cross-identity).- The persist-then-commit reorder in
6cb67f7is correct as written.
1. (high) On web this is a strict downgrade from durable to session-only — and the migration marker makes it permanent
main.dart:63 guards initDb with !kIsWeb, so on web app_db::db() is always None. set_backup_confirmed therefore skips the save entirely and returns Ok — the flag lives only in the in-memory IdentityState. Meanwhile the store it is being migrated out of, SharedPreferences, is backed by localStorage on web and does survive a reload.
Walk it through:
- First load after upgrade: legacy
true→_setConfirmed(true)succeeds (no store, no error) →backupCompletedMigratedToRustis written to localStorage, durably. - Reload:
load_identity_from_mnemoniccomputesstoredfromdb(), which isNoneon web, sorestore_backup_confirmed(None, ...)→false. - The migration marker is set, so the legacy value is never re-read.
Result: a web user who confirmed their backup gets the reminder re-armed on every page reload, permanently, and the durable value that used to answer the question has been consumed. The description says "on web the flag simply doesn't persist, which fails safe" — it does not fail safe, it fails permanently, and it destroys state that previously persisted.
The fix has to be to not burn the marker on a write that isn't durable. Options, roughly in order of how much I'd like them: gate the whole migration behind !kIsWeb until #233 lands; or keep mirroring into SharedPreferences on web so the legacy value stays authoritative there; or have the bridge tell Dart whether the write actually reached a store and only set the marker when it did. Whichever way, please list #233 as a blocker in the description — right now it is mentioned as a benign footnote.
2. (high) The three new bridge functions have no Rust tests at all
set_backup_confirmed, get_backup_confirmed and reset_backup_confirmation are untested. Only the pure restore_backup_confirmed helper and the serde default are covered. Details inline — the sharp edge is that commit 6cb67f7 reordered persist-before-commit specifically to fix a review finding, and nothing pins that ordering.
3. (high) The irreversible legacy dismissal happens before the authoritative write
Both call sites (account_screen.dart:82-83, backup_ritual_screen.dart:216-217) do:
await ref.read(backupReminderProvider.notifier).confirmBackupComplete(); // permanent local dismissal
await ref.read(backupCompletedProvider.notifier).markCompleted(); // authoritative Rust writeconfirmBackupComplete() sets kBackupReminderDismissedKey = true, which is permanent — the reminder never comes back. If markCompleted() then throws (no identity loaded, storage error), the user ends up with the reminder permanently dismissed and backup_confirmed = false: the account screen reports the backup as not done, and the prompt that would have asked them again is gone for good.
Swapping the two lines fixes it: do the authoritative Rust write first, and only dismiss locally once it succeeded. The catch at both call sites already handles the failure path correctly once the order is right.
4. (low) Dead legacy writes — the "single source of truth" goal is half done
After the migration runs, kBackupCompletedKey has exactly one reader left (backup_reminder_provider.dart:161, inside the one-shot migration) and two live writers: showBackupReminder() (line 88, writes false) and confirmBackupComplete() (line 110, writes true). Those writes now go nowhere. Either drop them or leave a comment saying why they stay — as it stands the next reader has to trace all three sites to work out which one is authoritative.
5. (low) Branch is CONFLICTING with main
Base is a149b8f (#264), 37 commits behind. I checked what actually conflicts: only rust/src/frb_generated.rs, which is generated — rebase and re-run ./scripts/frb-generate.sh, no manual merge needed.
On the semantic question you flagged
Keep it. Importing a mnemonic should not auto-confirm the backup, and the reason is stronger than the one in the description: the ritual verifies the user can reproduce the words from their own record. Typing words they are reading off the screen in front of them proves nothing about a backup existing anywhere. Unconfirmed-after-import is correct.
| anyhow!("StorageError: failed to persist backup_confirmed={confirmed}: {e}") | ||
| })?; | ||
| } | ||
| state.identity_info = updated; |
There was a problem hiding this comment.
(high) The persist-then-commit ordering this line implements has no test.
Commit 6cb67f7 moved the assignment here specifically because the earlier version mutated first and could report a confirmed backup that never reached disk — and, thanks to the == short-circuit at the top, could never be re-saved on retry. That is exactly the kind of fix that regresses silently the next time somebody "simplifies" this function, because nothing fails when the two lines swap back.
There is currently no test for set_backup_confirmed, get_backup_confirmed, or reset_backup_confirmation — only the pure restore_backup_confirmed helper and the serde default are covered.
load_derive_then_delete_identity_lifecycle is the established home for tests that need the identity_lock singleton (it is kept as one test precisely so parallel threads can't race it). Extending it there would cover:
set_backup_confirmed(true)against a working store →get_backup_confirmed()istrueanddb.get_identity()reportstrue;- against a failing store → returns
Err, andget_backup_confirmed()still reports the old value (this is the assertion that pins6cb67f7); - a retry after that failure, against a working store, actually writes — i.e. the short-circuit was not poisoned by a half-applied mutation.
(2) and (3) are the ones that would catch the regression the commit was written to prevent. Note this needs a Storage impl whose save_identity fails; temp_store always succeeds.
| /// backed up. Called when a new identity is generated so the security-relevant | ||
| /// reminder re-appears (issue #141). A no-op when no identity is loaded. | ||
| pub async fn reset_backup_confirmation() -> Result<()> { | ||
| if get_identity().await?.is_none() { |
There was a problem hiding this comment.
(low) Two lock acquisitions where one would do, and the guard can lose the race it exists to win.
get_identity() takes the read lock and drops it; set_backup_confirmed() then takes the write lock. If the identity is deleted between the two — delete_identity() only needs the write lock, which is free in that window — set_backup_confirmed hits its own ok_or_else(|| anyhow!("NoIdentity")) and the error escapes to Dart, where reset() throws. That is precisely the outcome this is_none() check was added to avoid.
Narrow, and the consequence is mild, but the fix is smaller than the check: drop the pre-flight entirely and let set_backup_confirmed decide under its single write guard, mapping NoIdentity to Ok(()) if a no-op is what you want.
Also worth noting for the caller: create_identity already constructs with backup_confirmed: false, so on the regenerate path (account_screen.dart:385) this call always hits the == short-circuit and does nothing. That is fine — it keeps the import path honest — but the doc comment reads as if it is doing the re-arming, when create_identity already did.
| // which is safe. The migration flag is only set once the copy sticks. | ||
| await _setConfirmed(true); | ||
| } | ||
| await prefs.setBool(_kMigratedKey, true); |
There was a problem hiding this comment.
(high) The marker is written even when the copy could not possibly have been durable — see point 1 of the summary.
On web, _setConfirmed(true) succeeds without persisting anything: main.dart:63 guards initDb with !kIsWeb, so app_db::db() is None and set_backup_confirmed skips its save_identity and returns Ok. This line then durably records "migration done" in localStorage — the one part of the sequence that does survive a reload.
So the legacy value is consumed to satisfy a write that evaporates, and step 2's restore_backup_confirmed(None, ...) returns false on every subsequent load. The reminder re-arms forever and the original answer is gone.
The comment above says "The migration flag is only set once the copy sticks" — that is true only for thrown failures. A successful-but-non-durable write is the case that actually happens on the platform this affects. Gating the migration on !kIsWeb, or on some signal that a store exists, would make the comment true.
| // fall back to unconfirmed so the reminder stays armed. | ||
| state = false; | ||
| } | ||
| _loaded = true; |
There was a problem hiding this comment.
(medium) _loaded = true runs on the failure path too, so a transient error pins the UI for the whole session.
If the catch above fires — the bridge is not ready, no identity is loaded yet when the provider is first watched — state becomes false and this line makes it permanent: load() is a no-op from here on, and nothing else ever re-reads the bridge. The user sees "not backed up" and an armed reminder until they restart the app, even though Rust knows better the moment the identity finishes loading.
Moving this inside the try, after state = await _getConfirmed(), makes the next load() retry instead. markCompleted() and reset() both await load() first, so a retry costs nothing.
(medium, related) load() has no in-flight guard and the constructor fires it un-awaited. If the user taps confirm while that first load() is still running, markCompleted() sees _loaded == false, starts a second concurrent load(), and sets state = true; the first one can then land in its catch and set state = false, reverting a confirmation that actually succeeded in Rust. The race predates this PR, but the catch-writes-false branch is new and is what makes it user-visible. Caching the in-flight future (Future<void>? _loading) closes both.
(low) catch (_) discards the error entirely. This is a security-relevant flag and the rest of this feature logs (debugPrint('[account] _confirmBackup error: $e')); a debugPrint here would turn "the reminder is back and I don't know why" into something diagnosable.
aa4b0b3 to
7fc71d0
Compare
There was a problem hiding this comment.
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 `@lib/features/account/providers/backup_reminder_provider.dart`:
- Around line 164-185: Update the confirmation persistence flow in load(),
markCompleted(), and reset() so web uses SharedPreferences as the authoritative
source for kBackupCompletedKey, while the Rust bridge remains limited to non-web
platforms. Ensure load() reads the web value after reload, markCompleted()
writes it, and reset() clears it; preserve the existing native migration
behavior and add coverage for web reload, confirmation, reset, and reminder
state.
🪄 Autofix
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: c31ed945-acae-4f06-abd0-2dd09326a335
📒 Files selected for processing (8)
lib/features/account/providers/backup_reminder_provider.dartlib/features/account/screens/account_screen.dartlib/features/account/screens/backup_ritual_screen.dartrust/src/api/identity.rsrust/src/api/types.rsrust/src/db/sqlite.rsrust/src/frb_generated.rstest/features/account/backup_reminder_provider_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- rust/src/api/types.rs
- rust/src/db/sqlite.rs
- rust/src/api/identity.rs
- test/features/account/backup_reminder_provider_test.dart
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/account/backup_reminder_provider_test.dart`:
- Around line 149-158: Update the bridge-backed notifier construction in this
test group to pass isWebOverride: false, including the makeNotifier() helper and
every nested notifier constructor, so tests consistently exercise the native
Rust bridge path regardless of the browser test environment.
- Around line 160-179: The BackupCompletedNotifier load flow must coalesce
concurrent calls so legacy migration writes Rust state only once. Update load()
to share an in-flight future, return it to overlapping callers, and clear it on
failure so retries remain possible; preserve the existing _loaded behavior for
completed loads. Extend the migration test around BackupCompletedNotifier to
verify the setConfirmed bridge callback is invoked exactly once.
🪄 Autofix
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: 48f1c4a9-f3ea-4967-9c8a-ce19cc362c14
📒 Files selected for processing (2)
lib/features/account/providers/backup_reminder_provider.darttest/features/account/backup_reminder_provider_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/account/providers/backup_reminder_provider.dart
|
Both addressed in the latest commit. Native test path: added Concurrent load() coalescing: |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the current head (21abf00b). The web SharedPreferences-authoritative path, native one-time migration, persist-before-commit Rust update, reset path, and concurrent load() coalescing are now covered by the code and tests. I also rechecked the prior unresolved threads: the high-severity storage/migration concerns are fixed on this head; the remaining reset preflight race is narrow/non-blocking and does not affect the PR's correctness.
Local verification:
cargo test --libpassed (258 passed, 8 ignored)cargo clippy --lib -- -D warningspassedgit diff --check origin/main...HEADpassed
GitHub checks for this head are green. I don't see any blocking issues.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
The direction is right and the earlier review rounds show. Two blocking findings below; both are reproduced, not inferred.
Blocking 1 — the web bug this PR fixed is still live on native
The diagnosis in the description is exactly right: the durable migration marker was written against a write that evaporates. But the root cause was never "web" — it's burning a durable marker against a write that may not be durable — and that reproduces on native.
set_backup_confirmed_with does if let Some(db) = db { save… }: with no store it skips the save, commits in memory, and returns Ok. And main.dart:63-73 deliberately treats an initDb failure as non-fatal ("running in memory-only mode"), so db() can be None for a whole session on a real device.
The chain: initDb fails → migration runs (not web) → _setConfirmed(true) returns Ok without touching disk → prefs.setBool(_kMigratedKey, true) is durable → restart → Rust says false, and the marker stops the legacy value from ever being read again → the confirmed backup is permanently lost and the reminder is armed forever. Same failure as web.
I reproduced this with a test that mirrors the no-store write: the marker gets burned, and after the simulated restart the state is false.
What makes this worth blocking on is that the module already has the right discipline and this PR departs from it. identity.rs:353 on main defines require_durable_storage, used by derive_trade_key with this reasoning: "without durable storage a derived index is consumed with no record of it… refusing here is what makes that explicit instead of silently corrupting the counter (issue #249)". Here the opposite choice is made, and set_backup_confirmed's doc comment defends it by saying the worst case "fails safe" because the reminder comes back. That was true before the migration marker existed; it isn't now.
Suggested fix: apply require_durable_storage on the native path of set_backup_confirmed (web exempt, exactly as derive_trade_key does it), or only write the marker after a verified read-back. Either way the doc comment needs updating — as written it asserts something this PR's own web analysis disproves.
Blocking 2 — web regression for legacy installs
On main, load() reads, on every platform:
prefs.getBool(kBackupCompletedKey) ?? prefs.getBool(kBackupReminderDismissedKey) ?? falseThat fallback exists because, per the code's own comment, "legacy installs only have the dismissed flag". In this PR the fallback survives only inside the migration block, which is !_isWeb. The web read path (_readConfirmed) is prefs.getBool(kBackupCompletedKey) ?? false — no fallback.
So a web user who confirmed their backup before kBackupCompletedKey existed flips from confirmed to unconfirmed. I verified it against the branch: state is false where main returns true. The web app is deployed at mostro.network/app, so the cohort isn't hypothetical — and this is precisely the case the PR says it preserves ("the legacy SharedPreferences flag stays authoritative on web").
One line in _readConfirmed, plus a test: the current web tests only ever seed kBackupCompletedKey, never the legacy dismissed flag on its own.
Non-blocking
The _loaded retry comment doesn't hold for the case it names. _load() says leaving _loaded = false lets a later load() retry "if the bridge was not ready (no identity yet)". But get_backup_confirmed is written specifically not to fail without an identity — it returns Ok(false). That case therefore doesn't throw, sets _loaded = true, and pins the notifier to false for the session. The window is narrow in practice (the provider is only built when the Account screen opens, well after identity load), but the comment promises protection that isn't there for the case it cites.
Latent merge conflict with #239. I checked the API: #239 still adds a struct FailingStore to the identity.rs test module, and this PR adds an identical one. Whichever lands second needs a rebase that dedupes it — git won't conflict textually (different regions of the test block) and the result won't compile. Worth deciding the merge order and noting it on both PRs.
Formatting churn. Of the ~431 changed lines in the two screens, roughly six are functional (the two swapped awaits and their comment). The rest is dart format under Dart 3.9's new style reformatting code written with an older SDK. CI doesn't run dart format --set-exit-if-changed (only flutter analyze and flutter test), so nothing requires this: it makes the PR harder to review, will conflict with any other PR touching those screens, and will bounce back when someone on a different SDK formats them. I'd split or revert it.
Verified working
The cross-identity guard does its job — restore_backup_confirmed compares the public key, so a leftover blob from another mnemonic can't leak its state. create_identity and import_from_nsec start at false, and reset() is wired at both identity-change sites (account_screen.dart:394 and :440). The screen reordering is correct: if markCompleted() throws, the catch runs before the permanent local dismissal. And blob compatibility holds in both directions — #[serde(default)] covers older records, and serde ignores unknown fields, so an older build reading the new blob is fine too.
|
#239 is merged, so this needs a rebase. Three things it will run into, in order: 1. Two conflicts in 2. A trait method that isn't in either branch yet. #320 added async fn update_trade_peer_reputation(
_order_id: &str,
_rating: f64,
_reviews: u32,
_days: u32,
) -> Result<()> {
unimplemented!()
}3. Worth fixing the class while you're in there. A hand-written stub that implements the whole Since this rebase has to rewrite Nothing above changes the review findings on the migration itself. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: fbecd766e8
ℹ️ 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".
| // commit discipline as the trade_key_index path (#217). | ||
| let mut updated = state.identity_info.clone(); | ||
| updated.backup_confirmed = confirmed; | ||
| if let Some(db) = db { |
There was a problem hiding this comment.
On native launches where initDb fails, main.dart deliberately continues in memory-only mode, so this branch skips save_identity but still returns Ok. The Dart confirmation flow then permanently dismisses the reminder, and the migration flow may persist its one-time marker, even though backup_confirmed exists only in memory; after a restart Rust restores false and the migration cannot retry, leaving the backed-up badge and reminder state inconsistent. Require a durable store on native before reporting success.
Useful? React with 👍 / 👎.
| // The backup-confirmed flag now lives in the Rust identity record. On the | ||
| // first run after upgrading, copy the legacy SharedPreferences value into | ||
| // Rust once, then read from Rust exclusively. |
There was a problem hiding this comment.
This implements the previously planned SharedPreferences-to-Rust migration, but specs/004-mostro-p2p-client/plan.md still says the state is entirely Dart-owned, labels these APIs as future work, and describes a SQLite column rather than the implemented identity JSON field. Update that active contract, including the intentional web fallback, so it no longer contradicts the shipped behavior.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
9a3a6f9 to
924219f
Compare
…cord Moves the backup-confirmed flag out of Dart SharedPreferences into the Rust identity record (Principle I), rebuilt cleanly on current main. Rust: - backup_confirmed on IdentityInfo (#[serde(default)] — legacy blobs load false). - get/set/reset with persist-then-commit (MostroP2P#217 discipline): save before committing in memory, so a failed save never reports a confirmed backup that didn't reach disk. - set requires durable storage only when confirming (true): a native memory-only session (initDb failed) must not report a non-durable confirm as success, or Dart burns the one-time migration marker against a write that evaporates on restart, permanently losing the flag (review). Reset (false) is fail-safe and does not require durability, so new-identity re-arm still works with no store. - restore_backup_confirmed guards on the public key so a blob from a different mnemonic can't leak its state. Dart: - BackupCompletedNotifier: native one-time migration (marker-guarded, coalesced load), native via the Rust bridge, web via SharedPreferences. - Web read falls back to the legacy dismissed key so a pre-migration confirmed web install isn't flipped to unconfirmed (review). - Both confirm sites do the Rust write before the permanent local dismissal. Tests: restore helper + serde default; persist-then-commit, the durable-storage gate, the no-store refusal and reset pinned in the identity_lock lifecycle test; Dart notifier (migration, coalesced load, native + web, dismissed-key fallback) and both screens. cargo test --lib green; clippy --locked clean; flutter analyze + account tests green.
924219f to
d1583db
Compare
|
Rebuilt cleanly on current main and tested manually on both platforms. CI is green (Rust native + wasm, Flutter, and the web smoke-test). Changes. Rebased onto main (the branch was 190 behind and carried `dart format` churn the two screens are now +6 lines each, just the functional `await` swap; no formatting noise). Review findings:
Manually verified native (Nokia C31): confirm backup -> `am force-stop` -> cold relaunch -> still "backed up" (same pubkey loaded from storage; the flag persists in the Rust identity record across a full process restart). Generate new user -> reminder re-arms. Restart again -> the new identity stays unconfirmed. Manually verified web (wasm, COOP/COEP): app loads and the backup confirm flow works (web stays on the SharedPreferences path). A page reload still hits the pre-existing `frb_generated.rs` `Option::unwrap()` panic on the `BondSlashedStream` / `TradeKeyIndexStream` opaque-stream teardown unrelated to this change and tied to #233's stubbed web storage, same as noted in the original description. That's why this PR keeps web authoritative on SharedPreferences; the fallback read is unit-covered. `cargo test --lib` green (389); `clippy --locked -- -D warnings` clean on native and wasm32; `flutter analyze` + account tests green. |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Re-reviewed at d1583db, merged against today's main (56 commits behind, merges clean). Both blockers from the previous round are fixed, and I verified them at runtime rather than by reading. One new blocker came out of that same session.
Runtime verification
Linux debug bundle of main + this PR, run against a sandboxed XDG_DATA_HOME so the identity blob and SharedPreferences could be seeded and read between launches. Memory-only mode was provoked the honest way — chmod 000 on mostro.db, which makes initDb fail exactly as app_bootstrap.dart:75-85 tolerates.
| Scenario | Result |
|---|---|
| Confirm the backup in the UI | backup_confirmed: true lands in the SQLite identity blob |
Tamper flutter.backupCompleted: false in prefs, relaunch |
Badge still shows — the Rust record is authoritative, prefs no longer decide |
| Confirm with no durable store | Error snackbar; [account] _confirmBackup error: AnyhowException(StorageUnavailable: …) |
| Migration with no durable store | [backup] load() failed, reminder stays armed; backupCompletedMigratedToRust absent, legacy backupCompleted: true intact |
| Same, then repair the store and relaunch | Migration completes by itself, badge appears, marker written — nothing lost, no need to redo the ritual |
| The permanent dismissal on a failed confirm | backupReminderDismissed stays false — the call-site swap does what it claims |
That fourth row is the previous Blocking 1: the failure mode it prevented (a durable marker burned against a write that evaporates) is demonstrably gone on native.
Also, on the merged tree: cargo test --locked → 415 passed; cargo clippy --locked -- -D warnings (the exact CI command) clean; cargo clippy --target wasm32-unknown-unknown clean; flutter analyze clean; flutter test → 333 passed; ./scripts/frb-generate.sh reproduces the committed rust/src/frb_generated.rs byte-for-byte. Plus two throwaway probes: an end-to-end pass through the real global path (set_backup_confirmed + APP_DB: confirm → persisted blob → reload restores it → the same blob under a foreign pubkey reads as unconfirmed), and a deserialization of a genuine pre-#266 identity blob taken out of a live install — #[serde(default)] holds on real data, trade_key_index and all.
Blocking 2 (the web legacy fallback) is fixed in code and covered by a test; I did not re-run it in a browser.
Blocking 1 — the Account screen can contradict itself: "backed up" and "confirm you wrote them down" at the same time
This PR splits one source of truth into two stores and reconciles neither:
- the green backed-up badge comes from Rust —
account_screen.dart:146,backupCompletedProvider→backup_confirmed; - the backup-ritual banner and the confirm checkbox come from SharedPreferences —
:84,_showBackupCheckbox = backupPending, driven bybackupReminderProvider.
BackupReminderNotifier.showBackupReminder() re-arms the reminder and writes kBackupCompletedKey = false, but never touches the Rust flag — and after the migration that prefs write is dead on native, because nothing reads the key any more. So the moment anything re-arms the reminder while Rust says confirmed, the user gets the ritual banner urging them to back up, the green badge saying they already did, and a checkbox to confirm it again, all on one screen.
Reproduced, no hand-seeding: with backup_confirmed: true in the identity blob, delete only the SharedPreferences file and keep the database. On Linux the two live in separate directories (~/.local/share/foundation.mostro.app/ vs ~/.local/share/mostro/), so this is an ordinary partial data loss. Without firstRunComplete the app enters the walkthrough, walkthrough_screen.dart:172 calls showBackupReminder(), and the Account screen renders all three at once.
Before this PR the badge and the checkbox read the same prefs key, so they could not disagree. This is a regression, and it's the kind users file screenshots about.
The fix is a line: either have showBackupReminder() clear the Rust flag as well, or drop the now-dead kBackupCompletedKey writes from BackupReminderNotifier and leave a comment saying Rust owns this state on native. Whichever you pick, please pin it with a test — this is exactly the seam the PR creates.
Blocking 2 — the scope this PR closes
#141's scope says "Add ... column to the identity table (SQLite + IndexedDB)" and "Update BackupReminderNotifier to read/write through the bridge". On web this PR deliberately does the opposite, which is the right call — but merging auto-closes #141 with the web half unimplemented and untracked: backup_confirmed appeared in exactly one open issue (#141), and #233 doesn't list it among the "new storage keys keep landing on top of it and each one silently becomes native-only" cases, which is precisely what this is. I opened #403 for the web half; please reference it from the description and from the _readConfirmed comment, and add a line to #233.
For the record on why the web half doesn't belong here: indexeddb.rs:197-205 returns Err("IndexedDB not yet implemented") for save_identity/get_identity, and implementing them changes nothing while app_bootstrap.dart:75 guards initDb with !kIsWeb. Lifting that guard switches the IndexedDB backend on for every subsystem at once (app_db holds one global Storage) — that's #233's job.
Blocking 3 — specs/004-mostro-p2p-client/plan.md still asserts the opposite of what ships
Codex's second P1 from Sep 3, unanswered:
- Objective 2 (L233): "Migrating this flag to the Rust storage layer ... is planned future work".
- Objective 3 (L234):
*(Planned future work)*for the three functions this PR adds. - The
> **Current implementation note**(L245): "managed entirely in Dart via BackupReminderNotifier and SharedPreferences". - The "Rust core (planned future work)" table (L251-252): prescribes a
backup_confirmed INTEGER NOT NULL DEFAULT 0column plus adb/schema.rsrow. What ships is a#[serde(default)]field inside the JSON blob, which is the correct shape — so the prescription needs correcting, not just marking done.
Document the deliberate web exception there too, linking #403. Per CLAUDE.md: "Specs are a living artifact — update the matching spec/contract as part of any behavior/contract change."
Non-blocking
4. rust/src/api/identity.rs:262-267 — the /// Import identity from an nsec ... no BIP-39 mnemonic backup doc now heads restore_backup_confirmed; import_from_nsec sits at L345 with no doc of its own. The insertion landed between the comment and its function.
5. identity.rs:802-806 — same class: the FailingStore doc ("A Storage whose save_identity always fails, for exercising the resync rollback path...") now heads the test restore_reads_the_persisted_backup_flag_for_the_same_identity, with struct FailingStore 30 lines below at L834. Move the four new tests under the struct, or the doc back onto it. That doc also covers the backup_confirmed path now, not just the resync rollback.
6. identity.rs:476-479 — the error message is wrong in this context, and users see it. Reusing require_durable_storage is right; its text isn't. What the log and the debug snackbar actually printed when I failed a backup confirmation:
[account] _confirmBackup error: AnyhowException(StorageUnavailable: deriving a trade key requires durable storage)
Safe to generalize: three callers now (L452 derive_trade_key, L546 ensure_trade_key_index_at_least, L323 set_backup_confirmed), and the only test that inspects it (deriving_without_durable_storage_is_refused, L979) asserts the StorageUnavailable: prefix, not the prose.
7. lib/features/account/providers/backup_reminder_provider.dart:240-247 — the comment still promises a retry for the case it names: "If the bridge was not ready (no identity yet), leaving _loaded false lets the next load() retry". But get_backup_confirmed is written specifically not to fail without an identity — it returns Ok(false). That path doesn't throw, so _loaded = true and the notifier is pinned to false for the session. Small impact (only account_screen.dart:146 observes it, by which time the identity has loaded), but the comment should stop promising protection that isn't there.
8. Refresh the PR description. It still describes an earlier revision — it mentions neither the native durable-storage gate nor the web fallback read, the two substantive changes in the last push (they live only in the Sep 5 comment). This body becomes the squash commit message.
9. Observed, not a request: in a memory-only session the UI goes mixed the other way — no reminder (the local dismissal survives) and no badge (Rust reads false). It converges on the next healthy launch and loses nothing, so I'd leave it; noting it so it isn't mistaken for a bug later.
Not asking for
- A widget test with a throwing
setConfirmed. It would pin themarkCompleted()→confirmBackupComplete()ordering, which the durable-storage gate makes genuinely reachable now — but I confirmed that behaviour by hand instead (the reminder stayed armed and undismissed after a failed confirm). - Reverting the
cargo fmtchurn inidentity.rs(~8 unrelated hunks): CI doesn't runcargo fmt --checkand it moves the file toward canonical rustfmt. Just don't let it grow. Thedart formatchurn is gone — the two screens are +6/-2 now, only the functional swap.
|
This merges cleanly, but its web gate now protects against something that no longer exists. #408 ( What to do on rebase:
|
|
@codaMW please fix the conflicts and address the reviews |
3 participants
Closes #141. Moves the backup-confirmed flag out of Dart
SharedPreferencesinto the Rust identity record, per Principle I (Rust core, Flutter shell). grunch verified the issue is still valid on 2026-07-16.Blocked by #233
On web,
init_dbis!kIsWeb-guarded so there is no store, and IndexedDBsave_identityis a stub. Until #233 lands durable web identity storage, this PR deliberately does not migrate on web see the web section below. #233 is a hard blocker for makingbackup_confirmeddurable on web; on native this ships as-is.Scope
Migrates only the security-relevant
backup_confirmedflag (thebackupCompletedstate). The reminder-scheduling state (active / dismissed / snoozed) stays in Dart it's a UI concern, not identity state.Rust
backup_confirmedadded toIdentityInfo, serialized into the existing identity JSON blob.#[serde(default)]so identities persisted before this field deserialize asfalse(unconfirmed -> reminder stays armed). No schema migration the identity is a JSON blob, not columns.get_backup_confirmed/set_backup_confirmed/reset_backup_confirmationinidentity.rs.setpersists viasave_identitybefore committing in memory, mirroring thetrade_key_indexpersist-then-commit discipline from Restore: resync trade_key_index to the max recovered index (prevents trade-key reuse) #217: a save failure returnsErrand leaves the in-memory flag unchanged, so a confirmed backup is never reported unless it reached disk.create_identity/import_from_nsecconstruct withbackup_confirmed: falsea fresh mnemonic is by definition not backed up, which re-arms the reminder for a new identity (grunch's stated concern).load_identity_from_mnemonicrestores the flag from the persisted blob via a purerestore_backup_confirmedhelper, guarded on the public key so a leftover blob from another mnemonic can't leak its state.Web behaviour (the important correction)
An earlier revision described web as "the flag doesn't persist, which fails safe." That was wrong: it fails permanently. On web
set_backup_confirmedhas no store and returnsOkwithout persisting, but the migration marker is written to localStorage durably so the legacy SharedPreferences value was consumed to satisfy a write that evaporates, and the reminder re-armed on every reload, permanently, destroying state that previously persisted. Fixed by gating the whole migration behind!kIsWeb: on web the migration never runs and the legacy SharedPreferences flag stays authoritative until #233 lands durable web storage.Semantic choice
Importing a mnemonic does not auto-confirm the backup typing recovery words isn't the in-app verification ritual so an imported identity with no persisted flag stays unconfirmed.
Dart
BackupCompletedNotifierreads/writes through the bridge, with a one-time copy of the legacy SharedPreferences value into Rust (guarded by a migration marker, and skipped entirely on web) and a fallback tofalsewhen the bridge is unavailable.markCompleted) before the permanent local dismissal (confirmBackupComplete), so a failed write can't leave the reminder permanently dismissed whilebackup_confirmedstays false._loadedis set only after the successful bridge read, so a transient failure lets the nextload()retry instead of pinning the UI to unconfirmed for the session; the failure is logged.identity_apifunctions) so the notifier is testable without a live Rust runtime.Tests
restore_backup_confirmedunit tests (same-identity read, default-false, cross-identity guard), a serde-default deserialization test, a SQLite round-trip, and folded into theidentity_locklifecycle test the persist-then-commit path against an injectedFailingStore:set_backup_confirmed_witherrors withStorageError:and leaves the flag unchanged, a retry against a working store writes, andreset_backup_confirmationclears the flag. This pins the ordering commiteb63419introduced.markCompleted, andresetexercised through fake bridge functions.scripts/build-web.sh) and served with COOP/COEP. Identity persists across reload (same pubkey, "identity loaded" not "created") and the!kIsWebmigration gate is in effect. The account/backup UI is unstable after reload due to pre-existing wasm-threading panics unrelated to this change (Atomics.wait cannot be called in this context, and anOption::unwrap()onNoneinfrb_generated.rson the bond-slashed stream) consistent with web storage being stubbed (Web: IndexedDB storage backend is a stub — nothing persists across a reload #233), which is why this PR keeps web on the legacy SharedPreferences path. Native persist-then-commit is covered by the unit test above. (Filing the wasm panics separately.)cargo test --lib/clippy -D warnings/cargo check --target wasm32/flutter analyzeclean.Summary by CodeRabbit
New Features
Bug Fixes