feat: add blockstream jade hardware wallet support - #1231
coreyphillips wants to merge 11 commits into
Conversation
|
- Route activity teardown through JadeRepo so the transport, core session and cached connection are cleared together, off the main thread - Refuse Jade signing when the connected session belongs to a different wallet
# Conflicts: # gradle/libs.versions.toml
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at ca919d98b as a funds/signing change. No HIGH, no MEDIUM. Three LOW notes below, all on the lifecycle surface rather than the signing path — none loses funds, none wedges the app.
The signing path holds up. This was the thing I most wanted to break, so here is the trace, because the two vendors are protected by different mechanisms and that is worth writing down:
- The PSBT is composed app-side from the stored account xpub, so the change output is app-derived and never device-sourced. The only device-supplied input to compose is the master fingerprint, which affects key-origin metadata only.
- Jade:
signPsbtfeeds the device's return tofinalizePsbt(originalPsbt, signedPsbt). In bitkit-core v0.5.16src/modules/onchain/psbt.rs, :55-59 rejectsoriginal.unsigned_tx != signed.unsigned_tx, :61-73 pins each input's previous output, thenfinalize_mut+interpreter_checkverify the signatures against the pinned inputs. The device can contribute signatures and nothing else. - Trezor: does not go through
finalizePsbt— it broadcastsserializedTxdirectly. It is protected instead by trezor-connect-rs 0.4.0, which derivesexpected_scriptsbefore signing and then runsverify_signed_txon the device-returned bytes (src/tx_verify.rs:23-73, a port of @trezor/connect'sverifyTx): output count, every output amount, every output scriptPubKey against independently derived expectations. Equivalent guarantee, different code path. Worth knowing if anyone later assumesfinalizePsbtcovers both. - One caveat, pre-existing and not this PR: the Trezor path does not pin input outpoints, so a device could in principle substitute another UTXO the same seed controls. That matches upstream Trezor Connect.
- Every signing caller routes through
signFunding/broadcastFunding. The only othersignTxFromPsbt/broadcastRawTxuser is the pre-existing dev TrezorScreen. Nothing broadcasts a device return without one of these checks.
Also verified clean: amount and address shown on HwSendSignScreen are the same values that build the HwSendRequest, and miningFeeSats comes from the same compose result that produced the PSBT — no recompute after display. A swapped Jade with a matching efuse MAC is caught downstream (different xpubs produce a new walletId, so ensureConnected(oldWalletId) throws; in the locked silent-reconnect path where xpubs aren't re-read, signing fails at interpreter_check). No xpub, PSBT or fingerprint reaches a Logger call. runSuspendCatching is used throughout; plain runCatching appears only around non-suspending calls and once with the correct explicit CancellationException/TimeoutCancellationException rethrow guard.
Trezor regression surface — the part I'd most expect a second-vendor PR to break. TrezorRepo changes are limited to vendor-scoped store reads/writes, the (null-for-Trezor) fingerprint passthrough, and helpers moved to KnownDevice.kt with identical logic plus a vendor equality check. TrezorTransport only extracts requestUsbPermission into UsbPermissionRequester with the same action, flags and timeout. HwWalletStore.saveKnownDevices keeps the other vendor's entries inside one updateData, so concurrent writes from both repos can't drop entries. HwWalletId.derive's default stays "trezor", so existing Trezor wallet ids are stable, and legacy entries deserialize with vendor = TREZOR. Bluetooth discovery now alternates vendors with SCAN_INTERVAL 2s → 4s, so Safe 7 discovery is slower — a deliberate trade against Android's scan-rate limit, not a defect.
Both greptile threads are genuinely fixed at head (2dd3da77e), with tests; I checked rather than taking the claim.
Interaction with #1248 (fix/receive-liquidity-parity): no hidden semantic conflict, but expect a textual one. This PR renames ReceiveTab.TREZOR on the onClickEditInvoice line in ReceiveQrScreen.kt (:383-390) while #1248 rewrites the two lines just below it, and similarly in :156). #1248 adds no new ReceiveSheet.kt (ReceiveTab.TREZOR references, so once the conflict is resolved nothing compiles silently wrong. ReceiveInvoiceEditStateTest.kt hunks are disjoint. Whoever merges second should expect to resolve by hand rather than trusting a clean auto-merge.
journeys/hardware-wallet/README.md honestly scopes Jade as unit-test + manual-only, which is the right call given there's no Jade emulator.
- Close a hardware session from the receive sheet only after the sheet used the device - Ignore a transport restore for a vendor with no paired device so it cannot drop the other vendor's session - Let the send sheet be dismissed while it connects or waits for a PIN, and keep blocking it during device signing and broadcast - Close the Jade link before the core session so cancelling releases a pending unlock
There was a problem hiding this comment.
Verdict: ♻️ Comment
Review: diff 58 files.
Findings:
3 inline (non-blocking)
Security audit: no findings
Coverage:
Journeys: 25% - No journey added or changed: bitkit-docker has no Jade emulator, so Jade flows rely on unit tests and manual runs, and the Trezor journeys are untouched.
Unit tests: 85% - Nine test files added or extended across the touched layers, giving every one of the thirteen author claims a named test; the null-efuseMac identity path is the gap.
QA: Manual Tests not run
Reviewed by claude-opus-5-high via gh-pr-review-loop skill
| if (version.jadeState == JadeState.UNINIT) rejectDevice(HwDeviceUninitializedError()) | ||
| val expectedHardwareId = expected?.jadeDeviceId | ||
| val hardwareId = version.efuseMac | ||
| if (expectedHardwareId != null && hardwareId != null && expectedHardwareId != hardwareId) { |
There was a problem hiding this comment.
rejectUnusableDevice only compares the ids when both are present:
if (expectedHardwareId != null && hardwareId != null && expectedHardwareId != hardwareId) {
rejectDevice(JadeIdentityMismatchError())
}If we paired with a Jade that reported an efuseMac but the device we are now talking to reports null, the guard is false and the session is accepted as the expected device. That is the fail-open direction for an identity check. HwWalletRepo.signJadeFunding still gates signing on the wallet id so funds are not at risk here, but the pairing UI would otherwise report the wrong physical device as connected.
if (expectedHardwareId != null && expectedHardwareId != hardwareId) {
rejectDevice(JadeIdentityMismatchError())
}That reads the same and is one condition shorter, and a JadeRepoTest.kt case with a stored jadeDeviceId and efuseMac = null would pin it. Could we drop the hardwareId != null condition so a missing efuse MAC fails closed?
| isSetup.await() | ||
| } | ||
|
|
||
| private suspend fun loadKnownDevices(): List<KnownDevice> = runCatching { |
There was a problem hiding this comment.
This block calls the suspend hwWalletStore.loadKnownDevices and hwWalletStore.saveKnownDevices, so plain runCatching catches Throwable and swallows cancellation — the .onFailure below then logs it as an error and the caller gets an empty list while its scope is already unwinding.
saveKnownDevices right underneath at :857 already uses runSuspendCatching over the same store, so this reads as an oversight rather than a deliberate carve-out. (The runCatching at :790 is the documented withTimeout exception and is fine; :501 wraps the non-suspend jadeTransport.closeAllConnections() so it is fine too.)
private suspend fun loadKnownDevices(): List<KnownDevice> = runSuspendCatching {Could we change this one runCatching to runSuspendCatching?
| if (jadeRepo.state.value.connected?.transport == JadeTransportKind.BLUETOOTH) return true | ||
| val trezorId = trezorRepo.state.value.connected?.id ?: return false | ||
| return devicesForDeviceId(trezorId).any { it.transportType == TransportType.BLUETOOTH } || | ||
| trezorId.startsWith("ble:") |
There was a problem hiding this comment.
"ble:" now appears as a constant in two places and as a bare literal here:
JadeTransport.kt:67—private const val BLE_PATH_PREFIX = "ble:"JadeRepo.kt:942— a second file-levelprivate const val BLE_PATH_PREFIX = "ble:"HwWalletRepo.kt:349—trezorId.startsWith("ble:")
TrezorTransport.kt:922 and :1372 already carried the literal before this PR, so a shared declaration would fold those in too and bring all five call sites onto one definition. The bare literal here is the one a search for the constant would miss, and it gates whether Bluetooth scanning is suppressed while a session is open.
Could we promote one BLE_PATH_PREFIX — or a small isBlePath(path: String) helper next to the other Hw* extensions — to a shared location and import it at all three sites?
This PR:
Requires bitkit-core 0.5.16 (synonymdev/bitkit-core#153), which carries the Jade module and pins
jade-client-rsatd52ccd9.Description
A Jade can now be paired from Connect Hardware over either transport, unlocked with its PIN, and
used exactly like a paired Trezor: watch-only balances, on-device receive address verification, and
on-device signing for both a normal send and a transfer to spending. The protocol, the pinserver
round trip and every deadline live in bitkit-core. This app supplies the byte transport over the
phone's radios and the UI that drives the flows.
The transport covers USB serial through a CP210x bridge on Jade v1 and native USB CDC on Jade Plus,
plus Bluetooth over the Nordic UART Service. Three USB device filter entries were added so Android
offers Bitkit when a Jade is plugged in.
Four things only a physical device revealed, each fixed here:
indications when notify is absent instead of failing the connection.
stored entry is recognised by name, which is Jade plus the last six hex digits of its efuse MAC,
rather than by address.
power-cycled. Links are now closed when the activity finishes, and released after 30 seconds in
the background so the same thing does not happen when Android kills a backgrounded Bitkit. Coming
back to the foreground reconnects without a prompt.
wide enough to cover a re-pair, and a message telling the user to forget the Jade in Android's
Bluetooth settings and pair again.
The vendor-neutral part is a refactor rather than new behaviour. The hardware wallet repository now
merges both vendors' discovery state, routes connect, verify and sign by the vendor stored on the
paired entry, and alternates which vendor gets the Bluetooth half of a scan so repeated searches
stay under Android's scan-rate limit. Watchers, transaction composition and broadcast are vendor
neutral already and stay where they are. Entries saved before this change carry no vendor and are
read as Trezor, so paired Trezors are untouched. Reconnect gets a longer deadline for a Jade,
because that reconnect may be waiting for a PIN to be entered on the device.
Session and identity hardening added during review:
Jade-specific failures get their own copy: PIN entry, wrong PIN, an unreachable pinserver, a device
that is busy, firmware too old, a device that has no wallet yet, a network mismatch, and a PSBT the
device cannot hold.
Two gaps worth naming. The Jade illustration is a placeholder vector until design supplies the real
asset. Signet is not supported by Jade, so that combination throws rather than mapping to a network.
Preview
QA Notes
Verified against a Jade v1 on firmware 1.0.41. There is no Jade emulator in
bitkit-docker, sothese are all physical-device checks.
Manual Tests
unlock completes, accounts export and the wallet tile appears.
regression:USB → Send → pick the Jade source → sign on device → broadcast:transaction confirms.
matches the one in the app.
d955bc0c....no pairing prompt and no PIN re-entry.
still recognised even though the device advertises a new address.
regression:Trezor paired before this branch → open the wallet, verify an address andsend: unchanged.
Automated Checks
JadeTransportTest.ktcovers USB driver selection, the CP210x and CDC open andclose sequences, chunk sizing and read and write timeouts;
JadeRepoTest.ktcovers connect,unlock, replug and reconnect, recognising a Bluetooth Jade by name after its address changed, the
background release and its USB counterpart, signing and address verification;
JadeServiceTest.ktcovers the
finalizePsbtalias that used to recurse into itself;HwUsbIdTest.ktcovers vendordetection from USB ids;
KnownDeviceTest.ktcovers vendor-aware entry matching, migration ofpre-Jade entries, wallet identity and equal-seed vendor isolation;
HwErrorPresenterTest.ktandHwExceptionExtTest.ktcoverthe Jade error copy and classification. The Bluetooth GATT paths themselves, including the
indicate-only fallback, are not unit testable and were validated on hardware.
HwWalletRepoTest.kt,HwConnectViewModelTest.kt,HwSendViewModelTest.kt,HwReceiveViewModelTest.kt,TransferViewModelTest.kt,TrezorRepoTest.ktandReceiveInvoiceUtilsTest.ktmove onto the vendor-neutral device state and the per-vendor routing.just compile,just test(2626 tests, 0 failures) andjust lintall pass.