Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
 [Medium impact] The PR appears safe to merge; the latest change fixes Android Back without changing the existing toolbar behavior. SummaryAmount-bearing Bitcoin requests open Confirm directly, and amount-bearing USDT links request a quote on entry.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[Bitcoin request with payable amount] --> Confirm
Confirm -->|Toolbar or Android Back| Amount
Amount -->|Continue| Confirm
Amount -->|Back| Origin[Previous screen or close sheet]
Reviews (2) · Last reviewed commit: "fix: route system back on direct confirm..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Added to the 3.2.0 milestone: this fix was opened after the 3.1.0 cut and closes no issue, so this PR carries the milestone itself. |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. On-chain BIP21 with a payable amount now opens Confirm (Back swaps it for Amount), and a USDT payment link with an amount quotes once on entry. Reviewed 5e95204, full tier, static: the branch was not built or run for this review, so the evidence is the code and CI.
What I checked, and 7 candidates I ruled out
Read in full: the diff; onScanOnchain, onAmountContinue, prepareCoinSelection, prepareHardwareSendFee, validateAmount, refreshOnchainSendIfNeeded, navigateToSendRoute, resetSendState in AppViewModel.kt; the Confirm and Amount routes and navigateToSendAmount in SendSheet.kt; UsdtWalletScreen.kt, UsdtViewModel.quote, UsdtRepo.quote, PaykitAmount.parse.
Call sites traced: openOnchainConfirmForInvoiceAmount (one, after the balance and dust checks), navigateToAmountFromConfirm, Routes.UsdtPayment (built only by handleUsdtPaymentInput).
CI: build-local and Greptile green; build-and-test, lint, compose-ui and the local e2e shards still running when this was posted.
Ruled out
- Confirm skips a step Amount's Continue does: for auto coin selection
prepareCoinSelectionreturns true with no side effect,selectedUtxosis already null after the reset, the hardware fee is prepared before navigating, andrefreshOnchainSendIfNeededfollows; Lightning routing estimation does not apply to an on-chain pay method. - Manual coin selection or a contact payment reaching Confirm without the picker: both return false before any state change and fall through to Amount, covered by the manual-coin-selection test.
- Back loops or strands the sheet: from the main scanner the stack becomes
[Amount]and Amount's Back hides the sheet; from the send sheet it becomes[Recipient, ..., Amount]; after Continue, Back pops to Amount through the existingpopBackStack(SendRoute.Amount)branch.canReturnToAmountlives inSendUiState, so every new send clears it throughresetSendState. - Amount loses the invoice amount on the way back:
SendAmountScreenseeds its input fromuiState.amountwhen it is positive. - A hardware fee estimate failure: it toasts and returns false, then the scan falls through to Amount, the same place it went before.
- USDT quote repeats on rotation or process death:
isRequestedis saveable and the effect is keyed onUnit, so Back, Edit and a restore never quote again;canQuoteguards a busy state. - USDT amount string rejected by the quote: the route amount is
PaykitAmount.value(a plain dot decimal), which bothPaykitAmount.parseInputandUsdtRepo.quoteaccept, the same string the Review button sends.
Merge confidence: 4/5, not built here and CI still running; the e2e send suite expects Amount for this route until the bitkit-e2e-tests change named in the PR lands.
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
System Back on a Confirm opened straight from a system-back-from-confirm.mp4 |
coreyphillips
left a comment
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
needs changing before merge
- E2E send shard breaks: e2e main still expects Amount for BIP21 with amount (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:5402)
worth doing, does not block
- USDT journey clipboard step still says the amount screen opens (journeys/usdt-payment-links-and-scans.xml:6)
- Hardware-wallet direct-Confirm path has no test (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:5426)
- Tapping the amount on direct Confirm does nothing (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:5434)
- The PIN journey expects Home one Back press too early (journeys/security/pin-lock-on-resume.xml:45-46)
nits
- KDoc on a private function (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:5415-5418)
| return | ||
| } | ||
|
|
||
| if (amount > 0u && openOnchainConfirmForInvoiceAmount(amount, fromMainScanner, walletGeneration)) return |
There was a problem hiding this comment.
needs changing before merge: E2E send shard breaks: e2e main still expects Amount for BIP21 with amount
This change makes @send_1 and @send_2 on bitkit-e2e-tests main fail, and nothing lets CI pick up an updated spec. How the E2E workflow behaves: - .github/workflows/e2e.yml runs on every non-draft PR that touches app/**. - default-feature-branch resolves to an e2e branch with the same name as the app branch, and falls back to main otherwise. - e2e-status fails when any shard fails. - synonymdev/bitkit-e2e-tests has no fix/scan-amount-to-confirm branch (gh api repos/synonymdev/bitkit-e2e-tests/branches/fix%2Fscan-amount-to-confirm returns 404), and the PR body has no Companion: line. What the specs on e2e main expect: - send.e2e.ts:157-169 (@send_1) enters bitcoin:bcrt1q07x3…?amount=0.0001 and taps AddressContinue. It then calls verifyAmountToSend(10_000) and waits for ContinueAmount. - send.e2e.ts:356-366 (@send_2 @ios_gate) enters an on-chain 0.0002 BIP21. Its 200 000 sat Lightning invoice is above Spending. It then waits for AssetButton-savings and taps ContinueAmount. With this PR, both inputs reach openOnchainConfirmForInvoiceAmount (AppViewModel.kt:5402) and open Confirm instead. The PR's own test send sheet onchain address with payable amount navigates to confirm asserts exactly that. How I confirmed it: I downloaded send.e2e.ts from e2e main and traced both inputs through onScanOnchain. I also checked the branch lookup and the workflow's ref resolution. The PR's live send shard (run 41648112580) had not finished when I checked at 14:40 UTC, so I have not seen the failure itself. The author's Gaps section predicts the same break. Once this merges, the send shard fails on every later Android PR that touches app/** until e2e main is updated. Fix: push the spec update to fix/scan-amount-to-confirm in bitkit-e2e-tests so this PR's E2E run picks it up, link it as Companion:, and merge the two in lockstep. @send_2 is also an iOS gate, so coordinate with the iOS twin too.
| <action>Enable USDT in the introductory sheet or Settings, General, Payments before starting this flow.</action> | ||
| <action>Open an ethereum: USDT receive link with chain 42161, the pinned Arbitrum token, an owned recipient and uint256=10001. Verify the USDT amount screen preserves the recipient and exact input 0.010001 USDT; the review rounds its displayed value to $0.01. Verify no payment occurs without approval.</action> | ||
| <action>Open an ethereum: USDT receive link with chain 42161, the pinned Arbitrum token, an owned recipient and uint256=10001. Verify the link opens on payment review without tapping Review, preserving the recipient and exact input 0.010001 USDT; the review rounds its displayed value to $0.01. With a USDT balance below the amount plus fees, verify the amount screen stays open with the same insufficient-balance or fee error that tapping Review shows. Go back to the recipient and continue again; verify the review is not requested again until Review is tapped. Verify no payment occurs without approval.</action> | ||
| <action>Copy the USDT receive URI, return to Home and accept the clipboard prompt. Verify the USDT amount screen opens with the copied recipient and amount. Cancel without paying. Repeat with a wrong-chain URI and verify no payment prompt appears.</action> |
There was a problem hiding this comment.
worth doing, does not block: USDT journey clipboard step still says the amount screen opens
Step 6 of usdt-payment-links-and-scans.xml is stale. The clipboard prompt goes through handleUsdtPaymentInput, which navigates to Routes.UsdtPayment(recipient, amount) (AppViewModel.kt:4942). Links use the same route, so InitialUsdtQuoteEffect now quotes on entry for a copied URI too. With enough USDT, a copied URI that carries an amount opens on review. Step 6 still says "Verify the USDT amount screen opens with the copied recipient and amount". The PR updated step 2 for links but not this step. Reword it the way step 2 now reads: the review opens, or the quote error shows on Amount.
| ): Boolean { | ||
| if (_sendUiState.value.payMethod != SendMethod.ONCHAIN || hasActiveContactPaymentContext()) return false | ||
| if (!validateAmount(amount)) return false | ||
| val isHardware = _sendUiState.value.hardwareWalletId != null |
There was a problem hiding this comment.
worth doing, does not block: Hardware-wallet direct-Confirm path has no test
openOnchainConfirmForInvoiceAmount (AppViewModel.kt:5426-5430) has one new branch that does real work before navigating. For a hardware wallet it calls prepareHardwareSendFee(). If that fails, it shows the error toast and falls through to Amount. None of the added tests sets a hardware wallet. The PR also says the Trezor flow was not driven on a device. The existing suite already stubs hwWalletRepo.estimateFundingMiningFee (for example AppViewModelSendFlowTest.kt:6489). That makes two cheap tests possible: - A successful estimate opens Confirm with the fee in onchainFeeUi. - A failed estimate opens Amount.
| /** | ||
| * An on-chain invoice with a payable amount opens on Confirm, as Amount's continue would. | ||
| * Manual coin selection and contact payments keep going through Amount. | ||
| */ |
There was a problem hiding this comment.
nit: KDoc on a private function
openOnchainConfirmForInvoiceAmount is private but has a KDoc block. CLAUDE.md says "NEVER add code comments to private functions".
|
|
||
| Logger.info("Found payable amount in invoice, proceeding to confirm", context = TAG) | ||
| _sendUiState.update { it.copy(isAmountInputValid = true, canReturnToAmount = true) } | ||
| navigateToSendRoute(fromMainScanner, SendRoute.Confirm, SendEffect.NavigateToConfirm, walletGeneration) |
There was a problem hiding this comment.
worth doing, does not block: Tapping the amount on direct Confirm does nothing
Open a funded BIP21 request with an amount, then tap the amount on Confirm. Nothing happens: BackToAmount still emits PopBack(Amount), but this shortcut creates no Amount entry. The amount tap needs the same fallback as the Back arrow.
| <action>Verify the Send Confirm screen opens (title "Confirm", amount testTag "ReviewAmount") showing 10 000 sats and the Receive sheet (testTag "ReceiveScreen") is gone</action> | ||
| <action>Press back to close the Send sheet and verify the wallet home screen is visible</action> |
There was a problem hiding this comment.
worth doing, does not block: The PIN journey expects Home one Back press too early
J8's final assertion fails under the new navigation. After unlocking opens Confirm, one Back press now opens Amount, but the next action expects the sheet to close and Home to appear. The journey needs another Back press before checking Home.
pwltr
left a comment
pwltr
left a comment
There was a problem hiding this comment.
Reviewed commit 1d280b6c59a51ede602149b453981acf160656b7 through general, security and cross-platform source review, followed by targeted finding verification.
[P2] Tapping the amount cannot edit it after opening confirmation directly
For a valid amount-bearing Bitcoin request with Autopilot coin selection, the new flow opens Confirm without adding Amount to the navigation stack. Tapping the amount still emits BackToAmount, which becomes PopBack(Amount). SendSheet.kt only calls popBackStack for that effect, so the tap silently does nothing when Amount is absent.
The new fallback is wired to toolbar/system Back, which remains a workaround, but the amount-edit control does not use it. Route that control through the same fallback, or explicitly open Amount when it is missing. This is an introduced navigation regression confirmed by source tracing; it does not itself submit a payment.
Review details
General review
Model: Claude Opus 5.5
Reasoning effort: xhigh
Security review
Model: Kimi K3
Reasoning effort: max
Cross-platform review and verification
Model: GPT-6.1-sol
Reasoning effort: xhigh
This PR:
Motivation, from the Slack report:
Description
fixed-price-bitcoin.xmland the payment-requests README, which still described the Amount detour.Out of Scope
viewmodels/AppViewModel.kt: a unified BIP21 whose Lightning invoice has no amount but whose on-chain part does keeps opening Amount on Spending.viewmodels/AppViewModel.kt: manual coin selection keeps going through Amount before the coin picker; opening the picker straight from a scan needs a sheet route the scan path cannot target today.send.e2e.tsexpectations for on-chain BIP21 with an amount are updated in test: expect review screen for bip21 with an amount bitkit-e2e-tests#290.Design
N/A — no design available.
Preview
On-chain BIP21 with an amount, opened as a link, goes straight to Confirm; Back opens Amount with the same amount; Continue returns to Confirm:
onchain-amount-to-confirm.mp4
The same URI entered through Send → Enter Manually opens Confirm:
onchain-amount-manual-entry.mp4
BIP21 without an amount still opens Amount:
onchain-no-amount-to-amount.mp4
Manual coin selection still goes through Amount and then the coin picker:
onchain-amount-manual-coins.mp4
USDT link with an amount: after Enable USDT, the quote runs on entry and the insufficient-balance error appears without tapping Review (the wallet has no USDT, so the success review could not be recorded):
usdt-amount-auto-quote-error.mp4
Fixed 2.50 USD Paykit request with USDT off goes from Preparing straight to Confirm at 2 500 sats:
paykit-usd-request-to-confirm.mp4
QA Notes
Setup
Dev regtest build from this branch on an emulator, against the staging regtest backend the dev flavor targets, with a wallet holding about 700 000 sats in Savings and a Lightning channel. Coin Selection on Autopilot. The Paykit cases used the
paykit-fixture-senderfrom bitkit-e2e-testsmain, linked to a Pubky profile created on the payer. USDT cases enable USDT through the link's prompt; the wallet has no USDT balance.Journeys
scan-onchain-amount-to-confirm.xml— BIP21 with an amount opens Confirm directly, Back (arrow and system Back) opens Amount with the amount, over-balance shows the insufficient savings toastscan-onchain-no-amount-to-amount.xml— BIP21 without an amount, or with amount 0, still opens Amountusdt-payment-links-and-scans.xml— a USDT link with an amount opens review without tapping Review, or shows the quote error on Amountfixed-quoted-confirmation.xml— a fixed USD request accepting Bitcoin and USDT opens Bitcoin Confirm with USDT offfixed-price-bitcoin.xml— fixed-price requests open confirmation directly with 21 000 and 50 000 satsonchain-accepted-result.xml— the fixed-amount send starts on Confirmrbf-own-send.xml— the boosted send starts on Confirmpin-lock-on-resume.xml— the payment URI opened while locked opens Confirm after unlocksignin-with-second-link.xml— the payment link opens Confirm once the sign-in sheet closesManual Tests
N/A
Automated Checks
UsdtWalletScreenTest.kt— the initial quote runs only with a recipient and a positive, valid USDT amountAppViewModelSendFlowTest.kt— on-chain BIP21 with an amount opens Confirm from the main scanner and from Send; zero, dust and manual coin selection open Amount; a fixed USD request accepting USDT opens Bitcoin Confirm while USDT is offSendSheetNavigationTest.kt— Back from a Confirm opened by an invoice amount replaces it with Amount, from both start destinations; Back from a first-screen Confirm opens Amount only when it can return to AmountEvidence
Verified on a Pixel 9 emulator (API 37) with the dev debug build of this branch:
amount=0.01shows the Insufficient Savings toast. A cold-start link waits through Connecting to network and then opens Confirm.KEYCODE_BACK) on a link-opened Confirm at 10 000 sats opens Amount at 10 000; a second Back closes the sheet. Before, it closed the sheet. Video in the PR comments.bitcoin:<addr>and?amount=0open Amount at 0; entering 10 000 continues to Confirm.uint256opens an empty Amount with no quote.usdandbtcpresets open Confirm at 21 000 and 50 000 sats.amount=0.00001opens Confirm at 1 000 sats with Savings and the recipient../gradlew testDevDebugUnitTest(4 861 tests),./gradlew detektand./gradlew lintDevDebugpassed locally.Gaps
signin_grantURLs. Its Confirm step follows the same PIN-gated link route verified in J8.send.e2e.tswould break:@send_1tapsAddressContinueforbitcoin:bcrt1q07x3…?amount=0.0001and then expects Amount (verifyAmountToSend(10_000),ContinueAmount);@send_2sendsunified2(on-chain 0.0002 with a 200 000 sat Lightning invoice above the Spending balance) and expectsAssetButton-savingsandContinueAmount.Models used
claude-opus-5-5(reasoning: Not exposed)claude-opus-5-5(reasoning: Not exposed)The reasoning setting is not exposed in this session's metadata.
🤖 Generated with Claude Code