feat: upgrade paykit to rc51 - #1200
Conversation
Greptile SummaryUpgrades Paykit to rc50 and adopts app-scoped Pubky grants, remote grant revocation on normal sign-out, and local-only session forgetting for destructive flows.
Confidence Score: 2/5This PR should not merge until cancellation reliably revokes newly activated grants and failed sign-out no longer leaves authenticated users with removed payment endpoints. Session activation can complete before its cleanup guard is set, leaving a valid grant after cancellation, while the sign-out ordering removes payment publication before a revocation failure returns with the identity still active. Files Needing Attention: app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/repositories/PubkyRepo.kt | Adds revocation-aware authentication, creation, sign-out, wipe, and restore behavior, but cancellation can bypass revocation and failed sign-out can leave endpoint publication inconsistent. |
| app/src/main/java/to/bitkit/services/PaykitSdkService.kt | Adopts rc50 client IDs and local session forgetting while preserving the existing serialized SDK boundary. |
| app/src/main/java/to/bitkit/services/PubkyService.kt | Replaces force/local-clear APIs with the rc50 local forget operation and retains queued service execution. |
| app/src/main/java/to/bitkit/ui/screens/profile/ProfileViewModel.kt | Clears private Paykit state only after confirmed remote sign-out and preserves it on failure. |
| app/src/main/java/to/bitkit/ui/screens/profile/EditProfileViewModel.kt | Aligns profile disconnect behavior with fail-closed remote grant revocation. |
Sequence Diagram
sequenceDiagram
participant UI
participant Repo as PubkyRepo
participant Paykit
participant Pubky
UI->>Repo: Sign out
Repo->>Paykit: Remove published endpoints
Paykit-->>Repo: Endpoints removed
Repo->>Pubky: Revoke Bitkit grant
alt Revocation succeeds
Pubky-->>Repo: Success
Repo->>Repo: Clear local identity and Paykit state
Repo-->>UI: Signed out
else Revocation fails
Pubky-->>Repo: Error
Repo-->>UI: Preserve identity and report error
Note over Repo,Paykit: Published endpoints remain removed
end
Reviews (1): Last reviewed commit: "feat: upgrade paykit auth to rc50" | Re-trigger Greptile
|
The cancellation and cleanup findings are fixed in signed commit |
piotr-iohk
left a comment
There was a problem hiding this comment.
e2e ACK.
Latest (dd3b7ba) 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 Pixel emu: online Delete/Disconnect clears session + secret for pubkyif…ejhsa1y. Offline Delete: transport_error, profile kept. Offline Disconnect from that dialog: endpoint cleanup WARN, then ~22s later network returned and session cleared. Offline Disconnect waited for network instead of a clean fail-closed error. Not a blocker.
Did not retest other-app grant stays authorized, or backup replace.
|
Non blocking: I got this error when trying to sign in with Pubky Ring Probably same issue of jvsena42/loopky#130 Screen_recording_20260902_142153.mp4Will probably work on next Ring release |
jvsena42
left a comment
There was a problem hiding this comment.
Review notes from testing this branch on a dev build (regtest, Pixel_9 emulator).
The first two comments were reproduced on device with logs attached inline; the rest come from reading the diff. The happy paths all worked: create identity, restore across restart, and online profile delete with full keychain teardown.
|
@jvsena42 Confirmed the remaining Ring observation separately: rc50 intentionally emits the grant-auth request shape, while the currently installed Ring build does not yet recognize it. That is a Ring compatibility gap for its next release, not another Bitkit change. We will retest the end-to-end handoff with a Ring build that supports grant auth. |
jvsena42
left a comment
There was a problem hiding this comment.
Follow-up on error localization in 65491308d/54a620df0.
jvsena42
left a comment
There was a problem hiding this comment.
Follow-up on the sign-out cancellation hardening in 54a620df0.
jvsena42
left a comment
There was a problem hiding this comment.
One more on the client-ID change.
jvsena42
left a comment
There was a problem hiding this comment.
The consequence of the client-ID change on the companion-claim path.
447e781 to
36bfd02
Compare
a681dfc
21d5a06 to
a681dfc
Compare
|
Follow-up cross-platform lifecycle audit is fixed in |
|
One final self-review follow-up is in |
ovitrif
left a comment
There was a problem hiding this comment.
The ghost-session fix is close but stops one level short. discardAbandonedSession() now falls back to forgetSessionAccess() when the remote revoke fails, and that is the right shape, but forgetSessionAccess() does not delete the keychain credentials the way the code it replaced did. PubkyRepo already has a network-independent clearLocalState() that both signOut() and wipeLocalState() use; the abandoned-session path is the only cleanup that never reaches it, so a canceled Ring completion or a failed identity creation can still leave a live grant behind PubkyAuthState.Idle when both calls fail.
One non-blocking note follows: the new fallback has no test that would fail if it stopped clearing the session.
There was a problem hiding this comment.
Went through the auth/session/credential lifecycle at head against full-file context, and pulled the rc51 AAR to check the error-context strings rather than assume them. Notes:
- rc51 context strings match. The native lib contains the exact literal
restore Pubky grant session from platform provider, socanDeferStaleSession(PaykitSdkService.kt:1088-1089,1108) matches it. rc51 also addsgrant credential has expiredandstored grant credential client key does not match the grant cnf, and confirmssignOutfails withcannot revoke Pubky grant without persisted session accesswhen nothing is persisted — which is what thediscardAbandonedSessionfallback chain is built around. - Cancellation semantics are sound.
ServiceQueue.CORE.backgroundruns under its ownSupervisorJob, so cancelled callers still wait for the Rust call and then getCancellationException; theNonCancellableplacement at :297-300, the flag clear at :334/:575, and thecatch (CancellationException)paths behave as intended. No path leaves a persisted session with_publicKey == nullexcept through logged double failures, which now fall back to local credential deletion. completeAuthenticationarms its revoke flag correctly —shouldRevokeSessionOnFailureis set insidewithContext(NonCancellable)afterwaitForAuthApprovalreturns, so a cancelled or timed-out Ring prompt sits outside the armed region and cannot revoke a pre-existing identity. (iOS diverged here and does have that problem; worth not "aligning" the two in that direction.)- Backup restore ordering is fine:
restoreSessionBackupState(METADATA,BackupRepo.kt:677) runs beforepaykitSdkService.restoreBackupState(WALLET,PrivatePaykitRepo.kt:523), soforgetSessionAccess()doesn't wipe a restored SDK blob. - Requester pinning holds —
approvalBootstrapuses the request's own client ID, and the displayed and approvedclientIdcome from the same immutable URL, so no TOCTOU. createIdentity's revoke-while-live-session path is only reachable viabitkit://screen/create-profile, andScreenDeepLinkRuntime.isEnabledis hard-codedfalseinapp/src/release— debug-only.
One migration gap, non-blocking (Paykit is behind isPaykitEnabled, default false in SettingsStore.kt:42, so this reaches dev/QA devices rather than shipped users — but it becomes user-facing the moment the flag flips on with rc46-era sessions still on disk).
canDeferStaleSession (PaykitSdkService.kt:1088-1089, :1108) matches a single literal:
fun canDeferStaleSession(errorContext: String): Boolean =
errorContext == STALE_SESSION_RESTORE_CONTEXT && hasSessionAccess()
const val STALE_SESSION_RESTORE_CONTEXT = "restore Pubky grant session from platform provider"consumed at :178 as if (!sessionProvider.canDeferStaleSession(e.context)) throw e.
In rc51 that literal (paykit-ffi/src/session.rs:372) wraps only an error from restore_session. A session that restores successfully but isn't grant-backed produces a different, non-deferrable context: "Pubky session must be grant-backed" (session.rs:431 validate_grant_session_client_id, also paykit-sdk/src/identity.rs:289).
Why an upgrade lands there:
libs.versions.tomlgoes0.1.0-rc46→0.1.0-rc51, i.e. pubky 0.8.0 → 0.11.0.- rc46 persists
export_secret()— pubky 0.8.0's<pubkey_z32>:<cookie>. pubky 0.8.0 has zero occurrences ofas_grant/GrantCredentialand rc46 zero ofgrant-backed. - pubky 0.11.0
pubky.rs:382 restore_sessionstill accepts legacy tokens by design (nopubky-grant-credential-prefix →import_secret→ cookie credential), andgrant/view.rs:28documents that cookie-backed sessions returnNonefromas_grant(). loadSessionAccess()(:1063-1081) buildsPubkySessionAccessfrom the raw keychain string with no format check.- → non-deferrable throw → Paykit never initialises.
I grepped the full origin/master...origin/codex/paykit-rc50-auth diff: no legacy/cookie/migration handling anywhere.
Android degrades more gracefully than iOS but is still non-functional: PubkyRepo.initialize() (:166-170) wraps this in runSuspendCatching { }.onFailure { Logger.error(...) }.getOrNull() ?: return@withContext, so it logs and returns, and ContentView.kt:1914-1920 navigateToProfile has no error branch, so the user does reach Routes.PubkyChoice. But startAuth()/signUp() all begin with isSetup.await() (PaykitSdkService.kt:206+), and initialize() completed that deferred exceptionally, so re-auth fails too. PubkyRepo is constructed once per process, so every cold start reproduces it.
Caveat that decides severity: this depends on the homeserver still revalidating the stored cookie. Valid cookie → grant-backed error → broken. Expired → RequestExpired → deferrable → re-sign-in recovers.
Fix options, smallest first: widen the predicate to the grant-backed and client-ID-mismatch contexts (or invert it — defer on any identity error when a stored session exists); or clear the stale session on the non-recoverable branch so the re-sign-in fallback runs.
|
Addressed the legacy-session restore gap in c461106. The stale-session predicate now recognizes rc51's cookie-backed and grant-client mismatch contexts in addition to restore failure, while still requiring stored session access and rejecting unrelated identity errors. This lets the existing re-authentication path run instead of leaving startup permanently failed. Mirrored on iOS #697. The cleanup-flag follow-up is also fixed and regression-tested; full Android unit tests passed. |
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at c4611067d. No HIGH/MEDIUM — nothing to file. One candidate died on verification and I'm recording why below so it doesn't get re-chased.
Migration was the top risk for this bump and the fix is correct — traced through the rc51 Rust, not assumed. For a stored rc46 cookie: paykit-ffi/src/session.rs:367-375 restore_session succeeds (pubky 0.11 still accepts legacy tokens) → validate_grant_session_client_id:430 → Identity{"Pubky session must be grant-backed"} → runtime/mod.rs:355 propagates with ? and no re-wrap → sdk.rs:182-188 → errors.rs:40-44 → Kotlin PaykitException.Identity.context → canDeferStaleSession:1088-1093 returns true. The client-ID mismatch format at session.rs:437-439 is Pubky grant client ID `{actual}` did not match `{expected}` , so the startsWith prefix is exact rather than an equality miss. After deferral, local-seed users re-sign-in and persistSessionAccess overwrites the cookie with a grant secret — a transparent migration — while Ring users get RestorationFailed plus the expired-session toast and re-auth overwrites it.
PAYKIT_SDK_STATE is format-stable across the bump, which matters because runtime/mod.rs:358-377 writes an identity blob even with no session, so every rc46 device has one. IdentityState is field-identical between the tags including sign_out_generation, paykit-ffi/src/storage.rs is byte-identical with the same blob version constant, and the storage/records.rs serde shapes are unchanged.
Blast radius is nil. Paykit has never shipped — v2.3.2, v2.4.0 and v2.4.1 libs.versions.toml carry no paykit dependency, and rc46 exists only on master. So every upgrade concern here is dev/QA by construction.
Binding API diff rc46→rc51 (javap): PubkyClientConfig gained authRelayUrl, and since the app uses defaultPubkyClientConfig() it picks up the SDK default — the same relay rc46 URLs already carried — with the E2E override unchanged. PubkySessionAccess's constructor now throws, but only validates ClientId, which is the constant bitkit.to/staging.bitkit.to. resumeAuth's signature changed and is unused. No casts, !!, or catch-alls were introduced to keep anything compiling, which was the specific failure I was looking for.
Sign-out semantics check out. rc51 sign_out is revoke → clear_local_identity_state → provider clearSessionAccess → clearPubkySessionCredentials, and Keychain.delete is edit { remove } which doesn't throw on a missing key, so Ring users without PUBKY_SECRET_KEY can't fail post-revocation. deleteProfile's markPublicCleanupPending = hasPaykitState() keeps signOut's hadPaykitState true because hasPaykitState includes publicPaykitCleanupPending, so the retry flag survives clearLocalState. The rc50→rc51 delta itself is one 12-line patch in payment_requests/derivation.rs changing an event sort key — no amount or destination derivation touched.
The candidate I dropped: rc51 rejects pre-grant pubkyauth:// URLs and that rejection reaches the approval toast as a raw code=protocol_error, context=… string. Real mechanically, but pre-existing — master at rc46 has the identical unwrapped parseAuthUrl and the same PubkyAuthErrorMessage fallthrough, so any malformed URL already surfaced a raw string; rc51 only adds one more trigger. It's also the same mechanism as my existing :172 thread, which you closed by mapping the two new IllegalArgumentException sites. And the grant-only behaviour is intentional SDK design per your note about the Ring-side compatibility gap, so there's nothing here for Bitkit to fix.
|
Thanks @jvsena42. One explicit scope correction to the migration part of your review: Paykit has not launched, so we intentionally do not support migrations from earlier development builds. d888261 removes the cookie-backed and old client-ID session recovery cases and their legacy-only test cases. Normal offline/expired recovery for current-format grants and the pending endpoint-cleanup fix remain intact. The same cleanup is in iOS #697, and signup #1224 is restacked on top. Compilation, all 2,316 unit tests, and lint pass here; the restacked signup branch also passes all 2,346 tests. This is intentional, not an outstanding migration issue to fix before launch. |
There was a problem hiding this comment.
QA Notes
Tested on a sdk_gphone16k_arm64 emulator (Android 17, regtest), built and installed from this head with binary identity confirmed.
- 1. Pubky profile → delete/sign out while online: returned to the disconnected profile state, and the log shows
PAYKIT_SESSIONandPUBKY_SECRET_KEYboth deleted. - 2. Pubky profile → network disabled → delete/sign out: the app showed
Unable to Delete Profilewith Retry and Disconnect, and no session deletion was logged, so local state survived the failed revoke. Restoring the network and retrying then completed the sign-out with the full keychain teardown.
Tests 3 to 5 were not run: they need a separate Pubky Ring approver, a backup with different Pubky state, and two funded privately linked wallets, none of which this environment provides. None of them touch the lines changed in this delta.
The clearLocalState() fallback is now the last resort when both the remote revoke and forgetSessionAccess() fail, and PubkyRepoTest covers it by stubbing both to throw and asserting both keychain deletions. publicPaykitCleanupPending = true is the right call there, since a failed revoke can leave endpoints published and AppViewModel reconciles that flag later.
Approve.
—
Reviewed by Claude Code (claude-opus-5 high) via gh-pr-review-loop skill
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed d8882611f ("refactor: remove paykit migration handling"). No HIGH/MEDIUM on Android — safe.
I checked this carefully because I flagged the same removal as a problem on the iOS twin (#697), and because my own note that Paykit has never shipped may be what prompted it. The removed clauses were "Pubky session must be grant-backed" and the "Pubky grant client ID \"` prefix. The first is genuinely migration-only — it fires for an rc46 cookie session, and an rc46-era QA device now gets a silent init failure and the unauthenticated choice screen rather than a deferral, which is consistent with dropping migration support on a feature that never shipped.
The second is the one that isn't a migration condition — validate_grant_session_client_id raises it whenever a stored grant's client id differs from the one the build passes, so on iOS a Debug build run over a TestFlight build hits it. Android is not exposed, and the reason is structural rather than luck: app/build.gradle.kts gives each flavour an applicationIdSuffix (.dev / "" / .tnet), so the flavours have separate keychains and a grant can't cross between them. Env.network is compile-time from BuildConfig.NETWORK, BitkitPaykitSdkConfig.clientId is identical at every commit on this branch, and vssStoreIdPrefix is network-scoped so a backup can't carry a grant across networks either. approvalBootstrap uses the requester's client id but never persists to PAYKIT_SESSION.
So the mismatch can't arise here and the removal costs nothing on this platform. I've raised the client-id half on #697, where the single shared bundle id makes it reachable — you may want to keep the prefix clause on Android purely for parity if iOS restores it, but there's no defect here either way.
Test narrowing is fine: missing session or unrelated identity failures are not deferred still asserts the surviving context with a null keychain, so no live coverage was lost.
Fixes #1219
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 wallet wipe and backup replacement forget the local session. Existing local E2E homeserver behavior remains unchanged.
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.
Preview
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
PubkyRepoTest.kt: covers canceled completed authentication revocation, revocation failure, wallet wipe, and backup replacement.ProfileViewModelTest.ktandEditProfileViewModelTest.kt: cover preserving private Paykit state when sign-out fails.PaykitSdkServiceTest.kt: covers the stable Bitkit client ID and Pubky client configuration.Linked Issues/Tasks