Repository navigation
fix: deliver Paykit payment proofs promptly - #1025
ben-kaufman wants to merge 24 commits into
Conversation
|
Triage: milestone 3.1.0, the iOS twin of synonymdev/bitkit-android#1591. Paykit proof delivery starts promptly after the payment is persisted. It's an improvement, not a release blocker, and it closes no issue, so this PR carries the milestone itself. |
 [Medium impact] The changes affect Paykit proof delivery and USDT payment confirmation within the app. The PR appears safe to merge, with a non-blocking improvement needed in the concurrency test. Findings
SummaryThis PR sends payment proofs through the intended recipient’s queue, retries already-queued proofs, and starts USDT reconciliation sooner.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Payment proof ready] --> B[Wait for submission lock]
B --> C[Check identity and request]
C --> D{Exact proof already queued?}
D -- No --> E[Queue proof in SDK]
D -- Yes --> F[Drain intended recipient queue]
E --> F
F --> G[Keep failed delivery queued for retry]
G --> H[Release submission lock]
Reviews (1) · Last reviewed commit: "fix: deliver Paykit payment proofs witho..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Delivers proofs through the recipient's own outbound queue, serialises overlapping proof submissions behind a second lock, starts USDT reconciliation on confirmation and adds duration-only SDK diagnostics. Reviewed 74919dd, full tier, reasoned from the code and CI (iOS does not build on my side).
What I checked, and 5 candidates I ruled out
Read in full: the PaykitPaymentProofService.swift diff with submit, submitReady, submitInBackground, reconcile, removeRequestProofs and PaykitProofMutationLock; the PubkyService.swift changes to processOutboundPrivateMessages, withSdk and withStateRevisionTracking; the UsdtSendView.swift task, send() and PaykitUsdtPaymentService.reconcile; the test changes up to 74919dd.
Call sites traced: every submit( / submitInBackground( caller (lines 981, 983, 1079, 1165, 1343, 1570, 1590), and every mutationLock.withLock body for a nested submit or remove.
CI: validate green; Run Tests and Run Integration Tests were still running on 74919dd.
Ruled out
- Lock-order deadlock between
submissionLockandmutationLock:submittakessubmissionLockthen, throughremoveRequestProofs,mutationLock; nomutationLockbody callssubmit,submitInBackgroundor a remove helper, so the reverse order never occurs. - Delivery under a switched identity: the new
expectedIdentityoverload checksidentityStatus()inside the SDK lock and throwsidentityChanged, andsubmitReadyalready re-checks the identity before submitting. - Lost delivery when the flush fails: the error is logged and the proof is still treated as queued in the SDK, which is the durable outbox; the updated test retries an already-queued proof to the same recipient without a second submission.
- Early USDT reconcile racing the send:
reconcilereturns early whilesending,preparingorreconcilingis set, and the task still runs the finalreconcileafter the polling loop, so a no-op early call loses nothing. - Skipping the sleep after a status change: the new guard only breaks the loop once the status leaves the pending set, which the
whilecondition would have done after the sleep anyway.
Merge confidence: 4/5, no findings, but the unit and integration runs on 74919dd had not finished, and the lock ordering was checked by reading, not by a build.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- No lock inversion.
submissionLockandmutationLockare always taken in that order, and nomutationLockbody callssubmit, so there's no inversion or re-entry. The lock also closes a pre-existing iOS window where a background submit and reconcile could both submit the same proof. UsdtSendViewchanges only cut latency. TheTask { reconcile }on.confirmedbefore the celebration sleep, and the early break when the status leaves pending, are no-ops if a reconcile is already running (guard !reconciling, !sending, !preparing).
success-dismissal.xml matches synonymdev/bitkit-android#1591.
Device gate (partial): not run yet. Perf-relevant: the same extra debug log volume per SDK operation as Android, and reconcile() now waits behind an in-flight background submit including its network flush. Pending: proof latency and log lines per minute against master, plus success-dismissal.xml. The device result will follow.
|
The unit-test failure was The same test passes locally on the unchanged PR head with Xcode 27. I have restarted the failed Xcode 16.4 CI job to check for a repeat; no proof-delivery change is justified by this failure. |
|
@ben-kaufman on the flaky |
b17c070 to
ae8d401
Compare
|
@ovitrif out of scope: tracked in #1033 for 3.1.0, because the intermittent tag-return transition assertion is in unchanged base code and the current hosted unit run is green. The follow-up covers your test-scoped animation suggestion while preserving the cancellation, resumed-preparation and final-route assertions. This is separate from #989's custom-fee navigation failure. I've linked it in this PR's Out of Scope section. |
|
@jvsena42 The current head guards these diagnostics with The latest change also removes the redundant early claim for new inline requests, using the existing atomic claim-and-accept operation. Early claims remain before versioned endpoint consumption and accepted retries. All 406 focused simulator tests pass. Staging timing is recorded in the description with its limitations; matched final-change measurements remain in progress. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict:
The PR moved to 0634b16 after this run; these results are for a5aafe1.
Findings: None at a5aafe1.
QA for a5aafe1: Debug build on two iOS 26.5 simulators, using disposable staging identities and regtest funds.
Test 1: Payment Request success dismissal ✅ The 6,007-sat payment delivered its proof, restart kept one outgoing payment, and closing a later 5,000-sat request left it unpaid.
The requester showed Proof submitted for the original request. I observed both dismissals for five seconds and checked the UI snapshots: no error toast, reopened sheet or additional payment. The recording shows the payer on the left and requester on the right, aligned at 2×.
J1-success-dismissal.2x.mp4
Diagnostics recorded durable proof enqueue 2.9 seconds after Lightning settlement. The recipient delivery call returned 11.1 seconds after settlement; its queue wait was 1.3 seconds and work took 6.9 seconds. This is one sample, not a before/after benchmark.
Test 2: Recipient transport isolation and retry ⏭️ Not started before the head changed.
The PR advanced to 0634b16 while Test 1 was running. Transport-stall isolation and failed-delivery recovery remain unverified on-device.
Test 3: Staging payment through content unlock ⏭️ No merchant purchase or content unlock was reached.
The staging Shop redirected to an HTTP 401 page. An alternate merchant fixture encountered a build error; its retry ended when the PR advanced. Merchant completion latency remains unmeasured.
Automated regression coverage relies on the author's focused simulator-test report; I did not rerun that suite.
The Android pair, synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly, matched proof delivery and inline claiming at 35b83d5; iOS also advances USDT reconciliation.
Full review of b45b22a…a5aafe1, covering all nine changed files and reached callers. USDT confirmation timing was reviewed statically only. The newer head 0634b16 is outside this review.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
201dd22 to
780713e
Compare
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 2142997f.
No new actionable code findings.
A persisted Lightning or on-chain proof is handed to the SDK queue without holding the submission lock across private delivery. A proof already removed after that durable enqueue is not read from the SDK again, and a failed local save still attempts immediate enqueue. Acceptance without a versioned payment list claims inside claimAndAccept; a versioned list is still claimed before consumption. A blocked peer fails that acceptance before the payment is approved, and a later authorization rechecks the peer. Confirm, PIN, hardware signing, and a quoted USDT payment defer background inbox refresh and release that hold when the app is backgrounded. Identity and linked-peer reads retry one contended attempt while holding the SDK lock.
Compared with bitkit-android#1591 at 537d35e1: per-recipient proof delivery, inline claiming, review release when the app stops, and both payment-request journeys match. iOS also starts USDT reconciliation when the transfer is confirmed, then reconciles again after the status loop.
Validation: Run Tests and Run Integration Tests succeeded on this revision. This review inspected those results and the test sources. Device testing: not performed in this review. A success-dismissal pass was reported for a5aafe1, before the review-deferral and read-contention changes. Ready for device testing.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at a95f85a: no issues found. The branch was rebased, so I reviewed the full diff against cf2b97e with a focus on what's new. Both items from my last review are fixed in 0fa3035:
- the debug log volume is behind
#if DEBUG; submissionLocknow covers only the durable enqueue, and transport runs in a detachedTask(PaykitPaymentProofService.swift:1677), soreconcile()no longer waits behind a network flush.
Checked and clean, reviewed as a funds change:
- Locks and partial failure:
- Lock order stays
submissionLock→mutationLock, andreconcilecallssubmitoutside anymutationLockbody. onlyIfRetainedskips proofs another worker already handed off.- A failed Lightning persist falls back to an immediate
submit.
- Lock order stays
- Inline claim:
prepareForPaymentskipsensurePaymentAllowed/claimForPaymentonly for requests that need acceptance and have no versioned list.service.accept(the SDK claim-and-accept) still runs before dispatch.SendSheetpasses the consumption closure only whenpaymentListVersionis set.
- Review hold:
SendSheet.cleanup()andSendPendingScreen.onDisappearrelease it, it's released on.background, andendReviewwakes theincludingReviewswaiters. - Deferred contacts:
ContactsManagerrethrows, andMainNavView.swift:913-921waits withwaitUntilIdle(includingReviews: true)and retries. The deep-link failure I'm raising on the Android twin (synonymdev/bitkit-android#1591) doesn't exist here. - Cross-identity: transport
expectedIdentityis checked inside the SDK lock. - Journeys: the four files are identical to Android.
Perf-relevant: proof transport re-timed to fire-and-forget, Background refresh deferred during review, contended reads retried, on-chain resolution coalesced.
Device gate (partial): a95f85a — not run yet. Pending:
review-refresh.xml,success-dismissal.xmlandonchain-original-payment-retry.xmlcross-device with Android;- timing from
Queued a Paykit payment proof for private deliverytoFinished Paykit operation processOutboundPrivateMessages, and on to "Proof submitted" on the requester, against master.
Device results will follow in a separate review.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up since 2142997f, using baseline review 5581590217, at a95f85ae. This pass covered interactive enqueue for Lightning paymentSuccessful and public on-chain confirmation, the ordered persist-failure fallback, background delivery, hardware follow-up remaining ordered, Pending recovery review, and the updated on-chain retry journey. Inherited baseline coverage remains the per-recipient delivery and skip-after-handoff behavior, coalesced resolution, inline claim, confirm/PIN/hardware/USDT review deferral, identity and linked-peer read retry, contact deep-link retry, USDT reconciliation timing, review-refresh.xml and success-dismissal.xml, and the Android comparison of those unchanged contracts at 537d35e1.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
An authorized on-chain Paykit retry still enqueues its proof through the ordered private completion. Android's recovered completion at a28a0772 uses interactive priority for a default-wallet proof. Identity and payment-list reads that precede submitPaymentProof stay ordered on both sides.
Validation: Run Integration Tests and SwiftFormat passed on this revision. Run Tests was still in progress. The new priority and recovery-review tests were inspected and not executed here. Device testing: not performed in this review. The success-dismissal pass reported on a5aafe1 predates this priority and recovery-review change.
Recommended before device testing: wait for the in-progress unit-test run on this revision.
Suggested additional test cases
- iOS. Use a disposable regtest wallet, one Paykit shop request, and a retained unresolved original on-chain send, with a background Paykit refresh queued. Authorize the original-payment retry through the fee dialog, the exact prepared fee, and the payment PIN until broadcast is accepted. The proof should be enqueued once at interactive priority ahead of that background refresh, for the original recipient and amount.
Findings
- [MEDIUM] Enqueue an authorized on-chain retry proof ahead of background refresh — inline at
Bitkit/Services/PaykitPaymentProofService.swift:1137.
There was a problem hiding this comment.
Verdict:
Findings: No new findings. I confirmed the existing MEDIUM retry-proof priority finding at PaykitPaymentProofService.swift:1137: an authorized original-payment retry still reaches proof enqueue at .ordered.
QA for a95f85a:
Test 1: payment review lifecycle ✅ Review and PIN waits, cancellation and background/reopen preserved the original request; it paid once with proof delivery.
The review and PIN each stayed open for at least 30 seconds. The original 1,000-sat request paid once; the later 2,000-sat request remained unpaid. I checked the transaction and balances against the backend.
J1-combined-2x.2x.mp4
Test 2: success dismissal ✅ Closing Lightning success and restarting retained one payment; closing a later unpaid request sent nothing.
The 6,007-sat request reached Proof submitted. Closing success for five seconds produced no error; restart retained the payment. A later 5,000-sat request closed without payment or a balance change.
J2-combined-2x.2x.mp4
Test 3: original onchain retry ✅ Fee and PIN cancellation retained the original; one authorized retry spent its original inputs and recipient amount, checked against the backend.
I used a controlled unresolved state that prevented the initial broadcast. Fee entry, prepared-fee and PIN waits/cancellation, plus background/reopen, retained that original. The actual retry used the original inputs, paid 1,300 sats and charged the approved 281-sat fee; the requester reached Proof submitted. Invalid fee input disabled Continue. This did not simulate native broadcast-response loss.
J3-combined-2x.2x.mp4
Test 4: Pending recovery cancellation ✅ Retry and abandon biometric cancellation, confirmation cancellation and release-fee cancellation preserved the same original payment.
A controlled unresolved 1,500-sat payment remained intact through those cancellations and background/unlock. Trace checks showed passive refresh deferred during abandon review and admitted on idle Pending. The biometric overlay made the scene inactive, so no passive polling attempt was observed there. A separately authorized 422-sat self-return cleared the ended payment afterward.
M1p-compact-2x.2x.mp4
Test 5: recipient isolation and delivery recovery ⏭️ Restart recovery passed, but the injection did not establish recipient network isolation.
A 1,303-sat Lightning payment settled while injected recipient delivery failure left the requester Pending. Payer restart recovered its proof without another payment. A separate recipient's proof also completed while unrelated app transport calls were suspended. However, another native SDK path delivered the held recipient's proof, so the required sustained network backlog was not proved.
M2-2x.2x.mp4
Test 6: merchant content unlock ✅ One 1,000-sat payment completed the original order and unlocked its content; invoice, bundle and file bytes were checked against the backend.
The original invoice reached amount_matched and proof_submitted; its original Locks bundle completed and the same order became paid. Authenticated access returned the original 43-byte file, with integrity verified in the UI.
Trace/backend timing for this single run: proof queued at 14:04:59.555 CEST; delivery waited 6.04 seconds for admission and spent 7.07 seconds in the SDK, returning at 14:05:12.671 CEST. Locks completed at 14:05:07.295 CEST and the marketplace recorded completion at 14:05:27.338 CEST. These are separate observations, not a latency benchmark. This used a compatible staging-service fixture after the deployed Shop returned HTTP 401.
M3-combined-2x.2x.mp4
M3-unlocked-2x.2x.mp4
Test 7: confirmed USDT reconciliation ✅ One USDT transfer confirmed; after success dismissal the requester verified payment, and payer restart retained one transfer.
A $1 request accepted only USDT. Review approved 1 USDT plus a maximum 1.60-USDT fee; the final displayed fee was 0.83 USDT. I checked the successful canonical receipt and exact 1-USDT recipient transfer on the local test fork. After dismissal the requester reached Payment verified, and restart retained one debit and a 23.17-USDT balance. Sender transport had returned before dismissal; recipient verification completed afterward.
U1-2x.2x.mp4
Twin: synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly at a28a0772 matches the core proof-delivery and review-lifecycle contracts; the existing iOS retry-priority difference remains.
Coverage: I reviewed the full current cf2b97e..a95f85a diff after the rebase, plus a5aafe1..a95f85a. Identity barriers, persistence-failure fallbacks and scheduler ordering were checked in source; device runs did not inject native broadcast-response loss or a crash. Current-head unit tests and integration tests passed; local E2E CI remains pending.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
The current SwiftFormat failure comes from the PR merge checkout: both settings files reproduce the same findings on master |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up of the delta since a95f85a, reviewed at d7600d5. Unchanged coverage is inherited from the baseline review. This pass inspected background outbound delivery after the expected-identity read, the shared backup-revision deferral path, the proof-delivery caller, and the matching Android behavior at a28a077.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Background delivery that finds a payment active after identityStatus() throws OperationDeferral.paymentActive, leaves the SDK lock, and retries after the payment without clearing the backup snapshot or bumping backupStateVersion. Resume reads identity again and then sends. Cancellation, clearState, and wallet wipe while that wait is outstanding do not send. PaykitPaymentProofService starts this work with the payment identity and does not observe the deferral; the call waits inside processOutboundPrivateMessages. Android at the comparison pin returns null from revision tracking instead of throwing, which also avoids the failed-write backup update. Interactive and ordered delivery still do not wait.
Validation is source inspection only. testBackgroundOutboundDeliveryRechecksPaymentAfterIdentityRead in PaykitSdkOperationLockTests.swift covers resume, cancel, reset, and wipe, and this review did not run it. Run Tests was still in progress on this head. SwiftFormat failed on unchanged Electrum and RGS settings screens, outside this delta. Device testing: not performed in this review.
Recommended before device testing: wait for Run Tests on d7600d5, the automated check for this deferral.
Earlier findings reconciled at this revision: 1 still present, 1 verified fixed.
Findings
- [MEDIUM] Enqueue an authorized on-chain retry proof ahead of background refresh — now at
Bitkit/Services/PaykitPaymentProofService.swift:1137. Still present from the earlier review.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta at d7600d5: no issues found.
.background outbound delivery now re-checks PaykitPaymentActivity.shared.isActive after the expectedIdentity read (PubkyService.swift:1092-1095) and throws the private OperationDeferral.paymentActive before any SDK write.
withSdkcatches the deferral and retries afterwaitUntilIdle(), so it never reaches a caller.- It fires only while a payment is in progress, not during reviews.
- Every
begin()has adefer end(), so cancel, Back and errors all release it. - The proof stays in the durable SDK outbound queue.
withBackupStateRevisionTrackingskipsonChangeonly for this deferral.
This matches the Android recheck (PaykitSdkService.kt:992). It doesn't reproduce the contact-deeplink failure I raised on synonymdev/bitkit-android#1591.
Device gate (partial): d7600d5 — not run yet. Pending:
review-refresh.xml,success-dismissal.xmlandonchain-original-payment-retry.xmlcross-device with an Android master build;- proof-delivery timing against master, including paying B right after A.
Device results will follow in a separate review.
|
Resolved the master conflicts in Validation: both 19-target builds passed; 478 focused tests and 2,874 combined canonical tests passed with zero failures (the six existing CI exclusions remain). SwiftFormat reports 0/841 files requiring changes on each branch. Translation validation has zero errors and the same 15 warnings as current master. Complete diagnostics and an independent source review were checked; retained SDK QoS/host appearance warnings remain documented, with the LNURL appearance warning's cause unestablished. These are build/unit results, not new native payment or unlock measurements. Every PR commit was signature-verified locally and reports verified/valid on GitHub. All PRs remain ready for review and mergeable; human review requests are renewed and new-head CI is running. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the pull request at 2ca5d02, against merge base ec2d68a5. No coverage inventory was supplied, so nothing was inherited.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Deferred contact observation is dropped by the contacts screen, so the first load of a session can remain on the empty state after a Paykit payment or review ends. Proof enqueue, background delivery, review deferral for background SDK reads, inline acceptance, and the hardware-receipt prefilter were traced on this revision. The matching Android branch at 766f4a89 retries this contact load; its proof handoff uses the same durable enqueue and background delivery split.
unit-tests and integration-tests succeeded for this revision. This review inspected those results and the source; it did not run the suites. Device testing: not performed in this review.
Suggested additional test cases
- iOS, signed-in regtest wallet with Paykit UI enabled, payment PIN on, and at least one saved contact. Open Contacts so its load is in flight, present a payment-request confirmation over it, leave that confirmation up briefly, then close it without paying and stay on Contacts. The saved contact is listed. The empty-state add-contact screen does not remain.
Findings
- [MEDIUM] Retry the contacts load when payment deferral ends — inline at
Bitkit/Views/Contacts/ContactsListView.swift:301.
Reviewer model: grok-4.7
Reasoning effort: xhigh
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — Contacts can remain empty after payment review ends.
Findings: No new distinct findings. I confirmed the existing MEDIUM Contacts retry finding with the recorded reproduction in Test 6; that thread remains open.
QA for 2ca5d02:
Test 1: payment review and PIN deferral ✅ Cancellation submitted nothing; one authorized original payment and its proof survived restart.
Held review for 30 seconds and PIN for 42 seconds, cancelled both, received a later 2,000-sat request live, and returned through background/unlock. The original 1,000-sat request paid once with a 143-sat fee; the later request stayed unpaid. Electrum history and Bitcoin RPC decoding independently confirmed the exact recipient output, fee and single transaction.
Background/unlock needed the close/reopen workaround for the previously reported master preparing-state issue.
Sequential clips at 2x; PIN entry omitted.
| Review and cancellation | Fresh payment review | Sent |
|---|---|---|
J1-review-2x.2x.mp4 |
J1-payment-2x.2x.mp4 |
J1-paid-2x.2x.mp4 |
| Payer after restart | Requester proof |
|---|---|
J1-restart-2x.2x.mp4 |
J1-proof-2x.2x.mp4 |
Test 2: Lightning success dismissal ✅ Close and restart retained one payment; dismissing a later request paid nothing.
Paid 6,007 sats once. Native payer/receiver logs agree on one payment hash, a 6,008,006-msat outbound HTLC and a 1,006-msat fee. Closing success while proof transport was in flight showed no error; restart retained the same successful payment. The original request showed proof submitted. A later 5,000-sat request stayed pending after dismissal, with balances unchanged.
Sequential clips at 2x; PIN entry omitted.
| Review | Sent and close | Payment after restart |
|---|---|---|
J2-ready-2x.2x.mp4 |
J2-paid-2x.2x.mp4 |
J2-restart-2x.2x.mp4 |
| Requester proof | Unpaid request dismissal |
|---|---|
J2-proof-2x.2x.mp4 |
J2-unpaid-check-2x.2x.mp4 |
Test 3: retry the original on-chain payment ✅ A real refused broadcast retained the original; fresh authorization reused its input and exact recipient amount.
An app-selected Electrum proxy refused the initial broadcast without forwarding it. Fee and PIN holds/cancellation submitted nothing further. At 1,000 sat/vB, Continue was disabled. Fresh approval at 2 sat/vB displayed a 281-sat fee and required PIN. The allowed retry paid the original 1,300 sats once and delivered proof. Proxy events and Bitcoin RPC decoding verified the complete original input set, recipient and fee; the refused transaction is absent from the chain.
Sequential clips at 2x; PIN entry omitted.
| Pending, fee and PIN cancellation | Fresh fee approval | Sent and fee | Requester proof |
|---|---|---|---|
J3-pending-2x.2x.mp4 |
J3-final-review-2x.2x.mp4 |
J3-paid-2x.2x.mp4 |
J3-proof-2x.2x.mp4 |
Test 4: merchant content unlock ✅ One native payment completed the original invoice, proof, bundle and order, then unlocked the original file.
Paid 1,000 sats with a 143-sat fee on staging regtest. Independently correlated backend records show invoice a90979ef amount matched, request 243601a3 reached proof_submitted, bundle DK9TK18Z65267EKRQ13WG06BCR completed and the same order 74233ddd became paid. Authenticated unlock returned the original 50-byte file unchanged; the UI reported integrity verified. Recipient history contains one transaction.
Broadcast accepted → proof queued: 5.51 seconds. Broadcast accepted → request proof submitted: 27.17 seconds. Broadcast accepted → order paid: 59.92 seconds. These are one transaction's observed boundaries, not a latency benchmark.
Used the repository's Shop/Locks/Paykit fallback with staging identities. Sequential clips at 2x; PIN entry omitted.
| Shop checkout | Native review | Native payment |
|---|---|---|
M3-checkout-2x.2x.mp4 |
M3-review-2x.2x.mp4 |
M3-paid-2x.2x.mp4 |
| Order confirmation and unlock | Integrity and download |
|---|---|
M3-unlock-2x.2x.mp4 |
M3-content-2x.2x.mp4 |
Test 5: focused simulator regressions ✅ 847 tests passed with no failures or skips, verified in the Xcode result bundle.
Covered proof persistence/delivery, request presentation, SDK admission/identity, cancellation, overlapping recovery, native attempts, confirmation lifecycle, allowances, allowance execution, payment-state backup, contact observation, migrations, scanner, biometric policy and toast suppression. SwiftFormat 0.64.0 passed for changed Swift files; translations reported no errors and 15 existing warnings. These were automated assertions, without a UI recording.
Test 6: deferred cold Contacts load ❌ Contacts stayed empty after the real payment review ended; reopening the screen restored the saved contact.
I reproduced the existing finding with a local E2E-only hook: clear the in-memory Contacts cache and pause its first screen load before SDK admission. Persisted contacts, SDK admission and payment behavior were unchanged. A real 1,700-sat request overlaid Contacts; releasing the load while review deferral was true exercised the normal operationInProgress path. After dismissing without paying, the empty/add-contact state remained for at least 37 seconds. Leaving and reopening Contacts immediately showed the saved contact.
This case used the reviewed head plus that disclosed setup hook; Tests 1–5 used the unmodified head. No payment was submitted in this case. Recording at 2x.
J4-contact-deferral-2x.2x.mp4
Check 1: retained coverage ✅ Earlier passing reports cover unchanged recovery and transport implementations.
- Prior native fee, backup, hardware and biometric coverage at 01e4769: reached authorization, retained-attempt and hardware recovery paths are unchanged. The changed manual entry was exercised freshly above.
- Prior recipient transport isolation and restart coverage at bc2fb60: recipient-scoped enqueue/drain and recovery implementations are unchanged.
Check 2: delta and twin integration ✅ Payment admission, allowance admission and backup revision guards remain present.
Reviewed the contiguous 01e4769..2ca5d02 delta and reached callers, including imported allowance scheduling, cancellation, identity/reset and existing review threads. Allowance UI, expiry toasts and the imported USDT amount layout were reviewed in source and relevant focused tests; they were not driven as separate device journeys. No planned case remains unrun.
Twin: synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly matches the dependency pins and proof/admission contracts. Its Contacts loader also retries deferred observation, as the existing iOS finding notes.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Integrated master Validation on the final edits: both 19-target builds, 2857 bottom and 2883 combined canonical hosted unit tests passed, with CI’s six live-integration exclusions. All eight full SwiftFormat checks report 0/841; translations have zero errors and 15 warnings matching current master. Full diagnostics and a fresh independent source review found no introduced issue. Existing SDK QoS and hosted-runtime notices remain. Every PR commit is signed and verified locally and on GitHub; review requests are outstanding or renewed. The previous-head E2E run 39837073468 remains failed on the known LNURL crash/blank-confirmation symptoms, recorded in #1087 and handled separately by #1099. New-head CI will be tracked independently; hosted unit success does not resolve that native issue. |
|
Current-head E2E run 39857711251 failed only LNURL: 13/14 selected cases passed initially, followed by a blank-sheet timeout and confirmed No manual E2E retry or competing fix was started. #1099 remains a separately owned, unmerged fix; this PR’s current CI is not fully green. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: 2ca5d02 to 745df7d, including the Contacts discussion.
Findings: None.
QA for 745df7d:
Device cases ran on iPhone 17 Pro simulators with iOS 26.5, disposable regtest wallets and staging Pubky profiles.
Test 1: Contacts load and Retry ✅ Both deferred reads resumed without reopening Contacts.
The pristine build showed five saved contacts, preserved the published bio in the contact/edit screens, and retained the list after returning.
J4-list2x.2x.mp4
Normal list, contact bio and edit/cancel, at 2×.
On 745df7d plus the test hook below, I cleared only the in-memory contact list and released the real observation while a real incoming payment review was active. The SDK deferred it; cancelling review populated all five rows without leaving Contacts. I repeated this from Try Again after a genuine SDK expected-owner rejection, then cancelled a second incoming review. No payment was authorized.
| Cold read resumes | Retry starts a new view task | Retry resumes after review |
|---|---|---|
M4-cancel2x.2x.mp4 |
M4-retry-start2x.2x.mp4 |
M4-retry-resume2x.2x.mp4 |
Deferred read and Retry recovery, at 2×. Hosted tests separately verified screen disappearance, identity change and same-key session invalidation; those cancellations are not claimed as device coverage.
Unpushed test hook
The hook holds the view task before its real read until explicit release, logs admission/resumption, and requests one observation with a deliberately mismatched expected owner to reach the production error path. It does not substitute an SDK result.
diff --git a/Bitkit/Services/PubkyService.swift b/Bitkit/Services/PubkyService.swift
index 3813f6b7..b9b49c48 100644
--- a/Bitkit/Services/PubkyService.swift
+++ b/Bitkit/Services/PubkyService.swift
@@ -361,7 +361,15 @@ enum PubkyService {
}
static func observedContactRecords(expectedIdentity: String) async throws -> [Paykit.ContactRecord] {
- try await PaykitSdkService.shared.observeState(priority: .background, expectedIdentity: expectedIdentity).contacts
+ #if E2E_BUILD
+ let fault = FileManager.default.temporaryDirectory.appendingPathComponent("v4-1025-contacts-wrong-owner") // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-contacts"), FileManager.default.fileExists(atPath: fault.path) { // test hook
+ try FileManager.default.removeItem(at: fault) // test hook
+ Logger.info("QA requesting real observation with mismatched expected owner", context: "ContactsDeferralQA") // test hook
+ return try await PaykitSdkService.shared.observeState(priority: .background, expectedIdentity: "pubky" + String(repeating: "y", count: 52)).contacts // test hook
+ } // test hook
+ #endif
+ return try await PaykitSdkService.shared.observeState(priority: .background, expectedIdentity: expectedIdentity).contacts
}
static func saveContact(publicKey: String, label: String?,
diff --git a/Bitkit/Views/Contacts/ContactsListView.swift b/Bitkit/Views/Contacts/ContactsListView.swift
index ce254c08..d344296a 100644
--- a/Bitkit/Views/Contacts/ContactsListView.swift
+++ b/Bitkit/Views/Contacts/ContactsListView.swift
@@ -49,6 +49,20 @@ struct ContactsListView: View {
.background(Color.customBlack)
.navigationBarHidden(true)
.task(id: loadGeneration) {
+ #if E2E_BUILD
+ if ProcessInfo.processInfo.arguments.contains("--qa-contacts") { // test hook
+ let gate = FileManager.default.temporaryDirectory.appendingPathComponent("v4-1025-contacts-pause") // test hook
+ if FileManager.default.fileExists(atPath: gate.path) { // test hook
+ contactsManager.reset() // test hook
+ try? Data().write(to: gate.appendingPathExtension("started")) // test hook
+ Logger.info("QA contacts view paused before observation", context: "ContactsDeferralQA") // test hook
+ while FileManager.default.fileExists(atPath: gate.path) { // test hook
+ do { try await Task.sleep(for: .milliseconds(100)) } catch { return } // test hook
+ } // test hook
+ Logger.info("QA contacts released review=\(PaykitPaymentActivity.shared.shouldDeferRefresh)", context: "ContactsDeferralQA") // test hook
+ } // test hook
+ } // test hook
+ #endif
await loadContacts()
}
.sheet(isPresented: $showAddContactSheet) {
@@ -299,10 +313,16 @@ struct ContactsListView: View {
try await contactsManager.loadObservedContacts(for: pk)
break
} catch PaykitPaymentRequestError.operationInProgress {
+ #if E2E_BUILD
+ Logger.info("QA actual observation deferred; waiting for idle", context: "ContactsDeferralQA") // test hook
+ #endif
try await PaykitPaymentActivity.shared.waitUntilIdle(includingReviews: true)
}
}
guard !Task.isCancelled, pubkyProfile.publicKey == pk, pubkyProfile.currentSession == session else { return }
+ #if E2E_BUILD
+ Logger.info("QA view load completed rows=\(contactsManager.contacts.count)", context: "ContactsDeferralQA") // test hook
+ #endif
if !contactsManager.contacts.isEmpty {
app.hasSeenContactsIntro = true
}[2026-10-22 23:27:32.592 UTC] INFOℹ️: QA contacts view paused before observation - ContactsDeferralQA [ContactsListView.swift: body line: 58]
[2026-10-22 23:29:02.210 UTC] INFOℹ️: QA contacts released review=true - ContactsDeferralQA [ContactsListView.swift: body line: 62]
[2026-10-22 23:29:03.467 UTC] INFOℹ️: QA actual observation deferred; waiting for idle - ContactsDeferralQA [ContactsListView.swift: loadContacts() line: 317]
[2026-10-22 23:29:55.582 UTC] INFOℹ️: QA view load completed rows=5 - ContactsDeferralQA [ContactsListView.swift: loadContacts() line: 324]
[2026-10-22 23:30:49.984 UTC] INFOℹ️: QA contacts released review=false - ContactsDeferralQA [ContactsListView.swift: body line: 62]
[2026-10-22 23:30:49.990 UTC] INFOℹ️: QA requesting real observation with mismatched expected owner - ContactsDeferralQA [PubkyService.swift: observedContactRecords(expectedIdentity:) line: 368]
[2026-10-22 23:31:56.127 UTC] INFOℹ️: QA contacts view paused before observation - ContactsDeferralQA [ContactsListView.swift: body line: 58]
[2026-10-22 23:32:45.888 UTC] INFOℹ️: QA contacts released review=true - ContactsDeferralQA [ContactsListView.swift: body line: 62]
[2026-10-22 23:32:45.910 UTC] INFOℹ️: QA actual observation deferred; waiting for idle - ContactsDeferralQA [ContactsListView.swift: loadContacts() line: 317]
[2026-10-22 23:32:57.185 UTC] INFOℹ️: QA view load completed rows=5 - ContactsDeferralQA [ContactsListView.swift: loadContacts() line: 324]
Test 2: Hosted regressions ✅ Contacts, cleanup, admission and payment regressions passed.
I verified the current-head hosted suite results in the result bundle: Contacts retry/cancellation, profile deletion ownership and endpoint cleanup, SDK admission/identity, proof and request recovery, payment activity, on-chain retry, migration, backup and authorization regressions passed. A supplemental unpushed Contacts table case also passed after invalidating the session while retaining the same public key, confirming that the old deferred task stops.
The reviewed Swift files passed formatting and whitespace checks. The hosted run retained the known SDK QoS warnings.
Test 3: Funding selector ✅ Source selection and the on-chain-only disabled state behaved correctly.
On the pristine build, a unified Bitcoin URI showed the outlined funding button with a trailing chevron. The menu displayed Savings and Spending balances; selecting each source updated the button, available amount and checkmark. An on-chain-only URI showed Savings without a chevron and a dimmed Spending row, also exposed as disabled in accessibility and guarded in source. No send was authorized.
| Both sources available | On-chain-only address |
|---|---|
I1-dropdown2x.2x.mp4 |
I1-onchain2x.2x.mp4 |
Funding selector states, at 2×. This is functional and visual smoke coverage, without a Figma comparison.
Test 4: Fresh-channel inbound limit ✅ One 40,000 sat payment settled over a fresh 200,000 sat channel.
The pristine rc77 build negotiated a new private 200,000 sat channel. A single incoming HTLC of 40,000 sats, 20% of capacity, settled successfully. The native LDK event reported 40,000,000 msat and the same payment hash as the staging paid result. The app showed Successful and Spending increased from 50,000 to 90,000 sats.
I2-receive2x.2x.mp4
Receipt and successful activity, at 2×. This checks the new-channel receiving behavior; it does not claim an rc76 comparison, an existing-channel renegotiation or a CJIT bootstrap result.
Check 1: Retained device coverage ✅ Earlier passing cases still cover unchanged paths.
I compared the covered implementations with the reviewed head before retaining these reports:
- J1, J2, J3 and manual 3 at 2ca5d02: payment review/PIN cancellation, success dismissal, original-payment retry and staging merchant completion. Proof delivery, authorization, request presentation and native dispatch remain unchanged.
- Manual 1 and hardware/USDT authorization coverage: original-payment recovery, hardware receipt checks and USDT warning/PIN paths remain unchanged.
- Manual 2 transport isolation: unrelated recipient transport and restart recovery remain unchanged.
- Profile-cleanup device report at f882023: the imported deletion, endpoint cleanup, interruption/restart and contact-retention paths match the source implementation; current-head hosted regressions also passed.
These are retained functional results. Historical timing does not establish a fresh latency improvement at this head. Hardware, biometric, USDT and profile deletion flows were not driven again in this run.
Check 2: CI ⚠️ The separately tracked LNURL failure remains.
Current-head unit, integration and formatting checks passed. The current-head E2E run still failed the LNURL case with a blank sheet and the confirmed nil unwrap in LnurlPayAmount.swift:14; CI is not fully green.
- synonymdev/bitkit-ios#1087 LNURL flow can crash or show a blank confirmation after shared payment data is cleared contains the current-head evidence.
- synonymdev/bitkit-ios#1099 fix: preserve active LNURL payment state is the separate fix.
Pair PR: synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly matches the shared Paykit 0.1.1-rc1 / Core 0.8.0-rc10 contracts and retry/authorization behavior; the Contacts task ownership here is iOS-specific.
Reviewed by gpt-6.1-sol (reasoning: high) via gh-pr-review-loop.
|
@ben-kaufman #1099 (the LNURL fix behind this PR's red e2e) is merged. This branch now conflicts with master. Once you merge master in, e2e should go green, and then this PR and #1039 can merge for 3.1.0. |
|
@ben-kaufman please resolve conflicts. |
|
Resolved the conflict with master f650cbb, including its merged LNURL retained-presentation fixes, and propagated the signed merge through all eight latency branches without changing stacked bases. Payment review ownership now lives in the retained presentation state; the presentation-end callback is installed before admission, so closing an unmounted ready or unresolved-preparation view releases only its own token. The two-case hosted remount regression passes. Both 19-target builds passed; exact #1025 passed 2,897 canonical hosted unit tests and the pure combined stack passed 2,923, with zero failures/skips and the six existing CI live-integration exclusions. The 16 focused lifecycle cases also passed. All eight SwiftFormat scans are 0/843; translation checks have zero errors and 15 warnings matching executed master f650cbb. Complete runtime diagnostics were inspected, including retained SDK-construction QoS and hosted-view/fixture notices; this is not a warning-free runtime claim. The first canonical attempt failed the new test because its host lacked an active scene phase; that test-only setup was corrected without relaxing assertions or production checks. All complete PR commit ranges verify locally and GitHub reports verified=true/reason=valid. New-head CI is pending. The old LNURL E2E crash remains recorded in #1087; integrating the merged fix and passing hosted tests is not yet a successful native E2E replay. @piotr-iohk the reported conflict is resolved. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 3b275205 after the comparison base changed from ec2d68a5 to f650cbb4 and no inherited coverage baseline was supplied. The reviewed merge-base is f650cbb47bbd47eb684a07ee650ddcaa7db8a9e1.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Proof completion returns after the preimage or on-chain proof is stored, then enqueues it without holding the submission lock across private delivery. Inline acceptance without a versioned list consume goes through Paykit claim_and_accept_payment_request at paykit-rs f38469456; the separate execution claim remains when a versioned list is consumed. Passive refresh yields while a review token or payment admission is active, and Contacts loading and contact deep links wait for that admission and retry. A shared-state read is retried once only after a pre-read SharedStateBusy failure.
Device testing: not performed in this review. The changed unit tests and journeys were inspected at this revision and were not executed. bitkit-android#1639 (86688287) describes the same deferred proof-display recovery on a later poll; that repository was not code-reviewed.
J1–J4 and manual cases 1–4 still depend on the fixture controls and staging merchant/Locks access named in the PR.
Earlier findings reconciled at this revision: 1 still present, 3 verified fixed, 1 tracked separately outside this PR. Tracked separately (outside this PR; unresolved): Tag-return confirmation test can time out on an animated route transition. The unresolved issue is the hosted test's exact visible-route check during an animated transition. waitForAppearance still requires visibleRoutes to equal one route within three seconds, and the repository has no UIView.setAnimationsEnabled(false). That helper body is the same at the pinned base and head. Issue #1033 remains open and records that the original failure was seen when this test matched its base and that no production payment regression was established. The head reorders expectations inside testPendingWalletSwitchResumesAfterTags; it does not introduce the exact-set check or disable animations. This is not a runtime verification or merge-readiness declaration.
Findings
- [LOW] Record that the retry journey disables Continue without inline invalid-fee feedback — inline at
journeys/send/onchain-original-payment-retry.xml:23. Still present from an earlier unpublished review.
Reviewer model: grok-4.7
Reasoning effort: high
|
Current-head E2E 39919145707 at All three failure screenshots show “Funds in Transfer” with “Continue Using Bitkit” visible. The third attempt's video remains on that screen after the test's Continue step. The pinned helper clicks that button without checking navigation, then waits for the backup sheet, so the missing reminder is a downstream symptom; the root cause is unestablished. Full logs, screenshots, video and pinned test source were inspected. The six relevant transfer/navigation/backup source files match base Tracked separately at #1109. No manual rerun, competing LNURL change or local device/payment experiment. LNURL passing here is CI evidence only, not a new Paykit/Locks latency measurement. Unit, integration, build and formatting jobs passed; E2E remains failed. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None new. The existing retry-fee journey wording remains deferred to synonymdev/bitkit-ios#1053 docs: record retry-fee journey platform difference.
QA for 3b27520: iPhone 17 Pro simulators, iOS 26.5, disposable regtest wallets and staging Pubky identities. The app was built from the unmodified reviewed head. Recordings play at 2×.
Test 1: Hosted regressions ✅ The canonical hosted suite passed.
I checked the Xcode result bundle for retained Send presentation/review ownership, scanner cancellation, LNURL and Quick Pay remounts, Contacts deferral, USDT protection, proof/request recovery, authorization and fee recovery. The build passed. The run emitted SDK QoS and toolchain/simulator notices. These are XCTest assertions, without a UI recording.
Test 2: J1 review and PIN lifecycle ✅ Cancellation preserved the request; background/unlock restored a payable confirmation and one payment delivered proof.
Review and PIN each remained open for at least 30 seconds, checked against snapshots and the app trace. I cancelled both, received a later 2,000-sat request without restart, closed it unpaid and reopened the original. Background/unlock restored the original confirmation without the earlier close/reopen workaround. Fresh payment PIN approval sent 1,000 sats once. Independent transaction decoding confirmed its exact output and 143-sat fee. The requester showed Proof submitted; payer restart retained one outgoing payment. The later request remained pending.
Sequential clips; requester proof was checked afterward.
| Review/PIN cancellation | Later request and original reopening |
|---|---|
J1-hold-p2x.2x.mp4 |
J1-arrival-p2x.2x.mp4 |
| Background/unlock and approval | Successful payment |
|---|---|
J1-background-p2x.2x.mp4 |
J1-sent-p2x.2x.mp4 |
| Requester proof | Payer restart |
|---|---|
J1-proof-r2x.2x.mp4 |
J1-restart-p2x.2x.mp4 |
Test 3: J2 Lightning success dismissal ✅ Close and restart retained one payment; dismissing a later request sent nothing.
Native payer and receiver events agree on one payment hash and 6,007,000 msat received, with a 1,006-msat fee. The requester showed Proof submitted. Closing success produced no error; restart retained one Lightning debit and 43,991 sats in Spending. A later 5,000-sat request was dismissed unpaid, with the balance unchanged. Dismissal was checked in the recordings and accessibility snapshots.
| Payment and Close | Requester proof |
|---|---|
J2-ready-p2x.2x.mp4 |
J2-proof-r2x.2x.mp4 |
| Payer restart | Later unpaid dismissal |
|---|---|
J2-restart-p2x.2x.mp4 |
J2-unpaid-p2x.2x.mp4 |
Test 4: J3 original on-chain retry ✅ Fee/PIN cancellation and background/unlock retained the original; fresh approval paid its original input and amount.
An endpoint-scoped Electrum proxy refused the original broadcast without forwarding it. The app retained Pending. Invalid fee input disabled Continue; fee-entry cancellation, a 30-second prepared-fee hold/cancellation and a 30-second PIN hold/Back sent nothing further. Background/unlock retained the same Pending payment.
After restoring transport, a freshly approved 2-sat/vB retry paid 1,300 sats with the displayed 281-sat fee. The proxy ledger and independent transaction decoding confirm the exact original input, recipient and fee. The refused original remains absent from chain history. The requester showed Proof submitted. No app source hook was used.
| Refused original | Fee cancellation |
|---|---|
J3-original-p2x.2x.mp4 |
J3-cancel-p2x.2x.mp4 |
| PIN cancellation and background/unlock | Authorized retry and proof |
|---|---|
J3-pin-p2x.2x.mp4 |
J3-retry-pair2x.2x.mp4 |
The paired retry recording is time-aligned: payer left, requester right.
Test 5: LNURL integration smoke ✅ Ordinary Send retained the 150-sat draft on Back and opened a fresh 501-sat fixed confirmation.
Quick Pay was verified off. A variable LNURL showed 150 sats with comments; Back retained 150, and continuing restored that review. Closing it and pasting a fresh 500,500-msat fixture opened 501 sats without comments or another amount step. Home-scanner checks also showed 222, 223, 223 and 501 sats for the four fixed fixtures, checked from recorded device accessibility snapshots. No LNURL payment was authorized.
L1-complete-p2x.2x.mp4
In-Send draft, Back and fresh fixed confirmation, at 2×. This is an integration smoke pass; the repeated scanner/remount race assertions are covered by Test 1 and the imported LNURL review.
Check 1: Retained coverage ✅ Earlier passing reports cover unchanged implementations.
I compared the reached sources before retaining these reports:
- Contacts normal load and deferred Retry: J4/manual 4. Contacts loading is unchanged; current-head hosted tests also passed.
- Recipient transport isolation and restart recovery: manual 2. Recipient enqueue/drain implementations are unchanged.
- Merchant payment through unlock: manual 3. Delivery and merchant completion paths are unchanged; historical timing is not a fresh latency result.
- Native fee, hardware and biometric recovery: retained native attempt, receipt and authorization implementations are unchanged; presentation integration was exercised again above.
- Imported USDT protection device cases: delayed proof, later billing period and failed retry. The payment-protection service matches the merged source, and current-head regressions passed.
- Imported USDT copy-warning checks: the copy implementations are unchanged.
Hardware, biometric, USDT, transport-fault and merchant flows were not driven again in this run. No planned case remains unrun after retaining the unchanged coverage.
Check 2: Current CI ⚠️ E2E remains red on the hardware-transfer backup-introduction wait.
Unit, integration, formatting and translation checks passed. In the current-head E2E run, LNURL and the other completed cases passed, while the remaining hardware-transfer case timed out waiting for BackupIntroViewDescription. That separate hardware-transfer case was not replayed here; this review does not establish its cause or claim fully green CI.
Reviewed the contiguous 745df7d…3b27520 delta, all 49 changed files, reached callers and existing feedback. Scanner/photo cancellation races, USDT owner/reset interleavings and forced view remounts were checked in source and hosted tests rather than separate native fault journeys.
Twin: synonymdev/bitkit-android#1591 fix: deliver completed Paykit proofs promptly matches the shared dependency pins, durable proof handoff, admission and retry contracts. Retained Send presentation ownership is iOS-specific.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
@piotr-iohk could you help triage the remaining hardware-transfer E2E failure on this PR? At Could you check whether the E2E helper needs tap/navigation synchronization changes or whether this exposes an app navigation bug? Details and artifacts are tracked in #1109. If it is test-side, please fix it in the E2E repo and verify against this app head; if it is app-side, please identify the failing transition so we can fix it here. |
Related to #868, #989 and #1033.
Description
Exact amounts, quote expiry, authorization, wallet/identity/period isolation, durable recovery and duplicate-payment protection remain enforced.
Out of Scope
journeys/send/onchain-original-payment-retry.xml and journeys/README.md: document the iOS-only invalid-fee expectation: #1053, deferred from review and reconfirmed review under the repository LOW-finding policy.
Deployed server/Locks and remaining end-to-end latency investigation: Contact payment latency on staging with the shared Paykit runtime: pin paykit-rs#171 and re-measure #868. No settlement or polling-policy changes.
Design
N/A — no layout changes.
Preview
N/A — no layout changes.
QA Notes
Setup
Use the journey guide and Payment Request setup at
01e4769awith 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— review and PIN cancellation, background/reopen and later request arrival preserve one-payment behavior.success-dismissal.xml— closing payment success preserves proof delivery and reopening does not send another payment.onchain-original-payment-retry.xml— fee and PIN waiting/cancellation retain the original Pending payment; explicit retry still needs fresh approval.pubky-profile/contacts-list-loading.xml— contacts still load normally; interrupted-observation recovery is covered separately by manual case 4.Manual Tests
Automated Checks
updated
ContactsListViewTests.swift— the real hosted view resumes after review/submission admission and abandons the deferred read on disappearance or identity change.added
PaykitSdkIdentityCheckTests.swiftcases — combined reads retain runtime ownership, retry only acquisition contention, and respect cancellation and wallet wipe.updated
PrivatePaykitServiceTests.swiftandPaykitPaymentRequestServiceTests.swift— scheduling snapshot admission, generation invalidation and retry intent survive a failed read.updated
PaykitPaymentProofServiceTests.swift— durable enqueue priority, recovery overlap, negative hardware-receipt prefilter, Background delivery, restart and identity/period guards.updated
PaykitSdkOperationLockTests.swift— actual admission, payment starting during identity lookup, deferral without backup changes, resumption, reset/wipe invalidation and cancellation.updated
PaykitPaymentActivityTests.swiftandTransferServiceActivityTests.swift— Pending authentication scene policy and idle recovery admission.updated
PaykitPaymentRequestServiceTests.swiftandContactsManagerTests.swift— inline acceptance, versioned consumption, resolution coalescing, nested review, fresh authority and retry.updated
SendConfirmationLifecycleTests.swift— synchronize custom-fee and Tags-return preparation with the visible route while retaining cancellation, resumption and final navigation assertions.Evidence
Current review follow-up validation
At
61567d7b, the all-target build passed (19 targets), and the canonical unit lane passed 2,849 tests, 0 failures/skips, using the six existing CI exclusions. The new hosted Contacts table covers resumption after review/submission, disappearance and identity change. Replacing only the production view with the original2ca5d02aversion makes the regression fail in the two resumption cases; restoring the fix passes. No native payment was authorized.The same four-file delta was propagated without conflicts through all eight iOS branches. Every branch passed SwiftFormat (0/841 files requiring formatting); complete translation diagnostics show 0 errors and the same 15 warnings as executed master
ec2d68a5. Full compiler/runtime diagnostics were inspected: no compiler warnings/errors, retained SDK QoS/host appearance warnings and expected negative-fixture messages. The hosted Contacts appearance warning also occurs with the original-source negative control. This is not a claim of warning-free runtime.The earlier October 22 integration with
ec2d68a5passed both 19-target builds, 478 focused tests and a 2,874-test combined stack/sibling composition. Those results predate this Contacts fix; the new 2,849-test run is for #1025, not eight independent full-suite runs. New-head CI is pending publication checks. Later master additions ate968e218affect unrelated dependencies, funding UI and docs; no new-master integration build is claimed here.Gaps
Earlier successful native payments, interrupted proof delivery and merchant-unlock runs remain historical evidence; they do not establish coverage or a latency improvement for these integrated heads. Earlier SDK-constructor/DHT QoS notices were inspected, but their occurrence on an identical baseline host was not reproduced.
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. Cases J1–J4 and manual cases 1–4 remain unchecked on this head; hosted regression coverage does not replace the controlled native fixture in case 4. Native USDT-warning, PIN and biometric flows have not been verified by this integration validation. Staging merchant/Locks unlock remains blocked; SDK enqueue or a sent report does not establish merchant unlock.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)Metadata confirms this fork and its linked reviews. For earlier contributions, the model is
Unknown(reasoning:Not exposed): the original session metadata was not recoverable.