Repository navigation
fix: keep Paykit inbox observation together - #1070
ben-kaufman wants to merge 28 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.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the complete PR diff against merge base b421181b, at 02986d70. The comparison base changed from 50a33e3a to b421181b, and those target-branch changes touch Bitkit/AppScene.swift and Bitkit/Services/PubkyService.swift. No verified baseline was supplied, so inherited coverage is empty.
No new actionable code findings.
Automatic background inbox refresh receives linked-peer messages and reads payment-request state in one SDK operation, after the full-mode pending-message drain. The SDK lock stays held until that read returns, so a background publication queued during receive waits for it. Saved-contact forced refresh stays at background priority; an open payment review defers it, and foreground polling (about every 10 seconds) is the later read. Receive backup accounting and intake diagnostics remain when the later read yields to review, cancellation, identity change, or wallet wipe. Send confirmation starts a payment review, so that deferred refresh leaves the open request in place.
The same background pairing, forced-refresh split, and during-review second request are in bitkit-android#1645 at bad5ae85. Android master 0f44d936 still uses separate receive and observe calls.
Validation: inspected the new service and lock tests; did not execute them. unit-tests and SwiftFormat succeeded at this revision. E2E jobs were skipped. Device testing: not performed in this review.
Ready for device testing. Journeys J1 and J2 and manual case 1 remain the device contract.
Reviewer model: grok-4.7
Reasoning effort: high
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 974be754 after the comparison base changed from b421181b to 5277d370. No coverage baseline was supplied, so this pass inherited nothing. The verified merge base is 5277d370.
No new actionable code findings.
Automatic background inbox refreshes drain pending outbound messages when the mode is full, then receive linked-peer messages and read fresh payment-request state in one background SDK lock hold. If that later read yields to a payment review, cancellation, identity change, or wallet wipe, the receive backup mark and intake log remain, and the request list stays unchanged until a later poll. Saved-contact forced refreshes stay at background priority, so an open payment review can defer them. The same refresh contract is in bitkit-android#1645 at bad5ae85; that comparison was source-only.
Unit tests and SwiftFormat passed on this revision. I inspected the new lock and refresh tests and did not run them locally. Device testing was not performed in this review. Earlier device notes are for older commits (05c92291, 02986d70), so they are not a pass of this head.
Ready for device testing.
Reviewer model: grok-4.7
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 974be75: iPhone 17 Pro simulators on iOS 26.5, disposable regtest wallets and local test USDT. Videos run at 2×; paired videos show payer left and requester right.
Test 1: automatic request presentation (J1) ✅ Preparation, cancellation, explicit retry and competing sheets preserve the request without sending.
The journey retained sender, amount and note during disabled preparation, kept Receive and the drawer open while new requests arrived, and presented each request after the competing surface closed. Reviewed requests stayed unpaid and did not reopen automatically. The unaffordable request produced feedback once automatically and again on explicit retry. Timing gates held preparation; balances and Activity were checked afterward.
Preparation, sheet ownership and insufficient-balance retry.
Test 2: review refresh deferral (J2) ✅ The original request survives another arrival, PIN cancellation and app lock; authorization sends it once.
The pristine payer retained its 1,000-sat review through the confirmation and payment-PIN holds, cancellation and app unlock. Its authorized payment confirmed with a 143-sat fee. Chain data, Activity, the requester's receipt and “Proof submitted” status verified the payment; the second 2,000-sat request stayed pending.
A USD 1 request with the specified endpoints and fixed rates restored its unpaid 1,000-sat Bitcoin confirmation after app lock from both confirmation and payment PIN. A fresh swipe required payment PIN again. Cancellation left no additional transaction. This leg used a local DEBUG payload logger to check the exact received terms; the issuer supplied fixed test terms.
| Bitcoin review and payment | Fixed-USD app-lock recovery |
|---|---|
Test 3: saved-contact callback during review (manual 1) ✅ The callback defers during review, and ordinary polling recovers the fresh request afterward.
A timing gate released the saved-contact callback during the original 21,000-sat review after the issuer sent a real 3,000-sat request. The callback trace verified Background admission deferred without replacing the warm inbox. The original review stayed unchanged; closing it allowed polling to present the new request without restarting or paying.
Original review retained, then the fresh request recovered.
Test 4: exact-head build and regressions ✅ The signed pristine build and focused hosted regressions passed.
The XCTest result bundle verified inbox observation, request protection, Send lifecycle, scanner cancellation, QuickPay remount and LNURL confirmation coverage.
Hosted test-app recording; results were verified in the XCTest bundle.
Test 5: USDT warning and PIN cancellation ✅ Address navigation, warning retry and two PIN cancellations retain the original review without submission.
On the pristine payer, Back from Amount retained the manual address. A 20-USDT review from a 25-USDT balance repeated its warning after cancellation. Accepting the warning opened payment PIN; two separate PIN cancellations preserved the recipient and amount. The balance stayed 25 USDT, and fixture status independently verified zero submissions.
USDT navigation and authorization cancellation.
Check 1: queued publication ordering ✅ Prior passing evidence still applies to the unchanged SDK queue and operation-lock implementation.
I checked the current integration against the recorded publication-ordering result at 7cd096f. This case was carried forward without another device run.
Reviewed delta: 02986d7..974be75 and reached callers. Separate LNURL/QuickPay device journeys were not driven; their changed callers and relevant hosted regressions were checked. Biometrics were disabled for J2's PIN precondition.
Twin: bitkit-android#1645 fix: keep incoming request refreshes together matches the combined receive/read and saved-contact refresh behavior; Android also forces endpoint refresh.
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
…it-preparation-failure-refresh
…it-inbox-observation
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the complete PR diff against merge base 5ab68459, at 7a3cc5a9, after the comparison base changed from 5277d370 to 5ab68459. No earlier coverage was inherited.
No new actionable code findings.
Automatic background inbox refreshes drain pending outbound messages in full mode, then receive linked-peer messages and read payment-request state under one SDK lock hold. A background publication queued during receive waits until that read returns. Saved-contact forced refreshes stay at background priority on the separate receive/read path, so an open payment review on confirm, PIN, or hardware sign defers them; the foreground poll (about every 10 seconds) applies the request afterward. A completed receive still records backup changes and intake diagnostics when the later read yields to review, cancellation, identity change, or wallet wipe, and the open confirmation keeps its original request. The same background pairing and forced-refresh split are in bitkit-android#1645 at bad5ae85; that comparison was source-only.
SwiftFormat passed on this revision. Unit tests were still running; this review inspected the new lock and refresh tests and did not execute them.
Device testing: not performed in this review. Device reports on 974be75, 02986d7, and 05c9229 belong to those earlier builds.
Recommended before device testing: wait for the in-progress unit-tests run on this head.
Reviewer model: grok-4.7
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 7a3cc5a: iPhone 17 Pro simulators on iOS 26.5, disposable regtest wallets, linked staging identities and local test USDT. Videos play at 2×.
Test 1: J2 Amount app-lock recovery ✅ The reopened request retains Savings and 1,000 sats through app unlock and continues to ready confirmation.
I reopened the unpaid fixed-USD request, selected Savings, backgrounded the app and entered only the app-unlock PIN. Amount still showed 1,000 sats with Savings selected. Continue opened the same request's Bitcoin confirmation with a prepared 143-sat fee and ready swipe. Closing it left the request pending and reopenable. A separate Activity inspection and the Bitcoin balance showed no outgoing payment.
amount-lock.2x.mp4
Amount selection, app unlock, ready confirmation and the retained pending request.
Test 2: incoming USDT review app lock ✅ The request retains its recipient and amount after app unlock; cancelling payment PIN leaves it unpaid.
I also exercised the USDT request route reached by the new preservation branch. App unlock returned to the same USD 1 request and recipient. A fresh swipe and fee-warning confirmation reached payment PIN. Cancelling PIN and closing review retained the pending request and balances. A separate Activity inspection showed only funding receipts; the local fixture independently recorded zero submissions.
usdt-request-lock.2x.mp4
USDT review restoration and cancellation before payment authorization.
Test 3: exact-head build and hosted regressions ✅ The signed pristine build and focused regression suites passed.
The XCTest result bundle verified the new Amount and USDT-context preservation/dismissal case, existing Bitcoin confirmation/PIN retention, inbox ordering and admission, request protection, Send lifecycle, scanner, QuickPay and LNURL regressions. The video shows hosted execution; assertion outcomes were checked in the result bundle.
regressions.2x.mp4
Hosted regression execution on the unmodified head.
Check 1: retained device coverage ✅ Earlier passing cases still apply to the unchanged paths.
I checked the recorded results at 974be75 against this delta. J1, manual case 1, J2's original 26 actions and ordinary USDT warning/PIN cancellation were not driven again. Preparation and callback admission are unchanged; Bitcoin confirmation/PIN app-lock retention still takes its existing branch. Test 1 above covers J2's four appended Amount actions.
Check 2: queued publication ordering ✅ The receive/read and operation-lock implementation remains unchanged.
The passing ordering result at 7cd096f remains applicable. I checked its integration with the current callers in source; no new device run for this case.
Fixed-request test hook
Not pushed. The issuer supplies the fixed test rate and exact accepted endpoints; the payer only enables received-term logging. Payment, lock and cleanup decisions are unchanged. The payer log confirmed amount=1 asset=usd, accepted identifiers btc-regtest-p2wpkh and usdt-arbitrum-address, and fixed rates btc=0.00001,usdt=1.
diff --git a/Bitkit/AppScene.swift b/Bitkit/AppScene.swift
index c3c81ba2..dc0790d8 100644
--- a/Bitkit/AppScene.swift
+++ b/Bitkit/AppScene.swift
@@ -1489,6 +1489,17 @@ struct AppScene: View {
refreshPriority: refreshPriority, canRefresh: canRefresh
)
guard refreshed else { return false }
+ #if DEBUG
+ if ProcessInfo.processInfo.arguments.contains("-request-qa") { // test hook
+ for request in paykitPaymentRequestManager.pendingRequests { // test hook
+ let conversion: String // test hook
+ if case let .fixed(rates) = request.pricing.conversion { // test hook
+ conversion = "fixed " + rates.map { "\($0.asset)=\($0.value)" }.joined(separator: ",") // test hook
+ } else { conversion = "none" } // test hook
+ Logger.info("received request id=\(request.paymentRequestId) amount=\(request.amount.value) asset=\(request.amount.asset.rawValue) accepted=\(request.acceptedPaymentEndpointIdentifiers) conversion=\(conversion)", context: "RequestQA") // test hook
+ } // test hook
+ } // test hook
+ #endif
// Deleting the profile withdraws the payment lists an automatic payment would consume.
if canRefresh(), pubkyProfile.automaticPaykitSession == session, !pubkyProfile.isDeletingProfile {
await paykitAllowanceManager.refresh()
diff --git a/Bitkit/Services/PaykitPaymentRequestService.swift b/Bitkit/Services/PaykitPaymentRequestService.swift
index bfccc9fa..6d38fd7e 100644
--- a/Bitkit/Services/PaykitPaymentRequestService.swift
+++ b/Bitkit/Services/PaykitPaymentRequestService.swift
@@ -686,7 +686,13 @@ struct PaykitPaymentRequestService {
LightningService.shared.channels?.contains(where: \.isUsable) ?? false
},
marketRate: @escaping @MainActor @Sendable ()
- -> PaykitExchangeRate? = { CurrencyService.shared.loadCachedRates()?.first { $0.quote == "USD" }.map { PaykitExchangeRate(
+ -> PaykitExchangeRate? = {
+ #if DEBUG
+ if ProcessInfo.processInfo.arguments.contains("-fixed-issuer-qa") { // test hook
+ return PaykitExchangeRate(price: "100000", timestamp: Date().addingTimeInterval(-60)) // test hook
+ } // test hook
+ #endif
+ return CurrencyService.shared.loadCachedRates()?.first { $0.quote == "USD" }.map { PaykitExchangeRate(
price: $0.lastPrice,
timestamp: $0.timestamp
) } },
@@ -873,9 +879,14 @@ struct PaykitPaymentRequestService {
expectedIdentity: String
) async throws -> PaykitPaymentRequest {
let enabled = await Self.acceptedPaymentEndpointIdentifiers(canReceiveLightning: canReceiveLightning())
- let acceptedPaymentEndpointIdentifiers = draft.acceptedPaymentEndpointIdentifiers.map { selected in
+ var acceptedPaymentEndpointIdentifiers = draft.acceptedPaymentEndpointIdentifiers.map { selected in
enabled.filter { selected.contains($0) }
} ?? enabled
+ #if DEBUG
+ if ProcessInfo.processInfo.arguments.contains("-fixed-issuer-qa"), draft.amount.asset == .usd { // test hook
+ acceptedPaymentEndpointIdentifiers = acceptedPaymentEndpointIdentifiers.filter { ["btc-regtest-p2wpkh", "usdt-arbitrum-address"].contains($0) } // test hook
+ } // test hook
+ #endif
let proposalDate = now()
guard draft.amount.atomic > 0, !acceptedPaymentEndpointIdentifiers.isEmpty else {
throw PaykitPaymentRequestError.requestUnavailableUnit CI and SwiftFormat passed at this head; separate E2E jobs were skipped.
Coverage: contiguous 974be75..7a3cc5a, all four changed files and reached callers. Biometrics and hardware-signing UI were not driven. This run makes no latency comparison.
Twin: bitkit-android#1645 fix: keep incoming request refreshes together at bad5ae8 matches combined receive/read and Background refresh deferral. Android retains content beneath its lock overlay; the added view-recreation fix 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
…re-refresh fix: avoid inbox waits after preparation errors
|
Prior-head CI run 40394372963 failed on 9d14175: all 2,934 unit tests passed; UI had 43 executions, one failure and four existing prerequisite skips. The complete result bundle identifies Test, ReceiveSheet, camera and workflow source comparisons against the actual parent and passing #1081 retry are retained; this is not a base runtime reproduction or an established production cause. Complete logs retain the same 571 duplicate-symbol records as that passing retry, plus the existing destination/xcbeautify notices. No blind CI rerun or assertion/timeout change was made. A coordinated, isolated unfunded comparison followed after disk capacity was restored; its completed outcome is below. Publication follow-up: signed integration The original-head controlled UI run passed (59.636 s), including Receive dismissal; XCTest's default interruption handler allowed the later camera alert. The explicit-camera version also passed (58.357 s), with the entire 157-file production app bundle identical across those two runs. Thus the Receive failure did not reproduce and its cause remains unestablished; this camera correction is not claimed to fix it. Final validation: two 19-target builds; 2,913 parent and 2,940 combined canonical unit tests, zero failures/skips with the exact six CI integration exclusions; combined unfunded UI passed (62.976 s), including both blank-band swipes and explicit camera handling. Full SwiftFormat 0.64 passed across all seven final trees. Complete diagnostics retain SDK/DHT QoS, hosted/fixture notices and a shared-keychain -25300 diagnostic whose exact cause remains unestablished. Local Xcode 26.1/SDK 26.1/iOS 26.5 differs from hosted CI. The normal atomic push and full-range signatures are verified; new-head hosted CI has completed successfully. No manual CI rerun, payment, funded USDT, hardware, latency or merchant-unlock experiment was performed. João, Piotr and Ovi remain requested without duplicate notifications. The own simulator is Shutdown with data retained; resources remain protected. Hosted confirmation (October 26, 23:48 UTC audit): current-head run 40412986788 attempt 1 passed 2,937 units and 43 UI executions (39 passed/four existing skips), zero failures. The previously failing unfunded Receive/invalid-payment case passed. Formatting 40412986734 passed 0/844. Complete logs retain 571 duplicate-symbol records matching the passing #1025 reference plus destination/xcbeautify notices; comparison is not exact-base runtime replay or warning-free output. The earlier Receive-dismissal cause remains unestablished. The red aggregate contains cancelled duplicate jobs with successful replacements; no manual rerun or new review request was sent. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for ac364f0: Xcode 27.0, iPhone 17 Pro simulator on iOS 26.5. Application and test sources were unmodified. Videos play at 2×. Capture stutter remains after recording retries; the published copies retain all captured changed frames.
Test 1: exact-head build and hosted regressions ✅ The signed build and canonical unit selection passed.
The XCTest result bundle verifies receive/read ordering, receive-only backup accounting, review/wipe admission, identity rejection, stored preparation-failure projection and full rejection recovery. Assertions also cover bitkit-ios#1098 fix: prepare the wallet switch once on send review, request app-lock restoration, blocked-request reservations, recovered success amounts and USDT subscription/expiry branches. The six CI integration exclusions were retained. The recording shows a focused repeat of the inbox lock and Send lifecycle suites; assertions were checked in the result bundles.
regressions.2x.mp4
Hosted regression execution on the reviewed head.
Test 2: USDT Receive and invalid-payment flow ✅ Both blank-band swipe directions, Receive dismissal and explicit camera permission passed.
The unfunded UI case verifies QR/address consistency, committed and discarded amount edits, and Receive tab switching. A Bitcoin-shaped address is rejected in USDT entry; a valid USDT URI opens at 1.23 and further entry stops at six decimals without opening confirmation. XCTest assertions establish the result; the recording shows the flow.
usdt-unfunded.2x.mp4
Receive gestures, camera permission and invalid-payment checks.
Test 3: unknown USDT balance in a comma region ✅ German-region entry uses a dot decimal and displays an unknown balance instead of zero.
The UI case checks the Home and wallet unknown-balance states, manual-address routing, a dot-decimal value of 1.5, the Arbitrum default and the network picker. Assertion outcomes were checked in the result bundle.
usdt-comma.2x.mp4
Unknown-balance display and manual decimal entry.
Check 1: retained inbox and app-lock journeys ✅ Earlier passing cases still apply to their unchanged paths.
The recorded J1, J2 and manual-case-1 results at 974be75 remain applicable to one-time request intake, competing sheets, insufficient-balance retry, saved-contact callback deferral, original Bitcoin review and authorization. The Amount and ordinary incoming-USDT app-lock results at 7a3cc5a also remain applicable. These journeys were not repeated. The recorded Bitcoin cases keep Savings selected without a funding-source switch. The ordinary USDT request is not an initial subscription payment, so its authorization route remains unchanged.
The queued-publication ordering result at 7cd096f still applies to the unchanged combined SDK operation and lock implementation.
Check 2: merged preparation-failure coverage ✅ The earlier live fault result and current-head regressions cover stored projection after failure.
The live claim-failure report at 430656e proved fresh stored projection before Transaction Failed while unrelated transport stayed held, no payment submission, and fresh PIN authorization on explicit reopen. bitkit-ios#1081 fix: avoid inbox waits after preparation errors is included in this delta. Test 1 checks its processing guard, uncertain acceptance and identity rejection at this head. The live fault was not repeated; the earlier report did not establish seamless Try Again.
Unit/UI CI and SwiftFormat passed for this published head. Separate E2E jobs were skipped.
Coverage: contiguous 7a3cc5a..ac364f0, all 88 changed files and reached callers. Inherited profile, terms, subscription and payment-recovery journeys were source/unit checked; their separate native flows, funded initial-USDT subscription, biometrics and hardware signing were not driven. This run makes no latency comparison.
Twin: bitkit-android#1645 fix: keep incoming request refreshes together at bad5ae8 still matches combined receive/read and Background refresh deferral. The merged iOS view-lifecycle and camera changes are platform-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
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the complete PR diff against merge base 18ef46bf, at ac364f01, because the comparison base changed from 5ab68459 to 18ef46bf and no baseline coverage was supplied. No earlier coverage was inherited.
No new actionable code findings.
Automatic Background inbox refreshes drain pending outbound messages in full mode, then receive linked-peer messages and read payment-request state under one SDK lock hold. A background publication queued during receive waits until that read returns. Saved-contact forced refreshes stay on the separate receive/read path at Background priority, so an open payment review on confirm, PIN, or hardware sign defers them; the foreground poll (10-second inbox interval) applies the request afterward. A completed receive still records backup changes and intake diagnostics when the later read yields to review, cancellation, identity change, or wallet wipe, or when that read fails. Preparation failures refresh stored local state and leave inbox delivery to a later full refresh. The open confirmation keeps its original request.
The same combined background receive/read and Background contact-refresh deferral are in bitkit-android#1645 at bad5ae85. That comparison was source-only. Android's default branch at ea89ed18 still uses separate lock acquisitions while that PR is open.
Run Tests and SwiftFormat succeeded on this head. This review did not rebuild or rerun those workflows. Device testing: not performed in this review.
Reviewer model: grok-4.7
Reasoning effort: high
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed the new commit 6dd46e4 (avoid inbox waits after preparation errors). No findings.
- After a failed
prepareForPayment, the.storedrefresh only re-reads SDK state. - New messages still arrive on the 10 s foreground
.fullpoll, so a request can be delayed by one poll at most, never dropped. - The refresh recursion runs at most once, and a concurrent
.fullpoll still re-runs because its mode ranks higher.
Device gate: not needed for this delta.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at d9566ad8, covering the delta since ac364f01 (baseline review). This pass inspected the recreated confirmation's retained submission flag, its automatic-start and PIN-stage interaction, and the new confirmation tests. Inherited coverage remains the background inbox refresh, saved-contact deferral, receive backup accounting, preparation-failure refresh, refresh coalescing, the payment-request journeys and README limits, and the shared refresh contract in bitkit-android#1645 at bad5ae85.
No new actionable code findings.
A recreated confirmation keeps the swipe busy from the sheet's retained isSubmittingPayment value and leaves submitPayment before another PIN check or performPayment. App unlock during payment PIN entry still sees that flag clear, matching review-refresh.xml: the unpaid review must accept a new swipe and authorization. Request failure retry still closes the sheet and presents again.
SwiftFormat passed for this head. Unit tests were still running; this review did not execute them. The author reports a local unit pass that includes the two new confirmation tests. Device testing: not performed in this review. Earlier device results remain tied to their own revisions.
Recommended before device testing: wait for the in-progress unit-tests run on this head.
Suggested additional test cases
- iOS, with payment PIN enabled, biometrics disabled, and a funded regtest wallet. Start a send that has already passed payment authorization and is still submitting. Background the app and complete app unlock. The confirmation stays busy, does not request payment authorization again, and the original payment completes once.
Reviewer model: grok-4.7
Reasoning effort: high
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.
October 26 master integration
Published integration head
3f4e60097668b5ffb5106a71a6e6e74d201dd8caincorporates actual master2ce210252be8ae7ea130d2422a2fa704a191ef7dthrough ordinary signed merges, preserving the stacked base and prior child changes. The two parent conflict resolutions retain review-state ownership, root-safe subscription Back navigation, one outer USDT status task, prompt recipient-scoped proof reconciliation and the terminal-refresh exit. No local assertion/timeout weakening or new test suppression was introduced.Validation: two 19-target builds and two canonical hosted unit suites passed: 2,910 on the parent and 2,937 on the combined tree, zero failures/skips with the exact six CI integration exclusions. This PR is covered by the combined stack/sibling tree
f471e8a6(2,937 units), including this branch; these are two suites, not eight independent runs. Full SwiftFormat0.64 passed on all eight final trees (0/844); translations had zero errors and 15 warnings matching execution on complete actual-master source. Independent frozen integration review found no actionable issue. Local environment: Xcode26.1/SDK26.1, iOS26.5 simulator.Complete diagnostics retained: two App Intents notices per build; parent1,082/combined1,099 runtime scan records, including four/six SDK/Pubky/DHT QoS stacks, fault fixtures and hosted appearance/identity notices. Source comparisons are not exact-base runtime replay or warning-free evidence. New-head hosted CI follows publication. Master's #1130 retired Blocktank skips and E2E285 channel/migration omissions remain coverage gaps, not fixes for original #1109. Historical #1054 preparation/UI-launch causes remain unresolved. Existing QA case IDs and native gaps remain: no new payment, funded USDT, physical hardware, latency or merchant-unlock journey was run by this integration.
October 26 lifecycle integration and camera fixture
Published head
ac364f01ff1b6cee95c12c5d978c27aaa3f23f09includes approved and merged #1098 through masterb8b7f3d9902de5669c025bb839232c1d7e98d2df. The overlapping lifecycle test retains one owned switch task per current context, holds resumed preparation until confirmation is visible, and keeps the exact-one-success assertion, cancellation/context guards and retained presentation/app-lock behavior. Seven ordinary signed merges preserve stacked bases and prior child changes; #1084/#1085 also retain merged #1081 through #1070.The existing unfunded USDT UI case now resets camera permission before launch, explicitly waits for the camera alert, allows it and waits for dismissal before manual entry. This adds six test-only lines and two assertions; no test, assertion or timeout was removed or weakened. Production camera behavior and the merged Receive blank-band checks are preserved.
Final validation passed two 19-target builds, 2,913 parent canonical units and 2,940 units on combined tree
db653be56e228dec55f4e6c0fd57e42e7d8475c2, with zero failures/skips and the exact six CI integration exclusions. This branch is included in that combined composition. These are two suites, not seven independent runs. The combined unfunded UI case passed, including both Receive blank-band swipes and explicit camera handling, using runner-only loopback endpoint settings. Full SwiftFormat 0.64 passed on all seven final trees (0/844); translations are unchanged. Independent final review found no actionable issue. All 151 distinct commits across local current/prospective base ranges verified cryptographically; all 129 distinct commits across the final published GitHub ranges report verified=true/reason=valid.Complete diagnostics remain retained: parent 1,107/combined 1,125 broad records, including four/six SDK/Pubky/DHT QoS stacks, App Intents notices, hosted appearance and fault-fixture messages. The combined shared-keychain -25300 publication diagnostic has unchanged emitter/test source; its exact runtime cause remains unestablished. Source comparison is not exact-base runtime replay or warning-free evidence.
The old Receive-dismissal failure in #1070 run 40394372963 did not reproduce in the controlled original-head run; that run passed and used XCTest's default camera interruption handler. The camera-fixture comparison passed with an identical 157-file production app bundle. Neither result establishes the Receive failure's cause or repair. Local Xcode 26.1/SDK 26.1/iOS 26.5 differs from hosted CI; the completed hosted results are recorded below. The daemon launch issue #1110, original hardware-transfer issue #1109 and earlier coverage gaps remain. No new payment, funded USDT, physical-hardware, final latency or merchant-unlock result is claimed. João, Piotr and Ovi remain requested; existing pending requests are preserved without duplicate notification.
October 26 hosted CI confirmation
For published head
ac364f01ff1b6cee95c12c5d978c27aaa3f23f09, unit/UI run 40412986788 passed on attempt 1: 2,937 unit tests, zero failures/skips; 43 UI executions, 39 passed and four existing prerequisite skips, zero failures. SwiftFormat 40412986734 passed: 0/844 files require formatting, 84 skipped. These workflows tested GitHub's PR merge commit using Xcode 16.4/SDK 18.5 on iOS 26.2.Complete workflow diagnostics were inspected. The 571 duplicate-symbol records match the retained passing #1025 run 40376172874; workflow/cache-action sources match actual master. This is retained-log/source comparison, not an exact-base runtime replay or warning-free result. The remaining red aggregate contains cancelled duplicate jobs; the replacement unit/UI and formatting workflows passed.
No manual CI rerun was requested. Historical Receive-dismissal/daemon-launch causes and original hardware-transfer issue #1109 remain unresolved; existing skipped/deferred coverage and final Android 206/iOS 215 payment/unlock measurement remain incomplete. Existing human review requests are preserved.
October 29 inherited app-lock guard
Published d9566ad8 carries the four-file R1 correction from #1039 at aae380fc: a recreated confirmation uses the retained submission flag for its busy state and refuses another authorization while the original payment is in flight. This branch retains its existing Core 0.8.0-rc10, LDK 0.7.0-rc.77, Paykit 0.1.1-rc1 and VSS 0.5.23 pins, stacked base and prior history.
On this exact branch candidate, Xcode 27.1 / SDK 27.1 / iOS 26.5 passed the complete selected unit suite: 2,939 passed, 0 failed, 0 skipped, with the six existing unit-workflow integration exclusions. Both new confirmation regression tests passed. Full SwiftFormat 0.64.0 passed: 0/844 files require formatting; 84 existing exclusions. The shared private package copy was corrected before this branch's first test invocation; the sibling #1054's initial build-path failure remains retained. This branch's first full test run passed with unchanged source and dependency pins.
Complete diagnostics were inspected: two AppIntents metadata notices, six native SDK/Pubky/mainline DHT construction QoS warnings, and retained fault-fixture/host output. Forty-five emitter/test files were compared with this branch's pre-fix head; the warning-emitting methods in the two changed files are unchanged. No introduced actionable finding was found. This is source and retained-evidence comparison, not an exact-base runtime replay or warning-free claim. The identical production patch reuses the two independent R1 source reviews and guard negative control from #1039, with the older child callers inspected separately.
All 28 commits in the actual current-parent PR range verified cryptographically locally and report GitHub verified=true / reason=valid after the normal push. New-head hosted checks remain pending and validate their own recorded PR-merge inputs separately from this local older-pin branch.
Existing cases J1, J2 and manual 1 and their documented fixture/admission prerequisites remain unchecked on this new head. The external native R1 report on #1039 reports two successful controlled regtest executions at aae380f; it is attributed parent evidence, not a device run on this child. No new financial journey, binary adoption or final Android206/iOS215 latency/unlock result is claimed.
Models used
gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:xhigh)gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:xhigh)gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:high)