Repository navigation
fix: keep Paykit inbox observation together - #1070
ben-kaufman wants to merge 3 commits into
Conversation
 [Medium impact] This PR appears safe to merge; no actionable issues were found. SummaryKeeps automatic Background message receipt and its fresh Payment Request read in one queued operation, after any pending-message drain.
No actionable issues found. Tests were inspected, not run. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Automatic Background refresh] --> B{Full refresh?}
B -->|Yes| C[Drain pending messages]
B -->|No| D[Acquire SDK operation lock]
C --> D
D --> E[Check admission and identity]
E --> F[Receive messages]
F --> G[Record backup changes and intake reports]
G --> H{Still admitted?}
H -->|Yes| I[Read fresh state under the same lock]
H -->|No| J[Stop and allow later polling]
I --> K[Release lock and update request list]
Reviews (1) · Last reviewed commit: "fix: keep Paykit inbox observation toget..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Automatic Background inbox refreshes now run message receipt and the fresh observeState read in one queued SDK operation, so a queued background publication can no longer land between them; saved-contact forced refreshes drop to Background priority so a payment review can defer them. Reviewed b1d65a7, full tier, reasoned from the code and CI (iOS does not build on this reviewer's Linux host).
What I checked, and 6 candidates I ruled out
Read in full: PaykitSdkService.receiveAndObserveState, withSdk, both withStateRevisionTracking overloads, withBackupStateRevisionTracking, checkRefreshAdmission, PaykitSdkOperationLock (withLock, withoutLock, withWalletWipe, admit/validate), PaykitPaymentRequestService.synchronize, PaykitPaymentRequestManager.refresh (both) and performRefresh, AppScene.refreshIncomingPaykitPaymentRequests and the saved-contacts publisher.
Call sites traced: refresh(... refreshPriority:) from AppScene.swift:682 and :705; synchronize from performRefresh, where priority and messagePriority are always equal, so the combined path is taken exactly for non-forced Background refreshes with an active identity.
CI: SwiftFormat green; Run Tests still pending at review time.
Ruled out
- Observation outside the lock:
operationLock.withoutLockruns inside thewithSdkclosure, so the lock acquired for receive is still held;withoutLockonly adds wipe admission (admit/validate), which is what makes the wipe case throwwallet_wipe_in_progress. - Backup version bumped by the read:
withStateRevisionTracking(sdk:)wraps onlyreceivePrivateMessagesFromLinkedPeers; the observation runs after the revision comparison, and a deferral raised frombeforeOperationpropagates before thedo/catchthat marks the backup changed. - Stale key between receive and observe: no other operation can take the lock in between, and the
sdk ===,cachedPaykitKeypublic key and generation checks plus the post-readobservation.keyGenerationcheck cover an identity error that cleared the cache. - Intake diagnostics or sync time lost when the read fails:
onReceiverunslogIntakeFailuresand takesnow()before the second admission check, matching the old order (receive, log, date, observe). - Saved-contact forced refresh lost during review: at Background it now defers, but forced refreshes keep the separate receive/observe path and ordinary polling picks the request up later; this is the stated intent and the journeys were updated to drop the immediate-arrival assumption.
- Overlapping refresh drops the flag: the recursive upgrade in
refresh(excludingProtectedRequestId:...)forwardscombineReceiveObservation.
The MainActor-hop wipe boundary after the awaited admission check is not forced by a test, as the PR body says; the post-read validate in withoutLock covers it in source.
Merge confidence: 4/5, no findings, but Run Tests had not finished and the change could not be built here.
|
Milestone 3.2.0, matching its Android twin synonymdev/bitkit-android#1645: it is stacked on #1066 and its device measurements are still in progress, so it isn't a 3.1.0 fix. #868 stays on 3.1.0 for now; this PR refers to it rather than closing it. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for b1d65a7: Two iPhone 17 Pro simulators on iOS 26.5, disposable Bitcoin-regtest wallets and a local USDT fork. Recordings show payer left and requester right at 2×.
Test 1: automatic payment request presentation ✅ Cancellation, retry, sheet ownership and insufficient-balance handling passed.
The 21,000-sat request stayed pending after cancellation during preparation, then became ready in the same confirmation on explicit retry. Receive retained ownership while a 5,000-sat request arrived; closing Receive presented it. Both pending rows remained unique and unpaid. The drawer also retained ownership. An unaffordable request showed insufficient-balance feedback once automatically and again on explicit retry, without payment.
A debug gate controlled preparation timing; SDK records and responses were real. Foreground resume was checked in the lifecycle trace before the retained recording.
automatic-presentation.2x.mp4
Preparation cancellation, retry, presentation ownership and insufficient-balance feedback.
Test 2: payment review refresh deferral ✅ Review survived cancellation and app lock, the original paid once, and the USD review required fresh authorization.
On the pristine build, the 1,000-sat confirmation remained unchanged during a 30-second wait and a second, 2,000-sat request. Payment PIN stayed open for another 30 seconds. Cancelling PIN and review sent nothing; ordinary polling then presented the second request. Reopening the original and unlocking the app preserved its amount, prepared 143-sat fee and working details. Authorization completed one payment: the requester received 1,000 sats and recorded “Proof submitted”; the second request remained unpaid. Regtest block decoding confirmed the payment and fee.
The additional USD 1 request used fixed rates btc=0.00001 and usdt=1. Its 1,000-sat Bitcoin confirmation survived app lock, including backgrounding at payment PIN. Unlock returned to unpaid confirmation and required a new swipe and payment PIN. Cancelling left the balance and outgoing Activity unchanged. The issuer used a controlled rate; the payer remained pristine. The local USDT fixture recorded zero submissions.
| Bitcoin review and payment | USD review and app lock |
|---|---|
review-refresh.2x.mp4 |
fixed-price-lock.2x.mp4 |
Both recordings retain the original amounts through app lock and authorization changes.
Test 3: delayed saved-contact callback during review ✅ Background admission deferred, the original review stayed intact, and polling recovered the new request.
A debug gate held the real saved-contact callback before its refresh. With the 21,000-sat confirmation open, the requester sent a 3,000-sat request. Releasing the callback preserved the original review; the admission trace confirmed Background deferral with the three warm requests retained. After cancelling review, ordinary polling presented the new request without restarting or sending a payment. SDK records and responses were real.
saved-contact-callback.2x.mp4
Original review retained during callback release, followed by ordinary polling recovery.
Test 4: pristine build and focused regressions ✅ The unmodified head built and both selected hosted suites passed.
I ran PaykitSdkOperationLockTests and PaykitPaymentRequestServiceTests. The XCTest result bundle confirms execution without failures or skips, including receive/read ordering, receive-only backup accounting, admission, cancellation, identity rejection and later refresh recovery.
focused-tests.2x.mp4
Hosted test execution at 2×; assertion outcomes were checked in the XCTest result bundle.
Full review: 6153563…b1d65a7, all nine changed files and reached callers. All planned cases passed. Wipe, identity, cancellation and backup interleavings have source and automated coverage; latency against the base remains unmeasured. Earlier source report: review.
Pair: bitkit-android#1645 fix: keep incoming request refreshes together at 8564ba2 matches receive/read composition and Background contact-refresh deferral in source.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against merge base 6153563f7f73b44ed1a5f99c8606a3fab89e2bf4, at b1d65a7f0787717f03cb59a8922610a017113ed1. No earlier assessment baseline was available.
No new actionable code findings.
Automatic Background inbox refreshes receive linked-peer messages and read fresh Payment Request state in one queued SDK operation, after the existing full-refresh pending-message drain. A queued background publication waits until that pair finishes. Saved-contact forced refreshes stay on the separate receive/read path at Background priority, so an open payment review defers them and a later foreground poll (10-second inbox interval) shows the request. Receipt still records backup changes and intake diagnostics when the following read is deferred, cancelled, rejected for identity or wipe, or fails. Review admission is the send sheet’s confirm, PIN, and hardware-sign routes.
The shared refresh contract matches bitkit-android#1645 at 8564ba2a1a96e4d1ac56c4b084143f619968d48a: combined automatic receive/read, Background contact-refresh deferral, and later polling. iOS still parses requests with the timestamp taken immediately after receipt.
Validation: Run Tests and SwiftFormat succeeded on this head. Those runs and the new lock and service tests were inspected; this review did not execute them. Device testing: not performed in this review.
Ready for device testing. Use the updated automatic-presentation.xml and review-refresh.xml journeys and the saved-contact callback manual case.
Suggested additional test cases
- iOS, two linked regtest wallets, payer confirmation open for a 1,000 sat request. Send a 2,000 sat request, then background and app-lock the payer before that request is shown. Unlock. The restored confirmation still shows 1,000 sats. After ordinary foreground polling, the 2,000 sat request appears once and stays unpaid.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at b1d65a7: no issues found. Reviewed this PR's own commit on top of #1066 (6153563). Reviewed as a Paykit sync change.
- No deadlock: the SDK lock isn't reentrant, and nothing inside the combined receive+observe hold takes it again.
- Stale-state checks: wipe, runtime and identity generations are checked before the lock, before receive and before observe. The key generation is checked against the observation, which is stricter than the old observe path.
- Narrow scope: only non-forced Background refreshes use the combined path. Forced and explicit refreshes keep the separate calls.
- Partial failure: if observe defers or fails after a successful receive, nothing is lost or shown twice. The refresh generation was already reset, so the next inbox poll still runs.
- No payment path touched.
- Parity: this side moves one forced refresh to Background (
AppScene.swift:682). Android moves two, because only Android forces a refresh after endpoint refresh. See synonymdev/bitkit-android#1645.
Device gate (partial): b1d65a7 — not run yet. Pending:
automatic-presentation.xmlandreview-refresh.xml;- time how long an incoming request takes to appear while a background publication is queued;
- re-check the resync time after closing review.
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: Re-reviewed the full PR diff against merge base 5e801df, at e16d13bc, after the merge base changed since the saved assessment. No prior coverage was inherited.
No new actionable code findings.
Automatic Background inbox refreshes receive linked-peer messages and read fresh Payment Request state in one queued SDK operation, after the full-refresh pending drain. A queued background publication waits until that pair finishes. Saved-contact forced refreshes use Background priority on the separate receive/read path, so an open payment review on confirm, PIN, or hardware sign defers them. The 10-second foreground poll applies the request after review ends. A completed receive still records backup changes and intake diagnostics when the following read is deferred, cancelled, rejected for identity or wipe, or fails. The open confirmation keeps the original request context.
The shared refresh contract matches bitkit-android#1645 at 8564ba2: combined automatic receive/read and Background contact-refresh deferral. Android also defers a forced refresh after endpoint refresh. iOS leaves endpoint refresh on the existing poll schedule.
Validation: SwiftFormat passed on this head. Run Tests was still in progress. The new lock and service tests were inspected and not executed here. Device testing: not performed in this review. A simulator pass is attributed to b1d65a7 in review 5595146723. The feature files are unchanged by the merge onto the current base; this review did not rerun those journeys. Inbox latency against a queued publication was not measured.
Recommended before device testing: wait for the in-progress Run Tests workflow on this head.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: I found no actionable defects.
QA for 7cd096f: iPhone 17 Pro simulators on iOS 26.5, disposable regtest wallets and a local USDT fork. Videos run at 2×; paired videos show payer left and requester right.
Test 1: Automatic request presentation ✅ Intake, preparation dismissal/retry, competing sheets and insufficient-balance feedback passed.
Foreground intake retained the sender, amount and note while preparing. Dismissal did not reopen the same request automatically; explicit retry worked. Receive and the drawer kept ownership until closed, then the fresh request appeared once. The unaffordable request produced fresh feedback on explicit retry without payment. This case used debug timing gates around real SDK operations.
automatic-presentation.2x.mp4
Automatic intake, sheet ownership, cancellation and retry.
Test 2: Review refresh deferral and app lock ✅ Original review survived incoming work and lock transitions; one payment confirmed on regtest, and cancellation sent nothing.
The unmodified payer retained the original 1,000-sat confirmation while a second request arrived, including background/unlock before that request became visible. Waiting at payment PIN and cancelling left both requests unpaid. Retrying the original after app unlock required fresh payment authorization. The sole authorized payment paid 1,000 sats with a 143-sat fee; the regtest transaction confirmed, and the requester showed the receipt and submitted proof. The second request stayed pending.
A real USD 1 request also retained its 1,000-sat Bitcoin review across app lock. Unlocking from payment PIN returned to unpaid confirmation; another swipe required payment PIN again. Cancellation preserved balances. Node mempool and activity checks confirmed no additional send. The issuer used a fixed test exchange-rate input; the payer remained unmodified.
| Bitcoin review and payment | Fixed-USD lock and cancellation |
|---|---|
review-refresh.2x.mp4 |
fixed-price-lock.2x.mp4 |
Review preservation, authorization boundaries and final activity.
Test 3: Delayed saved-contact callback ✅ The callback left the original review intact; ordinary polling recovered the new request after cancellation.
A controlled saved-contact callback was released while the original 21,000-sat confirmation remained ready. SDK diagnostics confirmed Background admission deferred. After cancelling review, foreground polling presented the real 3,000-sat request without restart, manual refresh or payment. The callback gate controlled timing; SDK records and refresh admission were real.
saved-contact-callback.2x.mp4
Original review retained, then the fresh request recovered.
Test 4: Current-head build and regressions ✅ The pristine head built and focused XCTest suites passed, verified in the result bundle.
Executed receive/read lock ordering, request service, biometric policy, migrations, scanner and payment activity regressions. The installed test-app binary matched the pristine build. The recording shows execution; the XCTest result bundle establishes the assertions.
focused-tests.2x.mp4
Focused regression execution on the current-head app.
Test 5: Queued publication ordering ✅ Fresh inbox observation completed before queued publication admission, checked against SDK timestamps.
With receive deliberately held, a real publication was queued behind it. Releasing receive produced fresh observation before publication admission, and the 4,000-sat confirmation appeared while the publication subsequently remained held. The read completed about 0.15 seconds after release; UI observation followed within 1.83 seconds. These are controlled ordering measurements, not a natural-latency benchmark or a comparison against base.
publication-order.2x.mp4
Fresh request visible while the queued publisher remained held.
Test 6: USDT navigation and authorization cancellation ✅ Address back-navigation, warning cancellation and repeated PIN cancellation retained the review without submission.
On the unmodified payer, manual USDT address entry reached the hosted amount screen, and Back restored the same address. A 20-USDT review from a 25-USDT balance showed the over-half-balance warning. Cancel kept the review usable; a new swipe repeated the warning. Accepting it opened payment PIN. Cancelling PIN allowed another swipe to reopen PIN; cancelling again retained the original recipient and amount. The balance stayed 25 USDT. Local fixture status independently confirmed zero submissions.
usdt-warning-pin.2x.mp4
USDT address navigation, warning retry and payment-PIN cancellation.
Check 1: Current-head CI — Run Tests passed at 7cd096f.
Coverage: contiguous delta b1d65a7..7cd096f, prior review and reached callers. Merchant headings, receive-sheet wrap gestures, biometric prompts, migration UI and build/CI changes were source/unit checked; those UI paths were not separately driven. Every planned case was rerun here.
Twin: bitkit-android#1645 fix: keep incoming request refreshes together at 8564ba2 matches the receive/read grouping and background refresh deferral; Android additionally defers endpoint-triggered refreshes.
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
5 participants
This PR prevents unrelated background publishing from delaying the request list after an incoming message is received. Automatic message receipt and its fresh Payment Request read now run in the same queued operation.
Stacked on #1066. Related to #868. No measured native latency improvement is claimed.
Description
Out of Scope
None.
Design
N/A — no UI changes.
Preview
Native recording pending; no layout changes.
QA Notes
Setup
Use the journey guide and Payment Request setup at
7cd096f4with 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
automatic-presentation.xml— foreground resume receives each request once and keeps the current sheet intact.review-refresh.xml— a second request arriving during review leaves the first unchanged and becomes available after cancellation and ordinary synchronization.Manual Tests
Automated Checks
PaykitSdkOperationLockTests.swift— verifies receive/read ordering, receive-only backup accounting, cancellation, review and wipe admission, and identity/runtime rejection.PaykitPaymentRequestServiceTests.swift— preserves the prior pending drain, reports and synchronization time; checks both ordinary and forced Background deferral while explicit recovery remains Ordered.Evidence
Final integration validation
Integration with master
4530ff7bat7cd096f4retains 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; the previous-head #1070 launch failure remains disclosed below.Gaps
The exact MainActor-hop wipe boundary is rechecked in source after the awaited admission check; tests do not force that interleaving with a new hook. Earlier geo-response/hosted-view messages and one shared-keychain status were disclosed without claiming identical baseline reproduction. At preceding head
e16d13bc, both CI attempts failed with an XCTest app-launch background-assertion timeout; attempt 2 passed all 2,765 unit tests but failed one UI launch case. The cause remains unresolved and is not proven to be infrastructure; a local unit pass does not close this CI gap.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, J2, 1 remain 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)