fix(unlock+bitbox): #461 follow-ups — finish routing, robustness, tests, idle-lock#472
Closed
Blume1977 wants to merge 14 commits into
Closed
fix(unlock+bitbox): #461 follow-ups — finish routing, robustness, tests, idle-lock#472Blume1977 wants to merge 14 commits into
Blume1977 wants to merge 14 commits into
Conversation
TaprootFreak
force-pushed
the
fix/pr461-followups
branch
from
e19daf3 to
e18d4dd
Compare
Contributor
|
PR-Body-Behauptungen wurden Punkt für Punkt gegen den Code verifiziert. Aussagen sind ehrlich, Coverage ist breit. Hier die strukturierte Bewertung. 1. Issue #468 Coverage — gegen den Code verifiziert
Verdict: 🟢 22/22 Items adressiert, keine stillen Skips. Keine überhöhten Claims gefunden. 2. Architektur 🟢
3. Security 🟢PIN-Bump 100k → 250k mit nachvollziehbarer Begründung im Code-Kommentar (sub-second auf mid-range Phones, Hardware-Boundary + Lockout-Cascade als primäre Verteidigung). Issue 4.1 wollte „measured number" — 250k ist begründet, nicht arbiträr. Mnemonic wird tatsächlich gedroppt (kein Flag-Flip): 🟡 nice-to-have: 4. Routing 🟢1.1, 1.2, 1.3 alle drei verifiziert (siehe Tabelle). 5. Robustness 🟢3.1 ( 6. Tests 🟢
7. Code-Qualität / Konventionen 🟢
8. Compliance 🟢
9. PR-Body 🟢Strukturiert nach Severity-Buckets, mapped 1:1 auf Issue #468, hat Test-Plan inkl. manueller iOS-Schritte, referenziert 🟡 nit: „1 pre-existing failure on 10. Production-Readiness —
|
TaprootFreak
force-pushed
the
fix/pr461-followups
branch
from
e18d4dd to
57b2dc5
Compare
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
Senior review follow-ups on PR RealUnitCH#472 — four small correctness fixes around the post-unlock locking contract introduced in RealUnitCH#461. - DFXAuthService.getSignature now wraps the sign + saveSignature in a try/finally so a thrown SigningCancelledException / TimeoutException / BitBox disconnect / cache-write failure can't skip lockCurrentWallet() and leave the mnemonic resident for the full 60 s window. Matches the pattern already used in RealUnitSellPaymentInfoService.confirmPayment and RealUnitRegistrationService.completeRegistration / registerWallet. - WalletService._scheduleIdleLock callback no longer double-nulls the timer field. lockCurrentWallet() owns the cancel + null itself, so the callback now binds directly to the bound method (Timer(timeout, lock)) and the two pieces of code stop sharing the invariant. - WalletService._idleLockTimeout / _idleLockTimer renamed to _postUnlockLockTimeout / _postUnlockLockTimer. The timer isn't reset on user activity — it caps the lifetime of the in-memory mnemonic after each explicit unlock — so "idle" was the wrong noun. Doc comment updated to say so. - BitboxService._credentialsByAddress now goes through a private _key() normaliser (lowercase) so a future EIP-55-vs-raw-hex address handoff can't fork the map and leak / orphan credentials. Lowercase invariant documented at the field declaration.
Contributor
Re-Verification nach Fix-Pass
|
| Iteration | Findings | Status |
|---|---|---|
| #1 Deep-Review | 1 Bug (Lock-Lifecycle-Contract-Bruch im häufigsten Auth-Pfad) + 3 Should-Fix + 3 Nice-to-have | gefunden |
Fix-Pass (45720cf) |
5 Fixes angewandt, lokal verifiziert | erledigt |
| #2 Re-Review | 0 Blocker, 0 Should-fix | clean |
Was Iteration #1 fand und Fix-Pass behoben hat
-
DFXAuthService.getSignatureohnetry/finally→ Mnemonic blieb bei jedemSigningCancelledException/TimeoutException/ Cache-Write-Fehler 60s im Heap. Bricht den ganzen 1.3/4.2-Lock-Lifecycle-Contract am am häufigsten ausgelösten Pfad. → Gefixt mittry { sign; saveSignature; return; } finally { lockCurrentWallet; }wie inconfirmPayment/completeRegistration/registerWallet. -
_idleLockTimerCallback nullte das Feld doppelt →lockCurrentWallet()macht das selbst. → Auf direkte Method-Bindung gewechselt. -
BitboxService._credentialsByAddresswar case-sensitive → Multi-Wallet-Future (3.5) hätte bei EIP-55-vs-raw-hex Adressen-Mismatch eine zweite Credentials-Instanz angelegt. →_key()Normalizer (lowercase) eingeführt, Invariant dokumentiert. -
_idleLockTimeoutNaming war irreführend (Timer reset NICHT bei User-Aktivität, nur bei Unlock). → Umbenannt zu_postUnlockLockTimeoutmit präziserem Doc-Kommentar. -
PR-Body sagte "1373 pass, 1 pre-existing failure" — stimmte nach Rebase nicht mehr. → Aktualisiert auf "1382/1382 green".
Was Iteration #2 verifiziert hat
- ✅ Alle 5 Fixes korrekt + komplett angewandt
- ✅ Alle 5
ensureCurrentWalletUnlocked()Call-Sites paaren sauber mitlockCurrentWallet()(4× try/finally in Services, 1× Cubit-close()+_postUnlockLockTimerSafety-Net) - ✅ Defense-in-depth bei
_LockedCredentials+SoftwareViewWalletAccount:assert(false, ...)Debug +throw StateError(...)Release - ✅ Map-Read-Paths sind alle key-agnostisch (
.valuesIteration) — kein orphan direct-index - ✅ Auto-tag / auto-release Workflows unbetroffen
- ✅
flutter analyzeclean außerhalblib/generated/ - ✅
flutter test --coverage1382/1382 green
Verbleibende 🟡-Items (alle non-blocking / pre-existing)
- Timer-Callback feuert
Future<void>ohne await —lockCurrentWalletist effektiv non-throwing - iOS suspendiert Dart-
Timers im Background — 60s-Cap ist Best-Effort, das PIN-Gate (5min Lockout) ist die echte Schutzschicht; pre-existing, nicht durch diese PR verschlechtert - App-Lifecycle ruft
lockCurrentWalletnicht explizit auf Hide auf — könnte ein eigenes Follow-up-Issue werden
Merge-Empfehlung: ✅ code ready
Code-seitig alles sauber. Architektur-Migration konsistent, alle Issue #468-Items adressiert, Tests grün, kein neuer Defekt eingeführt.
Was noch fehlt für vollen Production-Greenlight (Process, nicht Code):
- CI-Approval ("Approve and run" für Fork-PR)
- Code-Review-Approval (
reviewDecision: REVIEW_REQUIRED) - Manuelle Hardware-QA (5 Checkboxen im Test-Plan)
Issue RealUnitCH#468 follow-ups RealUnitCH#461 — items A (routing) and F (architecture). Restores AppStore as a pure state container by giving the locking lifecycle to WalletService directly: - AppStore: drop _unlocker / attachUnlocker / ensureUnlocked. - WalletService: take AppStore as a constructor dep, add ensureCurrentWalletUnlocked() (1.3) and lockCurrentWallet() (4.2 prep). - WalletService now a lazySingleton (6.2). - DFXAuthService and all 12 subclasses thread WalletService through their constructors; getSignature() unlocks → signs → locks again. - SellPaymentInfoService.confirmPayment / RegistrationService.completeRegistration / registerWallet lock the wallet in a finally block so a mid-sign throw doesn't leave the mnemonic resident. - SettingsSeedCubit takes WalletService directly; locks again on close(). SellButton routes PaymentInfoError.bitboxDisconnected through the showBitboxReconnectSheet helper (1.1) — matches the buy / sell-bitbox / user-data flows. _LockedCredentials / SoftwareViewWalletAccount drop the typed WalletLockedException (1.2): every signing path now goes through ensureCurrentWalletUnlocked() first, so the locked sign call is a programmer error — surfaced via assert + StateError instead of an exception nothing catches. WalletLockedException class removed. SellBitboxCubit drops the redundant credentials.isConnected pre-check in confirmSwap / confirmDeposit (6.3) — the existing on BitboxNotConnectedException catch covers it. Page tests for CreateWallet / RestoreWallet now register DfxKycService alongside WalletService so the cubit's DFXAuthService dep resolves (5.1). SettingsSeedCubit._loadSeed uses copyWith so a toggleShowSeed that races ahead of the unlock isn't dropped when the seed lands.
Issue RealUnitCH#468 follow-ups RealUnitCH#461 — items 2.1 / 2.2 (UX regressions). ConnectBitboxCubit / CreateWalletCubit / RestoreWalletCubit now wrap the DFXAuthService.ensureSignatureFor call in unawaited(), so a slow 20 s HTTP timeout no longer freezes the onboarding UI between create/restore/pair and the success state. The lazy sign in DFXAuthService.getSignature still recovers on failure. ConnectBitboxCubit additionally: - starts the connection-status observer before the unawaited warm-up so a disconnect mid-warm-up is detected. - raises the warm-up failure to SEVERE so it surfaces in support logs — the failure mode it papers over (extra BitBox prompt on first auth) is exactly the regression RealUnitCH#461 was fixing. i18n: new `connectBitboxSignInHint` key (en + de, alphabetically inserted between connectBitboxFailed and connectBitboxTitle) shown on the channel-hash verify screen, so the user knows a second BitBox confirmation is coming after the code match.
Issue RealUnitCH#468 follow-ups RealUnitCH#461 — items 3.1 through 3.5. - _runOrThrowDisconnect narrows to `on Exception catch` so a TypeError / assert from a sign-internal bug stays its own error instead of being silently re-labelled as a BitboxNotConnectedException. The original error + stack are logged before the rewrap. (3.1) - Every sign method snapshots `bitboxManager` + `derivationPath` into locals up-front, so the disconnect observer nulling them between the null-check and the call can no longer NoSuchMethod the bang-operator path. (3.2) - BitboxService tracks an in-flight `_pendingDisconnect` Future and awaits it at the top of `init()`, so a rapid disconnect-then-reconnect on the same singleton manager no longer races. The disconnect path also moves to `on Exception catch` to match the rest of the file. (3.3) - `BitboxCredentials.setBitbox(connection, [path])` falls back to the existing `derivationPath` when no path is passed, and `clearBitbox()` no longer wipes the path. Default-path-today, but multi-account would silently revert on every reconnect otherwise. (3.4) - `BitboxService._credentialsByAddress` (a Map) replaces the single `_credentials` field, so a wallet switch that hands out fresh credentials no longer orphans the previous wallet's reference. The reconnect observer + clear paths iterate the map. (3.5)
Issue RealUnitCH#468 follow-ups RealUnitCH#461 — items 4.1 and 4.2. - SecureStorage._pinHashIterations: 100_000 → 250_000. Roughly halves the offline brute-force budget for an attacker who has already broken the Keystore/Keychain boundary while staying sub-second on a mid-range phone. 100_000 hashes are now accepted once via [_legacyIterationCandidates] and transparently rehashed up to the new target on the next successful unlock. (4.1) - WalletService.ensureCurrentWalletUnlocked() arms a 60 s idle timer that calls lockCurrentWallet() if the caller forgets. Covers the "user sells once then leaves the app foregrounded" path the reviewer flagged — the mnemonic can't sit on the heap for an hour just because one sign path skipped the explicit lock. lockCurrentWallet() cancels the timer. (4.2) - RealUnitRegistrationService.completeRegistration / registerWallet now use try/finally so the explicit lock fires on the throw path too.
Issue RealUnitCH#468 follow-ups RealUnitCH#461 — items 5.2 and 5.3. - $SoftwareViewWallet: assert every sign entry point (signMessage, signToSignature, signPersonalMessage, signPersonalMessageToUint8List, signToEcSignature) surfaces as an Error subtype — AssertionError in debug, StateError in release — so a caller that bypasses WalletService.ensureCurrentWalletUnlocked() can't quietly read a wrong- type signature. - $WalletService.ensureCurrentWalletUnlocked: promotes a view wallet to a SoftwareWallet via getUnlockedWalletById, no-op for already-unlocked wallets. - $WalletService.lockCurrentWallet: writes back a SoftwareViewWallet, no-op for non-software wallets. - $BitboxCredentials race tests: - disconnect-mid-sign: manager.devices returns empty during sign → BitboxNotConnectedException (not the underlying Exception leaking). - cleared credentials: a sign called after clearBitbox() throws BitboxNotConnectedException — exercises the snapshot-on-entry pattern so a NoSuchMethodError can't slip through the bang-operator path.
Senior review follow-ups on PR RealUnitCH#472 — four small correctness fixes around the post-unlock locking contract introduced in RealUnitCH#461. - DFXAuthService.getSignature now wraps the sign + saveSignature in a try/finally so a thrown SigningCancelledException / TimeoutException / BitBox disconnect / cache-write failure can't skip lockCurrentWallet() and leave the mnemonic resident for the full 60 s window. Matches the pattern already used in RealUnitSellPaymentInfoService.confirmPayment and RealUnitRegistrationService.completeRegistration / registerWallet. - WalletService._scheduleIdleLock callback no longer double-nulls the timer field. lockCurrentWallet() owns the cancel + null itself, so the callback now binds directly to the bound method (Timer(timeout, lock)) and the two pieces of code stop sharing the invariant. - WalletService._idleLockTimeout / _idleLockTimer renamed to _postUnlockLockTimeout / _postUnlockLockTimer. The timer isn't reset on user activity — it caps the lifetime of the in-memory mnemonic after each explicit unlock — so "idle" was the wrong noun. Doc comment updated to say so. - BitboxService._credentialsByAddress now goes through a private _key() normaliser (lowercase) so a future EIP-55-vs-raw-hex address handoff can't fork the map and leak / orphan credentials. Lowercase invariant documented at the field declaration.
…removed the class PR RealUnitCH#478 landed an exception surface test that enumerates every typed exception. PR RealUnitCH#472 removed WalletLockedException in favour of assert + StateError (locked-sign is now a programmer error, not a runtime exception). Remove the obsolete entry + import so the test compiles against the post-RealUnitCH#472 surface.
…#472 map PR RealUnitCH#473 pinned the pre-RealUnitCH#472 contract: each getCredentials() returned a fresh instance. PR RealUnitCH#472's robustness pass replaced the single _credentials slot with a Map<address, BitboxCredentials> + a per-entry clear loop in the disconnect observer. Under the new contract every caller for a given address must see the *same* canonical instance — otherwise the observer would clear an orphaned reference while the caller keeps holding the older one. Flip the assertion + rewrite the defensive-pin comment to match.
TaprootFreak
force-pushed
the
fix/pr461-followups
branch
from
45720cf to
a07d7e0
Compare
…can't trip MnemonicReadOnlyField's length assert RealUnitCH#472 left SettingsSeedView wide open to a first-frame crash whenever the app boots with a SoftwareViewWallet — the default production state after RealUnitCH#461. The cubit's initial state has an empty seed, the view used to hand that straight to SeedBlurCard → MnemonicReadOnlyField([]) and the `length == 12` assert killed the screen on every "Wallet sichern" open in debug, with a RangeError taking its place in release. The existing settings_seed_page_test couldn't catch it because the mocked appStore.wallet was a SoftwareWallet, not a SoftwareViewWallet — the seed was already in the initial state and the path that crashes in production never ran. Show a CupertinoActivityIndicator inside the BlocBuilder until the seed actually has 12 words. The early-seed optimisation for the already-unlocked case stays intact (state.seed is filled in the constructor), so re-opening the screen after one successful unlock still renders the SeedBlurCard on frame 0. Regression test added: drives a real SettingsSeedCubit against a SoftwareViewWallet fixture, holds the unlock future on a Completer, asserts the first frame shows the spinner with no SeedBlurCard, then completes the unlock and asserts the SeedBlurCard appears.
…nlock await `_loadSeed` awaits `ensureCurrentWalletUnlocked` and then casts + emits. If the user navigates away while the DB decrypt is in flight the cubit closes mid-await — the post-await emit then throws `StateError: Cannot emit after close` as an unhandled async error. Add an `if (isClosed) return;` between the await and the cast so the cubit closing during the unlock window is a quiet no-op instead of an uncaught error.
…lows can't tear the unlocked wallet out from under each other RealUnitCH#472's ensure-then-lock pair was racey for two concurrent sign flows: flow A unlocks → signs → finally → locks; flow B between A's ensure and A's lock saw no-op (already unlocked) and proceeded mid-sign. A then locks, B reads `appStore.wallet.currentAccount.primaryAddress` → `_LockedCredentials` → `Eip712Signer` throws `UnsupportedError` because the locked sentinel has no signing surface. Add an `_activeUnlockHolders` counter: every ensure increments, every lock decrements and only flips the wallet back when the count hits 0. The 60s safety net can't respect the counter — a stuck holder would keep the mnemonic resident past the window — so it now runs through a dedicated `_forceLock` that zeroes the counter and unconditionally locks. The explicit `lockCurrentWallet` path still owns the holder contract. Test: two parallel ensures + one lock leaves the wallet unlocked (second holder still active); the second lock flips it back to a SoftwareViewWallet.
…oss paths can't race two disconnect() calls The periodic callback's body is async and straddles the `getAllUsbDevices()` await. On a slow probe two ticks can both reach the `devices.isEmpty` branch, both run `_pendingDisconnect = _disconnectAndForget()`, and the second assignment clobbers the first — now two `disconnect()` calls race on the SDK manager singleton with nothing left to await on. Guard the branch with `if (!_isConnected) return;` *before* flipping `_isConnected = false`. The first tick's flag flip is the single-writer fence: any second tick that arrived between the two ticks sees the already-cleared flag and bails before touching credentials, the observer cancel, or _pendingDisconnect.
…oarding flows log at the same SEVERE level
Three near-duplicate `_warmAuthSignature` helpers existed in
CreateWalletCubit, RestoreWalletCubit and ConnectBitboxCubit. Only the
BitBox one logged at `level: 1000` (SEVERE) — the other two used
default-level developer.log, which doesn't reach support telemetry.
PR body claimed SEVERE everywhere; only one of three actually was.
Move the helper to a top-level `warmAuthSignature(authService,
account, {required loggerName})` in dfx_auth_service.dart. All three
cubits call it; all log at SEVERE; the BitBox-specific copy in the
"next call triggers a fresh confirmation" wording is generalised to
"next authenticated call will trigger a fresh signature request".
create_wallet_page_test: the cubit now reads `wallet.currentAccount`
synchronously before handing it to the unawaited helper, so the
MockWallet has to surface a real account. Stub it (was: relied on the
old helper's try/catch swallowing the unstubbed-null cast).
…amed the method to ensureCurrentWalletUnlocked Three callsites in the test tree still referenced the old name: - settings_seed_cubit_test L33 test name + L36 comment - real_unit_sell_payment_info_service_confirm_test L110 comment Rename so a future reader searching for the actual method finds the test, not the drift.
6 tasks
Blume1977
added a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
Stacked on top of RealUnitCH#472. 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 exactly the scenario the reviewer flagged in RealUnitCH#468 (4.2) as the residual gap 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: it fires earlier than paused on iOS (covers the multitasking-switcher and notification-drag-down cases), Android raises it via the unified lifecycle pipeline, and PinAuthCubit already uses it for the lockout-time stamp.
3 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 21, 2026
…light dedup (#483) > Stacked on #472 — apply only after that merges, or rebase onto develop post-merge. > > **Base note:** GitHub couldn't host this PR with `base = fix/pr461-followups` because that branch only exists on Blume1977's fork (not on DFXswiss). So the base falls back to `develop` and the diff therefore includes everything from #472 plus the polish delta below. Once #472 merges, this PR will show only the polish delta. This collects the iteration-4 audit's "nice-to-have" polish items on top of #472 (`fix/pr461-followups`). The 4 polish items are each their own commit and live on top of `7ef2224` (current tip of #472's branch). ## Polish items ### 1. Real reentrancy race test `test/packages/service/wallet_service_test.dart` The existing reentrancy test sequentially `await`s both `ensureCurrentWalletUnlocked()` calls — that exercises the counter, not a true race. Added a `Completer<WalletInfo>`-gated test that holds two `ensureCurrentWalletUnlocked()` calls mid-decrypt, fires a `lockCurrentWallet()` between them, then completes the gate. Asserts the wallet stays unlocked (lock did not shadow the in-flight unlock) and locks back only after the remaining holders are drained. The earlier mechanical test stays in place — they're complementary. ### 2. DRY the `_LockedCredentials` sign-method rationale `test/packages/wallet/wallet_test.dart` Five identical ~4-line comments explaining the `assert(false)` + `StateError` contract were extracted into a top-level `_viewWalletErrorRationale` constant. Each test now passes that string as the `reason:` argument (first occurrence) or references the constant by name in a one-line comment. No test bodies changed. ### 3. Visibility consistency: colocate `SoftwareViewWalletAccount` with `_LockedCredentials` `lib/packages/wallet/wallet.dart` + `lib/packages/wallet/wallet_account.dart` `DebugWallet`'s analogs (`_DebugCredentials` + `DebugWalletAccount`) live in the same file (`wallet.dart`). The view-wallet analogs were split — `_LockedCredentials` in `wallet.dart`, `SoftwareViewWalletAccount` in `wallet_account.dart`. Moved `SoftwareViewWalletAccount` next to `_LockedCredentials` in `wallet.dart` to match the existing pattern. Safe because `SoftwareViewWalletAccount` is only referenced from `wallet.dart` (production) and one test file that already imports both files. No import changes needed elsewhere. ### 4. Track in-flight unlock to dedupe DB decryption `lib/packages/service/wallet_service.dart` Two overlapping `ensureCurrentWalletUnlocked()` calls on a `SoftwareViewWallet` previously each triggered `unlockCurrentWallet()` (DB read + AES-GCM decrypt). Functionally identical results, but wasteful. Added a `Future<SoftwareWallet>? _unlockInFlight` field; the first caller starts the unlock and stores the future, subsequent overlapping callers await the same future. Cleared in `finally` (only by the caller that started it, via `identical` check) so the next post-lock ensure runs a fresh unlock. New test verifies that two parallel ensures only invoke `repository.getUnlockedWalletById` once. ## Test plan - [x] `flutter analyze` — only pre-existing `lib/generated/` issues (33), no new lint warnings - [x] `flutter test --coverage` — 1412 tests pass (1410 baseline + 2 new race / dedup tests) - [ ] Manual smoke: trigger a sign flow, lock, sign again — confirm no `StateError` from `_LockedCredentials` and the in-memory mnemonic still drops on the 60s safety net --------- Co-authored-by: Blume1977 <jana.ruettimann@dfx.swiss>
Contributor
|
subsumed by #483 |
7 tasks
TaprootFreak
pushed a commit
that referenced
this pull request
May 21, 2026
Supersedes Blume1977#1 (closed) — same intent, rebased onto post-#483 develop, addresses TaprootFreak's two review points by pushing the precondition into `WalletService` instead 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: > 🟡 iOS suspendiert Dart-`Timer`s im Background — 60s-Cap ist Best-Effort. App-Lifecycle ruft `lockCurrentWallet` nicht explizit auf Hide auf — könnte ein eigenes Follow-up-Issue werden. This closes that gap. ## What changed Three atomic commits: 1. **`feat(app-store): isWalletLoaded getter`** — non-throwing predicate so services can early-return defensively. Named distinctly from `WalletService.hasWallet()` (which is persisted state, not in-memory load state) so the two predicates can't be confused at the call site. 2. **`fix(wallet-service): no-op lockCurrentWallet when no wallet is loaded`** — Onboarding-path guard inside `WalletService` itself. An alternative would have been a `try/catch` at the call site, which would silently mask any future regression in `lockCurrentWallet`. 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 `!isWalletLoaded` no-op, and a lifecycle-hidden-during-active-sign scenario that pins the counter-underflow guard. 3. **`feat(lifecycle): lock the wallet on hidden`** — `LifecycleInitializer._onHidden()` fires `unawaited(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` (not `paused`) - Fires **earlier than `paused`** on iOS (covers multitasking switcher + notification drag-down). - Android raises it via the unified lifecycle pipeline. - `PinAuthCubit` already uses `_onHidden` for 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 in `CreateWalletState.wallet` (a `Cubit` state inside the create-wallet flow) — that copy lives independent of `AppStore` and `lockCurrentWallet` is 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 `catchError` the unawaited future" The architecture decision — _the lifecycle caller deliberately does not attach `.catchError` or 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 that `tester.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: - **Naming-Kollision (Blocker 1)** — renamed `hasWallet` to `isWalletLoaded` on `AppStore` to avoid colliding with `WalletService.hasWallet()` which has different semantics. - **Holder-Counter test gap (Blocker 2)** — added a test that simulates `ensure → unpaired lifecycle lock → finally lock`, verifying the underflow guard prevents counter drift and the next ensure cycle still works. - **Inline comment too long (R3)** — condensed from 15 lines to 7; rationale moved to commit message + this PR body. - **Microtask ordering note (R4)** — added a one-line note in the lifecycle hook for any future contributor who introduces `ensureCurrentWalletUnlocked()` in `_onResumed`. - **Test setup drift (R5)** — `isWalletLoaded = true` moved out of the global `setUp` into per-group `setUp` so unrelated groups don't carry the stub and the negative path isn't masked. - **Doc accuracy (R7)** — clarified that both `LoadCurrentWalletEvent` and `LoadWalletEvent` populate `AppStore.wallet`. Why the architecture diverges from the original draft (Blume1977#1): previous incarnation wrapped `lockCurrentWallet()` in a `try { } 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 to `WalletService`, 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 - [x] \`flutter analyze\` — clean - [x] \`flutter test --coverage\` — 1424 / 1424 green (1417 develop baseline + 7 new: 2 isWalletLoaded, 1 no-wallet lock, 1 unpaired-lock counter race, 3 lifecycle states) - [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 --coverage\` → \`lcov\` filter — all green; \`bitbox-audit\` 0 findings - [ ] **iOS manual**: sell → swipe up to multitasking → return → next sign re-decrypts in <100 ms, no PIN re-prompt within PinAuthCubit's lockoutDuration - [ ] **iOS manual**: sell → home button → wait 10 min → return → PIN re-prompt as before (unchanged) - [ ] **Android manual**: sell → recent-apps switcher → return → same as iOS path 1 - [ ] **Onboarding manual**: fresh install → reach onboarding screen → background app → return → no crash, no unhandled async error
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.
Closes #468.
Follow-up to the merged #461. Picks up the items the reviewer (#468) flagged as Critical / High / Medium / Low, plus the test gaps. Open as Draft — no manual QA yet.
Critical (1.x) — routing & silent-failure modes
bitboxDisconnected→ reconnect sheet. Matches Buy / SellBitbox / SettingsUserData.WalletLockedExceptiondeleted. Every sign path now goes throughWalletService.ensureCurrentWalletUnlocked()first, so a locked-credentials sign is a programmer error:assert(false, ...) + throw StateError(...)on_LockedCredentials.sign*+SoftwareViewWalletAccount.signMessage. The typed Exception that nothing caught is gone.AppStore.ensureUnlockedno-op gone. Lifecycle moved intoWalletService.ensureCurrentWalletUnlocked()directly — AppStore is back to a pure state container, no_unlockercallback to forget.SoftwareViewWallet. The cubit's initial empty seed used to flow straight intoMnemonicReadOnlyField'slength == 12assert; the view now shows aCupertinoActivityIndicatoruntil the async unlock fills in the 12 words. Regression test added (SoftwareViewWallet shows spinner, then SeedBlurCard).High (2.x) — onboarding latency + extra-press UX
ensureSignatureForis now fire-and-forget inConnectBitboxCubit/CreateWalletCubit/RestoreWalletCubit. A 20 s HTTP timeout on the warm-up no longer blocks the success-state emit.connectBitboxSignInHint(en + de, alphabetically inserted betweenconnectBitboxFailedandconnectBitboxTitle) shown on the channel-hash verify screen, so the user knows a second BitBox confirmation is coming after the code match. The warm-up failure path now logs at SEVERE so it lands in support telemetry instead of silently degrading the next auth call.warmAuthSignatureextracted to a top-level helper indfx_auth_service.dart. The three cubits used to each carry a near-duplicate_warmAuthSignature, two of which logged at default level while only the BitBox one was SEVERE. One helper, SEVERE everywhere — the PR-body claim now matches reality.Medium (3.x) — BitBox robustness
_runOrThrowDisconnectnarrows toon Exception catch— TypeErrors / asserts from a genuine sign-internal bug stay their own error instead of being relabelled as a disconnect. Original error + stack are logged on the rewrap path.bitboxManager+derivationPathcopied to locals up-front so the disconnect observer nulling them mid-sign producesBitboxNotConnectedException, not aNoSuchMethodError.BitboxService._pendingDisconnectFuture is awaited at the top ofinit()so a rapid disconnect-then-reconnect can't race two ops on the SDK manager singleton.setBitbox()preservesderivationPath. The reconnect path no longer silently reverts a non-default path on every re-attach;clearBitbox()keeps the path so the nextsetBitbox()restores it._credentialsByAddress: Map<String, BitboxCredentials>replaces the single_credentialsfield. Multi-wallet would orphan the previous wallet's reference otherwise.getAllUsbDevices()await used to both enter the empty-list branch and clobber_pendingDisconnect, racing twodisconnect()calls on the SDK manager. An earlyif (!_isConnected) return;at the top of the branch makes the second tick bail.Medium (4.x) — security trade-off revisit
_pinHashIterations = 250_000(was 100k). Roughly doubles offline brute-force cost while staying sub-second on a mid-range phone. 100k hashes are now in_legacyIterationCandidatesand transparently rehashed on the next successful unlock.WalletServicearms a 60 s idle-lock timer onensureCurrentWalletUnlocked(). If a sign path forgets the explicitlockCurrentWallet, the mnemonic is dropped 60 s later anyway — the reviewer's "user sells once then leaves app foregrounded" path now has a hard cap.RealUnitRegistrationService.completeRegistration/registerWalletmove their lock-down intotry/finallyso the throw path locks too.ensure/lockare now reentrancy-safe._activeUnlockHolderscounts active holders so a flow A finishing its sign + lock while a parallel flow B is still mid-sign can no longer tear the unlocked wallet out from under B (was: B's next sign hit_LockedCredentials→UnsupportedError). The 60s safety net bypasses the counter via_forceLockso a stuck holder can't pin the mnemonic past the window.SettingsSeedCubit._loadSeedbails onisClosed. A user navigating away during the DB-decrypt await used to surface as an unhandledCannot emit after closeasync error.Low (5.x + 6.x)
DfxKycServicealongsideWalletServiceso the cubits' DFXAuthService dep resolves. Was latent flaky._LockedCredentials.sign*, andWalletService.ensureCurrentWalletUnlocked/lockCurrentWallet.bitbox_credentials_test: disconnect-mid-sign surfacesBitboxNotConnectedException(not the underlying Exception), and a sign called afterclearBitbox()doesn't slip through via NoSuchMethod.wallet_service_test: two parallelensureCurrentWalletUnlocked+ onelockCurrentWalletleaves the wallet unlocked for the second holder.attachUnlockercallback gone. Lifecycle owned byWalletServicenow (see 1.3).WalletServiceisregisterLazySingletoninstead of factory —ensureUnlockedno longer rebuilds the service on every promote.SellBitboxCubit.confirmSwap/confirmDepositdropped. The existingon BitboxNotConnectedException catchalready routes to the reconnect state.Architectural shape after this PR
AppStoreis a pure state container. No DI-wired callbacks.WalletServiceowns the unlock/lock lifecycle (ensureCurrentWalletUnlocked+lockCurrentWallet+ post-unlock timer). Every DFX service that signs threads it via the constructor — 12 subclasses updated, no service-locator workarounds.WalletLockedExceptionbecause it doesn't exist.Test plan
dart run tool/generate_localization.dartflutter analyze— cleanflutter test— 1410/1410 green