feat: add blockstream jade hardware wallet - #765
coreyphillips wants to merge 21 commits into
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…wipe The foreground Trezor reconnect was launched in a task that only marked the session active once it ran, so a Jade operation taking the lock in between could dial alongside it. The reconnect now registers before the lock is released and is cancelled when the session is released. A wipe only tore down the Jade, so a Trezor reconnect finishing after the wipe could save its device back. The Trezor session is now released and its loaded devices dropped before the wipe clears storage.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ⛔️ Request Changes
Review: diff 89 files.
Counterpart synonymdev/bitkit-android#1231: equivalent.
Findings:
2 inline (2 blocking)
Audit:
Awaits QA.
Coverage:
Unit tests: 85% - The 26 changed test files cover Jade transport, sessions, storage, routing, presentation, connect, send, and transfer paths, but not the two lifecycle races.
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
Reaudit: diff 6 files.
Counterpart synonymdev/bitkit-android#1231: not compared.
Findings:
N/A
Audit:
Skipped - no security surface in the diff (score 0, threshold 3).
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)
addressed - reaudit confirmed
There was a problem hiding this comment.
No HIGH/MEDIUM findings, and no LOW worth posting.
Checked:
- Funds. The amount and address on
HwSendSignVieware the values passed tosignAndBroadcast. The fee shown comes from the compose that produced the PSBT. The PSBT is built app-side from the stored xpub, and the device contributes only the fingerprint. Jade results go throughfinalizePsbt(originalPsbt:signedPsbt:), and core 0.5.17's guards pin the unsigned tx and each input'sprevious_outputand runinterpreter_check. No double broadcast:pendingPaymentcaches the signed tx per request, re-sign happens only when no result reached the app, andcancel()is a no-op onceisBroadcastUnresolved. The transfer path does the same viapendingHwFundingBroadcast. - Device identity.
requireJadeSession(holding:)runs beforesignPsbt.rejectUnusableDevicefails closed on a nil efuse MAC. Known-device reconnects are efuse-checked before unlock, and the advertised name is only a hint. - Key material. No xpub, PSBT, fingerprint or PIN in logs.
HwKnownDeviceonly gainsvendorandjadeDeviceId. The backup snapshot is still{walletId: customLabel}. - Upgrade from Trezor-only state. Storage keys are unchanged. A missing
vendordecodes as Trezor unless the id has thejade:prefix, and unknown vendors are preserved on rewrite.bitkit-core0.5.14 → 0.5.17 adds only Jade code and the newfinalize_psbtguards, with no storage or migration change. - Lifecycle. A pairing failure before
addOrUpdateKnownDevicestores nothing.resetForWipecovers both vendors ahead ofremovePersistentDomain, and theconnectEpochbump stops a late connect from persisting after a wipe. Both greptile threads are fixed at head. - Parity with synonymdev/bitkit-android#1231. Signing identity check, efuse nil check, and the Receive tab not tearing down a live Trezor session: present. Activity-teardown cache and USB-attach disconnect: absent, since iOS has no activity lifecycle or USB. Locked Jade with no exit from Send:
canLeavecovers it.
Not verified: device-only behaviour, i.e. link release during on-device confirm at 30s in background, and firmware dropping the confirm screen. That needs a real Jade and iPhone (manual tests 5b/8).
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 6 files.
Counterpart synonymdev/bitkit-android#1231: not compared.
Findings:
N/A
Audit:
Already done in comment.
QA:
Tested on sim-1 iPhone 17 Pro simulator on iOS 26.5
Test 1
Test 2
Test 3
Test 4
Test 5
Test 6
Test 7
Test 8
Test 9
Test 10
Test 11
Test 12
Test 13
Approve.
Coverage:
Unit tests: 100% - Focused tests cover synchronous reconnect activity, cross-vendor release, wipe cancellation, both-vendor reset, and loaded-device clearing.
QA: 0 of 13 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)
Warning
Thirteen of thirteen manual tests need a physical Blockstream Jade and a Bluetooth-capable iPhone, and the simulator has neither: pairing, PIN handling, on-device address verification, signing and broadcast stand unverified here.
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Already done in review.
QA:
Tested on iOS 26.5 simulator (iPhone 17 Pro)
Test 1
Test 2
Test 3
Test 4
Test 5
Test 6
Test 7
Test 8
Test 9
Test 10
Test 11
Test 12
Test 13
Warning
No physical Blockstream Jade, Trezor, or compatible iPhone was available on the host. All 13 physical-device checks remain unverified; the exact-head simulator build launched but cannot substitute for either signer. Tests 5 and 6 above correspond to the checklist's nested 5a and 5b, with the later tests in PR order.
Approve.
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
|
@coreyphillips conflicts |
…s pin The Send sheet already allowed leaving while the device was being reached. The transfer sign screen kept Back and the drawer disabled for the whole reconnect, which for a locked Jade can last up to five minutes. Track the connecting phase and gate navigation on it, so leaving cancels the connect and releases the link.
A Jade restored with another seed keeps its efuse MAC, so a reconnect stored the new wallet with no pairing step and left the old one looking usable. A reconnect now fails closed when the exported keys match no wallet paired on that device; adding the new wallet stays an explicit pairing.
…wallet # Conflicts: # Bitkit.xcodeproj/project.pbxproj # Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
One LOW inline. The two commits that port my android findings both check out.
b3879ac9.isConnectingDeviceis set only inside the twoensureConnectedbrackets, andcanLeaveHwSigndrives the navigation bar, so Back is available while the device waits for its PIN and disabled again once it is asked to sign or a broadcast is out. Leaving tears the link down throughbeginStaleSessionTeardown, which bumps the connect epoch and closes the transport without waiting for the 5-minute unlock. The Send path already had the equivalent throughcanLeave.5c3c9420. The xpub-overlap rejection runs before anything is written, and on throw the connect tears down with no store write. A silent reconnect of a re-seeded Jade is rejected when unlocked, and when locked it only refreshes the expected entry's path.
Parity note: android also added an hwTransferAttempt counter so a cancelled job cannot reset a newer attempt's flags. TransferViewModel here resets unconditionally in its defer. I could not build a reachable clobber — every device phase is inside withTimeout, whose cancellation unwinds on the same main-actor turn as cancelHwSigning(), and the one await that is not cancellation-aware runs while isConnectingDevice is false — and the send path already has signingAttempt. Worth knowing the two sides differ.
Checked and clean:
- Signing.
finalizePsbtpins the unsigned transaction and every previous output, fails when an input lacks a valid signature, runs the interpreter check and accepts onlySIGHASH_ALL/ Taproot default. The broadcast bytes are that finalized transaction, and the session is pinned to the wallet before signing. - Change. Composed by core with
[fingerprint/path]xpuborigins on both keychains, with the fingerprint read from the live device at compose time, so change is derived from the pinned account xpub and the device can verify it as its own. - Shown vs broadcast. The Send view's amount and address are the values passed to sign-and-broadcast, the fee is refreshed from the composed PSBT, and the transfer path pins order, wallet, address and fee in
PendingHwFundingBroadcast, so a retry never re-signs. - Transport. No secret crosses the app: the PIN is typed on the device and the pinserver exchange happens in core. No PSBT, xpub, fingerprint or PIN reaches a log. A dropped response fails the waiters, and the retry after a reconnect produces the same txid; a broadcast timeout keeps the pending payment for a retry rather than re-signing.
- Trezor. The shared-code changes are renames plus session helpers; legacy entries without a vendor decode as Trezor, storage keys are unchanged and unknown-vendor entries survive a rewrite.
Still device-only: the 30 s background release during an on-device confirm, and firmware dropping the confirm screen. Those need a physical Jade.
| } | ||
| guard let knownTrezorId = known.trezorDeviceId, let entryTrezorId = entry.trezorDeviceId else { return false } | ||
| return entryTrezorId != knownTrezorId | ||
| guard let knownHardwareId = known.hardwareId, let entryHardwareId = entry.hardwareId else { return false } |
There was a problem hiding this comment.
An explicit re-pair of a re-seeded Jade keeps the old, unsignable wallet forever. LOW, and shared with the android twin rather than new here.
connect(path:) passes expected: nil, so 5c3c9420's reject is deliberately skipped and addOrUpdateKnownDevice reaches isReplaced. There, the vendor and the id match, the walletKeys differ, refreshed is nil, and the hardware ids are equal — because a Jade's hardware id is its efuse MAC, which survives a wipe — so it returns false and the seed-A entry stays. The comment says wallets of a seed the device no longer carries go too, and for Trezor that holds because its device id regenerates on wipe.
After wiping a Jade and restoring seed B, Home shows two Jade tiles. The seed-A tile still shows its balance, Send and Transfer from it fail every time with "reconnect hardware device", and Receive reports a mismatch for an address that is actually correct. Nothing is lost, but it is a dead end with no explanation.
Fix: when expected == nil and previous == nil, drop the other entries sharing this id whose xpubs are disjoint from the fresh export, or give isReplaced a vendor hook for "same hardware id, disjoint xpubs on an explicit pairing". synonymdev/bitkit-android#1231 has the same gap, so it is worth fixing on both.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The master merge took the old build setting, which says Bitkit does not use Bluetooth. With GENERATE_INFOPLIST_FILE on, that setting is what ships, so set it to the same sentence as Info.plist for Debug and Release.
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 1 file.
No new findings; the rest is in the review.
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)
addressed - reaudit confirmed
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta review of fed7027 (and the ab3d949 merge). No findings.
Checked:
INFOPLIST_KEY_NSBluetoothAlwaysUsageDescriptionis set in both configurations of the Bitkit target (Debug and Release). These are the only two configurations in the project; E2E is a scheme, not a configuration.Info.plistcarries the same sentence, so there is no conflicting text. The key was present in every commit; this commit restores the wording the merge replaced.- The merge took master's side for
project.pbxprojandPackage.resolved: CodeScanner removed, bitkit-core 0.5.18, which contains the Jade module and the PSBT finalize guards. The project uses synchronized root groups, so no file references were lost.
Parity note, non-blocking: synonymdev/bitkit-android#1231 now shows a dedicated wallet-mismatch message when a re-seeded Jade is used from the old tile (commit 8383cfe89). iOS JadeManager.rejectUnpairedWallet still throws "Reconnect Hardware Device" for that case, which is the wording the Android thread objected to.
My earlier LOW at HwKnownDeviceMatching.swift:64 (re-pairing a re-seeded Jade keeps the old tile) is still unaddressed.
3 participants
This PR:
It is the iOS counterpart to bitkit-android#1231 and requires bitkit-core 0.5.17, which carries the Jade module (synonymdev/bitkit-core#153).
Description
A Jade can now be paired from Connect Hardware, unlocked with the PIN entered on the device, and used like a paired Trezor: watch-only balances, on-device receive address verification, and on-device signing for both a 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 Bluetooth and the UI that drives the flows.
Differences from Android worth knowing:
Tab-trezorfor a Trezor wallet (unchanged) andTab-jadefor a Jade. bitkit-android#1231 renames its tag toTab-hardwarefor both, while the shared helper in bitkit-e2e-tests and Android'sReceiveHardwareFlowTeststill tapTab-trezor.Out of Scope
Bitkit/Assets.xcassets/Illustrations/jade-placeholder.imageset: the Jade illustration is a placeholder vector until design supplies the real asset.journeys/hardware-wallet: no Jade journeys, since there is no Jade emulator inbitkit-dockerand the simulator has no Bluetooth.Design
N/A: no design available.
Preview
Recording of the Bluetooth pair, verify and sign flow to be attached from a physical Jade.
QA Notes
Manual Tests
These need a physical Jade and a physical iPhone, since the simulator has no Bluetooth.
regression:Trezor → pair, Receive → Verify on Device, Send and Transfer to Spending: all work as before, and the receive tab reads "Trezor".regression:Trezor connected → background and reopen Bitkit: the Trezor reconnects.Automated Checks
JadeManagerTests.swift: pairing, unlock, account export with the taproot fallback, identity checks, reconnect by advertised name, silent reconnect, the reconnect loop, cancel and teardown ordering, the disconnect notice, and background release. It ports the Bluetooth-relevant cases of Android'sJadeRepoTest.kt.JadeTransportTests.swift,JadeBLELinkStateTests.swiftandBlockingQueueTests.swift: chunk sizing, read timeouts, close waking every waiter, error codes, the stale bond message, and that Bluetooth stays off until a Jade is used.JadeServiceTests.swift:finalizePsbtreaches core instead of recursing.JadeDeviceIdentityTests.swift: device ids, models, advertised name matching and network mapping.HwWalletManagerVendorTests.swift: vendor routing, one vendor session at a time, the Jade signing identity check, on-device verification, the Jade fingerprint on compose, passphrase refusal for a Jade, per-vendor reconnect timeouts, and foreground reconnect.HwErrorPredicateTests.swiftandHwErrorPresenterTests.swift: Jade error classification and copy.HwEngagedSessionTests.swift: the receive sheet releases only a session it used.TrezorManagerVendorIsolationTests.swift: Trezor loads, renames and forgets never touch a Jade entry.HwWalletVendorPresentationTests.swift: vendor assets, copy and the receive tab identifier.HwKnownDeviceMatchingTests.swiftandHwKnownDeviceStorageTests.swift(renamed from their Trezor names): vendor slices, migration of entries saved before this change, and entries of an unknown vendor being kept.HwConnectViewModelTests.swift: Jade discovery, cancel and unlock hint.HwFundingSignerTests.swift: when the send sheet can be left, cancel release, and the attempt counter.TransferViewModelHwTests.swift,HwWalletIdTests.swift,HwWalletNameTests.swift,HwWalletManagerTests.swiftandHwWalletManagerPassphraseTests.swift.node scripts/validate-translations.jsreports 0 errors, and SwiftFormat passes on the changed files.