feat: expose the connected device's firmware version - #35
Open
Danswar wants to merge 15 commits into
Open
Conversation
Adds `getFirmwareVersion()` so a host app can tell which firmware a paired BitBox is running. On Bluetooth the version already arrives on the product characteristic, which the peripheral publishes on connect — `BluetoothManager.parseProduct()` has been parsing it all along to hand to `GetDeviceWithInfo`. This only surfaces that existing value to Dart, so it costs no device round-trip and is readable before pairing. USB carries no product characteristic and cannot report a version. The Android plugin therefore registers no handler, and the method channel converts the resulting MissingPluginException to null. Null means "this transport cannot report a version", never "old firmware" — a caller that conflates the two would penalise every USB device. Both the interface doc and a dedicated test state this. `SimulatedBitboxPlatform` gains a `firmwareVersion` knob (null simulates USB) so host apps can drive version-dependent behaviour in tests without hardware.
The first cut exposed getFirmwareVersion from the iOS BLE product characteristic only, and documented the resulting gap as a property of the transport: "the USB transport does not carry it, and returns null." That is not true. On USB the SDK infers the version from OP_INFO while initialising (inferVersionAndProduct), so the value was available all along -- it simply was not exported. A host gating on a minimum firmware version would have read null on every Android device and, following the doc, skipped the gate. Complete the platform method the way CONTRIBUTING prescribes: export FirmwareVersion from the gomobile boundary behind recoverPanic, add Version() to the bitboxDevice interface, register getFirmwareVersion on the Android bridge, and cover the wiring in the Go fake. Both native bridges now read the version the SDK already holds, so iOS and Android answer identically. Version() panics when the device has not reported a version yet -- the normal state for USB before initBitBox -- so recoverPanic returns the "" zero value, which the Dart side maps to null. Null therefore means "not known yet", never "old firmware", and the docs say so. Drop the testkit's needsOpen: false. It modelled a pre-pairing read that hardware does not do: the product characteristic only arrives once paired, and its arrival is what sets isPaired. A consumer's version gate would have passed against the simulator and read null in the field. Regenerated api.aar, api-sources.jar and Api.xcframework via gomobile; the 16 KB ELF page alignment from 0.0.9 is preserved on all four ABIs.
…ions Round-2 review turned up three ways the new API could hand a host a version that is not the connected device's. The Go binding never cleared its device: `bitbox` was only ever assigned, so after close() the previous device answered. iOS made this reachable for the firmware version specifically -- the replaced implementation read the BLE product characteristic, which handleDisconnect() nils, so it correctly went quiet on disconnect. Add an exported ReleaseDevice() and call it from close() on both bridges. getDeviceStatus() was affected the same way and its own doc already promised "an empty string when there is no device", so this brings it back in line too. GetDeviceWithInfo substitutes an invented v9.25.0 when the version string the device reported does not parse. That placeholder is now externally visible, so a gate could clear a device whose firmware was never identified. Track that the version was synthesised and report it as unknown instead. The availability window was still documented wrong. open() only establishes the link; initBitBox() is what binds the device the version is read from, on Bluetooth as much as on USB. Correct all five doc sites, and make the testkit withhold the version until initBitBox has run rather than merely until open, so a consumer's gate cannot pass against the simulator and read null on hardware. Regenerated api.aar, api-sources.jar and Api.xcframework; both new exports are present as symbols and the 16 KB ELF alignment holds on all four ABIs.
…s leaking Round-3 review found the previous commit closed only part of each problem. ReleaseDevice was wired to the explicit close() call, but handleDisconnect() has three call sites: close(), a peripheral that drops on its own, and Bluetooth being switched off. Since iOS open() does not rebind, an unplanned drop left the binding reporting the old device for the whole reconnect. Move the release into handleDisconnect() so every teardown path clears it, and pin it from CI with a source assertion on Bluetooth.swift, next to the one that guards the 60s read timeout. Withholding the invented fallback version from FirmwareVersion was not enough either. GetDeviceWithInfo hands that placeholder to the SDK, and the SDK derives SupportsETH and SupportsERC20 from the version -- so a device whose version string did not parse still answered "ETH supported" from a number it never sent, which is exactly the gate this feature exists to inform. Withhold the capability answers too. The synthetic-version flag was also only tested through its reader: a mutation that stopped setting it left every test green. Test it through GetDeviceWithInfo instead, which needs no hardware -- neither u2fhid.NewCommunication nor firmware.NewDevice touches the transport. While here, close the race the previous commit introduced. close/disconnect runs on a different thread than an in-flight signature or the pairing handshake, so replacing the device could be observed half-applied -- a fault recoverPanic cannot catch. The device and its flag are now replaced as a pair under a mutex and every export takes one snapshot, rather than reading the global twice. The testkit forgot the version on close but not on a reopen without one, nor when initBitBox failed, so it stayed more permissive than hardware in exactly the reconnect path. Clear it in open() and on entry to initBitBox(). Regenerated api.aar, api-sources.jar and Api.xcframework; go test -race is green and the 16 KB ELF alignment holds on all four ABIs.
… guard Round-4 review found the release still had a hole and two of the guards this PR added did not hold. connect(to:) starts a new peripheral without going through handleDisconnect, and nothing rebinds the Go side until initBitBox, so opening a second device without closing the first left the first one answering. Android gets this free because open() calls GetDevice. Release there too. The testkit modelled the new fail-closed rule for getFirmwareVersion but not for supportsETH/supportsERC20, which the SDK derives from the same version -- so the simulator answered "supported" in exactly the states where the plugin answers false. A consumer's capability gate would have passed its tests and failed in the field, which is the divergence the testkit exists to prevent. The BTC/ETH harness test now initialises the device before asking about capabilities, as hardware requires. The mutex added last round was listed in TESTING.md as covered by go test -race, but every test was single-goroutine, so deleting the lock left the suite green. Drive readers and writers concurrently so the claim is true. The Swift source assertion had two ways to pass on a real regression: it discarded the result of the body Cut, so a reformat would silently widen it to the whole file, and a substring match accepted a commented-out call. It now fails loudly when it cannot find the function, skips comments, and also pins close() -> handleDisconnect, which was left unguarded when the release moved. No artefact rebuild: the Go exports are unchanged this round.
Round-5 review found the contract still had a hole in the direction that matters, plus three guards that did not guard. Over Bluetooth the version arrives on the product characteristic before pairing, so a FAILED initBitBox -- a declined confirmation, a failed handshake -- still answered with a real version and real ETH capabilities, vouching for a channel that was never established. The docs and the testkit already said "once initBitBox has succeeded"; only the binding disagreed. Track whether init succeeded and gate the version and the capabilities derived from it on that. Releasing the device on a failed init would have been the other fix, but Android's initBitBox does not rebind -- only open does -- so it would have broken retrying a declined pairing. markInitialised only marks while the device is still the connected one, so a disconnect during the pairing wait is not undone by the init already in flight. Android's open() released nothing on the failure path: connectBitBox throws before Api.getDevice rebinds, gracefullyReset tears down USB but leaves the Go device, so a failed reconnect kept the old one answering. Release at the top of the operation, mirroring iOS connect(to:). Three guards were weaker than they read. The testkit gated capabilities on supportsETH/supportsERC20 but not supportsLTC, which is equally unknown before init; the source assertion accepted a call named in a trailing comment; and adding the capability gate quietly made the rejected-pairing test's configured results unreachable, so flipping them to true left the suite green. That test now initialises first, and a new one covers the pre-init state directly. Regenerated api.aar, api-sources.jar and Api.xcframework; go test -race green, 25 Flutter tests, 16 KB alignment holds on all four ABIs.
…ing nil Round-6 review found the init gate keyed off the wrong signal, so the scenario it was added for was still open. The SDK returns no error when the user declines the pairing on the device: it discards both ciphers, clears the channel hash, sets StatusPairingFailed and returns nil (api/firmware/pairing.go, and Init's own doc comment lists StatusPairingFailed as a normal outcome). Marking on err == nil therefore marked a declined pairing as initialised, and getFirmwareVersion answered v9.26.4 with supportsETH true for a channel that had just been torn down -- exactly what a host gating EIP-712-over-BLE would act on. Key the flag on the channel hash being device-verified instead, which is true in both successful pairing branches and false only on a decline. InitDevice keeps returning true there, as before; only the version and the capabilities derived from it are withheld. The flag was also never cleared when an init attempt started, so on Android -- where initBitBox re-inits the bound device without reopening -- a failed retry kept the previous success answering, even though Init discards the old channel before it can fail. Clear on entry and set from the outcome. The concurrency test had quietly gone vacuous: its writer never marked the device initialised, so the reader could only ever observe "" and the torn-value assertion could not fire. Mark in the writer loop. Also pin Android's release calls from CI the way the Swift ones are -- they had no coverage at all, since CI runs no Gradle job -- and cover the identity check that stops a late init from marking a device that was replaced while it was in flight. Regenerated api.aar, api-sources.jar and Api.xcframework.
…ards Round-7 review found three guards that read stronger than they were, plus one asymmetry in the gate. The gate cleared on a device decline but not on a host one. ChannelHashVerify is a shipped export whose false branch puts the SDK into StatusPairingFailed without clearing its own device-verified flag, so the version kept answering for a code the user had just rejected as mismatched. Unreachable from Dart today -- both bridges hardcode true -- but the export is public and the false branch was wrong. Clear there too. The Android source assertion grepped whole files, so it checked neither the enclosing method nor the ordering its own comment described: moving Api.releaseDevice() to after connectBitBox -- which drops the device Api.getDevice had just bound -- passed. Scope it to onMethodCall and assert the release precedes the rebind. The concurrency test asserted only that no torn value appeared, which an always-empty reader satisfies, so removing the writer's setInitialised left it green. Count the successful observations and require at least one. Finally, four doc sites still said the version is available once initBitBox "succeeded". It resolves true on a decline, so that phrasing promised exactly what the previous commit removed. BitboxManager -- the facade consumers hover -- had not been updated at all. Regenerated api.aar, api-sources.jar and Api.xcframework.
The Go test cache does not hash files outside the module root, and the source assertions in ios_bluetooth_regression_test.go read Swift and Kotlin. With CI's own command a PR that only touches those files is served a cached "ok", so all four release guards -- and the pre-existing 60s BLE read-timeout guard, which the file itself says regressed signing on real hardware -- never run. Reproduced: deleting Api.releaseDevice() from ConnectBitBoxOperation returns "ok (cached)"; with -count=1 it fails. Add -count=1 in CI and in TESTING.md; the suite takes about two seconds. The ordering half of the Android assertion indexed the raw body while the existence half stripped comments, so a commented-out decoy above the rebind satisfied it while the real call sat after -- the exact regression it was added for -- and a comment-only edit could fail it. Both halves now share one comment-aware callIndex. The concurrency test's non-vacuity assertion was schedule-dependent: with a single P the writer never yielded between marking and releasing, so every read observed "" and it failed 100% of the time on correct code. A Gosched between them makes the marked state observable without weakening the assertion. ChannelHashVerify only cleared; re-affirming a code the host had rejected left the channel usable for signing but the version withheld until a full re-init. Set from the outcome instead, which still clears on a device decline. The testkit could not model a declined pairing at all: it keyed on initBitBox's return value, the very coupling the binding stopped using. Add a pairingVerified knob so the state that motivated this whole gate -- init true, version null -- is reachable, and correct the parity claims that said otherwise. Also correct six remaining doc sites that still said "once initBitBox succeeded", and a Swift comment claiming Android releases implicitly when this PR made it explicit.
Round-9 review found the conjunct doing the safety work in ChannelHashVerify was untested: mutating `ok && deviceVerified` to `ok` left the whole suite green. It is reachable through the plugin's own surface -- both bridges hard-code channelHashVerify(true), and a device decline still resolves initBitBox to true -- so a host that shows the code and then confirms hits exactly that sequence when the user declines. Cover it, plus the restore on re-affirm, which was equally unpinned. The rest is wording that had drifted behind the code: - CONTRIBUTING's PR-gate table still documented the go test command without -count=1, the exact form the previous commit showed silently skips the source assertions. - The testkit's supportsLTC comment and the CHANGELOG's testkit bullet still justified the gate as "waits for a successful initBitBox", which the pairingVerified knob added in the same commit falsified. - A testkit comment read as though the version returns after close; it goes absent again. - TESTING.md described the signal as device-verified only, omitting the host repudiation added since, and its coverage list omitted the host-rejected case. - The two native-bridge comments were the last sites implying "available after init". - The CHANGELOG advertised the host-rejection state without noting that BitboxManager cannot produce it -- channelHashVerify() always affirms -- so a reader would look for an API that is not there. No artefact rebuild: no non-test Go source changed.
Round-10 review found the testkit's close path unpinned: deleting `_isInitialised = false` from close() left the whole Flutter suite green, because every close-path test either reconnects -- open() clears the flag too -- or leans on requireOpen's exception rather than the flag. With requireOpen: false, a supported configuration the suite already uses, the mutant hands back the previous device's version after close. Three doc sites promise otherwise, including a comment corrected in the last commit. Add the test that fails without it. The remaining items are wording: - TESTING.md still said the testkit "withholds the version until initBitBox has run", the justification the pairingVerified knob falsified, contradicting the bullet six lines below it. - Its coverage list omitted the host-affirming-a-decline case, which is the only one of these reachable through the plugin's own surface. - CONTRIBUTING's gate table said flutter analyze --no-fatal-infos while CI runs --fatal-infos, so a contributor following it locally goes green and fails CI on any info-level diagnostic. Stale since DFXswiss#23, in the row above the one the last commit corrected. No artefact rebuild: no non-test Go source changed.
Transitive dependency resolutions picked up by running the example's pub get locally. Nothing in this PR depends on them, and a lockfile bump does not belong in a feature diff.
The CHANGELOG credited the new pairing gate to supportsLTC alone, "matching supportsETH / supportsERC20" -- implying those two already behaved that way. They did not: all three were ungated on develop and all three changed in this release. As written, a downstream app fixes its LTC assertions, then hits the same failure on ETH and ERC20 with the CHANGELOG implying they were untouched. Name all three in the migration note. TESTING.md described recoverPanic as the mechanism that yields "" for an unknown version. Since the pairing gate landed, FirmwareVersion returns "" from the device/initialised check before Version() is ever called, so in production the panic is unreachable and recoverPanic is a backstop. The Go test reaches it by constructing the state directly. CONTRIBUTING step 1 named BitboxUsbMethodChannel, which does not exist; the class is MethodChannelBitboxUsb.
…n version The previous commit corrected this in TESTING.md but left the two source comments it describes, so the file and the code now disagreed. FirmwareVersion returns "" from the pairing gate before Version() is reached in every state the SDK can produce, so recoverPanic there is a backstop, not the mechanism. The test's framing had the same problem: it cannot sit in the window it described, because reaching Version() at all requires a device-verified channel. It deliberately constructs initialised with no version -- a combination the SDK cannot produce -- to keep the backstop pinned. Renamed and reworded to say that. The recovered-panic line still appears in its output, so the path is genuinely driven. Comments and a test name only; the exported doc comment is unchanged, so the generated headers and the checked-in artefacts still match.
The empty string persists past OP_INFO -- past attestation and unlock too -- until the device confirms the pairing, which is why a decline resolves initBitBox to true and still reads null. Every other site says so; this Dart test comment was the last one left with the old framing.
Author
|
Ready for review. It took 14 review passes to reach zero findings — the first nine surfaced real defects, the last five were documentation wording. What the substantive passes changed:
Every behaviour above is mutation-checked: reverting any one of them fails a test. Known residual, not fixed here: Android has no Worth a hardware re-check before merge: the earlier BLE run exercised the product-characteristic implementation this replaces with the Go path. The value should be identical, but the code path reporting it is new and the disconnect handling changed. |
Danswar
marked this pull request as ready for review
July 31, 2026 00:50
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
1 participant
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.
Why
A host app currently has no way to learn which firmware a paired BitBox is running.
RealUnit needs it: firmware up to and including v9.26.4 cannot sign an EIP-712 message whose domain omits
chainIdwhen the device is reached over Bluetooth — it answersHWW_RSP_NACKinstead of a signature (upstream BitBoxSwiss/bitbox02-firmware#2019). The app wants to show an explanatory screen rather than let the user fill in a registration form that cannot be submitted, and that decision needs the version.What
BitboxManager.getFirmwareVersion(), returning e.g."v9.26.4", ornull.It takes the uniform path CONTRIBUTING prescribes for a platform method —
//export FirmwareVersionbehindrecoverPanic,Version()on thebitboxDeviceinterface, both native bridges, the testkit, and Go + Dart tests — so Bluetooth and USB both report a version. An earlier revision of this PR surfaced the value from the iOS BLE product characteristic only; that is what the description used to describe.The contract
Available once the pairing has been established, on both transports.
open()only establishes the link;initBitBox()is what binds the device the version is read from — andinitBitBox()returning true is not sufficient either, see below. Over Bluetooth the version reaches the SDK withGetDeviceWithInfo, over USB the SDK infers it fromOP_INFOwhile initialising. Reading it afterwards costs no device round-trip.nullmeans "not known", never "old firmware". No device, an init that has not run or did not succeed, a pairing the device declined (whichinitBitBox()still reports as true), released, or a version string that would not parse. A host gating on a minimum version must treat the two apart and refuse rather than pass when the version is absent. This is stated on every doc site and pinned by tests.Four defects found while completing it
Each was reachable through the new API and is fixed here:
A device that was gone kept answering. The Go binding only ever assigned its device, never cleared it, and iOS
open()does not rebind — so after a disconnectgetFirmwareVersion()andgetDeviceStatus()still reported the previous device.ReleaseDeviceis now called fromhandleDisconnectand fromconnect(to:), covering an explicitclose(), a peripheral that drops on its own, Bluetooth being switched off, and reconnecting without closing first; Android releases onopenfor the same reason. Source assertions pin it from CI, the way the 60s read timeout is.A version could vouch for a pairing that never happened. Over Bluetooth the version is known before pairing, so a failed
initBitBox— a declined confirmation, a failed handshake, a failed retry — still answered with a real version and real capabilities, over a channel that was never established. The binding now tracks whether the pairing was established and withholds both until it was. The signal is the channel hash being device-verified and not since repudiated by the host, notInit()returning without error: the SDK returns nil on a decline, having already discarded both ciphers (api/firmware/pairing.go), soinitBitBox()still resolves true there — only the version and the capabilities derived from it are withheld.An invented version was reported as the device's.
GetDeviceWithInfosubstitutes a placeholderv9.25.0when the version string does not parse. That placeholder is now withheld — fromFirmwareVersionand also fromsupportsETH()/supportsERC20(), which the SDK derives from the version, so a capability gate cannot be cleared by a number the device never sent.The device state was written unsynchronised.
close/disconnect runs on a different thread than an in-flight signature or pairing handshake, so replacing the device could be observed half-applied — a faultrecoverPaniccannot catch. The device, its synthetic-version flag and its initialised flag are now replaced as a pair under a mutex, and every export takes a single snapshot.getDeviceStatus()was affected by 1 and 4 the same way and is fixed with it; its own doc already promised "an empty string when there is no device".supportsETH()/supportsERC20()change with 2 and 3. All are noted in the CHANGELOG as behaviour changes.Known residual: Android has no
ACTION_USB_DEVICE_DETACHEDreceiver, so unplugging a USB device without callingclose()leaves the binding stale until the nextopen()(which rebinds). Closing that needs a broadcast receiver and its lifecycle, which is a change to USB session handling rather than to this feature — better as its own PR.Test evidence
Local gates green:
dart format --set-exit-if-changed,flutter analyze --fatal-infos,flutter test(27 passing),gofmt,go vet ./...,go test -race -timeout 60s -count=1 ./....The behaviours above are mutation-checked, not just covered. Each of these fails a test when reverted: the
vprefix, the synthetic-version guard, theSupportsETH/SupportsERC20guards, the device release on either platform, the pairing-verified signal, clearing the flag on a re-init attempt, the identity check that stops a late init marking a replaced device, the host-repudiation clear and its re-affirm restore, the testkit's init and close gates, anddeviceMuitself (which trips-race).Regenerated
api.aar,api-sources.jarandApi.xcframeworkvia the pinned gomobile fromgo.mod. Both new exports are present as real symbols in the device and simulator slices, and the 16 KB ELF page alignment from 0.0.9 holds on all four ABIs.