Skip to content

feat: share paykit state across apps - #856

Open
ben-kaufman wants to merge 72 commits into
masterfrom
codex/paykit-shared-runtime-local-20260930
Open

ben-kaufman wants to merge 72 commits into
masterfrom
codex/paykit-shared-runtime-local-20260930

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Closes #815

This PR moves Bitkit to Paykit's identity-wide shared state using the published 0.1.0-rc65 SDK.

SDK: https://github.com/pubky/paykit-rs/releases/tag/v0.1.0-rc65

Companions: Android #1401, Paykit Server #46.

Description

  • Deletes contacts with one bulk block/remove operation, pauses background contact preparation during profile deletion, and batches local cleanup. Active subscriptions, busy peer leases and public contact markers remain protected.

  • Explicit contact re-add and import save contacts and unblock their selected peers atomically, without per-contact writes or compensating re-block loops. Label edits do not unblock peers.

  • 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 shared-state recipients even while links are recovering, and keeps unfinished withdrawals pending.

  • 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.

  • Shows incoming requests in the existing payment sheet while preparing, with payment disabled until validation finishes. Closing the sheet prevents a late result from reopening it; failed payments remain retryable.

  • Prioritizes selected-recipient and request-delivery work over queued background reads while preserving active SDK calls and identity-change barriers.

  • Retains due-reminder targets through failed, canceled or stale refreshes. Manual Pay and payment retries take priority without dropping an unrelated reminder or interrupting an active payment.

  • Normalizes uppercase Bech32 request addresses for attribution while preserving validation and ambiguity checks.

  • Runs Dev Settings Paykit-disable cleanup through the same coordinator as contact-sharing changes.

Out of Scope

  • Guaranteed private-list withdrawal on contact deletion. Deletion blocks immediately even if withdrawal fails. The old list can remain at the peer, and registry cleanup can remain pending until the contact is explicitly re-added.
  • Migration from receiver-folder data. Paykit has not launched, so that development data is unsupported.
  • Homeserver lock-finalization safety: the SDK cooldown is a mitigation, not a fix for a write completing after lock expiry.

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.

  • Repeat import-all-contacts.xml with the same 62-contact identity after profile deletion. The reported re-import stall needs a staging retest on rc65; no network deadline or safety wait was shortened.

  • 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.

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 Force a lock conflict after acceptance commits but before its response read, restart Bitkit, then refresh and retry the accepted one-time request. Lock fault injection is not a journey capability.

  • 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.

Automated Checks

  • added PaykitReceivedPaymentContactsTests.swift - combined attribution, cache invalidation, and a database-backed test of backfill retry, saved contact attribution, and skipped completed scans.
  • added AddressSearchCoordinatorTests.swift - companion-account lookup, isolated search indexes, and conservative handling of unknown outputs.
  • updated PaykitSdkClientConfigTests.swift and PubkyProfileManagerTests.swift - shared identity setup, cached key reuse, rotation and rollback rejection, and identity switching.
  • updated PaykitBackupStateTrackingTests.swift - cached backup fingerprints and rechecking uncertain writes.
  • updated PubkyAuthRequestTests.swift, PubkyAuthApprovalSheetTests.swift, and WatchOnlyAccountServiceTests.swift - independent claims, combined consent, and malformed request rejection.
  • updated PrivatePaykitServiceTests.swift, PaykitContactLifecycleTests.swift, and ContactPaymentsServiceTests.swift - publication ordering, deferred work, contact cleanup, and attribution.
  • updated PaykitPaymentRequestServiceTests.swift, PaykitPaymentProofServiceTests.swift, and PaykitPaymentStateBackupTests.swift - request destinations, execution ownership, fresh snapshots after state changes, and retained wallet payment state.
  • removed PaykitReceiverNoiseKeyStoreTests.swift - keys belong to the identity, not individual receivers; authorizer coverage is in PaykitSdkClientConfigTests.swift.
  • ran the iOS/Android/server regtest flow on published rc58: a 17,000-sat payment used the request's exact address, the server confirmed it, and iOS showed Received from Buyer. All 37 simultaneous-sync readiness samples stayed healthy.
  • verified SwiftPM resolves rc65 from the signed release tag and its downloaded framework archive matches the manifest checksum.

Local validation against published rc65: production build and 277 focused tests passed, covering contact restoration, identity changes, queued/active cancellation, SDK locking, backup tracking, contacts, profiles and private Paykit handling. No local package override or introduced compiler/lint diagnostics. The full formatter has 15 verified pre-existing findings in six untouched files; translation validation has no errors or new warnings. SwiftPM resolves the signed rc65 release and verified framework checksum.

Performance is not signed off. Latest mobile measurements used the rc63 SDK source with measurement instrumentation: incoming preparation took 3.745s, sending 15.272s, Contact Pay 6.235-8.601s, and full sharing withdrawal 64.542s including 20.317s of queue wait. Fresh linking and request-detection delays are reported separately there. These are individual runs, not matched controls or full payment completion. Cold start, 61-contact import, 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.

Profile deletion was measured with 62 unregistered contacts on fresh staging identities while preparation was active: iOS 34.3s total (4.2s contact cleanup), Android 48.5s (5.4s contact cleanup). Both returned to the signed-out screen. These are individual runs, not a matched retest of the reported identity; uncertain-write safety waits remain unchanged. The remaining withdrawal and sign-out time is not yet fully attributed.

@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 0/5

[High risk] Updates payment SDK and refactors payment state sharing across apps.

The PR should not merge until received-payment attribution and compatibility with existing persisted payment state are addressed.

Findings

  1. P1 Security Unrelated payments gain payer labels ▶
  2. P1 Older wallet backups cannot restore ▶
  3. P1 Existing pending proofs become unreadable ▶
  4. P1 Legacy reservation keys lose contacts ▶

Summary

This PR moves Paykit integration from receiver-specific local state to identity-wide shared state, adds independent authorization claims, and uses shared requests for payment and received-activity attribution.

  • The new attribution path can assign an unrelated historical receipt to a request counterparty.
  • Existing wallet backups, local pending proofs, and reservation ledgers need compatibility handling for their changed persisted formats.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Shared Paykit requests] --> B[Request endpoint resolution]
  B --> C[Payment and proof]
  A --> D[Endpoint-to-contact index]
  D --> E[Historical received activity backfill]
  F[Local proof and reservation state] --> C
  G[Wallet backup] --> F
Loading

Reviews (1) · Last reviewed commit: "docs: clarify paykit integration contrac..."

Comment thread Bitkit/Services/PaykitReceivedPaymentContacts.swift Outdated
Comment thread Bitkit/Models/PaykitPaymentStateBackup.swift
Comment thread Bitkit/Services/PaykitPaymentProofService.swift
Comment thread Bitkit/Services/PrivatePaykitAddressReservationStore.swift

@ovi-reviewer ovi-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment thread Bitkit/Services/CoreService.swift Outdated
Comment thread journeys/pubky-marketplace/wallet-leg.xml Outdated
Comment thread Bitkit/Services/PrivatePaykitService+Backup.swift

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread journeys/pubky-marketplace/wallet-leg.xml Outdated

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, with rawUrl re-checked in approveAuthRequest. The requester clientID and relayOrigin are displayed. A Paykit-only claim skips watch-only account allocation, while a combined claim still goes through the watch-only consent step. PubkyAuthClaim.encode refuses 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 to acceptedPaymentEndpointIdentifiers, and the post-broadcast lookup reuses the captured contactPaymentContext.
  • No app-group or keychain-access-group changes, and Env.keychainGroup stays private.
  • Biometric and PIN checks run in submitPayment before performPayment takes the execution claim, so declining auth leaves no claim.
  • Not raised, because nothing reaches them today: claims are never released on abandon (no releasePaymentRequestExecutionClaim call 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.

Comment thread Bitkit/Services/PaykitPaymentRequestService.swift
Comment thread Bitkit/AppScene.swift Outdated
Comment thread Bitkit/Utilities/Keychain.swift

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 J4
Test J4

Passed.

J4-retry-104009.mp4
J4-retry-104009-buyer.mp4

🔴 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)

@ben-kaufman

Copy link
Copy Markdown
Contributor 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 N/A — no design available.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ovi-reviewer[bot]

This comment was marked as resolved.

@ben-kaufman
ben-kaufman requested a review from piotr-iohk October 1, 2026 12:33

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. ensurePaymentAllowed requires isApprovedForPayment and re-checks generation, identity and approval after the async linkedPeers call. A stale proposal on the second install fails at the SDK accept, which re-validates Proposed inside the locked transaction.
  • Backfill is skipped while the identity, contact snapshot, activity revision and reservation revision are unchanged. Every ActivityService write 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.xml matches 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.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 1, 2026 13:05
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-shared-runtime-local-20260930 branch 2 times, most recently from 23d6ddb to fe281dd Compare October 1, 2026 13:52

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ben-kaufman please resolve the merge conflicts. QA review has not been performed for this request.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Updated to published Paykit rc63 in 591ac12. The downloaded framework and checksum are verified; build and 250 focused simulator tests passed with no introduced compiler, format, or translation diagnostics. Server compatibility is in pubky/paykit-server#46. Latest measurements are linked in the PR body; cold-start, slow-flow, and remaining device checks stay open. The previous device report used rc62, not this updated release.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Fixed the matching reminder issue in 90799f2: a retained subscription reminder no longer blocks manual Pay or retry for another request, and cannot take over its preparation or confirmation. The reminder is preserved for later. Also corrected the import QA instructions to describe atomic batch saves. Build and 241 focused tests passed against published rc63, with no introduced format, compiler or translation diagnostics. The pending device/performance checks remain open.

@jvsena42

jvsena42 commented Oct 6, 2026

Copy link
Copy Markdown
Member

Device gate (partial) at 591ac12 (rc63), two simulators on staging. Times UTC, 2026-10-06.

Upgrade in place from the rc62 build: the identities did not restore. Neither simulator logged Paykit session restored in 6 min 20 s. Both repeated:

WARN Failed to import saved session, attempting re-sign-in: Storage(code: "storage_error", context: "SDK state blob failed validation") - PubkyProfileManager
WARN Re-sign-in failed, keeping saved session for retry: Storage(code: "storage_error", context: "SDK state blob failed validation")
ERROR Backup failed for: 'WALLET': Storage(code: "storage_error", context: "SDK state blob failed validation") - BackupService

The Profile screen stayed on a spinner, the bell was gone, and a bitkit://contact deeplink did nothing (Failed to load contacts before routing contact link). Same as on Android (details there); not raised as a finding since there is no public release and rc63 has no legacy migration. One side effect worth a look: the wallet backup fails with the same error for as long as this lasts.

Fresh wallets and identities on rc63:

Measure rc63 rc62
First link: second Save → Request and Pay offered ≤ 2 min 49 s, ≤ 3 min 3 s ~7 min
Send Request → "Sent" ≤ 15 s ≤ ~30 s, 57–173 s
Send tap → sheet opens on the receiver ~17.5 s 43–59 s
Automatic sheet: preparing → ready 3.3 s 57–60 s
Swipe → broadcast ~33 s ~54 s
Cold launch → session restored ~11–12 s on both, no Deferred session restoration 19.6–117 s
Cold launch → Request and Pay offered amount screen only at +35 s; offered at +2 min 21 s (not resolved finer) up to 4 min 53 s

Timestamps: Send tap 14:26:58; Showing sheet send on B 14:27:15.430; Opened private Paykit payment 14:27:18.760; swipe ~14:28:22; Successfully broadcast transaction f93ccb40… 14:28:55.431.

During first link, both sides logged recovery_required on an early Pay tap (A 14:23:58.930, B 14:23:41.574) before the link came up.

Still running: the reverse request and explicit bell Pay, sharing OFF/ON, and a second relaunch.

jvsena42
jvsena42 previously approved these changes Oct 6, 2026

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 6c86c83..90799f2 (the rc63 adoption in 591ac12 and 90799f2). No HIGH or MEDIUM, nothing inline.

Code:

  • 591ac12: Package.resolved pins 0.1.0-rc63 at the release commit. The new batch saveContacts runs under the ordered operation lock with requireSignedInIdentity in the same locked operation, and unblocks only peers in the batch. Backup tracking still invalidates once on failure or cancel. Client config is unchanged. ContactImportUITestFixture is inside #if DEBUG.
  • 90799f2: an explicit Pay is routed before the reminder target is consulted and clears only a target that matches it; the reminder is then handled once, and the consumption points from 6c86c83 are unchanged, so the earlier loop is not reintroduced. canHandleSubscriptionNotification is checked before and after the awaited refresh.

Device gate: two simulators on staging at 591ac12 (rc63), fresh wallets and identities. 90799f2 was not driven on a device. No crash, no shared_state_busy or requestUnavailable lines.

Measure rc63 rc62
First link: second Save → Request and Pay offered ≤ 2 min 49 s, ≤ 3 min 3 s ~7 min
Re-link after delete and re-add ≤ 2 min 59 s n/a
Send Request → "Sent" ≤ 13 s, ≤ 15 s, ≤ 17 s ≤ ~30 s, 57–173 s
Send tap → sheet opens on the receiver 17.5 s, 27 s 43–59 s
Automatic sheet: preparing → ready 3.3 s, 3.3 s, 15.5 s 57–60 s
Bell Pay tap → sheet / → ready 0.75 s / 15.6 s 0.74 s / 36.7 s
Swipe → broadcast ~33 s, ~36 s ~54 s
Sharing OFF / ON 26–46 s / ≤ 20 s 89–113 s / 28–34 s
Cold launch → session restored 11–12 s (three of four launches) 19.6–117 s
Launch right after sharing ON → restored 61 s on A (two concurrent_update), 11 s on B up to 6 min 13 s
Cold launch → Request and Pay offered between 35 s and 2 min 21 s; between 49 s and 1 min 44 s up to 4 min 53 s
Delete a linked contact ≤ 19 s n/a

Of the swipe → broadcast time, Consumed private Paykit payment list came 7–9 s after the swipe; the other ~27 s ran up to the LDK broadcast line.

Observations for #868 and for you to judge, none blocking:

  • Pay taps that end in nothing. Four times a contact Pay tap showed a loading button and returned to idle with no sheet and no log line: once during first link (14:24:36), twice shortly after a launch (14:39:02, 14:39:29), and on a contact whose sharing was off. No toast was caught.
  • Pay to a contact with sharing off. The chooser still offered Pay and Request; tapping Pay spun for 15–20 s and returned to the idle sheet with no message. Android shows "Unable To Pay Contact — The contact you're trying to send to hasn't enabled payments." in the same case.
  • First Pay after linking or launch skips the chooser and goes straight to the amount screen (14:30:18, 14:45:18); the next tap offers Pay and Request.
  • Repeating fee calculation. After a send sheet closed on a funding failure, A logged Calculated transaction fee: 112sats for sending to address bcrt1qee8r… twice every 10 s for at least a minute.
  • Upgrade from the rc62 build does not restore (details in the partial above); the app retries forever and shows no error.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: follow-up at 90799f2d, inspecting the entire delta since 591ac124, affected callers, payment ownership, reminder refresh/session guards, dismissal/retry paths, regression tests and contact-import instructions. Unchanged coverage is inherited from baseline e0269834-45bd-41bd-b994-f659162c0279. Ancestry, unchanged base and merge base, and final Git HEAD were verified locally.

No new actionable code findings.

Swift syntax parsing passed for both changed Swift files; native tests were inspected but not run locally. The pinned revision’s unit-test CI passed. Targeted source comparison with Android 117072b5 supports manual-payment priority and deferred-reminder recovery; this was not a full Android review.

The revised cold-launch reminder failure/manual retry case remains unexecuted on this head. Performance validation remains tracked in #868. Concurrent installations using one wallet or Pubky payment identity and pre-2.6.0 profile compatibility are outside supported coverage.

Device testing: not performed in this review.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

Some iOS runtime observations on 90799f2 are recorded in #861:

The deletion behavior looks like a possible performance regression from this observation, although a comparable baseline has not been measured and background preparation overlapped deletion. It seems reasonable to track this in a separate follow-up ticket rather than treat it as a critical blocker for this PR, given that deletion eventually completed. The successful import result is separate from this concern.

Copy link
Copy Markdown
Contributor Author

Fixed a matching restore issue in f09f1c5: callers waiting on an automatic recovery now share its result even when it fails, instead of immediately repeating the same attempt. A later call can still retry, and the first retry after failed startup is preserved. Identity-replacement and cancellation guards are unchanged.

All 103 PubkyProfileManagerTests pass, including the new overlapping-failure case. No introduced compiler or formatting findings; translation validation passes.

The Android counterpart also lets contacts load independently of the own-profile lookup and combines overlapping profile reads. iOS already avoids those two waits. No lock or uncertain-write timeout was shortened.

@ben-kaufman
ben-kaufman requested a review from piotr-iohk October 6, 2026 16:41
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Updated to published rc64 in 32fcc1d, including the handshake checkpoint batching and bulk profile deletion. The 62-contact staging runs completed in 34.3s on iOS and 48.5s on Android; contact cleanup itself took 4.2s / 5.4s. Breakdown and remaining limits. Safety waits are unchanged, and broader performance testing remains open.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

Retested the rc64 immediately-after-import, preparation-active condition on 32fcc1d, using fresh Stag6 with the exact original 62-contact fixture.

Import phases: 19.138s / 1.875s / 12.876s. Confirmed deletion 3.353s after the profile became available; logs confirm preparation was still active. Contact cleanup completed by 6.835s, Paykit session deletion by 14.609s, and the signed-out profile screen was detected by 15.932s from confirmation. No retry/restart was needed; Contacts were empty and wallet remained usable.

Full observations, build identity, timestamps and limitations on #861.

Clear improvement in this sample over the earlier ~13m33s iOS deletion. The settled-preparation variant and subscription/busy-lease retention cases remain untested; this is a narrow simulator retest, not full PR sign-off.

@piotr-iohk

piotr-iohk commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up settled-preparation deletion attempt on 32fcc1d is blocked by re-import after deletion.

Reusing Stag6 with the same 62 follows: Ringpreview took 15.475s, then ImportAll remained spinning for at least 10m33s. At+5m42s the log reported a contact-save/peer-block-restoration transport failure: write encrypted Pubky shared state could not be confirmed. No Continue or final UI outcome was observed by the end of the window; no deletion/retry/reset was performed.

Full repro, timestamps, error and source-path hypothesis on #861.

The fresh import/immediate-delete result remains successful. This reused-identity behavior should be distinguished from that result; settled deletion has not yet been measured.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: full reassessment of the complete PR diff at 32fcc1d5, against merge base a0889514. Covered changed production code and tests, affected callers, shared SDK/server contracts, authorization, payment and backup state, contact cleanup, request presentation, and scoped Android parity. No baseline coverage is inherited.

1 actionable finding — resolve or provide an evidence-backed rebuttal.

Validation: syntax parsing passed for all 78 changed Swift files, and isolated Swift execution checked batch-import rollback. Native tests were inspected but not run locally. The pinned head’s unit tests and E2E check passed in CI. Dependency contracts used Paykit rc64; functional source parity used Android fe364a5c, without claiming a full Android review or visual equivalence.

The same-head re-import stall still needs root-cause investigation; the isolated rollback check does not establish its duration or cause. Broader staging performance remains tracked in #868, and the author’s unchecked journeys/fault-injection cases remain unexecuted here. Concurrent Bitkit installations paying under one Pubky identity and pre-2.6.0 profile compatibility are outside supported coverage.

Device testing: not performed in this review.

Findings

  • [LOW] Limit rollback to peers whose unblock was attempted — inline at Bitkit/Services/PubkyService.swift:788.

Comment thread Bitkit/Services/PubkyService.swift Outdated
return try await sdk.saveContacts(updates: updates)
} catch {
let restorationError = error
for peer in blockedPeers {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Limit rollback to peers whose unblock was attempted

When re-importing a batch of previously blocked contacts, an early unblockPeer failure enters this loop for every originally blocked peer, including those the forward loop never reached. In rc64, blockPeer updates the peer timestamp inside a shared-state transaction even when it is already blocked. These unnecessary remote writes run while the import remains loading and the global SDK operation lock is held, delaying the original error and other queued SDK work. Track peers before attempting each unblock and restore only that set; include the failed attempt because its outcome can be uncertain. A failure after all unblocks still requires restoring all peers.

Evidence: source analysis at 32fcc1d5, plus isolated Swift execution of the unchanged production method with SDK boundary doubles. With 62 blocked peers and the first unblock failing, it made 1 unblock and 62 rollback calls, including 61 untouched peers; middle/last-unblock and final-save failures were also checked. No homeserver or device execution was performed. This verifies unnecessary rollback work, not the cause or duration of the reported staging stall.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f79cfcb using Paykit rc65. Explicit re-add/import now saves the contacts and unblocks the selected peers in one SDK transaction, so there is no per-peer forward loop or rollback loop. Rejected commits leave contacts and blocks unchanged; uncertain writes keep the existing safety handling. Label edits do not unblock peers. The remote-release build and 277 focused tests passed, including failure retry, identity changes and cancellation. The ten-minute staging observation still needs a repeat on this build; I am not treating the local tests as proof that it is resolved.

@ben-kaufman
ben-kaufman requested a review from piotr-iohk October 6, 2026 20:13
@ovi-reviewer

ovi-reviewer Bot commented Oct 6, 2026

Copy link
Copy Markdown

Interim test results for 32fcc1d

The run is still going; the final review follows when it ends. This is a progress note, nothing is approved or requested here.

  • Passed: J15, J1, J9, J17, 4, J5, 1, J3
  • Failed, J13: Automatic confirmation showed Connection Issues.
  • Failed, J12: Try Again crashes the LNURL payment review
  • Retrying on another seat: J11, 2
  • Retried because they failed on an earlier commit: J11, J12, 2
  • Carried from earlier results, passed there and not run again: J2, J4, J6, J7, J8, J14, J16, J10, 3, 5

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: 👍 Approve

Reaudit: diff 28 files.
No new findings; the rest is in the review.
The Android pair synonymdev/bitkit-android#1401 at 9083227 carries the same rc65 atomic contact operations, deletion pause, backup tracking, and reminder ownership checks.

QA:
Tests running.

Replies:

A payment-proof or subscription-clock change now waits for an in-flight request refresh and then reads again. (comment)

I verified the generation checks still discard stale request snapshots. This delta preserves those checks; device timing remains unverified.

Sharing off should withdraw payment details, not erase requests or payment history. (comment)

I found no request or history purge in the changed cleanup paths. I have no new device timestamps for that observation; latency and the remaining recovery checks stay open in #868.

All of this is latency already tracked in #868; nothing failed. (comment)

The reported payment completed. The rc63 and rc64 measurements come from later individual runs, so they do not establish rc65 performance or settle the re-import and idle-deletion checks.

Note

Retest Suggested J1, J2, J3, J4, J5, J6, J7, J8, J9, J10, J11, J12, J13, J14, J15, J16, J17, J18, 1, 2, 3, 4, 5, 6

@ovi-reviewer retest J1,J2,J3,J4,J5,J6,J7,J8,J9,J10,J11,J12,J13,J14,J15,J16,J17,J18,1,2,3,4,5,6

Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@jvsena42

jvsena42 commented Oct 6, 2026

Copy link
Copy Markdown
Member

Device gate (partial) at f79cfcb (rc65), two simulators on staging, installed over the rc63 build. Times UTC, 2026-10-06.

Upgrade rc63 → rc65 in place: both identities restored, in 12.1 s and ~12 s, with no storage_error, "failed validation", recovery_required, concurrent_update or Deferred session restoration lines. Profiles, balances, the contact and the pending request were all kept.

Measure rc65 rc63
Cold launch → session restored 12.1 s, ~12 s 11–12 s
Launch → Request and Pay offered A ≤ 1 min 14 s (first tap); B between 1 min 27 s and 2 min 23 s 35 s – 2 min 21 s
Send Request → "Sent" ≤ 14 s ≤ 13–17 s
Send tap → sheet opens on the receiver ~23 s 17.5–27 s
Automatic sheet: preparing → ready 3.5 s 3.3 s
Swipe → broadcast ~34–35 s (Consumed private Paykit payment list 8 s after the swipe) 33–36 s

Timestamps: Send tap 20:50:34; Showing sheet send on B 20:50:57.270; Opened private Paykit payment 20:51:00.737; swipe 20:51:34; Successfully broadcast transaction b94c0e3e… 20:52:08.082.

Same as rc63 on every measure, within sample spread. After the launch, B's first two contact Pay taps (+44 s, +1 min 27 s) went straight to the amount screen; the third offered Pay and Request.

Still running: sharing OFF/ON, delete and re-add a contact (the rc65 single-transaction path), and a second relaunch.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 90799f2..f79cfcb (3 commits; Paykit rc63 → rc65). No HIGH or MEDIUM; one LOW question inline.

Code, checked with no finding:

  • Package.resolved pins 0.1.0-rc65 at the tagged commit. rc64's changelog says persisted formats are unchanged from rc63; rc65 has no changelog entry at its tag, and its diff touches no storage format.
  • f79cfcb: only addContact and the import pass the unblock option (ContactsManager.swift:598, 640). saveContactLabel and refreshContactLink use the plain save, which needs an existing record and never unblocks. requireSignedInIdentity runs in the same tracked operation before the write, and the local list is updated only after the call returns.
  • 32fcc1d: deletion order is full private endpoint withdrawal, then bulk block and remove, then profile delete. A withdrawal failure keeps cleanupPending. Bulk removal blocks every removed peer in the same transaction.
  • f09f1c5: waiters await a Task<Void, Never>, so cancelling one waiter does not cancel the attempt; an identity replacement sends waiters back through the normal guards.
  • Test note: the partial-removal branch in ContactsManager.swift:872 (subscribed, leased or public-marker contacts retained) has no test, and testProfileDeletionDefersPreparationUntilDeletionEnds covers work that is dropped, not deferred.

Device gate: two simulators on staging at f79cfcb, installed over the rc63 build. The rc63 identities restored under rc65 with no validation error. No crash.

Measure rc65 rc63
Send Request → "Sent" ≤ 14 s, ≤ 14 s ≤ 13–17 s
Send tap → sheet opens on the receiver ~23 s, ~20 s 17.5–27 s
Automatic sheet: preparing → ready 3.5 s, 3.5 s 3.3 s
Swipe → broadcast ~34–35 s, ~42 s 33–36 s
Sharing OFF / ON 25–43 s / ≤ 21 s 26–46 s / ≤ 20 s
Pay to a contact whose sharing is off "Unable To Pay Contact" toast in ≤ 4 s spinner 15–20 s, no message
Delete a linked contact ≤ 17 s ≤ 19 s
Re-add → "Contact Saved" ≤ 6 s n/a
Re-link after re-add ≤ 2 min 55 s, ≤ 3 min 7 s (one recovery_required) ≤ 2 min 59 s (two)
Launch → Request and Pay offered ≤ 1 min 14 s to 2 min 51 s 35 s – 2 min 21 s
Cold launch → session restored, quiet 12.1 s, ~12 s 11–12 s
Cold launch 38 s after a payment broadcast 69.3 s (A), 62.6 s (B) n/a
Cold launch 12 s after sharing ON settled 6 min 2 s (A), 63.4 s (B) 61 s, 11 s

Launches shortly after a write are slow, and this is for #868, not blocking. Payment and sharing timings match rc63. The launch does not:

  • About a minute: both sides logged Deferred session restoration, keeping saved session with 3–4 concurrent_update lines when relaunched 38 s after a payment, and B did again after the sharing toggle.
  • Six minutes on A, relaunched 12 s after sharing ON settled: Deferred session restoration at 21:00:44.374 and 21:01:06.774, SharedStateBusy … pending writes could not be ruled out; retry later four times (endpoint refresh and Backup failed for 'WALLET'), then no Paykit or Pubky line from 21:01:26 until Paykit session restored at 21:06:37.556. That gap is the SDK's five-minute uncertain-write wait.
  • While restoration is deferred there is no bell, activity rows lose the contact name, and a contact deeplink fails with Failed to load contacts before routing contact link: ConcurrentUpdate.

So terminating the app within about a minute of a write costs the next launch one to six minutes. This also corrects what I wrote on the Android PR: the slow relaunch is not Android-only.

Other lines seen: Failed to receive Paykit private messages from pubkydso8zn5…: private receive failed on A 45 s after B's re-add; two CancellationError() warnings right after sharing OFF; one Pay tap during re-link (21:08:11) spun over 25 s and ended with nothing shown.

defer {
if case .automaticRecovery = mode, revision == Self.sessionRevision {
completedRecoveryVersion += 1
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW (question) — the recovery counter advances when the task exits without attempting

The defer bumps completedRecoveryVersion on every exit of an .automaticRecovery task, including the early return at the sessionMutationCount == 0 guard two lines below, where no recovery was attempted. Callers waiting in restoreSessionIfNeeded then return as if an attempt had completed. The periodic retry covers it, but the one-shot callers (AppScene.swift:1182, :1764, ProfileView.swift:283) get no second try. Should the bump happen only after initializeSessionState ran?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: update paykit to the pubky 0.14 release

4 participants