Skip to content

feat: add allowances for automatic paykit payments - #1340

Open
ovitrif wants to merge 103 commits into
masterfrom
feat/paykit-allowances
Open

ovitrif wants to merge 103 commits into
masterfrom
feat/paykit-allowances

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1352
Closes #1658
Twin: synonymdev/bitkit-ios#799
Refs:

This PR adds allowances: a payer sets a per-payment and a monthly limit for a Paykit contact, and that contact's requests within the limits are paid without asking.

Description

It builds against Paykit 0.1.0, the version master pins. That release includes the allowance stack (pubky/paykit-rs#158 to #161) and the shared identity runtime, where one Encrypted Link serves a contact's identity and the allowance is bound to the two identities instead of to a receiver folder.

  • Adds an Allowances tab to Subscriptions, with an empty state, the list and a Set Allowance sheet with per-payment and monthly USD limits, so a payer can let a contact's requests pay themselves
  • Sends the allowance over the existing private Paykit link and opens a review sheet on the receiver, who accepts or declines; either side can end an allowance from its detail sheet
  • Asks the payer for the same PIN or biometrics as Send before setting, accepting or ending an allowance when a PIN is required for payments, and changes nothing when the check is cancelled; the payee's answers and automatic payments never ask, since the checked grant is what authorises them
  • Binds each allowance to the contact's identity, so one grant covers every Paykit app of that contact and there is one entry per grant, with no per-link proposals
  • Pays an incoming request within the limits automatically, over Lightning only: the request is reserved in the SDK's allowance accounting before the send and reported back afterwards, so a kill and relaunch mid-payment never pays twice
  • Claims the request's execution for Bitkit before accepting it automatically, and saves this install as the accepting install first, so an automatic payment that fails stays a payable manual request on this install under the shared-state ownership rules
  • Pays the endpoint the request is bound to and names the paying app in the payment proof, as the shared runtime requires
  • Pays only bitcoin-denominated requests automatically; a request priced in another asset stays an ordinary Payment Request, so USDT and USD-priced requests keep the manual flow, while a bitcoin request that also offers a USDT rail is paid over Lightning
  • Keeps a request manual when the payee offers only an on-chain endpoint, because an on-chain fee would fall outside every allowance limit
  • Caps the routing fee of an automatic Lightning payment at 1% of the request, since routing fees also fall outside the allowance's accounting
  • Keeps automatic payments out of the user's own Send flow: their success or failure never moves an open Send sheet, clears its contact or shows a send error, and a request the allowance is paying, or whose manual sheet is open, is never paid twice
  • Adds the allowance strings in every translated language
  • Pays automatically only under terms with both a per-payment and a monthly limit, so a contact's proposal without limits this wallet can show is never presented or accepted
  • Holds background Paykit delivery and contact import back while automatic payments run, as a manual payment submission does
  • Binds accepting an allowance to the received proposal's stream item, as accepting a Payment Request is
  • Keeps the allowance id of an automatic payment's pending proof in the payment state backup, so a proof restored from backup is still submitted with it
  • Leaves a request above the per-payment limit, over the monthly cap, or after an end as an ordinary Payment Request, with a Limit Reached notification for the payer and a Payment Executed notification for an automatic one
  • Tags automatically paid rows "Auto-paid" in the Payments tab and shows the amount paid automatically on the allowance detail
  • Waits for the payee's next private payment list instead of asking the payer when the previous list was used up, and holds automatic payments until the node has a usable channel, because both cases fell back to manual on regtest
  • Fixes automatic payments so an admitted payment finishes even when the refresh that started it is cancelled, because a cancelled send left the request accepted, unpaid and off the request sheet
  • Lets an explicit E2E homegate URL win on every backend, a dev-only change used to run the branch against a local Pubky homegate

Out of Scope

Design

Figma "Bitkit - Experimental New": Allowances empty state, Set Allowance, Allowances list. The receiver's review sheet and the detail sheet have no frame and follow the drawn film. Deliberate differences: the first tab stays "Overview", the list row's right column reads the monthly limit, and the copy typos are fixed.

Preview

Allowances empty Set Allowance Review on the payee Detail
Recording
side-by-side.mp4

QA Notes

Journeys

  • new set-and-accept.xml — the offer opens on the payee by itself and both rows turn Active
  • new auto-pay-under-limit.xml — a $2 request pays itself with only a Payment Executed notification
  • new above-limit-asks.xml — a $20 request arrives as an ordinary Payment Request
  • new monthly-cap-reached.xml — the third $4 request on a $10 cap raises Limit Reached and asks
  • new end-stops-auto-pay.xml — after either side ends it, the next request asks
  • new restart-never-pays-twice.xml — a kill right after the hand-off never leads to a second payment
  • new pin-gates-allowance-changes.xml — with a PIN required for payments, saving and ending ask for it, a cancel changes nothing, and the payee's accept and automatic payments never ask

Automated Checks

  • added PaykitAllowanceTest.kt — limits, terms, capacity and month anchoring of the allowance model, and status and capacity on real time while the subscription clock offset is set
  • added PaykitAllowanceExecutorTest.kt — reserve, hand-off, outcome recording and restart recovery of automatic payments, the execution claim before the automatic acceptance, and admission on real time while the subscription clock offset is set; a request in another asset or with only an on-chain endpoint stays manual before any acceptance; an uncertain Lightning send stays with the node; an unsaved journal stage stops the attempt before any send and gives back the consumed payment list; an on-chain attempt recovers as paid only on the wallet's positive evidence
  • added PaykitAllowancePayerTest.kt — a manual on-chain allowance payment counts as paid only when the wallet's send attempt for that request was accepted or observed, or a verified proof of its transaction is held
  • added PaykitAllowanceRepoTest.kt — admission decisions: automatic, manual, deferred and limit reached, one grant per contact identity, and coverage on real time while the subscription clock offset is set
  • added PrivatePaykitAllowancePaymentTest.kt — resolving the payee's request-bound private endpoint for an automatic payment
  • added AllowancesViewModelTest.kt — list, set, review and detail state; the payer's save and end wait for the PIN check when payments require it, a cancelled check changes nothing, and the payee's answers and the payer's decline never ask
  • updated PaykitPaymentRequestRepoTest.kt — an automatic acceptance saves this install as the owner and drops it on a definite failure
  • updated AppViewModelSendFlowTest.kt — the send flow with the allowance repo wired in
  • updated PaykitPaymentProofRepoTest.kt — payment proofs carry the allowance id and the paying app
  • updated PaykitPaymentStateBackupTest.kt — a pending proof keeps its allowance id through backup and restore
  • ran set-and-accept.xml, auto-pay-under-limit.xml, above-limit-asks.xml and end-stops-auto-pay.xml on two Android 15 emulators with fresh wallets against the Blocktank staging regtest LSP and the staging homegate, on 0b1f2664a, before the sync with master — all four pass; a $2 request paid itself over Lightning, a $20 request asked, and after an end the next request asked. Staging was slow: the automatic payment arrived about 5 minutes after the request, and two payment request sends timed out once before succeeding on retry. The review sheet opens on its own only when no intro sheet is on top, and the payer row of an ended allowance reads "Ended" where end-stops-auto-pay.xml expects "Ended · $0.00 paid automatically"; the journey text is not updated
  • not run: monthly-cap-reached.xml and restart-never-pays-twice.xml; the cap and the restart rules are covered by PaykitAllowanceExecutorTest.kt; not run yet: pin-gates-allowance-changes.xml, whose gate logic is covered by AllowancesViewModelTest.kt but whose PIN pad over the sheet is not checked on a device

@ovitrif ovitrif self-assigned this Sep 24, 2026
Comment thread app/src/test/java/to/bitkit/repositories/PaykitAllowanceRepoTest.kt Fixed
@ovitrif
ovitrif requested a review from coreyphillips October 21, 2026 14:50
coreyphillips
coreyphillips previously approved these changes Oct 21, 2026

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

Rechecked after 1 new commit (fb3a7ab). Nothing blocking remains.

addressed

  • Failed automatic payments leave the payment list consumed (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:592). Fixed: A node-reported failure of an automatic entry releases the journaled list version at PaykitAllowanceExecutor.kt:646, and recovery now does the same at :284. The release only resets the consumed version when that exact version is still consumed (PrivatePaykitRepo.kt:707), so the manual resolver's afterPrivatePaymentListVersion (:580-585) sees the list again. I confirmed this by reading the code; I did not run it.
  • Manual retry of a failed automatic invoice is treated as an automatic payment (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:218). Fixed: isAutomaticAllowancePayment (AppViewModel.kt:1578-1579) now defers to pendingPaymentRepo.isPending, and both handlers use it (:2592, :2689), so a pending manual retry reaches resolution. The new test manual retry of a failed automatic invoice still resolves as pending stubs both predicates true and verifies resolve, so it would fail without the fix.
  • Restart recovery fails a consumed automatic attempt without releasing its list (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:280). Fixed: Recovery calls releasePaymentList(entry) for an automatic entry it records as FAILED (PaykitAllowanceExecutor.kt:284). That covers both a SUBMITTED journal stage and a node-reported FAILED. recovery gives the payment list back when it fails an automatic attempt checks the release with version 4.
  • Requests sheet is not restored after the 'being paid automatically' toast (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:1582). Fixed: AppViewModel.kt:1586 captures shouldRestorePaymentRequestSheet before clearRequestedPaymentRequest(), and :1593 reopens Sheet.PaymentRequests, the same as the retry-exhausted path at :1878-1887. The new test asserts the sheet is back. It would fail without the line, because hiding the sheet leaves currentSheet null.
  • Manual retries can remain pending after settlement (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:222). Fixed: This is the same fix as point 3: while the manual retry is tracked as pending, both handlers fall through to pendingPaymentRepo.resolve. A narrower case after a process restart is filed separately as a new non-blocking finding.

still open

  • BTC requests from a payee with USDT enabled are never auto-paid (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:817). allowanceSats() requires pricing.conversion == null (PaykitAllowanceExecutor.kt:817, also used by coversRequest). A Bitkit payee with USDT on adds usdt-arbitrum to the accepted endpoints of every request (acceptedPaymentEndpointIdentifiers). The create screen does not filter methods by asset. PaykitRequestPricing.rates(BTC, ...) then returns a USDT rate, and buildPaymentRequestProposal attaches PaymentConversion.Fixed. So an ordinary BTC-denominated $2 request from such a payee always takes the manual flow, with no Limit Reached or other hint. That breaks the first Done-when for any payee with USDT enabled (publicPaykitUsdtEnabled defaults to true). USD-priced requests are already excluded by the asset check. Paying a BTC request over bolt11 does not use the conversion, so the guard looks broader than intended. No test covers a BTC request that carries a conversion. I could not check whether the SDK's automatic acceptance itself rejects such requests. allowanceSats() at PaykitAllowanceExecutor.kt:880-881 still rejects any request with a conversion, and publicPaykitUsdtEnabled still defaults to true (SettingsStore.kt:205). Deferring to #1658 is a reasonable call for a non-blocking point, but it does not show the point is wrong.
  • View-model fixes for the two blocking points have no tests (app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt:711). The author's reply says tests for each fix are in PaykitAllowanceExecutorTest.kt and PaykitAllowanceRepoTest.kt. Those do not exercise the view-model changes: - the early returns in handlePaymentSuccessful and handlePaymentFailed (points 1 and the matching failure path) - finishRequestedAutomaticPayment (point 2) - the filtered re-present check (point 4) - the manualIds exclusion in processAllowanceRequests AppViewModelSendFlowTest.kt stubs isAutomaticallyHandling to false and never stubs isAutomaticPayment. A regression in either blocking fix would pass CI. The early returns and finishRequestedAutomaticPayment now have tests that discriminate: without the return, notifyPaymentSentOnLightning would call findActivityByPaymentId and notifyPaymentFailed would enqueue a toast. The manualIds exclusion in processAllowanceRequests (AppViewModel.kt:1192-1198) and the re-present filter at :1570 are still untested. processIncomingRequests(any()) is only stubbed at test line 711, so a regression in that double-pay guard would still pass CI.

new since the last review

worth doing, does not block

  • After a restart, a manual retry of a failed automatic invoice settles silently (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:1578). After an app restart, a manual retry of a failed automatic invoice still hits the automatic early return, so the user gets no sent or failed notice. PendingPaymentRepo is in memory only (PendingPaymentRepo.kt:19, :31), so the new isPending guard holds only within one process. The chain: 1. The app is killed while the retry is pending (past SEND_LN_TIMEOUT). 2. After relaunch, the node replays PaymentSuccessful or PaymentFailed. 3. isPending is now false, and isAutomaticPayment still matches the old FAILED automatic entry. 4. handlePaymentSuccessful returns at AppViewModel.kt:2689 and skips notifyPaymentSentOnLightning (:2691), which an ordinary manual payment gets after a restart. handlePaymentFailed likewise skips notifyPaymentFailed (:2604). The allowance repo emits nothing either: lightningPaymentSettled emits PaidAutomatically only for automatic entries (PaykitAllowanceExecutor.kt:647), and this entry is manual. Funds and the ledger are correct; only the confirmation is lost. Letting the newest journal entry for the hash decide in isAutomaticPayment would cover this case too, because the manual entry is appended later and carries the hash via manualLightningPaymentSent. Found by reading; not reproduced on a device.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 2d5e2b5: three more fixes, matching what Codex reproduced on synonymdev/bitkit-ios#799 and found on Android too.

  • An automatic payment is announced as paid in one place only: the journal update that moves an automatic entry to succeeded. Settlement and recovery can no longer announce it twice.
  • Restart recovery now syncs attempts that ledger reconciliation had already settled. A succeeded one gets its contact link and notice; a failed one gives the payee's payment list back.
  • A request this wallet may cover is held from manual presentation while allowances are refreshing or requests are being admitted. This is checked right before its sheet shows, so the race between a new allowance and its first request can't open a manual sheet under an automatic payment.

The expired-grant point from the iOS review doesn't apply here: the local capacity check already counts only active allowances. Tests are in PaykitAllowanceExecutorTest.kt, PaykitAllowanceRepoTest.kt and AppViewModelSendFlowTest.kt. detekt, Android Lint, the Kotlin compiles and unit tests pass locally.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips thanks for the approval at fb3a7ab. The head has moved on since, with two pushes:

After a restart, a manual retry of a failed automatic invoice settles silently

Fixed as you suggested: isAutomaticPayment now lets the newest journal entry for the hash decide. A manual retry is journaled after the failed automatic attempt, so after a restart it still gets its sent or failed notice.

The manualIds exclusion in processAllowanceRequests and the re-present filter are still untested

Added in AppViewModelSendFlowTest.kt:

  • the allowance pass never takes a request whose manual sheet is being prepared
  • a pending covered request does not make presentation start over, which uses a subscription-period request with a pending link, your example

Each fails with its fix reverted.

#1658 stays deferred. Could you re-approve at c493635? detekt, Android Lint, the Kotlin compiles and unit tests pass locally.

@ovitrif
ovitrif requested a review from coreyphillips October 21, 2026 15:13

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

Rechecked after replies, with no new commit since fb3a7ab. One point still blocks a merge.

still open

  • BTC requests from a payee with USDT enabled are never auto-paid (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:817). allowanceSats() requires pricing.conversion == null (PaykitAllowanceExecutor.kt:817, also used by coversRequest). A Bitkit payee with USDT on adds usdt-arbitrum to the accepted endpoints of every request (acceptedPaymentEndpointIdentifiers). The create screen does not filter methods by asset. PaykitRequestPricing.rates(BTC, ...) then returns a USDT rate, and buildPaymentRequestProposal attaches PaymentConversion.Fixed. So an ordinary BTC-denominated $2 request from such a payee always takes the manual flow, with no Limit Reached or other hint. That breaks the first Done-when for any payee with USDT enabled (publicPaykitUsdtEnabled defaults to true). USD-priced requests are already excluded by the asset check. Paying a BTC request over bolt11 does not use the conversion, so the guard looks broader than intended. No test covers a BTC request that carries a conversion. I could not check whether the SDK's automatic acceptance itself rejects such requests. allowanceSats() at PaykitAllowanceExecutor.kt:880-881 still requires pricing.conversion == null, and coversRequest (PaykitAllowanceRepo.kt:320) still depends on it, so a BTC request from a payee with USDT on still asks. No reply addresses this. The Out of Scope line pointing at #1658 defers the point; it does not fix or rebut it. 2d5e2b5 does not touch allowanceSats either.
  • View-model fixes for the two blocking points have no tests (app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt:711). The author's reply says tests for each fix are in PaykitAllowanceExecutorTest.kt and PaykitAllowanceRepoTest.kt. Those do not exercise the view-model changes: - the early returns in handlePaymentSuccessful and handlePaymentFailed (points 1 and the matching failure path) - finishRequestedAutomaticPayment (point 2) - the filtered re-present check (point 4) - the manualIds exclusion in processAllowanceRequests AppViewModelSendFlowTest.kt stubs isAutomaticallyHandling to false and never stubs isAutomaticPayment. A regression in either blocking fix would pass CI. Part of this was already covered at fb3a7ab: AppViewModelSendFlowTest.kt:6364, :6385, :6406 and :6426 test both early returns, the isPending guard and finishRequestedAutomaticPayment. The manualIds exclusion in processAllowanceRequests (AppViewModel.kt:1192) is still untested, because processIncomingRequests(any()) is stubbed at :711 and never verified. The isAutomaticallyHandling filters at AppViewModel.kt:1547 and :1570 on the unrequested presentation path are also untested. The tests your comment mentions are in 2d5e2b5, which is not the head reviewed here.
  • After a restart, a manual retry of a failed automatic invoice settles silently (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:1578). After an app restart, a manual retry of a failed automatic invoice still hits the automatic early return, so the user gets no sent or failed notice. PendingPaymentRepo is in memory only (PendingPaymentRepo.kt:19, :31), so the new isPending guard holds only within one process. The chain: 1. The app is killed while the retry is pending (past SEND_LN_TIMEOUT). 2. After relaunch, the node replays PaymentSuccessful or PaymentFailed. 3. isPending is now false, and isAutomaticPayment still matches the old FAILED automatic entry. 4. handlePaymentSuccessful returns at AppViewModel.kt:2689 and skips notifyPaymentSentOnLightning (:2691), which an ordinary manual payment gets after a restart. handlePaymentFailed likewise skips notifyPaymentFailed (:2604). The allowance repo emits nothing either: lightningPaymentSettled emits PaidAutomatically only for automatic entries (PaykitAllowanceExecutor.kt:647), and this entry is manual. Funds and the ledger are correct; only the confirmation is lost. Letting the newest journal entry for the hash decide in isAutomaticPayment would cover this case too, because the manual entry is appended later and carries the hash via manualLightningPaymentSent. Found by reading; not reproduced on a device. isAutomaticAllowancePayment (AppViewModel.kt:1578) still relies on the in-memory PendingPaymentRepo.isPending. isAutomaticPayment (PaykitAllowanceExecutor.kt:218-222) still matches any automatic journal entry for the hash. After a relaunch, a manual retry therefore still returns at :2689 or :2592 without a sent or failed notice. No reply addresses this, and 2d5e2b5 shows both functions unchanged.

new since the last review

needs changing before merge

  • Defects the latest comment reports as fixed are still present at fb3a7ab (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:286). The three fixes announced in the 15:04 comment are not in the reviewed head. gh pr view 1340 still reports head fb3a7ab. git ls-remote shows feat/paykit-allowances at 2d5e2b5 (parent fb3a7ab), so that commit was not reviewed in this round. Each defect it describes can be seen in the code at fb3a7ab. Race with a new allowance: pendingRequests.drop(1).collect(::onPendingPaykitRequestsChanged) (AppViewModel.kt:1458) can run presentNextIncomingPaykitPaymentRequest while refreshIncomingPaykitPaymentRequests is still between the request refresh (:1175) and paykitAllowanceRepo.refresh() (:1181). In that window coversRequest reads a stale _allowances (PaykitAllowanceRepo.kt:322), so isAutomaticallyHandling returns false and a manual sheet starts for a covered request. Nothing checks again before the sheet shows. Depending on timing, manualIds (AppViewModel.kt:1192) either keeps the allowance off the request, so it asks instead of paying itself, or the sheet opens over an automatic payment already in flight. Recovery after reconciliation: recoverOpenAttempts hands verified outcomes to the SDK through ensureReconciled (PaykitAllowanceExecutor.kt:241-251). It then skips any attempt the reconciled history reports as SUCCEEDED or FAILED (:286). The journal entry stays at SENT or SUBMITTED and no Payment Executed or contact link is emitted. A failed automatic attempt keeps the payee's list consumed. The test stub returns the history unchanged (PaykitAllowanceExecutorTest.kt:127-136), so no test covers this. Double announcement: recoverOpenAttempts reads the journal entry (:279) without settleMutex, while settleOpenLightningAttempts and replayed node events settle the same hash under that mutex (:636). Both paths can emit PaidAutomatically. I found these by reading and did not run them. Without the SDK sources I could not check two things: whether the reconciled history marks those attempts as terminal, and whether recordPaymentOutcome accepts a repeat. None of these pays twice. The next round should check 2d5e2b5 against each of them.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 70482d0: merged master (#1637, a USDT review screen fix). Master only; @coreyphillips this is the head to approve. detekt, Android Lint, the Kotlin compiles and unit tests pass locally.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips your last two reviews ran on fb3a7ab because GitHub kept showing that head for a while after the pushes. The current head is 8a7b309, and it covers every point in them.

BTC requests from a payee with USDT enabled are never auto-paid

Fixed in 8a7b309. I checked Paykit v0.1.0's execution_wallet_checks (paykit-sdk/src/domain/allowance_accounting/execution.rs). For a Fixed conversion in automatic mode it looks up a rate for the paid endpoint (ConversionRate::for_endpoint). A bitcoin Lightning endpoint has no USDT rate, so it checks the plain bitcoin amount, and that passes. If the conversion also prices a bitcoin rail, the SDK blocks with WalletChecksFailed and the request stays manual (test_accounting_same_asset_rail_pricing_requires_manual_approval). So allowanceSats() now requires only a bitcoin amount, and a bitcoin request that also offers usdt-arbitrum is covered and auto-paid. This PR now closes #1658. The test is a bitcoin request that also offers a USDT rail is paid automatically.

The manualIds exclusion … and the isAutomaticallyHandling filters … are still untested

Tested since c493635 (#1340 (comment)), and each test fails with its fix reverted.

After a restart, a manual retry of a failed automatic invoice settles silently

Fixed in c493635: isAutomaticPayment lets the newest journal entry for the hash decide.

Defects the latest comment reports as fixed are still present at fb3a7ab

They are fixed in 2d5e2b5, which is in the current head:

  • presentation holds a request that may be covered while allowances update, checked right before the sheet shows;
  • recovery syncs attempts the ledger already settled;
  • PaidAutomatically fires only from the journal transition to succeeded, so it can't double.

Could you review 8a7b309? detekt, Android Lint, the Kotlin compiles and unit tests pass locally.

@ovitrif
ovitrif requested a review from coreyphillips October 21, 2026 15:26

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

Rechecked after 9 new commits (638ab48, c6068a1, 3bfde44, and 6 more). One point still blocks a merge.

addressed

  • BTC requests from a payee with USDT enabled are never auto-paid (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:817). Fixed: Fixed at PaykitAllowanceExecutor.kt:899: allowanceSats() now checks only the asset. I checked Paykit v0.1.0 execution_wallet_checks (execution.rs:14-20): the only rate PaykitRequestPricing.rates adds to a bitcoin request is usdt, so the bitcoin Lightning endpoint gets no rate, the SDK compares the plain bitcoin amount and the check passes. The manual flows accept the request before sending (AppViewModel.kt:6227), so the wider coverage can't take a request the user is paying over USDT.
  • View-model fixes for the two blocking points have no tests (app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt:711). Fixed: Fixed: the allowance pass never takes a request whose manual sheet is being prepared covers the manualIds filter (AppViewModel.kt:1192-1197), and a pending covered request does not make presentation start over covers the re-present filter (:1572). The early-return and in-progress tests at AppViewModelSendFlowTest.kt:6435-6515 were already there. I read each test against its branch but couldn't run them here (no GitHub Packages credentials); the CI build job passes at 8a7b309.
  • After a restart, a manual retry of a failed automatic invoice settles silently (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:1578). Fixed: Fixed at PaykitAllowanceExecutor.kt:221-226: the newest journal entry for the hash now decides. A manual retry is appended by journal() (:834-841), and manualLightningPaymentSent writes its hash before sendLightning runs (AppViewModel.kt:6119), so an event replayed after relaunch gets the normal sent or failed notice. The only retry left out is one the ledger refused to record, and that takes a ledger failure.

still open

  • Defects the latest comment reports as fixed are still present at fb3a7ab (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:286). The three fixes announced in the 15:04 comment are not in the reviewed head. gh pr view 1340 still reports head fb3a7ab. git ls-remote shows feat/paykit-allowances at 2d5e2b5 (parent fb3a7ab), so that commit was not reviewed in this round. Each defect it describes can be seen in the code at fb3a7ab. Race with a new allowance: pendingRequests.drop(1).collect(::onPendingPaykitRequestsChanged) (AppViewModel.kt:1458) can run presentNextIncomingPaykitPaymentRequest while refreshIncomingPaykitPaymentRequests is still between the request refresh (:1175) and paykitAllowanceRepo.refresh() (:1181). In that window coversRequest reads a stale _allowances (PaykitAllowanceRepo.kt:322), so isAutomaticallyHandling returns false and a manual sheet starts for a covered request. Nothing checks again before the sheet shows. Depending on timing, manualIds (AppViewModel.kt:1192) either keeps the allowance off the request, so it asks instead of paying itself, or the sheet opens over an automatic payment already in flight. Recovery after reconciliation: recoverOpenAttempts hands verified outcomes to the SDK through ensureReconciled (PaykitAllowanceExecutor.kt:241-251). It then skips any attempt the reconciled history reports as SUCCEEDED or FAILED (:286). The journal entry stays at SENT or SUBMITTED and no Payment Executed or contact link is emitted. A failed automatic attempt keeps the payee's list consumed. The test stub returns the history unchanged (PaykitAllowanceExecutorTest.kt:127-136), so no test covers this. Double announcement: recoverOpenAttempts reads the journal entry (:279) without settleMutex, while settleOpenLightningAttempts and replayed node events settle the same hash under that mutex (:636). Both paths can emit PaidAutomatically. I found these by reading and did not run them. Without the SDK sources I could not check two things: whether the reconciled history marks those attempts as terminal, and whether recordPaymentOutcome accepts a repeat. None of these pays twice. The next round should check 2d5e2b5 against each of them. Two of the three are fixed. syncSettledJournal (PaykitAllowanceExecutor.kt:297-302) copies attempts the ledger has settled into the journal and gives a failed attempt's payment list back. applyJournalStage (:378-395) announces only on the move to SUCCEEDED, under stateMutex, and Paykit's apply_outcome (history.rs:158-164) refuses to rewrite a terminal outcome. The race is still open in the window the point named. isUpdating goes up only inside PaykitAllowanceRepo.refresh() and payCovered. On a poll or a resume, the acceptance and a new request can arrive in one intake (PaykitPaymentRequestRepo.kt:1253). The pendingRequests collector then prepares a manual sheet against the stale PROPOSED allowance before AppViewModel.kt:1181 runs, and manualIds keeps the request manual. Holding from before :1172 through :1182 would close it.

new since the last review

worth doing, does not block

  • New recovery test passes with the recovery sync and announcement fix reverted (app/src/test/java/to/bitkit/repositories/PaykitAllowanceExecutorTest.kt:719). The new test recovery syncs an attempt the ledger already settled and announces it once passes with 2d5e2b5 reverted, so it guards neither the recovery sync nor the single announcement. At fb3a7ab, recoverOpenAttempts ignored an attempt the ledger reported as SUCCEEDED. After recover, the test calls lightningPaymentSettled, which moves the SENT entry to SUCCEEDED and emits PaidAutomatically once. The recordPaymentOutcome stub (PaykitAllowanceExecutorTest.kt:201) accepts any report, so both assertions hold with or without the fix. Asserting the journal stage and the event right after sut.recover(identity), before any node event, would fail without syncSettledJournal. The FAILED branch, which gives the payee's payment list back (PaykitAllowanceExecutor.kt:301), has no test. I traced this by reading the test against the fb3a7ab executor. I couldn't run it because the build needs GitHub Packages credentials that aren't set up here.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips thanks, all three are handled in d6b83b5.

The race is still open in the window the point named … Holding from before :1172 through :1182 would close it.

Fixed as you suggested. refreshIncomingPaykitPaymentRequests now takes a coverage hold (PaykitAllowanceRepo.beginCoverageUpdate()) before the request refresh and releases it only after paykitAllowanceRepo.refresh() has run. So a request that arrives in the same intake as its allowance's acceptance is held, not given a manual sheet against the stale PROPOSED allowance. The allowance pass and presentation then run after the hold ends. The hold is released once even if the refresh throws. Tests: coverage is held from the request refresh until the allowances are refreshed and a coverage update holds until it is released once; both fail with the change reverted.

New recovery test passes with the recovery sync and announcement fix reverted … The FAILED branch … has no test.

Fixed. recovery syncs an attempt the ledger already settled and announces it once now asserts the journal stage and the PaidAutomatically event right after recover, before any node event. The new recovery syncs an attempt the ledger already failed and gives its payment list back covers the FAILED branch and fails without syncSettledJournal.

Also in this push: a payment list could be released twice. When an automatic Lightning payment fails before it leaves, failUnsent released the list in memory, and a node PaymentFailed event for the same hash, handled while the entry was still SENDING, released the journaled version again. If another payment had consumed that version in between, the second release cleared its claim. The executor now takes the version out of the journal entry before any release, so each consumed list is released at most once (as iOS does in synonymdev/bitkit-ios#799). Test: a payment list given back by a failed send is not given back again by the node's failure event; it fails without the fix.

Could you review d6b83b5? detekt, Android Lint, the Kotlin compiles and unit tests pass locally.

@ovitrif
ovitrif requested a review from coreyphillips October 21, 2026 16:48
Comment thread app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt Outdated
coreyphillips
coreyphillips previously approved these changes Oct 21, 2026

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

Rechecked after 2 new commits (d6b83b5, 480ae3a). Nothing blocking remains.

addressed

  • Defects the latest comment reports as fixed are still present at fb3a7ab (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:286). Fixed: The named window is now held from before the request refresh through paykitAllowanceRepo.refresh() (AppViewModel.kt:1174-1189) and rechecked before each sheet (:1567). Ledger-settled attempts sync into the journal and give a failed list back (PaykitAllowanceExecutor.kt:290-302), and applyJournalStage (:384-400) announces only the move to SUCCEEDED under stateMutex. I confirmed this by reading, not by a build, and filed the follow-up intake at AppViewModel.kt:1191 separately.
  • New recovery test passes with the recovery sync and announcement fix reverted (app/src/test/java/to/bitkit/repositories/PaykitAllowanceExecutorTest.kt:719). Fixed: PaykitAllowanceExecutorTest.kt:758-763 now asserts SUCCEEDED and one PaidAutomatically right after recover, and :770-787 covers the FAILED branch. With those ledger statuses mapped back to Unit, the journal stays SENT and both tests fail; I traced this by reading because the build needs package credentials that are not set up here.

new since the last review

worth doing, does not block

  • Opened requests wait out the network refresh and stay hidden if it fails (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:1190). The coverage hold taken at AppViewModel.kt:1174 now spans the network request intake, so a request the user taps is not shown until that refresh ends. After a failed refresh it is not shown until a later one succeeds. isAllowancePending (:1589) holds every pending request whose contact has an allowance this wallet grants, in any state (mayCover, PaykitAllowanceRepo.kt:223). That includes the request in requestedPaymentRequestId. Tapping it runs presentNextIncomingPaykitPaymentRequest from openIncomingPaymentRequestWithTags (:7721). The filter at :1557 drops it, and the function returns at :1559 without scheduling a retry. The hold now lasts through fetchRequestSnapshot (receivePrivateMessagesFromLinkedPeers, observeState), which runs on every 10 second poll (:1316). After the release, presentation runs only inside result.onSuccess (:1190), so a failed refresh leaves the tapped request unopened. At 8a7b309 the hold covered only the local allowance listing and payCovered. A likely case is a payer who gets Limit Reached and taps the request: the Payment Requests sheet closes and nothing opens. I found this by reading and could not run it, because the build needs GitHub Packages credentials that are not set up here. Presenting after the release on failure too would close the gap, since the request state did not change then.
  • Follow-up request refresh after an automatic payment intakes outside the hold (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:1191). The paykitPaymentRequestRepo.refresh() at AppViewModel.kt:1191 is a FULL intake that runs after the hold is released, and no allowance refresh follows it. So the case d6b83b5 closes can still happen there. This refresh runs whenever payCovered started an automatic payment. Suppose contact Y's allowance acceptance and a new request from Y land in that intake. Then onPendingPaykitRequestsChanged and the call at :1192 present the request against Y's stale PROPOSED allowance, because isUpdating is false and coversRequest is false. The open sheet then puts the request in manualIds (:1201), so it asks instead of paying itself. synchronizeAfterSubscriptionAction (PaykitPaymentRequestRepo.kt:1651) is a second FULL intake outside the hold. Nothing is paid twice. I traced this by reading and did not run it. Holding coverage around the follow-up refresh, and refreshing allowances before the release, would cover it.
  • A failed claim save permits duplicate payment-list release (app/src/main/java/to/bitkit/repositories/PaykitAllowanceExecutor.kt:871). If saving the cleared version fails, takePaymentListVersion() still returns the entry and permits release. updateLocalState() swallows the save error, leaving the stored version available to another cleanup path. I reproduced this with the current cleanup and settlement methods in isolation, failing the first journal save. After the first release, another payment consumed the same version. A node failure event then caused a second release and cleared that payment's claim. The claim needs to be saved successfully before cleanup proceeds.

nits

  • Test for a failure event after a failed send never reaches the race it names (app/src/test/java/to/bitkit/repositories/PaykitAllowanceExecutorTest.kt:582). This test runs lightningPaymentSettled only after autoPay returns, so it never reaches the interleaving its name describes. By then failUnsent has recorded FAILED. lightningPaymentSettled matches only SETTLEABLE_STAGES, so it finds no entry, and verify(payer, times(1)) holds with or without the take. Only the assertNull on the journal's paymentListVersion fails at 8a7b309. The order d6b83b5 describes is an event handled between failUnsent's release and its record, and no test covers it. The 480ae3a test covers the reverse order. Settling the failure from inside a releasePaymentList answer would reach it.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips thanks for the approval. The four non-blocking points are now in #1659 (items 4 to 7) for the next release, with your notes on each, so this PR's head stays at 480ae3a for the merge.

@ovitrif
ovitrif enabled auto-merge October 21, 2026 18:42
@ovitrif
ovitrif disabled auto-merge October 21, 2026 18:43
@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@pwltr your MEDIUM in #1340 (comment) is fixed in 480ae3a, and @coreyphillips approved that head. The branch ruleset still needs one more approval because two older commits on the branch are unsigned. Could you approve 480ae3a if it looks good to you?

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: allowances never auto-pay bitcoin requests from a payee with USDT enabled feat: add allowances for automatic paykit payments

7 participants