Split PR 578: BitBox connection lifecycle#607
Open
joshuakrueger-dfx wants to merge 15 commits into
Open
Conversation
5 tasks
joshuakrueger-dfx
force-pushed
the
split/pr578-02-bitbox-lifecycle
branch
from
ae7a9d4 to
02d3b37
Compare
Failing-on-purpose pin (commit-test-before-implementation per Mandate §3 Operating Loop). Asserts the Initiative I contract before the refactor lands: Stream<BitboxConnectionStatus> as the sole source of truth, replay- last semantics, init-concurrency property, state-machine traversal property, dispose()/clear()/signalDeviceLost() lifecycle. Test fails today with compile-time errors on missing API (clear, dispose, status, currentStatus, signalDeviceLost) — exactly the surface ADR 0001 promises to add. The next commit refactors BitboxService and turns this green.
…atus>
BitboxService now owns a broadcast Stream<BitboxConnectionStatus> with
replay-last-value semantics as the sole source of truth for the connect
state, per ADR 0001 (docs/adr/0001-bitbox-connection-lifecycle.md). The
`_isConnected` boolean is removed; consumers subscribe to `status` or
read `currentStatus` instead. `init()` is guarded by `_pendingInit` so
concurrent callers funnel onto a single bitboxManager.connect(). New
lifecycle methods land here as a coherent unit:
- `init(BitboxDevice) -> Future<BitboxConnectionStatus>` — guarded by
`_pendingInit`; emits `Connecting -> Paired` on success or
`Connecting -> Disconnected` on failure.
- `clear() -> Future<void>` — detaches credentials, tears down the
observer, walks `Disconnecting -> Disconnected`, empties the
credentials map. Idempotent.
- `signalDeviceLost(LostReason)` — emits `Lost(reason)` from Paired /
InUse only; tears down the observer; idempotent from non-live states.
- `dispose()` — emits a final `Disconnected`, closes the controller,
rejects subsequent `init()` with StateError. Idempotent.
The replay-last-value getter installs the upstream subscription BEFORE
delivering the cached value so any `_emit` racing with `service.status.listen`
lands in the broadcast pending queue rather than being silently dropped
(an async* `yield initial; yield* upstream;` would have surfaced this as
a lost Lost emission between observe + signalDeviceLost in fakeAsync).
BitboxCredentials gains an optional `_onSignQueueTimeout` callback so the
sign-queue timeout can later propagate Lost(signQueueTimeout) to the
service via a uni-directional closure (call-site lands in the propagation
commit so the wiring change is reviewable on its own).
The existing service test suite is migrated to the new return-type
contract:
- `init().catchError(...)` returns a typed BitboxConnectionStatus.
- F-007 concurrent-init pin tightens from `<=2 opens` to exactly `1`
(post-`_pendingInit` invariant).
- F-033 dispose pin flips from absence-check to a real lifecycle assertion
(final Disconnected + StateError on post-dispose init).
Extends the lifecycle suite with multi-subscriber + cancel-leak guards
and three observable post-conditions of clear():
- two simultaneous subscribers observe identical traversals (broadcast
contract);
- cancelled subscriptions stop accruing events (close-leak guard for
cubit close()-paths);
- clear() drops cached credentials (re-init starts fresh);
- clear() nulls the BitboxManager on every credentials in the map;
- currentStatus is Disconnected after clear() completes.
These pin the post-conditions ADR 0001 enumerates so a refactor of the
Stream wiring (e.g. dropping the replay-last semantics in favour of
plain broadcast) trips immediately.
On a TimeoutException inside `_synchronizeBoundedSign`, BitboxCredentials now also calls the service-wired `_onSignQueueTimeout` closure so the service-level Stream transitions to `Lost(signQueueTimeout)`. Before this change the timeout cleared local credentials but left BitboxService still reporting Paired; the observer kept polling, and the consuming cubit had no way to learn the device was lost without polling currentStatus. The closure is wired one-way (service → credentials) in `BitboxService.getCredentials` so credentials never reach back through a singleton getter — closes F-009 without introducing a circular import or a service-level static.
Five new tests cover the wire from sign-queue timeout to the
service-level signalDeviceLost callback:
- hung sign fires the callback exactly once AND clears credentials;
- successful sign does NOT invoke the callback (negative pin);
- native-error path (non-timeout exception) does NOT invoke the
callback — only the timeout flips the service to Lost;
- omitting the callback parameter keeps the timeout path safe (no NPE);
- property over K in [1..6] signs: after a mid-batch timeout, callback
fires exactly once and every subsequent sign fails fast at the
snapshot null-check without ever reaching the native mock.
Together with the lifecycle suite these close F-009: the sign-queue
timeout can no longer silently desync BitboxService from the local
credentials state.
`_onDeleteCurrentWallet` now invokes `BitboxService.clear()` in addition to the existing `stopConnectionStatusObserver` call. Per ADR 0001 the clear walks the lifecycle Stream to Disconnected, empties the credentials map, and disconnects the BitboxManager — closing F-024 so a subsequent "restore different seed, re-pair the same device" can no longer silently re-attach the old derivation path against the device's new static pubkey. Two new home_bloc tests pin the call (with and without an existing wallet).
ConnectBitboxCubit now subscribes to the service-owned lifecycle Stream in its constructor. A service-emitted Lost (sign-queue timeout, observer device-vanish, future static-pubkey mismatch) routes the cubit back to BitboxNotConnected and re-arms the scan timer without forcing the existing try/catch blocks to also poll currentStatus. The subscription is cancelled in `close()` so the broadcast Stream stops holding a reference to the closed cubit — matches the subscription-leak guard in bitbox_service_lifecycle_test.dart. `_pendingInit` is retyped to `Future<BitboxConnectionStatus>` and the confirmPairing path checks for `Paired`/`InUse` instead of a Boolean. The previous `bool isConnected`-like flag was already absent from the cubit (only the service held it); this change finishes the conversion by making the cubit a consumer of the stream rather than a polling caller. Five new tests cover: - construction registers a single status subscription; - mid-flow Lost bounces an in-progress pairing back to NotConnected; - non-Lost transitions on the stream do NOT perturb the cubit state; - close() cancels the subscription (no listener leak); - Lost emitted after close() is ignored without throwing.
End-to-end Tier-1 suite for ADR 0001's state machine. No mocks above the
service surface: real BitboxService, real BitboxCredentials, real broadcast
Stream<BitboxConnectionStatus>. The simulated platform from
bitbox_flutter is the only seam.
Covers:
- happy path: init → pair → sign → clear (full lifecycle round-trip);
- disconnect-mid-sign: observer flips service to Lost(deviceUnreachable);
- reconnect after Lost: clear() resets the map and a fresh init() heals
the new credentials;
- sign-queue timeout: signalDeviceLost(signQueueTimeout) surfaces Lost
on the stream before BitboxNotConnectedException reaches the caller;
- 3-cycle stress (pair → sign → clear repeated): no observer-timer leak,
sign count matches cycle count exactly;
- signalDeviceLost from Disconnected is a no-op (no spurious Lost);
- sign on cleared service fast-fails with BitboxNotConnectedException;
- dispose() closes the stream and rejects subsequent init().
PR RealUnitCH#468's 17-item tracking issue gets the Tier-1 conformance pin it
needed.
…ls.dart
Drives the remaining surface to 100% line coverage on both files:
- integration lifecycle: startScan delegation, connect-throw catch arm
(Connecting → Disconnected), getChannelHash + confirmPairing
delegation, !didVerify branch on confirmPairing, end-to-end pin of
`_onCredentialsSignQueueTimeout` (the closure wired in
`BitboxService.getCredentials` actually routes a hung credentials
sign through `signalDeviceLost(signQueueTimeout)` — fakeAsync drives
the 5-minute queue-bound to virtual time);
- credentials defensive pins: the pre-existing `address` getter,
`signToEcSignature` and `signPersonalMessageToUint8List`
UnimplementedError throws, and the >32-bit chainId truncation loop in
`signToSignature`.
bitbox.dart: 91.7 → 100% line coverage
bitbox_credentials.dart: 90.5 → 100% line coverage
TaprootFreak
force-pushed
the
split/pr578-02-bitbox-lifecycle
branch
from
02d3b37 to
f3ffa0a
Compare
TaprootFreak
added
the
tier3:full
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.
Draft split from #578 after TaprootFreak review.
Scope:
Validation: