Skip to content

fix: allowance follow-ups - #1707

Merged
ovitrif merged 27 commits into
masterfrom
fix/allowance-follow-ups
Oct 28, 2026
Merged

ovitrif merged 27 commits into
masterfrom
fix/allowance-follow-ups

Conversation

@ovitrif

@ovitrif ovitrif commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Held and tapped requests around an allowance update (bug: allowance requests can stay hidden or present manually around an allowance update #1659):
    • A request held while allowances update is presented as soon as the update ends, whether or not the refresh succeeded, including one the user tapped.
    • Only a proposed or active allowance holds a request; an ended or declined one no longer does.
    • The follow-up intake after an automatic payment, and the intake after a subscription action, run under the coverage hold and refresh the allowances.
    • Restart recovery and reconciliation read the journal once per pass.
    • A payment list is released only for the attempt that consumed it, so a failed attempt never gives back a list another payment consumed since; a release that fails is retried by the next recovery.
    • The test for a failed send's release now reaches the interleaving it names; two more tests prove the interleavings of bug: a failed automatic payment can release the payee's payment list twice bitkit-ios#1089 release a list once, so that issue has no further Android twin.
  • A released manual on-chain allowance payment is recorded failed, so its capacity returns to the allowance (bug: a released on-chain allowance payment keeps holding allowance capacity #1585). The outcome is saved before the release removes the payment's saved attempt, and the release stays unfinished if it cannot be saved.
  • The PIN screen for saving, ending or accepting an allowance says it confirms an allowance change, not a payment (bug: allowance PIN check says it confirms sending a payment #1646).
  • A bitcoin request that accepts no Lightning invoice, such as one payable only over USDT, is never held for an allowance, not even while allowances update, and goes straight to the manual sheet: the Android twin of bug: a bitcoin request payable only over usdt waits for an allowance it can't use bitkit-ios#1090.
  • "Paid automatically" totals and the "Auto-paid" tag are kept apart from the journal, which keeps only its newest 200 entries, so trimming never changes them (bug: older auto-paid totals and tags disappear after 200 allowance payments #1584).
  • An offer accepted automatically while the app leaves the foreground, or the process dies, is announced once on the next refresh (bug: allowance added notice is lost when the app leaves during an automatic accept #1672): an offer is marked durably before its accept and stays marked until the app has announced it; accepted offers wait in a state the app announces from outside the polling job, so a cancelled refresh never loses one and overlapping refreshes never announce it twice. An offer already announced is not announced again when its mark fails to clear. Its journey wording and test name follow-ups are in the same commit.
  • Offers accepted together in one refresh are announced in one "Allowance Added" notice naming every contact as the locale lists them, so no toast replaces another (bug: allowance added toasts replace each other when several offers arrive at once #1599).
  • The allowance journeys' README now says to enter requests in sats on Android: a request entered in the fiat unit is priced in USD, and allowances pay only bitcoin-priced requests, so it stays manual. iOS converts a dollar entry to a bitcoin amount when it creates the request, so its fiat entry is paid automatically.
  • New strings are translated in every locale.
  • No changelog fragment: allowances have not shipped yet, and the 3.1.0 entries already describe them.

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).

PIN check Auto-paid (J4) Limit reached (J2) Ended (J3)
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

  • J1: updated 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 steps
  • J2: updated monthly-cap-reached.xml — the payee's wallet accepts the allowance by itself
  • J3: updated end-stops-auto-pay.xml — the payee's wallet accepts the allowance by itself
  • J4: auto-pay-under-limit.xml (unchanged) — an automatic payment still shows "Auto-paid" and counts in "Paid automatically"

Manual Tests

N/A

Automated Checks

  • added AllowanceOffersAcceptedDescriptionTest.kt — offers accepted together get one description naming each contact once
  • updated PaykitAllowanceExecutorTest.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 it
  • updated PaykitAllowanceRepoTest.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 again
  • updated PrivatePaykitRepoTest.kt — a payment list is given back only to the owner that consumed it
  • updated PaykitPaymentProofRepoTest.kt — a finished on-chain release fails the allowance attempt, a refused one does not
  • updated AppViewModelSendFlowTest.kt — a request held while allowances update is presented once they settle
  • updated PaykitPaymentRequestRepoSubscriptionTest.kt — the intake after accepting a subscription runs under the coverage hold and refreshes the allowances
  • updated PaykitPaymentRequestRepoTest.kt — wired for the coverage hold

Evidence

  • just test and just lint pass at 2a75d6d, and every commit up to it compiles on its own; the two review fixes after it pass PaykitAllowanceExecutorTest, PaykitAllowanceRepoTest and detekt.
  • J1–J4 pass at 2dbeae7 (the code of this head; later commits only touch journey text) on two Android 16 emulators against a local regtest from bitkit-docker (Lightning channels from its LND fixture, local Pubky homeserver), with requests entered in sats:
    • J1: the PIN pad reads "Please enter your PIN code to confirm this allowance change."; the payee gets "Allowance Added" with no review sheet and the payer's row turns Active; requests are paid automatically with no PIN; ending asks for the PIN and the row reads "Ended · $3.19 paid automatically".
    • J2: two requests are paid automatically, the third raises "Limit Reached" and opens the Payment Request sheet; "Paid automatically" reads $7.98.
    • J3: ended by the payer and, on a fresh allowance, by the payee; later requests are not paid and open as ordinary Payment Requests.
    • J4: "Payment Executed" posts, the row reads "· Auto-paid" and the allowance detail counts it.
  • Send to "Payment Executed" took 11 to 21 s over four requests, with no wait for a new payment list (bug: automatic allowance payment takes about 40 seconds after the request arrives #1614 measured on a local homeserver; most of it is the payer's next private-message poll).

Gaps

  • J2: Cap3 was not dismissed by its own id, since older pending requests shared the bell.
  • The journeys ran on a local regtest: the staging LSP no longer opens channels.

Models used

  • Planning/scoping: claude-opus-5-5 (reasoning: medium)
  • Implementation: claude-opus-5-5 (reasoning: medium)
  • Review: claude-sonnet-5-5 (reasoning: low)

@ovitrif
ovitrif force-pushed the fix/allowance-follow-ups branch from b6ee6e6 to 2dbeae7 Compare October 27, 2026 01:30
@ovitrif
ovitrif marked this pull request as ready for review October 27, 2026 03:54
@ovitrif
ovitrif requested review from a team, ben-kaufman and pwltr and removed request for a team October 27, 2026 03:54
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T03:58:01.096660Z 5dd04a8 Draft marked ready
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread app/src/main/java/to/bitkit/repositories/PaykitAllowanceRepo.kt
@greptile-apps

greptile-apps Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[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.

Findings

  1. P1 Canceled payment keeps allowance capacity ▶
  2. P2 Accepted offer goes unannounced ▶

Summary

This PR improves allowance request holds, payment totals, offer notices, and PIN wording.

  • Requests wait only when an allowance can still pay them.
  • Failed payments return their payment-list claims only after the claim is saved.
  • Automatic-payment totals and tags survive journal trimming.
  • Accepted allowance offers produce one notice, even after the app restarts.
  • The allowance PIN screen says what the PIN confirms.

Acknowledged scope:

  • ovitrif explicitly states that a failed save must keep the payment list unreleased; this behavior is intentional.
  • ovitrif states that allowances have not shipped. No supported upgrade requiring migration from the old journal-only totals was established.

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]
Loading

Reviews (1) · Last reviewed commit: "docs: show a request after an ended allo..." · Reviewed by Greptile

Comment thread app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/PaykitAllowanceRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt Outdated
@ovitrif

ovitrif commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

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 PaykitAllowanceExecutor.kt:618. The description is updated to match.

@ovitrif
ovitrif requested review from ben-kaufman and piotr-iohk and removed request for jvsena42 and pwltr October 27, 2026 17:01

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-293 and :308-310 releases nothing.
  • A failed save after the consume can't release twice, because the store keeps no memory cache.
  • The stranded-list fix from b15acda still 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.

Comment thread app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt Outdated
@ovitrif

ovitrif commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

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 PaykitAllowanceExecutor.kt:955.

@ovitrif
ovitrif requested a review from ben-kaufman October 27, 2026 18:37
Comment thread app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt
@ovitrif

ovitrif commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

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 PaykitAllowanceExecutor.kt:925). J3's payer step for an allowance that paid nothing now reads plain "Ended", matching iOS and the app.

@ovitrif
ovitrif requested a review from ben-kaufman October 27, 2026 19:10

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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

@piotr-iohk

Copy link
Copy Markdown
Collaborator

@ovitrif please resolve conflicts.

…w-ups

# Conflicts:
#	app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt
@ovitrif

ovitrif commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

please resolve conflicts.

@piotr-iohk Resolved in 2122f57 by merging master. The one conflict was in PaykitPaymentProofRepo.finishRelease: it keeps this PR's allowance release, which fails the request's ledger attempt first, and master's skip of the original-proof removal for Blocktank funding sends.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

piotr-iohk commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Device test results — scoped smoke

Passed at 2122f57dcdf0b330b8034ad9807d65be1e14f169, tested 28 October 2026 on the existing Android 16 emulator. Paired with iOS #1141 at f08577398630bc86750f19e1339bc34c8c95762d.

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:

  • After cold start, the linked contact appeared in Add Allowance without opening Contacts first.
  • Save displayed “Please enter your PIN code to confirm this allowance change.” Cancelling preserved $5/$50 and sent no offer; entering the test PIN saved it and both sides became Active.
  • An incoming offer was accepted without a recipient review or PIN. One “Allowance Added” system notification appeared; it did not repeat after restart.
  • Payer End used the allowance-change PIN wording. Cancelling kept Active; authorizing changed both copies to Ended.
  • Recipient short swipes cancelled; a full swipe ended the allowance on both copies without a payer PIN.
  • With the allowance Active, a BTC-priced 1,000-sat request accepting USDT only opened the manual payment sheet. Explicit swipe, fee confirmation and payment PIN completed the local-fork payment, received on iOS. Sent/received screens displayed 999 sats after conversion. “Paid Automatically” remained $0.00.
  • All four test allowances ended on both apps. The temporary payment-PIN requirement was restored to Off.

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 ben-kaufman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ovitrif

ovitrif commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

@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.

@ovitrif
ovitrif merged commit ed52ddb into master Oct 28, 2026
21 checks passed
@ovitrif
ovitrif deleted the fix/allowance-follow-ups branch October 28, 2026 15:11

Copy link
Copy Markdown
Collaborator

Device test results — automatic-payment follow-up

Passed at 2122f57dcdf0b330b8034ad9807d65be1e14f169 on 28 October 2026, Android 16 arm64 emulator. Paired with iOS #1141 at f3b582580e9ba665ae37219a2aea0e20974ba460.

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.

  • J4 / automatic under-limit: Android paid a BTC-priced 2,000-sat request without payer interaction. iOS received 2,000 sats once; the routing fee was 1 sat. Android posted “Payment Executed,” showed “AutoAndroidBase · Auto-paid,” and counted $1.61 in “Paid Automatically.”
  • Android also received three separate 2,000-sat automatic payments from iOS during its baseline and restart/recovery checks.
  • After force-stop and relaunch of both apps, the four request rows remained, Android’s automatic total stayed $1.61, and both channels’ sent/received/update counters were unchanged, with no pending HTLCs. No duplicate payment was observed.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants