Skip to content

feat: share paykit state across apps - #856

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

ben-kaufman wants to merge 41 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-rc62 SDK.

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

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

  • 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

  • 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.
  • 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; this requires fault injection.
  • 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.
  • 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.
  • 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 PaykitContactKeysObserverTests.swift - hosted SwiftUI checks for stable contact keys, membership changes, and callback-driven view updates.
  • 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, 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 rc62 from the signed release tag and its framework archive matches the manifest checksum.

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.

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

@ovitrif ovitrif 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.

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

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 2, 2026 20:36

@ovitrif ovitrif 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.

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)

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.

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?

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.

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

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)

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift
Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift

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

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.

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift

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

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, toasts contacts__error_loading with error.localizedDescription.
  • ContactsManager.loadContacts logs and rethrows CancellationError as 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.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 2, 2026 22:23

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

Comment thread Bitkit/AppScene.swift Outdated
@ben-kaufman
ben-kaufman requested a review from jvsena42 October 2, 2026 22:35

@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 (iOS 21a7a60) inline. 4dd4d60 is fine: discovery still runs the flag, identity, link and capability checks for the selected contact and fails closed. The open HIGHs await the staging retest at this head.

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift
@ben-kaufman
ben-kaufman requested a review from jvsena42 October 3, 2026 01:29

@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 new LOW, inline. The earlier withdrawal MEDIUM is fixed and resolved. The eligibility and lock-load HIGHs stay open for the device retest at c334a6f, which is running now.

}

func updateSavedPublicKeys(_ publicKeys: [String]) {
guard publicKeys != savedPublicKeys else { return }

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: updateSavedPublicKeys treats a reordered contact list as a changed one.

It compares ordered arrays, but its two callers pass different orders:

  • PaykitContactKeysObserver passes keys sorted by key (AppScene.swift:215).
  • The inbox round passes them in display-name order (:1124, :1149).

So whenever display order differs from key order, the next inbox round sees a "change" and bumps eligibilityGeneration (:1301). Two cases:

  • Renaming a contact. updateContact re-sorts the list (ContactsManager.swift:421-422), but the observer doesn't fire because the key set is unchanged. The next inbox round at :1124 still reads it as a change.
  • The window between the observer and :1124. A propose that starts there snapshots the key-sorted list, so its post-await check at :1491 fails.

The bump discards an eligibility discovery already in flight (:1325-1326), or a propose's history insert (:1491). Nothing is lost for good: the next 30–60 s refresh recovers it. It is new in this PR; neither the observer nor updateSavedPublicKeys exists on master.

Fix: compare normalized key sets at this line. Don't sort what gets stored, because :1326 and :1491 compare it against callers' display-order arrays.

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 10b85a1. Eligibility invalidation and the post-await checks now compare normalized key sets, so reordering the same contacts does not discard a completed refresh. The regression test covers reordering while a refresh is in flight.

jvsena42
jvsena42 previously approved these changes Oct 3, 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.

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.

@ben-kaufman

ben-kaufman commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor Author

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.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

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

@ben-kaufman

ben-kaufman commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor Author

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 Cannot find worker "blocktank-lsp-btc". Those tests and their Blocktank path are unchanged by this PR. Integration log.

The local E2E run passed 12 scenarios. Hardware-wallet transfer to spending failed on all three attempts because HardwareTransferAmountContinue stayed disabled. Its screenshot shows zero available funds after test funding. The screen and transfer-limit implementation match current master, but the artifact logs do not establish why the limit stayed zero, so I am not calling it a confirmed backend failure or a Paykit regression. E2E log and artifacts.

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.

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