Repository navigation
fix: show Lightning invoice fee estimates - #1461
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Estimates the Lightning routing fee before opening confirmation for a fixed-amount invoice or a Paykit request, which previously skipped the estimate and showed only "Instant". Reviewed f597f9f, full tier, reasoned from the code and CI (Bitkit Android branches are not built on the review host).
What I checked, and 4 candidates I ruled out
Read in full: onScanLightning, estimateLightningRoutingFeesIfNeeded, launchScan, navigateToSendRoute, LightningRepo.estimateRoutingFees/estimateRoutingFeesForAmount/executeWhenNodeRunning, LightningService.estimateRoutingFees*, the three new tests and the updated Paykit test
Call sites traced: every estimateLightningRoutingFeesIfNeeded() caller (funding switch, amount continue, unified fixed-amount, this one)
CI: build, build-local, lint and detekt green; the e2e shards were still pending at review time
Ruled out
- Paykit request with a zero-amount invoice estimates for zero:
_sendUiStategetsamount = incomingPaymentRequest.amountSatsjust before the call, so theamountSatoshis == 0branch usesestimateRoutingFeesForAmountwith the request amount. The updated Paykit test pins that (4 sats). - Estimate failure blocks review: the helper only writes the fee in
onSuccess, and navigation follows unconditionally. Covered byfixed lightning invoice remains reviewable when routing fee estimation fails. - A stale estimate lands on a newer scan: the estimate runs inside the scan job, and the newer scan's job replaces it. Covered by
new scan cancels pending routing fee estimation before opening confirmation. - Confirmation now waits on the LDK queue (ruled out narrowly): the estimate runs on the single-thread
ServiceQueue.LDK, the same queue asnode.syncWallets(). A scan that lands during a wallet sync therefore opens confirmation only once the sync finishes. The unified fixed-amount path navigates first and estimates afterwards. I could not measure sync length from here, and the journey's regtest run would not show it. Worth a look on mainnet: scan a fixed invoice right after foregrounding the app.
Merge confidence: 4/5, the e2e shards had not finished and the queue-ordering candidate was ruled out only narrowly.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 90b8dd4: no code findings so far. Device result follows, and one timing question is left for it.
Checked:
- Estimated and paid are the same invoice and amount:
estimateRoutingFees(bolt11)for a fixed-amount invoice andestimateRoutingFeesForAmount(bolt11, amount)for a zero-amount invoice carrying a Paykit request amount (AppViewModel.kt:5559-5562);proceedWithPaymentpays the samedecodedInvoice.bolt11with the same amount (:4993-4994). A Paykit amount mismatch is rejected before this (:4110-4114). - Stale estimates: every scan starts from a fresh
SendUiState; a second scan cancels the first job before it reaches:4157;resetSendStateandhideSheetbumpsendStateGeneration, so a pending estimate neither writes (:5567) nor navigates (:4157) after Back, dismiss or a new Send sheet. Method switches bump the generation before starting their own estimate. - Failure: a failed estimate leaves
lightningFeeSatsnull, the review still opens with plain "Instant", no toast, and swipe-to-pay does not read the field. - The estimate is local pathfinding only (
bolt11Payment().estimateRoutingFees…); no probe is sent. - QuickPay returns before the estimate (
:4117-4123) and is unchanged. A zero-amount invoice without a request still goes to Amount and estimates on Continue as before. - iOS
masteralready shows the same row for these invoices (SendConfirmationView.swift:659-668), and the journey file is on synonymdev/bitkit-ios#884.
Left for the device: confirmation now opens only after the estimate returns (:4155-4158), with the scanner already closed and no loading state, where master opened it at once and iOS opens first and fills the fee in place. The estimate waits for the node to be running (up to a minute in executeWhenNodeRunning) and for the single LDK thread. I will time deep link to Confirm on this build and on master, warm and right after launch.
perf: relevant (a new wait before a primary screen).
Device gate (partial): 90b8dd4 — not run yet; pending lightning-fee-review.xml and the timing above.
|
Addressed the confirmation-timing concern from the latest reviews in 0d7d929. Fixed-amount Lightning invoices and Paykit requests now open confirmation first, then fill in the optional routing-fee estimate, matching the existing unified-invoice flow. The generation guard still prevents a delayed fee from touching a reset or replaced send flow. The strengthened regression holds the estimate unresolved, checks that confirmation is already visible with the correct amount and no fee, then completes it and verifies the displayed fee. It fails on the previous head (confirmation was still null) and passes with this change. Existing failure, scan-replacement, reset and Paykit amount tests remain green. Validation: compilation and all 3,948 unit tests pass, including 394 send-flow tests. The full Detekt/formatting report matches the verified 1,981-finding baseline with zero introduced findings and none intersecting edited lines. Independent final review is clean. Cold-start/mainnet device timings have not been measured in this pass. |
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 0d7d929. The code scan of the earlier commits is in my review from today; I read 0d7d929 on top of it and ran the device gate.
0d7d929 answers the timing question I left open: confirmation opens first and the estimate fills the fee in place. The stale-result guard is still inside estimateLightningRoutingFeesIfNeeded (generation captured before the call, checked in the state update), and the estimate still runs inside the scan job, so a newer scan cancels it.
Device gate: 0d7d929 — journeys: 1 passed (lightning-fee-review.xml, 9 of 9 actions; actions 1–5 also at 90b8dd4); perf: cold-start lightning: deep link to confirmation 15.6 s and 12.1 s (master 12.0 s and 12.1 s; 90b8dd4 20.3 s and 21.9 s), fee row fills in about 8 s later on a cold start; warm estimate 2 ms; Figma: not compared (the change is the text of an existing row); iOS: not run, the journey file is on the draft synonymdev/bitkit-ios#884.
Pixel 9 emulator as payer (100,000-sat channel through the staging LSP), a tablet emulator as payee, regtest. Both the invoice and the Paykit request showed Instant (± ₿ 1) with Spending and the right recipient, and neither review created a payment.
Cold-start timing, log lines (UTC)
90b8dd4: the estimate queued behind the first wallet sync, and nothing was on screen until it returned.
13:40:53.914 DEBUG [AppViewModel.kt:2890] Starting scan from 'deeplink'
13:41:00.350 INFO [LightningService.kt:419] Node started
13:41:01.363 INFO [AppViewModel.kt:4153] Found amount in invoice, proceeding with payment
13:41:01.364 INFO [LightningRepo.kt:2273] Estimating routing fees for bolt11: lnbcrt10u1p4vec42…
13:41:05.874 INFO [ldk_node::chain::electrum:821] Sync of Lightning wallet finished in 5527ms.
13:41:08.510 INFO [ldk_node::chain::electrum:433] Incremental sync of primary on-chain wallet finished in 2413ms.
13:41:09.416 INFO [LightningRepo.kt:2275] Routing fees estimated: '1'
0d7d929: same 7.6–7.8 s estimate, but the review is already open (Instant, then Instant (± ₿ 1)).
13:46:09.392 DEBUG [AppViewModel.kt:2890] Starting scan from 'deeplink'
13:46:17.494 INFO [AppViewModel.kt:4153] Found amount in invoice, proceeding with payment
13:46:17.496 INFO [LightningRepo.kt:2273] Estimating routing fees for bolt11: lnbcrt10u1p4vec42…
13:46:25.144 INFO [LightningRepo.kt:2275] Routing fees estimated: '1'
One note on the journey, no change requested: I did not find a control on the request screen for "Spending as its only accepted payment method", so I sent a plain 1,000-sat request; the payer's review defaulted to Spending.
Skipped — covered by CI at 0d7d929: unit tests, detekt, lint (green). Four e2e shards were still running when I posted this.
This PR shows available routing-fee estimates when a Lightning invoice or Paykit request opens payment confirmation with a set amount.
Description
Out of Scope
Design
Payment request review.
Preview
Native Android checks passed on the integrated USDT build: both a fixed-amount invoice and a Paykit request show “Instant (± 1 sat)” in Show Details. Both reviews were canceled without submitting payment.
QA Notes
Journeys
lightning-fee-review.xml— fixed-amount invoices and Paykit requests show a fee estimate before approval; both reviews are canceled without payment. Matching journey is included in feat: add usdt wallet and paykit payments bitkit-ios#884.Manual Tests
N/A
Automated Checks
AppViewModelSendFlowTest.kt— verifies confirmation opens while estimation is pending, invoice and Paykit amount-based estimates, review after estimation failure, scan replacement, and send-flow reset. The delayed-estimate regression fails on the previous PR head and passes after moving estimation behind navigation.