Repository navigation
fix: refresh Paykit views and local completion - #914
Conversation
|
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Swaps the request and contact display reads for one identity- and generation-checked observeState observation, and publishes verified hardware-payment resolution before proof delivery on the reconcile path. Reviewed abc28553, full tier, reasoned from the code, the pinned paykit-rs source and CI; iOS is not built here.
What I checked, and 4 candidates I ruled out
Read in full: completeHardwareOnchainPayment, consumeOnchainPaymentResolution, submit/submitInBackground (PaykitPaymentProofService.swift), PaykitSdkService.observeState, withSdk, refreshPaykitKey, PaykitSdkOperationLock (PubkyService.swift), ContactsManager.loadContacts, PaykitPaymentRequestService.synchronize, PaykitPaymentRequestManager.refresh/performRefresh, SendPendingScreen.applyOnchainPaymentResolution
SDK side: observe_state, contact_records, list_payment_requests, load_remote_state at paykit-rs 0eff3d4 (tag v0.1.0-rc73, matches Package.resolved)
Call sites traced: onchainPaymentResolutionPublisher (AppScene, HwSendSignView, SendPendingScreen), loadContacts(for:) (AppScene, ContactsManager), service.synchronize (only performRefresh)
CI: validate, detect-changes and Greptile green; Run Tests, Run Integration Tests and build-local still running at review time
Ruled out
- Duplicate proof submission when resolution is published before
submiton the reconcile path:consumeOnchainPaymentResolutioncan now interleave with the foregroundsubmit(completed)and start a secondsubmit, both doing a check-then-submit againstpaymentRequests(). The same interleave already exists on the defaultdeliverInBackgroundpath, and the SDK documents a repeated proof for the same occurrence as evidence that implies no second payment. observeStatefailing where the old getters returned data: signed-out, no Paykit identity secret, and uninitialized identity all errored on the oldcontact_records()/payment_requests()path too, because the shared storage transaction needs the same session and secret. A missing blob with no registry noise key still yields default state, somissingDataIsEmpty: falseloses no empty-list case.performRefreshnow requiresactiveIdentitybefore syncing: the only state this skips is a manager whose stored acceptance or subscription state failed to load. Outbound proof delivery still runs fromPaykitPaymentProofService.submitand the otherprocessPendingMessagescallers.- Dropped identity guard after consume in
SendPendingScreen:applyOnchainPaymentResolutionalready matchesresolution.identityagainstpubkyProfile.publicKeyon entry, and navigation now runs on the main queue via the added.receive(on:).
Merge confidence: 4/5, the unit and build jobs were still running and the iOS target was not built here.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 3d2283e: no HIGH or MEDIUM finding and nothing to raise inline. The device result follows in a second review; not approving until it has run.
Checked and clean:
- Durable before publish.
completeHardwareOnchainPaymentverifies the exact transaction and persistsproofDataandonchainAcceptanceVerified, runs the activity follow-up, persistsonchainLocalFollowupCompleteandonchainResolutionAwaitingConsumption, and only then publishes the resolution and submits. A crash between publish and submit re-enters on the nextreconcile(), skips lookup and follow-up, republishes and submits: no second signing or broadcast. - No second payment. The request stays off the payable list while the row exists, and
hardwareCompletionsInProgressblocks a concurrent completion for the same key across the actor's awaits. - Consume against submit.
consumeflipsawaitingConsumptionundermutationLockand submits that copy; the reconcile'ssubmitholds the stale copy, so itsremoveRequestProofspredicate does not match and cannot delete the row.isAlreadyQueuedprevents a duplicate proof. SendPendingScreen. The resolution is matched against the current identity and now also againsthardwareTransactionId; delivery is on the main queue; the.taskreplay andonReceivereplay end in oneconsume. Close hides the sheet and leaves the row and the resolution in place, so reopening replays it. Back and swipe-back are disabled.- Observation binding.
observeStateruns under the operation lock afterrefreshPaykitKey()and checks identity and key generation;performRefreshre-checksactiveIdentityafter the await;ContactsManagerre-checksloadGenerationandcontactsRevision, and sign-out bumps the generation. - A failed observation keeps the previous display for both contacts and requests.
- SDK pin.
0.1.0-rc73resolves to0eff3d4, whose parent is the rc72 commit; the request filter is the same function the old getter used.
CI is green at this head.
Device gate (partial): 3d2283e — not run yet. Pending, against a master build on the same simulator: time from opening Contacts to the list publishing now that it reads the full state observation (contacts, linked peers, requests and receipts) where master read contact records only, with an empty and with a populated request history; request-refresh time; and shop-onchain-proof.xml if the Trezor emulator and issuer fixture can be stood up for the simulator. Performance-relevant: the PR states a speed purpose and changes what runs under the SDK operation lock.
jvsena42
left a comment
There was a problem hiding this comment.
Device result for 3d2283e. No finding from the run. Not approving yet: shop-onchain-proof.xml, the only journey that exercises the Pending → Success change, has not been driven by anyone.
Device gate (partial): 3d2283e — perf: Contacts reload 1.02 s median, range 1.00–3.74, 8 of 8 completed (master 4e0c8ba: 1.40 s median, 1.07–2.14, with 2 of 8 not finishing in the 5 s on screen); request refresh 3.60 s median, 2.69–3.98, n=12 (master 4.86 s, 3.75–6.61, n=11); refresh on opening Subscriptions 5.92 / 3.78 / 3.71 s (master 4.74 / 4.80 / 15.66 s); five quick switches in and out of Contacts start five loads on both builds with no "already loading" storm; no hang or crash report. iPhone 17 simulator, iOS 26.5, regtest, staging backends.
Read with this limit: the identity has 2 contacts, 1 pending request and one retained hardware proof. That is close to an empty history, so the run does not stress the part of the change that could cost more, which is Contacts now reading the full state observation (requests and receipts included) where master read contact records only. At this size the PR is not slower than master on any path measured.
The first Contacts open after a cold launch took about 21 s on both builds whenever it landed before the Paykit session was restored (about 64 s after launch, after Deferred session restoration warnings). Same on master, so it is not this PR.
Refresh was timed with breakpoints from performRefresh entry to persistSubscriptionState, since it logs nothing at either ref.
shop-onchain-proof.xml: it needs a linked paykit-server issuer fixture, an E2E Bridge build of this head and the Trezor emulator. None of the three was up; I will retry it on the next pass.
jvsena42
left a comment
There was a problem hiding this comment.
No HIGH or MEDIUM finding at 3d2283e, in the code scan or on the device. Approving; the code trace and the measurements are in my two earlier reviews today.
Device gate: 3d2283e — journeys: 0 driven by me; perf: Contacts reload 1.02 s median (master 1.40 s), request refresh 3.60 s (master 4.86 s), with 2 contacts and 1 request; no re-entry storm, no hang report; Figma: no screen changed.
Skipped — covered by CI at 3d2283e: unit tests, integration tests, e2e (all green)
shop-onchain-proof.xml. It needs a linked paykit-server issuer fixture, which is not available on my setup; hardware Shop payment coverage is left to the e2e CI. The Pending → Success ordering rests on the code scan and testHardwareReconciliationPublishesDurableResolutionBeforeProofDelivery.
|
I added myself as reviewer because:
|
|
Addressed the stale J1 selectors from this QA report in signed commit 532eb15. The journey now uses the existing iOS Validation: XML parses; all 12 actions remain, with only the platform lookup in action 6 changed. The complete 102 SwiftFormat findings and 1,898 translation warnings match the previously base-verified reports, with zero introduced findings. A fresh frozen correctness/native-pattern/polish review found no actionable issue. This documentation-only follow-up did not rerun native tests or devices. J1 remains unchecked: the reported run stopped before signing, and the corrected journey still needs signing and proof-delivery validation. Ovi's pending review is preserved; no device pass or approval is implied. |
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 532eb15. The one commit since my last approval changes journey text only; the code review and measurements from 3d2283e stand.
HardwareSendSign(HwSendSignView.swift:113) and theHardwareSendSignAmountprefix (:36) exist, and "To address (confirm on device)" is thehardware__send_confirm_addressstring, so the reworded action names what the screen has. NoHardwareSendAddressidentifier exists on iOS, which is what the new README row records.- The Android names in the new README rows (
HardwareSendAmount,HardwareSendAddress) matchHwSendSignScreen.kton Androidmaster.
shop-onchain-proof.xml needs the paykit-server issuer fixture, which is not on my setup; it is left to the e2e CI and to the QA run already on this PR. perf: unchanged from my review at 3d2283e (no code changed).
There was a problem hiding this comment.
Verdict: ✅ Approve
QA for 532eb15:
Tests for the review: journey J1 passes.
Tested on iOS 26.5 simulator (iPhone 17 Pro), regtest.
Request: Ovi asked to use the available local issuer setup; J1 completed signing and verified the original hardware transaction and its exact issuer proof.
Request: The author asked for corrected J1 signing and proof checks; J1 completed signing and verified the original hardware transaction and its exact issuer proof.
🟢 Test J1
Test J1
Trezor signed one 100,000-sat payment; the original hardware transaction and issuer proof matched, with no second payment.
J1.mp4 |
Note
J1 checked in the PR description.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
This PR:
Related: SDK #183, Server #78. Pins published rc73; neither PR needs to merge for this consumer build.
Description
0eff3d4e694ba0dbbea2d47852fe796e456671b0; the actual SwiftPM archive matches published SHA256511715acf3ca84e322d19ea6651c767c98b5325845d571abe2a9527b1eb6ae26, with no local override or unrelated dependency change.Out of Scope
Design
N/A - no UI layout or copy changes.
Preview
The J1 QA report and recording on
532eb159reports a completed regtest payment on an iPhone 17 Pro simulator running iOS 26.5: Trezor signed one 100,000-sat payment, and the original hardware transaction matched the issuer's exact proof, with no second payment. This is reviewer-reported simulator/Bridge evidence; it does not establish physical USB/BLE validation.QA Notes
Journeys
shop-onchain-proof.xml- reviewer reports the common hardware payment/proof journey passed on532eb159, including signing and the original transaction's exact issuer proof without a second payment. This supersedes the earlier pre-signing selector/input blockers. The report does not establish injected broadcast/response-loss faults, held-delivery/restart ordering, payer-switch or later activity-edit recovery checks; those remain separate from this reported pass. Held-delivery/restart ordering retains deterministic unit coverage, not new device evidence.Manual Tests
N/A.
Automated Checks
ContactsManagerTests.swift,PaykitPaymentRequestServiceTests.swift- preserve prior displays on failed/wrong-identity observations and keep cached display separate from action authority.PaykitSdkIdentityCheckTests.swift- enforce observation identity and cancellation without overlapping native operations.PaykitPaymentProofServiceTests.swift,TransferServiceActivityTests.swift- prove durable held-delivery/restart recovery and main-thread Pending completion before held acknowledgement, with exact-context rejection and proof retention.ContactsListViewTests.swift,ProfileDestinationViewTests.swift,PaykitPaymentRequestServiceTests.swift- cover the explicit display loader and full-observation failure.3d2283e2replaces fixed navigation/acknowledgement sleeps with bounded completion expectations; all 2,371 tests passed again with no introduced diagnostics and a clean bounded independent review. Production code and the measured read path are unchanged.Measurement Limits