Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 5a907e097. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Traced every send entry at the PR head: main scanner and Send-sheet scanner (onScanResult), Send > Paste (onPasteClick), deep links (processDeeplink), clipboard auto-read and ShopWebView payment intents, manual address input (validateAddressWithFeedback plus AddressContinue, which re-runs launchScan), and Paykit contact payments (openContactPayment). All of them funnel through handleScan -> handleDecodedScan -> onScanLightning / onScanOnchain, and the own-invoice check runs there before handleQuickPayIfApplicable, waitForUsableChannels and canSend, so QuickPay (including the quickPayRepo.hasOpen re-entry) cannot pick up an own invoice. The unified bitcoin: path is covered in extractViableLightningInvoice, which now checks isOwnInvoice before the optimistic NodeLifecycleState.Stopped branch, so a remembered node id makes a stopped node fall back to on-chain rather than keep the own invoice. False positives: bitkit-core fills payeeNodeId via recover_payee_pub_key for every valid BOLT11 (src/modules/scanner/implementation.rs:278), so the compare is against the signature-recovered key, hex vs LDK's hex node id with ignoreCase; lastKnownNodeId is only ever written from lightningService.nodeId while Running and cleared in wipeStorage (called by WipeWalletUseCase.wipeLocal), and walletIndex is always 0, so it cannot carry another wallet's id into a fresh wallet. LNURL-pay resolves its invoice at pay time (fetchLnurlInvoice) and is not guarded, but that is pre-existing and ends in the LDK route error rather than a wrong payment. Timeout paths (awaitNodeId returning null) fail open to the pre-PR behaviour, which the author accepted in the greptile thread. No iOS twin PR found. One LOW (restart failure drops the remembered node id, so the guard fails open) is posted as a reply on the existing LightningRepo thread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head d3910ff20. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up on d3910ff: the earlier LOW (remembered node id dropped on restart) is fixed. Follow-up at d3910ff resolves the LOW posted on the LightningRepo thread (4046157262). setup() (LightningRepo.kt:294-311) no longer touches lastKnownNodeId, so a stop followed by a failed start (ErrorStarting via 364-377 or 431-434) keeps the remembered id and awaitNodeId() (1669-1672) / getLastKnownNodeId() (1667) still return it; the new test 'awaitNodeId should return last known id when restart after stop fails' reproduces exactly that sequence (start, stop, setup throws, ErrorStarting) and fails without the change because executeWhenNodeRunning fails fast on ErrorStarting.canRun() == false. The id is now written only from lightningService.nodeId while Running (1655-1657, 1672) and cleared only in wipeStorage() (827), which now runs before lightningService.wipeStorage so a failing native wipe still forgets it; when stopLocked() fails, mapCatching is skipped and the id is retained, but WipeWalletUseCase.wipeLocal returns before keychain.wipe() on that failure (WipeWalletUseCase.kt:91), so the mnemonic is unchanged and the retained id is still the active wallet's. Every mnemonic write is createWallet/restoreWallet (WalletRepo.kt:437-475), reachable only from the startup routes that MainActivity shows when !walletExists (MainActivity.kt:144), or the RN migration which only runs when no native mnemonic exists; every wipe entry (Settings reset, recovery mode, dev settings, ResetAndRestore) funnels through WipeWalletUseCase -> lightningRepo.wipeStorage, and start() refuses while isWiping (350) so no node can observe the old id between the LDK wipe and the keychain wipe. walletIndex is always 0 and only selects the storage path (LightningService.setup/build derive entropy from the keychain mnemonic), and the field is in-memory so a process restart resets it. The existing wipe test still passes with the reordered clear. No other lines changed in the delta, so the previously traced send entry paths in AppViewModel are unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nvoice # Conflicts: # journeys/README.md
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 8dd6b1dee. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: MEDIUM (journey, non-blocking) — Unified URI deep link is cut at the first &, so lightning= never reaches the app
Checked: Round 4 (0a83c99, new journey; the finding was re-checked against the journey text at head): Follow-up on 0a83c99 (own invoice guard journey). Every testTag the journey names exists at head: Receive (TabBar), SpecifyInvoiceButton (ReceiveQrScreen), ReceiveNumberPadTextField / ReceiveNumberPadSubmit / ShowQrReceive (EditInvoiceScreen), N1 and N000 (NumberPad, INTEGER pad under the default BITCOIN display), send_amount_screen and ContinueAmount (SendAmountScreen), SendConfirmToggleDetails and ReviewUri (SendConfirmScreen), QuickpaySettings (SettingsScreen); the toast title matches other__pay_self_invoice_title and the log line matches AppViewModel.kt:2187. The QR's content-desc is the raw QR content (QrCodeImage), and the default AUTO tab carries the bip21 with lightning= when a usable channel exists. The BOLT11 deep link steps would fail if the guard were reverted: without the isOwnInvoice check in onScanLightning the scan goes to handleQuickPayIfApplicable / canSend and opens Confirm or QuickPay (1000 sats is under the default $5 QuickPay threshold), so the toast and 'overview still visible' assertions fail. No step completes a payment: the Confirm step says not to swipe, and at head the QuickPay step is stopped by the guard. No bitkit-ios twin exists; the skip is recorded in the cross-platform table and the suite row is added. One MEDIUM on the unified-URI deep link quoting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head cb6531022. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up: Reviewed cb65310 only. The new step adb shell "am start -a android.intent.action.VIEW -d '<unified bitcoin: URI>' to.bitkit.dev" hands the host shell one argument that still carries the single quotes, so the device sh -c sees the URI (including ?, & and lightning=) as one word and the package argument survives. The follow-up assertion matches the code: extractViableLightningInvoice (AppViewModel.kt:2175-2208) drops an own lightning= param via isOwnInvoice() and logs "Skipped own lightning invoice in unified URI, defaulting to onchain" at AppViewModel.kt:2187, so the amount screen opens on-chain with SAVINGS; without that check the usable-channel path returns canSend true and the URI would route to Lightning, as the step now says. The two lightning:<invoice> steps keep bare double quotes, which is safe for a bech32 bolt11.
There was a problem hiding this comment.
Verdict: ✅ Approve
Review: diff 8 files.
Findings:
N/A
Audit:
Audited - no findings.
Coverage:
QA: journeys and manual tests await green CI checks
Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
|
@jvsena42 conflicts and e2e red |
|
Both points are covered centrally in #1275 (comment): the conflict is the shared I will confirm the result on this PR once the merge lands and CI re-runs on the new head. |
…nvoice # Conflicts: # journeys/README.md
…nvoice # Conflicts: # journeys/README.md
|
You were right — it is the missing companion e2e branch, not the change. The Created it: synonymdev/bitkit-e2e-tests#242, same commit as the others. Re-ran the failed job so it picks the branch up. |
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Already done in comment.
QA: emu-1 and emu-2 redroid Android 15 (API 35) on m5a-linux-2, exact-head dev E2E build e2e71e9
-
✅ passed: Own Lightning invoice was rejected with QuickPay off; Home remained visible and no payment screen opened.
-
✅ passed: Own Lightning invoice was rejected with QuickPay on; QuickPay never opened.
-
✅ passed: Pasting the own invoice and cold-opening it during node startup both showed Cannot Pay Own Invoice without opening QuickPay or Confirm.
-
✅ passed: Own unified URI fell back to Savings on-chain send and Confirm showed the wallet's own address.
-
✅ passed: Zero-Savings own unified URI showed Cannot Pay Own Invoice, while a foreign unified URI showed Insufficient Savings.
-
✅ passed: Foreign 2,000-sat invoice paid through QuickPay and reached Bitcoin Sent.
-
✅ passed: With QuickPay off, the foreign 2,000-sat invoice opened Confirm, paid after swipe, and reached Bitcoin Sent.
Tip
Test 1 worth a journey:
- Go to Receive
- Tap Edit and enter 1 000 sats
- Tap Continue and show the QR code
- Read the own Lightning invoice from the unified QR
- Return to Home
- Open the own Lightning invoice as a deep link
- Verify Cannot Pay Own Invoice appears
- Verify Home remains visible with no Send or Confirm screen
Test 2 worth a journey:
- Go to Settings
- Open QuickPay
- Enable QuickPay
- Return to Home
- Open the own Lightning invoice as a deep link
- Verify Cannot Pay Own Invoice appears
- Verify Home remains visible and QuickPay does not open
Test 3 worth a journey:
- Go to Receive and select Spending
- Copy the wallet's own Lightning invoice
- Return to Home and tap Send
- Tap Paste Invoice
- Verify Cannot Pay Own Invoice appears with no Amount or Confirm screen
- Force-stop Bitkit
- Cold-open the own Lightning invoice as a deep link
- Verify the toast appears after the node starts
- Verify QuickPay and Confirm never open
Test 4 worth a journey:
- Create a 1 000-sat unified receive QR
- Return to Home
- Open the own unified URI as a deep link
- Verify Send Amount shows 1 000 from Savings
- Tap Continue
- Tap Show Details
- Verify Confirm shows Savings and the wallet's own on-chain address
- Return to Home without paying
Test 5 worth a journey:
- Open an own unified URI on a wallet with zero Savings
- Verify Cannot Pay Own Invoice appears
- Verify Home remains visible with no payment screen
- Open a foreign unified URI on the same wallet
- Verify Insufficient Savings appears
- Verify Home remains visible with no Send Amount or Confirm screen
Test 7 worth a journey:
- Enable QuickPay on the funded wallet
- Create a 2 000-sat Lightning invoice on a second wallet
- Open the second wallet's invoice on the funded wallet
- Verify QuickPay pays without confirmation
- Verify Bitcoin Sent shows 2 000 sats
Test 8 worth a journey:
- Disable QuickPay on the funded wallet
- Create a fresh 2 000-sat Lightning invoice on a second wallet
- Open the second wallet's invoice on the funded wallet
- Verify Confirm shows 2 000 sats
- Swipe to pay
- Verify Bitcoin Sent shows 2 000 sats
Coverage:
Unit tests: 90% - New cases in AppViewModelSendFlowTest.kt and LightningRepoTest.kt cover every new guard and the remembered node id; the hardware-wallet arm of the zero-savings check has none.
QA: 7 of 7 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)
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Already done in comment.
QA:
Tested on emu-1 and emu-2 on SSH host m5a-linux-2
Tests 1-5, 7-8 already done in comment.
Coverage:
QA: 7 of 7 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)













Fixes #901
Related to #1275
This PR blocks paying a Lightning invoice created by the wallet's own node, showing a dedicated error instead of the misleading "Insufficient Spending" toast.
Description
bitcoin:URI, so the send falls back to on-chain when there are enough savings.Out of Scope
AppViewModelunified URI with own invoice: this PR falls back to an on-chain send when savings exist, which differs from verification step 4 of [Bug]: Self-payment on Lightning shows misleading "Insufficient Spending" error #901 (toast for unified URIs). The Lightning part cannot be paid, while the on-chain part is a valid self-send.AppViewModelunified URI with own invoice and savings above zero but below the amount: still shows "Insufficient Savings" instead of the self-payment error. Only the zero-savings case is covered; follow-up.AppViewModel.estimateLightningRoutingFeesIfNeeded/estimateMaxAmountRoutingFee: step 4 of [Bug]: Self-payment on Lightning shows misleading "Insufficient Spending" error #901 (mapping LDK's "Cannot generate a route to ourselves" to the self-payment toast) is not implemented. The payee check before these calls covers the case when the node ID is available.LightningRepo.awaitNodeId: in a process where the node has never run, no node ID was ever observed, so the guard is skipped and the existing LDK route error behaviour remains. Persisting the ID across process restarts needs per-wallet scoping in the cache store and is a follow-up. While the node is Starting and the ID is not yet known, a scan can wait up to 15s with no extra feedback.bitkit-ioswas not checked; a matching own-invoice guard there is a follow-up.Design
N/A — no design available.
The toast has no design. The on-chain fallback for a unified URI uses the standard Amount and Confirm screens; the resulting self-send activity is designed in Wallet Self Send with the note Self Send. Nothing was broadcast during testing, so those activity rows were not compared.
Preview
Recordings and screenshots from the regtest emulator on this branch (commit 2b2a3a6).
Own BOLT11 deep link, QuickPay off: toast shows and the app stays on Home.
own-bolt11-toast.mp4
Own BOLT11 deep link, QuickPay on: toast only, QuickPay does not open.
own-bolt11-quickpay-on-toast.mp4
Own unified URI deep link: falls back to the on-chain Amount and Confirm screens.
own-unified-onchain.mp4
Regression, foreign invoice with QuickPay on: QuickPay still runs; the payment then fails on purpose because the invoice has a fake route hint (no funds moved).
regression-foreign-quickpay.mp4
QA Notes
Manual Tests
Create the own invoice from Receive → Edit → 1000 sats → QR Code, and hand it to the app as a deep link (
adb shell am start -a android.intent.action.VIEW -d "lightning:<invoice>" to.bitkit.dev) or by scanning.regression:Scan foreign LN invoice with QuickPay on: QuickPay pays it.regression:Scan foreign LN invoice with QuickPay off: Send Confirm opens and payment succeeds.regression:Scan foreign unified QR with savings = 0: "Insufficient Savings" toast still shows when LN cannot be used.Journey covering tests 1a, 1b and 2a, committed at
journeys/send/own-invoice-guard.xml(row added to the suites and cross-platform tables injourneys/README.md). Inline for review:own-invoice-guard.xml
Automated Checks
AppViewModelSendFlowTest.kt: own LN invoice scan shows the self-payment toast without QuickPay orcanSend; foreign invoice still uses QuickPay; missing node ID does not block; scans wait for a starting node ID before QuickPay and before Confirm; unified URI with own invoice falls back to on-chain; foreign unified invoice keeps Lightning; manual input of own invoice shows the toast; own unified invoice with zero savings shows the toast on scan and manual input; foreign unified invoice with zero savings keeps the Insufficient Savings toast.LightningRepoTest.kt:awaitNodeIdreturns null without waiting when the node cannot run, waits for a starting node and returns its ID, returns null on timeout, returns the remembered ID after the node stops, and returns it when a restart after a stop fails and leavesErrorStarting;getLastKnownNodeIdreturns the remembered ID while the node is not running and null after a storage wipe.just compile,just test(2612 tests, 0 failed) andjust lintpass.