Skip to content

fix: show ordinary Lightning results without Paykit waits - #1684

Open
ben-kaufman wants to merge 2 commits into
fix/usdt-recovery-parityfrom
fix/ordinary-wallet-paykit-waits
Open

ben-kaufman wants to merge 2 commits into
fix/usdt-recovery-parityfrom
fix/ordinary-wallet-paykit-waits

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

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

  • J1: new 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

  • Added held-proof regressions to AppViewModelSendFlowTest; unchanged production fails both. Candidate focused suite: 504 passed.
  • Required just compile, just test, just lint: passed. Complete unit suite: 4,606 tests across 254 reports, zero failures/errors/skips.
  • Complete Detekt SARIF and Android Lint XML/SARIF: zero findings. All three structured Gradle reports match the unchanged Detekt plugin deprecation; no Kotlin compiler diagnostics.

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

  • Planning/scoping: gpt-6-astra (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh)
  • Review: gpt-6-astra (reasoning: xhigh)
  • Review rounds: 2

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 24, 2026
@ovitrif

ovitrif commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Triage: 3.1.0. It fixes the shipped Lightning payment result screen, and 3.1.0 is due 28 Oct.

@greptile-apps

greptile-apps Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; ordinary results skip unrelated work without changing request-specific proof handling.

Summary

Ordinary Lightning failures and pending results now skip unrelated Paykit proof work. Payments tied to an incoming request keep their existing checks and proof handling.

  • Ordinary Lightning results reach their screens without waiting for Paykit work.

Reviews (1) · Last reviewed commit: "fix: show ordinary Lightning results wit..." · Reviewed by Greptile

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: associateLightningPaymentProof returns before writing anything when request == null, and preparePaymentProof does too, so this flow never creates a proof keyed to that hash. The node's PaymentFailed event still runs failLightningPayment(paymentHash) off the send path in handlePaymentFailed.
  • An allowance payment on the same hash loses its cleanup: PaykitAllowanceExecutor associates its proof and fails it itself, through payer.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 PaymentSuccessful and PaymentFailed and on the polling loop, and nothing on the ordinary pending path changes request state. track and preserveContactPaymentContext still run first, so the pending hash and the contact context are kept.
  • Reordering the when changes how request payments are handled: when isPaymentRequest is true, the guard is skipped and LightningPaymentFailedError and the two-argument failLightningPayment run as before. The guard now comes before LightningPaymentFailedError, but that arm and the old !isPaymentRequest arm both returned true, 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)

@github-actions

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 3b95acd (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants