Repository navigation
fix: pause automatic payments after restore - #1779
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
[Critical impact] The PR appears safe to merge; no actionable defects were found. SummaryAutomatic allowance payments now stay paused after a wallet restore until the user resumes them.
No actionable issues were found. Tests and the device journey were not run during this review. Acknowledged scope: ben-kaufman states that the restore pause covers every identity, can affect allowances created later, leaves per-payment minimum checks to the SDK, and does not change the shared backup format. These choices were not reported as defects. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Restore wallet backup] --> B[Save local restore pause]
B --> C[Restore allowance and SDK state]
C --> D[Incoming request]
D --> E{Automatic payments paused?}
E -->|Yes| F[Ask for approval]
E -->|No| G{Checked allowance fits and SDK permits?}
G -->|Yes| H[Pay automatically]
G -->|No| F
I[User taps Resume] --> J[Check and accept ledger]
J --> K[Clear restore pause]
K --> D
Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile |
|
Placed in 3.2.0: a non-blocking fix found after the 3.1 cut. I filed #1781 to track it under the milestone and added |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Pauses automatic Allowance payments after a wallet restore until Resume, and keeps requests manual under Allowances whose terms carry limits Bitkit does not check. Reviewed 28a9601, full tier, static: the branch was not built here (Bitkit-track policy), reasoned from the code and CI.
What I checked, and 6 candidates I ruled out
Read: PaykitAllowanceExecutor.kt (classifyAboveLimit, autoPay, admitAndPay, recover), PaykitAllowanceRepo.kt, PaykitAllowanceStore.kt, PaykitAllowance.kt (fromRecord, PaykitAllowanceCapacity.fits), BackupRepo.kt (performFullRestoreFromLatestBackup, restoreWalletBackup), Keychain.kt
Call sites traced: autoPay (only PaykitAllowanceRepo), acceptPaymentRequestAutomatically / reserveAutomaticPayment (only admitAndPay), restoreWalletBackup (fresh and retained-payload branches)
CI: lint, detekt, build green at review time; build-and-test and compose-ui still running
Ruled out
- Pause bypass:
autoPaychecks the pause beforeadmitAndPay, andadmitAndPayis the only caller of the SDK accept and reserve calls;recoveronly settles existing attempts. - Restore entry without the pause: both the fresh WALLET restore and the retained
PAYKIT_PENDING_BACKUP_RESTOREpayload go throughrestoreWalletBackup, which writes the pause before any Paykit state. - Pause read failure:
isPausedAfterRestoredefaults to paused on a read error, fail closed. - Pause leaking past a wipe:
Keychain.wipeclears the whole DataStore, so the new key goes with it; it is not part of the backed-up allowance state. - False Limit Reached on unchecked terms:
classifyAboveLimitandadmitAndPaynotify only whenfittingis empty; a request that fits only an unchecked Allowance returns manual without a notice. - Resume accepting a stale ledger: from the restore notice,
isLedgerBehindOrThrowruns first, a behind ledger returns without clearing the pause, an unreadable one throws.
Merge confidence: 4/5, no findings, but CI's unit test job was still running and J1 has not been run on a device.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the review session's metadata does not report a reasoning setting)
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 28a960106: no HIGH, MEDIUM or LOW. Reviewed as a funds change: it gates automatic Paykit payments.
Checked and clean:
- Only one path pays automatically:
acceptPaymentRequestAutomaticallyandreserveAutomaticPaymentare called only fromadmitAndPay, andautoPaychecks the pause beforeensureReconciled.recover/recoverOpenAttemptsonly record outcomes. Subscription periods never auto-pay (billingPeriod == null). - Both restore entry points (a fresh WALLET payload, and the retained
PAYKIT_PENDING_BACKUP_RESTOREpayload) go throughrestoreWalletBackup, which writes the pause first.pubkyRepo.initialize()runs only after the full restore, so no identity session handles requests before the pause exists. IfpauseAfterRestore()throws, the WALLET category fails, and the pending marker keepshasPendingWalletRestore()true. - The pause has its own wallet-indexed keychain key. A read failure counts as paused. It is not in
backupSnapshot()orWalletBackupV1, andKeychain.wipe()clears it only together with the wallet. - Resume from RESTORED runs
isLedgerBehindOrThrowfirst: if the ledger is behind, the pause stays with the ledger notice; if the ledger is unreadable, Resume fails and the pause stays. ThenacceptCurrentLedger,resumeAfterRestore,recover,refresh. A failed keychain delete leaves a paused state that can be retried. The UI goes throughauthorizeThen(isGated = true). - After Resume,
fitsapplies the per-payment cap and the current monthly window at pay time, so a backlog cannot pay more than one period's cap. A request the pause handed over can auto-pay after Resume once the in-memory manual mark is dropped (relaunch). That matches the copy ("Requests ask for your approval until you resume") and the LEDGER_BEHIND behaviour on master, so it is not a finding. hasUncheckedLimitsflags lifetime limits, monthly counts, and non-monthly or extra periods.fitsrefuses an allowance with no cap. Limit Reached shows only when nothing fits.- The new key exists in all 16
values*/strings.xml. - iOS twin synonymdev/bitkit-ios#1214 has the same gate, the same fail-closed read, the same Resume order and the same notice gating.
perf: not relevant (one keychain read per covered request per poll).
Device gate (partial): 28a960106 — not run yet; allowance-paused-after-restore.xml pending on two linked emulators. The device result follows in a second review.
|
Device gate (partial):
|
jvsena42
left a comment
There was a problem hiding this comment.
Device gate result: no HIGH or MEDIUM, approving. One LOW inline, on the journey text. It can be fixed in this PR or moved to a follow-up issue, your choice; please reply on the thread saying which.
Device gate: 28a960106 — journeys: 2 run (set-and-accept.xml passed; allowance-paused-after-restore.xml steps 1-10 and 12-15 passed, step 11 drifted, step 17 partial); perf: Resume tap → notice gone 20 s, request sent → payer decision 52 s paused and 2 min 4 s after Resume, restore 117 s, no ANR or crash; Figma: not compared; l10n: 1 key, not checked on a small screen; iOS: synonymdev/bitkit-ios#1214 gate running.
Payer on Pixel_9, payee on Pixel_Tablet, staging regtest.
- The paused notice reads exactly "Automatic payments paused" / "This wallet was restored from a backup. Requests ask for your approval until you resume."
AllowancesResumeis present (set throughSecondaryButton(testTag = …)), and the allowance row stays Active. The pause key was written during the restore, at 11:19:11. - A $2 request while paused was kept manual, and no "Payment Executed" notification was posted:
11:24:33.459 Kept an incoming request manual: paused after a wallet restore (PaykitAllowanceExecutor.kt:627) 11:24:33.460 Decided 'MANUAL' for an incoming request covered by an allowance - After force-stop and relaunch the notice and Resume were still shown. For about 2 minutes after the relaunch, the tab showed the empty "Set up allowances" state while allowances loaded.
- Resume: tap at 11:33:00.077, pause key deleted at 11:33:15.169, notice gone by 11:33:19.87. There is no master baseline, since master has no restore pause. Paykit operations were queueing 3-5 s each at the time, with lock contention left over from a snapshot boot.
⚠️ Step 17 partial. After Resume, the next $2 request was admitted automatically:The payment could not route, because the payee has no inbound Lightning capacity, so "Auto-pay sent $2.00" never fired. Staging Blocktank returns 404 for11:37:03.327 Handed an allowance payment to the node (PaykitAllowanceExecutor.kt:897) 11:37:03.328 Decided 'STARTED' for an incoming request covered by an allowance 11:37:06.409 LDK event fired: {"type":"PaymentFailed", … ROUTE_NOT_FOUND}POST /channels,/cjitand/gift/order, so no inbound channel could be bought. The payee's own 300k Manual Setup channel has outbound capacity only. The PR's part (Resume turns automatic payments back on) is shown by the decision line. Delivery of the notification is not shown.
Skipped — covered by CI at 28a960106: unit tests, lint, local e2e (all green).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Re-approving at f99874a5d. The only change since my approval is the journey fix for my LOW: steps 10-11 now accept the confirm screen. PaymentRequestConfirm (SendConfirmScreen.kt:260) and PaymentRequestFor (PaymentRequestSummary.kt:62) both exist. No app code changed.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (not exposed in session metadata)
Closes #1781
Twin: synonymdev/bitkit-ios#1214
This PR:
Description
allowance-paused-after-restore.xmljourney.Out of Scope
repositories/PaykitAllowanceStore.kt: the restore pause covers every identity on the device and is cleared only by Resume; a wallet restored without any payer Allowance shows no notice, but an Allowance it creates later starts paused until Resume.repositories/PaykitAllowance.kt: a per-payment minimum in received terms is checked by the Paykit SDK for each request and is not treated as an unchecked limit.models/PaykitAllowanceStateBackup.kt: the shared backup format is unchanged; the pause is device-local and set by the restore itself.Design
N/A — no design available.
AllowancesScreen.ktis mapped totodoindocs/screens-map.md; the notice reuses the existing paused card with a new description.Preview
IMAGE_1
QA Notes
Setup
journeys/README.md.journeys/allowances/README.md: the payer has a Lightning spending balance above 50,000 sats with a usable channel, the payee has receiving capacity, and both have notification permission granted. Start J1 with the $5 a payment, $50 a month allowance fromset-and-accept.xmlActive. J1 wipes and restores the payer, so use a disposable wallet.Journeys
allowance-paused-after-restore.xml— after a restore, automatic payments stay paused across a relaunch with the restore notice, a request within the limits asks for approval, and Resume turns automatic payments back onManual Tests
Automated Checks
PaykitAllowanceStoreTest.kt— the restore pause survives a reload until Resume, is kept in its own keychain entry apart from the backed-up allowance state, never rewrites state this build cannot read, and counts as paused when it cannot be readPaykitAllowanceExecutorTest.kt— a restored wallet keeps requests manual until resumed, the local capacity check hands requests back while paused or when every matching Allowance has unchecked limits, an Allowance with unchecked limits stays manual while a Bitkit-style Allowance still pays automatically, and only the strict ledger check fails on an unreadable ledger; with a checked and an unchecked Allowance for one contact, a request only the unchecked one fits stays manual without Limit Reached both while the node cannot pay and when it can, a request the checked one fits pays through it, and a request neither fits raises Limit Reached once; a request whose covering Allowances have all ended stays manual without Limit Reached in both paths; and a checked Allowance the SDK blocks stays manual without Limit Reached when it fits locallyPaykitAllowanceRepoTest.kt— the restore pause shows only while the wallet pays an Allowance, the ledger-behind reason wins when both apply, Resume ends the restore pause, Resume from the restore notice keeps a ledger behind this wallet paused with its own notice, and an unreadable ledger fails Resume without accepting itPaykitAllowanceTest.kt— terms with a lifetime, count or other period limit are flagged unchecked; a per-payment cap with one monthly amount limit is notBackupRepoTest.kt— a wallet restore pauses automatic payments before it restores allowance or Paykit SDK state, also without any allowance stateAllowancesScreenTest.kt— the paused notice shows the restore or ledger-behind description for its reasonAllowancesViewModelTest.kt— follows the pause reason exposed by the repositoryEvidence
357faa866:just compile,just test(4,877 unit tests, 0 failures) andjust lint(detekt and Android Lint, 0 issues) passed locally on macOS.2301247ffpassed the full gate (4,874 unit tests);52d99e108passed the full gate (4,870 unit tests);c56662a16passed the full gate (4,865 unit tests);f4b560aa5(pause only) passedcompileDevDebugKotlinand the Allowance, backup and Allowances UI unit tests.Gaps
Models used
Claude Opus 5.5(reasoning:Not exposed)Claude Opus 5.5(reasoning:Not exposed)Claude Opus 5.5(reasoning:Not exposed)Reasoning effort is not exposed in the session metadata available to the implementation agent.
🤖 Generated with Claude Code