Repository navigation
feat: share paykit state across apps - #856
ben-kaufman wants to merge 89 commits into
Conversation
|
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 72 files.
Pair PR synonymdev/bitkit-android#1401: equivalent.
Findings:
3 inline (1 MEDIUM, 2 LOW)
QA:
Tests running: 7 of 9 passed.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
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 ab88d1c9, at 4c7f705.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Compatibility with the unreleased receiver-path backup, pending-proof, and reservation formats is an intentional out-of-scope break. Paykit has not launched, and this PR does not claim those development documents remain readable. Authorization still shares only the requested watch-only account and generation-bound Paykit secret, and payment requests resolve through the request endpoint rather than a later private list.
The new marketplace wallet-leg consent step does not match the on-screen Paykit access copy or the Android companion's action text.
GitHub reports unit tests and integration tests succeeded on this revision. This review did not run them. The local e2e job was still running and is not evidence. bitkit-android#1401 was compared only for the updated consent journey step, not reviewed in full.
Recommended before device testing: correct the wallet-leg Paykit access action so that journey checks the localized consent copy.
Device testing: not performed in this review.
Findings
- [LOW] Wallet-leg journey checks the wrong Paykit access copy — inline at
journeys/pubky-marketplace/wallet-leg.xml:19.
jvsena42
left a comment
There was a problem hiding this comment.
One MEDIUM (two installs can pay one request twice) and two LOWs inline. Paykit is ungated on main, so these are user-facing from the next release.
Checked and clean:
- Auth sheet: approval is pinned to the immutable
config.request, withrawUrlre-checked inapproveAuthRequest. The requesterclientIDandrelayOriginare displayed. A Paykit-only claim skips watch-only account allocation, while a combined claim still goes through the watch-only consent step.PubkyAuthClaim.encoderefuses mismatched payload and claim combinations. - The exported Paykit secret is a one-way blake3 derivation of root and generation, signed and encrypted to the relay channel. Nothing logs the payload.
- No new auto-start payment path. Amounts are still gated by
validateIncomingPaymentRequestAmounts, endpoints are limited toacceptedPaymentEndpointIdentifiers, and the post-broadcast lookup reuses the capturedcontactPaymentContext. - No app-group or keychain-access-group changes, and
Env.keychainGroupstays private. - Biometric and PIN checks run in
submitPaymentbeforeperformPaymenttakes the execution claim, so declining auth leaves no claim. - Not raised, because nothing reaches them today: claims are never released on abandon (no
releasePaymentRequestExecutionClaimcall site), and a missing registry counts as generation 1 against the saved floor (PubkyService.swift:1051). Both start to matter once a second executor app, or key rotation, exists. Same on synonymdev/bitkit-android#1401.
Non-blocking: is there a Figma frame for the new PubkyAuthPaykitAccess block in the approval sheet? Link it and I'll diff the implementation against it on the next pass.
There was a problem hiding this comment.
Verdict: ⛔️ Request Changes
Retest for the review: journey J8 fails; journey J4 passes now.
QA:
Tested on two iOS 26.5 simulators (iPhone 17 Pro), regtest.
Tests J1, J2, J3, J5, J6, J7, J9 already done in review.
🔴 Test J8
Test J8
Written review Back control unavailable.
J8-retry-104009.mp4 | J8-retry-104009-buyer.mp4 | J8-retry-104009-resume.mp4 | J8-retry-104009-buyer-resume.mp4 |
![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() |
log
Timed out after 3000ms waiting for UI predicate exists for identifier NavigationBack.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
|
Updated in 71904a7. For the reported J8 failure, an automatically opened review is the root of the send sheet, so it has no Back button. The journey and README now use a downward swipe from the drag indicator. The consent step also matches Android. I have not rerun the full marketplace journey, so it remains unchecked. For the design question, no Figma frame was supplied for this authorization UI. The PR keeps |
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of the changes since 4c7f705, at 36ed45d. The inherited baseline is the full review of the PR diff against merge base ab88d1c. That base and merge base are unchanged, and 4c7f705 is an ancestor of this head. This pass covered the payment-ownership, received-payment attribution, reservation, keychain, and journey delta, plus the callers those paths use.
No new actionable code findings.
A one-time request stays payable on the install that stored its acceptance. Once that accepted state is visible, another install's refresh leaves the request out of pending and auto-presentation, and payment, retry, and send authorization require the local acceptance id. testOnlyAcceptingInstallCanResumeOneTimePayment covers the stale proposal and the restarted accepting install. The two-install payment comment matches this gate: the SDK execution claim still succeeds again for app id bitkit. Received-payment labeling requires the wallet receiving output and one contact across the transaction's mapped outputs, and it stops when the identity or reservation revision changes during lookup. The wallet-leg consent step now asks for private Paykit data and messages without sharing identity or spending keys, matching pubky_auth__paykit_access_description, and the automatic review is dismissed with a downward swipe. That resolves the previous consent finding.
This review did not run the simulator tests. Unit tests and integration tests were still running on this revision. Device testing was not performed. accepted-device-ownership and wallet-leg remain unchecked on the PR. The PR description's regtest payment report was not re-executed here.
jvsena42
left a comment
There was a problem hiding this comment.
Follow-up at 36ed45d. One MEDIUM is still open: recurring periods are double-payable across installs. I replied on the existing two-install thread rather than opening a new one.
Resolved:
- Two-install double pay for one-time requests. Every entry needs the local acceptance id: auto-present, notifications, list and detail, retry,
finishPayment, SendConfirmationView, LnurlPayConfirm, quickpay and the hardware path.ensurePaymentAllowedrequiresisApprovedForPaymentand re-checks generation, identity and approval after the asynclinkedPeerscall. A stale proposal on the second install fails at the SDK accept, which re-validatesProposedinside the locked transaction. - Backfill is skipped while the identity, contact snapshot, activity revision and reservation revision are unchanged. Every
ActivityServicewrite invalidates it, and an incomplete scan is not cached. - Attribution requires the receiving address to be an actual output and a single contact across all mapped outputs, and conflicts stay unlabelled. The live path re-checks auth, identity and snapshot after the async lookup.
- The ledger is keyed by normalized identity, removed by
wipeEntireKeychain(), and kept out of backups. accepted-device-ownership.xmlmatches the Android copy apart from identifiers.
Not raised:
- An accepted one-time request that no install owns stays blocked until the payee cancels. That is the stated trade-off.
- Activation failing closed on a ledger read error matches the existing subscription-store behaviour.
- ovi-reviewer's open J8 thread is not repeated here.
23d6ddb to
fe281dd
Compare
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 13 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-android#1401: equivalent.
QA:
Tests wait for CI.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
piotr-iohk
left a comment
There was a problem hiding this comment.
@ben-kaufman please resolve the merge conflicts. QA review has not been performed for this request.
|
Device gate at 5dd730b (rc68), final. Two simulators on staging, same identities as the rc65 run, UTC 2026-10-07. No crash; 0 Quiet launches regressed against rc65. After the first slow quiet launch I added two more rounds:
Across the run, 5 of 11 launches took about 60 s (the table above has the first seven), including 4 of 6 launches after three minutes idle. On rc65 and rc63 a quiet launch on these simulators was 11–12 s. The shape is always the same: one
Not run: the payment-deadline journey. The Create Payment Request screen only offers "Expires in" (1 hour at the shortest), with no payment-deadline field, so it needs a controlled issuer. The hardware-deadline MEDIUM in my review is therefore from code only. The review stays at changes requested for that MEDIUM. The launch regression goes to #868. Log linesApp log, UTC, Pubky and Paykit lines (the iOS log has no SDK-level lines). Keys and txids shortened. Slow quiet launch, 60.2 s (simulator A, quiet #3, Fast launch, 10.9 s (simulator A, Both republish the identity about 6 s after launch. The fast one restores 4.4 s later; the slow one is silent for 27 s, defers, and restores 27 s after that. Payment #1 on the payer, swipe ended 09:37:22 ( Of the ~15 s from swipe to broadcast, about 4 s go to consuming the payment list and about 10.7 s pass between Raw SDK error in a toast (simulator A, Pay tap 09:53:45, Re-add lost right after delete (simulator B, The first Save has no |
|
Historical test results — earlier revision (October 7, 2026) Tested iOS Both directions of the paired 1,000-sat request payment completed, with independent regtest confirmation. Recipient-sharing-OFF rejection and contact delete/re-add also worked; private Request/Pay availability recovered after re-add. Intermittent slow session restoration was observed, including 60.79s and 73.80s. Background preparation remained active, so this run does not establish performance after preparation fully settles. The detailed report includes differing sharing/relaunch conditions and the Android enabled-state observation correction. Full timings, limitations and attached logs/evidence. Repeated measurements are labelled Run 1–3, using the same profile. This is historical device evidence, not verification of the current head or a merge approval. |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 2 files.
No new findings; the rest is in the review.
QA:
Tests running.
Tests 1, J1, J9, J17 passed at 32fcc1d.
Tests 2-4, 6, J7 passed at 976538f.
Tests 6, J2, J4, J6, J11, J12, J13, J18, J19, J21 failed at 5dd730b.
Tests J5, J14 passed at a684677.
Tests J8, J10 passed at cb23093.
Test J15 passed at 5dd730b.
Replies:
@piotr-iohk: The same-identity re-import stalled and prevented settled-deletion measurement. (comment)
The later rc68 re-imports completed with preparation retries active. Strictly settled deletion remains unverified.
@jvsena42: The rc63-to-rc65 upgrade restored both identities; latency and remaining device checks stayed open. (comment)
I have retained that upgrade result separately from the earlier rc62-to-rc63 failure. It does not close the remaining device or latency checks.
@ben-kaufman: Linked-contact recovery needs logs from both peers, the SDK error and the last successful link check. (comment)
This documentation change supplies no new peer logs or recovery evidence. Linked-contact recovery and fixture-blocked journeys remain unverified.
@piotr-iohk: Three rc68 imports and two deletions completed, but peer retries persisted and strictly settled deletion remains unverified. (comment)
I have retained the successful re-import results and the background-overlap caveat. They do not certify strictly settled deletion or private-link readiness.
@ben-kaufman: Hardware reconciliation now preserves wallet scope and view-recreation state; real-device lost-response testing remains open. (comment)
The new manual checklist covers lost responses, expiry, completion, view recreation and mismatched resolutions. Those fault-injection checks still require device execution.
@jvsena42: Quiet launches regressed to about 60 seconds; deadline testing needs a controlled issuer and was not run. (comment)
I have retained the launch regression under #868 and the controlled-issuer requirement. This documentation delta changes neither restoration nor the expired-retry behavior when no transaction reconciles; it provides no device evidence to close those concerns.
Note
Retest Suggested J19, 7
@ovi-reviewer retest J19,7
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: follow-up at 05ec22d3, covering the entire delta and affected recovery, persistence and UI paths since 5dd730b2. Unchanged coverage is inherited from the completed baseline; Git ancestry, base and merge base were verified.
No new actionable code findings.
The issuer deadline documentation is corrected. The existing expired hardware retry with no reconciled transaction concern remains: a broadcast that never reached the network supplies no resolution to unlock the sign screen. The new matching-transaction recovery path does not settle that case. The view-recreation regression-test request also remains; the author now explicitly requires manual fault injection. Earlier startup/performance reports are not cleared by this delta.
Validation: Swift parsing and a standalone harness using the pinned coordinator with dependency stubs passed uncertain-broadcast, expired-retry, wallet-mismatch and no-rebroadcast checks. The harness omits the observation macro and does not establish SwiftUI event delivery; native XCTest suites were inspected, not executed locally. Matching unit CI and integration CI were still running. Targeted source comparisons used Android 804e7547 and BitkitCore b53fa54a; neither was reviewed in full. The older Android coordinator does not establish parity for the new iOS recovery observer. Concurrent wallet/shared-Pubky installations and pre-2.6.0 profile compatibility remain excluded by the supplied support rules.
Device testing: not performed in this review. The author's deadline and hardware lost-response/view-reattachment checks remain unexecuted here and require a controlled issuer and fault-injection setup.
This comment has been minimized.
This comment has been minimized.
|
Fixed the raw Paykit error and a contact re-add navigation race in d99d5a6. Paykit failures now use the existing localized retry message, and cancellation stays silent. A delayed deletion callback only returns to Contacts if the user is still viewing or editing that same contact, so it cannot dismiss a newer Add Contact screen. 439 focused native tests passed, including these paths and the hardware-expiry fix. The full quick-delete/re-add sequence still needs a staging recheck on this head. Startup and linking performance remain open; these fixes do not establish that those delays are resolved. |
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed 05ec22d..d99d5a6. The hardware-deadline MEDIUM is fixed and resolved. One new MEDIUM inline, introduced by the contact navigation fix in this commit.
Checked with no finding: contactPaymentErrorDescription (PaymentNavigationHelper.swift:295-299) maps every PaykitError, bare or wrapped in AppError, to the localized retry message and CancellationError to nil, at all three toast sites; the specific messages for no endpoint, link pending and waiting for the updated list are unchanged. A delete of contact X can no longer pop an Add screen or another contact's screen.
Device gate: a retest of the quick delete → re-add sequence and the error toast is running on two simulators at this head; results will follow as a comment with log lines.
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 13 files.
No new findings; the rest is in the review.
The hardware deadline fix matches the pair PR synonymdev/bitkit-android#1401, which restores the prior navigation-lock state after queued expiry. Android already shows localized contact-payment failure guidance; the contact deletion route guard fixes SwiftUI callbacks specific to iOS.
QA:
Tests running.
Tests 1, J1, J9, J17 passed at 32fcc1d.
Tests 2-3, 5, J7 passed at 976538f.
Tests 6, J4, J11, J12, J13, J18, J19, J20 failed at 5dd730b.
Tests J2, J6 failed at 05ec22d.
Tests J5, J14 passed at a684677.
Tests J8, J10 passed at cb23093.
Test J15 passed at 5dd730b.
Replies:
@ben-kaufman: Keeping the linked-contact recovery result open. (comment)
The requested peer logs are still needed. Linked-contact recovery and fixture-blocked checks remain unverified.
@ben-kaufman: Real-device lost-response/expiry fault-injection remains an open QA check. (comment)
Lost-response, expiry, and view-recreation checks remain pending.
@ben-kaufman: I am keeping the startup and deadline checks open. (comment)
The reported timings remain open under #868. Hardware lost-response and deadline checks still need fault injection.
@jvsena42: Raw SDK error in a toast; re-add right after delete did nothing. (comment)
The SDK-error and re-add navigation fixes have regression coverage; the complete staging sequence remains pending. Quiet-launch delays remain tracked in #868.
@jvsena42: Quiet launches regressed against rc65; deadline testing needs a controlled issuer. (comment)
I verified the expired-retry correction in code and retained the controlled-issuer check for QA. The quiet-launch regression remains tracked in #868.
@ben-kaufman: The full quick-delete/re-add sequence still needs a staging recheck on this head. (comment)
The code and regression tests address both reported contact failures. The full quick-delete/re-add sequence and SDK-contention behavior remain queued for staging verification; startup and linking performance stay open.
Note
Retest Suggested J10, J11, J19, 7
@ovi-reviewer retest J10,J11,J19,7
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Device gate (partial) at d99d5a6 (rc68), two simulators on staging, UTC 2026-10-07. Launch after install: restored in 12.0 s (A) and 11.6 s (B), no Quick delete → re-add: fixed. Four attempts, each with the Add Contact deeplink sent while the deletion was still finishing and Save tapped 5–10 s after
All four ended on Contact Saved. No Delete from the Contact Saved screen (attempts 3 and 4) left the "Unable to load contact." screen; details and log lines are in the MEDIUM thread. Pay taps right after a re-add (three taps, 5–10 s after Contact Saved): each spun about 31 s, logged Still running: a Pay tap right after a relaunch (the case that showed the raw error before) and a sanity payment. |
|
Device retest at d99d5a6 (rc68), rest of the run. Two simulators on staging, UTC 2026-10-07. No crash; 0 Error toast: not exercised. No Pay tap in this run ended in a toast, so the localized retry message was not seen on a device. The case that showed the raw text before needs a sharing OFF/ON cycle before a relaunch, which I did not repeat. No raw SDK text appeared anywhere. The fix is confirmed in code only. Relaunch on B was slow again (65 s), same shape as before ( A contact deeplink sent during that minute was only handled at 12:06:37.169. For comparison, the launch after install on both simulators restored in 12.0 s and 11.6 s with none of these lines. Sanity payments:
No WARN or ERROR between the sheet and the broadcast. This payment ran about two minutes after B's slow relaunch and four minutes after a re-add, and every step was slower than in the earlier run on a settled pair; I have one sample, so I am not calling it a regression. A second, older request paid from A took 32.0 s from sheet to ready ( Pay right after a re-add costs about 39 s each time before the plain amount sheet: 31 s to Review state: changes requested stands for the Contact Saved delete MEDIUM. |
|
Interim test results for d99d5a6 The run is still going; the final review follows when it ends. This is a progress note, nothing is approved or requested here. 14 passed so far, 4 failed. 🟠 Test J4: failed at d99d5a6; waits for round 2Required fixed-address monthly proposal cannot be prepared. 🟢 Test 1: passed at 32fcc1d; its recheck on d99d5a6 is still to comeOn the exact-head binary, two Home → Bitkit foreground cycles (19:17:40–19:17:57 UTC) overlapped devctl offline/online (19:17:40–19:17:56) while native Pubky relay connections failed for 12 seconds, including the auth-relay interval 19:17:47–19:17:59; the device recorded one import/re-sign-in failure for that shared active interval and preserved its session for a later attempt. After healing the… 🟢 Test 3: passed at 976538f; its recheck on d99d5a6 is still to comeThe one-shot QA adapter fault threw SharedStateBusy after the native SDK returned an accepted record, before its caller received success; it did not intercept a native SDK read. After restoring the clean source and baseline app, restarting and refreshing retained the request and Pay action, and retry reached Bitcoin Sent for 21,000 sats. The fault build hash, nonce log, accepted record state,… 🟢 Test 5: passed at 976538f; its recheck on d99d5a6 is still to comeAccepted the 21,000-sat request while its real LNURL callback failed before invoice creation, then waited for All Synced and stopped the original wallet. Restored its recovery phrase on the replacement install, which recovered the accepted request, profile and Lightning channel, and retried to Bitcoin Sent after restoring the callback. The receiver recorded one settled 21,000-sat invoice and the… 🟢 Test J1: passed at 32fcc1d; its recheck on d99d5a6 is still to comeImported 62 disposable fixture friends into a profile originally created through Bitkit, then drove Edit Profile → Delete → Yes, Delete after the preparation sweep settled and again immediately after import with preparation active. Both recordings show the dimmed spinner and disabled profile actions, followed by profile onboarding in about 5 seconds settled and 3 seconds active; Home → Profile… 🟢 Test J3: passed at 32fcc1d; its recheck on d99d5a6 is still to comeThe linked rc64 fixture proposed a monthly 5000-sat request whose serialized terms prove payment_deadline null and the explicit fixed P2WPKH address; Bitkit accepted it and opened the funded on-chain confirmation without payment. After the issuer canceled it, the foreground timeline showed confirmation closing at t=2.1 s and the request moving to the Expired section; its details offered no Pay… 🟢 Test J5: passed at a684677; its recheck on d99d5a6 is still to comeAll nine actions driven in order. General toggle started at 1, one tap disabled sharing, then the completed update showed ContactPaymentsToggle value 0. A 30-second timeline retained value 0 after completion. Returned to wallet Home, reopened Settings and General, and value remained 0. The other wallet remained a distinct identity, not a second install of this wallet. Linked proof is the actual… 🟢 Test J7: passed at 976538f; its recheck on d99d5a6 is still to comeOpened the same fresh request on both independent wallets; A accepted it and showed SendFailure while the issuer confirmed Accepted without proof or receiver dispatch. B safely dismissed the stale review, then after restart retained the request in history without Pay; after A restarted, its retained Pay completed with SendSuccess, one new settled21,000-sat receiver invoice and one shared proof.… 🟢 Test J8: passed at cb23093The fresh Paykit-only request displayed exactly /pub/paykit with READ, WRITE access and private Paykit data/message consent without spending keys; watch-only consent was absent. Cancel dismissed authorization, the Contacts screen stayed stable for30.9seconds, and before/after watch-only account names, paths and tracking lists were identical and empty. 🟢 Test J9: passed at 32fcc1d; its recheck on d99d5a6 is still to comeCreated one active, tracked service account through the exact Server46 grant and recorded paykit account, m/84'/1'/1', tracking enabled, and its xpub from the wallet’s own persisted account metadata. Opened a fresh server-generated /setup/reconnect URL scoped exactly to /pub/paykit/:rw and paykit-access-v1; PubkyAuthPaykitAccess explained private data/messages and no identity or spending keys,… 🟢 Test J10: passed at cb23093; its recheck on d99d5a6 is still to comeBoth staging wallets saved each other and the funded payer opened ContactViewName, then RequestOrPaySheet. The recording shows Pay loading with Request disabled and SendAmount opening within 2.25 seconds; closing it restored ContactPay, and tapping Request opened PaymentRequestAmount. Identifiers were asserted through XcodeBuildMCP; the XML journey ran end to end without submitting a payment. 🟢 Test J14: passed at a684677Sent completion matched its description. The 21000-sat request opened automatically with Seat2Requester and Lunch last week; Show Details hid From/For identifiers and showed the contact and invoice note. The separate 5000-sat request with no note opened automatically and showed Not specified. 🟢 Test J15: passed at 5dd730bTerminated pinned staging app, opened exact public dummy watch-only-account-v1 URI from XML, and native PubkyAuthWatchOnlyConsent exists predicate succeeded. No system Open prompt was presented. Cancel was tapped; native gone predicate succeeded. Opened exact unsupported-v1 URI; native PubkyAuthInvalidRequestToast exists captured Invalid auth request. Final consent gone predicate succeeded. All… 🟢 Test J16: passed at 9cb87b8; its recheck on d99d5a6 is still to comeSeat1Creator proposed Journey Sub, 5,000 sats monthly, to linked contact Seat1Payer. Choose Recipient stayed disabled until the name and amount were set. A 600-letter description was rejected on Propose Subscription with "Shorten the subscription name or description and try again" and no sent confirmation. Replacing it with "Monthly support" produced the sent sheet (Sent / Proposal) for… 🟢 Test J17: passed at 32fcc1d; its recheck on d99d5a6 is still to comeCreated a monthly 5,000-sat proposal to the linked Seat1Payer contact. The 600-letter description produced the shorten-content error, which disappeared during the saved 30-second timeline without a sent confirmation; replacing it with Monthly support produced Sent Proposal for Journey Sub and a CREATED row labelled Proposal sent after OK. The other 10 tests run next. |
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: follow-up at d99d5a67, covering the entire delta since 05ec22d3, affected callers and tests, and the complete current PR inventory. Unchanged coverage is inherited from the completed baseline; ancestry, base and merge base were verified locally, and HEAD was rechecked before returning.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
The expired hardware-retry navigation concern is fixed in source. Started proofs retain reconciliation and duplicate-payment protection after dismissal. The contact-deletion regression was independently confirmed and reconciled with its existing canonical thread. Startup and linking performance remain unresolved.
Validation: changed Swift files passed syntax parsing. A standalone Swift harness using the pinned navigation helper confirmed that Contact Saved remains on the stack after deletion; it does not exercise SwiftUI. Native XCTest assertions were inspected, not executed here. The author's 439-test report is attributed evidence; matching unit and integration CI remained in progress when collected. Structured code-result and diff-anchor validation passed. Targeted comparison used Android 804e7547, whose older expiry path still locks dismissal; this does not establish current Android parity or a full Android review.
Device testing: not performed in this review. Hardware lost-response, no-reconciliation and retained-resolution view-recreation checks still require the author's controlled issuer and fault-injection setup. Concurrent wallet/shared-Pubky installations and pre-2.6.0 profile compatibility remain excluded by the supplied product rules.
Findings
- [MEDIUM] Return to Contacts after deleting from Contact Saved — inline at
Bitkit/ViewModels/NavigationViewModel.swift:209.
|
|
||
| func returnToContactsAfterRemoving(publicKey: String) { | ||
| switch currentRoute { | ||
| case let .contactDetail(shownKey), let .editContact(shownKey): |
There was a problem hiding this comment.
[MEDIUM] Return to Contacts after deleting from Contact Saved
After adding a contact, tap Delete on the Contact Saved screen and confirm. AddContactView sets .contactSaved(publicKey:), and MainNavView mounts ContactDetailView with showsDeleteAction: true for that route. Both deletion callbacks now call this helper, but its switch excludes .contactSaved, leaving the path unchanged while the contacts publisher clears profile. This path would leave the user on the deleted contact's empty screen instead of returning to Contacts.
Include the matching .contactSaved(shownKey) route and move it out of the negative test cases, while preserving newer Add and unrelated-contact routes. testDeletedContactDoesNotDismissANewerRoute currently asserts the regression.
Evidence basis: source analysis at d99d5a67 and an executed standalone Swift harness using this exact helper; no SwiftUI or device execution was performed here. This is the same mechanism as the canonical thread, which also contains an attributed simulator reproduction at this head.
There was a problem hiding this comment.
Fixed in 291328d, as detailed in the canonical thread: #856 (comment). Matching Contact Saved/detail/edit entries are removed, while newer Add and unrelated-contact routes remain. Navigation tests pass; fixed-head device verification remains open.
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed d99d5a6..291328d. No findings. The Contact Saved delete MEDIUM is fixed and resolved, verified on the simulator: three deletes from the Contact Saved screen all returned to Contacts with no error screen, Back did not restore the deleted contact, and each re-add reached Contact Saved. The Edit-screen delete still returns by itself.
Also in this commit: HwFundingSigner.swift:622 now drops the pending payment only on a definite pre-broadcast failure (InvalidHex, InvalidTransaction) with no prior attempt, matching Android 50811b2a5; Electrum and unclassified errors keep the signed transaction for retry.
Device gate: two simulators on staging at 291328d, contacts only (timeline and log lines in the resolved thread). Payments, launches and sharing were last driven at d99d5a6 and 5dd730b. The hardware deadline, reconciliation and failure paths are verified in code and unit tests only; the manual fault-injection checklist is unexecuted. The localized error toast from d99d5a6 was not seen on a device.
Open and tracked in #868: about one minute to restore the session in roughly half of launches, and the delete and Contacts-reload waits noted in the thread.
|
Updated to published Paykit rc69 in b633684, on top of the hardware and contact-navigation fixes in 291328d. SwiftPM resolved the release artifact and 610 focused native tests passed. The SDK now returns temporary lock contention after one bounded acquisition batch instead of repeating it as a revision conflict. The staging SDK test reduced a blocked call from about 26s to 3s, but still recovered only when the 60s lock expired. This is not a new mobile startup or linking speed claim; those timings remain open. The reviewer’s three Contact Saved deletion runs are now recorded as passed. Hardware fault-injection and the other unchecked device scenarios remain open. |
|
The matching hardware lost-response case now passed on b633684 / published rc69, using the normal iOS app, a Trezor emulator and disposable local regtest funds. Core independently accepted the signed payment while the proxy dropped the response and held reconciliation reads. Core made two initial attempts with identical bytes. Retry after the actual deadline added no broadcast attempt. Releasing the reads automatically reached Bitcoin Sent, returned Home, and delivered exactly one matching proof to the issuer. The attempt count remained two. This verifies that recovery path, not the whole checklist. View reattachment, process death and other injected failure combinations remain unverified. It is not a latency result or physical-hardware test, and no public-network payment was made. |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 12 files.
No new findings; the rest is in the review.
The hardware pre-submission classifier matches synonymdev/bitkit-android#1401 at 7bc3ccb; J22 has identical filename, name and action prose on both platforms. Published rc69 changes shared lock backoff for both mobile consumers without a new mobile latency claim.
QA:
Tests queued.
Replies:
Error toast: not exercised. (comment)
The localized toast remains a device QA gap. Contact Saved deletion is fixed in 291328d and separately confirmed in the canonical thread. The rc68 relaunch and payment waits remain open in #868; rc69 changes contention backoff without establishing current mobile latency.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
The latest CI unit failure was the test’s two-second wait for contact loading to start; its sign-out/publication assertions did not fail. 480b8d2 gives that setup wait ten seconds, with the existing load/sign-out gates and assertions unchanged. All 22 tests in the class passed ten repetitions after the edit. This changes only the test, not app delays or safety timeouts. CI is rerunning. |
Device gate (partial) — b633684 (Paykit rc69)Two simulators on staging, build installed over the rc68 state from 291328d. UTC 2026-10-07. Still running: payment request round trip, contact delete and re-add. Upgrade in place: the session restores on both from rc68-written state. No Relaunch (terminate, launch), first log line →
8 of 12 launches were deferred. Not deferred: 10.7–11.7 s. Deferred: 96.6–114.3 s, and one at 363 s. At d99d5a6 and 291328d (rc68) on the same simulators the deferred launches took 60–68 s with a single deferral, so in this run the slow case is about 35–45 s longer and takes three deferrals instead of one. The first launch after the install showed the same split (A 58.0 s with two deferrals, B 11.7 s). Slow launch, B5 ( Fast launch, B2 ( The 363 s launch, A2 ( Notes on the method: each kill was issued 38–127 s after the previous restore (A3 1 s after, B3 325 s after), and in every deferred launch the last keychain poll line was 0.1–1.2 s before the kill. In every deferred launch the WALLET backup fails once with |
Device gate (partial 2) — b633684 (Paykit rc69)Payment request round trip, both directions, UTC 2026-10-07. No
Both payments completed and both sides show the activity. Payer log for the second one ( Seen in passing on the payer after the first payment: |
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed 291328d..480b8d2: the rc69 bump (b633684) and a test timeout (480b8d2). One MEDIUM, inline: rc69 makes the slow launch slower.
rc69 otherwise: the session restores from rc68-written state on both simulators. No failed validation, recovery_required or concurrent_update line in any of the 14 logs; every lock error is shared_state_busy, which the app already handles wherever it handles ConcurrentUpdate (PubkyProfileManager.swift:1820, PaykitPaymentRequestService.swift:1787).
Payments: a request and payment in each direction completed, swipe → broadcast 18.6 s and 19.2 s, no lock errors. Contacts: delete from the Edit screen returned to Contacts and the re-add reached Contact Saved in under 5 s. Numbers and log lines: #856 (comment).
480b8d2 only raises a test wait from 2 s to 10 s (ContactPaymentsServiceTests.swift:514).
Device gate: b633684 — relaunch 10.7–11.7 s when not deferred, 97–114 s when deferred (8 of 12, one at 363 s); payments 18.6 s and 19.2 s; contact delete and re-add pass; no crash. 480b8d2 is test-only and was not driven. Hardware paths are code-only as before.
| requirement = { | ||
| kind = exactVersion; | ||
| version = "0.1.0-rc56"; | ||
| version = "0.1.0-rc69"; |
There was a problem hiding this comment.
[MEDIUM] With rc69 a deferred session restore takes 97–114 s, against 60–68 s on rc68
When the first restore attempt after a launch hits the shared-state lock, rc69 returns shared_state_busy within seconds. The app is deferred three times in the first ~50 s, then logs no restore activity for about 49 s, and the attempt after that gap succeeds in ~10 s. At d99d5a6 and 291328d (rc68, same simulators) the deferred launches had one deferral and restored in 60–68 s.
Two simulators on staging at b633684, terminate then launch, 12 relaunches: 8 deferred at 96.6–114.3 s, one of them 363 s; 4 not deferred at 10.7–11.7 s. Full table and the fast/slow log excerpts: #856 (comment).
Slow launch, B5 (B_bitkit_foreground_2026-10-07_13-51-23.log, UTC):
13:51:23.220 PERF: init(walletIndex:) took 0.0 seconds on core queue
13:51:31.161 WARN: Deferred session restoration, keeping saved session - PubkyProfileManager [resolveSessionInitialization line: 1786]
13:51:43.331 WARN: Failed to refresh public paykit endpoints after receive refresh: SharedStateBusy(code: "shared_state_busy", context: "Pubky shared state remains locked; retry later") - WalletViewModel [refreshBip21 line: 1496]
13:51:49.840 WARN: Deferred session restoration, keeping saved session - PubkyProfileManager
13:51:59.428 WARN: Failed to refresh public Paykit endpoints on foreground: SharedStateBusy(...) - WalletViewModel [line: 1358]
13:52:02.691 ERROR: Backup failed for: 'WALLET': SharedStateBusy(...) - BackupService [triggerBackup line: 262]
13:52:18.887 WARN: Deferred session restoration, keeping saved session - PubkyProfileManager
-- 48.9 s without a paykit line, no WARN/ERROR --
13:53:07.824 DEBUG: paykit_session loaded from keychain
13:53:17.530 INFO: Paykit session restored for pubkydso8zn5... - PubkyProfileManager [line: 322]
Fast launch, B2: first line 13:41:43.678, Paykit session restored 13:41:54.607, no deferral.
The 363 s launch (A2) is the same pattern with a longer gap: two deferrals, a third attempt started at 13:41:58, then 304.8 s with no paykit or pubky line and no Deferred or restored result. During it Contacts opened as a screen titled "Profile" with only a spinner. It restored by itself at 13:47:11.099. Seen once in 12.
Fix: retry a deferred restoration on a short bounded backoff (a few seconds) instead of waiting for the next trigger. The launch after install restored at 58 s with two deferrals, so the lock was probably free well before the three-deferral launches tried again; the app log does not show when it was released.
Android shows the same at 7bc3ccb1a (64–115 s, three deferrals in the slowest): synonymdev/bitkit-android#1401 (review). What holds the lock at launch remains the open question in #868.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: follow-up at 480b8d25, covering the entire delta since d99d5a67, affected callers and tests, the rc68-to-rc69 SDK delta, and the complete current PR inventory. Supported unchanged coverage is inherited from the completed baseline. Ancestry, base and merge base were verified locally; HEAD was rechecked before returning.
No new actionable code findings.
The Contact Saved deletion concern is fixed in source, including stale Back history and newer-route preservation. Hardware failures now retain signed transactions and pending proofs unless local decoding definitively failed before any submission. Earlier uncertain attempts remain protected.
Validation: changed Swift files passed syntax parsing; a standalone harness using the exact pinned navigation helper and key normalizer passed 10 route assertions. Native XCTest assertions were inspected, not executed. Code-result and publication-preview validation passed. Pinned-head unit and integration CI were still running when collected. Dependency inspection used Paykit dd97fc9a and Core b53fa54a. Targeted comparison at Android 804e7547 retains older hardware error and expiry behavior; this does not establish current Android parity or a full Android review.
Startup, linking and backup delays remain unresolved; attributed rc69 relaunch measurements do not establish improved latency. The author's hardware lost-response recovery pass applies to b633684 and that single scenario. View reattachment, process death and other unchecked fault cases remain unverified.
Device testing: not performed in this review. Remaining author journeys and fault-injection cases remain required runtime work. Concurrent wallet/shared-Pubky installations and pre-2.6.0 profile compatibility remain excluded under the supplied product rules.




































Closes #815
This PR moves Bitkit to Paykit's identity-wide shared state using the published
0.1.0-rc69SDK.SDK: https://github.com/pubky/paykit-rs/releases/tag/v0.1.0-rc69
Companions: Android #1401, Paykit Server #46.
Description
Out of Scope
Design
N/A — no design available.
Preview
QA Notes
Journeys
J1 updated
delete-profile.xml- bulk deletion with 62 contacts; active-preparation runs completed on both platforms, with a settled/idle repeat still pending.J2 updated
import-all-contacts.xml- Continue completes public setup without waiting for every contact to link; retest with 61 contacts and unavailable profiles.J20 Repeat
import-all-contacts.xmlwith the same 62-contact identity after profile deletion. Three imports and two overlapping deletions completed on rc68; fully idle deletion and private-link readiness remain open.J3 new
cancellation-during-confirmation.xml- a subscription canceled while confirmation is open cannot be paid after its cancellation is received.J4 new
fixed-onchain-destination.xml- later unpaid subscription periods can use the request's fixed on-chain address, while paid periods remain unavailable.J5 new
contact-payment-sharing.xml- disabling contact payments stays off after leaving and returning to Settings.J6 updated
automatic-presentation.xml- linked contacts on separate identities show new requests in the payment sheet, keep payment disabled during preparation, and defer presentation while another sheet is open.J7 new
accepted-device-ownership.xml- only the accepting install can resume a one-time request after restart.J8 new
paykit-only-approval.xml- approves Paykit access without creating a watch-only account.J9 new
paykit-reconnect.xml- renews server access without replacing its account or invoices.J10 updated
contact-request-or-pay.xml- contact payments and requests use identity-wide state.J11 updated
delete-and-readd-contact.xml- deletion blocks private requests and refreshes the list without waiting for another poll.J12 updated
definite-pre-broadcast-retry.xml- a failed request can retry immediately using fresh state.J13 updated
issuer-interoperability.xml- requests from another app retain their exact endpoint and request context.J14 updated
request-summary.xml- request details show the shared request and endpoint correctly.J15 updated
open-watch-only-link.xml- the OS handoff opens the requested authorization flow.J16 updated
wallet-leg.xml- authorizes the server, pays its exact request destination, attributes the received payment, and preserves manual detachment after sync and restart.J17 updated
create-and-propose.xml- oversized proposals are rejected before sending, and shorter proposals use the shared identity and app ownership.J18 updated
requested-resolution-failure.xml- progress is visible while preparing and clears after failure.J19 new
absolute-payment-deadline.xml- accepted requests remain payable until their payment deadline; expiry during callbacks, fee/PIN entry, signing or queue waits prevents submission, while earlier uncertain broadcasts and late proofs remain recoverable. The lost-response/expiry/reconciliation case passed on b633684/rc69 in the normal app with a Trezor emulator and local regtest. The remaining deadline and reattachment cases are still unverified.J21 Updated
payment-deadline-history.xml- expired one-time and unsupported recurring deadlines remain visible without enabling payment.J22 new delete-newly-saved-contact.xml - deletion from Contact Saved returns to Contacts; Back does not reopen the deleted contact. Verified by the reviewer in three staging deletion/re-add runs on 291328d; see the device report.
Manual Tests
1 With network fault injection, overlap foreground/connectivity recovery requests while restoration fails, then recover and retry. Waiting callers must share the active attempt; a later attempt must remain available.
2 Tap a due subscription reminder during background contact preparation, both with Bitkit open and on cold launch. Follow the reminder checks and record tap-to-sheet timing separately from authentication and SDK lock waits.
3 Hold sharing withdrawal in progress, foreground the app, then request sharing on again. Cleanup must not overlap, and publication must wait for it to finish. Repeat with foreground cleanup already active, and with Paykit UI disabled/re-enabled before Contact Payments is enabled; this requires fault injection.
4 Drop the acceptance response after its durable commit, restart Bitkit, then refresh and retry the accepted one-time request. The accepting installation must retain ownership, and retry must not duplicate a payment. This requires fault injection.
5 Inject private withdrawal and public/app-registry update failures, then disable contact payments. Both sharing settings must stay off and cleanup must remain pending until recovery, without re-sharing cleared endpoints. Repeat with only the public/app update failing, and with a recipient removed by another authorized app while Bitkit has no local contact cache, including Linking and RecoveryRequired recipients.
6 Back up an accepted but unpaid one-time request, stop the original wallet, then restore on a replacement install and retry. Automated wallet backup/restore is not a journey capability. Running the same wallet on multiple devices concurrently is unsupported.
7 Hardware broadcast recovery: follow the manual fault-injection checklist. Drop a successful broadcast response, let the deadline expire, and verify reconciliation completes without rebroadcasting, restores navigation, and survives Activity/view recreation. Lost-response/expiry/reconciliation passed on b633684/rc69 with no extra broadcast after expiry, normal completion/navigation and one matching proof. View reattachment, process death and other failure combinations remain unverified; these require controlled fault injection and are not automated journey capabilities.
Automated Checks
PaykitReceivedPaymentContactsTests.swift- combined attribution, cache invalidation, and a database-backed test of backfill retry, saved contact attribution, and skipped completed scans.AddressSearchCoordinatorTests.swift- companion-account lookup, isolated search indexes, and conservative handling of unknown outputs.PaykitSdkClientConfigTests.swiftandPubkyProfileManagerTests.swift- shared identity setup, cached key reuse, rotation and rollback rejection, and identity switching.PaykitBackupStateTrackingTests.swift- cached backup fingerprints and rechecking uncertain writes.PubkyAuthRequestTests.swift,PubkyAuthApprovalSheetTests.swift, andWatchOnlyAccountServiceTests.swift- independent claims, combined consent, and malformed request rejection.PrivatePaykitServiceTests.swift,PaykitContactLifecycleTests.swift, andContactPaymentsServiceTests.swift- publication ordering, deferred work, contact cleanup, and attribution.PaykitPaymentRequestServiceTests.swift,PaykitPaymentProofServiceTests.swift, andPaykitPaymentStateBackupTests.swift- request destinations, execution ownership, fresh snapshots after state changes, and retained wallet payment state.PaykitPaymentActivityTests.swiftand updatedPaykitSdkOperationLockTests.swift- deferred proof refresh and delivery, payment-priority barriers, cancellation, and backup admission across wallet changes.PaykitReceiverNoiseKeyStoreTests.swift- keys belong to the identity, not individual receivers; authorizer coverage is inPaykitSdkClientConfigTests.swift.Local validation against published rc69: the simulator app/test build and 610 focused native tests passed, covering hardware send coordination, request execution, proof reconciliation and contact navigation. SwiftPM uses the published release without a local package override. Complete compiler, formatter and translation reports have no introduced diagnostics against the base; the 18 existing format findings remain unchanged. These tests do not replace the unchecked device journeys or establish payment-flow latency.
Performance is not signed off. Paired mobile measurements on the rc66/rc67 runtime recorded Send Request at 13.68s, confirm/swipe to native send at 15.56s, first-returned LINKED at about 80s, and private sharing withdrawal at 30.63s (not full cleanup). These single samples predate the final queue fix and do not establish current-head latency. Cold start, backup stalls, sharing cleanup, reminder failure recovery and full content unlock remain open in #868 and the unchecked journeys above.
The reminder cold-launch network-failure journey is not yet device-tested.
The rc68 62-contact staging retest completed three imports and two deletions of the same identity, without reproducing the previous contact-save/re-import stall. Preview took 15.89-16.44s, Import All 1.358-1.830s and Continue 8.993-9.530s. Deletion reached onboarding in 17.068s and 15.324s. These are individual UI-polled observations. Preparation retries remained active, so fully idle deletion and private-link readiness are unverified; no payment was attempted.