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 3 commits into
Conversation
…to the Rust identity record The backup-confirmed flag lived in Dart SharedPreferences, violating Principle I (Rust core, Flutter shell). Move the security-relevant state into the Rust identity record, keeping the reminder-scheduling state (active / dismissed / snoozed) in Dart since that is UI concern. Rust: - Add backup_confirmed to IdentityInfo, serialized into the existing identity JSON blob. #[serde(default)] so identities persisted before this field load as false (unconfirmed → reminder stays armed); no schema migration. - get/set/reset_backup_confirmation in identity.rs. set persists via save_identity, mirroring the trade_key_index durability pattern (MostroP2P#217) — but best-effort, not required: a lost flag only re-arms the reminder (safe), unlike a lost key index. On the web IndexedDB backend (save_identity is a stub) the flag simply does not persist, which fails safe. - create_identity / import_from_nsec construct with backup_confirmed: false; a fresh mnemonic is by definition not backed up (this re-arms the reminder for a new identity). load_identity_from_mnemonic restores the flag from the persisted blob via a pure restore_backup_confirmed helper — guarded on the public key so a leftover blof from another mnemonic cannot leak its state. - Semantic choice worth review: importing a mnemonic does NOT auto-confirm the backup — typing recovery words is not the in-app verification ritual — so an imported identity with no persisted flag stays unconfirmed. Dart: - BackupCompletedNotifier reads/writes through the bridge, with a one-time copy of the legacy SharedPreferences value into Rust (guarded by a migration marker) and a fallback to false when the bridge is unavailable. - The three bridge calls are injectable (constructor params defaulting to the real identity_api functions) so the notifier is testable without a live Rust runtime — the seam pattern MostroP2P#213 established. Tests: restore_backup_confirmed unit tests (same-identity read, default-false, cross-identity guard), a serde-default deserialization test, and a SQLite save/load round-trip asserting the flag persists. Dart tests exercise the migration, read, markCompleted and reset through fake bridge functions. Verification: cargo test (239) / clippy / wasm check clean; flutter analyze clean; account tests pass. (Pre-existing, unrelated escrow_mode_dev_card test failures reproduce on clean main.)
|
Warning Review limit reached
Next review available in: 47 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 (1)
WalkthroughBackup confirmation now persists in Rust identity state. The Dart notifier performs a one-time legacy migration, then uses Rust bridge APIs for loading, completion, and reset. Identity loading validates the stored public key before restoring the flag. ChangesBackup confirmation persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BackupCompletedNotifier
participant SharedPreferences
participant RustIdentityAPI
participant IdentityStorage
BackupCompletedNotifier->>SharedPreferences: Read legacy completion value
BackupCompletedNotifier->>RustIdentityAPI: Migrate or get confirmation
RustIdentityAPI->>IdentityStorage: Read or persist identity flag
IdentityStorage-->>RustIdentityAPI: Return confirmation state
RustIdentityAPI-->>BackupCompletedNotifier: Return confirmation state
BackupCompletedNotifier->>RustIdentityAPI: Set or reset confirmation
RustIdentityAPI->>IdentityStorage: Persist updated flag
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
…memory (CodeRabbit) set_backup_confirmed mutated state.identity_info before save_identity could fail. On a persistence failure the session would report a confirmed backup that never reached disk, and the no-op short-circuit would stop a retry from re-saving — so the flag silently vanished on restart. Build the updated record, persist it, and assign to state only after the save succeeds (the persist-then- commit discipline from MostroP2P#217). Also strengthen the Dart markCompleted/reset tests to assert the fake bridge's backing value changed, not only that notifier.state flipped.
…fake bridge (CodeRabbit) The markCompleted/reset tests asserted only notifier.state, which would pass even if the bridge write regressed. Hold the fake's backing value and assert it flips to true/false.
|
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.
2 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.Scope
Migrates only the security-relevant
backup_confirmedflag (thebackupCompletedstate). The reminder-scheduling state (active / dismissed / snoozed) stays in Dart it's 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_identity, mirroring thetrade_key_indexdurability pattern from Restore: resync trade_key_index to the max recovered index (prevents trade-key reuse) #217 but best-effort, not required: a lost flag only re-arms the reminder (safe), unlike a lost key index (which causes CantDo). On the web IndexedDB backend (save_identityis a stub) the flag simply doesn't persist, which fails safe.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.Semantic choice worth a look
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. Happy to change if you'd prefer import-implies-backed-up.
Dart
BackupCompletedNotifierreads/writes through the bridge, with a one-time copy of the legacy SharedPreferences value into Rust (guarded by a migration marker) and a fallback tofalsewhen the bridge is unavailable.identity_apifunctions) so the notifier is testable without a live Rust runtime the seam pattern feat(bond): recognize pay-bond-invoice and add taker bond payment flow #213 established.Tests
restore_backup_confirmedunit tests (same-identity read, default-false, cross-identity guard), a serde-default deserialization test, and a SQLite save/load round-trip asserting the flag persists.markCompleted, andresetexercised through fake bridge functions.cargo test(239) /clippy -D warnings/cargo check --target wasm32clean;flutter analyzeclean; account tests pass.Note: pre-existing
escrow_mode_dev_card_testfailures reproduce on clean main (unrelated to this change).Summary by CodeRabbit
New Features
Bug Fixes