Repository navigation
Conversation
09f07b7 to
4f8ae14
Compare
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. |
![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: 4f8ae14822
ℹ️ 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".
 [Critical impact] The changes affect automatic Bitcoin payments and how their limits are tracked. Fix the lost payment checks, reconciliation order, failed-spend cleanup, and End race before merging. Findings
SummaryAdds wallet-backed checks before automatic allowance payments:
Four fixes remain: save failures can discard checks, reconciliation can run before the floor check, failed spends can survive recovery, and an End can race with handoff. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Covered request] --> B[Check saved ledger floor]
B --> C[Reconcile ledger]
C --> D[Check returned ledger]
D --> E[Check ended IDs and monthly spend]
E --> F[Reserve attempt]
F --> G[Save journal and local spend]
G --> H[Check ended IDs]
H --> I[Receive private messages]
I --> J[Begin execution and pay]
K[User ends allowance] --> L[Save ended ID]
L --> M[Send End through SDK]
N[Wallet backup] <--> O[Merge floor, spends, and ended IDs]
Reviews (1) · Last reviewed commit: "fix: harden automatic allowance payments" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 3c09544.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
A Send-sheet payment of a request this change keeps manual still reconciles the allowance ledger. When a floor is already stored and the SDK serves no ledger, that call attests an empty history. Recovery and automatic payment were changed to leave that ledger alone.
While the node cannot pay, the above-limit handoff still uses ledger and local spend totals only. A request the new floor will force onto the manual flow stays off the Send sheet until a channel is usable.
Comparison with bitkit-ios#1175 at dd478be covered the allowance floor, local spends, ended allowances, and manual ledger reporting. beginManualPayment and exceedsLimits follow the same split.
These open threads still match this revision: floor save still accepts the ledger, reconciliation uses a later read, failed spend kept when the journal row is already failed, and End during receive. Recovery now drops a failed spend whose journal entry was trimmed (thread); that part matches syncSettledJournal when the journal entry is absent.
Validation: inspected PaykitAllowanceExecutorTest.kt, PaykitAllowanceStoreTest.kt, and PaykitAllowanceStateBackupTest.kt at this revision. This review did not execute them. The missing-ledger test asserts that automatic payment does not reconcile; it does not call beginManualPayment. No GitHub issue matched these mechanisms. Device testing: not performed in this review.
Suggested additional test cases:
- Android unit test in
PaykitAllowanceExecutorTest. Setup: store a ledger revision floor and stuballowanceAccountingState()as null. Action: callbeginManualPaymentfor a bitcoin incoming request. Expected:reconcileAllowanceAccountingis not called. - Android unit test of the covered-request pass used when the node cannot pay. Setup: floor set, ledger missing or older, and the request amount within the ledger and local spend totals. Action: run that pass. Expected: the request is marked manual so it can be presented, including before a channel is usable.
Findings
- [HIGH] Manual send rewrites a ledger the floor refused — Trigger: the wallet has a ledger revision floor, and the SDK serves no ledger or an older one. Automatic payment returns manual, the request is presented on the Send sheet, and the user confirms the payment.
Mechanism: proceedWithPayment calls beginManualAllowancePayment, which calls beginManualPayment and reportManualPayment. That function always calls ensureReconciled before reserveManualPayment. ensureReconciled does not consult the floor. A null ledger uses an empty history and a null expectedRevision, and the reconciliation is submitted. An older ledger that requires reconciliation is repaired from that older snapshot. reserveManualPayment then records the attempt on the resulting state.
Consequence: this path would replace a ledger the wallet has already read with an empty or older history. Allowance spends that exist only in the discarded ledger stop constraining the monthly cap once the rewritten revision reaches the floor and automatic payment accepts it again. Local automaticSpends still cover only the spends this wallet stored itself.
Expected behavior: a request kept manual because the ledger is missing or behind should leave that ledger untouched, as recovery does. The Send sheet can still pay. The allowance report should wait until a ledger at or above the floor is served. A failed report already lets the send continue.
Evidence basis: source analysis at 3c09544e3f37a65efb67c6c92e909f9fbf7f8087. a missing ledger after one was read keeps the request manual asserts that automatic payment performs no reconciliation. It does not call beginManualPayment. This review did not execute that test or a device. The same unguarded ensureReconciled call is present in bitkit-ios beginManualPayment at dd478be41ea60887baf613777cb87b7a930a1944.
Source. Summary placement: The write is ensureReconciled in reportManualPayment, which this PR does not modify. The new manual result is what reaches it after the floor refuses the ledger.
- [MEDIUM] Channel wait ignores the ledger floor — inline at
app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:403.
Reviewer model: grok-4.7
Reasoning effort: high
|
@piotr-iohk both findings from your QA review at 3c09544 are fixed on this PR's current head:
The iOS twin, synonymdev/bitkit-ios#1175, has both fixes (0e18333a, e503eb7f). 🤖 Addressed by Claude Code |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up of e93e25509 since 3c09544e, on merge base c080139b. Inherited baseline review 5713748206 for allowance backup restore merge and the shared wire format. This pass covered the descendant delta: durable floor saves, reconciliation before any ledger write, failed-spend removal, the pre-handoff end check, above-limit handoff, manual reporting, and the new executor tests.
No new actionable code findings.
Reconciliation throws LedgerBehind before reconcileAllowanceAccounting when the served ledger is older than the floor, missing after a floor was saved, or the floor cannot be saved. Automatic payment and recovery stop there. Manual reporting stops as well and returns no attempt id, so the Send sheet still sends; that matches the existing unreported-ledger contract and bitkit-ios e503eb7f. While the node cannot pay, that behind ledger is handed back without Limit Reached. Recovery removes local spends for attempts the ledger has failed, including after the journal entry is gone. The private-message fetch runs before the ended-allowance check, the same order as iOS.
Validation: PaykitAllowanceExecutorTest was inspected at e93e255, including the unsaved-floor and behind-ledger cases, and was not executed in this review. Device testing: not performed. A journey cannot produce a ledger older than one this wallet has already read.
Suggested additional test cases:
- Android unit test in
PaykitAllowanceExecutorTest: setledgerRevisionFloorto 5 and the accounting ledger revision to 4, then callbeginManualPaymentfor an incoming bitcoin request. Expected: null attempt id, noreconcileAllowanceAccounting, and noreserveManualPayment.
Earlier findings reconciled at this revision: 2 verified fixed.
Reviewer model: grok-4.7
Reasoning effort: high
|
@piotr-iohk thanks, added your suggested case on both apps (b431971 here, d9265ab4 on synonymdev/bitkit-ios#1175): with a floor of 5 and a ledger at revision 4, 🤖 Addressed by Claude Code |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at b431971 after the merge base changed and no prior coverage baseline was available. The comparison base and merge base are fdc1012.
No new actionable code findings.
Automatic payment, recovery, and manual reporting stop before writing a ledger that is missing or older than the stored revision floor. A manual send still proceeds, with no allowance attempt id. While the node cannot pay, that ledger returns the request immediately and does not raise Limit Reached. The monthly check uses the higher of ledger usage and this wallet's own spends, a failed attempt stops counting after its journal row is gone, and an End saved during the private-message fetch stops the handoff. Restore keeps the higher floor and every stored spend and ended allowance. The same checks are on bitkit-ios#1175 at d9265ab.
The findings from the review at 3c09544 do not match this head. PaykitAllowanceExecutorTest.kt, PaykitAllowanceStoreTest.kt, and PaykitAllowanceStateBackupTest.kt were inspected, including the behind-ledger manual-payment case, and were not executed. Device testing: not performed. A journey cannot serve a ledger older than one this wallet has already read.
Reviewer model: grok-4.7
Reasoning effort: high
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at 47ec1b9. One MEDIUM (inline), present in the same shape on synonymdev/bitkit-ios#1175.
Checked and clean, for fund draining:
- All three new checks only tighten admission.
- Spends are removed only where the SDK reservation was already released as FAILED.
- Requests are processed serially, which rules out a race on a stale
localsnapshot. AllowanceIdis a UUID v4, so ended keys can't collide.- Signed-in backups keep the floor at or below the ledger revision.
perf: marginal. About three extra keychain or store writes per automatic payment, with no user wait.
Device gate: not run. It waits for the MEDIUM.
Twin: synonymdev/bitkit-ios#1175
Automatic allowance payments now also check state the wallet keeps itself, and backs up with the wallet, before they pay.
Description
ledgerRevisionFloor,automaticSpends,endedAllowanceIds; the same wire format as fix: harden automatic allowance payments bitkit-ios#1175). Restore merges them with any already stored: the higher floor, every spend and every ended allowance, so a restore never weakens them, and the restored wallet applies the same checks. A backup taken without Paykit SDK state restores without the floor, since the floor only holds next to the ledger it was read from.Out of Scope
None.
Design
N/A — no UI changes.
Preview
N/A
QA Notes
Setup
N/A — no device testing required: the checks change behaviour only when the allowance ledger read back is older than one the wallet already read, which no journey can produce. Existing allowance journeys cover the unchanged paths.
Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
PaykitAllowanceExecutorTest.kt— a floor that cannot be saved keeps the request manual; while the node cannot pay, a ledger behind the wallet hands the request back without Limit Reached; a ledger below the floor, one that drops below it during admission, or one missing after one was read keeps the request manual; an automatic payment raises the floor; local spending above the ledger's keeps the request manual; a spend counts until its attempt fails; an allowance seen ended never pays automatically; recovery drops the spend of a failed attempt the journal no longer holdsBackupRepoTest.kt— a wallet restore without Paykit SDK state drops the floor and keeps the spends and ended allowances; one with SDK state keeps the floorPaykitAllowanceStoreTest.kt— restore never lowers the floor and keeps every spend and ended allowance already storedPaykitAllowanceStateBackupTest.kt— the golden wire format carries the floor, the spends and the ended allowancesEvidence
just compile,just testandjust lintpass on this head.Gaps
Models used
claude-opus-5-5(reasoning:medium)claude-opus-5-5(reasoning:medium)claude-sonnet-5-5(reasoning:Not exposed)The review subagent's reasoning setting is not in the session metadata.