feat: share paykit state across apps - #856
ben-kaufman wants to merge 41 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.
ovitrif
left a comment
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 6 files.
No new findings; the rest is in the review.
synonymdev/bitkit-android#1401 at 10a3a647 matches. Both receive linked-peer messages on every inbox poll, and both log queue, delivery, and pending private-endpoint withdrawal failures while keeping a failed contact pending.
Note
Retest Suggested J8, J9, J10, J11
@ovi-reviewer retest J8,J9,J10,J11
QA:
Tests wait for CI.
Reviewed by grok-4.7-xhigh via gh-pr-review-loop skill
ovitrif
left a comment
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 14 files.
New findings: 1 inline (1 MEDIUM); the rest is in the review.
synonymdev/bitkit-android#1401 at eaf6a092b matches deferred contact preparation, established-link reuse, unavailable-link cooldowns and elapsed 30/60-second maintenance with shared-state-only inbox polls. Both add the same Continue action to contact import.
Note
Retest Suggested J1, 2
@ovi-reviewer retest J1,2
QA:
Tests queued.
Reviewed by grok-4.7-xhigh via gh-pr-review-loop skill
| await prepareSavedContacts( | ||
| if !requireImmediatePublication { | ||
| let keys = rememberSavedContacts(publicKeys, replacing: true) | ||
| scheduleContactPreparation(keys, wallet: wallet) |
There was a problem hiding this comment.
MEDIUM: The new background preparation path has no eventual-publication assertion.
The deferred branch now returns after scheduling, but the new coalescing test replaces its operation with a closure that only records keys, and the publication tests call syncLocalEndpointPublication directly. Those tests would still pass if this scheduling call or the worker’s publication call disappeared, leaving imported contacts without private endpoints. Could we add a test that drives deferred prepareSavedContacts with publication available and waits for the worker to deliver the expected reservation updates?
There was a problem hiding this comment.
Added in 9a52ba0. The test calls deferred prepareSavedContacts, lets the real background worker run, and asserts the exact reservation update passed to the SDK. It also checks that waiting for preparation does not complete before publication.
jvsena42
left a comment
There was a problem hiding this comment.
Review at the newest head. The lock-saturation thread stays open; the recount for these heads is on that thread and a staging rerun is in progress.
New LOWs, inline:
- the 5-min Transport cooldown also stalls a handshake that is in progress
- a deleted and re-added contact keeps that cooldown (iOS only; Android clears it)
jvsena42
left a comment
There was a problem hiding this comment.
Device run at the newest heads found a HIGH regression, inline: on a fresh app start, a linked contact never becomes a payment-request target, so Contact Pay never offers Request. The deferred contact preparation now races target discovery for the SDK lock. The lock-load thread stays open; this head's numbers are on it.
jvsena42
left a comment
There was a problem hiding this comment.
LOW: a cancelled contacts load shows the user an error toast. This is pre-existing, but this PR makes it common. ContactsListView.swift isn't in the diff, so I'm putting it here.
Seen on staging at 27e921a, 21:52:03 UTC: ERROR Failed to load contacts: CancellationError() (ContactsManager.swift:285), then Failed to load contacts in view: CancellationError(). An error toast was shown.
Why it happens:
ContactsListView.loadContacts()(:289-305) catches every error and, when contacts are already cached, toastscontacts__error_loadingwitherror.localizedDescription.ContactsManager.loadContactslogs and rethrowsCancellationErroras an ERROR.- So leaving or re-rendering Contacts while a load is in flight cancels the
.task, and the user gets an error. - master has the same handling. But with this PR a contact load waits 45–90 s on the shared-state lock, so navigating away mid-load is now common.
Fix: add catch is CancellationError { return } before the generic catch in both places, as refreshContactLink already does.
jvsena42
left a comment
There was a problem hiding this comment.
One new LOW (iOS only), inline. The fixes for the earlier threads are verified and replied to; the resolved ones are closed. The eligibility HIGH and the lock-load thread stay open until the staging rerun.
jvsena42
left a comment
There was a problem hiding this comment.
Approving at c334a6f. No HIGH or MEDIUM remains.
Device gate at c334a6f (staging):
- Request or Pay is offered within ~4 min of a cold launch, and in 4.1 s once warm.
- Contact payments off: 6 min 45 s; on: 53 s. No Policy error and no backup failure.
- Send Request reaches "Sent" in 80 s.
- Session load is flat at ~48/min.
The remaining latency comes from the rc59 SDK. Pinning paykit-rs#171 and re-measuring is tracked in #868, with a journey to reproduce it.
Open LOW: the inline updateSavedPublicKeys comparison of ordered arrays (PaykitPaymentRequestService.swift:1300). I will re-review once it is fixed.
|
rc60 is published and pinned here. The new staging run confirmed cross-platform request delivery and completed sharing withdrawal, but not acceptable latency yet: warm Request or Pay is under one second, while Send Request exceeded 26.7 seconds and withdrawal exceeded 30 seconds. Full results and remaining targets are in #868: #868 (comment). Keeping that issue open; this update does not claim the 61-contact or full-payment journey is resolved. |
|
Updated to published rc62 in 134dce1. It includes the SDK fix for outbound batches exhausting peer leases while waiting on shared storage; the earlier batching and lightweight polling changes remain. All 468 focused iOS tests passed against the remote release, the framework checksum matches, and the staging build passed. No new formatting findings. This is not a staging performance sign-off. The latest rc61 run sent in 23-26 seconds but took 4.9-6.4 minutes to appear in Android’s open request list. rc62 fixes a separate multi-peer failure; the one-peer delay remains open, with results in #868: #868 (comment). |
|
Checked CI on 134dce1. Unit tests passed. The integration run failed 9 of 34 tests on all three attempts while calling staging Blocktank: regtest deposits return HTTP 500, fee estimation fails, and order creation reports The local E2E run passed 12 scenarios. Hardware-wallet transfer to spending failed on all three attempts because No code change or timeout increase for these failures. Integration needs a healthy staging Blocktank run, and the hardware-transfer failure still needs its cause isolated. Paykit performance remains tracked separately in #868. |
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 134dce1 (Paykit rc62). No HIGH or MEDIUM in scope.
Code:
- The saved-keys LOW is fixed and resolved.
- The rc59 → rc62 adoption is clean. The refresh modes (stored, inbox, full) are correct, including the upgrade-while-waiting path. Third-party approvals still validate the normal session scope, so the new authorizer scope cannot leak to a delegated app. No fund-path change on the app side.
Device gate: 134dce1 — two iOS simulators on staging, fresh identities:
- First link in ~5 min, with no lease-expired lines.
- Send Request both ways: 33–100 s to "Sent"; presented on the receiver after 2 min 43 s and 4 min 48 s.
- One real payment: paid and received, ~70 s from swipe to success.
- Contact payments off: ~110 s; on: 29–47 s, with no errors.
- Request or Pay after a cold launch: 3 min 21 s.
- No crash or freeze.
- Android twin: not run. Its build can't download paykit-android rc62 here (a package-auth problem on my machine).
Everything is faster than on rc59 and nothing fails. The remaining latency against the targets is tracked in #868, which I updated with these numbers.
Non-blocking question: after turning contact payments off and on and then relaunching, the pending incoming request left the bell, and the activity row changed from "Received from " to plain "Received". Is dropping the pending request and the contact attribution when sharing is turned off intended?




































Closes #815
This PR moves Bitkit to Paykit's identity-wide shared state using the published
0.1.0-rc62SDK.SDK: https://github.com/pubky/paykit-rs/releases/tag/v0.1.0-rc62
Companions: Android #1401, Paykit Server #33.
Description
Uses encrypted homeserver state and one Encrypted Link per contact identity, shared by authorized Paykit apps.
Stores wallet reservations, pending payment proofs, and recovery backups locally while following shared request state and execution claims.
Retains broadcast transaction IDs before remote identity reads, defers publication for unavailable contact links, and preserves manually detached activity contacts through sync.
Saves one-time acceptance intent before the remote call and includes it in wallet backups, so interrupted acceptance and wallet restore can resume safely, including shared-state lock conflicts. Execution rechecks subscription state after asynchronous lookups, and acceptance IDs are removed only when shared state confirms payment, cancellation, or rejection.
Lets Bitkit authorize Paykit access, a watch-only account, or both as independently requested claims, without sharing wallet spending keys.
Shows the requested access in the authorization sheet. Server reconnect requests Paykit access without allocating another watch-only account.
Pays the exact endpoint supplied by a Payment Request instead of substituting a later private list, permits a fixed on-chain destination for unpaid recurring periods, and attributes received activity through its matching receiving address, including companion accounts.
Combines reservation and request attribution, leaves ambiguous transactions unlabeled, and skips unchanged history backfills.
Handles contact publication, requests, and subscriptions using app-owned endpoints inside the shared identity.
Saves contact-sharing OFF and cleanup-pending before withdrawal, preventing new endpoint publication during cleanup. Serializes private/public cleanup with sharing changes and coalesces foreground retries. Retries failed withdrawals and registry updates, and discovers established publication recipients from shared state without adding unrelated unfinished links.
Includes app ownership fields when validating subscription proposals against the transport size limit. One-time and subscription proposals revalidate and deliver only to the selected saved recipient, without draining unrelated peers.
Reuses validated Paykit keys and backup fingerprints for unchanged state, and refreshes keys after identity errors without replaying failed writes.
Checks incoming private messages during the ten-second foreground poll without draining outbound work. Full synchronization runs on startup, explicit refreshes, and elapsed-time maintenance after 30 seconds, then every 60 seconds. Notification handling can reload saved requests without network intake. Temporary session-restoration transport and lock failures retain the saved session for retry instead of starting another auth flow.
Completes public payment setup before preparing private contacts in the background. Coalesces repeated preparation, reuses established links, backs off unavailable lookups, and invalidates pending publication during cleanup. Unchanged contact keys do not trigger preparation when SwiftUI rebuilds the view.
Reads public request capabilities up to eight contacts at a time outside the SDK mutation queue, without waiting for all-contact preparation. Private-message retries drain only the affected peers; idle cleanup skips unrelated work.
Out of Scope
Design
N/A — no design available.
Preview
QA Notes
Journeys
updated
import-all-contacts.xml- Continue completes public setup without waiting for every contact to link; retest with 61 contacts and unavailable profiles.new
cancellation-during-confirmation.xml- a subscription canceled while confirmation is open cannot be paid after its cancellation is received.new
fixed-onchain-destination.xml- later unpaid subscription periods can use the request's fixed on-chain address, while paid periods remain unavailable.new
contact-payment-sharing.xml- disabling contact payments stays off after leaving and returning to Settings.updated
automatic-presentation.xml- linked contacts on separate identities automatically present new requests and defer them while another sheet is open.new
accepted-device-ownership.xml- only the accepting install can resume a one-time request after restart.new
paykit-only-approval.xml- approves Paykit access without creating a watch-only account.new
paykit-reconnect.xml- renews server access without replacing its account or invoices.updated
contact-request-or-pay.xml- contact payments and requests use identity-wide state.updated
delete-and-readd-contact.xml- deletion blocks private requests until the contact is explicitly re-added.updated
issuer-interoperability.xml- requests from another app retain their exact endpoint and request context.updated
request-summary.xml- request details show the shared request and endpoint correctly.updated
open-watch-only-link.xml- the OS handoff opens the requested authorization flow.updated
wallet-leg.xml- authorizes the server, pays its exact request destination, attributes the received payment, and preserves manual detachment after sync and restart.updated
create-and-propose.xml- oversized proposals are rejected before sending, and shorter proposals use the shared identity and app ownership.Manual Tests
Automated Checks
PaykitContactKeysObserverTests.swift- hosted SwiftUI checks for stable contact keys, membership changes, and callback-driven view updates.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, and retained wallet payment state.PaykitReceiverNoiseKeyStoreTests.swift- keys belong to the identity, not individual receivers; authorizer coverage is inPaykitSdkClientConfigTests.swift.468 focused simulator tests passed against the published rc62 release. Coverage includes polling modes and overlapping refreshes, identity/session recovery, sharing serialization, bounded public discovery, cancellation, wallet-wipe isolation, scoped retries and the hosted contact observer. Changed-file formatting is clean; the full formatter's ten findings exactly match the base. The full local suite remains blocked by funding timeouts in
AddressTypeIntegrationTests.swift.Staging performance remains open. The last published rc61 one-contact run sent a request in 23-26 seconds, but Android's open request list showed it only after 4.9-6.4 minutes. Android's private withdrawal phase took about 49 seconds in an earlier rc61 run. rc62 fixes the separately reproduced multi-peer lease failure; it does not establish the remaining latency targets. These unfunded-wallet runs do not verify payment execution, complete Marketplace unlock, or the 61-contact, cold-start and backup-stall scenarios. The separate broadcast-outcome dependencies remain iOS #844 and Android #1384. The unchecked journeys above still need device QA.