Repository navigation
fix: allowance follow-ups - #1141
Conversation
A settlement and restart recovery could both record one failed attempt and release the payee's payment list twice, and a list released before broadcast was released again when clearing its claim failed to save.
The Auto-paid label and the amount paid automatically now come from the Paykit SDK's accounting history, which keeps every attempt, instead of the local journal, which keeps only the newest 200.
A bitcoin request whose accepted endpoints are only USDT or on-chain was held for an allowance that can never pay it, waiting on a usable channel and keeping it off the Payment Request sheet. Coverage now requires an accepted Lightning endpoint.
…ears Signing in, restoring or ending a profile deletion turns authState authenticated while the session change is still in flight, so allowances never got their identity and contacts never loaded until Contacts was opened. The profile manager now announces when a session change settles, and the app activates allowances and loads contacts then.
An offer is marked before the automatic accept and unmarked once announced, so an accept that a stop or a cancelled refresh cut short is announced on the next refresh. Offers accepted in one refresh share one announcement, since each fallback toast replaced the one before it. The shared allowance journeys now say the payee's wallet accepts by itself.
Setting, answering or ending an allowance asked for the PIN on the payment PIN screen, whose text said it confirms sending a payment. Those checks now say they confirm an allowance change.
When the journal can't save an attempt's outcome, the entry stays open and a later settlement or recovery acts on it once, instead of releasing the list now and again after a relaunch.
The accounting history is scoped by the local identity; payments of another identity no longer show as Auto-paid, and a failed read keeps the labels and totals already shown.
The start marked the identity as started before it ran, so a deletion or session change that stopped it left contacts unloaded until sign-out.
Also matches the shared allowance journeys' wording to the Android twin.
The allowance state (USD limit labels and the attempt journal recovery verifies from) lived only in the keychain, so a restored wallet lost its labels and left open attempts unverifiable. It is now part of the Paykit payment state backup, in a format both apps share, and changes to it schedule a backup.
|
Pushed 99b02d1:
Allowance, profile manager and backup suites pass on this head; the full unit suite passed at cc24f0e. |
Allowance state rides in the wallet backup, so reading, restoring or capturing it is now best-effort: an entry this version can't read is skipped and logged, and the rest of the wallet backup and restore go ahead.
…ge restored state A same-key session that appeared while a start waited now runs its own start. Restoring allowance state keeps local identities the backup lacks, backup identity keys are normalized as on Android, and the combined Allowance Added notice names each contact once by key.
|
Pushed aa2b648:
The device run of the allowance journeys and the QA notes are in the description. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P1). Follow-ups to allowances: wallet-backup support for allowance state in the format shared with Android, a session that appears mid-start restarting allowances, restored state merging with local identities, and the combined Allowance Added notice. Reviewed aa2b648c, full tier. Reasoned from the code and CI; the iOS app does not build on this reviewer's Linux host.
What I checked, and 5 candidates I ruled out
Read: PaykitAllowanceStateBackup.swift, PaykitPaymentStateBackup.swift, PaykitAllowanceExecutor.swift (store, recovery, autoPay, manual payment), PaykitAllowanceManager.swift, the backup restore path in BackupService.swift, and the new tests in PaykitAllowanceTests.swift. Cross-checked against the Android counterpart in synonymdev/bitkit-android#1708 (PaykitAllowanceStateBackup.kt, BackupRepo.restoreWalletBackup).
CI: SwiftFormat, build-local and validate green; Run Tests, Integration Tests and e2e still pending at review time.
Ruled out
- Backup keyed by a non-normalized identity: the Paykit FFI returns every public key through
to_app_key()(pubky+ z32), andpublicKeyFromSecretKeynormalizes, so the identity keys and the store keys agree. - Restore drops local identities: the head commit merges restored identities into the local store instead of replacing it.
- Unknown stage fails the whole wallet backup on iOS:
PaykitAllowanceStateBackupsdecodes each identity throughLenient, and the new test covers it. billingPeriodStartsAtomitted when nil: Android'sPaykitPaymentRequestIdgives it anulldefault, so an absent key decodes.- USD labels lose precision across platforms: iOS writes
Decimalstrings, and Android parses them withBigDecimal, so the strings round-trip. - Cold reader: not run on the finding below, given the run budget. The finding rests on an executed decode instead.
Merge confidence: 3/5, the P1 on cross-platform restore of a manual-payment journal entry.
@jvsena42 fixed in 08225c9 instead of deferring, since this PR added the set. A new test, |
|
Pushed e71b591: a payment list consumed by an automatic payment is now given back after a restart. @ben-kaufman raised this on the Android twin (comment on synonymdev/bitkit-android#1707), and iOS had the same gap. The list was consumed before its version was journaled. A restart between the two writes left recovery with no version, so it recorded the failure and never gave the list back. The same happened when the rollback before broadcast failed to save.
New tests cover a restart before the consume, the owner checks, a consume over a changed list, and a release retried after a failed save. The executor, allowance, payment proof, payment request and private Paykit suites pass. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to 08225c9. announcedOfferIds is now cleared on sign-out and on a profile switch (PaykitAllowanceManager.swift:135, :155), with a test, which matches Android. No new findings.
Device gate (partial): 08225c9, not run yet. Pending: J1–J5, including restore-keeps-history.xml, paired with the Android twins.
The executor, allowance, private Paykit and payment request suites pass. |
QA@jvsena42's pending device gate (J1 to J5), run on
The recovery phrase is blurred in the first J5 recording and cut from the second.
|
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to f085773. No new findings.
- Releases are owner-checked and become no-ops after the first.
- A list version is cleared only after its release succeeds.
- The new claim lock is taken only in consume and release, with no re-entry or cross-lock.
- Restore writes the ledger and claims together before any recovery can read the journal.
- Backups stay compatible both ways: v3.0.0 ignores the new owner field, and older backups default to an empty owner. The key matches across Android and iOS.
Device gate (partial): f085773, not run yet. Pending: the allowance journeys paired across both platforms, including restore-keeps-history.xml.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: full review of the complete PR diff against its merge base b8b7f3d, at f085773. There was no earlier baseline, so no coverage is carried over. The review covered: allowance activation once the Pubky session settles (AppScene, PubkyProfileManager); offer marking and the combined "Allowance Added" notice; writing the journal before consuming the list, and releases that only the owning attempt can make; node-backed recovery of restored attempts and the new recovery run when the node starts; Lightning-only coverage; Auto-paid totals from the SDK ledger; the allowance PIN message; allowance-state backup, restore and backup scheduling; translations; and journeys. Shared behavior and the backup wire format were compared with the Android twins bitkit-android#1707 at 2122f57 and bitkit-android#1708 at bc5f8ef.
No new actionable code findings.
- The allowance backup reads correctly in both directions with #1708. Identity keys are the same
pubky-prefixed form on both sides; the Paykit FFI returns keys viato_app_key()at paykit-rs dd8e75e, the revision pinned inPackage.resolved. Stages are lowercase, timestamps are RFC 3339 with milliseconds, and empty optionals are written as explicit nulls. - I checked the earlier review concerns against this head's source, and all are fixed: the explicit-null
allowanceId(thread), the failed pre-accept mark and the repeated notice (thread, thread), recovery before the node has payment history (thread), and consuming a list for an attempt that was never journaled (thread). - A remaining risk, not a finding: recovery now also runs on every node
.runningtransition (AppScene.swift:1256), andrecoverdoes not skip requests the executor is currently paying (inFlightRequestIds). Suppose the node restarts, for example after "Reset payment routing caches", while an automatic payment is betweenbeginPaymentExecutionand.sending. Recovery could then record it as failed while the send goes ahead, becausesetStageDurablydoes not check for a terminal stage. I couldn't show that this timing can actually happen. Skipping in-flight requests inrecoverwould close it.
Non-blocking consistency suggestions
- iOS
mayCoverholds a request from any contact with a proposed or active allowance. Android's version (#1707) also requires a Lightning endpoint. On iOS, a request with only USDT or on-chain endpoints therefore waits for the refresh in progress before its sheet opens. Adding the same Lightning check would match the two.
Validation: CI passed at this head: Run Tests, Integration Tests, e2e-tests-local and SwiftFormat. This review is source analysis only; I did not build the app or run any tests locally. The author reports J1–J5 passing on e71b5916 (comment); later commits are covered by unit tests only.
Device testing: not performed in this review. Ready for device testing.
Suggested additional test cases
- iOS, J5 setup (payer with the $5 a payment / $50 a month allowance Active): after Reset And Restore, open the payee's allowance row and detail sheet. Expected: the limits read exactly $5 a payment and $50 a month, not an amount converted at the current rate. On iOS, PAID AUTOMATICALLY and "Auto-paid" now come from the SDK ledger, so J5 passes even if the new local allowance backup did not restore. The USD labels are the only visible output of that backup.
- Restore across platforms. Setup: an Android payer on the #1708 build with an Active allowance, one automatic payment and one incoming request paid by hand. Wait for the wallet backup, then restore the phrase on iOS at this head. Expected: the allowance row is Active with the same USD limits, and PAID AUTOMATICALLY and "· Auto-paid" are shown. Repeat in the other direction (iOS backup restored on Android #1708).
- iOS: run the existing
restart-never-pays-twice.xmlat this head. It is not in the PR's journey list, but recovery changed here: submitted attempts are now settled from the node, and recovery reruns when the node starts. Expected: the interrupted payment is paid once, and "Payment Executed" and the Auto-paid total appear once.
Reviewer model: claude-opus-5-5
Reasoning effort: high
Recovery also runs when the node starts, so a node restart during an automatic payment could record its attempt as failed while the send went ahead. Recovery now skips requests the executor is paying.
|
Pushed f3b5825.
Done in f3b5825.
out of scope: tracked in #1159, because
Not run on this head. The restart case is covered by the unit tests on the recovery paths, including the new one. The two restore cases need a device run. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of the changes since f085773 (baseline review), at f3b5825. The comparison base and merge base are unchanged at b8b7f3d. This pass covered recovery leaving an in-progress automatic payment alone, the new unit test, and the earlier review threads. The other baseline surfaces are inherited.
No new actionable code findings.
f3b5825 skips recovery for a request autoPay is still executing. That is the window the baseline review described: a node .running transition while the attempt is between beginPaymentExecution and the node handoff. inFlightRequestIds is set before autoPay's first await and cleared when payLightning returns. ensureReconciled leaves the ledger untouched while requiresReconciliation is false. At paykit-rs dd8e75e, that flag is set by restore and invalidation, and reserve waits until reconciliation is clear, so a normal in-progress send is not reconciled again. A terminal Failed outcome cannot later be rewritten to Succeeded, which is why skipping that record matters.
Android still skips only the live attempt id after reservation (#1707 at 2122f57, #1708 at bc5f8ef). iOS skips the whole request. autoPay enters only while the request is still proposed, so a crashed post-accept attempt stays recoverable.
The earlier threads stay fixed. This diff leaves the explicit-null backup, offer marking, unavailable node history, and the journal-before-consume guard as they were.
Validation: Source analysis only. I inspected testRecoveryLeavesTheAttemptOfAPaymentInProgressAlone and did not run it. SwiftFormat passed at this head. Run Tests and Integration Tests were still in progress.
Recommended before device testing: Wait for Run Tests at this head. It includes the new check that recovery during the send records no outcome and leaves the journal Sent.
Device testing: not performed in this review.
Reviewer model: grok-4.7
Reasoning effort: high
Device test results — scoped smokePassed at Clean dev installs with separate fresh wallets and Pubky profiles, linked as contacts. Bitcoin used regtest; USDT used the existing local EVM fork. Limits were $5 per payment / $50 per month. No Lightning channel was needed for the exercised scope. Observed on iOS:
This exercised the UI portions of J1/J3, the single-offer acceptance/notice behavior in J4, and manual case 1. J1/J3 were not executed end to end. Automatic Lightning payments, monthly-cap exhaustion, interruption/recovery, manual case 2 (multiple payers), J5 restore and cross-platform restore were not run. This is a scoped device result, not a complete PR device pass. Revision boundary: the newer |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to f3b5825. No new findings.
f3b5825 makes recovery skip any ledger occurrence whose request is in inFlightRequestIds (PaykitAllowanceExecutor.swift:489). Before it, a recovery pass on a running node could record a terminal Failed on an automatic attempt that sat between journal(.prepared) (:850) and setStageDurably(.sending) (:950). paykit-rs can't rewrite Failed, so that attempt's capacity could be overspent.
I checked:
- The marker is set at :738 before any await and removed by
deferafter the unstructuredadmitAndPaytask completes, so cancelling the caller doesn't remove it early. - Recovery never pays, so this can't pay twice.
- The set is in memory and empty after a restart, so an attempt that crashed still settles through the node as before.
- The new test fails on both assertions without :489.
Device gate (partial): f3b5825, not run yet. Pending: the allowance journeys paired with the Android twin synonymdev/bitkit-android#1708, including restore-keeps-history.xml and restart-never-pays-twice.xml.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approved at f3b582580e9ba665ae37219a2aea0e20974ba460, following the full code review at f085773, the follow-up review at f3b5825, and the targeted device run below. Current unit, integration, lint, translation and E2E workflows passed; all five inline review threads are resolved.
Device test results — automatic-payment follow-up
Tested 28 October 2026 on the existing iPhone 17 simulator, iOS 26.0, using a rebuilt, clean-installed Debug E2E app at the exact revision above. Paired with Android #1707 at 2122f57dcdf0b330b8034ad9807d65be1e14f169.
Local Docker regtest, distinct fresh wallets and Pubky profiles, mutually saved contacts, contact payments enabled, and active $5/payment / $50/month allowances. Each wallet had a local Lightning channel with 500,000 sats pushed. PIN was disabled and iOS notifications were off.
- Automatic under-limit: iOS paid a BTC-priced 2,000-sat request without payer interaction. Android received 2,000 sats once, with a 1-sat routing fee. The request showed Auto-paid and “Paid Automatically” became $1.61. iOS also received Android’s 2,000-sat automatic baseline payment.
- Recovery overlapping a live automatic attempt: the unchanged Debug binary was observed with LLDB. A temporary local proxy hold paused the private-inbox refresh after reservation/journaling, and the real
recover(identity:)method was scheduled through LLDB. The node was restarted through Dev Settings while the attempt remained live. After releasing the hold, recovery reached the new line-489 guard with the same request ID still ininFlightRequestIds; it skipped that request’s attempt loop. Older requests did enter the loop in the same pass. The live request then paid once and retained its Auto-paid tag and accounting. - Across the baseline and two restart probes, iOS sent three separate 2,000-sat payments, and its total became $4.83. Android received each once. After force-stop/relaunch of both apps, all four request rows and the totals remained correct; channel sent/received/update counters stayed unchanged with zero pending HTLCs. No incorrect Failed state or duplicate transfer was observed.
Instrumentation boundary: the first UI-only node restart did not dispatch allowance recovery and therefore did not prove the overlap. The successful overlap used debugger-invoked recovery; no ledger, outcome or in-flight IDs were injected, and no app source was changed. Automatic lifecycle dispatch and recovery during an already-submitted HTLC are not claimed. The debugger was detached and temporary proxy rules removed before relaunch.
This adds current-revision payment/recovery evidence to the earlier UI/manual-flow smoke at f085773. It remains targeted coverage: cap exhaustion, restore, history trimming, multiple concurrent payers and PIN-protected automatic payment were not rerun. Approval does not represent a complete device-matrix pass.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Fixes #1137
Fixes #1103
Fixes #1089
Fixes #1090
Fixes #1016
Twin: synonymdev/bitkit-android#1707
Twin: synonymdev/bitkit-android#1708 (the restore part)
This PR:
Description
pin-gates-allowance-changes.xmlare separate steps.restore-keeps-history.xml, the journey for a payer restored from its recovery phrase keeping its allowance history.Out of Scope
Design
N/A — no design available.
Preview
Allowance journeys on two iOS simulators (payer left, payee right), head of this PR against a local regtest; 2x speed.
set-and-accept.mp4
pin-gates-set.mp4
auto-pay-no-pin.mp4
pin-gates-end.mp4
ended-asks.mp4
monthly-cap.mp4
QA Notes
Setup
Allowances are unreleased: a wallet that already holds allowances from an earlier build loses its local allowance state (USD limit labels, journal) on upgrade, since the stored format gained a field. Start with fresh allowances.
Two instances set up as in
journeys/allowances/README.md, each other saved as linked contacts, the payer with a usable channel and over 50,000 sats spending, the payee with receiving capacity. Staging Blocktank no longer opens channels, so the runs here used a localsynonymdev/bitkit-dockerregtest (marketplace profile: local Pubky testnet and homeserver, Paykit server, bitcoind, Electrum, LND with the LNURL channel fixture) and an E2E build:SWIFT_ACTIVE_COMPILATION_CONDITIONS="DEBUG E2E_BUILD" E2E_BACKEND=local E2E_NETWORK=regtest E2E_HOMESERVER_PUBKY=8pinxxgqs41n4aididenw5apqp1urfmzdztr8jt4abrkdn435ewo. Each wallet took a 1M sat channel with 500k pushed from the fixture'sGET /generate/channel(itsLND_P2P_ADDRESSmust name the port the simulators reach LND on). Amounts are entered in USD.Journeys
pin-gates-allowance-changes.xml— the PIN screen for an allowance change says it confirms the allowance change; payee and payer checks splitmonthly-cap-reached.xml— wording: the payee's wallet accepts by itselfend-stops-auto-pay.xml— wording: the payee's wallet accepts by itselfset-and-accept.xml(unchanged) — regression: the payee's "Allowance Added" shows once after the automatic acceptrestore-keeps-history.xml— after Reset And Restore, the payer's allowance row, its paid-automatically total and the "Auto-paid" tag come backManual Tests
Automated Checks
PaykitAllowanceExecutorTests.swift— a settlement and recovery that both record one failure release the payment list once, in either orderPaykitAllowanceExecutorTests.swift— a list version that cannot be journaled, or an attempt whose journal entry was never saved, never consumes the list; the list is consumed as its attempt, and a release that could not be saved is retried by the next recoveryPrivatePaykitServiceTests.swift— a wallet restore brings back a consumed list's owner, and a backup from before list owners still decodesPaykitPaymentRequestServiceTests.swift— only the attempt that consumed a list gives it back, a manual payment's consumption of the same version is left alone, a list consumed since its version was read is not consumed over, and saved contact state from before list owners still decodesPaykitAllowanceExecutorTests.swift— a list released before broadcast is not released again when clearing its claim fails, and a failure that can't be saved is released once by the next recoveryPaykitAllowanceExecutorTests.swift— a bitcoin request with only USDT or on-chain endpoints is not covered and reaches the Payment Request sheetPaykitAllowanceExecutorTests.swift— Auto-paid labels and totals count all 250 automatic payments in the SDK history, and only the signed-in identity'sPaykitAllowanceExecutorTests.swift— an offer whose accept was cut short after the SDK committed it is announced once on the next refreshPaykitAllowanceExecutorTests.swift— an offer whose mark fails to save is announced after the next refresh, and one whose clear fails is not announced againPaykitAllowanceExecutorTests.swift— signing out or switching profiles forgets which offers were announced, so a new session announces an offer whose mark is still storedPaykitAllowanceExecutorTests.swift— recovery settles a restored submitted attempt as paid when the node reports it paidPaykitAllowanceExecutorTests.swift— recovery during an automatic payment's send leaves its attempt alone, so the payment ends Sent, not FailedPaykitAllowanceExecutorTests.swift— recovery leaves a restored attempt open while the node has no payment history yet, then settles it once the node reports it paidPaykitAllowanceTests.swift— the allowance state backup keeps labels and the journal in the shared wire format and restores to the same statePubkyProfileManagerTests.swift— the automatic Paykit session is nil during a session change and readable when the change is announced as settledEvidence
Earlier runs:
Device runs used an app build at 99b02d1 on two iPhone 16 simulators, iOS 26.5. Later commits make the allowance backup best-effort, restart a session start cut short by a same-key session, and merge restored state; they are covered by unit tests, not re-run on devices.
BitkitTestswith CI's skip list passed on the final head aa2b648 (2,894 tests). The new release and coverage tests fail when their fix is reverted.masteralike (see Gaps).Gaps
masterdid not reproduce it on a plain cold start. This PR fixes the cause the issue describes (the session not yet ready when the profile turns authenticated, as during a sign-in, a wallet or session restore, or a profile deletion), covered byPubkyProfileManagerTests.swift; the reporter's exact trigger was not reproduced.Models used
claude-opus-5-5(reasoning:10)claude-opus-5-5(reasoning:10)claude-opus-5-5(reasoning:10)Reasoning values are the harness's effort setting for these sessions; the reviewer was an independent subagent with fresh context.