Skip to content

feat: add blockstream jade hardware wallet support - #1231

Open
coreyphillips wants to merge 32 commits into
masterfrom
feat/jade-hardware-wallet
Open

coreyphillips wants to merge 32 commits into
masterfrom
feat/jade-hardware-wallet

Conversation

@coreyphillips

@coreyphillips coreyphillips commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

This PR:

  1. Adds Blockstream Jade as a second hardware wallet vendor, over USB and Bluetooth
  2. Generalises the hardware wallet layer so every device call is routed by the vendor of the paired entry
  3. Releases an open Jade Bluetooth link while Bitkit is backgrounded and reconnects silently on return

Requires bitkit-core 0.5.16 (synonymdev/bitkit-core#153), which carries the Jade module and pins jade-client-rs at d52ccd9.

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:

  • The TX characteristic is indicate-only on this firmware, so the transport subscribes to
    indications when notify is absent instead of failing the connection.
  • A Jade advertises under a new random Bluetooth address after every reboot or pairing reset, so a
    stored entry is recognised by name, which is Jade plus the last six hex digits of its efuse MAC,
    rather than by address.
  • A link left open when the process dies wedges the device's single connection slot until it is
    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.
  • A bond that went stale across a re-pair used to stall the first write. It now gets a write budget
    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:

  • Cancelling pairing or receive verification explicitly cancels Jade work and closes the native transport in cleanup that survives caller cancellation.
  • Every failed post-connect step closes both the Android transport and core session.
  • A known USB reconnect checks each attached Jade until its efuse identity matches, but stops immediately if the expected Jade itself fails.
  • The shared repository enforces one active vendor session and checks both vendor states before Bluetooth scans.
  • Equal seeds on different vendors keep separate wallet identities, labels, watchers, and signing routes.
  • Signing on a Jade checks that the connected session belongs to the wallet being spent from, as the Trezor path does.
  • Finishing the activity releases the Jade transport, core session and cached connection together, off the main thread.
  • The receive sheet closes a hardware session only if it used the device, and a USB attach for a vendor with nothing paired no longer drops the other vendor's session.
  • The send sheet can be dismissed while it connects or waits for a Jade PIN; it stays locked while the device signs and while a broadcast is unresolved.

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, so
these are all physical-device checks.

Manual Tests

  • 1. Jade over USB → Connect Hardware → Search → Pair: PIN prompt shows on the device,
    unlock completes, accounts export and the wallet tile appears.
  • 2. regression: USB → Send → pick the Jade source → sign on device → broadcast:
    transaction confirms.
  • 3. Jade over Bluetooth → Connect Hardware → Search → Pair: pairs and unlocks.
  • 4. Bluetooth → Receive → Hardware tab → Verify on Device: the address shown on the Jade
    matches the one in the app.
  • 5. Bluetooth → Send → sign on device → broadcast: broadcast succeeded, txid
    d955bc0c....
  • 6. Bluetooth connected → background Bitkit for about 45 seconds → reopen: reconnects with
    no pairing prompt and no PIN re-entry.

Automated Checks

  • Unit tests added: JadeTransportTest.kt covers USB driver selection, the CP210x and CDC open and
    close sequences, chunk sizing and read and write timeouts; JadeRepoTest.kt covers 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.kt
    covers the finalizePsbt alias that used to recurse into itself; HwUsbIdTest.kt covers vendor
    detection from USB ids; KnownDeviceTest.kt covers vendor-aware entry matching, migration of
    pre-Jade entries, wallet identity and equal-seed vendor isolation; HwErrorPresenterTest.kt and HwExceptionExtTest.kt cover
    the Jade error copy and classification. The Bluetooth GATT paths themselves, including the
    indicate-only fallback, are not unit testable and were validated on hardware.
  • Unit tests modified: HwWalletRepoTest.kt, HwConnectViewModelTest.kt, HwSendViewModelTest.kt,
    HwReceiveViewModelTest.kt, TransferViewModelTest.kt, TrezorRepoTest.kt and
    ReceiveInvoiceUtilsTest.kt move onto the vendor-neutral device state and the per-vendor routing.
  • Local: just compile, just test (2626 tests, 0 failures) and just lint all pass.

@coreyphillips
coreyphillips marked this pull request as ready for review September 15, 2026 13:19
@greptile-apps

greptile-apps Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 3/5

The PR is not yet safe to merge because activity teardown can leave a stale Jade session and Jade signing can run against a session that changed after composition.

Findings

  1. P1 Transport Cleanup Leaves Stale Session ▶
  2. P1 Jade Signing Skips Identity Check ▶

Summary

This PR adds Blockstream Jade and Jade Plus support over USB and Bluetooth, generalizes hardware-wallet state and operation routing across vendors, adds Jade-specific pairing, unlocking, verification and signing flows, and introduces lifecycle-based Bluetooth release and reconnect behavior.

  • Adds CP210x, CDC, and Nordic UART transports for Jade.
  • Persists vendor-scoped hardware identities and keeps legacy entries mapped to Trezor.
  • Routes discovery, connection, address verification, transaction composition, and signing by vendor.
  • Adds vendor-specific UI, errors, USB filters, and extensive unit coverage.
  • Two lifecycle/session-boundary defects remain in transport teardown and Jade signing.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    UI[Hardware wallet UI] --> HWR[HwWalletRepo]
    HWR -->|vendor = Trezor| TR[TrezorRepo]
    HWR -->|vendor = Blockstream| JR[JadeRepo]
    JR --> JS[JadeService / bitkit-core]
    JS --> JT[JadeTransport]
    JT --> USB[CP210x or USB CDC]
    JT --> BLE[Nordic UART BLE]
    APP[Process lifecycle] --> HWR
    ACT[MainActivity teardown] -. currently bypasses repository .-> JT
Loading

Reviews (1) · Last reviewed commit: "chore: bump bitkit-core to 0.5.16"

Comment thread app/src/main/java/to/bitkit/ui/MainActivity.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt Outdated
- 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
@github-actions

github-actions Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 8e5b9e5 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: signPsbt feeds the device's return to finalizePsbt(originalPsbt, signedPsbt). In bitkit-core v0.5.16 src/modules/onchain/psbt.rs, :55-59 rejects original.unsigned_tx != signed.unsigned_tx, :61-73 pins each input's previous output, then finalize_mut + interpreter_check verify the signatures against the pinned inputs. The device can contribute signatures and nothing else.
  • Trezor: does not go through finalizePsbt — it broadcasts serializedTx directly. It is protected instead by trezor-connect-rs 0.4.0, which derives expected_scripts before signing and then runs verify_signed_tx on the device-returned bytes (src/tx_verify.rs:23-73, a port of @trezor/connect's verifyTx): output count, every output amount, every output scriptPubKey against independently derived expectations. Equivalent guarantee, different code path. Worth knowing if anyone later assumes finalizePsbt covers 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 other signTxFromPsbt/broadcastRawTx user 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 ReceiveSheet.kt (:156). #1248 adds no new 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.

Comment thread app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt
Comment thread app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt
@jvsena42 jvsena42 mentioned this pull request Sep 15, 2026
4 tasks
- 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 ovi-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment thread app/src/main/java/to/bitkit/repositories/JadeRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/JadeRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt Outdated
coreyphillips and others added 3 commits September 16, 2026 12:38
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.
ovi-reviewer[bot]

This comment was marked as resolved.

ovi-reviewer[bot]

This comment was marked as resolved.

…wallet

# Conflicts:
#	gradle/libs.versions.toml

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⚠️ skipped: hardware absent in env

evidence
1.mp4

Test 2 ⚠️ skipped: hardware absent in env

evidence
2.mp4

Test 3 ⚠️ skipped: hardware absent in env

evidence
3.mp4

Test 4 ⚠️ skipped: hardware absent in env

evidence
4.mp4

Test 5 ⚠️ skipped: hardware absent in env

evidence
5.mp4

Test 6 ⚠️ skipped: hardware absent in env

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)

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve


Tests for the review: 0 of 6 manual tests passed.

QA:
Tested on Android 15 emulator, regtest

Test 1 ⚠️ skipped: hardware absent in env

evidence
1.mp4

Test 2 ⚠️ skipped: hardware absent in env

evidence
2.mp4

Test 3 ⚠️ skipped: hardware absent in env

evidence
3.mp4

Test 4 ⚠️ skipped: hardware absent in env

evidence
4.mp4

Test 5 ⚠️ skipped: hardware absent in env

evidence
5.mp4

Test 6 ⚠️ skipped: hardware absent in env

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_signable rejects any sighash other than ALL/DEFAULT and requires a BIP32 origin matching the device fingerprint; verify_signed_psbt pins unsigned_tx, the input and output counts and the UTXOs, and requires a gained signature. finalize_psbt re-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 cleanupFailedConnection closes the link under NonCancellable.
  • Trezor regressions. The changes there are vendor-scoped store access, the fingerprint passthrough and helpers moved to KnownDevice.kt. Legacy rows default to TREZOR, so derived ids are unchanged, and ReceiveTab.TREZOR → HARDWARE is 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.

Comment thread app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/JadeRepo.kt

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta since 1dd08c6e2 (28113c190, 3c51337ed): no findings. Both LOWs are fixed.

  • Leaving while the device connects. isConnectingDevice is set around ensureHardwareConnected only, and canLeave = !isBusy || isConnectingDevice now drives Back, the top bar and the drawer. The window is safe on the retry path too: signHardwareFunding only reconnects after a failed sign attempt, so pendingHwFundingBroadcast is still null there, and cancelHardwareTransfer proceeds normally. Once the device is asked to sign, or a broadcast is pending, canLeave is false again.
  • Attempt counter. hwTransferAttempt stops a cancelled job's finally from resetting a newer attempt's state, and the same guard protects the isConnectingDevice reset.
  • Re-seeded Jade. rejectOtherWallet runs before addOrUpdateKnownDevice, so a reconnect whose exported xpubs share nothing with the stored entry throws HwWalletMismatchError and 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.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreyphillips

Copy link
Copy Markdown
Contributor Author

Resolved the conflict in 6c08cfe, which merges current master. The only conflict was in TransferViewModel.kt: master added confirmLeavingAmountSats to TransferToSpendingUiState and this branch added canLeave in the same place. Kept both.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta review of 8383cfe (and the 6c08cfe merge). One low-severity observation inline.

Checked:

  • HwWalletMismatchError is 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 throws JadeIdentityMismatchError. No misfire.
  • Send: the mismatch branch sits before the pending-broadcast and else branches. It cannot run while a broadcast is pending, because that path skips signing.
  • Transfer: the early return happens before HardwareReconnectError, while pendingHwFundingBroadcast is still null. finally resets 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 TransferToSpendingUiState by keeping both canLeave and confirmLeavingAmountSats.

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.

@coreyphillips

Copy link
Copy Markdown
Contributor Author

@jvsena42 on the re-pair of a re-seeded Jade keeping the old tile (KnownDevice.kt:70-76, from the iOS thread): confirmed, fixed in f38d724, with one narrowing.

An explicit pair (Add, where expected is null) of a Jade that reports READY now drops every stored Jade entry on the same efuse MAC whose xpubs share nothing with the fresh export. The check uses the hardware id rather than the entry id, so a seed-A entry paired over the other transport goes as well. The removal happens in the same write that adds the new entry, and the watchers reconcile from the known devices the way they do for a replaced Trezor entry.

The narrowing: a Jade in the TEMP state keeps the old entries. A temporary session runs a seed on top of the stored one, so the stored seed is still there and its wallet is still signable. The known trade-off is an on-device BIP39 passphrase: nothing the Jade reports tells it apart from a wipe, so pairing the passphrase wallet replaces the standard one. The entries are watch-only, so pairing the standard wallet again brings it back and no funds are at risk.

Reconnects (expected set) still fail closed through rejectOtherWallet and never remove anything.

New tests: pairing a jade restored with another seed drops the old seed's wallet and pairing a jade in a temporary session keeps the stored seed's wallet in JadeRepoTest.kt, and a jade entry holds another seed only when the same hardware shares none of its keys in KnownDeviceTest.kt.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ⛔️ Request Changes


Reaudit: diff 6 files.
New findings: 1 inline (1 blocking); the rest is in the review.

Coverage:
Unit tests: 60% - Receive mismatch and Jade re-seed/TEMP paths have tests; the READY passphrase pairing that deletes a still-signable wallet has no test.
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)

Comment thread app/src/main/java/to/bitkit/repositories/JadeRepo.kt Outdated

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f38d724 deletes a wallet the user still holds. Pairing a second BIP39-passphrase wallet on the same Jade removes the first one, with its activity, tags and label, and shows no prompt. Details are on the existing thread at JadeRepo.kt:773. My earlier ask to drop the old wallet on re-pair led to this; iOS kept both entries for this reason, and Android should do the same. Funds are not at risk on-chain, but that wallet's history and metadata are lost, and its balance is hidden until the passphrase is paired again. No flag gates Jade pairing.

f4083bc: fixed. The mismatch branch runs before isHwDeviceBusy() in handleVerifyFailure, with the same hardware__wallet_mismatch copy as Send and Transfer, and a test covers it.

One low-severity follow-up inline: the unlock re-check iOS added in 24f66088.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ⛔️ Request Changes


Reaudit: diff 4 files.
New finding: 1 inline; see the review for the rest.

Coverage:
Unit tests: 65% - Pairing and unlock mismatch tests cover individual paths, but no test reconnects both retained same-ID passphrase wallets after the session closes.


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)

)
// Another seed on the same Jade is kept: an on-device passphrase looks exactly like a wipe, and its
// wallet is still signable. A stale one is removed from Settings, where its data can be kept.
val updated = knownDevices.filterNot { it.isReplacedBy(known, refreshed = previous) } + known

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When passphrase wallet B is paired, A is kept, preserving its metadata. Since both entries share the same id, after the session closes, HwWalletRepo.reconnect(B) passes that ID to JadeRepo.connectKnownDevice. knownDevice(id) selects A, so rejectOtherWallet rejects B's valid, disjoint xpubs. ensureConnected(B) does the same when no session is live. A test could pair A and B, close the session, then reconnect each. Could we carry the requested wallet ID into the expected-entry lookup?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. a7a0e7a adds the same root cause on the locked-session path. ensureConnected (:295) checks the unlocked export only against the entry the locked reconnect trusted. That reconnect always lands on the first entry (knownDevice(id), :668). Steps: passphrase wallet B paired after A. The app is backgrounded and foregrounded, and the reconnect lands locked on A. The user opens B → Verify and enters B's passphrase. rejectOtherWallet(Bxpubs, A) throws HwWalletMismatchError, and cleanupFailedConnection drops the link. The toast then tells the user to add B as new, although B is paired, and every retry fails the same way. iOS c9e898a4 re-targets instead: HwKnownDeviceMatching.previous runs over all same-id entries, adoptPairedWallet follows, and ensureJadeConnected re-checks the wallet id.

One fix covers both legs. rejectOtherWallet accepts any same-id entry whose xpubs overlap and returns it. ensureConnected sets connected.walletId to that entry. Then HwWalletRepo.ensureConnected refuses an operation for A when the Jade opened B, ideally with HwWalletMismatchError rather than the generic AppError at :459-460.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, both legs. Fixed in 8e5b9e5 with both suggestions combined.

  • The requested wallet is carried into the entry lookup. JadeRepo.connectKnownDevice, ensureConnected and warmUpKnownDevice take an optional walletId, and knownDevice(id, walletId) prefers the same-id entry holding that wallet before falling back to the first. HwWalletRepo.reconnect, ensureConnected and warmUpKnownDevice pass it, and autoReconnect passes the entry it picked.
  • rejectOtherWallet is now openedWallet: it accepts any same-id entry whose xpubs overlap the export and returns it, and still throws HwWalletMismatchError when none do. A re-seeded Jade is still refused and its link closed.
  • After a locked reconnect, ensureConnected sets connected.walletId to the wallet the unlock opened. So if the reconnect landed on A and the user unlocks with B's passphrase, the session becomes B instead of failing with the "add as new" copy.
  • HwWalletRepo.ensureConnected and reconnect refuse a session holding another wallet with HwWalletMismatchError, replacing the generic AppError.

New tests: reconnecting opens each paired wallet sharing one jade (pairs A and B, reconnects B, closes the session, then reconnects A) and ensureConnected follows a locked session into another paired wallet on the same jade in JadeRepoTest.kt, and ensureConnected reports a mismatch when the jade opened another paired wallet in HwWalletRepoTest.kt.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at 8e5b9e5. knownDevice(id, walletId) prefers the entry holding the requested wallet. openedWallet follows a locked A session into B when B's passphrase is typed, and requireWallet now reports HwWalletMismatchError instead of the generic error. The two new JadeRepo tests cover both legs. Resolved on my side.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de6ddd7: fixed. Pairing another passphrase wallet keeps the first one with its activity, tags and label, matching iOS HwKnownDeviceMatching.merged.

a7a0e7a: fixed for a re-seeded Jade. The unlocked export is checked and the session is closed on a mismatch. It still shares the root cause of the open thread at JadeRepo.kt:775: with two paired passphrase wallets on one Jade, B cannot be reached after a locked reconnect lands on A, and now shows the "add as new" copy. I added the trace and the iOS c9e898a4 fix shape to that thread rather than opening a new one. MEDIUM, ungated.

Checked: cancel during the new export goes through disconnectStaleSession or cancelPendingConnection, and isConnectingDevice resets in both the cancel functions and the attempt-guarded finally. HwWalletMismatchError is not a session failure, so the Send and Receive retry loops do not re-dial on it.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked 8e5b9e5. No findings.

Clean: every sign goes through HwWalletRepo.ensureConnected(walletId) → requireWallet, and signJadeFunding checks isSessionOf(walletId), so a session opened as A cannot sign or verify for B. Receive addresses derive from stored xpubs of devicesForWallet(walletId). openedWallet/addOrUpdateKnownDevice match on xpub intersection within the same transport id, so disjoint seeds cannot be conflated. isReplacedBy keeps B when A refreshes. A typo passphrase still throws and cleans up without adding an entry. Cancel during exportAccounts leaves the locked session intact. Cross-transport reconnect (A on Bluetooth live, B on USB only): bitkit-core v0.5.18 JadeManager::connect closes the previous session before opening the new one, so nothing leaks. No shipped Jade state needs migration.

@jvsena42

Copy link
Copy Markdown
Member

Pending manual test on hardware wallet

This branch has not been deployed

No deployments
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