refactor: integrate paykit sdk#606
Conversation
Greptile SummaryThis PR replaces Bitkit's hand-rolled Paykit payment plumbing (noise-encrypted links, manual homeserver reads/writes, bespoke snapshot serialisation) with the native Paykit SDK (
Confidence Score: 3/5Safe to land after fixing the BackupService error handling gap; the rest of the SDK integration is well-structured. The restore path is missing a local error guard for the non-nil SDK backup state case. A corrupted or version-mismatched blob causes restoreBackup to throw, which propagates to the outer catch and silently skips the blocktank restore and PIN-reset steps. Because this touches the wallet restore flow — the path users depend on after loss or device migration — a failure here can leave the wallet in a partially-restored state. The rest of the SDK integration (actor isolation, atomic keychain writes with revision checking, auth cancellation via requestID, public payment fallback guarded against CancellationError) is well-structured. Bitkit/Services/BackupService.swift lines 249-261 (missing local do/catch for non-nil SDK backup restore). Bitkit/Managers/ContactsManager.swift updateContact (richer profile fields now depend on MetadataBackupV1 surviving restore).
|
| Filename | Overview |
|---|---|
| Bitkit/Services/BackupService.swift | Restore flow deferred Paykit SDK restore after wallet category; non-nil backup string throws are not caught locally, aborting blocktank and PIN-reset steps on a bad SDK blob. |
| Bitkit/Services/PubkyService.swift | Major refactor: replaced PaykitFFI free-function calls with the new PaykitSdkService actor. Introduces PaykitSdkStateBlobStore (atomic keychain persistence with revision checking), PaykitSdkSessionProvider, and PaykitSdkOperationLock. Auth-cancel race is handled correctly via requestID comparison inside completeAuth. |
| Bitkit/Services/PrivatePaykitService+Payments.swift | Private/public payment resolution simplified: delegates to SDK prepareAndResolveContactPayment; cached endpoints and stale-lightning-hash eviction logic retained at app layer. Public fallback on SDK error is correctly guarded against CancellationError. |
| Bitkit/Services/PrivatePaykitService+Backup.swift | Backup now exports a single SDK blob string and restores via PaykitSdkService; the old per-contact link snapshot serialisation is removed. |
| Bitkit/Managers/ContactsManager.swift | Contact CRUD now goes through SDK contactRecords/saveContact/removeContact APIs. updateContact stores bio/image/links/tags as a local contactProfileOverride rather than on the homeserver; these are only recoverable via the MetadataBackupV1 category. |
| Bitkit/Services/PrivatePaykitService+Endpoints.swift | Endpoint publication simplified: buildLocalEndpoints now @mainactor to allow synchronous walletHasUsableChannels access; syncLocalEndpointPublicationLocked batches reservation updates for all contacts in one SDK call. |
| Bitkit/Services/PrivatePaykitService+Contacts.swift | Contact preparation delegates to syncLocalEndpointPublication; profile-recovery re-establishment logic removed (now handled by SDK). |
| Bitkit/Models/BackupPayloads.swift | WalletBackupV1 replaces per-contact link snapshots with a single optional paykitSdkBackupState string; MetadataBackupV1 gains pubkyContactProfileOverrides for local contact customisations. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI
participant PrivatePaykitService
participant PaykitSdkService
participant PaykitSdk
participant Keychain
Note over UI,Keychain: Payment preparation flow
UI->>PrivatePaykitService: beginSavedContactPayment(publicKey)
PrivatePaykitService->>PaykitSdkService: identityStatus()
PaykitSdkService->>PaykitSdk: identityStatus()
PaykitSdk-->>PaykitSdkService: IdentityStatus (privateLinkCapable)
PaykitSdkService-->>PrivatePaykitService: status
PrivatePaykitService->>PaykitSdkService: syncPrivatePaymentListsWithReservations(updates)
PaykitSdkService->>Keychain: saveStateBlobAtomically (revision-checked)
PaykitSdkService-->>PrivatePaykitService: PrivatePaymentListDeliveryReport
PrivatePaykitService->>PaykitSdkService: prepareAndResolveContactPayment(counterparty)
PaykitSdkService->>PaykitSdk: prepareAndResolveContactPayment(...)
PaykitSdk-->>PaykitSdkService: PreparedContactPayment
PaykitSdkService-->>PrivatePaykitService: resolution
alt private endpoints available
PrivatePaykitService-->>UI: .opened(paymentRequest)
else public endpoints available
PrivatePaykitService-->>UI: .opened(paymentRequest via public)
else no endpoints
PrivatePaykitService-->>UI: .noEndpoint
end
Note over UI,Keychain: Backup restore flow
UI->>BackupService: restore()
BackupService->>BackupService: performRestore(.wallet) sets pendingPaykitSdkBackupState
BackupService->>BackupService: performRestore(.metadata) restoreContactProfileOverrides
BackupService->>PrivatePaykitService: restoreBackup(pendingPaykitSdkBackupState)
PrivatePaykitService->>PaykitSdkService: restoreBackupState(blob)
PaykitSdkService->>Keychain: saveStateBlobAtomically
PaykitSdkService-->>PrivatePaykitService: ok / throws
%%{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 UI
participant PrivatePaykitService
participant PaykitSdkService
participant PaykitSdk
participant Keychain
Note over UI,Keychain: Payment preparation flow
UI->>PrivatePaykitService: beginSavedContactPayment(publicKey)
PrivatePaykitService->>PaykitSdkService: identityStatus()
PaykitSdkService->>PaykitSdk: identityStatus()
PaykitSdk-->>PaykitSdkService: IdentityStatus (privateLinkCapable)
PaykitSdkService-->>PrivatePaykitService: status
PrivatePaykitService->>PaykitSdkService: syncPrivatePaymentListsWithReservations(updates)
PaykitSdkService->>Keychain: saveStateBlobAtomically (revision-checked)
PaykitSdkService-->>PrivatePaykitService: PrivatePaymentListDeliveryReport
PrivatePaykitService->>PaykitSdkService: prepareAndResolveContactPayment(counterparty)
PaykitSdkService->>PaykitSdk: prepareAndResolveContactPayment(...)
PaykitSdk-->>PaykitSdkService: PreparedContactPayment
PaykitSdkService-->>PrivatePaykitService: resolution
alt private endpoints available
PrivatePaykitService-->>UI: .opened(paymentRequest)
else public endpoints available
PrivatePaykitService-->>UI: .opened(paymentRequest via public)
else no endpoints
PrivatePaykitService-->>UI: .noEndpoint
end
Note over UI,Keychain: Backup restore flow
UI->>BackupService: restore()
BackupService->>BackupService: performRestore(.wallet) sets pendingPaykitSdkBackupState
BackupService->>BackupService: performRestore(.metadata) restoreContactProfileOverrides
BackupService->>PrivatePaykitService: restoreBackup(pendingPaykitSdkBackupState)
PrivatePaykitService->>PaykitSdkService: restoreBackupState(blob)
PaykitSdkService->>Keychain: saveStateBlobAtomically
PaykitSdkService-->>PrivatePaykitService: ok / throws
Comments Outside Diff (1)
-
Bitkit/Managers/ContactsManager.swift, line 355-361 (link)Richer contact profile data (bio, image, links, tags) is now only stored locally
Previously,
updateContactserialised the fullPubkyProfileData(name, bio, image URL, links, tags) to the homeserver. Now it callsPubkyService.saveContactwith onlylabel: nameand stores the remaining fields in a localcontactProfileOverride. The override is backed up viaMetadataBackupV1.pubkyContactProfileOverrides, but if the metadata backup is absent or corrupted while the wallet backup is intact, all bio/image/links/tag customisations are silently lost on restore. This is a silent data-availability regression compared to the old homeserver-backed approach.
Reviews (1): Last reviewed commit: "chore: polish paykit cleanup" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if didRestoreWalletBackup { | ||
| if pendingPaykitSdkBackupState != nil { | ||
| try await PrivatePaykitService.shared.restoreBackup(pendingPaykitSdkBackupState) | ||
| await PrivatePaykitAddressReservationStore.shared.reconcileReservedIndexesWithLdk() | ||
| } else { | ||
| do { | ||
| try await PrivatePaykitService.shared.restoreBackup(nil) | ||
| await PrivatePaykitAddressReservationStore.shared.reconcileReservedIndexesWithLdk() | ||
| } catch { | ||
| Logger.warn("Failed to clear missing Paykit SDK backup state: \(error)", context: "BackupService") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When pendingPaykitSdkBackupState is non-nil, a throw from PrivatePaykitService.shared.restoreBackup (e.g. a corrupted or version-incompatible SDK backup string) propagates straight to the outer catch, which logs a warning and returns — skipping the blocktank restore and the PIN-reset that follows. In contrast, the nil branch wraps its call in its own do/catch and only warns, letting the restore continue. The old restoreBackup was non-throwing so this edge case didn't exist before. Wrap the non-nil call identically to the nil case so that a bad SDK blob degrades gracefully rather than aborting the full wallet restore.
| if didRestoreWalletBackup { | |
| if pendingPaykitSdkBackupState != nil { | |
| try await PrivatePaykitService.shared.restoreBackup(pendingPaykitSdkBackupState) | |
| await PrivatePaykitAddressReservationStore.shared.reconcileReservedIndexesWithLdk() | |
| } else { | |
| do { | |
| try await PrivatePaykitService.shared.restoreBackup(nil) | |
| await PrivatePaykitAddressReservationStore.shared.reconcileReservedIndexesWithLdk() | |
| } catch { | |
| Logger.warn("Failed to clear missing Paykit SDK backup state: \(error)", context: "BackupService") | |
| } | |
| } | |
| } | |
| if didRestoreWalletBackup { | |
| do { | |
| try await PrivatePaykitService.shared.restoreBackup(pendingPaykitSdkBackupState) | |
| await PrivatePaykitAddressReservationStore.shared.reconcileReservedIndexesWithLdk() | |
| } catch { | |
| Logger.warn("Failed to restore Paykit SDK backup state: \(error)", context: "BackupService") | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 703b592. The SDK backup restore is now wrapped locally, so a bad Paykit SDK backup only logs a warning and the rest of the wallet restore keeps going.
![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: fbaa310479
ℹ️ 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".
| let secretKeyHex = try deriveLocalSecretKeyFromWalletSeed(loadKeychainString: loadKeychainString) | ||
| try persistKeychainString(.pubkySecretKey, secretKeyHex) | ||
| try? deleteKeychainValue(.paykitSession) | ||
| _ = try await signInWithSecretKey(secretKeyHex) |
There was a problem hiding this comment.
When restoring a .localSeed Pubky backup while offline or while the homeserver/sign-in endpoint is unavailable, this call throws before anything writes the derived secretKeyHex back to Keychain. restoreSessionBackupState has just cleared session access, and BackupService only logs the restore error, so the restored wallet is left without .pubkySecretKey or .paykitSession and cannot retry Pubky restoration on the next launch. Persist the derived local secret before attempting the network sign-in, or otherwise retain it for retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 703b592. We now persist the derived local secret before trying the network sign-in, so a failed sign-in can be retried on the next launch.
| if cleanPrivatePaykitEndpoints { | ||
| try await Self.removePrivatePaykitEndpoints(context: "PubkyProfileManager.signOut") | ||
| } |
There was a problem hiding this comment.
If the app is offline or the SDK cannot clear a private payment list, removePrivatePaykitEndpoints marks cleanup pending and throws; because this try runs before PubkyService.signOut() and clearLocalState(), the Profile sign-out flow aborts and leaves the user signed in even though cleanup can be retried later. This affects users with private contact payments enabled during transient cleanup failures, so local sign-out should continue after recording the pending cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think we should keep this blocking for normal sign-out. If private cleanup fails and we clear the Pubky session anyway, we lose the credentials needed to retry removing those private lists. The pending flag only helps while the session is still available. For wipe we still have the best-effort emergency path, but for normal sign-out/delete I would rather fail visibly than leave private endpoints published with no retry path.
|
For the outside-diff contact override note: this is intentional with the SDK path. Edited contact details are app-local now and backed up in metadata. We do not want to publish bio/image/links/tags back to the homeserver. If metadata backup is missing or corrupt, those local customizations are lost like other metadata, but wallet backup should not own that app-local contact data. |
1 participant
Description
This PR:
v0.1.0-rc21Swift package release instead of local bindings.Linked Issues/Tasks
N/A
Screenshot / Video
Draft PR; proof videos can be attached after review of the SDK integration diff.
QA Notes
Manual Tests
Automated Checks
xcodebuild ... buildpassed for the E2E simulator build against Paykitv0.1.0-rc21.git diff --check origin/master...HEADpassed.StateLockerTestscompile issue before Paykit tests run; the app build is clean.