feat: upgrade paykit to rc51 - #697
Conversation
Greptile SummaryThe PR upgrades Paykit to rc50 and adopts app-scoped Pubky grants with environment-stable Bitkit client IDs.
Confidence Score: 4/5The 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
|
| 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
Reviews (1): Last reviewed commit: "feat: upgrade paykit auth to rc50" | Re-trigger Greptile
147778a to
efcf886
Compare
|
The failed unit job was caused by stale test URLs from before rc50: those fixtures still generated legacy |
piotr-iohk
left a comment
There was a problem hiding this comment.
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.
|
starting review |
jvsena42
left a comment
There was a problem hiding this comment.
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.restoreSavedContactsretry 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 insidecompleteAuth()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:367—deleteProfile()can queue a republish for a profile that no longer exists.PubkyProfileManager.swift:965— a throwingforgetSessionAccess()aborts the whole backup restore (QA item 4).PubkyService.swift:1220— delete order plus the newtrycan strand a restorable session secret.PublicPaykitService.swift:104—.publishReceiverMarkeris 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
|
One final self-review follow-up is in |
0cdee24 to
2777d79
Compare
There was a problem hiding this comment.
The grant-binding, revoke-on-sign-out and reconciliation work is sound, and every thread I opened earlier is fixed at head. Approval pinning is clean — AppViewModel.swift:805-806 puts the parsed request and the authUrl into one immutable PubkyAuthApprovalConfig, the sheet passes both together, and approvalBootstrap re-parses that same URL and fails closed unless approvedClientID == requestClientID. No TOCTOU.
One migration gap the review rounds haven't covered: rc48+ only accepts grant-backed sessions, pubky 0.11 still successfully restores the cookie-format secrets rc31/rc46 wrote, and the resulting error isn't the one the deferral predicate matches. Non-blocking given the dev-flag gating, but it leaves affected devices with a permanently broken Pubky tab until reinstall. Details inline.
Also replied on PubkyProfileManager.swift:553 — the fix I asked for there over-corrected and can now revoke a pre-existing identity on a cancelled sign-in.
One amendment to my earlier note: I wrote that the rc51 context string matches session.rs:372. That's right, but it's the check that misses the second context the SDK emits on the same path.
70c51fa to
9d10c44
Compare
There was a problem hiding this comment.
QA Notes
iPhone 17 simulator, iOS 26.5, on a second Mac (m1a), regtest. Debug build from a clean checkout of this head; the installed Bitkit.debug.dylib was checked for this branch's added string before the first tap.
-
✅ passed: Disconnect while online returns to the disconnected profile state, with
paykit_sessionandpubky_secret_keyboth cleared. -
✅ passed: with the app already signed in and its own sockets cut (the host stayed online throughout), Disconnect surfaced
Private Paykit is not available.and left the profile and private Paykit state intact; both reconciliation flags were set and the retry online then succeeded. -
✅ passed: a separate requester holding its own client ID kept its grant after Bitkit signed out, so sign-out revoked only Bitkit's own app-scoped grant.
-
✅ passed: restoring a backup carrying a different Pubky identity forgot the previous local session and installed the backup identity.
-
✅ passed: two privately linked wallets exchanged a 1,000-sat Lightning and a 25,000-sat on-chain request in each direction; after the peer paid all four, both wallets still showed all four rows after a refresh and an app restart.
On item 3, a second Bitkit install cannot serve as the "other app": PubkyService.clientID resolves to staging.bitkit.to on every non-mainnet build, so both installs would request the same app-scoped grant. The other app was therefore a minimal requester built against the same rc51 SDK with a distinct client ID, approved through Bitkit's own pubkyauth:// sheet.
Approve.
Replies
ben-kaufman: The approval sheet does have a visual change: it now shows the requester ID above the permissions. (comment)
That settles it, and moving the images to attachments in aa9cf0a8 keeps the evidence while dropping 2 MB of PNGs from the repo. Both attachment links still resolve, and the long-id shot shows the 253-character id truncated to one line with the trust warning and both action buttons visible.
Coverage
Total: 80%
- Journeys: 50% - The enable-contact-payments path now has an end-to-end service test, but no journey file exercises the Pubky screens.
- Unit tests: 90% - The restored deferral is pinned at the
enablelevel against the live service, and the test asserting the reverted behaviour is gone. - QA: 100% - All 5 Manual Tests driven and passed on the reviewed head.
Reviewed by Claude Code (claude-opus-5-high) via gh-pr-review-loop skill
addressed - reaudit confirmed
ovitrif
left a comment
There was a problem hiding this comment.
Approval to concur with the review bot























Fixes #723
This PR:
0.1.0-rc46to0.1.0-rc51.Description
Bitkit now identifies itself as
bitkit.toon mainnet andstaging.bitkit.toelsewhere. 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 has not launched, so migration from earlier development builds is intentionally unsupported. There is no upgrade handling for cookie-backed sessions or grants using old client IDs. Normal recovery of current-format grant sessions remains supported.
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
regression:restore a wallet backup with different Pubky state: the previous local session is forgotten and the backup identity is installed.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.git diff --checkpassed.