From c6e9b031cfd1c1d5620365acf9eb75da2e458d42 Mon Sep 17 00:00:00 2001 From: Blume1977 Date: Thu, 21 May 2026 12:32:26 +0200 Subject: [PATCH 1/4] fix(wallet-service): close _onHidden vs _unlockInFlight race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 #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())`), 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 #488. --- lib/packages/service/wallet_service.dart | 35 ++++++++++-- .../packages/service/wallet_service_test.dart | 56 +++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/lib/packages/service/wallet_service.dart b/lib/packages/service/wallet_service.dart index 25679b0e..0c4324e8 100644 --- a/lib/packages/service/wallet_service.dart +++ b/lib/packages/service/wallet_service.dart @@ -146,23 +146,44 @@ class WalletService { /// don't tear the unlocked state out from under each other. Future ensureCurrentWalletUnlocked() async { _activeUnlockHolders++; + // Tracks whether THIS call's unlock landed in [AppStore.wallet]. When + // the lock-during-flight invalidates the slot, the write is skipped and + // arming the post-unlock timer would just point at a [SoftwareViewWallet] + // — `_lockWalletInPlace` would safely no-op via `is! SoftwareWallet`, so + // it's dead work, not a correctness bug. Skip the arm in that case. + var landedInStore = false; if (_appStore.wallet is SoftwareViewWallet) { // Coalesce overlapping unlocks onto a single in-flight future so we // don't hammer the DB and re-run AES-GCM for every concurrent caller. final inFlight = _unlockInFlight ??= unlockCurrentWallet(); try { - _appStore.wallet = await inFlight; + final unlocked = await inFlight; + // If [lockCurrentWallet] fired while this unlock was in flight, it + // invalidates the slot (`_unlockInFlight = null`). Skip the write + // so the mnemonic doesn't resurface in [AppStore.wallet] after the + // user has already covered the app — closes the `_onHidden` race. + if (identical(_unlockInFlight, inFlight)) { + _appStore.wallet = unlocked; + landedInStore = true; + } } finally { // Only the caller that started the unlock clears the slot; later // joiners observe the field already nulled and skip the clear. if (identical(_unlockInFlight, inFlight)) _unlockInFlight = null; } + } else { + // Wallet was already unlocked (a previous holder's write is still in + // place). Re-arming the safety net is the right call — extends the + // 60 s window for the joining holder. + landedInStore = true; } // Safety net: if a caller forgets to call lockCurrentWallet() after the // sign, drop the mnemonic anyway once the post-unlock window elapses. The - // reviewer flagged "user sells once then leaves the app foregrounded" — - // that path now caps at [_postUnlockLockTimeout]. - _schedulePostUnlockLock(); + // reviewer flagged "user signs once then leaves the app foregrounded" — + // that path now caps at [_postUnlockLockTimeout]. Skip the arm when the + // intervening lock invalidated our write: the timer would just fire + // against a view-wallet (no-op) and we'd be doing dead work. + if (landedInStore) _schedulePostUnlockLock(); } /// Replaces the in-memory [SoftwareWallet] with its lock-screen-safe @@ -184,6 +205,12 @@ class WalletService { if (_activeUnlockHolders > 0) _activeUnlockHolders--; if (_activeUnlockHolders > 0) return; + // Invalidate any in-flight unlock so its resolution doesn't write the + // unlocked [SoftwareWallet] back into [AppStore.wallet] after this lock — + // the race the 60s safety net used to catch as defence-in-depth, now + // closed at the source. + _unlockInFlight?.ignore(); + _unlockInFlight = null; _postUnlockLockTimer?.cancel(); _postUnlockLockTimer = null; _lockWalletInPlace(); diff --git a/test/packages/service/wallet_service_test.dart b/test/packages/service/wallet_service_test.dart index dd4afd18..9a9bff8b 100644 --- a/test/packages/service/wallet_service_test.dart +++ b/test/packages/service/wallet_service_test.dart @@ -503,6 +503,62 @@ void main() { reason: 'final holder release flips back to view wallet'); }); + // The `_onHidden` race: a single sign-flow ensure is still mid-unlock + // when `lockCurrentWallet` fires from the app-lifecycle hidden hook. + // Without invalidating the in-flight unlock, its resolution would + // write the unlocked [SoftwareWallet] back to [AppStore.wallet] + // AFTER the lock — resurfacing the mnemonic in memory until either + // the 60s safety net or the sign-flow `finally lock` clears it + // again. The 60s window is best-effort under iOS isolate suspension + // (the gap #485 set out to close in the first place), so the fix + // closes it at the source: the lock invalidates `_unlockInFlight` + // and the ensure skips its write. + test('lock during a single in-flight unlock does not resurface the mnemonic', + () async { + final stored = [SoftwareViewWallet(7, 'Main', _debugAddress)]; + when(() => appStore.wallet).thenAnswer((_) => stored.last); + when(() => appStore.wallet = any(that: isA())).thenAnswer((inv) { + final newWallet = inv.positionalArguments.single as AWallet; + stored.add(newWallet); + return newWallet; + }); + when(() => settings.currentWalletId).thenReturn(7); + + // Pin the unlock mid-flight so we can fire `lockCurrentWallet` + // exactly between the ensure starting and its DB read resolving. + final gate = Completer(); + when(() => repo.getUnlockedWalletById(7)).thenAnswer((_) => gate.future); + + // Sign-flow ensure starts, counter=1, blocks on gated read. + final ensure = service.ensureCurrentWalletUnlocked(); + + // App-lifecycle hidden fires — counter goes to 0, lock would + // normally no-op (wallet still SoftwareViewWallet) and let the + // pending unlock leak through. + await service.lockCurrentWallet(); + expect(stored.last, isA(), + reason: 'lock observed the still-view wallet — nothing to flip'); + + // Release the gated DB read so the in-flight ensure resolves. + gate.complete( + _info(id: 7, name: 'Main', seed: _testMnemonic, type: WalletType.software), + ); + await ensure; + + // The fix: the post-resolve write is gated on the in-flight token + // still matching, which the lock invalidated. So the mnemonic + // never lands in [AppStore.wallet] after the user covered the app. + expect(stored.last, isA(), + reason: 'in-flight unlock invalidated by intervening lock must not ' + 'resurface the mnemonic'); + // Pin the mechanism, not just the outcome: the `_unlockInFlight` + // gate must suppress the post-resolve write — never let a future + // refactor pass this test by tolerating the write and clearing it + // again from somewhere else (which would still expose the mnemonic + // to any code path observing `AppStore.wallet` between the writes). + verifyNever(() => appStore.wallet = any(that: isA())); + }); + // Two overlapping ensures must coalesce onto a single DB read + // AES-GCM decrypt, not trigger the repository twice. Functionally // both versions would land on the same SoftwareWallet, but the From 051360f8236b1b77f51ee605f86f26e66ec6f1d8 Mon Sep 17 00:00:00 2001 From: Blume1977 Date: Thu, 21 May 2026 12:32:52 +0200 Subject: [PATCH 2/4] feat(create-wallet): drop & re-generate onboarding mnemonic on app-hidden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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()` (#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 #489. --- .../bloc/create_wallet_cubit.dart | 48 ++++- .../create_wallet_cubit_test.dart | 165 ++++++++++++++++++ 2 files changed, 212 insertions(+), 1 deletion(-) diff --git a/lib/screens/create_wallet/bloc/create_wallet_cubit.dart b/lib/screens/create_wallet/bloc/create_wallet_cubit.dart index 75ef9832..5e670250 100644 --- a/lib/screens/create_wallet/bloc/create_wallet_cubit.dart +++ b/lib/screens/create_wallet/bloc/create_wallet_cubit.dart @@ -1,5 +1,6 @@ import 'dart:async'; +import 'package:flutter/widgets.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; import 'package:realunit_wallet/packages/service/dfx/dfx_auth_service.dart'; import 'package:realunit_wallet/packages/service/wallet_service.dart'; @@ -8,10 +9,20 @@ import 'package:realunit_wallet/packages/wallet/wallet.dart'; part 'create_wallet_state.dart'; class CreateWalletCubit extends Cubit { - CreateWalletCubit(this._service, this._authService) : super(const CreateWalletState()); + CreateWalletCubit(this._service, this._authService) : super(const CreateWalletState()) { + // Onboarding-equivalent of `WalletService.lockCurrentWallet()` for the + // freshly generated mnemonic. While the user is on the create-wallet + // screen, the mnemonic lives in `CreateWalletState.wallet` — not in + // `AppStore.wallet` — so the service-level lock is a no-op for this + // path. Clearing the cubit state on `hidden` drops the seed before iOS + // suspends the isolate; the user returning is sent back to the start + // of the create flow, which is the safe restart point. + _lifecycleListener = AppLifecycleListener(onStateChange: _onLifecycleState); + } final WalletService _service; final DFXAuthService _authService; + late final AppLifecycleListener _lifecycleListener; void createWallet() async { final wallet = await _service.createSeedWallet('Obi-Wallet-Kenobi'); @@ -25,10 +36,45 @@ class CreateWalletCubit extends Cubit { loggerName: '$CreateWalletCubit', ), ); + // Async-tail guard: with the `_dropMnemonic` re-fire on `hidden`, the + // user can return to foreground and immediately pop the screen before + // the regenerated `createSeedWallet` resolves — the AppBar back closes + // the cubit, and a post-close `emit` would throw `StateError`. Matches + // the `connect_bitbox_cubit` / `kyc_cubit` pattern. + if (isClosed) return; emit(state.copyWith(wallet: wallet)); } void toggleShowSeed() { emit(state.copyWith(hideSeed: !state.hideSeed)); } + + void _onLifecycleState(AppLifecycleState state) { + if (state == AppLifecycleState.hidden) { + _dropMnemonic(); + } + } + + void _dropMnemonic() { + // Reset to the initial state — drops `wallet` (and its mnemonic) and + // restores the default `hideSeed: true`. `copyWith` would carry the + // existing wallet through, so we emit a fresh state explicitly. + if (state.wallet == null) return; + emit(const CreateWalletState()); + // The cubit is built once via `BlocProvider.create` (`..createWallet()` + // fires exactly once at construction), so without re-firing here the + // user would resume to a `state.wallet == null` and the view's + // `BlocBuilder` would render `CupertinoActivityIndicator` indefinitely + // — escapable only via the AppBar back button. Re-issue a fresh + // generation so the next emission replaces the cleared state; the + // screen briefly flashes the loading indicator, then re-renders with + // the new mnemonic. The prior in-memory seed is already gone. + createWallet(); + } + + @override + Future close() { + _lifecycleListener.dispose(); + return super.close(); + } } diff --git a/test/screens/create_wallet/create_wallet_cubit_test.dart b/test/screens/create_wallet/create_wallet_cubit_test.dart index b69336ad..ecac97ec 100644 --- a/test/screens/create_wallet/create_wallet_cubit_test.dart +++ b/test/screens/create_wallet/create_wallet_cubit_test.dart @@ -1,4 +1,5 @@ import 'package:bloc_test/bloc_test.dart'; +import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:mocktail/mocktail.dart'; import 'package:realunit_wallet/packages/service/dfx/dfx_auth_service.dart'; @@ -76,5 +77,169 @@ void main() { expect(cubit.state.wallet, same(wallet)); expect(cubit.state.hideSeed, isFalse); }); + + // Onboarding-equivalent of #485's app-hidden wallet lock: the freshly + // generated mnemonic lives in the cubit state (not in `AppStore.wallet`), + // so `WalletService.lockCurrentWallet` no-op's on this path. Closes #489. + // `AppLifecycleListener` dispatches through `WidgetsBinding`, so we use + // `testWidgets` to drive the binding's lifecycle state machine. + group('app lifecycle', () { + testWidgets('hidden drops the just-generated mnemonic from cubit state', + (tester) async { + final wallet = SoftwareWallet(7, 'Obi-Wallet-Kenobi', _testMnemonic); + when(() => service.createSeedWallet(any())).thenAnswer((_) async => wallet); + + final cubit = CreateWalletCubit(service, authService); + addTearDown(cubit.close); + // Record every emission so we can pin the intermediate cleared + // state — `_dropMnemonic` re-fires `createWallet()` synchronously + // after the clear, and the regenerated wallet would otherwise have + // overwritten the cleared snapshot by the time we sample the + // current state. + final emissions = []; + final sub = cubit.stream.listen(emissions.add); + addTearDown(sub.cancel); + + cubit.createWallet(); + await cubit.stream.firstWhere((s) => s.wallet != null); + expect(cubit.state.wallet, same(wallet), + reason: 'precondition — wallet is in cubit state before hidden fires'); + emissions.clear(); + + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.hidden); + await tester.pump(); + + // The first emission after hidden must be the fully cleared state. + // The reset-to-initial contract is what drops the mnemonic from + // memory — the regeneration that follows is the UX recovery for + // the stuck-on-spinner blocker (covered in the next test). + expect(emissions, isNotEmpty, + reason: 'hidden must emit at least the cleared state'); + expect(emissions.first.wallet, isNull, + reason: 'hidden must drop the mnemonic from cubit state'); + expect(emissions.first.hideSeed, isTrue, + reason: 'reset to initial — hideSeed defaults back to true'); + }); + + testWidgets('hidden is a no-op when no wallet has been generated yet', + (tester) async { + final cubit = CreateWalletCubit(service, authService); + addTearDown(cubit.close); + // No createWallet() call — state is the const initial. + final initial = cubit.state; + + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.hidden); + await tester.pump(); + + // No emission — the cubit state object is unchanged (no new + // CreateWalletState was emitted), so the listener stream is empty. + expect(cubit.state, same(initial), + reason: 'no wallet → no emission → reference equality holds'); + }); + + // Only `hidden` clears — pin every other lifecycle state that the + // user can realistically hit without going through `hidden` first as + // a no-op, so a future refactor (e.g. switching to a `switch` with a + // default-clear) can't silently regress the contract. Flutter's + // `AppLifecycleListener` enforces a strict transition graph + // (resumed↔inactive↔hidden↔paused↔detached): from the default + // `resumed` start state we can reach `inactive` directly, and back + // to `resumed` via `inactive`. Reaching `paused` / `detached` + // requires walking through `hidden`, which itself is the trigger we + // want to keep — those paths are covered by the dedicated `hidden` + // tests above. + const reachableWithoutHidden = [ + AppLifecycleState.inactive, + AppLifecycleState.resumed, + ]; + for (final lifecycle in reachableWithoutHidden) { + testWidgets('${lifecycle.name} does NOT clear the cubit state — only hidden does', + (tester) async { + final wallet = SoftwareWallet(7, 'Obi-Wallet-Kenobi', _testMnemonic); + when(() => service.createSeedWallet(any())).thenAnswer((_) async => wallet); + final cubit = CreateWalletCubit(service, authService); + addTearDown(cubit.close); + cubit.createWallet(); + await cubit.stream.firstWhere((s) => s.wallet != null); + + // resumed is the listener's default starting state — feed an + // intermediate `inactive` first so the resumed-back-to-resumed + // transition is valid per the AppLifecycleListener state machine. + if (lifecycle == AppLifecycleState.resumed) { + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.inactive); + await tester.pump(); + } + tester.binding.handleAppLifecycleStateChanged(lifecycle); + await tester.pump(); + + expect(cubit.state.wallet, same(wallet), + reason: '${lifecycle.name} must not drop the mnemonic — only hidden does'); + }); + } + + // The cubit is built once via `BlocProvider.create` and its + // constructor cascades a single `..createWallet()` call — that call is + // NOT re-invoked when the view rebuilds on resume. Without re-firing + // generation 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). This pins the resume-re-generation contract. + testWidgets( + 'hidden → resumed re-generates a fresh wallet so the view is not ' + 'stuck on the loading indicator', (tester) async { + var generated = 0; + when(() => service.createSeedWallet(any())).thenAnswer((_) async { + generated++; + return SoftwareWallet(generated, 'Obi-Wallet-Kenobi', _testMnemonic); + }); + // Record every emission so we can pin both the intermediate cleared + // state AND the regenerated state — without the recording, `pump` + // would drain both the clear and the regenerate microtasks before + // we sample, hiding the intermediate clear. + final emissions = []; + final cubit = CreateWalletCubit(service, authService); + addTearDown(cubit.close); + final sub = cubit.stream.listen(emissions.add); + addTearDown(sub.cancel); + + cubit.createWallet(); + await cubit.stream.firstWhere((s) => s.wallet != null); + final initial = cubit.state.wallet; + expect(generated, 1, reason: 'precondition — initial generation fired once'); + emissions.clear(); + + // Walk a realistic backgrounding sequence — `resumed` → `inactive` + // → `hidden` is the order iOS / Android actually emit. The strict + // `AppLifecycleListener` state machine also requires `inactive` + // before `hidden` from a `resumed` start. The `inactive` step is a + // no-op for `_dropMnemonic`; `hidden` is the trigger that clears. + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.inactive); + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.hidden); + await tester.pump(); + // Simulate the user returning from multitasking. Lifecycle ordering + // is irrelevant to `_dropMnemonic` (it kicks off `createWallet()` + // synchronously after the clear), but feeding `inactive` → `resumed` + // here pins the user-observable path end-to-end and stays within + // the lifecycle state machine. + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.inactive); + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.resumed); + await tester.pump(); + + // Two emissions: the cleared state (drops the mnemonic) followed by + // the regenerated state (recovers from the spinner). + expect(emissions, hasLength(2), + reason: 'hidden must emit cleared-then-regenerated, in that order'); + expect(emissions.first.wallet, isNull, + reason: 'first emission must be the cleared state'); + expect(emissions.last.wallet, isNotNull, + reason: 'fresh wallet must replace the cleared state — the view ' + 'must not stick on CupertinoActivityIndicator'); + expect(emissions.last.wallet, isNot(same(initial)), + reason: 'a NEW SoftwareWallet must be generated, not the cleared one'); + expect(generated, 2, + reason: '_dropMnemonic must re-fire createSeedWallet so the view ' + 'recovers from the cleared state'); + }); + }); }); } From e710804b3fb6b507d8f6f6ac8df07de2b1bbac23 Mon Sep 17 00:00:00 2001 From: Blume1977 Date: Thu, 21 May 2026 16:09:30 +0200 Subject: [PATCH 3/4] fix(create-wallet): defer mnemonic persistence to verify-seed confirm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splits createSeedWallet into generateUncommittedSeedWallet (in-memory) + commitGeneratedWallet (DB write). CreateWalletCubit now generates fresh mnemonics per hide-cycle without writing to walletInfos; only the verified mnemonic ever lands on disk. Closes the DB-row accumulation regression introduced by the earlier _dropMnemonic re-fire — N hide-cycles now produce 0 DB rows instead of N+1. --- lib/packages/service/wallet_service.dart | 39 ++++++++++++++++++- .../bloc/create_wallet_cubit.dart | 24 ++++++++---- .../verify_seed/cubit/verify_seed_cubit.dart | 18 ++++++++- 3 files changed, 70 insertions(+), 11 deletions(-) diff --git a/lib/packages/service/wallet_service.dart b/lib/packages/service/wallet_service.dart index 0c4324e8..02322e93 100644 --- a/lib/packages/service/wallet_service.dart +++ b/lib/packages/service/wallet_service.dart @@ -48,9 +48,40 @@ class WalletService { this._appStore, ); - Future createSeedWallet(String name) async { + /// Generates a fresh bip39 mnemonic and returns a [SoftwareWallet] that + /// is **not yet persisted** — `id` is the `0` sentinel and no row has + /// been written to `walletInfos`. Pair with [commitGeneratedWallet] once + /// the user has confirmed the seed (e.g. via the verify-seed quiz) so the + /// encrypted mnemonic only lands on disk for seeds the user has actually + /// kept. Prevents N+1 encrypted-seed rows from accumulating when the + /// onboarding cubit regenerates the mnemonic on every `hidden` cycle. + Future generateUncommittedSeedWallet(String name) async { final mnemonic = bip39.generateMnemonic(); - return _persistSoftwareWallet(name, mnemonic); + return SoftwareWallet(0, name, mnemonic); + } + + /// Persists a [draft] [SoftwareWallet] returned from + /// [generateUncommittedSeedWallet] into `walletInfos` (encrypted seed + + /// cached address) and returns a new [SoftwareWallet] carrying the + /// DB-assigned id. The draft is expected to carry the `0` sentinel id; a + /// different id indicates a misuse (commit called on an already-persisted + /// wallet) — surfaced via [assert] in dev and tolerated in release by + /// re-using the draft's seed. + Future commitGeneratedWallet(SoftwareWallet draft) async { + assert(draft.id == 0, + 'commitGeneratedWallet expects an uncommitted draft (id == 0); ' + 'got id=${draft.id} — likely double-commit or wrong caller.'); + return _persistSoftwareWallet(draft.name, draft.seed); + } + + /// Generate-and-commit convenience for callers that persist immediately + /// (e.g. [restoreWallet]). Onboarding callers should NOT use this — they + /// must call [generateUncommittedSeedWallet] and defer [commitGeneratedWallet] + /// until the user has confirmed the seed, otherwise every regenerate on + /// `hidden` writes an undeletable encrypted-seed row to `walletInfos`. + Future createSeedWallet(String name) async { + final draft = await generateUncommittedSeedWallet(name); + return commitGeneratedWallet(draft); } Future createBitboxWallet(String name) async { @@ -60,6 +91,10 @@ class WalletService { return BitboxWallet(walletId, name, address, _bitboxService); } + /// Persists a user-supplied seed phrase immediately — the user typed an + /// existing mnemonic, so there is no verify-seed quiz to gate the write + /// behind. Deferring would not help: the seed is already known and the + /// user expects to land on the dashboard on `restore` success. Future restoreWallet(String name, String seed) async { final wallet = await _persistSoftwareWallet(name, seed); await _settingsRepository.saveCurrentWalletId(wallet.id); diff --git a/lib/screens/create_wallet/bloc/create_wallet_cubit.dart b/lib/screens/create_wallet/bloc/create_wallet_cubit.dart index 5e670250..7418957c 100644 --- a/lib/screens/create_wallet/bloc/create_wallet_cubit.dart +++ b/lib/screens/create_wallet/bloc/create_wallet_cubit.dart @@ -25,10 +25,19 @@ class CreateWalletCubit extends Cubit { late final AppLifecycleListener _lifecycleListener; void createWallet() async { - final wallet = await _service.createSeedWallet('Obi-Wallet-Kenobi'); - // Fire-and-forget the auth-signature capture — the lazy path in - // DFXAuthService.getSignature is the safety net, and a 20 s HTTP timeout - // shouldn't gate the "creating wallet" UI. + // Defer the DB write to `VerifySeedCubit.verify()` via + // `WalletService.commitGeneratedWallet`. Writing on every regenerate + // would persist a fresh encrypted-seed row on each `_dropMnemonic` + // cycle (N+1 rows per onboarding session with N hide-cycles), and + // `WalletStorage.deleteWallet` only touches `walletAccountInfos` — + // those `walletInfos` rows would accumulate undeletable. The draft + // carries the `0` sentinel id until committed. + final wallet = await _service.generateUncommittedSeedWallet('Obi-Wallet-Kenobi'); + // Fire-and-forget the auth-signature capture. The signature is derived + // from the primary address, which is deterministic from the mnemonic + // — valid for the same wallet once it's committed. The lazy path in + // DFXAuthService.getSignature is the safety net, and a 20 s HTTP + // timeout shouldn't gate the "creating wallet" UI. unawaited( warmAuthSignature( _authService, @@ -38,9 +47,10 @@ class CreateWalletCubit extends Cubit { ); // Async-tail guard: with the `_dropMnemonic` re-fire on `hidden`, the // user can return to foreground and immediately pop the screen before - // the regenerated `createSeedWallet` resolves — the AppBar back closes - // the cubit, and a post-close `emit` would throw `StateError`. Matches - // the `connect_bitbox_cubit` / `kyc_cubit` pattern. + // the regenerated `generateUncommittedSeedWallet` resolves — the + // AppBar back closes the cubit, and a post-close `emit` would throw + // `StateError`. Matches the `connect_bitbox_cubit` / `kyc_cubit` + // pattern. if (isClosed) return; emit(state.copyWith(wallet: wallet)); } diff --git a/lib/screens/verify_seed/cubit/verify_seed_cubit.dart b/lib/screens/verify_seed/cubit/verify_seed_cubit.dart index 492ff179..63b799a4 100644 --- a/lib/screens/verify_seed/cubit/verify_seed_cubit.dart +++ b/lib/screens/verify_seed/cubit/verify_seed_cubit.dart @@ -17,7 +17,11 @@ class VerifySeedCubit extends Cubit { _initVerification(); } - final SoftwareWallet _wallet; + /// The draft wallet handed in by `CreateWalletCubit`. Until [verify] + /// succeeds and `WalletService.commitGeneratedWallet` lands the row, the + /// id is the `0` sentinel — it must NOT be passed to + /// `setCurrentWallet` directly; commit first, use the returned id. + SoftwareWallet _wallet; final WalletService _walletService; void _initVerification() { @@ -63,7 +67,17 @@ class VerifySeedCubit extends Cubit { } } - await _walletService.setCurrentWallet(_wallet.id); + // Commit the draft mnemonic to disk BEFORE marking it current — the + // wallet handed in by `CreateWalletCubit` is the in-memory-only draft + // produced by `WalletService.generateUncommittedSeedWallet` (id == 0). + // Persisting at confirm time means a regenerate triggered by an + // app-hidden cycle in the create flow never leaves an orphan row in + // `walletInfos`. The user only reaches this branch by typing the four + // requested words correctly, so the seed they kept is the seed we + // store. + final committed = await _walletService.commitGeneratedWallet(_wallet); + _wallet = committed; + await _walletService.setCurrentWallet(committed.id); emit(state.copyWith(isVerified: true)); return true; } From 68d3807def800903cfbc0d079aafbbe8a43f7685 Mon Sep 17 00:00:00 2001 From: Blume1977 Date: Thu, 21 May 2026 16:11:41 +0200 Subject: [PATCH 4/4] =?UTF-8?q?test(wallet-service):=20pin=20Option=20B=20?= =?UTF-8?q?disk-side=20contract=20=E2=80=94=20drafts=20off=20disk?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers: - generateUncommittedSeedWallet returns an id=0 draft + does NOT write to walletInfos, fresh entropy per call - commitGeneratedWallet writes exactly one row per call, preserves seed - createSeedWallet remains the generate+commit convenience - CreateWalletCubit verifies generateUncommittedSeedWallet is called and commitGeneratedWallet is NEVER called from the cubit (including through the _dropMnemonic regenerate path) - VerifySeedCubit.verify commits BEFORE setCurrentWallet, uses the committed id, and skips both on a wrong word --- .../packages/service/wallet_service_test.dart | 80 ++++++++++++++++++- .../create_wallet_cubit_test.dart | 36 ++++++--- .../create_wallet_page_test.dart | 3 +- .../cubit/verify_seed_cubit_test.dart | 50 +++++++++++- 4 files changed, 154 insertions(+), 15 deletions(-) diff --git a/test/packages/service/wallet_service_test.dart b/test/packages/service/wallet_service_test.dart index 9a9bff8b..d918748f 100644 --- a/test/packages/service/wallet_service_test.dart +++ b/test/packages/service/wallet_service_test.dart @@ -58,8 +58,86 @@ void main() { }); group('$WalletService', () { + group('generateUncommittedSeedWallet', () { + test('returns an in-memory SoftwareWallet with the id=0 sentinel and a valid bip39 mnemonic', + () async { + final draft = await service.generateUncommittedSeedWallet('Main'); + + expect(draft, isA()); + expect(draft.id, 0, + reason: 'uncommitted drafts use the 0 sentinel until commitGeneratedWallet lands the row'); + expect(draft.name, 'Main'); + expect(service.validateSeed(draft.seed), isTrue); + }); + + test('does NOT write to the repository — the encrypted seed must not land on disk', () async { + await service.generateUncommittedSeedWallet('Main'); + + // Pin the disk-side guarantee: nothing flows into `walletInfos` until + // a separate `commitGeneratedWallet` call. Without this, every + // `_dropMnemonic` regenerate in `CreateWalletCubit` would persist a + // fresh encrypted-seed row, and `WalletStorage.deleteWallet` only + // touches `walletAccountInfos`, so those rows would accumulate + // undeletable. + verifyNever(() => repo.createWallet(any(), any(), any(), any())); + verifyNever(() => settings.saveCurrentWalletId(any())); + }); + + test('two consecutive calls produce distinct mnemonics (entropy not pinned by the API)', + () async { + final a = await service.generateUncommittedSeedWallet('Main'); + final b = await service.generateUncommittedSeedWallet('Main'); + + expect(a.seed, isNot(equals(b.seed)), + reason: 'each call must produce a fresh mnemonic — pinning entropy would ' + 'silently break the "regenerate on hidden" contract'); + }); + }); + + group('commitGeneratedWallet', () { + test('persists the draft seed and returns a SoftwareWallet carrying the DB-assigned id', + () async { + when(() => repo.createWallet(any(), any(), any(), any())).thenAnswer((_) async => 42); + + final draft = await service.generateUncommittedSeedWallet('Main'); + final committed = await service.commitGeneratedWallet(draft); + + expect(committed.id, 42); + expect(committed.name, 'Main'); + expect(committed.seed, draft.seed, + reason: 'commit must preserve the draft mnemonic — no silent re-generation'); + final expectedAddress = committed.currentAccount.primaryAddress.address.hexEip55; + verify( + () => repo.createWallet('Main', WalletType.software, draft.seed, expectedAddress), + ).called(1); + }); + + test('writes exactly one row per call (no implicit dedup at this layer)', () async { + // Pin the disk-side contract: each commit call is one row. The dedup + // lives at the cubit layer (`VerifySeedCubit.verify` is invoked once + // per successful quiz). Surfaces a regression where commit silently + // dedups and a follow-up caller assumes idempotence. + when(() => repo.createWallet(any(), any(), any(), any())).thenAnswer((_) async => 1); + + final draft = await service.generateUncommittedSeedWallet('Main'); + await service.commitGeneratedWallet(draft); + + verify(() => repo.createWallet(any(), any(), any(), any())).called(1); + }); + + test('does not set the wallet as current (caller is responsible)', () async { + when(() => repo.createWallet(any(), any(), any(), any())).thenAnswer((_) async => 7); + + final draft = await service.generateUncommittedSeedWallet('Main'); + await service.commitGeneratedWallet(draft); + + verifyNever(() => settings.saveCurrentWalletId(any())); + }); + }); + group('createSeedWallet', () { - test('returns a SoftwareWallet with the generated mnemonic and address persisted', () async { + test('generate+commit convenience — persists a freshly generated mnemonic in one call', + () async { when(() => repo.createWallet(any(), any(), any(), any())).thenAnswer((_) async => 42); final wallet = await service.createSeedWallet('Main'); diff --git a/test/screens/create_wallet/create_wallet_cubit_test.dart b/test/screens/create_wallet/create_wallet_cubit_test.dart index ecac97ec..79447496 100644 --- a/test/screens/create_wallet/create_wallet_cubit_test.dart +++ b/test/screens/create_wallet/create_wallet_cubit_test.dart @@ -14,6 +14,8 @@ class _MockAuthService extends Mock implements DFXAuthService {} class _FakeWalletAccount extends Fake implements AWalletAccount {} +class _FakeSoftwareWallet extends Fake implements SoftwareWallet {} + const _testMnemonic = 'test test test test test test test test test test test junk'; @@ -23,6 +25,9 @@ void main() { setUpAll(() { registerFallbackValue(_FakeWalletAccount()); + // Needed by the disk-side regression test that asserts + // `commitGeneratedWallet(any())` is never called. + registerFallbackValue(_FakeSoftwareWallet()); }); setUp(() { @@ -41,15 +46,19 @@ void main() { test('createWallet stores the newly created SoftwareWallet in state', () async { final wallet = SoftwareWallet(7, 'Obi-Wallet-Kenobi', _testMnemonic); - when(() => service.createSeedWallet(any())).thenAnswer((_) async => wallet); + when(() => service.generateUncommittedSeedWallet(any())).thenAnswer((_) async => wallet); final cubit = CreateWalletCubit(service, authService); cubit.createWallet(); await cubit.stream.firstWhere((s) => s.wallet != null); expect(cubit.state.wallet, same(wallet)); - verify(() => service.createSeedWallet('Obi-Wallet-Kenobi')).called(1); + verify(() => service.generateUncommittedSeedWallet('Obi-Wallet-Kenobi')).called(1); verify(() => authService.ensureSignatureFor(wallet.currentAccount)).called(1); + // Pin the disk-side guarantee: the cubit MUST NOT commit on + // generation — that's `VerifySeedCubit.verify()`'s job, gated on + // the user actually keeping the seed. + verifyNever(() => service.commitGeneratedWallet(any())); }); blocTest( @@ -67,7 +76,7 @@ void main() { test('toggleShowSeed preserves the wallet field', () async { final wallet = SoftwareWallet(1, 'W', _testMnemonic); - when(() => service.createSeedWallet(any())).thenAnswer((_) async => wallet); + when(() => service.generateUncommittedSeedWallet(any())).thenAnswer((_) async => wallet); final cubit = CreateWalletCubit(service, authService); cubit.createWallet(); await cubit.stream.firstWhere((s) => s.wallet != null); @@ -87,7 +96,7 @@ void main() { testWidgets('hidden drops the just-generated mnemonic from cubit state', (tester) async { final wallet = SoftwareWallet(7, 'Obi-Wallet-Kenobi', _testMnemonic); - when(() => service.createSeedWallet(any())).thenAnswer((_) async => wallet); + when(() => service.generateUncommittedSeedWallet(any())).thenAnswer((_) async => wallet); final cubit = CreateWalletCubit(service, authService); addTearDown(cubit.close); @@ -156,7 +165,7 @@ void main() { testWidgets('${lifecycle.name} does NOT clear the cubit state — only hidden does', (tester) async { final wallet = SoftwareWallet(7, 'Obi-Wallet-Kenobi', _testMnemonic); - when(() => service.createSeedWallet(any())).thenAnswer((_) async => wallet); + when(() => service.generateUncommittedSeedWallet(any())).thenAnswer((_) async => wallet); final cubit = CreateWalletCubit(service, authService); addTearDown(cubit.close); cubit.createWallet(); @@ -188,9 +197,12 @@ void main() { 'hidden → resumed re-generates a fresh wallet so the view is not ' 'stuck on the loading indicator', (tester) async { var generated = 0; - when(() => service.createSeedWallet(any())).thenAnswer((_) async { + when(() => service.generateUncommittedSeedWallet(any())).thenAnswer((_) async { generated++; - return SoftwareWallet(generated, 'Obi-Wallet-Kenobi', _testMnemonic); + // id stays 0 — the draft is uncommitted until VerifySeedCubit + // confirms the seed. The `generated` counter is the proof of + // re-generation, not an artefact of the id field. + return SoftwareWallet(0, 'Obi-Wallet-Kenobi', _testMnemonic); }); // Record every emission so we can pin both the intermediate cleared // state AND the regenerated state — without the recording, `pump` @@ -237,8 +249,14 @@ void main() { expect(emissions.last.wallet, isNot(same(initial)), reason: 'a NEW SoftwareWallet must be generated, not the cleared one'); expect(generated, 2, - reason: '_dropMnemonic must re-fire createSeedWallet so the view ' - 'recovers from the cleared state'); + reason: '_dropMnemonic must re-fire generateUncommittedSeedWallet ' + 'so the view recovers from the cleared state'); + // Disk-side pin for the Option B refactor: the cubit must NEVER + // commit on its own. `WalletStorage.deleteWallet` only touches + // `walletAccountInfos`, so any commit here would write an + // undeletable row to `walletInfos` and accumulate one per + // hide-cycle. + verifyNever(() => service.commitGeneratedWallet(any())); }); }); }); diff --git a/test/screens/create_wallet/create_wallet_page_test.dart b/test/screens/create_wallet/create_wallet_page_test.dart index 95332d49..2c5c2236 100644 --- a/test/screens/create_wallet/create_wallet_page_test.dart +++ b/test/screens/create_wallet/create_wallet_page_test.dart @@ -48,7 +48,8 @@ void main() { // account or the unstubbed null trips the cast. final stubbedWallet = MockWallet(); when(() => stubbedWallet.currentAccount).thenReturn(MockWalletAccount()); - when(() => walletService.createSeedWallet(any())).thenAnswer((_) async => stubbedWallet); + when(() => walletService.generateUncommittedSeedWallet(any())) + .thenAnswer((_) async => stubbedWallet); getIt.registerSingleton(walletService); // CreateWalletCubit now depends on DFXAuthService (via DfxKycService — the // smallest registered subclass) to pre-warm the auth signature on diff --git a/test/screens/verify_seed/cubit/verify_seed_cubit_test.dart b/test/screens/verify_seed/cubit/verify_seed_cubit_test.dart index 47819c2c..2a5bb6a2 100644 --- a/test/screens/verify_seed/cubit/verify_seed_cubit_test.dart +++ b/test/screens/verify_seed/cubit/verify_seed_cubit_test.dart @@ -14,10 +14,21 @@ void main() { late _MockWalletService service; late SoftwareWallet wallet; + setUpAll(() { + // Needed for the `commitGeneratedWallet(any())` matcher. + registerFallbackValue(SoftwareWallet(0, 'fallback', _testMnemonic)); + }); + setUp(() { service = _MockWalletService(); - wallet = SoftwareWallet(1, 'Main', _testMnemonic); + // The cubit receives an uncommitted draft from `CreateWalletCubit` + // (id == 0). `verify` is what lands the row, via + // `WalletService.commitGeneratedWallet`. Mirror that contract here. + wallet = SoftwareWallet(0, 'Main', _testMnemonic); when(() => service.setCurrentWallet(any())).thenAnswer((_) async {}); + when(() => service.commitGeneratedWallet(any())).thenAnswer( + (_) async => SoftwareWallet(42, 'Main', _testMnemonic), + ); }); group('$VerifySeedCubit', () { @@ -71,7 +82,8 @@ void main() { expect(fresh.state.hasError, isFalse); }); - test('verify returns true and marks the wallet current when all words match', () async { + test('verify returns true and marks the COMMITTED wallet current when all words match', + () async { final cubit = VerifySeedCubit(wallet, service); final result = await cubit.verify(); @@ -79,10 +91,36 @@ void main() { expect(result, isTrue); expect(cubit.state.isVerified, isTrue); expect(cubit.state.hasError, isFalse); - verify(() => service.setCurrentWallet(wallet.id)).called(1); + // The current wallet id must be the COMMITTED id (42), not the + // uncommitted draft's `0` sentinel. Closes the regression where a + // future refactor passes `_wallet.id` directly to `setCurrentWallet` + // and silently routes onboarding to a non-existent wallet row. + verify(() => service.setCurrentWallet(42)).called(1); + verifyNever(() => service.setCurrentWallet(0)); + }); + + // Pin the ordering: commit must precede setCurrentWallet so the row + // exists before any downstream `getCurrentWallet` call can resolve it. + test('verify commits the draft BEFORE marking it current', () async { + final calls = []; + when(() => service.commitGeneratedWallet(any())).thenAnswer((inv) async { + calls.add('commit'); + return SoftwareWallet(99, 'Main', _testMnemonic); + }); + when(() => service.setCurrentWallet(any())).thenAnswer((inv) async { + calls.add('setCurrent(${inv.positionalArguments.single})'); + }); + + final cubit = VerifySeedCubit(wallet, service); + await cubit.verify(); + + expect(calls, ['commit', 'setCurrent(99)'], + reason: 'commit must land the row before `setCurrentWallet` points ' + 'the settings repository at it'); }); - test('verify returns false, sets hasError, and does NOT mark current on a wrong word', () async { + test('verify returns false, sets hasError, and does NOT commit or mark current on a wrong word', + () async { final cubit = VerifySeedCubit(wallet, service); cubit.updateWord(0, 'definitely-not-a-seed-word'); @@ -91,6 +129,10 @@ void main() { expect(result, isFalse); expect(cubit.state.hasError, isTrue); expect(cubit.state.isVerified, isFalse); + // The disk-side guarantee for failure paths: no `walletInfos` row + // is written for a rejected verification. Pairs with the + // CreateWalletCubit "zero commits across regenerates" pin. + verifyNever(() => service.commitGeneratedWallet(any())); verifyNever(() => service.setCurrentWallet(any())); }); });