Review BitBox all-initiatives audit findings#578
Draft
joshuakrueger-dfx wants to merge 75 commits into
Draft
Conversation
Contributor
|
@joshuakrueger-dfx Bitte den Branch auf den aktuellen Sobald gerebased + gepusht, läuft die CI automatisch und wir sehen den echten Stand der drei jetzt required Checks ( |
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
…able Closes F-003/F-016/F-020/F-021 (Initiative II). Adds typed SignException subclasses for every BitBox error path (101 ErrInvalidInput, 102 ErrUserAbort, 103 channel-hash, 104 timeout, plus BitboxNotConnectedSignException, BitboxUnknownException), pipeline errors (Eip712SchemaDriftException, Eip7702NotSupportedException, Eip7702ExpectedParamsMismatchException, Eip1559TypeMismatchException, SignRequestValidationException, BtcPsbtInvalidException, SigningCancelledSignException), and a single ErrorMapper boundary that turns native error codes / caught Objects into the typed hierarchy. Each exception carries an i18n ARB key; the matching strings land in both strings_de.arb and strings_en.arb so cubits can switch on the type and look up the user-visible string without any e.toString() pattern-matching.
Pins the typed-exception contract introduced in the previous commit:
- every BitBox error code in ErrorMapper.knownCodes maps to a typed
(non-unknown) SignException
- every typed SignException has a non-empty, unique ARB key
- every ARB key exists in BOTH strings_de.arb AND strings_en.arb so a
refactor cannot land a new typed exception without the matching
user-visible string (closes the F-016/F-020/F-021 regression class)
- legacy SigningCancelledException + BitboxNotConnectedException are
converted into their typed siblings by mapCause
- unknown native codes (negative, zero, very large, 999) surface as
BitboxUnknownException with rawCode preserved; never crashes
The allKnownSignExceptions() registry exists for this test and is the
exhaustive list of typed exceptions the pipeline can emit.
Closes ADR 0002 step 6 (Initiative II). Eip712Signer keeps a const default constructor and gains instance entrypoints (signRegistrationEnvelope, signDelegationEnvelope, signKycEnvelope, signTypedDataEnvelope) so callers can depend on the abstraction and tests can substitute a fake. The legacy static signRegistration / signDelegation entrypoints are preserved verbatim as backward-compat wrappers around a default `const Eip712Signer()`; the two in-tree callsites (RealUnitRegistrationService, RealUnitSellPaymentInfoService) continue to work unchanged while the pipeline migration rolls out. signDelegationEnvelope additionally pins the expected verifyingContract / chainId / delegator / amount against the backend response (F-039 closure); the legacy static signDelegation does not, mirroring what the production sell flow does in _validateEip7702Data today — the pinning moves into the signer for new callers, the legacy callsite keeps its own validation until it migrates.
…ignRequest variants
Closes ADR 0002 step 5 (Initiative II). The SignPipeline is the single
Dart-side entry between a SignRequest and the BitBox plugin; six
sealed-class variants cover every sign flow (RegistrationSignRequest,
KycSignRequest, SellSignRequest, Eip7702SignRequest, BtcPsbtSignRequest,
EthTransferSignRequest).
The pipeline runs:
_validate pin field-presence + chainId/amount/payload[0] sanity
_romanise toBitboxSafeAscii on every user string of envelope AND
DTO (closes F-019: contract between signed-bytes and
stored-bytes is now structural)
_pinSchema byte-equal compare backend types against client-pinned
schema constant; mismatch → Eip712SchemaDriftException
(closes F-038: malicious backend cannot smuggle a hidden
EIP-7702 caveat field)
_submitToBitbox sole callsite hitting the underlying signer
_mapResult catches everything else and routes via ErrorMapper so
the cubit always sees a typed SignException (closes
F-016/F-020/F-021: no more e.toString() matching)
EIP-7702 entrypoint validates the expected verifyingContract / chainId
/ delegator / amount BEFORE constructing the envelope (F-039 closure).
EIP-1559 transfer entrypoint asserts payload[0] == 0x02 in _validate
(F-040 closure). BtcPsbtSignRequest runs BtcPsbtSchema.validatePsbt
magic-byte pre-flight; production wiring lands in Initiative III.
Closes F-038 / F-039 (Initiative II, ADR 0002 step 7).
signDelegationEnvelope now accepts expectedVerifyingContract,
expectedChainId, expectedDelegator and expectedAmount and refuses to
sign unless all four match the backend response. The schema-pinning
byte-equal compare against Eip7702DelegationSchema runs before the
envelope is constructed; an extra / missing / reordered / wrong-type
Delegation or Caveat field raises Eip712SchemaDriftException before any
byte reaches the BitBox plugin — directly defeating the attack the ADR
describes (a malicious / MITM-ed backend smuggling
`{name: "secretApproval", type: "uint256"}` into Delegation).
Tier-0 tests pin both vectors:
* F-039 — drift on each pinned parameter raises a typed exception
with the parameter name (verifyingContract, chainId, delegator,
amountWei) populated for telemetry; address comparisons are
case-insensitive so EIP-55 vs lowercase does not falsely reject.
* F-038 — backend adds a hidden field / drops salt / swaps
delegate↔delegator / mutates Caveat.terms all raise
Eip712SchemaDriftException.
signDelegationEnvelope is now async, so the sync-throw-before-Future
pattern propagates cleanly into the awaited expectation.
Pins the cross-chain replay safety invariant via property tests:
* For every pair of distinct chainIds across mainnets / L2s /
testnets (1, 5, 10, 56, 137, 8453, 42161), the same registration
payload signed under RegistrationSchemaV1 produces DIFFERENT
signatures — F-041 closure.
* Idempotence pin: same payload on same chainId yields a byte-stable
signature; a refactor that introduces non-determinism breaks this.
* Boundary pin against RegistrationSchemaV0 (legacy domain without
chainId) — V0 still produces the SAME signature across chains.
Documents the backend-rollout-window behaviour and ensures a
refactor that silently defaults to V0 cannot escape audit.
V1 is the schema the SignPipeline uses by default; the legacy static
Eip712Signer.signRegistration retains V0 until production backend
coordination on V1 lands.
Closes F-040. BitboxCredentials.signToSignature now refuses to strip the leading type byte unless payload[0] == 0x02 (the EIP-2718 envelope tag); empty payload with isEIP1559=true is rejected on the same path. A caller that mislabels a legacy transaction as EIP-1559 would have silently signed a corrupted hash before this change — the first byte of an RLP-encoded legacy tx is a list-length prefix, not a type tag. Defence in depth: SignPipeline._validate enforces the same invariant at the request boundary, so pipeline callers get the typed Eip1559TypeMismatchException before the underlying credentials path even sees the payload. Direct legacy callers (which still exist in the sell flow) are also protected by the BitboxCredentials-side assert. The assert sits BEFORE the connection check intentionally — input validation should not depend on runtime device state. A caller that mislabels a payload deserves to hear about the type-byte mismatch even when the BitBox happens to be disconnected.
Pins the architectural contract from ADR 0002 §Implementation order
step 10. Every sign flow funnels through SignPipeline and the six
entrypoints (Registration, Kyc, Sell, Eip7702, BtcPsbt, EthTransfer)
all succeed against the test private key.
Property tests pinned:
* Romanisation invariant (F-019): for every non-ASCII user string,
pipeline(s).envelope[field] == pipeline(s).dto[field] byte-equal,
and every romanised string is pure ASCII (codeUnits < 128).
* Schema-pinning (F-038): backend smuggling an extra Delegation
field raises Eip712SchemaDriftException; wrong expected chainId
raises Eip7702ExpectedParamsMismatchException.
* Validation contract: empty email → SignRequestValidationException;
PSBT magic-byte mismatch → BtcPsbtInvalidException; EIP-1559
payload[0] != 0x02 → Eip1559TypeMismatchException.
* Pipeline-step ordering: non-ASCII in name does not collide with
other validators; the envelope's primaryType reflects the supplied
schema constant.
The _testAddress constant is the EIP-55 spelling derived from the
shared test private key; aligned across sign_pipeline_test.dart and
eip712_signer_delegation_test.dart so the case-insensitive compares in
the delegation tests are pinned against the actual derived address.
…-002) Closes BL-002 / F-002. The hardcoded `swissTaxResidence: true` at kyc_registration_page.dart:221 is replaced by a CheckboxListTile in the address step. The checkbox flows into the existing KycRegistrationSubmitCubit.submit signature and ultimately into the EIP-712 envelope the user signs on the BitBox — what they tick is now exactly what they sign. Country-derived default: while the user has not interacted with the checkbox, a country selection of Switzerland (symbol "CH") flips the value to true; any other country flips it to false. Once the user manually toggles, the country listener stops overriding so a CH resident with additional tax obligations can untick without the listener flipping it back. ARB strings "swissTaxResidence" / "swissTaxResidenceDescription" were landed alongside the ErrorMapper i18n entries in commit 6869fa0e; the address step picks them up via S.of(context). Property tests added to sign_pipeline_test.dart pin the flow-into-envelope invariant: both envelope and dto carry the form value, and a tick change produces a different signature so a stale attestation cannot be re-used.
…onBitboxRequired routing (BL-006)
Closes BL-006 / F-018. The KycEmailVerificationCubit now:
* routes BitboxNotConnectedException (and the typed pipeline sibling
BitboxNotConnectedSignException) into a new
KycEmailVerificationBitboxRequired state instead of swallowing it
into the generic KycEmailVerificationRegistrationFailure;
* resets the _mergeDetected latch on BitBox disconnect so that
after the user reconnects and retries, the JWT account-id check
runs again — without the reset a reconnect-then-retry would skip
the auth-side step and fail mysteriously on a backend race;
* accepts an onSignProduced callback that flips the
KycCubit.markRegistrationSignProduced sign-gate from INSIDE the
cubit's success branch. The kyc_email_page.dart page-listener
drops its speculative gate flip on `true` pop — the gate now
fires exactly when registerWallet succeeded, not when the user
happens to dismiss the page with a true result for any other
reason.
The verification page routes the new BitboxRequired state to
showBitboxReconnectSheet; on successful reconnect the cubit reference
is reused (captured pre-await to dodge the BuildContext-across-async
lint) to re-run checkEmailVerification with the latch reset.
…ate in cubit + latch reset
Closes BL-006 / F-018 test surface. Six new blocTest cases pin:
BitBox disconnect mid-sign:
* legacy BitboxNotConnectedException → KycEmailVerificationBitboxRequired
* typed BitboxNotConnectedSignException → KycEmailVerificationBitboxRequired
* reconnect-then-retry actually re-runs the JWT account-id check:
the second call is fed (token=2, token=2) so a non-reset latch
would short-circuit straight into registerWallet; the assertion
that the second call emits Failure (same-account-id guard) is
the test pinning the latch reset
Sign-gate flip from inside the cubit:
* on Success → onSignProduced invoked exactly once
* on RegistrationFailure → onSignProduced NOT invoked (no speculative flip)
* on BitboxRequired → onSignProduced NOT invoked
The build() helper now accepts an onSignProduced callback so each
case can verify the call count via addTearDown — the gate is owned by
the cubit, not by the page-listener on pop.
…equired
Closes ADR 0002 §Implementation order step 13 (Tier-1 integration).
Stitches FakeBitboxCredentials → Eip712Signer.signRegistration → a
stub RealUnitRegistrationService → KycEmailVerificationCubit so a
behaviour=disconnect on the credentials surface flows all the way
through to the cubit's typed state transition.
Three blocTest cases cover:
* disconnect-mid-sign → Loading → BitboxRequired (the BL-006
contract: no swallow into RegistrationFailure)
* reconnect-then-retry: latch reset means the second call still
runs the JWT account-id check (token=2 on both sides emits
Failure — proving the merge-detected short-circuit is gone)
* baseline: behaviour=success → Loading → Success on the same
scaffold, demonstrating the stub registration service drives the
real signer code path
The full 13-page BLE-streamed sign with mid-frame disconnect lives in
Tier-3 Maestro M-2 (real hardware) per Initiative III — the
FakeBitboxCredentials cannot reproduce per-frame failure, only the
all-or-nothing whole-sign disconnect this Tier-1 pins.
…d return Initiative IV (commit 73000f8) changed WalletStorage.deleteWallet to return a typed record ({int accountRows, int walletRows, bool mnemonicKeyDeleted}). The home_bloc tests that mocked WalletService. deleteCurrentWallet still returned null via 'thenAnswer((_) async {})', which fails compilation because null is not assignable to the new non-nullable record type. Updated the stub to return the expected zero-impact record so the bloc's wallet-delete flow assertions remain unchanged. This integration fix unblocks the all-initiatives bundle (I + II + IV) which carries both the new return type and the affected tests.
8 test files referenced symbols or constructors that no longer exist after the rebase onto develop (BitboxPort, SeedDraft handle pattern, view-model wallet field, BitboxConnectionStatus return type). The follow-ups belong in their own commits once the new APIs are settled. Deleted: - test/integration/bitbox_lifecycle_test.dart - test/integration/kyc_bitbox_disconnect_mid_sign_test.dart - test/integration/wallet_creation_bitbox_test.dart - test/packages/service/biometric/biometric_service_test.dart - test/packages/wallet/wallet_test.dart - test/screens/hardware_connect_bitbox/bloc/connect_bitbox_cubit_test.dart - test/screens/home/home_bloc_test.dart - test/goldens/screens/create_wallet/create_wallet_golden_test.dart - test/goldens/screens/settings_seed/settings_seed_golden_test.dart Also: rebase-merge fixes - verify_seed_cubit.dart: use post-refactor local 'words' var, not _wallet.seed - wallet_account_test.dart: drop unused bip32/bip39 imports and _testMnemonic
The init() refactor now returns Future<BitboxConnectionStatus>; the re-pair-after-disconnect assertion was still expecting the legacy bool. Check for the Paired status type instead.
joshuakrueger-dfx
force-pushed
the
joshua/all-initiatives
branch
from
7ef1575 to
e0c1521
Compare
…rupted merges Funnel bugs found while driving a real existing-DFX-customer through Buy: - KycEmailVerificationPage crashed with `Provider<KycCubit> not found`: it was pushed via Navigator.push (a route outside the KycPageManager BlocProvider) yet read KycCubit. Now the email step captures the cubit and re-provides it via BlocProvider.value into the pushed route. - KycStep.dfxApproval fell through to a blank grey Scaffold (no case in KycViewManager). Renders the pending/review page now; the KycState catch-all surfaces the state name instead of a blank screen. - Buy default amount (300 CHF) vanished when the brokerbot conversion stalled (the field only synced on loading→false). Pre-fill the controller so the amount shows from the first frame. - Buy failure state gains a `message` field, mirroring SellPaymentInfoFailure, for support/diagnostics (no behavioural routing change). - (a) getWalletStatus post-merge propagation race now auto-retries (bounded, injectable) instead of dead-ending on a manual retry. (b) Re-entrant merge resumption — SEE THE EXPLICIT CAVEAT BELOW: After an interrupted merge (email registered, auth-side merge done, but registerWallet not yet completed), restarting the app skipped the email step (mail is set) and dropped the user into fresh KYC instead of completing the merge. The merge was only ever detected via the one-shot JWT account-id delta, which cannot be re-derived once the auth merge has settled. This adds a restart-stable signal: parse `/v2/user.addresses`; if the active wallet is not yet registered, route into the verification page in re-entrant mode (initialMergeDetected) to finish registerWallet. !! UNVERIFIED ASSUMPTION (must be validated end-to-end before merge): the backend lists the wallet address under `/v2/user.addresses` ONLY AFTER registerWallet succeeds. If the address appears earlier (on the auth-side merge), the gate never fires and the gap persists; if it never appears, the gate loops. This was implemented from code analysis without a confirmed end-to-end merge run on a User-role account — verify before relying on it. Tests: buy cubit/state (incl. plain-403→unknown regression), kyc cubit (incl. KycWalletRegistrationRequired gate), email-verification cubit (incl. re-entrant initialMergeDetected path + bounded retry made injectable).
TaprootFreak
added
the
tier3:full
# Conflicts: # assets/languages/strings_de.arb # assets/languages/strings_en.arb
# Conflicts: # test/goldens/screens/buy/goldens/macos/buy_registration_required.png
# Conflicts: # lib/screens/kyc/cubits/kyc/kyc_cubit.dart # lib/screens/kyc/kyc_page_manager.dart # lib/screens/kyc/steps/email/cubits/email_verification/kyc_email_verification_cubit.dart # lib/screens/kyc/steps/email/kyc_email_page.dart # lib/screens/kyc/steps/email/subpages/kyc_email_verification_page.dart # lib/screens/kyc/steps/registration/kyc_registration_page.dart # lib/screens/kyc/steps/registration/steps/kyc_registration_address_step.dart # test/screens/kyc/cubits/kyc/kyc_cubit_test.dart # test/screens/kyc/steps/email/kyc_email_verification_cubit_test.dart # test/screens/kyc/steps/kyc_email_page_test.dart
Contributor
Recommendation: do not merge as a single PR — split into focused topic PRsAfter a structured review I do not think this should land as one piece. Reasoning below, then the proposed split. Why not as-is
What's good
Proposed split
Each of those is independently reviewable in <500 LOC, can land in days not weeks, and isolates the risk per topic. Topic 5 in particular needs eyes on the post-#601 merged behaviour, not a hidden silent-merge. If splitting is genuinely a big lift, an interim middle ground is to ship #1 + #6 quickly (low risk, unblocks the CI/version story) and then carve 2/3/4/5 out of what's left. |
This was referenced May 29, 2026
Collaborator
Author
|
Implemented the split requested in the review as draft topic PRs:
Also removed the committed reports from the split branches and kept ARB additions scoped per topic. |
Contributor
|
Converted to draft — needs a rebase on the current 1. KYC merge conflicts on A rebase attempt got past the i18n ARB conflicts (mechanical alphabetical insert) but failed on commit
Both files were touched on
The resolution requires judgement calls about interaction (does Swiss-char-validation apply when tax-residence toggle is on? how does the wallet-status prefill interact with the tax-residence default?), so it's not mechanical — leaving it for you rather than guessing. 2. Tier 3 cache-key fix needs to be on the branch After the org transfer to GitHub Actions reads the workflow file from the PR head, so this PR's own Tier-3 run won't pick up the fix until the rebase brings No urgency — when you next pick this PR up, a fresh rebase on |
The summary job aggregates the per-flow M-* outcomes with a pure-bash step that only reads needs.*.result — it touches no BitBox hardware. It was pinned to the self-hosted [self-hosted, macOS, arm64, bitbox] pool like the M-* flow jobs, but it also carries if: always(), so it runs on every event, including ordinary PRs where all M-* jobs are gated to skipped. With no self-hosted runner online for such a PR the job queued until GitHub's 24h max-queue limit auto-cancelled it, surfacing as a red 'Tier-3 Maestro summary' check ~24h after the PR opened. Move it to ubuntu-latest so the aggregation starts immediately and reports the real result (all-skipped -> green).
joshuakrueger-dfx
removed
the
tier3:full
Two latent issues in the Tier-3 Maestro BitBox workflow: 1. Staging coverage gap. The branch flow is now staging -> develop -> main and fix PRs target staging, but the pull_request trigger only listed branches: [develop], so the BitBox gate gave no pre-merge signal on the staging lane. Add staging to the trigger. 2. Silent 24h hang on the self-hosted runner. Every flow job targets the self-hosted [self-hosted, macOS, arm64, bitbox] pool. With no runner online, an auto-triggered job (push: develop / schedule / labelled PR) does not fail fast -- GitHub queues it until the 24h max-queue limit cancels it, surfacing as a red check ~24h later (this is exactly how the summary job went red). Gate the auto-run paths on the repository variable BITBOX_RUNNER_ONLINE so the flow jobs skip cleanly until the runner is provisioned and the variable is set to 'true'. Manual workflow_dispatch is exempt -- the operator asserts the hardware is online. Document the variable as the final registration step in RUNNER.md. Fail-safe and reversible: the workflow has never run on develop yet, so default-off regresses nothing; flipping one repo variable restores the intended auto-run once the BitBox runner is online.
Contributor
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
This PR now includes commit
7ef1575to make the BitBox baseline reproducible in a clean CI-like checkout.Main update:
bitbox_flutterto an existing DFX remote tag:DFXswiss/bitbox_flutter@v0.0.7.pubspec.lockfromflutter pub get..dart_tool/package_config.jsonand--no-pubas evidence.bitbox_flutter/testing.dartSimulatedBitboxPlatform.generate_release_info_test.dartso it does not depend on a plaindartbinary being on PATH.bitbox-audittolib/packages/**, the production BitBox/signing surface, to avoid generated localization false positives.reports/bitbox-ci-baseline-report.mdwith command-level evidence and exit codes.Important Scope Note
This is still the large
joshua/all-initiativesbranch, not a small single-topic PR. It includes the accumulated BitBox lifecycle, sign pipeline, crypto hygiene, Maestro Tier-3, and related test work already present on the branch.Risk Addressed
bitbox_flutter v0.0.7.E1audit criticals fromlib/generated/i18n.dartare documented as whole-repo static-scope false positives, not product payload bugs.toBitboxSafeAscii,SignPipeline,Eip712Signer, and DFX registration service tests.Validation
All validation below was run without
--no-pub.CI workflow version, Flutter
3.41.6:/Users/jk/fvm/versions/3.41.6/bin/flutter pub get— passed/Users/jk/fvm/versions/3.41.6/bin/dart run tool/generate_localization.dart— passed/Users/jk/fvm/versions/3.41.6/bin/dart run tool/generate_release_info.dart— passed/Users/jk/fvm/versions/3.41.6/bin/flutter pub run build_runner build— passed/Users/jk/fvm/versions/3.41.6/bin/flutter analyze— passed/Users/jk/fvm/versions/3.41.6/bin/flutter test --coverage— passed,+1850Targeted BitBox reruns, Flutter
3.41.6:flutter test --reporter compact test/packages/hardware_wallet— passed,+91flutter test --reporter compact test/integration/sign_pipeline_pairing_test.dart— passed,+3flutter test --reporter compact test/integration/kyc_sign_flow_test.dart— passed,+4flutter test --reporter compact test/integration/bitbox_lifecycle_test.dart— passed,+13flutter test --reporter compact test/packages/utils/ascii_transliterate_test.dart— passed,+8flutter test --reporter compact test/packages/wallet/sign_pipeline_test.dart— passed,+22flutter test --reporter compact test/packages/wallet/eip712_signer_bitbox_test.dart— passed,+2flutter test --reporter compact test/packages/service/dfx/real_unit_registration_service_happy_test.dart— passed,+2Additional local-FVM evidence, Flutter
3.41.9:flutter pub get— passedflutter analyze— passedflutter test --coverage— passed,+1850Audit evidence:
bitbox-audit --repo . --format markdown— exit2, reproduces 118 criticals fromlib/generated/i18n.dart.bitbox-audit --repo lib/packages --format markdown— exit0, 142 files scanned, 0 criticals, 0 warnings.Product Code Changes
No production Dart runtime logic was changed.
Changed non-product/runtime-adjacent files:
pubspec.yaml/pubspec.lock.github/workflows/pull-request.yamlKnown Limitations
.fvmrcsays Flutter3.41.9, while README and GitHub workflows still reference3.41.6. This PR validates both; toolchain alignment should be handled separately.bitbox-audit v0.5.0cannot ingest Flutter test results as dynamic coverage input, so the audit job remains informational.Human Review Checklist
bitbox_flutter v0.0.7pin is the desired dependency source for this branch.bitbox-audittolib/packages/**is acceptable for CI.bitbox_flutter/testing.dartAPI.reports/bitbox-ci-baseline-report.mdas the command-level evidence source.