feat: add blockstream jade hardware wallet support - #1231
coreyphillips wants to merge 27 commits into
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- 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
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
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ♻️ Comment
Review: diff 58 files.
Findings:
3 inline (non-blocking)
Audit:
Audited - 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 await all reviewers to approve, author can run it now via comment: @ovi-reviewer test
Reviewed by claude-opus-5-high via gh-pr-review-loop skill
Commands: @ovi-reviewer test · retest · audit (author or owner)
Order an external disconnect notification against the next connect, which 0.5.17 requires. jade_notify_disconnected matches the path it is given against the connected session, so a notice still in flight when a reconnect for that path completes tears the new session down instead of the old one. Nothing ordered the two before: observeExternalDisconnects awaited the notice in one coroutine while retryAutoReconnect connected from another. ServiceQueue.CORE does not close that gap, since a single thread dispatcher serialises dispatch rather than suspending work, and jadeConnect releases the thread while it awaits the handshake. The new mutex is held for the notice alone, so a long connect or a five minute unlock never delays one. No API changes in 0.5.17, only behaviour. Worth knowing: - A cancel now reports UserCancelled whether core notices the abort flag or the closed link first. It used to surface as DeviceDisconnected whenever it landed while a read was parked, which is the common case on Bluetooth, and isJadeUserCancellation only matches UserCancelled, so cancels were being classified as session failures. - jadeGetVersionInfo reads a cached copy and no longer waits behind an operation in flight. - jadeScan reports NotInitialized when no transport callback is registered, which JadeService already makes unreachable.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…wallet # Conflicts: # gradle/libs.versions.toml
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since my last pass (bf77458, 7bc62d9, 7e81da2): one LOW observation inline, which I traced statically and haven't verified on hardware.
Checked:
- Earlier threads. ovi-reviewer's three threads are answered.
rejectUnusableDevicenow fails closed on a null efuse MAC while tolerating legacy blank ids,loadKnownDevicesusesrunSuspendCatching, and the BLE prefix is shared. My three earlier LOWs hold at head:engagedWalletIdgating, theonTransportRestoredearly return, andcanLeave. disconnectNoticeMutex. No deadlock: the notice path waits only on corelifecycle, and connect takes the mutex beforelifecycle. The leftover race window needs a same-path reconnect within milliseconds of a GATT drop, andretryAutoReconnectwaits at least 2s.- bitkit-core 0.5.14 → 0.5.17. Adds the
Blockstreamvendor, catalog entries and thefinalize_psbtsighash-type check (strictly safer, and Trezor doesn't use it). The backup migration change is rustfmt in tests only, andjade:<hash>ids survive it. No storage changes. The pinserver host is pinned tojadepin.blockstream.com. - Sign path. Identity check →
check_signable(ALL/DEFAULT sighash only, fingerprint must match) →verify_signed_psbt→ corefinalize_psbtwithinterpreter_check. A swapped or re-seeded Jade either fails the wallet-id check or produces signatures that fail interpretation. Change is derived app-side, and a retry reuses the cachedsignedTx. - Upgrade from Trezor-only state.
vendordefaults to TREZOR,ignoreUnknownKeysis set, and vendor-scoped saves keep the other vendor's rows. - Cancel/Back. Leaving Send during the PIN wait cancels and closes the link before core,
NonCancellable. Cancelling the connect sheet resets state. The Receive sheet only disconnects when a device was engaged. - Gating. Not gated: Settings, the Home suggestion and USB attach are all reachable in release.
- iOS twin, synonymdev/bitkit-ios#765. Reviewed clean. The USB case below doesn't apply there.
| JadeTransportKind.BLUETOOTH -> path == device.path || advertisesAs(device.name) | ||
| // A plugged-in Jade cannot be told from a paired one before connecting, so a paired USB Jade | ||
| // claims every serial device; a second one is added through the Add button, which offers it anyway. | ||
| JadeTransportKind.SERIAL -> transportType == TransportType.USB |
There was a problem hiding this comment.
LOW (observation, static trace only): a second USB Jade looks unpairable while any USB Jade is paired, which contradicts this comment.
The SERIAL branch claims every plugged serial device for the paired USB entry, so scan() drops it from nearbyDevices (:196). Connect Hardware then falls back to hasKnownDevice(it.id, advertisedName = it.name) (HwConnectViewModel.kt:333-335). For a USB Jade, id is a path the device was never connected at and name is the USB product string. That string never ends with the efuse suffix, so matches and advertisesAs are both false. The OS attach route is closed as well: hasKnownUsbDevice returns true whenever any USB Jade is paired (JadeRepo.kt:476, AppViewModel.kt:5390), and the restore reconnect then rejects the second device with JadeIdentityMismatchError.
Scenario: Jade A is paired over USB. Plug in Jade B, then Settings → Hardware Wallets → Add → Continue: Searching keeps spinning. Accepting the Android USB chooser shows nothing. The workarounds are pairing B over Bluetooth or removing A first. No funds are involved.
Fix: in the SERIAL branch, treat a serial device as known only when a stored entry matches its path (or serial number when available), and let hasKnownUsbDevice require an exact matches(deviceId). If you've confirmed Add does offer B with two devices, disregard this and fix the trace in the comment.
There was a problem hiding this comment.
Confirmed, the trace holds and the comment was wrong. Fixed in 187fd19.
Add flow. isSameDevice now lets a paired USB entry claim a serial device only at the path it was last connected at, so Jade B is offered in Settings → Hardware Wallets → Add. A replugged Jade A at a new path is offered too, and pairing it again refreshes its existing entry rather than adding one (a replugged jade refreshes its stored entry instead of adding one). New tests in JadeRepoTest.kt: scan offers a usb jade plugged in away from the paired entry's path, scan does not offer a usb jade at the paired entry's path and pairing a second usb jade keeps the paired one.
Attach route. I kept hasKnownUsbDevice lenient on purpose. Before connecting there is no way to tell A from B, and A's path is renumbered on every plug, so an exact matches(deviceId) would open the Found sheet for the paired Jade each time it is plugged in through the chooser (over Home or Receive, since only Send and a few other sheets block it). So with A paired, attaching B still opens nothing, and B is paired from Add. The KDoc now says this, and a usb attach at a new path counts as known while a usb jade is paired pins it. Happy to switch if you'd rather take the replug prompt.
One related limit I left alone: with two USB Jades paired, the silent restore after an attach only tries the first USB entry, so the other one connects on first use instead. Send and Receive verification go through ensureConnected, which picks the right entry, so nothing is blocked.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 2 files.
Counterpart synonymdev/bitkit-ios#765: not compared.
Findings:
N/A
Audit:
Already done in comment.
QA:
Tested on emu-1 Pixel 10 Pro emulator on Android 16
Test 1
evidence
1.mp4 |
 |
Test 2
evidence
2.mp4 |
 |
Test 3
evidence
3.mp4 |
 |
Test 4
evidence
4.mp4 |
 |
Test 5
evidence
5.mp4 |
 |
Test 6
evidence
6.mp4 |
 |
Warning
Every item requires a physical Blockstream Jade. The lane has no Jade on USB or Bluetooth, and the repo toolbox has no Jade emulator. The exact-head binary, discovery scans, unpaired Send and Receive paths, and 46-second background/reopen path were exercised; the signer-dependent assertions remain hardware-absent rather than incomplete.
Coverage:
Unit tests: 100% - JadeRepoTest.kt covers both scan outcomes, broad USB attach recognition, and retaining two efuse identities.
QA: 0 of 6 manual tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
There was a problem hiding this comment.
Delta since my last pass (187fd196f): no findings.
- The USB thread's scenario is fixed. The
SERIALbranch ofisSameDevice(JadeRepo.kt:898) now claims a serial device only at the stored path. With A paired and B plugged in,scan()keeps B, and Add pairs it withexpected = null, B's own efuse id and B's own xpubs. A's row survives becauseisReplacedByneeds an equal id. The three new tests cover offered, not offered, and both-kept. - Replug. A replugged A at a new path refreshes its own entry instead of adding a second one.
- Attach route. It is unchanged on purpose. The KDoc and a test pin the lenient
hasKnownUsbDevice. The replug-prompt question stays on its thread. - Sign path. Untouched. The
ensureConnectedwalletId check and thesignJadeFundingidentity check still gate every Jade signature. - Cancel/Back with two Jades attached. I walked Intro, Searching, Found (idle and waiting on the PIN) and Paired. Each one ends in
resetState→cancelPendingConnection, with transport teardown before core underNonCancellable. A's stored row is only written after unlock and export, so cancelling B's connect can't touch it. - Bounds. USB permission prompts are one per candidate. Scans and silent reconnects never prompt, and reconnect retries stay capped at 4.
- Vendor mix-up. Nearby Jade entries carry
BLOCKSTREAM, the USB re-scan resolves by exact path, and the other vendor is disconnected undersessionMutexbefore connect.
|
@coreyphillips conflicts |
|
@jvsena42 Resolved in 92c3bba, which merges current master. Two conflicts, both in the tab row:
Compile, unit tests (2831) and detekt pass. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 2 files.
Counterpart synonymdev/bitkit-ios#765: not compared.
Findings:
N/A
Audit:
Already done in comment.
QA:
Tested on emu-1 emulator, Android 15, regtest
Test 1
evidence
1.mp4 |
 |  |
Test 2
evidence
2.mp4 |
 |  |
Test 3
evidence
3.mp4 |
 |  |
Test 4
evidence
4.mp4 |
 |  |
Test 5
evidence
5.mp4 |
 |  |
Test 6
evidence
6.mp4 |
 |  |
Warning
The lane has no Blockstream Jade on USB or Bluetooth: the emulator declares neither android.hardware.usb.host nor android.hardware.bluetooth_le, the host exposes no serial port and no Bluetooth adapter, and the repo toolbox has no Jade emulator. The exact-head binary, both discovery scans, the unpaired Send and Receive paths and the 45-second background and reopen path were driven here; every assertion that needs the signer itself — pairing and PIN, account export, on-device address verification, signing and broadcast, and session reconnect — stands unverified.
Coverage:
QA: 0 of 6 manual tests passed
Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 1 file.
Counterpart synonymdev/bitkit-ios#765: not compared.
Findings:
N/A
Audit:
Already done in comment.
Coverage:
QA: journeys and manual tests await all reviewers to approve, author can run it now via comment: @ovi-reviewer test
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 2 files.
Counterpart synonymdev/bitkit-ios#765: not compared.
Findings:
N/A
Audit:
Prior audit.
Coverage:
QA: journeys and manual tests await all reviewers to approve, author can run it now via comment: @ovi-reviewer test
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
There was a problem hiding this comment.
Verdict: ✅ Approve
Tests for the review: 0 of 6 manual tests passed.
QA:
Tested on Android 15 emulator, regtest
Test 1
evidence
1.mp4 |
 |
Test 2
evidence
2.mp4 |
 |
Test 3
evidence
3.mp4 |
 |
Test 4
evidence
4.mp4 |
 |
Test 5
evidence
5.mp4 |
 |
Test 6
evidence
6.mp4 |
 |
Warning
No physical Blockstream Jade was available over USB or Bluetooth. The installed regtest build, both discovery scans, the unpaired Send and Receive paths, and the background/reopen path were exercised; pairing, PIN, account export, on-device address verification, signing and broadcast, and live-session reconnect remain untested.
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Only LOWs, both inline. Delta since 187fd196f is three master merges with no PR-side commits; the conflict resolutions match what you described, and every earlier thread is still fixed at head.
Checked and clean, traced through core and jade-client-rs:
- Signing.
check_signablerejects any sighash other than ALL/DEFAULT and requires a BIP32 origin matching the device fingerprint;verify_signed_psbtpinsunsigned_tx, the input and output counts and the UTXOs, and requires a gained signature.finalize_psbtre-pins the previous outputs, fails when any input lacks a valid signature, and re-checks the sighash types on the finalized satisfactions. What the device shows is the transaction that gets broadcast, and fewer signatures than inputs means nothing is broadcast. - Change. Composed app-side from the stored account xpub with key origin; the only device-sourced value is the fingerprint, and a wrong one makes Jade sign nothing.
- Identity. An entry is replaced only when the walletKey matches, a second device has a different id, and a walletId is adopted only on an exact xpub-set match. A null efuse fails closed. Equal seeds across vendors stay separate through
vendorWalletKey. - Transport. The BLE link is bonded, the PIN is entered on the device and the pinserver exchange is encrypted end to end inside jade-client-rs. No PSBT, xpub, fingerprint, PIN or signed transaction reaches a log. The two USB permission actions are distinct, so the pending intents do not collide.
- Mid-flow failure. The signed transaction is cached for both Send and Transfer, so a retry never re-signs, and the uniffi async calls let a timeout drop the Rust future while
cleanupFailedConnectioncloses the link underNonCancellable. - Trezor regressions. The changes there are vendor-scoped store access, the fingerprint passthrough and helpers moved to
KnownDevice.kt. Legacy rows default toTREZOR, so derived ids are unchanged, andReceiveTab.TREZOR → HARDWAREis a rename of an enum that is never persisted.
Design: no ### Design section or Figma link in the body, and the Jade illustration is a declared placeholder, so N/A — no design available. is the right line. No new *Screen.kt, so docs/screens-map.md is untouched.
Both findings likely apply to synonymdev/bitkit-ios#765 as well — the reconnect trigger exists over BLE there too — but it is conflicting, so I did not compare the code.
| runCatching { | ||
| withTimeout(HW_RECONNECT_TIMEOUT) { | ||
| // A Jade reconnect may include entering the PIN on the device, so the budget is per vendor. | ||
| withTimeout(hwWalletRepo.reconnectTimeout(walletId)) { |
There was a problem hiding this comment.
The Send-sheet fix from 65f2293fa was not carried to the transfer screen, so a locked Jade can trap the user for up to 5 minutes. LOW.
This timeout is now reconnectTimeout(walletId), which for Jade is JADE_RECONNECT_TIMEOUT = 5.minutes. isSigning is set before prepareSignedHardwareFunding, so isBusy stays true while ensureConnected waits for the PIN on the device. On SpendingHwSignScreen that removes system Back (BackHandler(enabled = state.isBusy) {}), the top-bar Back and the drawer icon at once, and onCloseClick only fires when feeSat == 0. A session failure retries ensureHardwareConnected, doubling the window.
Scenario: a paired Jade is powered on but locked, the user reaches the sign step, then changes their mind. Nothing in the app responds until the PIN is entered, the device is power-cycled, or five minutes pass. The Trezor path capped this at 30 s, and HwSendUiState.canLeave fixed exactly this for the Send sheet.
Fix: mirror the Send fix — an isConnectingDevice flag on TransferToSpendingUiState, set around ensureHardwareConnected, and gate Back and the drawer on isBusy && !isConnectingDevice. cancelHardwareTransfer already closes the link before the core session, so leaving releases the pending unlock.
There was a problem hiding this comment.
Confirmed, fixed in 28113c1 by mirroring the Send fix.
TransferToSpendingUiStategainsisConnectingDevice, set aroundensureHardwareConnected(both the first connect and the retry after a session failure), and acanLeaveof!isBusy || isConnectingDevice.SpendingHwSignScreengates system Back, the top-bar Back and the drawer icon oncanLeave. Leaving disposes the screen, socancelHardwareTransferruns and closes the link before the core session, which releases the pending unlock.- Signing and a pending broadcast still block leaving:
isConnectingDeviceis cleared before compose and sign, andcancelHardwareTransferstill returns early while a signed transaction is cached. - Like
HwSendViewModel, an attempt counter keeps a cancelled job'sfinallyfrom resetting the state of a transfer started after it, which leaving mid-connect makes reachable.
New tests in TransferViewModelTest.kt: hardware sign screen can be left while the device connects and hardware sign screen cannot be left while the device signs.
| "Connected Jade '${device.path}' firmware '${version.jadeVersion}' state '${version.jadeState}'", | ||
| context = TAG, | ||
| ) | ||
| rejectUnusableDevice(version, expected) |
There was a problem hiding this comment.
A Jade restored with a different seed silently adds a second wallet and keeps the old, unsignable one. LOW.
rejectUnusableDevice compares only the efuse MAC, and the next lines export accounts and call addOrUpdateKnownDevice on every unlocked connect, including a silent autoReconnect. When the fetched xpubs share nothing with the stored entry, previous is null and a new entry with a new walletId is persisted. isReplacedBy then returns false, because the clause that drops a re-seeded device relies on the Trezor device id regenerating on wipe — a Jade's efuse MAC never changes, so it can never fire.
Scenario: pair a Jade on seed A, wipe it, restore seed B, unlock it, then background and foreground Bitkit. A second wallet appears with no pairing step, and the seed-A tile stays, still shows its balance and still hands out receive addresses, while signing fails closed at ensureConnected. Both tiles are named "Jade". A user who no longer holds seed A can be given a seed-A address from an indistinguishable tile. The same applies to Jade's TEMP restore state, which isUnlocked() treats as unlocked.
Fix: in connectDevice, when expected != null and the exported xpubs share nothing with expected.xpubs, reject before addOrUpdateKnownDevice, so a reconnect fails closed like the efuse check and pairing seed B stays an explicit Add.
There was a problem hiding this comment.
Confirmed, fixed in 3c51337.
connectDevice now checks the exported xpubs against the expected entry before addOrUpdateKnownDevice. When a reconnect targets a stored entry that holds xpubs and none of them come back, it fails with HwWalletMismatchError and nothing is stored, so the old tile is not joined by a silent second wallet. This runs on every unlocked reconnect, including a silent autoReconnect of a Jade in the TEMP state. The failed connect goes through the usual cleanup, and connectExpectedDevice moves on to the next USB candidate as it does for an efuse mismatch. Pairing seed B stays an explicit Add, where expected is null. An entry with no stored xpubs is left alone, since there is nothing to compare against.
New test in JadeRepoTest.kt: reconnecting a jade restored with another seed adds no wallet.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 1dd08c6e2 (28113c190, 3c51337ed): no findings. Both LOWs are fixed.
- Leaving while the device connects.
isConnectingDeviceis set aroundensureHardwareConnectedonly, andcanLeave = !isBusy || isConnectingDevicenow drives Back, the top bar and the drawer. The window is safe on the retry path too:signHardwareFundingonly reconnects after a failed sign attempt, sopendingHwFundingBroadcastis still null there, andcancelHardwareTransferproceeds normally. Once the device is asked to sign, or a broadcast is pending,canLeaveis false again. - Attempt counter.
hwTransferAttemptstops a cancelled job'sfinallyfrom resetting a newer attempt's state, and the same guard protects theisConnectingDevicereset. - Re-seeded Jade.
rejectOtherWalletruns beforeaddOrUpdateKnownDevice, so a reconnect whose exported xpubs share nothing with the stored entry throwsHwWalletMismatchErrorand persists nothing. The retry loop treats it like the identity mismatch, so it does not spin, and pairing the new seed stays an explicit Add.
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 5 files.
New findings: 1 inline (non-blocking); the rest is in the review.
Coverage:
QA: waits for the other reviewers' approval, or @ovi-reviewer test
Reviewed by grok-4.7-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
| }.getOrElse { | ||
| it.rethrowIfCancellation() | ||
| if (it.isHwUserCancellation()) throw it | ||
| throw HardwareReconnectError(it) |
There was a problem hiding this comment.
ensureHardwareConnected wraps HwWalletMismatchError in HardwareReconnectError, and the transfer failure path shows lightning__transfer_hw__reconnect_error_description, which says the device is disconnected. The Jade is connected and its exported accounts do not match the paired wallet, so reconnecting the cable repeats the same rejection. Could we show the mismatch message when the cause is HwWalletMismatchError?
There was a problem hiding this comment.
Agreed, fixed in 8383cfe.
A new hardware__wallet_mismatch string says the device holds a different wallet than the one paired, and suggests connecting the paired wallet or adding this one as a new hardware wallet. handleHardwareTransferFailure checks the cause chain for HwWalletMismatchError before the reconnect branch, so the wrapped reconnect failure and the sign-time session check both show it instead of the disconnected message.
The Send sheet had the same gap in another form: its fallback showed the error's hardcoded English message. HwSendViewModel.handleFailure now shows the same string.
New tests: onTransferToSpendingHwConfirm shows wallet mismatch when the device holds another wallet in TransferViewModelTest.kt and a device holding another wallet shows the wallet mismatch in HwSendViewModelTest.kt.
…wallet # Conflicts: # app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt
|
Resolved the conflict in 6c08cfe, which merges current master. The only conflict was in |
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 5 files.
No new findings; the rest is in the review.
Retest suggested: Tests 2, 5 (Only Tests 2 and 5 cover HwSendViewModel.kt; the other routes were unchanged).
Coverage:
QA: waits for the other reviewers' approval, or @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta review of 8383cfe (and the 6c08cfe merge). One low-severity observation inline.
Checked:
HwWalletMismatchErroris thrown only after a successful connect, unlock and export whose xpubs share nothing with the stored entry (JadeRepo.rejectOtherWallet), or when the cached session's walletId differs (HwWalletRepo.signJadeFunding). A disconnected device goes down the session-failure retry path, not the mismatch toast, and a different physical Jade throwsJadeIdentityMismatchError. No misfire.- Send: the mismatch branch sits before the pending-broadcast and
elsebranches. It cannot run while a broadcast is pending, because that path skips signing. - Transfer: the early return happens before
HardwareReconnectError, whilependingHwFundingBroadcastis still null.finallyresets the signing and connecting flags, and cancel and Back still work. - Trezor never throws this error, so the new branches do nothing for Trezor.
- The merge resolved
TransferToSpendingUiStateby keeping bothcanLeaveandconfirmLeavingAmountSats.
Still open from the iOS side: the re-pair of a re-seeded Jade keeping the old tile (synonymdev/bitkit-ios#765 (comment)) applies here too, at KnownDevice.kt:70-76.
| description = context.getString(R.string.hardware__verify_address_error), | ||
| ) | ||
| else -> ToastEventBus.send(error) | ||
| else -> ToastEventBus.send( |
There was a problem hiding this comment.
Receive → Verify on Device still shows the raw English mismatch text.
Steps: pair a Jade on seed A, wipe it and restore seed B, then open the seed-A tile's Receive sheet and tap Verify on Device. verifyJadeReceiveAddress → ensureConnected → JadeRepo.rejectOtherWallet throws HwWalletMismatchError. handleVerifyFailure has no branch for it, so it falls into else, and HwErrorPresenter returns error.message: "A different hardware wallet is connected". That text is unlocalized and differs from the new hardware__wallet_mismatch copy that Send and Transfer now show.
Fix: add the same generateSequence(error) { it.cause }.any { it is HwWalletMismatchError } branch here, before isHwDeviceBusy().
This branch has not been deployed
2 participants
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.
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.