fix(bitbox): recover from BLE/USB disconnect without losing wallet session#461
Merged
TaprootFreak merged 8 commits intoMay 20, 2026
Merged
Conversation
…ssion The BitBox service was nulling its `_credentials` reference on disconnect, so re-pairing produced a fresh manager but the wallet's existing credentials stayed cleared — every subsequent sign threw `BitboxNotConnectedException` that surfaced as raw `Instance of '...'` in the UI. Service layer: - Keep the `_credentials` reference across disconnects and re-attach the manager from `init()`, so existing wallets heal automatically on reconnect. - Close the underlying transport when the device disappears — Android needs the USB file-descriptor released for the next connect() to succeed. - Convert low-level errors mid-sign into a typed disconnect when the device is no longer reachable, instead of leaking raw plugin errors. UI / cubits: - Surface a typed `bitboxDisconnected` failure in buy/sell payment-info and a dedicated reconnect view in the user-data screen, each with a button that opens the pairing sheet and resumes the original action. - Catch `BitboxNotConnectedException` in `SellBitboxCubit.confirmSwap`/ `confirmDeposit` so a mid-flow drop routes through the existing reconnect state instead of dead-ending in a generic error snackbar. - Override `toString()` on the exception so any remaining untyped surface shows a readable message instead of `Instance of '...'`. Android: - Declare `android.hardware.usb.host` as an optional feature.
…s go BitBox-free By design, the BitBox is only required at pairing (to produce the DFX auth signature) and when selling RealUnit — buy, KYC and user-data should run off the cached signature alone. The signature was being created lazily on the first API call, so if BLE dropped between pairing and that first call the user hit `BitboxNotConnectedException` from a flow that shouldn't have needed the device at all. - `DFXAuthService.ensureSignatureFor(account)` — wallet-parameterised variant of the existing signature dance; no-op when the cache already has a signature for the same address. - `ConnectBitboxCubit.confirmPairing` calls it immediately after the wallet is created, while the noise channel is still active. Failures are logged but don't fail the pairing — the existing lazy path in `getSignature` is the fallback. - Wire `DfxKycService` in as the smallest concrete `DFXAuthService` for the cubit's DI, and update the cubit test to cover the new dependency.
…tion Three callsites (buy button, settings user-data, sell-bitbox) were each re-implementing the same `showModalBottomSheet → ConnectBitboxPage` pattern, two of them with the wrong HomeBloc event (`LoadWalletEvent` is for first- time pairing — re-pairing an existing wallet should use `SyncWalletServicesEvent`). - New `showBitboxReconnectSheet(context)` helper centralises the modal + sheet-pop + `SyncWalletServicesEvent` semantics; all three callsites use it. - `SettingsUserDataFailure` rendered the buy-flow `paymentInformationFailed` string for any user-data fetch failure. Replaced with a dedicated `userDataLoadFailed` key (de + en) and updated the page test. - `BitboxCredentials._runOrThrowDisconnect` collapsed the nested try/catch into a separate `_deviceLost()` predicate so the recovery path reads top-to-bottom.
…ubit close Two small lifecycle bugs surfaced in a second audit pass: - `SellBitboxCubit._startEthPolling` re-assigned `_ethPollingTimer` without cancelling the previous one. `retryAfterConnection` → `_checkEthBalance` → `_requestFaucet` → `_startEthPolling` can re-enter that branch with a timer still alive, leaking it (and emitting `SellBitboxEthReady` twice once balance arrives). Cancel before reassigning. - `ConnectBitboxCubit.close` nulled `_pendingInit` without detaching the underlying init future. If the BitBox plugin reports a late error after the cubit is gone, it surfaces as an unhandled exception. `ignore()` the future so late errors are swallowed.
`getSignMessage` and `ensureSignatureFor` were each running the same `GET /v1/auth/signMessage` request with slightly different response-parsing styles. Extracted a single private `_fetchSignMessage(address)` helper so the path, headers, timeout and error message live in one place.
Closes RealUnitCH#462. App-cold unlock for software wallets sat at ~10 s on mid-range phones: ~5 s for the 600 k-iteration PBKDF2 PIN check and another ~4 s for the eager mnemonic-decrypt + BIP32 derivation that ran purely so the dashboard could read the user's primary address. Neither of those steps gates anything the user is about to do — buy, KYC, user-data, balance and transaction history all run off the cached auth signature. Architecture: the private key only matters for sells and recovery-phrase display. Everything else is signature- and JWT-gated and tolerates the mnemonic staying encrypted. What changes: - Software-wallet rows now persist `(encryptedSeed, address)`. App start reads the address column and returns a `SoftwareViewWallet` — no decryption, no BIP32, no isolate. Legacy rows without a cached address fall back to the old decrypt path exactly once and the next write populates the column. - `WalletService.unlockCurrentWallet()` decrypts on demand. Sell signing (`RealUnitSellPaymentInfoService.confirmPayment`), KYC EIP-712 sign (`RealUnitRegistrationService.completeRegistration`), `DFXAuthService.getSignature` cache-miss and the recovery-phrase screen all call `appStore.ensureUnlocked()` immediately before reading credentials, which promotes the view wallet to a fully unlocked `SoftwareWallet` via a callback wired in DI. - `CreateWalletCubit` / `RestoreWalletCubit` mirror the BitBox flow and call `DFXAuthService.ensureSignatureFor` while the mnemonic is still in memory, so the very first authenticated call after onboarding hits the signature cache and never needs to re-unlock. - PIN-hash iteration count drops from 600 k to 100 k. The hash + salt live in `FlutterSecureStorage` (Android Keystore / iOS Keychain), so the offline-brute-force threat first has to break the hardware boundary; online brute-force is bounded by the existing 5/6/7/8/lock cascade. Earlier 10 k and 600 k hashes are still accepted and transparently upgraded on next verify.
…+ stub ensureUnlocked in service tests Two follow-ups discovered while reviewing the unlock-architecture commit: - `WalletService.getWalletById` for a legacy software-wallet row (empty `address` column) decrypted the mnemonic and returned a `SoftwareWallet` but never persisted the derived address. Every subsequent open hit the same slow path. Now writes the address back via the new `WalletRepository.updateAddress` / `WalletStorage.updateWalletAddress` so the next load takes the fast view-wallet path. - `RealUnitSellPaymentInfoService.confirmPayment`, `RealUnitRegistrationService.completeRegistration` and `DFXAuthService.getSignature` (cache-miss path) now call `appStore.ensureUnlocked()`. Tests that construct those services against `MockAppStore` now stub the call so mocktail doesn't trip on the missing default.
Contributor
|
Blume1977
force-pushed
the
fix/bitbox-disconnect-recovery
branch
from
413426e to
6b5f75c
Compare
Contributor
|
Lies nochmals die CONTRIBUTING.md vollständig und exakt durch und dann prüfe nochmals ob dein PR wirklich diesen Anforderungen entspricht. |
CONTRIBUTING.md requires keys in both ARBs to be alphabetically sorted. The three bitboxDisconnected* / bitboxReconnect keys were appended next to paymentInformationFailed instead of being inserted between bitbox and blockchain. Move them to the correct position so the build-time alphabetiser doesn't fight the source file.
This was referenced May 20, 2026
Contributor
|
Review abgeschlossen. PR ist mergebar — der Status Quo wird in jeder Hinsicht verbessert (typed Exceptions statt Bevor gemerged wird:
Nach dem Merge zwingend nachzuliefern (#468 als Tracking-Issue):
Vorgeschlagene Gruppierung in Folge-PRs A–F siehe Issue #468. Die drei Critical-Punkte (1.1–1.3) sollten in einem ersten kleinen Follow-up-PR direkt nach diesem Merge landen. |
This was referenced May 20, 2026
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
PR #461 introduced WalletLockedException; the surface-drift test should enumerate every typed exception so a missing toString() on any of them is caught at test time. ApiException's toString() is also worth pinning for the same reason — it's the most-leaked exception in the app.
9 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
Adds coverage for the BitboxService observer / re-attach paths fixed in #461, locking the recovery semantics so future refactors can't silently re-introduce the bug class. Driven by the official `installSimulatedBitboxPlatform` helper shipped with `bitbox_flutter` v0.0.7 — the tests exercise the same code paths the real plugin runs, only the device list and transport behaviour are controlled from the test. What's covered: - credentials handed out before pairing start disconnected, then become connected once init() runs (regression guard for the v0.0.6 bug where pre-pairing credentials stayed permanently disconnected) - observer clears credentials in place and calls disconnect() on device-loss (Android USB FD release) - the credentials reference survives a device-loss so the next init() can heal them — verified end-to-end with a vanish → reappear cycle - observer self-cancels after detecting device-loss - observer swallows transport-close errors instead of crashing the periodic - stopConnectionStatusObserver cancels the active timer - a second startConnectionStatusObserver call replaces the first (no double-tick race) `BitboxService` gains an optional `connectionStatusInterval` ctor parameter (default unchanged at 5 s) so the observer cadence can be tightened to 50 ms in tests. Production callers continue to use the no-arg constructor (`lib/setup/di.dart`).
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
…ender assert #461 changed SettingsSeedCubit from constructor-arg SoftwareWallet to async appStore + ensureUnlocked + _loadSeed. Initial state became SettingsSeedState('') with the seed only arriving after _loadSeed emitted. The first render of SettingsSeedView feeds that empty string into MnemonicReadOnlyField, whose constructor asserts seedWords.length == 12 — debug builds crash the screen on open, release builds would hit out-of-bounds in elementAt. Fix: seed the initial state from appStore.wallet when it's already a SoftwareWallet (the common case — software-wallet users land here from settings). _loadSeed still runs to cover the view-wallet case and to preserve the ensureUnlocked() invocation; the redundant emit for already-loaded software wallets is suppressed. Test impact: - initial-state test no longer needs stream.firstWhere — the seed is there synchronously; a Duration.zero microtask drain is enough to observe the ensureUnlocked() call. - toggleShowSeed blocTests no longer need to wait for the async load.
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
#470) ## Summary Adds an eight-test `BitboxService` lifecycle suite that locks in the device-loss / re-attach semantics introduced by #461 and drives the same code paths the real plugin runs at runtime via the official `installSimulatedBitboxPlatform` helper. **Stacks on top of #469** (`chore(deps): bump bitbox_flutter v0.0.5 → v0.0.7`) — that bump is what exposes the simulator helper. Once #469 merges, this branch can be rebased onto `develop` cleanly. ## What's tested | Test | Closes drift on | |---|---| | `getCredentials before init returns disconnected credentials` | base behaviour contract | | `init() promotes credentials handed out before connect (P461 #1)` | pre-fix the wallet stayed disconnected forever after re-pair | | `observer clears credentials and closes transport when device vanishes` | Android USB FD release | | `observer preserves the credentials reference so reconnect can heal them` | the core P461 #1 fix — verified end-to-end with a vanish → reappear cycle | | `observer stops ticking after a single device-loss event` | self-cancel after recovery | | `observer swallows transport-close errors instead of crashing` | Android plugin can throw on close() if FD is gone | | `stopConnectionStatusObserver cancels the active periodic` | start/stop hygiene | | `startConnectionStatusObserver replaces any prior periodic` | no double-tick race on re-arm | ## Why the simulator (not mocktail) The bug class is in a `Timer.periodic` callback that calls into `BitboxManager.devices` and `BitboxManager.disconnect()`. Mocking `BitboxService` itself loses the bug; mocking `BitboxManager` adds drift risk every time the plugin grows. `installSimulatedBitboxPlatform` is the plugin author's official testing seam — it stubs `BitboxUsbPlatform.instance` so the real `BitboxManager` runs against an in-process fake. Adding new scenarios (delays, errors, custom device lists) is a one-liner via `platform.throwOn(...)` / `platform.when(...)`. ## Tiny production-code change `BitboxService` gains an optional ctor parameter: ```dart BitboxService({Duration connectionStatusInterval = const Duration(seconds: 5)}) ``` Default is unchanged (5 s) so `lib/setup/di.dart` keeps the no-arg constructor it has today. Tests use 50 ms so the periodic can be exercised in real time without `fakeAsync` gymnastics. ## Verification - `flutter analyze lib/packages/hardware_wallet/ test/packages/hardware_wallet/` — clean - `flutter test test/packages/hardware_wallet/bitbox_service_test.dart` — 8/8 green - `flutter test test/packages/hardware_wallet/ test/screens/hardware_connect_bitbox/` — all green except one **pre-existing** failure on `develop` (`BitboxCredentials queue continues after a sign throws`) that's unrelated to this PR ## Test plan - [x] `flutter analyze` clean on touched files - [x] New suite green locally (8/8) - [x] Full BitBox test dirs green except the known pre-existing failure - [ ] CI green once #469 is merged + this PR rebased / marked ready
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
## Summary `develop` CI is red after #461 merged: `Run flutter test --coverage` reports `1342 tests passed, 17 failed`. All 17 are direct consequences of the new wiring shipped by #461 — they did not surface locally because Flutter's test runner parallelises per file and `GetIt`'s singleton state leaks across files differently than on CI. This PR repairs the test suite without touching any production code. ## Failure groups & fixes ### 1. `PaymentInfoError` variant count drifted (1 test) `test/packages/service/dfx/models/payment/buy_sell_dtos_test.dart` pins the enum to "four documented variants". #461 added `bitboxDisconnected` (now five) but did not update the pinning test. → Change `hasLength(4)` → `hasLength(5)`, add `bitboxDisconnected` to the asserted set, rename the test description. ### 2. `MockAppStore.ensureUnlocked` not stubbed (9 + 1 tests) #461 added `await appStore.ensureUnlocked()` to `RealUnitSellPaymentInfoService.confirmPayment` and the `SettingsSeedCubit._loadSeed()` path. Tests using `_MockAppStore`/`MockAppStore` without stubbing `ensureUnlocked` got `null` back from mocktail and threw `TypeError: type 'Null' is not a subtype of type 'Future<void>'`. Affected: - `test/packages/service/dfx/real_unit_sell_payment_info_service_validation_test.dart` (9 `confirmPayment validation guard` tests) - `test/screens/settings_seed/settings_seed_page_test.dart` (1 page test) → Add `when(() => appStore.ensureUnlocked()).thenAnswer((_) async {});` to each `setUp`. ### 3. Page tests don't register `DfxKycService` (4 tests) #461 added `DfxKycService` as a cubit dependency for `ensureSignatureFor`. The existing `create_wallet_page_test` and `restore_wallet_page_test` only register `WalletService` in `setUpAll`, so `BlocProvider(create:)` fails with `Bad state: GetIt: Object/factory with type DfxKycService is not registered` when the cubit is first read. → Add `MockDfxKycService` and register it in `setupDependencyInjection` of both files. ### 4. `settings_seed_cubit_test` race on slow CI (1 test) `toggleShowSeed flips showSeed and keeps seed unchanged` relies on `wait: const Duration(milliseconds: 10)` between `act` and `verify`. The async `_loadSeed()` chain (`ensureUnlocked` → seed emit) takes longer than 10 ms on CI, so the toggle races with the post-`_loadSeed` emit and `showSeed` ends up `false` instead of `true`. → Replace the static `wait:` with an explicit `await c.stream.firstWhere((s) => s.seed.isNotEmpty)` inside `act` so the test only toggles after `_loadSeed` has settled. Same fix on the sibling "toggleShowSeed twice" test. ### 5. `bitbox_credentials_test` queue test relabels error (1 test) `queue continues after a sign throws (slot released in finally)` expects the original `Exception('first sign explodes')` to surface. #461's new `_runOrThrowDisconnect` wrapper probes `manager.devices` on a thrown sign and relabels the error as `BitboxNotConnectedException` if the device list is empty. The mock did not stub `devices`, so it defaulted to `null` → `_deviceLost()` returned `true` → the original exception got relabelled. → Add a one-line stub `when(() => manager.devices).thenAnswer((_) async => [_FakeDevice()])` so the disconnect probe returns "still connected" and the original exception survives. ## Verification - Local repro of CI: `flutter test --coverage` matches the CI's failure profile (1342 / 17) on `develop`. - Post-fix expected on CI: 1359 / 0. - No production code touched. ## Why this needs to land fast `develop` CI is currently red, which blocks the auto-tag + release pipelines and propagates to any PR (e.g. #466) that runs against this branch. All seven changes are scoped to `test/` only. ## Test plan - [ ] `flutter analyze` — clean - [ ] `flutter test --coverage` — green (1359/0) - [ ] CI on this PR — green
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
PR #461 introduced WalletLockedException; the surface-drift test should enumerate every typed exception so a missing toString() on any of them is caught at test time. ApiException's toString() is also worth pinning for the same reason — it's the most-leaked exception in the app.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
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)
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
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.
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
…ing (#467) ## Summary Two cheap, plugin-independent test additions that close concrete drift risks identified during the PR #461 review. ### 1. Exception surface guard `BitboxNotConnectedException` had no `toString()` override. Any code path that surfaces the raw exception (snackbar fallback, log line, `error.toString()`) therefore rendered the default `Instance of '...'` Dart string instead of a human-readable message — a real failure mode observed in the BitBox-disconnect flows. - Adds `toString() => 'BitBox is not connected'`. - New `test/packages/service/dfx/exceptions/exception_surface_test.dart` enumerates every typed exception in the app (`BitboxNotConnectedException`, `SigningCancelledException`) and asserts the rendered string contains no `Instance of` and is non-empty. New typed exceptions just need to be appended to the list. ### 2. P1 channel-hash-before-confirm drift test BitBox firmware quirk **P1** (critical, per `DFXswiss/bitbox-testkit`): the pairing channel hash must reach the UI *before* the host calls `confirmPairing()` on the device — otherwise the user has no opportunity to verify the hash against the on-device display. `ConnectBitboxCubit` already does the right thing today (`BitboxCheckHash` is emitted before user-driven `confirmPairing()`), but nothing prevents a future refactor from silently inverting the order. The new test in `connect_bitbox_cubit_test.dart` pins this ordering with a `verifyNever(() => service.confirmPairing())` assertion at the `BitboxCheckHash` state. ## Out of scope (follow-up PRs) - `bitbox_flutter` v0.0.5 → v0.0.7 bump (unlocks the `installSimulatedBitboxPlatform` helper) — separate dependency-bump PR - `BitboxService` lifecycle suite (re-attach on reconnect, observer behavior, USB-FD release) — depends on the bump above ## Test plan - [x] `dart run tool/generate_localization.dart` - [x] `flutter analyze` on touched files — clean - [x] `flutter test test/packages/service/dfx/exceptions/exception_surface_test.dart test/screens/hardware_connect_bitbox/bloc/connect_bitbox_cubit_test.dart` — 11/11 green - [ ] Full `flutter test` in CI
3 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
Closes the last untested branch in BitboxCredentials introduced by #461: mid-sign plugin errors must surface as BitboxNotConnectedException only when the device probe confirms the device is gone, otherwise the original error must propagate untouched. Three new tests: - mid-sign throw with devices empty -> typed disconnect + clearBitbox - mid-sign throw with devices still present -> original error rethrown, credentials remain connected - mid-sign throw with the devices probe itself throwing -> treated as lost (matches the implementation's defensive fallback)
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 20, 2026
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.
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
## Summary Closes the last untested branch in `BitboxCredentials` introduced by #461. The `_runOrThrowDisconnect` helper converts a mid-sign plugin error into a typed `BitboxNotConnectedException` only when the device probe confirms the device is gone — but until now no test exercised any of its three branches. A future refactor could silently invert the logic (e.g. always converting to `BitboxNotConnectedException`, masking real errors) without anything catching it. ## Tests | Test | Pins | |---|---| | mid-sign throw + devices empty | typed disconnect + `clearBitbox()` ran | | mid-sign throw + devices present | original error rethrown, credentials stay connected | | mid-sign throw + devices probe itself throws | treated as lost (defensive fallback) | ## Test plan - [x] `flutter analyze` clean on touched file - [x] Full `bitbox_credentials_test.dart` green deterministically (two consecutive runs) - [ ] CI green once marked ready
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
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)
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 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.
TaprootFreak
pushed a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
…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.
3 tasks
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.
Summary
Closes #461 (BitBox disconnect recovery) and #462 (10 s PIN unlock).
BitBox —
BitboxServicenulled its credentials reference on disconnect, so the wallet's existing credentials stayed cleared after a re-pair and every sign threwBitboxNotConnectedExceptionthat surfaced as rawInstance of '...'in the UI. Now the reference is kept across drops and re-attached frominit(), mid-sign drops are converted into a typed exception, and the Android USB file-descriptor is released on disconnect.PIN unlock — Software-wallet cold start sat at ~10 s: ~5 s for 600 k-iteration PBKDF2 PIN check and ~4 s for eager mnemonic decrypt + BIP32 derivation. Neither step gates anything on the actual dashboard path. The branch caches the public address alongside the encrypted seed and returns a
SoftwareViewWalletat startup; the mnemonic is only decrypted right before a sign operation (sell, KYC EIP-712, recovery-phrase reveal, auth-signature cache miss). PIN hash iteration count drops to 100 k — the hash lives in OS-keystore-backedFlutterSecureStorageand the brute-force lockout cascade is unchanged.Initial signature capture —
CreateWalletCubit,RestoreWalletCubitandConnectBitboxCubitall callDFXAuthService.ensureSignatureFor(account)while the private key is guaranteed reachable, so the first authenticated call after onboarding hits the signature cache instead of triggering a lazy sign.What changed
Bitbox
BitboxService.getCredentialsretains its_credentialsreference across the observer's disconnect detection;init()re-attaches the manager on the cached credentials so existing wallets heal automatically on reconnect.BitboxCredentials._runOrThrowDisconnectconverts low-level errors mid-sign intoBitboxNotConnectedExceptionwhen the device is no longer reachable.<uses-feature android:name="android.hardware.usb.host" android:required="false"/>declared.UI / cubits
showBitboxReconnectSheet(context)helper consolidates the modal +SyncWalletServicesEventsemantics across buy / sell-bitbox / user-data.BitboxNotConnectedExceptionto dedicated reconnect states instead of falling through to a generic snackbar.SellButton) still falls through to the generic snackbar forbitboxDisconnectedandWalletLockedExceptionrouting exists nowhere — both are tracked as Critical follow-ups in Follow-ups from PR #461: BitBox disconnect recovery + lazy unlock #468 (1.1, 1.2) and addressed in the follow-up PR.SellBitboxCubit.confirmSwap/confirmDepositdrops re-enter the existingSellBitboxBitboxRequiredstate instead of dead-ending in a generic error.Unlock architecture
WalletInfo.addresspopulated for software wallets at create/restore.WalletRepository.getWalletInforeturns the row without decrypting;getUnlockedWalletByIdis the explicit decrypt path.WalletService.getWalletByIdreturnsSoftwareViewWalletfor cached-address rows; legacy rows fall back once and the derived address is backfilled into the row so subsequent loads stay on the fast path.unlockCurrentWallet()decrypts on demand.AppStore.ensureUnlocked()promotes view wallets via a DI-wired unlocker callback.WalletService.SecureStorage._pinHashIterations = 100000; older hashes (10 k, 600 k) are accepted exactly once via transparent rehash.Initial signature
DFXAuthService.ensureSignatureFor(account)factored out ofgetSignMessage/getSignaturesoConnectBitboxCubit/CreateWalletCubit/RestoreWalletCubitcan pre-warm the signature cache.i18n
bitboxDisconnectedTitle,bitboxDisconnectedDescription,bitboxReconnect,userDataLoadFailed(de + en). Requiresdart run tool/generate_localization.dartbefore build.Tests
wallet_service_test,create_wallet_cubit_test,restore_wallet_cubit_test,settings_seed_cubit_test,connect_bitbox_cubit_test,settings_user_datatests for the new constructors / repository methods.appStore.ensureUnlocked()in the service-level tests (dfx_auth_service_test,real_unit_registration_service_*_test,real_unit_sell_payment_info_service_confirm_test) that exercise the new path.Follow-ups (issue #468)
This PR landed with several deliberate scope cuts. The most important to land next are the three Critical items: Sell-button
bitboxDisconnectedrouting,WalletLockedExceptionrouting (or assert), and a non-silentAppStore.ensureUnlocked()fallback. Full list and grouping in #468.Test plan