Repository navigation
perf: keep request creation ahead of refreshes - #1085
ben-kaufman wants to merge 3 commits into
Conversation
|
Milestone 3.2.0: it is stacked on #1070 (3.2.0), the same Paykit latency track. |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Holds a review token while a one-time payment request checks eligibility, commits its proposal and attempts targeted delivery, so background refreshes wait. Reviewed bee8af8, full tier, reasoned from the code and CI: iOS does not build on this host.
What I checked, and 3 candidates I ruled out
Read in full: the diff; PaykitPaymentRequestManager.propose, PaykitPaymentRequestService.propose, discoverEligibleTargets, proposalWasSent, processPendingMessages, PaykitPaymentActivity, PubkyService.checkRefreshAdmission
Call sites traced: shouldDeferRefresh and waitUntilIdle(includingReviews: true) (contact publication, message drain retries, deep links, proof refresh)
CI: SwiftFormat green; Run Tests still pending at review time
Ruled out
- Creation deferring its own SDK calls: every call on the propose path runs at
.interactive(identity status, linked peers, proposal, targeted delivery, shared requests), andcheckRefreshAdmissiononly rejects.background, so the token cannot block its holder. - Token leaked on throw or cancellation:
endReviewsits in the samedeferasisCreatingRequest, outside the generation check, so it runs on every exit; the new cancellation test checks it. - Idle waiters stuck behind a released token:
endReviewresumes them once reviews are empty, and they re-check payments before proceeding.
Merge confidence: 4/5, Run Tests is still pending and the branch was reasoned about, not built here.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the runner does not report it)
 [Medium impact] This PR appears safe to merge. SummaryRequest creation now holds a review token while it checks the recipient, stores the proposal, and attempts delivery.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start request creation] --> B[Begin review token]
B --> C[Check recipient]
C --> D[Store proposal]
D --> E[Attempt targeted delivery]
E --> F[Return sent or queued request]
C --> G[Error or cancellation]
D --> G
F --> H[Release own review token]
G --> H
Reviews (1) · Last reviewed commit: "perf: keep request creation ahead of ref..." · Reviewed by Greptile |
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 merge base b1d65a7f, at bee8af89. Full coverage of bitkit-ios#1085; no earlier assessment baseline.
No new actionable code findings.
One-time creation in PaykitPaymentRequestManager.propose holds a shared PaykitPaymentActivity review from just before the service call until the function returns. That span includes the fresh eligibility read, proposal commit, and targeted delivery. The same defer ends that review on success, queued delivery, rejection, cancellation, clear(), and identity replacement, while a separate review token stays held. checkRefreshAdmission rejects .background work while the review set is non-empty and admits .ordered and .interactive work. Creation uses .interactive for identity status, linked peers, the proposal, targeted delivery, and the follow-up shared-request read. Subscription creation is outside this token. Compared only that one-time admission contract with bitkit-android#1661 at e552783b, which also wraps one-time propose and leaves subscription creation outside the token.
Validation: read the added and updated PaykitPaymentRequestServiceTests cases and did not run them. SwiftFormat passed on this revision. Run Tests was still in progress for bee8af89.
Device testing: not performed in this review. Journeys and manual tests are N/A in the PR because this is internal admission with no user-visible change.
Reviewer model: grok-4.7
Reasoning effort: Not exposed (cursor-cloud run-info reports originalModelName and has no reasoning-effort field)
![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 310e689:
Test 1: focused automated regression ✅ The exact head built and both payment-request and payment-activity XCTest suites passed.
Run with Xcode 27.0 on an iOS 26.5 simulator. I checked admission through eligibility, proposal commit and targeted delivery, cancellation cleanup, queued delivery, rejection, identity replacement and preservation of another review through the XCTest assertions and result bundle. SwiftFormat lint also passed for both changed files.
automated-2x.2x.mp4
XCTest host recording at 2×; assertion results establish the behavior.
No manual device journeys were run: the QA notes request none, and this internal scheduling change has no user-visible behavior. No native timing improvement was measured. The current-head CI test job also passed.
Full review: 7cd096f4 to 310e689a, both changed files; the admission change was exercised through automated checks rather than a manual device case.
Twin: synonymdev/bitkit-android#1661 perf: keep request creation ahead of refreshes at e552783b matches the one-time creation admission scope.
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: Re-reviewed the full PR at 310e689a after the comparison base changed from e16d13bc to 7cd096f4 and no verified baseline coverage was supplied. This pass covers the current diff from merge base 7cd096f4.
No new actionable code findings.
One-time creation in PaykitPaymentRequestManager.propose holds a shared PaykitPaymentActivity review from just before the service call until the function returns. That span includes the fresh eligibility read, proposal commit, and targeted delivery. The same defer ends that review on success, queued delivery, rejection, cancellation, clear(), and identity replacement, while a separate review token stays held. checkRefreshAdmission rejects .background work while the review set is non-empty and admits .ordered and .interactive work. Creation uses .interactive for identity status, linked peers, the proposal, targeted delivery, and the follow-up shared-request read. Subscription creation stays outside this token. Compared only that one-time admission contract with bitkit-android#1661 at e552783b, which also wraps one-time propose and leaves subscription creation outside the token. Already-admitted SDK work can still finish.
Validation: this review did not execute the suite. Run Tests passed on this revision; its log shows PaykitPaymentRequestServiceTests compiled and testProposeBuildsOneTimeBitcoinTermsAndDrainsOutbox and testCancelledProposalReleasesReviewAndAllowsAnotherCreation passed. SwiftFormat passed on this revision. The unit-test workflow includes this suite.
Device testing: not performed in this review. Journeys and manual tests are N/A in the PR because this is internal admission with no user-visible change.
Reviewer model: grok-4.7
Reasoning effort: Not exposed (cursor-cloud run-info reports originalModelName and has no reasoning-effort field)
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the entire PR diff against merge base 05c92291, at f73b5747. Inherited coverage is empty because the comparison base changed from 7cd096f4 to 05c9229 and no verified baseline was supplied.
No new actionable code findings.
One-time creation in PaykitPaymentRequestManager.propose holds a shared PaykitPaymentActivity review across the fresh eligibility read, proposal commit, and targeted delivery. The same defer releases that review when the attempt returns or throws, including success, queued delivery, rejection, cancellation, and an in-flight attempt that finishes after clear() or an identity change. Background refresh admission rejects only background work while a review is open; eligibility, the proposal write, and targeted delivery stay on the interactive lane. Subscription creation leaves the token untouched. The same one-time boundary is present in bitkit-android#1661 at e552783b. A background SDK operation that already passed admission can still finish.
Validation: this review did not execute the suite locally. Run Tests passed on this revision; its log shows PaykitPaymentRequestServiceTests compiled and the creation-admission cases passed, including eligibility-through-delivery, cancellation cleanup, queued delivery, pre-commit rejection, and identity replacement. SwiftFormat passed on this revision.
Device testing: not performed in this review. Journeys and manual tests are N/A because this is internal admission with no user-visible change.
Ready for device testing.
Reviewer model: grok-4.7
Reasoning effort: xhigh
4 participants
Twin: synonymdev/bitkit-android#1661.
This PR holds the existing review admission token while a one-time payment request checks eligibility, commits its proposal and attempts targeted delivery. Stacked on #1070.
Description
Out of Scope
None.
Design
N/A — no UI changes.
Preview
N/A — no user-visible behaviour change.
QA Notes
Setup
N/A — no device journeys or manual cases are requested for this change. Automated coverage is listed below; the shared journey guide remains the device-test reference.
Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
PaykitPaymentRequestServiceTests.swift— exercises creation admission through the actual caller, cancellation cleanup and a subsequent creation.PaykitPaymentRequestServiceTests.swift— checks queued delivery, pre-commit rejection, identity replacement and preservation of another review token.Evidence
The exact-base negative control failed at the expected refresh-admission assertion.
Final integration validation
Integration with master
4530ff7bat310e689aretains both USDT-warning cancellation and the payment-review callback. Bottom #1025: all-target build passed (19 targets), 378 focused tests passed, 0 failures/skips. One combined #1070 + #1081 + #1084 + #1085 composition, tree8f1e72d3from #10707cd096f4plus #10818967dff8, #1084b7561c08and #1085310e689a, covers the stack and those sibling changes: all-target build passed (19 targets) and 2,779 tests passed, 0 failures/skips using the six existing CI exclusions. These are not eight independent full-suite runs.All eight heads passed SwiftFormat. Translation checks report 0 errors and 15 warnings, all matching master by key, language, file and occurrence count, including
security__use_pin. Final toolchain/runtime and complete diagnostic disposition: Xcode 26.1 / iOS 27.0 simulator, serial tests; complete diagnostics inspected, with six SDK-construction QoS warnings and retained fixture/appearance messages matching baseline evidence; no introduced finding. Final-head CI after publication: pending.Gaps
Already-admitted work is not preempted. No native timing improvement is claimed.
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. 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)