Repository navigation
fix: restore payment review after app unlock - #1039
ben-kaufman wants to merge 5 commits into
Conversation
|
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. |
[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
SummaryPreserves matching unpaid Bitcoin reviews while app lock removes the send view, then reloads setup after 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]
Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
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
pinCheckContinuationsnever resume, butperformPayment(and with itPaykitPaymentActivityand 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, andfinishPaymentreturns early for a request not inacceptedRequestIds; a retried accepted request stays inpendingRequestsandpaymentRequestForRetryalready 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, soownsResolvedContextfails incleanupand the normal reset runs on already-cleared state. - SwiftUI fires the send sheet's
onDismisswhenMainNavViewleaves forAuthCheck, 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. setUpSendrefuses the remount:markPresentedIfPendingonly needs the request inpendingRequests, 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.
|
@ben-kaufman conflicts |
4022aa7 to
bc2fb60
Compare
b93acf7 to
e9b45c2
Compare
There was a problem hiding this comment.
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
jvsena42
left a comment
There was a problem hiding this comment.
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.xmllacks 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.
There was a problem hiding this comment.
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
This PR restores an unsubmitted incoming Bitcoin payment review after app unlock. It is stacked on #1025 (
fix/paykit-proof-latency).Description
Out of Scope
Preserve fee speed and tags across app-lock restoration: #1059, tracked for 3.2.0 (review thread).
USDT and hardware review restoration, financial retries and authorization changes.
The proof-delivery and Pending recovery-prompt changes inherited from fix: deliver Paykit payment proofs promptly #1025.
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
0df5ebfbwith disposable regtest wallets and a saved, linked fixture issuer. This source uses Paykit0.1.1-rc1and Core0.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
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
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.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
4530ff7bat0df5ebfbretains 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, tree8f1e72d3from #10707cd096f4plus #10818967dff8, #1084b7561c08and #1085310e689a, 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
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)