feat: support incoming paykit requests - #1098
Conversation
Greptile SummaryThis PR adds support for incoming Paykit payment requests. The main changes are:
Confidence Score: 4/5The legacy backup restore path needs a compatibility fix before merging.
app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt | Adds private resolution and consumed-version persistence, but the new backup envelope breaks restoration of legacy backups. |
| app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt | Adds synchronized request intake, filtering, expiration, and acceptance. |
| app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt | Connects request polling and presentation to the existing payment flow. |
| app/src/main/java/to/bitkit/services/PaykitSdkService.kt | Adapts the service to separate public and private Paykit APIs and advertises request support. |
Sequence Diagram
sequenceDiagram
participant Contact as Paykit Contact
participant SDK as Paykit SDK
participant Requests as Payment Request Repo
participant App as App View Model
participant Private as Private Paykit Repo
participant Wallet as Send Flow
Contact->>SDK: Publish payment request
Requests->>SDK: Receive and query requests
Requests-->>App: Emit actionable request
App->>Private: Resolve private payment details
Private->>SDK: Resolve after consumed version
SDK-->>Private: Endpoint and list version
Private-->>App: Open payment details
App-->>Wallet: Show confirmation
Wallet->>App: User approves
App->>Requests: Accept request
App->>Private: Persist consumed version
App->>Wallet: Submit payment
Reviews (1): Last reviewed commit: "feat: support incoming paykit requests" | Re-trigger Greptile
| paykitSdkService.clearState() | ||
| } else { | ||
| paykitSdkService.restoreBackupState(backup) | ||
| val decoded = json.decodeFromString<PrivatePaykitBackup>(backup) |
There was a problem hiding this comment.
Previous releases stored exportBackupState() as a raw string, but this line now decodes every non-null backup as PrivatePaykitBackup JSON. Restoring a backup created before this change therefore fails during decoding and never calls restoreBackupState(); the restore path needs to recognize and migrate the legacy format.
There was a problem hiding this comment.
No code change here: this raw backup format only exists in the unshipped parent Paykit work, so there are no production backups to migrate. Per the pre-release scope of this stack, we are intentionally not adding migration or backward-compatibility code.
5050c1e to
edb688f
Compare
eda7df2 to
4457134
Compare
57626b7 to
1ceb420
Compare
4457134 to
ece10d4
Compare
1e944d0 to
75610d7
Compare
cf14578 to
44d58d2
Compare
|
Restacked onto the current #1084 head after its force-push. The request/review-fix commits are signed; the retry fix previously referenced as The current |
ec8ee8c to
41ddb6b
Compare
44d58d2 to
796bc2b
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed the changes since my last pass. All 8 comments I raised are addressed — resolved those threads. Verified locally on 796bc2b1: the Paykit unit tests (PaykitPaymentRequestRepoTest, PrivatePaykitRepoTest, PublicPaykitRepoTest, AppViewModelSendFlowTest, PaykitSdkServiceTest, ContactPaymentSettingsRepoTest) all pass and detekt is clean.
A few new findings below. Only the first one blocks.
One extra nit that has no diff line to attach to: PrivatePaykitRepoTest.kt:32 — import org.mockito.kotlin.anyOrNull is now unused (base had 4 usages, this branch has 0). detekt reports it as NoUnusedImports, but ignoreFailures = true in app/build.gradle.kts keeps CI green.
| private suspend fun hasPrivatePaymentAccessForCurrentProfile(): Boolean { | ||
| pubkyService.currentPublicKey() ?: return false | ||
| return paykitSdkService.hasPrivatePaymentAccess() | ||
| } |
There was a problem hiding this comment.
This dropped the runSuspendCatching {}.getOrDefault(false) that wrapped the helper on master, and that turns a settings toggle into a crash path.
pubkyService.currentPublicKey() reaches PaykitSdkService.currentPublicKey() → handle() + handle.initialize(), which throws PaykitException. The public hasPrivatePaymentAccess() (line 121) is called by ContactPaymentSettingsRepo.enable() outside its runSuspendCatching:
val canUsePrivateContactPayments = privatePaykitRepo.hasPrivatePaymentAccess() // throws
return runSuspendCatching { ... }So the throw escapes setEnabled()'s Result<Unit> contract and lands in SettingsViewModel.setContactPaymentsEnabled's bare viewModelScope.launch — uncaught coroutine exception, and _isUpdatingContactPayments stays stuck at true. PayContactsViewModel.continueToProfile has a finally so it only loses the toast, but the exception still escapes.
The other callers (canPublishPrivateEndpoints, prepareRelevantPrivateLinksIfAvailable) are all inside a runSuspendCatching, so hasPrivatePaymentAccess() is the single exposure — but it's user-reachable. Please restore the guard:
private suspend fun hasPrivatePaymentAccessForCurrentProfile(): Boolean = runSuspendCatching {
pubkyService.currentPublicKey() ?: return@runSuspendCatching false
paykitSdkService.hasPrivatePaymentAccess()
}.getOrDefault(false)| PublicPaykitPaymentResult.WaitingForUpdatedPaymentList -> | ||
| showPayError(R.string.slashtags__error_pay_empty_msg) |
There was a problem hiding this comment.
slashtags__error_pay_empty_msg means "no payment endpoint found", but the actual state here is "waiting for the contact to publish an updated payment list" — a transient, retryable condition the user could act on by waiting. Worth a dedicated string.
The same branch is duplicated verbatim in SendContactSelectViewModel and AddContactViewModel; folding the PublicPaykitPaymentResult → message mapping into one shared helper would keep the three in sync.
| if (!validateAndAcceptIncomingPaymentRequest(contactPaymentContext)) return | ||
|
|
||
| consumePrivatePaymentListIfNeeded(contactPaymentContext).onFailure { |
There was a problem hiding this comment.
Separate from my earlier consume-before-send comment (which you answered — the rc39 contract makes that unavoidable, understood): the concern here is the window between these two calls.
validateAndAcceptIncomingPaymentRequest accepts the request on the Paykit side and removes it from pendingRequests. If consumePrivatePaymentListIfNeeded then fails (e.g. PaymentListAlreadyConsumed), we've accepted a request, made no payment, and there's no path left to retry or decline it — it's simply gone.
Consuming first (or accepting last) would close that gap. incoming payment request is accepted before its private list is consumed pins the current order, so I assume this is deliberate — just want to confirm it's the order you want.
| private val PAYKIT_PAYMENT_REQUEST_PRESENTATION_RETRY_DELAYS = listOf( | ||
| 30.seconds, | ||
| 60.seconds, | ||
| 120.seconds, | ||
| 300.seconds, | ||
| ) |
There was a problem hiding this comment.
The retry schedule saturates at 300s and never gives up. A request with expiresAt == null that never resolves will keep issuing beginPaymentRequest network calls every 5 minutes for as long as the app is foregrounded.
A max-attempt cap — after which the request falls back to the normal poll cycle — would bound it.
| if (result !is PublicPaykitPaymentResult.Opened || !paykitPaymentRequestRepo.isPending(request)) { | ||
| if (paykitPaymentRequestRepo.isPending(request)) deferPaymentRequestPresentation(request) | ||
| return false |
There was a problem hiding this comment.
Nit: paykitPaymentRequestRepo.isPending(request) is evaluated twice on adjacent lines — worth hoisting into a local.
Also, the Boolean return means two different things: true at line 689 is "abort, a sheet opened" while true at line 703 is "presented". The caller treats both as return, so behaviour is right, but the name openIncomingPaymentRequestIfAvailable reads like it returns "did I open it". A small sealed result, or a rename to something like presentIncomingPaymentRequestOrStop, would make the loop easier to follow.
| fun acceptsLightningInvoiceAmountMsats(amountMsats: ULong?): Boolean = | ||
| amountMsats == null || amountSats <= ULong.MAX_VALUE / 1000uL && amountMsats == amountSats * 1000uL |
There was a problem hiding this comment.
Nit: the amountSats <= ULong.MAX_VALUE / 1000uL guard is already enforced at construction by toSats() (takeIf { it <= ULong.MAX_VALUE / 1000uL }), so it's dead weight here. The unparenthesized ||/&& mix also reads ambiguously even though the precedence is correct — parentheses around the && clause would help.
Description
This PR builds on #1084 to support incoming Paykit payment requests:
0.1.0-rc39and uses its separate public and private payment resolution APIs.Payment proofs and receipts remain out of scope.
References:
Preview
N/A — the existing payment UI is reused, and no media is attached.
QA Notes
Manual Tests
Automated Checks
PaykitPaymentRequestRepoTest.kt: request mapping, expiry, lifecycle, and acceptance.PrivatePaykitRepoTest.kt: separate resolution, consumed-list persistence, and prevention of private payment-detail reuse.AppViewModelSendFlowTest.kt: presentation, polling, approval order, expiry, and duplicate or pending consumption.