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.
| override fun clearSessionAccess() { | ||
| runBlocking { | ||
| clearLiveSessionAccess() | ||
| keychain.delete(Keychain.Key.PAYKIT_SESSION.name) | ||
| keychain.delete(Keychain.Key.PUBKY_SECRET_KEY.name) | ||
| } | ||
| } |
There was a problem hiding this comment.
runBlocking {} in SDK session provider may block an unexpected thread
clearSessionAccess() is part of the SdkPubkySessionProvider interface and is called synchronously by the SDK. Using runBlocking {} without specifying a dispatcher runs on whatever thread the SDK happens to call this from — which is not guaranteed to be an IO thread. If the SDK ever calls this from a thin dispatcher (e.g., a single-threaded scheduler) and keychain.delete() itself queues work back on that dispatcher, this can deadlock. Additionally, if a CancellationException propagates out of the inner suspend calls, runBlocking will rethrow it, potentially crashing the SDK's internal cleanup logic. The other PaykitSdkSessionProvider methods (e.g., loadSessionAccess) avoid coroutines entirely; aligning clearSessionAccess to the same pattern, or dispatching explicitly to Dispatchers.IO, would be safer.
There was a problem hiding this comment.
Agree, will clean this up with an explicit IO path or synchronous helper so the SDK callback does not block an arbitrary thread.
| private suspend fun contactProfile( | ||
| publicKey: String, | ||
| label: String?, | ||
| paykitProfile: PaykitProfile?, | ||
| overrides: Map<String, PubkyProfileData>, | ||
| ): PubkyProfile { | ||
| val prefixedKey = publicKey.ensurePubkyPrefix() | ||
| overrides[prefixedKey]?.let { | ||
| return it.toPubkyProfile(prefixedKey) | ||
| } | ||
| paykitProfile?.let { | ||
| return PubkyProfile.fromPaykitProfile(prefixedKey, it) | ||
| } | ||
| resolveContactProfile(prefixedKey).getOrNull()?.let { | ||
| return it | ||
| } | ||
| return PubkyProfile.forDisplay( | ||
| publicKey = prefixedKey, | ||
| name = label, | ||
| imageUrl = null, | ||
| ) | ||
| } |
There was a problem hiding this comment.
PaykitProfile has no displayName and no valid extraJson
contactProfile() returns as soon as paykitProfile is non-null, calling PubkyProfile.fromPaykitProfile. Inside PubkyProfileData.fromPaykitProfile, the name is profile.displayName ?: extra?.name.orEmpty(). If displayName is null and extraJson is null or fails to parse, the resulting PubkyProfile.name is "". The ContactRecord.label (the name the local user assigned when saving the contact) is only used in the final PubkyProfile.forDisplay fallback, which is never reached when the SDK provides any PaykitProfile object at all. A contact with an empty name will appear as a blank entry in the contacts list. Adding ?: label as a final fallback to the name derivation in contactProfile() would cover this edge case.
There was a problem hiding this comment.
Good catch, will fix this and mirror it on iOS. If SDK profile display data is empty, we should fall back to the saved contact label instead of showing a blank name.
![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.
| 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.
| persistState() | ||
| invoice | ||
| private fun schedulePendingPrivateMessageDrainRetries(reason: String, publicKeys: List<String>) { | ||
| pendingMessageDrainRetryJob?.cancel() |
There was a problem hiding this comment.
When two private Paykit publications that need delayed draining happen before the first retry sequence finishes, this cancels the first job and replaces its peer list with only the newer batch. The remaining retries no longer advance links for the first peers, so queued private messages for those contacts can stay undelivered; keep per-peer retry jobs or merge the pending key set instead of canceling it outright.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. I will merge pending retry keys instead of replacing the current batch, and make the same fix on iOS.
|
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.
|
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: |
3 participants
This PR:
com.synonym:paykit-android:0.1.0-rc21artifact.Description
Preview
N/A - SDK integration; no UI layout change.
QA Notes
Manual Tests
Automated Checks
PaykitSdkServiceTest.ktcovers SDK config scoping for managed endpoints, local-only public contact sharing, and encrypted link recovery markers.PrivatePaykitRepoTest.ktcovers SDK private-list publication, reservation attribution, public fallback, cancellation propagation, endpoint discard behavior, and SDK backup state restore.PublicPaykitRepoTest.ktcovers SDK public endpoint sync/removal, publication failure handling, and SDK-resolved public payment endpoints.PubkyRepoTest.kt,BackupRepoTest.kt,EditProfileViewModelTest.kt,ProfileViewModelTest.kt,PaymentPreferenceViewModelTest.kt, andWipeWalletUseCaseTest.ktcover SDK-backed Pubky session/profile/contact state, backup/restore, cleanup, and wipe flows../gradlew compileDevDebugKotlin testDevDebugUnitTest detekt -Dmaven.repo.local=/private/tmp/paykit-empty-m2-rc21-verifyandgit diff --check origin/master...HEADpassed.