feat(lifecycle): drop mnemonic on app-hidden#485
Merged
TaprootFreak merged 3 commits intoMay 21, 2026
Conversation
6 tasks
Blume1977
force-pushed
the
feat/lock-mnemonic-on-app-hidden
branch
from
3ddfc0a to
368873a
Compare
Contributor
Review-Findings (Logik + Konsistenz)Zwei parallele Reviews — hier konsolidiert mit den Punkten, die vor Merge adressiert werden sollten. Blocker1. Naming-Kollision
Im Onboarding (currentWalletId gesetzt, 2. Holder-Counter-Race bei Empfehlungen3. Kommentar-Länge in 4. Race-Doku für 5. Test-Setup-Drift 6. PR-Description vs. Realität: Onboarding-Mnemonic 7. Doku-Ungenauigkeit Positiv
Vor Merge
5 und 7 sind Nice-to-have, könnten aber in derselben Iteration mit. |
Adds a non-throwing predicate on [AppStore] so services can early-return from lifecycle-triggered paths that fire before [HomeBloc] has populated the wallet. The first caller landing in the follow-up commit is [WalletService.lockCurrentWallet], which the app-lifecycle `hidden` hook invokes — and that hook fires the first time the user backgrounds the app, which can be during the onboarding flow before any wallet exists. Named distinctly from `WalletService.hasWallet()` so the two predicates can't be confused at the call site: * `AppStore.isWalletLoaded` — in-memory load state (`_wallet != null`) * `WalletService.hasWallet()` — persisted state (`currentWalletId != null`) The two diverge during onboarding when a wallet id has been persisted but `_wallet` is not yet populated.
The app-lifecycle `hidden` hook (added in the follow-up commit) calls
[lockCurrentWallet] without a paired [ensureCurrentWalletUnlocked]. That
hook fires the first time the user backgrounds the app, which can happen
during the onboarding flow — before [HomeBloc] has populated
[AppStore.wallet]. Without a guard, [_lockWalletInPlace] would
dereference the unset wallet via [AppStore.wallet] and raise
`Exception('No Wallet set')` — which the unawaited Future at the call
site would surface as an unhandled async error.
Push the precondition into [WalletService] itself via the
[AppStore.isWalletLoaded] predicate, so the lifecycle caller stays a
clean one-liner and a future lockCurrentWallet extension (DB write,
audit log) doesn't get its errors silently caught at the call site. The
Single-Responsibility cleanup matches the reviewer feedback on the
original lifecycle PR.
Regression tests added:
* `!isWalletLoaded` no-op (the new guard fires the early-return)
* Lifecycle-hidden unpaired lock + sign-flow finally — pins the
counter-underflow guard, so the 1:1 ensure↔lock invariant breaks
safely (no counter drift, next ensure cycle still works).
setUp for the `lockCurrentWallet` and `ensure/lock reentrancy` groups now
sets `isWalletLoaded = true` locally instead of globally; the unrelated
groups don't get the stub and the negative path can't accidentally pass
because of the default.
Blume1977
force-pushed
the
feat/lock-mnemonic-on-app-hidden
branch
from
368873a to
d1e533f
Compare
Contributor
Re-Review nach neuen Commits (
|
| # | Finding | Status | Fix |
|---|---|---|---|
| 1 | Naming-Kollision hasWallet |
✓ adressiert | AppStore.isWalletLoaded (lib/packages/service/app_store.dart:33), Doc trennt In-Memory- von Persistenz-Semantik |
| 2 | Holder-Counter-Race-Test (Sign-mid-flight + Hidden) | ✓ adressiert | test/packages/service/wallet_service_test.dart:387-433 — pinnt ensure → unpaired lifecycle-lock → finally-lock → next ensure |
| 3 | 15-Zeilen-Kommentar in lifecycle_initializer.dart |
✓ adressiert | Auf 5 Zeilen kondensiert (lib/setup/lifecycle_initializer.dart:56-60), Rationale wandert in Commit-Body |
| 4 | Onboarding-Scope-Gap im PR-Body | ✓ adressiert | Eigener Abschnitt "Scope gap (deliberately deferred)" im PR-Body |
| 5 | Doku-Ungenauigkeit AppStore.hasWallet-Setter |
✓ adressiert | Doc nennt jetzt beide Pfade (LoadCurrentWalletEvent + LoadWalletEvent) |
Alle fünf sauber abgehakt. Per-Group-setUp für isWalletLoaded (wallet_service_test.dart:320-322, :375-378) statt globalem Stub ist die richtige Variante.
Neue Findings
[gering] Staler Test-Kommentar — test/setup/lifecycle_initializer_test.dart:63 referenziert noch appStore.hasWallet, sollte appStore.isWalletLoaded sein. Einzeiler.
[info] Race _onHidden vs. _unlockInFlight — lib/packages/service/wallet_service.dart:153-162: Wenn _onHidden mitten in einem pending Unlock feuert, dekrementiert lockCurrentWallet den Counter auf 0 und _lockWalletInPlace no-op't (Wallet ist noch SoftwareViewWallet). Anschließend completes das inFlight-Future und schreibt SoftwareWallet in _appStore.wallet. Mnemonic lebt damit kurz nach _onHidden wieder im Speicher, bis das Sign-Flow-finally lock greift. Zeitfenster <100ms, 60s-Safety-Timer fängt es ab — aber technisch eine Erweichung des deklarierten Ziels "drop mnemonic before iOS suspends isolate". Optional als Follow-up.
[info] Doc-Inkonsistenz — lib/packages/service/app_store.dart:14: Setter ist ohne Doc, während der Getter ausführlich dokumentiert ist. Stilfrage, nicht-blockierend.
[info] Architecture-Lock-In-Test für unawaited-Nichtblockierung fehlt — der Error-Bubble-Test (lifecycle_initializer_test.dart:53-71) ist gut, ein expliziter Sync-Return-Test wäre fast tautologisch. Niedrige Priorität.
Positiv
- Drei atomare Commits mit Begründung im Body — beispielhaft für DFX-Repos
- Doc-Block am
isWalletLoaded-Getter nennt beide Setter-Pfade und kontrastiert mitWalletService.hasWallet() - Architektur-Lock-In-Test zementiert die Designentscheidung "kein
try/catchim Caller" - Kommentar in Test-Setup erklärt warum der Stub lokal statt global ist
- PR-Body ist vollständig (Test-Plan, UX-Impact, Scope-Gap)
- 1425/1425 Tests grün
Gesamturteil
Mergebar. Die fünf Blocker sind adressiert, neue Architektur ist defensiver und gleichzeitig minimaler als die ursprüngliche Variante. Vor Merge sollte nur der staler Test-Kommentar (!appStore.hasWallet → !appStore.isWalletLoaded in lifecycle_initializer_test.dart:63) als Trivial-Commit nachgeschoben oder beim Squash-Merge nachgepflegt werden.
Die Logik-Findings [info] sind Follow-up-würdig, nicht blockierend.
Blume1977
force-pushed
the
feat/lock-mnemonic-on-app-hidden
branch
2 times, most recently
from
a381baf to
b60c03d
Compare
The 60 s post-unlock timer in [WalletService] is a best-effort safety net — iOS suspends Dart timers in the background, so a backgrounded app could otherwise keep an unlocked [SoftwareWallet] (mnemonic in memory) resident until the OS kills the process. That's the residual gap the reviewer flagged in RealUnitCH#468 (4.2) after the explicit lockCurrentWallet() pattern landed. `LifecycleInitializer._onHidden` now fires `WalletService.lockCurrentWallet()` alongside the existing `PinAuthCubit.onAppHidden()`, so the mnemonic is dropped as soon as the user covers the app — before iOS suspends the isolate. The next sign re-decrypts the mnemonic via the OS-keystore-wrapped key in sub-100 ms, invisible to the user. `hidden` is the right hook: fires earlier than `paused` on iOS (covers multitasking-switcher + notification-drag-down), Android raises it via the unified lifecycle pipeline, and `PinAuthCubit` already uses it for the lockout-time stamp. The call is `unawaited(...)` — lifecycle callbacks are sync, and `lockCurrentWallet` is defensive (early-returns when no wallet is loaded yet, see previous commit). No try/catch / catchError on the unawaited Future, by design: a future Future.error from a lockCurrentWallet extension (DB write, audit log) is a real regression that must surface in the Zone uncaught-error sink — not get silently swallowed at the call site. The decision is documented inline in `_onHidden` because no test can reliably encode it: every variant (`thenThrow`, async-throw closure, `Future.error(...)`) routes the failure through the testWidgets framework's synchronous catch rather than the Zone sink that `tester.takeException()` reads from, and the routing differs between Flutter 3.41 and 3.44. The inline comment is kept short to match the brevity of the neighbouring `_onResumed`/`_onPaused` handlers; the longer rationale lives in this commit message and the PR body. Tests cover: hidden → lock called once, paused / resumed → no lock call.
Blume1977
force-pushed
the
feat/lock-mnemonic-on-app-hidden
branch
from
b60c03d to
38a69b6
Compare
Contributor
Re-Review nach Force-Push (
|
| # | Finding | Status | Verifikation |
|---|---|---|---|
| 1 | Staler Test-Kommentar !appStore.hasWallet |
✓ FIXED | Durch Löschung des betroffenen Tests; grep hasWallet in lifecycle_initializer_test.dart leer |
| 2 | Race _onHidden vs. _unlockInFlight |
✗ offen (info) | Race-Fenster <100ms besteht weiter, abgesichert durch Holder-Counter + 60s-Force-Lock (wallet_service.dart:177-191); nicht im Scope dieses PRs |
| 3 | app_store.dart:15 Setter ohne Doc |
✗ offen (info) | Setter weiter undokumentiert, während isWalletLoaded-Getter ausführlich dokumentiert ist |
| 4 | Optionaler unawaited-Test |
✓ bewusst nicht adressiert | Begründung in Source (lifecycle_initializer.dart:56-60), Test-Kommentar (lifecycle_initializer_test.dart:61-70) und PR-Body lokalisiert — Flutter-Version-Dispatch-Routing macht Test flaky |
Sind die ursprünglichen 5 Blocker noch adressiert?
| Blocker | Status |
|---|---|
Naming-Kollision hasWallet/isWalletLoaded |
✓ unverändert |
| Holder-Counter-Race-Test | ✓ unverändert (wallet_service_test.dart:388) |
| Inline-Kommentar-Länge | ✓ sogar weiter verdichtet |
PR-Body Scope-Gap zu CreateWalletState.wallet |
✓ unverändert |
Doku-Ungenauigkeit (LoadCurrentWalletEvent + LoadWalletEvent) |
✓ unverändert |
Kein Rückbau.
Neue Findings
- [info] Der ersetzende Erklär-Block in
lifecycle_initializer_test.dart:61-70sind 10 Zeilen reiner Kommentar ohnetest(...)-Aufruf. Manche Teams werten das als Code-Smell — hier durch CI-Stabilität gerechtfertigt, da der gelöschtetakeException-Test je nach Flutter-Version unterschiedlich dispatched. - [info] Die Microtask-Race-Notiz in
lifecycle_initializer.dart:56-60ist kürzer und etwas vager geworden — erfüllt aber ihren Tripwire-Zweck für künftige Modifier von_onResumed. - Keine Regressionen, keine neue Logik, keine Scope-Erweiterung.
Gesamturteil: Mergebar
Der Force-Push war minimal-invasiv und ausschließlich risikoreduzierend: entfernt einen potentiell flaky Architektur-Lock-In-Test und ersetzt ihn durch dokumentierte Designentscheidung mit Mehrfach-Verankerung (Source, Test, PR-Body). Trade-off ist vertretbar — der gelöschte Test wäre in einem zukünftigen Flutter-Upgrade ein false-positive geworden.
Restliche offene Punkte (app_store.dart:15 Setter-Doc, _unlockInFlight-Race) sind Polish/Follow-up-Material, keine Merge-Blocker.
This was referenced May 21, 2026
7 tasks
Blume1977
added a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
RealUnitCH#485 dropped the mnemonic on app-hidden via `WalletService.lockCurrentWallet()`, but a narrow race remained: when the lifecycle hook fired between a sign-flow's `ensureCurrentWalletUnlocked` starting and its DB-read + AES-GCM decrypt resolving, [AppStore.wallet] was still a [SoftwareViewWallet] at lock time (so `_lockWalletInPlace` no-op'd), then the in-flight unlock resolved and wrote the unlocked [SoftwareWallet] back. The mnemonic briefly resurfaced in memory until either the 60s safety-net timer or the sign-flow `finally lock` caught it. The 60s safety net is best-effort under iOS isolate suspension — that's the gap RealUnitCH#485 set out to close at the source for the foreground case. This commit extends the same principle to the in-flight unlock window: when `lockCurrentWallet` decrements the last holder, invalidate `_unlockInFlight` (both `ignore()` to silence any later Future.error and `= null` to break the identity check). The `ensureCurrentWalletUnlocked` write to `_appStore.wallet` is now gated on the in-flight token still being current — so a still-pending unlock that the lock has invalidated does not resurface the mnemonic. The in-flight sign-flow itself fails by design: it captured `credentials = appStore.wallet.currentAccount.primaryAddress` before its ensure awaited, the lock-during-flight invalidates the unlock, and `appStore.wallet` stays a [SoftwareViewWallet]. The captured `credentials` is the *old* `_LockedCredentials` from the view-wallet — `signToSignature` throws `StateError('SoftwareViewWallet credentials reached a sign call…')` (and the `assert(false, …)` trips in dev) which is caught by the existing `finally lock` in the sign flow. Security over usability: the mid-sign attempt fails, the *next* sign re-decrypts cleanly. The existing test `lock between two in-flight ensures preserves the unlocked wallet` continues to pass because in that scenario the counter goes from 2 to 1 (still > 0) and `lockCurrentWallet` returns before the invalidation, keeping the dedup contract intact for genuine concurrent holders. Only the single-holder-at-lock-time path (counter goes to 0) invalidates — exactly the case the lifecycle hook hits. While here, gate the post-unlock 60s timer on the same in-flight identity check: when the lock invalidated our write, arming the timer points at a [SoftwareViewWallet] and `_lockWalletInPlace` would safely no-op via `is! SoftwareWallet` — not a correctness bug, just dead work. The `landedInStore` local skips the arm in that case. New Completer-gated test pins the sequence: ensure-mid-flight → lock → resolve unlock → [AppStore.wallet] stays a [SoftwareViewWallet]. Reuses the gated-repository pattern from the neighbouring in-flight tests. Closes RealUnitCH#488.
Blume1977
added a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
…dden RealUnitCH#485 dropped the mnemonic stored in `AppStore.wallet` via `WalletService.lockCurrentWallet()` on app-hidden. During onboarding the freshly generated mnemonic lives in [CreateWalletState.wallet] (cubit state), NOT in [AppStore.wallet] — so the service-level lock no-op'd on this path via its `!isWalletLoaded` guard, leaving the just-generated seed resident in cubit memory if iOS suspended the isolate while the user was on the create-wallet screen. This is precisely the moment the mnemonic is most vulnerable: freshly generated, not yet backed up. A phone call coming in, or the user pulling down the notification shade, hands iOS the chance to snapshot the isolate. Option A from the issue: [CreateWalletCubit] owns its own [AppLifecycleListener] and resets state on `hidden`, dropping the wallet reference (and with it the mnemonic). The cubit remains the owner of its state — no extra indirection through a service-level cleanup hook. Re-fire `createWallet()` after the clear. The cubit is built once via `BlocProvider(create: (_) => CreateWalletCubit(...)..createWallet())` in `CreateWalletPage`, so the constructor cascade fires `createWallet()` exactly once. Without re-firing inside `_dropMnemonic`, the user would resume to `state.wallet == null` and the view's `BlocBuilder` would render `CupertinoActivityIndicator` indefinitely (escapable only via the AppBar back button). The fix: after `emit(const CreateWalletState())` clears the state, kick off a fresh `createWallet()` synchronously so the next emission replaces the cleared state. The screen briefly flashes the loading indicator, then re-renders with a NEW mnemonic — the prior in-memory seed is already gone before the new one is generated. Why `hidden` (and not `paused`): same reasoning as the service path — fires earlier than `paused` on iOS (multitasking switcher + notification drag-down), Android raises it through the unified lifecycle pipeline, and `PinAuthCubit` + `WalletService.lockCurrentWallet()` (RealUnitCH#485) already use `hidden` for the same purpose. The listener is `dispose()`'d in `close()` so the cubit's lifecycle stays self-contained — no leaks if the user navigates away before generation completes. Tests cover: * hidden → first emission is the cleared state (mnemonic dropped), asserted via an emissions log to survive the synchronous regenerate that follows. * hidden with no wallet generated yet → no emission (`same` ref). * hidden → resumed re-generates a fresh wallet; emissions log pins cleared-then-new-wallet ordering and asserts `createSeedWallet` was called twice. * inactive / resumed → state preserved. Parameterised so a future refactor (e.g. switching to a `switch` with a default-clear) can't silently regress the contract for non-`hidden` lifecycle states. `paused` / `detached` are unreachable from `resumed` without going through `hidden` per Flutter's `AppLifecycleListener` state machine — those paths exercise the same clear-and-regenerate cycle anyway, so the dedicated `hidden` tests pin them. VerifySeedCubit (`lib/screens/verify_seed/`) also receives a [SoftwareWallet] from the create flow and holds it for the seed-quiz step. It is not addressed here because (1) the issue scopes Option A to [CreateWalletCubit] specifically, and (2) `VerifySeedCubit` consumes a wallet owned by its parent — dropping it independently would break the seed-quiz flow on transient hides. Worth a follow-up if the threat model demands it. Closes RealUnitCH#489.
Blume1977
added a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
RealUnitCH#485 dropped the mnemonic on app-hidden via `WalletService.lockCurrentWallet()`, but a narrow race remained: when the lifecycle hook fired between a sign-flow's `ensureCurrentWalletUnlocked` starting and its DB-read + AES-GCM decrypt resolving, [AppStore.wallet] was still a [SoftwareViewWallet] at lock time (so `_lockWalletInPlace` no-op'd), then the in-flight unlock resolved and wrote the unlocked [SoftwareWallet] back. The mnemonic briefly resurfaced in memory until either the 60s safety-net timer or the sign-flow `finally lock` caught it. The 60s safety net is best-effort under iOS isolate suspension — that's the gap RealUnitCH#485 set out to close at the source for the foreground case. This commit extends the same principle to the in-flight unlock window: when `lockCurrentWallet` decrements the last holder, invalidate `_unlockInFlight` (both `ignore()` to silence any later Future.error and `= null` to break the identity check). The `ensureCurrentWalletUnlocked` write to `_appStore.wallet` is now gated on the in-flight token still being current — so a still-pending unlock that the lock has invalidated does not resurface the mnemonic. The in-flight sign-flow itself fails by design: it captured `credentials = appStore.wallet.currentAccount.primaryAddress` before its ensure awaited, the lock-during-flight invalidates the unlock, and `appStore.wallet` stays a [SoftwareViewWallet]. The captured `credentials` is the *old* `_LockedCredentials` from the view-wallet — `signToSignature` throws `StateError('SoftwareViewWallet credentials reached a sign call…')` (and the `assert(false, …)` trips in dev) which is caught by the existing `finally lock` in the sign flow. Security over usability: the mid-sign attempt fails, the *next* sign re-decrypts cleanly. The existing test `lock between two in-flight ensures preserves the unlocked wallet` continues to pass because in that scenario the counter goes from 2 to 1 (still > 0) and `lockCurrentWallet` returns before the invalidation, keeping the dedup contract intact for genuine concurrent holders. Only the single-holder-at-lock-time path (counter goes to 0) invalidates — exactly the case the lifecycle hook hits. While here, gate the post-unlock 60s timer on the same in-flight identity check: when the lock invalidated our write, arming the timer points at a [SoftwareViewWallet] and `_lockWalletInPlace` would safely no-op via `is! SoftwareWallet` — not a correctness bug, just dead work. The `landedInStore` local skips the arm in that case. New Completer-gated test pins the sequence: ensure-mid-flight → lock → resolve unlock → [AppStore.wallet] stays a [SoftwareViewWallet]. Reuses the gated-repository pattern from the neighbouring in-flight tests. The test asserts both the outcome (`stored.last is SoftwareViewWallet`) and the mechanism (`verifyNever(() => appStore.wallet = isA<SoftwareWallet>())`), so a future refactor cannot regress the gate while still passing by clearing the mnemonic via some other write — the suppressed write itself is the contract. Closes RealUnitCH#488.
Blume1977
added a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
…dden RealUnitCH#485 dropped the mnemonic stored in `AppStore.wallet` via `WalletService.lockCurrentWallet()` on app-hidden. During onboarding the freshly generated mnemonic lives in [CreateWalletState.wallet] (cubit state), NOT in [AppStore.wallet] — so the service-level lock no-op'd on this path via its `!isWalletLoaded` guard, leaving the just-generated seed resident in cubit memory if iOS suspended the isolate while the user was on the create-wallet screen. This is precisely the moment the mnemonic is most vulnerable: freshly generated, not yet backed up. A phone call coming in, or the user pulling down the notification shade, hands iOS the chance to snapshot the isolate. Option A from the issue: [CreateWalletCubit] owns its own [AppLifecycleListener] and resets state on `hidden`, dropping the wallet reference (and with it the mnemonic). The cubit remains the owner of its state — no extra indirection through a service-level cleanup hook. Re-fire `createWallet()` after the clear. The cubit is built once via `BlocProvider(create: (_) => CreateWalletCubit(...)..createWallet())` in `CreateWalletPage`, so the constructor cascade fires `createWallet()` exactly once. Without re-firing inside `_dropMnemonic`, the user would resume to `state.wallet == null` and the view's `BlocBuilder` would render `CupertinoActivityIndicator` indefinitely (escapable only via the AppBar back button). The fix: after `emit(const CreateWalletState())` clears the state, kick off a fresh `createWallet()` synchronously so the next emission replaces the cleared state. The screen briefly flashes the loading indicator, then re-renders with a NEW mnemonic — the prior in-memory seed is already gone before the new one is generated. The re-fire widens the async-tail window for `createWallet()`: user hides → `_dropMnemonic` re-fires generation → user resumes → user taps the AppBar back before the regenerated `createSeedWallet` resolves → cubit `close()` runs → the pending `emit(state.copyWith(wallet:))` would throw `StateError('Cannot emit new states after calling close')`. Guard with `if (isClosed) return;` at the async tail before the emit, matching the pattern used in `connect_bitbox_cubit` and `kyc_cubit`. The fire-and-forget `warmAuthSignature` does not emit on the cubit, so no guard is needed there. Why `hidden` (and not `paused`): same reasoning as the service path — fires earlier than `paused` on iOS (multitasking switcher + notification drag-down), Android raises it through the unified lifecycle pipeline, and `PinAuthCubit` + `WalletService.lockCurrentWallet()` (RealUnitCH#485) already use `hidden` for the same purpose. The listener is `dispose()`'d in `close()` so the cubit's lifecycle stays self-contained — no leaks if the user navigates away before generation completes. Tests cover: * hidden → first emission is the cleared state (mnemonic dropped), asserted via an emissions log to survive the synchronous regenerate that follows. * hidden with no wallet generated yet → no emission (`same` ref). * hidden → resumed re-generates a fresh wallet; emissions log pins cleared-then-new-wallet ordering and asserts `createSeedWallet` was called twice. * inactive / resumed → state preserved. Parameterised so a future refactor (e.g. switching to a `switch` with a default-clear) can't silently regress the contract for non-`hidden` lifecycle states. `paused` / `detached` are unreachable from `resumed` without going through `hidden` per Flutter's `AppLifecycleListener` state machine — those paths exercise the same clear-and-regenerate cycle anyway, so the dedicated `hidden` tests pin them. VerifySeedCubit (`lib/screens/verify_seed/`) also receives a [SoftwareWallet] from the create flow and holds it for the seed-quiz step. It is not addressed here because (1) the issue scopes Option A to [CreateWalletCubit] specifically, and (2) `VerifySeedCubit` consumes a wallet owned by its parent — dropping it independently would break the seed-quiz flow on transient hides. Worth a follow-up if the threat model demands it. Closes RealUnitCH#489.
TaprootFreak
pushed a commit
that referenced
this pull request
May 21, 2026
Closes #488 Closes #489 Three follow-up fixes for residual mnemonic-in-memory windows that PR #485 deliberately deferred plus a disk-side regression flagged in post-merge review. Two of the in-memory windows were already closed atomically (#488, #489); the third commit adds an Option B refactor of `WalletService` so the onboarding regenerate path no longer accumulates orphan rows in `walletInfos`. ## What changed Three atomic commits. ### 1. `fix(wallet-service): close _onHidden vs _unlockInFlight race` — closes #488 A single sign-flow ensure that's still in flight when `_onHidden` fires used to leave the wallet as `SoftwareViewWallet` at lock time (no-op), then the unlock resolved and wrote the unlocked `SoftwareWallet` back to `AppStore.wallet`. The mnemonic briefly resurfaced until the 60s safety net or the sign-flow `finally lock` caught it. The fix: in `lockCurrentWallet()`, when the last holder releases, invalidate `_unlockInFlight` (`ignore()` + `= null`). In `ensureCurrentWalletUnlocked()`, gate the post-resolve write to `AppStore.wallet` on the in-flight token still matching — so a pending unlock that the lock has invalidated does not resurface the mnemonic. This closes the `AppStore.wallet` resurfacing window: the heap window is bounded by the in-flight DB-read latency, not zero (the mnemonic still lives in the resolved future's locals until the GC reclaims it), but the only path that lets it land back into observable app state is now gated. The in-flight sign-flow itself fails by design: it captured `credentials = appStore.wallet.currentAccount.primaryAddress` before its ensure awaited, the lock-during-flight invalidates the unlock, and `appStore.wallet` stays a `SoftwareViewWallet`. The captured `credentials` is the *old* `_LockedCredentials` from the view-wallet — `signToSignature` throws `StateError('SoftwareViewWallet credentials reached a sign call…')` (and the `assert(false, …)` trips in dev), which is caught by the existing `finally lock` in the sign flow. Security over usability: the mid-sign attempt fails, the *next* sign re-decrypts cleanly. While here, gate the post-unlock 60s timer on the same in-flight identity check (`landedInStore`): when the lock invalidated our write, arming the timer would point at a `SoftwareViewWallet` and `_lockWalletInPlace` would safely no-op via `is! SoftwareWallet` — not a correctness bug, just dead work. The existing test `lock between two in-flight ensures preserves the unlocked wallet` still passes — the invalidation only runs when the counter goes to 0, which is precisely the lifecycle-hook scenario, not the genuine-concurrent-holders scenario. New Completer-gated test pins the sequence: ensure-mid-flight → lock → resolve → `AppStore.wallet` stays `SoftwareViewWallet`. Reuses the gated-repository pattern from the neighbouring in-flight tests. ### 2. `feat(create-wallet): drop & re-generate onboarding mnemonic on app-hidden` — closes #489 The mnemonic generated by `CreateWalletCubit.createWallet()` lives in `CreateWalletState.wallet`, not in `AppStore.wallet` — so `WalletService.lockCurrentWallet()` no-op'd on this path (its `!isWalletLoaded` guard). Option A from the issue: `CreateWalletCubit` owns its own `AppLifecycleListener` and resets state on `hidden`. The cubit is disposed-clean: the listener is `dispose()`'d in the cubit's `close()`. Re-fire `createWallet()` after the clear so the view recovers. `BlocProvider.create` runs once, so the constructor cascade `..createWallet()` fires exactly once — without re-firing inside `_dropMnemonic`, resume would leave `state.wallet == null` and `BlocBuilder` would render `CupertinoActivityIndicator` indefinitely (escapable only via the AppBar back button). After `emit(const CreateWalletState())`, the cubit immediately calls `createWallet()`; the screen briefly flashes the loading indicator, then re-renders with a NEW mnemonic. The prior in-memory seed is gone before the new one is generated. Why `hidden`: same reasoning as #485. Fires earlier than `paused` on iOS, Android raises it via the unified pipeline, and the rest of the app (`PinAuthCubit`, `WalletService.lockCurrentWallet()`) already uses `hidden` for the same purpose. Tests cover: hidden-clears (via emissions log so the synchronous regenerate doesn't hide the intermediate cleared state), hidden-no-op-when-no-wallet, hidden→resumed regenerates a fresh wallet, and `inactive` / `resumed` no-op (parameterised regression-pin). ### 3. `fix(create-wallet): defer mnemonic persistence to verify-seed confirm` — disk-side regression Surfaced in post-merge review of commit 2. The regenerate-on-hidden loop fixed #489 in memory but introduced a disk-side problem: each `createWallet()` call wrote a row to `walletInfos` (an AES-GCM-encrypted mnemonic), and `WalletStorage.deleteWallet` only deletes from `walletAccountInfos`. N+1 orphan encrypted-seed rows accumulated per onboarding session with N hide-cycles, none deletable. Option B refactor of `WalletService`: - `generateUncommittedSeedWallet(String name) → Future<SoftwareWallet>` — generates the mnemonic, builds a `SoftwareWallet` with `id == 0` sentinel. No DB write. - `commitGeneratedWallet(SoftwareWallet draft) → Future<SoftwareWallet>` — takes the draft, writes via `_repository.createWallet`, returns the id-bearing wallet. Asserts `draft.id == 0` to catch double-commit at the boundary. - `createSeedWallet(String name)` kept as a thin `generate → commit` convenience — `restoreWallet` still uses the underlying `_persistSoftwareWallet` directly (typed seed → immediate persistence is the legitimate happy path; there's no verify quiz on restore). Wiring: - `CreateWalletCubit.createWallet()` calls `generateUncommittedSeedWallet` instead of `createSeedWallet`. No DB row per regenerate. - `VerifySeedCubit.verify()` commits the draft via `commitGeneratedWallet` BEFORE `setCurrentWallet`, then uses the committed id. The user only reaches that branch by typing the four requested seed words correctly, so the seed that lands on disk is the seed the user kept. `warmAuthSignature` continues to fire on `createWallet()` — the signature is derived from the primary address, which is deterministic from the mnemonic, so the warm-up is valid for the eventual committed wallet. New tests pin the contract: - `WalletService`: `generateUncommittedSeedWallet` returns id=0 + does NOT write to the repository + fresh entropy per call; `commitGeneratedWallet` writes exactly one row, preserves the draft seed; `createSeedWallet` still works as the generate+commit convenience. - `CreateWalletCubit`: the cubit calls `generateUncommittedSeedWallet` and never `commitGeneratedWallet` (including across the `_dropMnemonic` regenerate path). - `VerifySeedCubit`: `verify()` commits BEFORE `setCurrentWallet`, uses the committed id (not the `0` sentinel), and skips both calls on a wrong word. ## Why `hidden` everywhere Mirroring #485 — the threat is iOS isolate suspension before the user notices anything is wrong. `hidden` is the earliest hook that gives us a chance to drop the secret. ## Scope - `VerifySeedCubit` (the second screen in the onboarding flow) also receives the `SoftwareWallet` from the create flow and holds it for the seed-quiz step. This PR does NOT add a lifecycle drop there because (1) the issue scopes Option A to `CreateWalletCubit` specifically, and (2) `VerifySeedCubit` consumes a wallet owned by its parent — dropping it independently would break the seed-quiz on transient hides (e.g. notification drag-down). With Option B, the draft is still in-memory only at that stage, so the disk-orphan window doesn't exist for verify either. The in-memory window between create and verify is worth a follow-up if the threat model demands it. - The 60 s safety net (`_postUnlockLockTimer`) remains in place — it's defence-in-depth for the "user signs once then leaves the app foregrounded" case (no `_onHidden` to trigger the lock). - `WalletStorage.deleteWallet`'s `walletAccountInfos`-only behaviour is a separate issue tracked elsewhere — Option B sidesteps it by simply not writing the orphan rows in the first place. ## Test plan - [x] `flutter analyze` — clean - [x] `flutter test` — 1437 / 1437 green (1430 baseline before commit 1 + 7 new across the three commits: 1 in-flight-unlock-race, 5 cubit-lifecycle, plus the Option B additions in `wallet_service_test` / `verify_seed_cubit_test` net out to +1 with the existing `createSeedWallet` test reshuffled into `generate` + `commit` groups) - [x] Local CI parity run: `flutter pub get` → `dart run tool/generate_localization.dart` → `dart run tool/generate_release_info.dart` → `flutter pub run build_runner build` → `flutter analyze` → `flutter test` — all green - [ ] **iOS manual (#488)**: start a sign flow that's slow enough to interleave (e.g. throttled network) → background the app while the unlock is in flight → return → the in-flight sign throws `StateError` (programmer-error path: locked credentials reached a sign call) and is caught by the existing `finally lock`; the *next* sign re-decrypts cleanly. No `AppStore.wallet` resurfacing to `SoftwareWallet` after the lock. Hard to engineer reliably on a real device — the test pins the contract. - [ ] **iOS manual (#489)**: fresh install → onboarding → reach the create-wallet screen with mnemonic visible → swipe up to multitasking → return → the screen briefly shows the loading indicator, then re-renders with a NEW mnemonic. No stale seed visible. Repeat on Android multitasker. - [ ] **iOS manual (Option B)**: same fresh-install onboarding flow → background and resume 3 times on the create-wallet screen → continue past verify-seed with the LAST regenerated mnemonic → settings → delete wallet → fresh install again → onboarding works cleanly (no leftover `walletInfos` rows). Hard to introspect SQLite directly on a real device — the unit-test pin is the contract. - [ ] **iOS manual regression**: onboarding flow → create-wallet → continue to verify-seed step → background → return → verify-seed step still has its wallet (parent flow not torn down by the cubit-local listener). ## Honest limitations - The #488 fix relies on the `_unlockInFlight` identity check — if a future refactor changes how the in-flight slot is managed, the check could silently drift. The new test pins the exact sequence, but conceptually it tests behaviour, not the implementation. A reviewer extending `ensureCurrentWalletUnlocked()` should re-check the invariant. - The #489 fix clears the cubit state on `hidden` and immediately re-fires `createWallet()` to recover the view. When the user returns, the view re-renders the `CupertinoActivityIndicator` while a fresh mnemonic is generated. UX-wise this is acceptable for the onboarding context (the user hasn't committed to anything yet) but it's a behaviour change worth flagging — the mnemonic the user saw before backgrounding is NOT the same one shown on resume. - Option B introduces an `id == 0` sentinel for uncommitted drafts. It's asserted at the commit boundary, but a future refactor that hands the draft to any code path expecting a persisted id (e.g. `getWalletById(0)`) will fail at runtime. Confined to the create→verify hop today.
This was referenced May 21, 2026
TaprootFreak
pushed a commit
that referenced
this pull request
May 23, 2026
…stability (#525) ## Summary Implements the high-priority items from the test engineering audit (#506). **4 commits:** - `test: add 45 tests from codebase audit (#506)` — regression guards and bug-proving tests - `fix: add isClosed guards to 8 cubits to prevent emit-after-close` — fixes the 8 bugs those tests exposed - `fix: cancel subscription in FilterCubit + isClosed guards in BuyConverterCubit` — 2 more cubits - `test: malformed JSON response tests for remaining 5 DFX services` — completes Phase 3 ## Rebase note This branch is 24 commits behind develop. Rebase has merge conflicts in `buy_payment_info_cubit_test.dart` and `sell_payment_info_cubit_test.dart` because develop refactored those cubits (removed `DFXPriceService` dependency, moved min-amount validation to API). The conflicts are in test files only — the resolution is: take develop's test structure and re-add the 3 new tests per file (BitboxNotConnected, emit-after-close, negative amount/comma). ## Bug fixes (10 cubits) Added `if (isClosed) return;` after every `await` in async methods that call `emit()`. Without these guards, navigating away from a screen while an HTTP request is in-flight throws `StateError: Cannot emit new states after calling close`. | Cubit | Trigger | |---|---| | `SellConfirmCubit` | User swipes modal during payment confirmation | | `SellBankAccountsCubit` | User taps back before bank accounts load | | `SellPaymentInfoCubit` | User navigates away during payment info fetch | | `SellConverterCubit` | Debounce timer fires after screen disposal | | `BuyPaymentInfoCubit` | User taps back during payment info fetch | | `BuyConverterCubit` | Same debounce pattern as sell | | `TransactionHistoryMultiReceiptCubit` | User leaves during PDF generation | | `TransactionHistoryReceiptCubit` | User scrolls away during receipt download | | `SettingsTaxReportCubit` | User navigates away during tax report generation (5-30s) | | `TransactionHistoryFilterCubit` | Stream subscription leaked on close (now stored + cancelled) | ## Source change (non-behavioral) - `AppDatabase.forTesting(QueryExecutor)` — `@visibleForTesting` constructor for in-memory DB tests. Zero production impact. ## Tests added (53 total) | Category | Count | Purpose | |---|---|---| | Emit-after-close | 10 | Proves/guards isClosed bugs | | Crypto regression | 1 | Non-ASCII signing (#289) | | Equality regression | 3 | BuyPaymentInfo field equality (#207) | | BitboxNotConnectedException | 1 | Missing exception path in BuyPaymentInfoCubit | | DashboardBloc error survival | 3 | Unhandled exceptions in 3 event handlers | | Financial boundaries | 4 | Negative amounts, comma normalization, Infinity/NaN | | JSON parsing (malformed responses) | 13 | All 11 DFX services + balance + registration | | SecureStorage corruption | 2 | Missing colon separator, empty ciphertext | | DB schema/migration | 4 | Table creation, wallet insert, FK integrity | | Parse fixed edge cases | 5 | Empty, multi-dot, dot, negative, zero | | Settings/URI pins | 2 | hideAmounts session-only, PaymentURI encoding | | Wallet persistence failure | 2 | Repository exception during create/restore | | Filter subscription cleanup | 1 | Stream subscription cancelled on close | | BuyConverterCubit close | 1 | Debounce timer + isClosed | ## Remaining from #506 (not in this PR) ### Phase 4.1-4.3: Storage atomicity (separate PR needed) These are real bugs but change database behavior and need careful review: - **`deleteWallet` doesn't cascade** — only deletes `walletAccountInfos`, not `walletInfos` (encrypted seed persists). Already tracked as #498. - **`insertDfxTransaction` non-atomic** — two separate inserts without a Drift `transaction()` wrapper. If the DFX details insert fails, the Transaction row is committed as an orphan. - **`saveBalance`/`saveAsset` TOCTOU races** — check-then-act pattern without atomicity. Needs `INSERT OR REPLACE` or Drift upsert. ### Phase 2: Security design decisions (need discussion) - **EIP-712 `signRegistration()` domain missing `chainId`** — unlike `signDelegation()` which includes it. Registration signatures are theoretically replayable across chains. Needs backend coordination. - **WebView accepts any URI scheme** — `javascript:`, `data:`, `file://` pass through without validation. Currently unreachable (both call sites hardcode empty amount), but should be guarded. ### Phase 6: Integration tests (separate scope) 5 cross-layer flows that require full DI container setup: 1. PIN verify -> wallet load -> dashboard (regression for `f9b89ea`) 2. Create wallet -> background -> resume -> seed cleared (regression for #485, #489) 3. Buy flow: switch currency -> payment details update (regression for #207) 4. KYC flow: existing DFX customer merge (regression for #466) 5. BitBox disconnect mid-sign -> reconnect -> retry (regression for #461) ## Test plan - [x] All 53 new tests pass (emit-after-close tests pass after isClosed fix) - [x] All existing tests pass (10 pre-existing loading failures unchanged) - [x] `flutter analyze` clean on changed source files - [x] No behavioral change to production code (only isClosed guards + subscription cleanup) Closes #506
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes Blume1977#1 (closed) — same intent, rebased onto post-#483 develop, addresses TaprootFreak's two review points by pushing the precondition into
WalletServiceinstead of catching it at the call site.Why
PR #472 / #483 added an explicit
WalletService.lockCurrentWallet()after every sign and a 60 s post-unlock safety-net timer. Reviewer (#468) flagged the residual gap in the second-pass review:This closes that gap.
What changed
Three atomic commits:
feat(app-store): isWalletLoaded getter— non-throwing predicate so services can early-return defensively. Named distinctly fromWalletService.hasWallet()(which is persisted state, not in-memory load state) so the two predicates can't be confused at the call site.fix(wallet-service): no-op lockCurrentWallet when no wallet is loaded— Onboarding-path guard insideWalletServiceitself. An alternative would have been atry/catchat the call site, which would silently mask any future regression inlockCurrentWallet. Pushing the precondition into the service means the call site stays clean and future extensions (DB write, audit log) won't get their errors quietly caught. Two regression tests added: the!isWalletLoadedno-op, and a lifecycle-hidden-during-active-sign scenario that pins the counter-underflow guard.feat(lifecycle): lock the wallet on hidden—LifecycleInitializer._onHidden()firesunawaited(getIt<WalletService>().lockCurrentWallet()). No try/catch, no helper method — the service is defensive on its own. Tests cover happy path + the two no-call states (paused/resumed).Why
hidden(notpaused)pausedon iOS (covers multitasking switcher + notification drag-down).PinAuthCubitalready uses_onHiddenfor its lockout-time stamp — semantically aligned.Scope gap (deliberately deferred)
This PR drops the mnemonic stored in
AppStore.wallet. It does not clear the mnemonic temporarily held inCreateWalletState.wallet(aCubitstate inside the create-wallet flow) — that copy lives independent ofAppStoreandlockCurrentWalletis a no-op for it. Backgrounding during onboarding still leaves the just-generated mnemonic in cubit memory until the cubit closes. Pre-existing condition, not introduced here. Worth a follow-up if the threat model demands it.Why no test for "lifecycle doesn't
catchErrorthe unawaited future"The architecture decision — the lifecycle caller deliberately does not attach
.catchErroror wrap in try/catch — is locked in by the inline comment in_onHidden, not by a test. Every variant I tried (thenThrow,thenAnswer((_) async => throw …),Future.error(...)) routed the failure through the testWidgets framework's synchronous catch path rather than the Zone uncaught-error sink thattester.takeException()reads from. The routing differs between Flutter 3.41 (CI) and 3.44 (local), so a test that's green locally would flake on CI. Brittle false-positives are worse than the source comment for catching a future regression at the call site — every code reviewer reads_onHidden.Reviewer feedback (TaprootFreak)
Iteration after the first review:
hasWallettoisWalletLoadedonAppStoreto avoid colliding withWalletService.hasWallet()which has different semantics.ensure → unpaired lifecycle lock → finally lock, verifying the underflow guard prevents counter drift and the next ensure cycle still works.ensureCurrentWalletUnlocked()in_onResumed.isWalletLoaded = truemoved out of the globalsetUpinto per-groupsetUpso unrelated groups don't carry the stub and the negative path isn't masked.LoadCurrentWalletEventandLoadWalletEventpopulateAppStore.wallet.Why the architecture diverges from the original draft (Blume1977#1): previous incarnation wrapped
lockCurrentWallet()in atry { } on Exception catch (e)helper in the lifecycle file. Reviewer pushed back that this would silently swallow future regressions (e.g. DB-write failures from a logging extension). Single Responsibility: the "is wallet loaded" precondition belongs toWalletService, not its callers.UX impact
None visible. The next sign re-decrypts the mnemonic via the OS-keystore-wrapped key in sub-100 ms.
Test plan