Repository navigation
fix: allowance follow-ups - #1707
Conversation
b6ee6e6 to
2dbeae7
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dd04a89ef
ℹ️ 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".
[High impact] The changes reach allowance-backed automatic payments and request handling. The on-chain release must preserve a retry until the allowance outcome is saved before this PR merges. FindingsSummaryThis PR improves allowance request holds, payment totals, offer notices, and PIN wording.
Acknowledged scope:
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[On-chain release completes] --> B[Delete original proof and saved send]
B --> C[Save failed allowance outcome]
C -->|Success| D[Return allowance capacity]
B -->|App stops| E[No saved release to retry]
C -->|Write fails| E
E --> F[Recovery cannot prove failure]
Reviews (1) · Last reviewed commit: "docs: show a request after an ended allo..." · Reviewed by Greptile |
|
Pushed ee81572: a consumed payment list now records which automatic attempt consumed it, and only that attempt can give it back. The version is journaled before the consume again, so a restart between the consume and the journal write no longer strands the list. A failed release is retried by the next recovery. This replaces the ordering from fd05195 and answers @ben-kaufman's comment on |
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to fd05195. My LOW at PaykitAllowanceExecutor.kt:308 is fixed, and I have no new findings.
- The list version is now written to the journal only after the consume succeeds (
:617-623), so a failure before the consume leaves no version, and recovery at:291-293and:308-310releases nothing. - A failed save after the consume can't release twice, because the store keeps no memory cache.
- The stranded-list fix from
b15acdastill works. - The crash window between the consume and the journal write is on ben-kaufman's open thread at
:618. It has the same shape iOS accepted, and no funds move.
Device gate (partial): fd05195, not run yet. Pending: J1–J4, paired with synonymdev/bitkit-ios#1141.
|
Pushed 1f80f9d: a payment list's journaled version is now cleared only after its owner-checked release succeeds, so a failed release or a restart mid-release leaves it for the next recovery. This answers @ben-kaufman's comment on |
|
Pushed cc2da38 and 21c6f49: payment list consumes and releases are now serialized through their cache write, so an overlapping release never reads a claim that a failed write later rolls back (answers @ben-kaufman's comment on |
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to 21c6f49. 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): 21c6f49, not run yet. Pending: the allowance journeys paired across both platforms, including restore-keeps-history.xml.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base 73b5fa7, at 21c6f49. No saved assessment baseline.
No new actionable code findings.
Compared with bitkit-ios#1141 at f085773 for the shared allowance flows. Held requests are presented when coverage settles, and only a proposed or active Lightning allowance holds one. A payment list is released only by the attempt that consumed it, and the claim lock covers the cache write and its rollback. A released manual on-chain payment is recorded failed before its saved attempt is removed. Accepted offers are queued outside the polling job, announced once together, and kept marked until that announcement is cleared. The allowance PIN copy and the ended-row journey wording match iOS. iOS reads paid-automatically totals from the SDK ledger; Android stores them beside the trimmed journal. For payments settled by this code, both still show the total after the journal drops the row. Allowances are not in a released build, so older dev journals are not a supported upgrade.
This review did not execute tests or a device. On this revision, lint, detekt, and the dev build succeeded. The reported J1–J4 pass is at 2dbeae73, before the later payment-list and announcement commits, so it is not evidence for this head. The checklist already asks for those journeys here.
Suggested additional test cases
- Android, two linked dev wallets. Have two contacts' allowances be accepted in one refresh. Expected: one "Allowance Added" notice naming both contacts, and a later refresh does not announce them again.
- Android, payer wallet. Accept an allowance offer, then leave the app or kill it before the notice appears. Relaunch. Expected: exactly one "Allowance Added" notice and an Active row. A second launch does not announce it again.
Device testing: not performed in this review.
Ready for device testing.
Reviewer model: grok-4.7
Reasoning effort: high
|
@ovitrif please resolve conflicts. |
…w-ups # Conflicts: # app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt
@piotr-iohk Resolved in 2122f57 by merging master. The one conflict was in |
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 2122f57 after the comparison base moved from ea89ed18 to 65f20828. There was no inherited coverage.
No new actionable code findings.
Compared with bitkit-ios#1141 at f085773. Held requests are presented when coverage settles, and only a proposed or active Lightning allowance holds one. A payment list is released only by the attempt that consumed it, and the claim lock covers the cache write and its rollback. A released manual on-chain payment is recorded failed before its saved attempt is removed. Accepted offers are marked before accept, queued outside the polling job, and announced once together. The allowance PIN copy matches the journey. iOS reads paid-automatically totals from the SDK ledger and restores a failed attempt's payment-list owner; Android stores those totals beside the trimmed journal and keeps the owner in the local cache. Restore of an open allowance attempt remains bitkit-android#1708. Android's allowance payment context still does not record a previous list version, so a failed automatic release clears the consumed watermark; that source is unchanged from the merge base.
This review did not execute tests or a device. On this revision, build, lint, and detekt succeeded. The reported J1–J4 pass is at 2dbeae73, before the payment-list owner, claim lock, and this master merge, so it is not evidence for this head. The checklist already asks for those journeys.
Device testing: not performed in this review.
Ready for device testing.
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 Android:
The first USDT quote expired while the screen was idle; refreshing the quote completed successfully. This exercised the UI portions of J1/J3 plus the checks above. J1/J3 were not executed end to end. Automatic Lightning payments, monthly-cap exhaustion, interruption/recovery, combined notices from multiple payers and restore scenarios were not run. This is a scoped device result, not a complete PR device pass. |
ben-kaufman
left a comment
There was a problem hiding this comment.
No blocking findings at 2122f57. Device coverage remains partial, and independent security coverage is incomplete.
Reviewer model: gpt-6.1-sol, claude-opus-5-5
Reasoning effort: high for both
|
@ben-kaufman Merging this PR on your approval of head 2122f57 and @piotr-iohk's passing device run on that head, with checks green and no open threads. #1708 is retargeted to master first, so it stays open. |
Device test results — automatic-payment follow-upPassed at Rebuilt and clean-installed the local E2E dev build. Separate fresh wallets and Pubky profiles, mutually saved contacts, contact payments enabled, and active allowances of $5/payment and $50/month. Each wallet had a local regtest Lightning channel with 500,000 sats pushed. No staging Blocktank channel was used. PIN was disabled in these fresh wallets.
This adds targeted automatic-payment and relaunch coverage to the earlier UI/manual-flow smoke. It is not a full PR device pass: cap exhaustion, restore, history trimming and PIN-protected automatic payment were not tested in this run. The iOS recovery instrumentation is described in the twin’s result. Published after this PR merged; the tested revision is explicitly preserved above. |
Closes #1659
Closes #1672
Closes #1599
Closes #1584
Closes #1585
Closes #1646
Twin: synonymdev/bitkit-ios#1141
Refs: synonymdev/bitkit-ios#1090, synonymdev/bitkit-ios#1089
Fixes the allowance follow-ups left by the review of #1340 and #1385, one commit per issue.
Description
Out of Scope
Design
N/A — no design available.
Preview
The allowance PIN check (#1646), and requests paid automatically, stopped at the monthly cap, and stopped by ending the allowance, on two dev builds against a local regtest (videos at 2x).
j4-verify-payer2x.mp4
j2e-payer2x.mp4
j3a2x.mp4
QA Notes
Setup
Two dev (regtest) builds of this branch against the staging LSP, set up as in
journeys/allowances/README.md: each wallet has a Pubky profile, the two are saved as contacts and linked over Paykit, the payer has a spending balance above 50,000 sats with a usable channel, the payee has receiving capacity, and both have notification permission granted. Create each request with the amount entered in sats, not in the fiat unit. On the payer, enable a PIN with "Also require for payments" for J1.Journeys
pin-gates-allowance-changes.xml— the PIN screen says it confirms an allowance change; the payee's "Allowance Added" and the payer's Active row are separate stepsmonthly-cap-reached.xml— the payee's wallet accepts the allowance by itselfend-stops-auto-pay.xml— the payee's wallet accepts the allowance by itselfauto-pay-under-limit.xml(unchanged) — an automatic payment still shows "Auto-paid" and counts in "Paid automatically"Manual Tests
N/A
Automated Checks
AllowanceOffersAcceptedDescriptionTest.kt— offers accepted together get one description naming each contact oncePaykitAllowanceExecutorTest.kt— a list is released once across a failed send, a node failure event, recovery and settlement interleavings, and not at all when its cleared claim cannot be saved, until the next recovery gives it back; a released manual on-chain payment is recorded failed; an automatic payment keeps its lasting total after the journal trims itPaykitAllowanceRepoTest.kt— an ended allowance holds no request; a USDT-only bitcoin request is never held; totals and tags come from the lasting fields; an offer whose accept committed as its refresh was cancelled is announced once on the next refresh; an announced offer whose mark fails to clear is not announced againPrivatePaykitRepoTest.kt— a payment list is given back only to the owner that consumed itPaykitPaymentProofRepoTest.kt— a finished on-chain release fails the allowance attempt, a refused one does notAppViewModelSendFlowTest.kt— a request held while allowances update is presented once they settlePaykitPaymentRequestRepoSubscriptionTest.kt— the intake after accepting a subscription runs under the coverage hold and refreshes the allowancesPaykitPaymentRequestRepoTest.kt— wired for the coverage holdEvidence
just testandjust lintpass at 2a75d6d, and every commit up to it compiles on its own; the two review fixes after it passPaykitAllowanceExecutorTest,PaykitAllowanceRepoTestand detekt.bitkit-docker(Lightning channels from its LND fixture, local Pubky homeserver), with requests entered in sats:Gaps
Models used
claude-opus-5-5(reasoning:medium)claude-opus-5-5(reasoning:medium)claude-sonnet-5-5(reasoning:low)