Skip to content

feat: upgrade paykit to rc51 - #1200

Merged
jvsena42 merged 17 commits into
masterfrom
codex/paykit-rc50-auth
Sep 8, 2026
Merged

feat: upgrade paykit to rc51#1200
jvsena42 merged 17 commits into
masterfrom
codex/paykit-rc50-auth

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Contributor

Fixes #1219

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

  • 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

  • PubkyRepoTest.kt: covers canceled completed authentication revocation, revocation failure, wallet wipe, and backup replacement.
  • ProfileViewModelTest.kt and EditProfileViewModelTest.kt: cover preserving private Paykit state when sign-out fails.
  • PaykitSdkServiceTest.kt: 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.
  • Local rc51 compilation, all 2,316 unit tests, and lint pass.
  • Added a regression for canceled approval where remote revoke and SDK forget both fail, verifying both stored credentials are removed and the app remains unauthenticated.

Linked Issues/Tasks

@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown

Greptile Summary

Upgrades Paykit to rc50 and adopts app-scoped Pubky grants, remote grant revocation on normal sign-out, and local-only session forgetting for destructive flows.

  • Configures Paykit with Bitkit’s stable environment-specific client ID.
  • Preserves profile and private Paykit state when remote revocation fails.
  • Adds cleanup for cancelled authentication and incomplete identity creation.
  • Updates session lifecycle tests and release notes.

Confidence Score: 2/5

This 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

Security Review

Identity-creation cancellation can leave an app-scoped grant and persisted session active because the cleanup guard is assigned only after the session-activating service call returns.

Important Files Changed

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
Loading

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

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated
ovitrif

This comment was marked as resolved.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

The cancellation and cleanup findings are fixed in signed commit dd3b7bad. Successful completeAuth() and its completion marker now cross a minimal NonCancellable boundary, so cancellation cannot leave a persisted grant while skipping cleanup; ordinary completion failures still preserve existing sessions. The focused tests cover cancellation after SDK completion and post-activation createIdentity() failure cleanup. All CI, lint, detekt, build, and E2E checks are green on the current head. @ovitrif please re-review.

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

@jvsena42

jvsena42 commented Sep 2, 2026

Copy link
Copy Markdown
Member

Non blocking: I got this error when trying to sign in with Pubky Ring

{
  "action": "unknown",
  "rawInput": "pubkyauth://signin_grant?caps=%2Fpub%2Fpaykit%2Fv0%2Fbitkit%2Fwallet%2F%3Arw%2C%2Fpub%2Fpaykit%2Fv0%2Fprivate%2Fbitkit%2Fwallet%2F%3Arw%2C%2Fpub%2Fstaging.bitkit.to%2Fbitkit%2Fwallet%2F%3Arw&relay=https%3A%2F%2Fhttprelay.pubky.app%2Finbox&secret=9P-JtlM1i5HvaJnmru5fZhgYU5dvpK3sL-KNi-1eAng&cid=staging.bitkit.to&cpk=cc4amfpzrugsxkbqgia9he9yy9jezq4kcjo3wdsgoozar4zt1b5o&x-success=bitkit%3A%2F%2Fpubky-auth%2Fsuccess%3Fnonce%3D90e53840-9b30-4e4d-9751-b6b018beca25&x-cancel=bitkit%3A%2F%2Fpubky-auth%2Fcancel%3Fnonce%3D90e53840-9b30-4e4d-9751-b6b018beca25&x-error=bitkit%3A%2F%2Fpubky-auth%2Ferror%3Fnonce%3D90e53840-9b30-4e4d-9751-b6b018beca25&x-source=Bitkit",
  "error": "Unrecognized format. Expected a recovery phrase, invite code, auth URL, or session request."
}

Probably same issue of jvsena42/loopky#130

Screen_recording_20260902_142153.mp4

Will probably work on next Ring release

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

Comment thread gradle/libs.versions.toml Outdated
Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
@ben-kaufman
ben-kaufman dismissed stale reviews from ovitrif and piotr-iohk via 6549130 September 2, 2026 18:04
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

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

ovitrif
ovitrif previously approved these changes Sep 2, 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

jvsena42

This comment was marked as 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.

Follow-up on error localization in 65491308d/54a620df0.

Comment thread app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalViewModel.kt 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.

Follow-up on the sign-out cancellation hardening in 54a620df0.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt

@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 more on the client-ID change.

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt

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

The consequence of the client-ID change on the companion-claim path.

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
@ben-kaufman
ben-kaufman dismissed stale reviews from jvsena42 and piotr-iohk via a681dfc September 4, 2026 12:24
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-rc50-auth branch from 21d5a06 to a681dfc Compare September 4, 2026 12:24
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Follow-up cross-platform lifecycle audit is fixed in a681dfc6c: auth cleanup is armed before activation can persist a session, profile deletion clears publication intent before a failed sign-out can queue republishing, backup restore proceeds safely when old-session revocation fails, and keychain teardown now attempts both credentials in session-first order. Regression tests cover each edge case. The branch was also rebased onto current master 4bb9d929f; detekt succeeds with only pre-existing repository warnings.

@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 d2634de17: the explicit forget-session path now resets the in-memory Paykit runtime in finally, even when SDK/keychain teardown throws, so restore cannot continue with the old runtime instance. Detekt remains green apart from the existing baseline warnings.

@ben-kaufman ben-kaufman changed the title feat: upgrade paykit auth to rc50 feat: upgrade paykit to rc51 Sep 4, 2026
ovitrif
ovitrif previously requested changes Sep 5, 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 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.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
Comment thread app/src/test/java/to/bitkit/repositories/PubkyRepoTest.kt
@ben-kaufman
ben-kaufman requested a review from ovitrif September 6, 2026 15:20

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

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, so canDeferStaleSession (PaykitSdkService.kt:1088-1089,1108) matches it. rc51 also adds grant credential has expired and stored grant credential client key does not match the grant cnf, and confirms signOut fails with cannot revoke Pubky grant without persisted session access when nothing is persisted — which is what the discardAbandonedSession fallback chain is built around.
  • Cancellation semantics are sound. ServiceQueue.CORE.background runs under its own SupervisorJob, so cancelled callers still wait for the Rust call and then get CancellationException; the NonCancellable placement at :297-300, the flag clear at :334/:575, and the catch (CancellationException) paths behave as intended. No path leaves a persisted session with _publicKey == null except through logged double failures, which now fall back to local credential deletion.
  • completeAuthentication arms its revoke flag correctlyshouldRevokeSessionOnFailure is set inside withContext(NonCancellable) after waitForAuthApproval returns, 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 before paykitSdkService.restoreBackupState (WALLET, PrivatePaykitRepo.kt:523), so forgetSessionAccess() doesn't wipe a restored SDK blob.
  • Requester pinning holdsapprovalBootstrap uses the request's own client ID, and the displayed and approved clientId come from the same immutable URL, so no TOCTOU.
  • createIdentity's revoke-while-live-session path is only reachable via bitkit://screen/create-profile, and ScreenDeepLinkRuntime.isEnabled is hard-coded false in app/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:

  1. libs.versions.toml goes 0.1.0-rc460.1.0-rc51, i.e. pubky 0.8.0 → 0.11.0.
  2. rc46 persists export_secret() — pubky 0.8.0's <pubkey_z32>:<cookie>. pubky 0.8.0 has zero occurrences of as_grant/GrantCredential and rc46 zero of grant-backed.
  3. pubky 0.11.0 pubky.rs:382 restore_session still accepts legacy tokens by design (no pubky-grant-credential- prefix → import_secret → cookie credential), and grant/view.rs:28 documents that cookie-backed sessions return None from as_grant().
  4. loadSessionAccess() (:1063-1081) builds PubkySessionAccess from the raw keychain string with no format check.
  5. → 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.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

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.

@ben-kaufman
ben-kaufman requested a review from jvsena42 September 7, 2026 17:26

@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 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:430Identity{"Pubky session must be grant-backed"}runtime/mod.rs:355 propagates with ? and no re-wrap → sdk.rs:182-188errors.rs:40-44 → Kotlin PaykitException.Identity.contextcanDeferStaleSession: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 clearSessionAccessclearPubkySessionCredentials, 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.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

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.

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

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_SESSION and PUBKY_SECRET_KEY both deleted.
  • 2. Pubky profile → network disabled → delete/sign out: the app showed Unable to Delete Profile with 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 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.

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.

@ovitrif
ovitrif dismissed their stale review September 8, 2026 10:30

addresed

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

Manually reviewed
✅ Tested signin and signet flows on emulator

@jvsena42
jvsena42 enabled auto-merge September 8, 2026 11:05
@jvsena42
jvsena42 merged commit a7db7bc into master Sep 8, 2026
18 checks passed
@jvsena42
jvsena42 deleted the codex/paykit-rc50-auth branch September 8, 2026 11:08
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