Repository navigation
fix: show ordinary Lightning results without Paykit waits - #1684
ben-kaufman wants to merge 2 commits into
Conversation
|
Triage: 3.1.0. It fixes the shipped Lightning payment result screen, and 3.1.0 is due 28 Oct. |
[Medium impact] The PR appears safe to merge; ordinary results skip unrelated work without changing request-specific proof handling. SummaryOrdinary Lightning failures and pending results now skip unrelated Paykit proof work. Payments tied to an incoming request keep their existing checks and proof handling.
Reviews (1) · Last reviewed commit: "fix: show ordinary Lightning results wit..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. For a Lightning send that has no incoming Paykit request, the result screen no longer waits on proof cleanup or reconciliation: a failure skips failLightningPayment, and a pending result skips refreshIncomingPaykitPaymentRequests(). Reviewed 3b95acd, light tier (7 changed production lines), stacked on #1681 and reviewed as the delta against fix/usdt-recovery-parity.
What I checked, and 4 candidates I ruled out
Read in full: proceedWithLightningPayment, clearFailedLightningPayment, associateLightningPaymentProof, preparePaymentProof, refreshIncomingPaykitPaymentRequests, handlePaymentFailed, handlePaymentSuccessful in AppViewModel.kt; failLightningPayment (both overloads), associateLightningPayment and reconcile in PaykitPaymentProofRepo.kt; the two new tests in AppViewModelSendFlowTest.kt.
Call sites traced: every failLightningPayment( and associateLightningPayment( caller in app/src/main, and every refreshIncomingPaykitPaymentRequests() call.
CI when read: build-local and lint green; build and the local e2e shards were still running. Not built here: on this track the branch's Gradle code would run with a write token in its environment, so CI is the gate.
Ruled out
- An ordinary failure leaves a stale Lightning proof:
associateLightningPaymentProofreturns before writing anything whenrequest == null, andpreparePaymentProofdoes too, so this flow never creates a proof keyed to that hash. The node'sPaymentFailedevent still runsfailLightningPayment(paymentHash)off the send path inhandlePaymentFailed. - An allowance payment on the same hash loses its cleanup:
PaykitAllowanceExecutorassociates its proof and fails it itself, throughpayer.failLightningPayment(paymentHash, it), so it does not depend on this branch. - A pending ordinary payment no longer brings up the next request: the refresh dropped from this path also runs after
PaymentSuccessfulandPaymentFailedand on the polling loop, and nothing on the ordinary pending path changes request state.trackandpreserveContactPaymentContextstill run first, so the pending hash and the contact context are kept. - Reordering the
whenchanges how request payments are handled: whenisPaymentRequestis true, the guard is skipped andLightningPaymentFailedErrorand the two-argumentfailLightningPaymentrun as before. The guard now comes beforeLightningPaymentFailedError, but that arm and the old!isPaymentRequestarm both returnedtrue, so the only difference is the dropped cleanup call.
Merge confidence: 4/5, build and e2e were still running when this was read and the branch was not built here; the change itself is covered by the two new AppViewModelSendFlowTest.kt cases.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the runtime does not report it to the reviewer)
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Dependency: #1681
Companion: synonymdev/bitkit-ios#1120
Description
An ordinary Lightning payment could already be failed or pending in the node while its result screen waited for unrelated Paykit proof maintenance. Return the node-derived result without that unrelated work when there is no incoming payment request. Pending payments retain their original hash and contact context; actual Paykit requests keep their authorization, claim and proof handling.
Out of Scope
No payment is retried by this change. Full-wallet backup separation, pending-restore admission and the separate iOS receive/startup waits are follow-up work. No historical contact incident attribution.
Design
N/A — no design available. Existing result screens and copy.
Preview
Existing error and pending screens appear when the node result is known.
QA Notes
Setup
Use the committed Debug dependencies (Core 0.8.0-rc11, Paykit 0.1.1-rc1, VSS 0.5.23). The deterministic repository tests inject an unrelated held proof cleanup/reconciliation and controlled ordinary node failure/pending results. No funds or services are needed for those tests. J1 requires disposable test-network wallets with synthetic funds, a usable Lightning channel, a controlled failing/hold invoice and a service fixture that delays only an unrelated retained proof. This full service fixture is not provisioned; never use live funds.
Journeys
ordinary-during-paykit-outage.xml— ordinary receive/startup/payment-result behavior while unrelated Paykit services wait. Native component tests cover the two changed Android payment-result steps; the complete journey remains unrun.Manual Tests
N/A — use J1 when its controlled service prerequisites are available.
Automated Checks
AppViewModelSendFlowTest; unchanged production fails both. Candidate focused suite: 504 passed.just compile,just test,just lint: passed. Complete unit suite: 4,606 tests across 254 reports, zero failures/errors/skips.Evidence
Controlled failure and pending results appear before the unrelated proof gate is released, with one invoice submission. Existing request-specific tests continue to pass. Two independent reviews inspected request ownership, proof cleanup, pending tracking, call paths and parity. iOS ordinary pending presentation already returns independently; iOS local proof mutation has a different locking boundary and no matching remote wait was confirmed.
Gaps
No full-app proof-fault/restart journey was run. These component tests mock the node/proof repository; they are not deployed Lightning or Pubky verification. Compatible released Paykit/Pubky authentication/lease fixes remain separate dependencies of the broader recovery work.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)