Skip to content

fix: restore payment review after app unlock - #1039

Open
ben-kaufman wants to merge 5 commits into
fix/paykit-proof-latencyfrom
fix/paykit-review-app-lock
Open

ben-kaufman wants to merge 5 commits into
fix/paykit-proof-latencyfrom
fix/paykit-review-app-lock

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Contributor

This PR restores an unsubmitted incoming Bitcoin payment review after app unlock. It is stacked on #1025 (fix/paykit-proof-latency).

Description

  • Retain the current Bitcoin confirmation even when the request originally opened on the amount/payment-method screen. Restore the confirmation route and reload fees and funding setup after unlock under the existing session and payment-context checks.
  • Keep amount selection available during synchronization, while confirmation and PIN routes retain their synchronization safeguards.
  • Clear dismissed reviews in either callback order. Preserve the existing handling of submitted payments, hardware signing, USDT and Pending recovery.

Out of Scope

Design

N/A — reuses the existing confirmation screen.

Preview

Updated device journey recording pending.

QA Notes

Setup

Use the journey guide and Payment Request setup at 0df5ebfb with disposable regtest wallets and a saved, linked fixture issuer. This source uses Paykit 0.1.1-rc1 and Core 0.8.0-rc10. Keep ordinary payment authorization enabled. The controlled transport/admission faults named below require additional fixture control; compatible issuer/homeserver access and staging merchant/Locks unlock must be established separately. Missing prerequisites leave the affected cases blocked.

Journeys

  • J1: updated review-refresh.xml — open a request through amount/payment-method selection, choose Bitcoin, lock/unlock during confirmation, then cancel PIN and complete a later single authorized payment. The original request, ready control and fee review must remain available.

Manual Tests

N/A

Automated Checks

  • updated PaykitPaymentRequestServiceTests.swift — actual teardown/remount restores both direct-confirmation and amount→confirmation routes; dismissal works in either callback order. Session, context, sheet ownership and active-submission changes prevent retention; hardware and USDT exclusions remain.
  • updated SendConfirmationLifecycleTests.swift — retain required setup/synchronization checks, one successful coin preparation and cancellation of an earlier task when a second enters.

Evidence

Final integration validation

Integration with master 4530ff7b at 0df5ebfb retains both USDT-warning cancellation and the payment-review callback. Bottom #1025: all-target build passed (19 targets), 378 focused tests passed, 0 failures/skips. One combined #1070 + #1081 + #1084 + #1085 composition, tree 8f1e72d3 from #1070 7cd096f4 plus #1081 8967dff8, #1084 b7561c08 and #1085 310e689a, covers the stack and those sibling changes: all-target build passed (19 targets) and 2,779 tests passed, 0 failures/skips using the six existing CI exclusions. These are not eight independent full-suite runs.

All eight heads passed SwiftFormat. Translation checks report 0 errors and 15 warnings, all matching master by key, language, file and occurrence count, including security__use_pin. Final toolchain/runtime and complete diagnostic disposition: Xcode 26.1 / iOS 27.0 simulator, serial tests; complete diagnostics inspected, with six SDK-construction QoS warnings and retained fixture/appearance messages matching baseline evidence; no introduced finding. Final-head CI after publication: pending.

Gaps

The updated app-lock journey remains unexecuted. The diagnostic CI rerun passed at preceding head c1e06c20; it does not verify this new integration head.

The final Android 206 / iOS 215 comparison cohort was blocked before financial authorization, with no debit. It supplies no final payment-latency result or native benefit claim. Case J1 remains unchecked and unexecuted on this integrated head. Native USDT-warning, PIN and biometric flows have not been verified by this integration validation.

Models used

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

@ben-kaufman
ben-kaufman marked this pull request as ready for review October 20, 2026 15:24
@ovitrif ovitrif added this to the 3.1.0 milestone Oct 20, 2026
@ovitrif

ovitrif commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Triage: milestone 3.1.0, with #1025 it is stacked on. It closes no issue, so this PR carries the milestone itself. Not a release blocker.

@greptile-apps

greptile-apps Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change reaches the iOS incoming Bitcoin payment review and send-sheet lifecycle.

Fix restoration for Bitcoin reviews reached through the amount page before merging.

Findings

  1. P1 Some Bitcoin reviews stay broken ▶

Summary

Preserves matching unpaid Bitcoin reviews while app lock removes the send view, then reloads setup after unlock.

  • An unpaid Bitcoin payment review returns after app unlock.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Incoming Bitcoin request] --> B{Original route}
  B -->|confirm| C[Review]
  B -->|amount| D[Choose Bitcoin]
  D --> E[Review; original route stays amount]
  C --> F[App locks]
  F --> G[Retain matching unpaid review]
  G --> H[Unlock and reload setup]
  E --> I[App locks]
  I --> J[Retention skipped]
  J --> K[Payment state cleared; setup cannot restart]
Loading

Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile

Comment thread Bitkit/AppScene.swift Outdated

@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. Keeps an unpaid Bitcoin payment-request review alive across app lock: the preparation is flagged on background, SendSheet.cleanup skips its reset while the review sits on confirm or PIN, and the remounted sheet re-runs setup; a dismissal in either callback order resets the retained state. Reviewed b93acf7 against fix/paykit-proof-latency, full tier, reasoned from the code and CI (iOS does not build on this reviewer's host).

What I checked, and 6 candidates I ruled out

Read in full: IncomingPaykitPaymentRequestPreparation and the background, presentation and invalidation paths in AppScene.swift; SendSheet .task(id:), cleanup, setUpSend, requestPinCheck; SendConfirmationView.submitPayment/performPayment; SheetViewModel.hideSheet and sendSheetItem; PaykitPaymentRequestManager.finishPayment, markPresentedIfPending, the approval set; both new tests
CI: SwiftFormat and Greptile green; Run Tests still pending at review time

Ruled out

  • Lock while on the PIN route leaves the old authorization waiting: the old pinCheckContinuations never resume, but performPayment (and with it PaykitPaymentActivity and the approval) only starts after PIN success, so nothing is submitted and the remounted sheet starts at confirm. That leak predates this PR.
  • Disappear-then-dismiss skips finishPayment: retention requires no submission, and finishPayment returns early for a request not in acceptedRequestIds; a retried accepted request stays in pendingRequests and paymentRequestForRetry already dropped its approval. Your test asserts the request is pending and re-presentable after either order.
  • Dismiss-then-disappear double reset: dismiss() clears the request, so ownsResolvedContext fails in cleanup and the normal reset runs on already-cleared state.
  • SwiftUI fires the send sheet's onDismiss when MainNavView leaves for AuthCheck, clearing the config before remount: the bug this fixes (a review stuck loading after unlock) shows the sheet config does survive the lock on device. Narrow: only the listed journey, still unexecuted on this head, would show it on a real presenter; the unit test swaps the host's root view instead.
  • setUpSend refuses the remount: markPresentedIfPending only needs the request in pendingRequests, which retention does not change.
  • Lock on fee-rate, tag or UTXO screens still leaves the review stuck: those routes are not retained and the remount guard returns before setup, but the behaviour is unchanged from the base branch and outside the routes this PR claims.

Merge confidence: 4/5, the remount after a real app lock is covered only by a host-swap test until the updated journey runs, and Run Tests was still pending.

@jvsena42

Copy link
Copy Markdown
Member

@ben-kaufman conflicts

@ben-kaufman
ben-kaufman force-pushed the fix/paykit-proof-latency branch from 4022aa7 to bc2fb60 Compare October 20, 2026 18:54
@ben-kaufman
ben-kaufman force-pushed the fix/paykit-review-app-lock branch from b93acf7 to e9b45c2 Compare October 20, 2026 19:29

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve at e9b45c2.

Findings: None. I verified the amount-selection retention finding is fixed in source and on-device.

QA for e9b45c2: iOS 26.5 simulators, disposable identities and Bitcoin regtest funds. In the paired recordings, payer is left and requester right, aligned at 2×.

Test 1: direct Bitcoin review after unlock ✅ The original review resumed without reopening and completed one freshly authorized payment.

Confirmation and payment PIN each stayed open for over 30 seconds. PIN cancellation and dismissal sent nothing; a later 2,000-sat request arrived without restarting. After reopening the original and locking the app, app-unlock PIN restored its 1,000-sat confirmation, ready swipe and working details with a 143-sat fee. Fresh payment PIN reached success once. The requester showed Proof submitted, while the second request stayed pending. Bitcoin backend data confirmed the exact 1,000-sat output and 143-sat fee; closing success left one outgoing activity.

J1.2x.mp4
Test 2: amount selection and payment-PIN lock ✅ Both locks restored the unpaid Bitcoin review and required fresh payment authorization.

A requester-only test hook issued native SDK terms for 1 USD, accepting Bitcoin and USDT at fixed rates of 0.00001 BTC and 1 USDT. The payer ran the unchanged reviewed binary. Amount showed Savings and 1,000 sats; Continue opened confirmation. Lock/unlock from confirmation and from payment PIN each returned directly to that same Bitcoin review with a prepared 143-sat fee and ready swipe. Entering only app-unlock PIN sent nothing. A new swipe required a new payment PIN; cancelling and closing preserved the balance and the sole earlier outgoing payment. The issuer remained Waiting for payment, and the Bitcoin backend confirmed zero received at this request's recipient address. No USDT payment was made.

J2.2x.mp4
Test 3: hosted lifecycle regressions ✅ App-lock and confirmation lifecycle suites passed.

Verified from the Xcode result: teardown/remount from direct and amount-entry routes, both dismissal callback orders, session/context/sheet ownership and submission exclusions, plus confirmation setup and cancellation. The recording shows hosted test execution; assertions were checked in the test result.

A1.2x.mp4

Coverage: full bc2fb605..e9b45c2, all six changed files and reached callers. Unavailable-node synchronization and hardware/USDT retention exclusions have source or hosted-test coverage only. Inherited proof-delivery and recovery coverage remains in the parent's QA report.

Twin: synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly shares the review/PIN deferral contract; Android keeps content beneath its lock overlay, so this view-remount fix is iOS-specific.

The full CI unit-test job was still running at completion; the focused local suites above passed.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

utAck

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving. No HIGH/MEDIUM. One LOW is inline; it can be fixed here or in a follow-up, your choice.

This fixes the master bug I posted as out-of-scope on #1025 (stuck spinner after a PIN background cycle). The confirmation is retained on app lock, and setup runs again on unlock.

Checked and clean, reviewed as an auth and payment change:

  • Retention scope: only an unsubmitted, non-hardware, non-USDT Bitcoin confirmation (or its payment PIN) owned by the same session, sheet, context and request.
  • No payment without re-confirmation: after unlock a payment needs a new swipe and a new payment PIN. The old view's PIN continuations are never resumed, and the app-unlock PIN authorizes nothing. A lock during an in-flight payment isn't retained.
  • Fresh terms: the amount is re-derived and the fee reloaded. Expired conversion terms return to Amount.
  • Session or ownership change while locked: invalidates and dismisses the request (isIncomingPaymentRequestPreparationInvalid).
  • Dismissal and the review hold: dismissal in either callback order resets exactly once. The review hold stays balanced across the re-host, with one token per sheet instance. It doesn't worsen my LOW on #1054.
  • Android parity: the bug can't happen there, because ContentView stays composed under AuthCheckView. synonymdev/bitkit-android#1591 review-refresh.xml lacks the new lock/unlock steps, so port them, or note the overlay difference in the README, to keep the spec diffable.

Device gate: no run by me — journeys: review-refresh.xml lock/unlock steps already driven at this head; perf: one setUpSend per unlock with a retained review; Figma: n/a; l10n: no UI text changed.
Skipped — already driven this round: payment-requests/review-refresh.xml lock/unlock, direct and amount → confirm entry (iOS, by @ovi-reviewer at e9b45c2, passed: #1039 (review))

The PR body lists SendConfirmationLifecycleTests.swift as updated, but only PaykitPaymentRequestServiceTests.swift changed.

Comment thread Bitkit/Views/Wallets/Send/SendSheet.swift

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve at 0df5ebf.

Findings: None in the delta. The bitkit-ios#1059 fix: retain fee speed and tags after app unlock remains deferred as agreed in the review thread.

QA for 0df5ebf: iOS 26.5 simulators, disposable identities and Bitcoin regtest funds. The payer ran the unmodified reviewed build. Recordings play at 2×; paired recordings show payer left and requester right.

Test 1: direct Bitcoin review after unlock ✅ The original review resumed directly and completed one freshly authorized payment.

Confirmation and payment PIN each stayed open for over 30 seconds. Cancelling PIN and dismissing sent nothing; a second 2,000-sat request arrived without restarting. After reopening the original and locking the app, app-unlock PIN restored its 1,000-sat confirmation with a ready swipe and working details showing a 143-sat fee. A fresh swipe and payment PIN reached success once. Closing success left one outgoing activity; the requester showed Proof submitted and the second request stayed pending. Bitcoin RPC independently confirmed the funding output stayed unspent before authorization, then the exact 1,000-sat output and 143-sat fee.

J1.2x.mp4

Direct review, cancellation, app unlock and the single authorized payment.

Test 2: amount selection and payment-PIN lock ✅ Both locks restored unpaid Bitcoin confirmation and required fresh payment authorization.

A requester-only fixture hook supplied native SDK terms for 1 USD, accepting Bitcoin and USDT with fixed rates of 0.00001 BTC and 1 USDT; those terms were checked in the hook, and the synchronized requester showed the 1 USD request. The unchanged payer entered Amount with Savings and 1,000 sats, then continued to Bitcoin confirmation. Lock/unlock from confirmation and from payment PIN each returned directly to the same unpaid review; restored details showed a prepared 143-sat fee. App-unlock PIN sent nothing. A new swipe required a new payment PIN; cancelling and closing retained 98,857 sats and the sole earlier outgoing activity. The requester remained Waiting for payment, and Bitcoin RPC confirmed zero received at this fixture-owned destination. No USDT payment was made.

J2.2x.mp4

Fixed-price amount entry and both lock boundaries.

Test 3: biometric app unlock and cancellation ✅ PIN fallback, manual Face ID retry and background interruption preserved the unpaid review.

With simulated Face ID enrolled, a failed match allowed Use PIN code fallback and restored the unpaid 2,000-sat review. Cancelling a later attempt exposed the app's Use Face ID button; a successful manual retry restored that review. Backgrounding another pending challenge and sending a match while backgrounded did not bypass app lock: returning showed a fresh challenge. A new match restored the same amount and a prepared 143-sat fee. Dismissal retained 98,857 sats and the sole earlier outgoing activity; Bitcoin RPC independently confirmed the entire remaining funding output stayed unspent.

J3.2x.mp4

Simulator Face ID failure, fallback, retry and interrupted app unlock.

Check 1: retained hosted regressions ✅ The previous passing app-lock and confirmation-lifecycle assertions remain applicable.

I retained the hosted results from the review at e9b45c2: their test bodies and Bitcoin retention/setup logic are unchanged. They cover both dismissal callback orders, session/context/sheet ownership, active submission, hardware/USDT exclusions and setup cancellation. The changed app-unlock authentication was driven above. These suites were not rerun here.

Coverage: contiguous e9b45c2..0df5ebf, including both integration merges and reached callers. Device coverage above exercises Bitcoin review and app authentication; imported USDT warning/routing, receive-tab, merchant-label and CI changes were inspected in code. Earlier inherited USDT routing and reconciliation evidence is in the parent's report at d702597; it does not prove the later warning integration. Hardware signing and unavailable-node synchronization were not driven in this pass.

Twin: synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly shares the payment-review deferral contract. Android keeps content beneath its lock overlay; this review-remount fix is iOS-specific.

The full CI unit-test job remained pending at completion.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

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.

4 participants