refactor: integrate paykit sdk#1040
Conversation
Greptile SummaryThis PR replaces Bitkit's hand-rolled Paykit plumbing with the published
Confidence Score: 4/5The core payment flow and session lifecycle look structurally sound; the main risks are edge cases in the new blocking-inside-synchronized SDK state store and empty contact names when the SDK returns a profile with no display data. The architectural shift is large but well-scoped: the SDK takes over state management that was previously hand-coded, and the delegation boundary is clear. The new PaykitSdkStateBlobStore uses runBlocking(ioDispatcher) inside a synchronized block — not a deadlock under normal load but a thread-starvation risk under sustained IO pressure. PaykitSdkSessionProvider.clearSessionAccess() uses a bare runBlocking {} without a dispatcher, which could misbehave if called from an unusual thread context. The backup restore path for legacy (pre-SDK) backups silently swallows SDK state-clearing errors. The contact-name-empty edge case is a UI regression when the SDK's profile record lacks both displayName and decodable extraJson. None of these are showstoppers, but the blocking-coroutine nesting deserves attention before shipping to broad audiences. PaykitSdkService.kt (the PaykitSdkStateBlobStore and PaykitSdkSessionProvider inner classes), BackupRepo.kt (legacy restore path around line 619), and PubkyRepo.kt (contactProfile method).
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/services/PaykitSdkService.kt | New singleton service wrapping the Paykit SDK; mixes runBlocking inside a synchronized block (saveStateBlobAtomically) and has a bare runBlocking in PaykitSdkSessionProvider.clearSessionAccess(). |
| app/src/main/java/to/bitkit/data/keychain/Keychain.kt | Adds a new synchronous upsert(ByteArray) method using runBlocking(this.coroutineContext); consistent with the existing snapshot pattern but called from a synchronized block, risking thread starvation under IO saturation. |
| app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt | Substantially trimmed by delegating link/handshake/recovery state to the SDK; backup snapshot now delegates to PaykitSdkService.exportBackupState(); logic looks correct. |
| app/src/main/java/to/bitkit/repositories/PubkyRepo.kt | Delegates session/profile/contact operations to PaykitSdkService; introduces contactProfileOverrides in PubkyStore and snapshotContactProfileOverrides/restoreContactProfileOverrides for backup; contact name may be empty when paykitProfile has no displayName and no extraJson. |
| app/src/main/java/to/bitkit/repositories/BackupRepo.kt | Backup listeners refactored to observeBackupChanges helper; wallet restore silently swallows SDK state-clearing errors for legacy backups (null paykitSdkBackupState). |
| app/src/main/java/to/bitkit/services/PubkyService.kt | Thin wrapper now fully delegates to PaykitSdkService; straightforward and correct. |
| gradle/libs.versions.toml | Bumps paykit-android from rc8 to rc21; no other dependency changes. |
| app/src/main/java/to/bitkit/models/BackupPayloads.kt | Replaces PrivatePaykitContactLinkBackupV1 map with a single paykitSdkBackupState string and adds pubkyContactProfileOverrides; old backup fields removed with no migration path for existing contact-link data. |
| app/src/main/java/to/bitkit/models/PubkyProfile.kt | Adapts to SDK PubkyProfile/PaykitProfile types; fromPaykitProfile may produce an empty contact name if displayName and extraJson are both absent. |
| app/src/main/java/to/bitkit/usecases/WipeWalletUseCase.kt | Wipe sequence unchanged in substance; closeAndClear() now delegates SDK state clearing, then keychain.wipe() removes all persisted state. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App as App/UI
participant PPR as PrivatePaykitRepo
participant SDK as PaykitSdkService
participant PaykitSdk as PaykitSdk (native)
participant Keychain as Keychain
participant BR as BackupRepo
App->>PPR: prepareSavedContacts(publicKeys)
PPR->>SDK: ensureLinkWithPeer(counterparty)
SDK->>PaykitSdk: ensureLinkWithPeer()
PaykitSdk->>Keychain: saveStateBlobAtomically() [synchronized + runBlocking]
SDK->>BR: backupStateVersion++ (via withStateRevisionTracking)
PPR->>SDK: syncPrivatePaymentListsWithReservations(updates)
SDK->>PaykitSdk: syncPrivatePaymentListsWithReservationsAndProcessOutbound()
PaykitSdk->>Keychain: saveStateBlobAtomically()
SDK->>BR: backupStateVersion++
App->>PPR: beginSavedContactPayment(publicKey)
PPR->>SDK: prepareAndResolveContactPayment(counterparty)
SDK->>PaykitSdk: prepareAndResolveContactPayment()
PaykitSdk-->>SDK: ContactPaymentResolution
SDK-->>PPR: PaykitContactPaymentResolution
PPR-->>App: PublicPaykitPaymentResult
BR->>PPR: backupSnapshot()
PPR->>SDK: exportBackupState()
SDK->>PaykitSdk: exportBackupString()
PaykitSdk-->>SDK: String (opaque blob)
SDK-->>BR: paykitSdkBackupState
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App as App/UI
participant PPR as PrivatePaykitRepo
participant SDK as PaykitSdkService
participant PaykitSdk as PaykitSdk (native)
participant Keychain as Keychain
participant BR as BackupRepo
App->>PPR: prepareSavedContacts(publicKeys)
PPR->>SDK: ensureLinkWithPeer(counterparty)
SDK->>PaykitSdk: ensureLinkWithPeer()
PaykitSdk->>Keychain: saveStateBlobAtomically() [synchronized + runBlocking]
SDK->>BR: backupStateVersion++ (via withStateRevisionTracking)
PPR->>SDK: syncPrivatePaymentListsWithReservations(updates)
SDK->>PaykitSdk: syncPrivatePaymentListsWithReservationsAndProcessOutbound()
PaykitSdk->>Keychain: saveStateBlobAtomically()
SDK->>BR: backupStateVersion++
App->>PPR: beginSavedContactPayment(publicKey)
PPR->>SDK: prepareAndResolveContactPayment(counterparty)
SDK->>PaykitSdk: prepareAndResolveContactPayment()
PaykitSdk-->>SDK: ContactPaymentResolution
SDK-->>PPR: PaykitContactPaymentResolution
PPR-->>App: PublicPaykitPaymentResult
BR->>PPR: backupSnapshot()
PPR->>SDK: exportBackupState()
SDK->>PaykitSdk: exportBackupString()
PaykitSdk-->>SDK: String (opaque blob)
SDK-->>BR: paykitSdkBackupState
Comments Outside Diff (1)
-
app/src/main/java/to/bitkit/repositories/BackupRepo.kt, line 619-628 (link)SDK state-clear failure silently ignored during legacy backup restore
When
paykitSdkBackupStateisnull(restoring a backup created before this PR),privateRepo.restoreBackup(null)is called and any failure is only logged viaonFailure { Logger.warn(...) }— execution continues regardless. InsiderestoreBackup(null),paykitSdkService.clearState()deletes thePAYKIT_SDK_STATEkeychain entry. If this deletion fails (e.g., keystore error), the stale SDK state persists while the rest of the wallet is restored from the new backup, leaving contact-link and session state out of sync with the freshly restored wallet. The successful path (paykitSdkBackupState != null) uses.getOrThrow()— the legacy path should follow the same convention or at least propagate the failure to surface the inconsistency.
Reviews (1): Last reviewed commit: "fix: preserve paykit cancellation" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| override fun saveStateBlobAtomically( | ||
| blob: SdkStateBlob, | ||
| expectedRevision: String?, | ||
| ): String = synchronized(lock) { | ||
| val currentRevision = keychain.load(Keychain.Key.PAYKIT_SDK_STATE.name) | ||
| ?.let { decodeSdkStateBlobSnapshot(it).revision } | ||
| if (currentRevision != expectedRevision) { | ||
| throw PaykitException.Storage( | ||
| code = "revision_conflict", | ||
| context = "SDK state revision changed", | ||
| ) | ||
| } | ||
|
|
||
| val nextRevision = UUID.randomUUID().toString() | ||
| val snapshot = SdkStateBlobSnapshot(blob = blob, revision = nextRevision) | ||
| keychain.upsert(Keychain.Key.PAYKIT_SDK_STATE.name, encodeSdkStateBlobSnapshot(snapshot)) | ||
| nextRevision | ||
| } |
There was a problem hiding this comment.
runBlocking inside synchronized may exhaust IO threads
saveStateBlobAtomically holds synchronized(lock) while calling keychain.upsert(), which in turn calls runBlocking(this.coroutineContext) (IoDispatcher). Because the thread holding the JVM monitor is then blocked waiting for a second IoDispatcher thread to complete the DataStore write, you effectively need at least two free IO threads for every concurrent SDK state save. Kotlin's Dispatchers.IO is elastic (default ceiling 64), so in normal operation this is fine, but under IO saturation (many concurrent LDK or network operations) you risk thread starvation and a deadlock between all threads waiting on each other via runBlocking. The equivalent upsertString suspend function avoids this pattern entirely — the same approach (a suspend overload or a plain keychain.edit call shared with the existing upsertString implementation) would eliminate the blocking-inside-synchronized nesting.
There was a problem hiding this comment.
Agree this is worth cleaning up. I will move the SDK blob write away from blocking DataStore work inside the monitor, or otherwise keep the synchronized section limited to revision checks.
There was a problem hiding this comment.
Fixed in 82bb55cf6. The SDK state store now uses a dedicated synchronous Keychain access path for SDK sync callbacks instead of calling the suspend Keychain APIs through the service coroutine context inside the store callback. The state blob transaction is still serialized because the SDK storage callback is synchronous and does the revision check/write as one boundary.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8202a59774
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return try { | ||
| runSuspendCatching { | ||
| withContext(ioDispatcher) { | ||
| pubkyService.completeAuth() |
There was a problem hiding this comment.
When the Ring auth request completes after the user has already canceled or a newer attempt has replaced it, pubkyService.completeAuth() now activates and persists the Paykit session inside PaykitSdkService before this ensureAuthAttemptActive check runs. The failure path only restores the UI state, so the app can appear to have canceled auth while the new session remains in keychain and is restored on the next launch; clear the SDK/session if the attempt is no longer active after completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, will fix. If auth is canceled or superseded after completeAuth activates the SDK session, we should clear that session before restoring the UI state.
There was a problem hiding this comment.
Fixed in 82bb55cf6 and mirrored on iOS. If SDK auth completion already activated a session but the app flow was canceled or superseded, Bitkit clears that completed session before restoring UI state. Added PubkyRepoTest coverage on Android and PubkyProfileManagerTests coverage on iOS.
| isSetup.await() | ||
| PubkySessionBootstrap().approveAuth( | ||
| authUrl = authUrl, | ||
| expectedCapabilities = requiredCapabilities(), |
There was a problem hiding this comment.
For Pubky auth approval links from other apps, the approval sheet parses and displays the caps from the incoming URL, but this call validates the request against Bitkit's own required capabilities instead. Any auth URL whose requested capabilities differ from Bitkit's namespace set (for example a pubky.app/paykit request) is approved with the wrong expectation and will fail validation; pass the capabilities from the parsed auth URL here instead of requiredCapabilities().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agree. This should use the capabilities from the incoming auth URL, and I will make the same fix on iOS for parity.
There was a problem hiding this comment.
Fixed in 82bb55cf6 and mirrored on iOS. Auth approval now parses the incoming auth URL through the SDK and passes that requested capabilities string into approval. Android also starts the sheet in Loading and reparses in confirmAuthorize if the user confirms before load finishes.
|
For the legacy backup migration note: this is intentional for this PR. The old private Paykit link backup format never shipped, so there is no production data to migrate. Treating it as if it never existed keeps the restore path simpler. |
ovitrif
left a comment
ovitrif
left a comment
There was a problem hiding this comment.
Left one inline comment.
| val homegate = fetchHomegateSignupCode() | ||
|
|
||
| val session = runCatching { | ||
| runSuspendCatching { |
There was a problem hiding this comment.
I think this path still needs to bump pubkyRepo.backupStateVersion after the sign-up/sign-in state is persisted. The SDK emits its own backup version here, but BackupRepo maps that to WALLET; the Pubky session snapshot is only included in METADATA when pubkyRepo.backupStateVersion changes. Without that, a newly created identity can back up SDK wallet state without backing up the Pubky session needed to restore it.
There was a problem hiding this comment.
Rechecked this one and no extra code change is needed. createIdentity already calls notifyBackupStateChanged() after sign-up/sign-in and profile state are persisted, so the metadata Pubky session snapshot is queued separately from the SDK wallet backup-state version. Restore paths notify too.
|
That is not necessarily due to this change, because I saw it on other PR also - however e2e tests here failed partially because of this. The failure is intermittent and most of the time tests pass after re-runs. To reproduce:
Result after hitting "Continue" on the following screen: Attaching logs from e2e run where this happened: |
…tive-integration # Conflicts: # gradle/libs.versions.toml
|
Fixed now in Root cause was Android public Paykit publishing only refreshed the reusable on-chain address if the cached address was already reserved/unavailable. In the delete profile -> recreate profile flow, Lightning receive could be unavailable and the cached reusable on-chain address could still be blank, so endpoint sync concluded there were no supported endpoints and showed the toast. I changed public Paykit endpoint sync to ensure a reusable on-chain address exists before deciding there is no publishable endpoint, and added regression coverage for the blank-address case. Also merged latest Checked:
GitHub now reports the PR as mergeable. |
|
Manual regression — Paykit / contact paymentsEnvironment: regtest, staging Test setup
Session 1 — fresh profiles (smoke)
Session 1 looked good for basic contact + payment flows cross-platform. Private Paykit in session 1: Incoming activity showing “Received from [contact]” indicates the receive path worked — that label is only set when the payment matches a private Paykit invoice/address (not a generic public profile invoice). There are no private Paykit link errors in session 1 logs on either platform. Send-side logs showing Session 2 — profile delete, re-create, re-add contacts, second delete blocked
Delete profile: Screen.Recording.2026-07-01.at.15.18.17.movRegression — private Paykit broken after profile resetSession 1: Private Paykit appears to work (receive-side “Received from contact” + no link errors). Later in the same session, a second profile delete also failed on both platforms — private Paykit cleanup runs before delete and throws Private Paykit errors (identical on both platforms)Every private Paykit attempt ( First failures appear immediately after profile re-create (~12:23 iOS, ~12:26 Android on contact re-add). Contact payments fall back to publicPayments use a public BIP21 unified invoice from the contact’s published profile — not an encrypted private payment list:
Second profile delete blockedProfile delete runs private Paykit endpoint cleanup first. With private Paykit already broken, cleanup throws Android ( iOS ( Profile reset sequence (both sides)
Likely cause: local Paykit SDK state is wiped on delete/re-create, but encrypted-link handshake state is inconsistent across peers. SDK reports recovery is required; the app logs warnings, skips private publish, and resolves contact payments via public endpoints (intentional fallback). Useful grep patterns: Verdict
Not approving on “private contact payments survive profile delete/re-add.” Session 1 private Paykit looks fine; session 2 regresses on private Paykit recovery and blocks a second profile delete. |
|
Fixed in Main thing is we now use Paykit I also fixed the related app-side edges:
Public fallback while private recovery/link work is unavailable is still intentional so contact payments can still complete. Ring is still public-only for now; this fixes the crash path, not full Ring private payments support. |
| updates = preparation.updates, | ||
| clearUnlistedLinkedPeers = false, | ||
| ) | ||
| val firstError = preparation.firstError ?: applyPrivatePaymentListDeliveryReport(report, reason) |
There was a problem hiding this comment.
applyPrivatePaymentListDeliveryReport(report, reason) is skipped for the whole batch whenever any single contact fails to prepare endpoints.
val firstError = preparation.firstError ?: applyPrivatePaymentListDeliveryReport(report, reason) short-circuits: when preparation.firstError != null, the report is never applied. The early return at line 489 only fires when preparation.updates.isEmpty(), so a partial-failure batch (one contact with empty endpoints -> firstError = PrivateUnavailable, another contact succeeds -> updates non-empty) still runs the sync at line 496 and delivers the successful contact's endpoints.
Trigger: refreshKnownSavedContactEndpoints publishes for all known contacts at once, so one un-publishable contact (e.g. Lightning can't receive and on-chain disabled) suppresses report application for every contact in the batch. The successful contacts then never get hasPublishedPrivatePaymentList=true, report.cleared cache cleanup, updateDeletedContactCleanupPending(pk, false), the failedToQueue/failedToDeliver warnings, or persistState(markWalletBackup=true), so publish state isn't persisted and the wallet backup version isn't bumped.
Fix: evaluate the report unconditionally, e.g. val deliveryError = applyPrivatePaymentListDeliveryReport(report, reason) then val firstError = preparation.firstError ?: deliveryError.
There was a problem hiding this comment.
Fixed in 14dd28d1a. The delivery report is applied before choosing the first error now, so partial publishes still update cache/cleanup state and bump backup state. Added PrivatePaykitRepoTest coverage for a mixed batch.
|
|
||
| private suspend fun persistState( | ||
| markWalletBackup: Boolean = false, | ||
| preserveCleanupMarkers: Boolean = true, | ||
| ) { | ||
| stateStore.persistState(markWalletBackup, ::notifyBackupStateChanged, preserveCleanupMarkers) | ||
| val currentState = state ?: PrivatePaykitState() | ||
| val stored = cacheStore.data.first() |
There was a problem hiding this comment.
persistState reads the cleanup markers from a snapshot taken outside the cacheStore.update {} transform, dropping the atomicity the previous PrivatePaykitStateStore.persistState had and allowing concurrently-set markers to be lost.
Line 1064 reads val stored = cacheStore.data.first(), then the cacheStore.update { ... } at 1065 ignores its transform input and writes cleanupPending/deletedContactCleanupPendingPublicKeys computed from that pre-read stored. All repo methods run on serializedDispatcher = ioDispatcher.limitedParallelism(1), which serializes CPU work but still lets coroutines interleave at suspension points.
Trigger: coroutine A calls updateContactSharingCleanupPending(true) (which correctly mutates via it.copy(...)) while coroutine B is inside persistState between its data.first() (1064) and its update {} (1065). B writes from the stale stored and overwrites A's marker back to false, silently dropping it (same for deletedContactCleanupPendingPublicKeys). Those markers drive removePublishedEndpoints for removed/disabled contacts, so losing them can leave private payment endpoints published for contacts the user removed.
Fix: read the marker values from the transform's own input instead of the pre-read stored, e.g. cacheStore.update { current -> currentState.cacheState(cleanupPending = if (preserveCleanupMarkers) current.cleanupPending else false, ...) }, and drop the separate data.first() read.
There was a problem hiding this comment.
Fixed in 14dd28d1a. persistState now preserves cleanup markers from the cacheStore.update {} input instead of a pre-read snapshot, so concurrent marker updates cannot be overwritten. Added coverage for preserving cleanup markers while saving publication state.
| privatePaykitRepo.removePublishedEndpointsForCleanup(TAG) | ||
| val result = pubkyRepo.signOut() | ||
| if (result.isSuccess) { | ||
| privatePaykitRepo.closeAndClear() |
There was a problem hiding this comment.
privatePaykitRepo.closeAndClear() runs only in the result.isSuccess branch, but pubkyRepo.signOut() clears the pubky session state unconditionally (it force-signs-out and clears the _publicKey/_profile StateFlows even on failure, returning Result.failure only when both signOut() and forceSignOut() throw).
Trigger: the rare double-failure path where both server and local sign-out throw. The pubky session is already cleared, but closeAndClear() is skipped and no SignedOut effect is emitted, so the private-Paykit local state (cached contacts, address reservations, and the on-disk PAYKIT_SDK_STATE blob via paykitSdkService.clearState()) is left intact and inconsistent with the now-empty session. This is a regression from the pre-PR code, which called closeAndClear() unconditionally before signOut(). EditProfileViewModel.disconnectProfile (EditProfileViewModel.kt:233) has the identical pattern with the same signOut() call.
Fix: clear the local Paykit state whenever pubkyRepo.signOut() has cleared the session (move closeAndClear() out of the isSuccess guard) while still surfacing the error toast on failure. Apply the same to disconnectProfile.
There was a problem hiding this comment.
Fixed in 14dd28d1a in both sign-out paths. closeAndClear() now runs after pubkyRepo.signOut() regardless of success, while still showing the error toast if sign-out reports a failure. Added tests for ProfileViewModel and EditProfileViewModel.
| contactProfile(record.publicKey, record.label, record.profile, overrides) | ||
| }.onFailure { | ||
| Logger.warn("Failed to load contact '$contactKey'", it, context = TAG) | ||
| Logger.warn("Failed to load contact '${record.publicKey}'", it, context = TAG) |
There was a problem hiding this comment.
Full unredacted pubky public keys (the user's and their contacts') are written to the app log here and at several sibling sites, while the rest of the codebase (including PrivatePaykitRepo touched in this same PR) routes keys through PubkyPublicKeyFormat.redacted().
Line 685 logs Failed to load contact '${record.publicKey}'. Related sites: 615 (deleteAllContacts), 718, 741, 772, 783, 799 (fallback/add/update/remove/import), 1048 (resolveContactProfile retry), plus the info-level '$pk' auth log at 318. The Paykit SDK deliberately renders keys/labels as <redacted> in its own Debug impls, and app logs are routinely adb-exported per the debugging workflow, so this discloses the user's identity and their full contact social graph.
Fix: wrap these values with PubkyPublicKeyFormat.redacted(...) as PrivatePaykitRepo already does, e.g. Logger.warn("Failed to load contact '${PubkyPublicKeyFormat.redacted(record.publicKey)}'", it, context = TAG).
There was a problem hiding this comment.
Fixed in 14dd28d1a. PubkyRepo now redacts pubky keys in auth/contact/profile logs using PubkyPublicKeyFormat.redacted(...). I also covered the sibling key-bearing log sites, not only this one.
|
@ben-kaufman is pubky-ring option disabled? |
|
@piotr-iohk Added it back for now, but we will likely remove it, still waiting for final decision on that... |
5 participants
This PR:
com.synonym:paykit-android:0.1.0-rc23artifact.Description
Preview
N/A - SDK integration; no UI layout change.
QA Notes
Manual Tests
Automated Checks
./gradlew compileDevDebugKotlinpassed../gradlew testDevDebugUnitTestpassed../gradlew testDevDebugUnitTest --tests to.bitkit.repositories.PrivatePaykitRepoTestpassed../gradlew detektpassed.git diff --checkpassed.