feat: add trezor receive - #1189
Conversation
f75f183 to
48273d1
Compare
Greptile SummaryAdds hardware-wallet receiving to the existing receive sheet, including watcher-backed address resolution, BIP21 editing, and on-device Trezor verification.
Confidence Score: 4/5The PR should not merge until receive amount and note state is scoped so Savings edits cannot silently alter the Trezor payment request. The Trezor QR directly consumes the same BIP21 metadata updated by Savings editing, causing a request for one account to be displayed for another account with unintended payment details. Files Needing Attention: app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveQrScreen.kt, app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveSheet.kt
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt | Adds watcher-backed receive-address lookup, account-scan fallback, typed watcher address data, and identity-aware Trezor address verification. |
| app/src/main/java/to/bitkit/ui/screens/wallets/receive/HwReceiveViewModel.kt | Coordinates address loading, watcher updates, verification retries, passphrase prompts, cancellation, and user-facing errors. |
| app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveQrScreen.kt | Adds the Trezor tab and its actions, but incorrectly sources its amount and note from primary-wallet BIP21 metadata. |
| app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveSheet.kt | Wires hardware receive navigation and editing into the shared receive sheet, including the shared metadata path implicated in the finding. |
| app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveInvoiceUtils.kt | Builds hardware-wallet BIP21 requests and selects the blue Bitcoin QR logo. |
| app/src/main/java/to/bitkit/ui/screens/wallets/receive/EditInvoiceScreen.kt | Adds an on-chain-only editing path that avoids Lightning receive work and hides tags. |
Sequence Diagram
sequenceDiagram
participant U as User
participant UI as Receive Sheet
participant VM as Hardware Receive VM
participant HW as Hardware Wallet Repo
participant W as Watcher
participant T as Trezor
U->>UI: Open hardware-wallet receive
UI->>VM: loadAddress(walletId)
VM->>HW: getReceiveAddress(walletId)
HW->>W: Read next unused address
alt Watcher address available
W-->>HW: Address and derivation path
else Watcher state unavailable
HW->>T: Scan public account
T-->>HW: First unused address
end
HW-->>VM: Hardware receive address
VM-->>UI: Display QR and details
U->>UI: Verify on device
UI->>VM: verifyAddress()
VM->>HW: verifyReceiveAddress()
HW->>T: Display address at derivation path
T-->>HW: Derived address
HW-->>VM: Match or mismatch
Reviews (1): Last reviewed commit: "feat: add trezor receive" | Re-trigger Greptile
48273d1 to
8be651e
Compare
ovitrif
left a comment
There was a problem hiding this comment.
The receive flow can complete verification for an address that is no longer displayed. I also found two smaller gaps in the Details actions and editor coverage.
e9afa2a to
0456645
Compare
6de987e to
09a681e
Compare
0456645 to
39230eb
Compare
09a681e to
f5f2c24
Compare
39230eb to
57adcbb
Compare
f5f2c24 to
5ef1318
Compare
57adcbb to
85564d9
Compare
5ef1318 to
80d47e3
Compare
85564d9 to
2b795f4
Compare
|
PLS add QA notes and Manual Tests + Automated Tests sections with details like we usually do for apps PRs. (this is more about formatting than content, content is a bit already there, just not structured in the same format we usually do) |
2b795f4 to
89daec5
Compare
|
Updated the PR description with structured QA Notes, Manual Tests, and Automated Tests sections. I left the manual checks unchecked until they are run on Android. |
0cf15db to
7ca35a3
Compare
89daec5 to
8aa95f3
Compare
a968811 to
de7fb9f
Compare
8aa95f3 to
1a7a0bd
Compare
9484f78 to
93cda1b
Compare
1a7a0bd to
b5be4ba
Compare
ovitrif
left a comment
There was a problem hiding this comment.
The global hardware edit path can return on a software-wallet QR. I also found misleading passphrase copy and two untested recovery paths.
| isEditingHardwareInvoice.value = true | ||
| navController.navigateTo(ReceiveRoute.EditInvoice) | ||
| }, | ||
| initialTab = if (hardwareWalletId != null) ReceiveTab.TREZOR else null, |
There was a problem hiding this comment.
I see the global one-wallet flow resolves selectedHardwareWalletId, but initialTab stays null because no explicit hardwareWalletId was supplied. After the user selects TREZOR, opens the hardware editor, and pops back, the ReceiveRoute.QR destination is recreated and the state stored with remember defaults to SAVINGS (then may auto-switch to AUTO), so the edited request is shown for the software wallet instead of the Trezor address. Could we preserve the selected receive tab across navigation or return to TREZOR when isEditingHardwareInvoice initiated the editor?
| } | ||
|
|
||
| if (hwReceiveState.isPassphraseRequired) { | ||
| HwPassphrasePromptSheet( |
There was a problem hiding this comment.
I see this receive-only branch reuses HwPassphrasePromptSheet, whose body reads hardware__passphrase_sign_text and says the device will “sign the transaction.” Address verification does not sign anything, so hidden-wallet users are shown a misleading spend-authorization prompt before a read-only check. Could we pass receive-specific body copy that explains the passphrase is reopening the wallet to verify its address?
| val response = if (firstError == null) { | ||
| firstAttempt.getOrThrow() | ||
| } else { | ||
| if (!firstError.isTrezorSessionFailure()) throw firstError |
There was a problem hiding this comment.
I couldn't find coverage for the session-failure retry branch in verifyReceiveAddress: HwWalletRepoTest exercises watcher lookup, scan fallback, and final address mismatch, but never makes the first getAddress call fail with a session error. If the disconnect, reconnect, or second device read regresses, the claimed recovery path fails while the current tests still pass. Could we add a focused test that makes the first device read fail with a session error and verifies reconnect plus a successful second read?
| } | ||
| } | ||
|
|
||
| fun retryAddress() { |
There was a problem hiding this comment.
I don't see coverage that makes getReceiveAddress fail and then exercises retryAddress; HwReceiveViewModelTest only covers successful loading, cancellation, watcher updates, and passphrase verification. If the new addressLoadFailed state or retry behavior regresses, users can remain stuck without an address while the current tests still pass. Could we add a focused test that fails the first load, asserts addressLoadFailed, retries, and verifies the recovered address?
Description
This PR:
Stack
QA Notes
Manual Tests
regression:Start Verify on Device → while device approval is pending, fund the displayed regtest address and advance the watcher: the stale verification is cancelled and Bitkit displays and copies the new receive address.Automated Tests
HwWalletRepoTest.kt: cover watcher-backed receive-address selection, stored-xpub fallback, and device-address mismatch rejection.HwReceiveViewModelTest.kt: cover address loading and reset, passphrase reconnection, live watcher-address updates, and cancellation of stale verification.ReceiveInvoiceUtilsTest.kt: cover the hardware-only BIP21 destination, shared amount and note details, and zero-amount omission.EditInvoiceContentTest.kt: verifies the on-chain-only hardware edit callback and hidden tag actions.receive-onchain.xml: defines the Trezor tab, QR, details, and exact on-device address-verification flow.testDevDebugUnitTestsuite pass; lint and detekt pass.git diff --checkpasses.