Skip to content

feat: upgrade paykit to rc51 - #697

Open
ben-kaufman wants to merge 14 commits into
masterfrom
codex/paykit-rc50-auth
Open

feat: upgrade paykit to rc51#697
ben-kaufman wants to merge 14 commits into
masterfrom
codex/paykit-rc50-auth

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #723

This PR:

  1. Upgrades Paykit from 0.1.0-rc46 to 0.1.0-rc51.
  2. Adopts app-scoped Pubky grants using Bitkit's stable client ID.
  3. Revokes Bitkit's grant on normal sign-out while preserving local state when remote revocation fails.
  4. Restricts local-only session forgetting to destructive reset and backup-replacement flows.
  5. Fixes payment requests disappearing from history after requests are exchanged and paid in both directions.

Description

Bitkit now identifies itself as bitkit.to on mainnet and staging.bitkit.to elsewhere. Normal sign-out remotely revokes only Bitkit's current grant. If revocation cannot be confirmed, the profile and private Paykit state remain available so the user can retry.

Completed Ring authentication that is later canceled, and identity creation that fails after activating a session, also attempt secure revocation. Explicit app reset and backup replacement forget the local session.

Paykit rc51 also fixes replay ordering when incoming and outgoing Payment Request lifecycles interleave. Each request's events are derived in the correct order, preserving its payer/payee role so the existing history mapping retains the row. The fix is in paykit-rs #151 and is available in the published release before that PR merges.

Linked Issues/Tasks

Screenshot / Video

N/A — no visual changes.

QA Notes

Manual Tests

  • 1. Pubky profile → Sign Out while online: Bitkit signs out and returns to the disconnected profile state.
  • 2. Pubky profile → interrupt network access → Sign Out: Bitkit shows an error and keeps the profile and private Paykit state; restore network access and retry successfully.
  • 3. Pubky auth → approve another app → Sign Out of Bitkit: Bitkit's session is revoked while the other app remains authorized.
  • 4. regression: restore a wallet backup with different Pubky state: the previous local session is forgotten and the backup identity is installed.
  • 5. regression: two privately linked wallets → each creates a 1,000-sat Lightning request and a 25,000-sat on-chain request → the peer pays all four → Payment Requests: both wallets retain all four rows after refresh and app restart.

Automated Checks

  • PubkyProfileManagerTests.swift: covers canceled completed authentication revocation and backup session replacement.
  • PaykitSdkClientConfigTests.swift: covers the stable Bitkit client ID and Pubky client configuration.
  • Upstream rc51 regression covers interleaved bidirectional request events; Paykit tests and both binding builds passed.
  • iOS simulator build and Paykit/auth tests passed against rc51, including payment-request history, payment proofs, public/private resolution, receiver keys, client configuration, and profile lifecycle.
  • Latest review verification: 103 auth, profile, and public-Paykit tests passed on stacked feat: support Pubky signup #724, including the private-only receiver-marker reconciliation regression.
  • Verified the resolved SDK commit and rc51 XCFramework checksum with a fresh compiler/module cache; git diff --check passed.

@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown

Greptile Summary

The PR upgrades Paykit to rc50 and adopts app-scoped Pubky grants with environment-stable Bitkit client IDs.

  • Replaces local session clearing with explicit remote revocation for ordinary sign-out and completed authentication cleanup.
  • Introduces local-only session forgetting for destructive reset and backup replacement.
  • Updates session bootstrap/provider configuration and adds focused authentication and client-ID tests.
  • Sign-out currently removes payment-sharing state before revocation is confirmed, undermining the intended retry behavior when revocation fails.

Confidence Score: 4/5

The PR should not merge until failed grant revocation can leave the retained authenticated account's payment-sharing state intact for a safe retry.

Normal sign-out deletes and persists endpoint state before attempting the operation allowed to fail, so the advertised failure recovery retains the identity but not its prior payment configuration.

Files Needing Attention: Bitkit/Managers/PubkyProfileManager.swift

Important Files Changed

Filename Overview
Bitkit/Managers/PubkyProfileManager.swift Coordinates the new revoke/forget lifecycle, but normal sign-out mutates payment state before revocation succeeds and can leave a retained account partially dismantled.
Bitkit/Services/PubkyService.swift Adopts rc50 client-scoped bootstrap/session access and exposes explicit revoke and local-forget operations.
BitkitTests/PubkyProfileManagerTests.swift Updates cancellation and backup-replacement tests, but does not cover normal sign-out when endpoint cleanup succeeds and revocation fails.
BitkitTests/PaykitSdkClientConfigTests.swift Verifies the network-dependent stable Bitkit client ID and existing Pubky client configuration.
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved Resolves Paykit 0.1.0-rc50 at the updated revision.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant M as PubkyProfileManager
    participant P as Paykit endpoint state
    participant G as Pubky grant
    U->>M: Sign out
    M->>P: Remove private/public endpoints
    P-->>M: Cleanup persisted
    M->>G: Revoke Bitkit grant
    G-->>M: Revocation error
    M-->>U: Show error and remain authenticated
    Note over U,P: Account remains active with payment sharing already dismantled
Loading

Reviews (1): Last reviewed commit: "feat: upgrade paykit auth to rc50" | Re-trigger Greptile

Comment thread Bitkit/Managers/PubkyProfileManager.swift Outdated

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

The restore-on-retry path for private contact endpoints is untested. After a failed sign-out the account stays authenticated with sharing still enabled, and nothing would fail if that publishingEnabledKey check were inverted so retry kept deleting those endpoints.

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift Outdated
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

The failed unit job was caused by stale test URLs from before rc50: those fixtures still generated legacy signin requests, while rc50 correctly requires signin_grant plus cid and cpk. Signed commit 3a0b0e97 updates only the fixtures. The exact two suites that failed in CI now pass 52/52 locally, SwiftFormat and git diff --check are clean, and fresh CI is running. @ovitrif @jvsena42 please re-review the current head.

ovitrif
ovitrif previously approved these changes Sep 1, 2026

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

utACK

piotr-iohk
piotr-iohk previously approved these changes Sep 2, 2026

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

e2e ACK.

Latest (3a0b0e9) with matching e2e branch codex/paykit-rc50-auth (#212). Recreated the two staging Paykit fixture pubkys. e2e-tests-staging - pubky_paykit green. Full CI green.

Manual on iPhone 17 sim: online Delete/Disconnect clears paykit_session. Offline Delete: transport_error, profile kept. Offline Disconnect from that dialog: endpoint cleanup WARN, no revoke-failure log, session still in keychain; next launch restored pubkyyc14…4rso. Offline Disconnect looked hung rather than a clean error + retry. Not a blocker.

Did not retest other-app grant stays authorized, or backup replace.

@ben-kaufman
ben-kaufman dismissed stale reviews from ovitrif and piotr-iohk via 8cb8220 September 2, 2026 18:03
@ben-kaufman
ben-kaufman requested a review from ovitrif September 2, 2026 18:17

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

The SDK approval path that binds an external grant to the requester's client ID is untested. The sheet test only records the value passed into a fake, so using Bitkit's own clientID in approvalBootstrap would still pass.

Comment thread Bitkit/Services/PubkyService.swift
@jvsena42

jvsena42 commented Sep 4, 2026

Copy link
Copy Markdown
Member

starting review

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

Superseded by the request-changes review below, which carries the same findings and inline comments.

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

Requesting changes: two high-severity issues below can leave a live Pubky grant or a published endpoint set behind, which is the opposite of what this PR sets out to guarantee.

Scope was origin/master...HEAD (17 files). The hardware-send / SwipeButton changes visible in a master...HEAD diff are already merged via #708 and out of scope.

Blocking

  • PrivatePaykitService+Contacts.swift:273 — the .restoreSavedContacts retry retires itself without republishing whenever the contact list has not loaded yet, which is exactly the offline case it exists for.
  • PubkyProfileManager.swift:528 — a session persisted inside completeAuth() is never revoked when the call throws afterwards, and gets signed back in on the next launch.

Non-blocking, but worth resolving in this PR

  • PubkyProfileManager.swift:367deleteProfile() can queue a republish for a profile that no longer exists.
  • PubkyProfileManager.swift:965 — a throwing forgetSessionAccess() aborts the whole backup restore (QA item 4).
  • PubkyService.swift:1220 — delete order plus the new try can strand a restorable session secret.
  • PublicPaykitService.swift:104.publishReceiverMarker is unreachable; both new tests only assert that unreachable pair.

Verification. Cleared the stale .pcm cache (the documented paykitFFI.h modified-since-module symptom of an rc bump), then ran the paykit suites in the simulator: 150 tests, 0 failures. The only failures in a full run were AddressTypeIntegrationTests hitting blocktank regtest → 404, which needs the Docker stack and is unrelated.

Each finding was reproduced as a failing test locally, and each suggested fix + regression test was applied locally and confirmed to flip it green with the 150 still passing. Nothing is committed — the diffs are suggestions, not a branch. Exceptions: finding 5 is confirmed by source only (PaykitSdkSessionProvider is private, so its test needs the seam in that comment), and finding 6's test is a forward guard that already passes.

Findings 2 and 6 are windows the Android counterpart (synonymdev/bitkit-android#1200) closed in the same PR that iOS has not mirrored.

Checked and correct: the rc50 stale-session context string matches paykit-ffi/src/session.rs:372; AppError(message: "pubky_auth__invalid_request") resolves through errorDescription's t(message), so no raw key reaches the user (the Android analogue leaked English); the requester label is in a ScrollView with lineLimit(1) + .tail, so a 253-char client ID cannot push the trust warning out of view; approvalBootstrap passing the requester's client ID is required by approve_auth's validate_auth_url_client_id; forgetSessionAccess does clear activeAuthRequest.

🤖 Generated with Claude Code

https://claude.ai/code/session_015HivAJvbwpdPjX99xw7sRP

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift
Comment thread Bitkit/Managers/PubkyProfileManager.swift
Comment thread Bitkit/Managers/PubkyProfileManager.swift
Comment thread Bitkit/Managers/PubkyProfileManager.swift Outdated
Comment thread Bitkit/Services/PubkyService.swift Outdated
Comment thread Bitkit/Services/PublicPaykitService.swift Outdated
@ben-kaufman
ben-kaufman requested a review from jvsena42 September 4, 2026 12:27
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

One final self-review follow-up is in 0cdee243: the explicit forget-session path now resets the in-memory Paykit runtime even when SDK/keychain teardown throws, so a backup restore cannot continue with the old runtime instance. The focused auth/session suite passes (53/53; the broader review suite passed 89/89 before this one-line cleanup change).

@ben-kaufman
ben-kaufman force-pushed the codex/paykit-rc50-auth branch from 0cdee24 to 2777d79 Compare September 4, 2026 16:31
@ben-kaufman ben-kaufman changed the title feat: upgrade paykit auth to rc50 feat: upgrade paykit to rc51 Sep 4, 2026

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

The receiver marker teardown in the reconciliation retry does not account for private-only sharing.

A user who has private contact payments on but public sharing off keeps a published receiver marker, and refreshPrivateOnlyPaykitReceiverMarker exists specifically to maintain it. After a failed sign-out arms reconciliation, the retry path routes that state into the .removePublishedState branch and clears the marker, so contacts can no longer send private payments or payment requests. The private reconciliation that runs afterwards only republishes endpoints, so the state does not recover on its own.

Comment thread Bitkit/AppScene.swift Outdated
@ben-kaufman
ben-kaufman requested a review from ovitrif September 6, 2026 15:18
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.

[Bug]: payment request lists diverge after cross-device pay

4 participants