Skip to content

test: add reusable BitBox testkit#17

Merged
TaprootFreak merged 5 commits into
DFXswiss:developfrom
joshuakrueger-dfx:joshua/generic-bitbox-testkit
May 18, 2026
Merged

test: add reusable BitBox testkit#17
TaprootFreak merged 5 commits into
DFXswiss:developfrom
joshuakrueger-dfx:joshua/generic-bitbox-testkit

Conversation

@joshuakrueger-dfx

Copy link
Copy Markdown

Summary

  • add generic Go fake-device coverage for BitBox pairing, capabilities, BTC/ETH signing, device errors, and panic recovery
  • add standalone Flutter testkit via package:bitbox_flutter/testing.dart for future apps to simulate BitBox flows without hardware
  • document the fast PR gate, official BitBox simulator option, and regression coverage

Verification

  • flutter test
  • flutter analyze --no-fatal-infos (existing avoid_print infos only)
  • go test ./api
  • git diff --check origin/develop..HEAD

@TaprootFreak TaprootFreak force-pushed the joshua/generic-bitbox-testkit branch from 1463b2d to caf6496 Compare May 16, 2026 21:00
@TaprootFreak

Copy link
Copy Markdown

Review (Maintainer-Edit Rebase + Code-Review)

Branch rebased onto develop, drei Konflikte in pull-request.yaml aufgelöst, drei Inhalts-PRs lokal verifiziert (Go race-tests, flutter test 12/12, dart format).

✅ Stark

  • Layered test pyramid (TESTING.md): "official simulator → U2FHID/BLE contract → native API fake → Flutter API fake" — saubere Ebenen-Trennung.
  • fakeBitboxDevice + Interface-Refactor: var bitbox *firmware.Devicevar bitbox bitboxDevice in go/api/bitbox_device.go macht die gomobile-exportierten Funktionen testbar ohne Hardware. *firmware.Device implementiert das Interface durch Strukturanpassung — sauberes Dependency-Inversion-Pattern.
  • ios_bluetooth_regression_test.go: Cross-language Regression-Guard, der Bluetooth.swift aus dem Go-Test heraus per String-Match überprüft. Smart.
  • SimulatedBitboxPlatform: Defensive _copy() aller Uint8List, requireOpen Guard, behaviorale per-Method Errors/Delays, calls-Log für Assertions. Vollständig und ergonomisch.

⚠️ Concerns

1. Versteckte Architektur-Änderung in einem test:-PR (KRITISCH)

PR-Titel und Beschreibung sprechen nur von Test-Infrastruktur, aber der PR entfernt komplett die BLE-Packet-Dedup-Logik aus ios/Classes/Bluetooth.swift (~25 produktive Zeilen, plus Löschung von dedup_logic_test.go mit 140 Zeilen). Das ist eine direkte Rückgängigmachung von PR #15 (fix(ble): dedup single-frame init retransmits + panic recovery for GetDevice, gemerged am 2026-05-15 — also vorgestern).

Kommentaränderungen in u2fhid_dedup_test.go bestätigen, dass das eine bewusste Architektur-Umkehr ist:

  • Vorher: "the iOS BLE per-message dedup is the only line of defence against BLE-layer retransmits"
  • Jetzt: "the BLE bridge must not leave stale frames in the read stream"

Aber im PR-Body steht nichts zum Warum — was war das Problem mit der Dedup, was ist die neue Strategie, wodurch ist das Risiko abgedeckt? Bitte entweder:

Das ist die wichtigste Sache zum Klären vor dem Merge — Wallet-Verlust-Risiko zwischen Hardware-Roundtrips muss vor dem Mergen verstanden sein.

2. API-Surface dreimal dupliziert (lib/testing/bitbox_testkit.dart)

SimulatedBitboxPlatform() Konstruktor, SimulatedBitboxPlatform.install() Static-Method und das Top-Level installSimulatedBitboxPlatform() deklarieren jeweils dieselben ~20 Parameter. Jede zukünftige Erweiterung muss in drei Stellen synchron passieren. Vorschlag: nur eine Entry-Funktion exportieren (z.B. installSimulatedBitboxPlatform), die intern den Konstruktor aufruft; den Static-Helper streichen.

3. TESTING.md "Fast PR gate" listet dart format nicht

Unsere pull-request.yaml enforced dart format --set-exit-if-changed --output=none lib test example/lib example/test. TESTING.md erwähnt nur flutter analyze + flutter test + go test. Sollte um den Format-Schritt ergänzt werden, sonst lokal-grün ≠ CI-grün.

4. Defaults sind keine validen Encodings

_defaultBtcXPub = 'xpub-simulated', _defaultSignedPsbt = 'psbt-signed-by-simulator' — falls Consumer-Code irgendwo PSBT/xpub parst, schlagen Tests mit Defaults überraschend fehl. Entweder echte Test-Vektoren (z.B. die validPSBT-Konstante aus fake_bitbox_test.go) oder im DartDoc deutlich kennzeichnen, dass die Defaults nur Echo-Strings sind.

Validierung lokal (nach Rebase)

Check Status
flutter analyze --no-fatal-infos 15 pre-existing infos, keine neuen
flutter test 12/12 pass (BIPPath + neuer testkit-Suite)
dart format --set-exit-if-changed pass
go vet ./... pass
go test -race -timeout 60s ./... pass beide Pakete

CI-Status auf dem rebasten Commit caf6496: 3/3 Checks pass, mergeStateStatus: CLEAN.

Empfehlung: Concern #1 vor dem Merge mit Joshua klären (PR-Body ergänzen oder splitten), Concerns #2#4 können als Follow-up adressiert werden.

…eout

This branch originally removed the iOS BLE per-packet dedup (Bluetooth.swift
seenPackets set + contains-before-reset ordering) and replaced the protection
with a comment-level contract that "the BLE bridge must keep request
boundaries clean". The change reverses PR DFXswiss#15 (merged 2026-05-15, same day)
which itself was a targeted fix to the dedup ordering.

No rationale was provided in the PR description for why dedup is now
unsafe/unnecessary, and the maintainer review pointed out that:

  * BLE/CoreBluetooth can deliver indications twice under poor RF conditions
  * U2FHID readFrame() has no SEQ validation
  * TestReadFrame_DuplicateContFrame_DesyncsFollowingMessage already
    demonstrates that a duplicated cont frame desyncs the next message
  * Buffer-clear between requests doesn't protect against duplicates
    arriving inside a single request-response cycle (which is the realistic
    failure mode under weak BLE)

Pending an explicit rationale, keep the dedup. Maintainer-edit applied
because the upstream author is unavailable.

Reverted to develop state:
  - ios/Classes/Bluetooth.swift (dedup logic restored)
  - go/u2fhid/dedup_logic_test.go (algorithm regression test restored)
  - go/u2fhid/u2fhid_dedup_test.go (comments restored)

Narrowed:
  - go/api/ios_bluetooth_regression_test.go now only guards the 60s read
    timeout (the uncontroversial fix), no longer forbids dedup patterns.

The rest of the PR (Flutter testkit, Go fake-device harness, interface
refactor, TESTING.md) is kept intact — that's all additive testing
infrastructure and the actual value of this PR.
@TaprootFreak

Copy link
Copy Markdown

Maintainer-Edit: BLE-Dedup behalten (Joshua nicht erreichbar)

Da Joshua nicht erreichbar ist und der PR keine Begründung für die Entfernung der BLE-Packet-Dedup-Logik enthält, habe ich konservativ entschieden:

Verworfen aus dem PR (Bluetooth.swift Dedup-Entfernung — Reversal von PR #15):

  • ios/Classes/Bluetooth.swift → develop-Stand (Dedup bleibt)
  • go/u2fhid/dedup_logic_test.go → wiederhergestellt (Algorithm-Test bleibt)
  • go/u2fhid/u2fhid_dedup_test.go → Comments zurück
  • go/api/ios_bluetooth_regression_test.go → eingeengt auf den 60s-Timeout-Guard, forbidden-patterns Block raus

Begründung: BLE/CoreBluetooth kann Indications unter schwacher Verbindung doppelt liefern; U2FHID readFrame() hat keine SEQ-Validierung; der bestehende Test TestReadFrame_DuplicateContFrame_DesyncsFollowingMessage zeigt explizit, dass doppelte cont-Frames den nächsten Read desynchronisieren. Buffer-Clear zwischen Requests schützt nicht innerhalb eines Requests — was der realistische Failure-Mode unter schwachem BLE ist. Risiko an Wallet-Signing-Roundtrips zu hoch ohne explizite Begründung.

Joshua kann nach Rückkehr mit dokumentierter Begründung einen eigenen "refactor(ble): drop packet dedup" PR aufmachen.

Behalten aus dem PR (alles additive Testing-Infrastruktur):

  • lib/testing/bitbox_testkit.dart + lib/testing.dart
  • test/bitbox_testkit_test.dart
  • go/api/bitbox_device.go (Interface)
  • go/api/fake_bitbox_test.go
  • go/api/api.go (interface-Refactor, var bitbox bitboxDevice)
  • TESTING.md
  • ios_bluetooth_regression_test.go (nur 60s-Timeout-Schutz)

Commit: 2f14fe4. Lokal validiert: go test (race), flutter test 12/12, dart format, YAML lint — alle grün.

Concerns #2#4 aus dem vorigen Review (API-Duplizierung, TESTING.md ohne dart format, unrealistische Defaults) bleiben offen für Follow-up — Joshuas Kommentar dazu abwarten.

TESTING.md listed only `flutter analyze` + `flutter test` + `go test ./...`
as the fast gate. The PR-checks workflow also runs `dart format
--set-exit-if-changed` over lib + test + example and `go vet` + `go test
-race -timeout 60s`. Without those, contributors run a green local check
and still hit CI failures.

Reproduce-CI-locally now reads exactly what `pull-request.yaml` runs.
@TaprootFreak TaprootFreak merged commit 08fa05c into DFXswiss:develop May 18, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants