Repository navigation
Conversation
0dde3eb to
145330c
Compare
coreyphillips
left a comment
There was a problem hiding this comment.
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'safterPrivatePaymentListVersion(: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 topendingPaymentRepo.isPending, and both handlers use it (:2592,:2689), so a pending manual retry reaches resolution. The new testmanual retry of a failed automatic invoice still resolves as pendingstubs both predicates true and verifiesresolve, 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 attemptchecks 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:1586capturesshouldRestorePaymentRequestSheetbeforeclearRequestedPaymentRequest(), and:1593reopensSheet.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 leavescurrentSheetnull. - 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()requirespricing.conversion == null(PaykitAllowanceExecutor.kt:817, also used bycoversRequest). A Bitkit payee with USDT on addsusdt-arbitrumto the accepted endpoints of every request (acceptedPaymentEndpointIdentifiers). The create screen does not filter methods by asset.PaykitRequestPricing.rates(BTC, ...)then returns a USDT rate, andbuildPaymentRequestProposalattachesPaymentConversion.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 (publicPaykitUsdtEnableddefaults 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()atPaykitAllowanceExecutor.kt:880-881still rejects any request with a conversion, andpublicPaykitUsdtEnabledstill 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.ktandPaykitAllowanceRepoTest.kt. Those do not exercise the view-model changes: - the early returns inhandlePaymentSuccessfulandhandlePaymentFailed(points 1 and the matching failure path) -finishRequestedAutomaticPayment(point 2) - the filtered re-present check (point 4) - themanualIdsexclusion inprocessAllowanceRequestsAppViewModelSendFlowTest.ktstubsisAutomaticallyHandlingto false and never stubsisAutomaticPayment. A regression in either blocking fix would pass CI. The early returns andfinishRequestedAutomaticPaymentnow have tests that discriminate: without the return,notifyPaymentSentOnLightningwould callfindActivityByPaymentIdandnotifyPaymentFailedwould enqueue a toast. ThemanualIdsexclusion inprocessAllowanceRequests(AppViewModel.kt:1192-1198) and the re-present filter at:1570are 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.
PendingPaymentRepois in memory only (PendingPaymentRepo.kt:19,:31), so the newisPendingguard holds only within one process. The chain: 1. The app is killed while the retry is pending (pastSEND_LN_TIMEOUT). 2. After relaunch, the node replaysPaymentSuccessfulorPaymentFailed. 3.isPendingis now false, andisAutomaticPaymentstill matches the old FAILED automatic entry. 4.handlePaymentSuccessfulreturns atAppViewModel.kt:2689and skipsnotifyPaymentSentOnLightning(:2691), which an ordinary manual payment gets after a restart.handlePaymentFailedlikewise skipsnotifyPaymentFailed(:2604). The allowance repo emits nothing either:lightningPaymentSettledemitsPaidAutomaticallyonly 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 inisAutomaticPaymentwould cover this case too, because the manual entry is appended later and carries the hash viamanualLightningPaymentSent. Found by reading; not reproduced on a device.
|
Pushed 2d5e2b5: three more fixes, matching what Codex reproduced on synonymdev/bitkit-ios#799 and found on Android too.
The expired-grant point from the iOS review doesn't apply here: the local capacity check already counts only active allowances. Tests are in |
|
@coreyphillips thanks for the approval at fb3a7ab. The head has moved on since, with two pushes:
Fixed as you suggested:
Added in
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. |
coreyphillips
left a comment
There was a problem hiding this comment.
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()requirespricing.conversion == null(PaykitAllowanceExecutor.kt:817, also used bycoversRequest). A Bitkit payee with USDT on addsusdt-arbitrumto the accepted endpoints of every request (acceptedPaymentEndpointIdentifiers). The create screen does not filter methods by asset.PaykitRequestPricing.rates(BTC, ...)then returns a USDT rate, andbuildPaymentRequestProposalattachesPaymentConversion.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 (publicPaykitUsdtEnableddefaults 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()atPaykitAllowanceExecutor.kt:880-881still requirespricing.conversion == null, andcoversRequest(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 touchallowanceSatseither. - 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.ktandPaykitAllowanceRepoTest.kt. Those do not exercise the view-model changes: - the early returns inhandlePaymentSuccessfulandhandlePaymentFailed(points 1 and the matching failure path) -finishRequestedAutomaticPayment(point 2) - the filtered re-present check (point 4) - themanualIdsexclusion inprocessAllowanceRequestsAppViewModelSendFlowTest.ktstubsisAutomaticallyHandlingto false and never stubsisAutomaticPayment. A regression in either blocking fix would pass CI. Part of this was already covered at fb3a7ab:AppViewModelSendFlowTest.kt:6364,:6385,:6406and:6426test both early returns, theisPendingguard andfinishRequestedAutomaticPayment. ThemanualIdsexclusion inprocessAllowanceRequests(AppViewModel.kt:1192) is still untested, becauseprocessIncomingRequests(any())is stubbed at:711and never verified. TheisAutomaticallyHandlingfilters atAppViewModel.kt:1547and:1570on 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.
PendingPaymentRepois in memory only (PendingPaymentRepo.kt:19,:31), so the newisPendingguard holds only within one process. The chain: 1. The app is killed while the retry is pending (pastSEND_LN_TIMEOUT). 2. After relaunch, the node replaysPaymentSuccessfulorPaymentFailed. 3.isPendingis now false, andisAutomaticPaymentstill matches the old FAILED automatic entry. 4.handlePaymentSuccessfulreturns atAppViewModel.kt:2689and skipsnotifyPaymentSentOnLightning(:2691), which an ordinary manual payment gets after a restart.handlePaymentFailedlikewise skipsnotifyPaymentFailed(:2604). The allowance repo emits nothing either:lightningPaymentSettledemitsPaidAutomaticallyonly 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 inisAutomaticPaymentwould cover this case too, because the manual entry is appended later and carries the hash viamanualLightningPaymentSent. Found by reading; not reproduced on a device.isAutomaticAllowancePayment(AppViewModel.kt:1578) still relies on the in-memoryPendingPaymentRepo.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:2689or:2592without 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 1340still reports head fb3a7ab.git ls-remoteshowsfeat/paykit-allowancesat 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 runpresentNextIncomingPaykitPaymentRequestwhilerefreshIncomingPaykitPaymentRequestsis still between the request refresh (:1175) andpaykitAllowanceRepo.refresh()(:1181). In that windowcoversRequestreads a stale_allowances(PaykitAllowanceRepo.kt:322), soisAutomaticallyHandlingreturns 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:recoverOpenAttemptshands verified outcomes to the SDK throughensureReconciled(PaykitAllowanceExecutor.kt:241-251). It then skips any attempt the reconciled history reports asSUCCEEDEDorFAILED(: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:recoverOpenAttemptsreads the journal entry (:279) withoutsettleMutex, whilesettleOpenLightningAttemptsand replayed node events settle the same hash under that mutex (:636). Both paths can emitPaidAutomatically. 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 whetherrecordPaymentOutcomeaccepts a repeat. None of these pays twice. The next round should check 2d5e2b5 against each of them.
|
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. |
|
@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.
Fixed in 8a7b309. I checked Paykit v0.1.0's
Tested since c493635 (#1340 (comment)), and each test fails with its fix reverted.
Fixed in c493635:
They are fixed in 2d5e2b5, which is in the current head:
Could you review 8a7b309? detekt, Android Lint, the Kotlin compiles and unit tests pass locally. |
coreyphillips
left a comment
There was a problem hiding this comment.
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.0execution_wallet_checks(execution.rs:14-20): the only ratePaykitRequestPricing.ratesadds to a bitcoin request isusdt, 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 preparedcovers themanualIdsfilter (AppViewModel.kt:1192-1197), anda pending covered request does not make presentation start overcovers the re-present filter (:1572). The early-return and in-progress tests atAppViewModelSendFlowTest.kt:6435-6515were 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 byjournal()(:834-841), andmanualLightningPaymentSentwrites its hash beforesendLightningruns (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 1340still reports head fb3a7ab.git ls-remoteshowsfeat/paykit-allowancesat 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 runpresentNextIncomingPaykitPaymentRequestwhilerefreshIncomingPaykitPaymentRequestsis still between the request refresh (:1175) andpaykitAllowanceRepo.refresh()(:1181). In that windowcoversRequestreads a stale_allowances(PaykitAllowanceRepo.kt:322), soisAutomaticallyHandlingreturns 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:recoverOpenAttemptshands verified outcomes to the SDK throughensureReconciled(PaykitAllowanceExecutor.kt:241-251). It then skips any attempt the reconciled history reports asSUCCEEDEDorFAILED(: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:recoverOpenAttemptsreads the journal entry (:279) withoutsettleMutex, whilesettleOpenLightningAttemptsand replayed node events settle the same hash under that mutex (:636). Both paths can emitPaidAutomatically. 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 whetherrecordPaymentOutcomeaccepts 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, understateMutex, and Paykit'sapply_outcome(history.rs:158-164) refuses to rewrite a terminal outcome. The race is still open in the window the point named.isUpdatinggoes up only insidePaykitAllowanceRepo.refresh()andpayCovered. On a poll or a resume, the acceptance and a new request can arrive in one intake (PaykitPaymentRequestRepo.kt:1253). ThependingRequestscollector then prepares a manual sheet against the stale PROPOSED allowance beforeAppViewModel.kt:1181runs, andmanualIdskeeps the request manual. Holding from before:1172through:1182would 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 oncepasses with 2d5e2b5 reverted, so it guards neither the recovery sync nor the single announcement. At fb3a7ab,recoverOpenAttemptsignored an attempt the ledger reported as SUCCEEDED. Afterrecover, the test callslightningPaymentSettled, which moves the SENT entry to SUCCEEDED and emitsPaidAutomaticallyonce. TherecordPaymentOutcomestub (PaykitAllowanceExecutorTest.kt:201) accepts any report, so both assertions hold with or without the fix. Asserting the journal stage and the event right aftersut.recover(identity), before any node event, would fail withoutsyncSettledJournal. 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.
…a payment list once
|
@coreyphillips thanks, all three are handled in d6b83b5.
Fixed as you suggested.
Fixed. Also in this push: a payment list could be released twice. When an automatic Lightning payment fails before it leaves, Could you review d6b83b5? detekt, Android Lint, the Kotlin compiles and unit tests pass locally. |
coreyphillips
left a comment
There was a problem hiding this comment.
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), andapplyJournalStage(:384-400) announces only the move to SUCCEEDED understateMutex. I confirmed this by reading, not by a build, and filed the follow-up intake atAppViewModel.kt:1191separately. - 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-763now asserts SUCCEEDED and onePaidAutomaticallyright afterrecover, and:770-787covers the FAILED branch. With those ledger statuses mapped back toUnit, 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:1174now 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 inrequestedPaymentRequestId. Tapping it runspresentNextIncomingPaykitPaymentRequestfromopenIncomingPaymentRequestWithTags(:7721). The filter at:1557drops it, and the function returns at:1559without scheduling a retry. The hold now lasts throughfetchRequestSnapshot(receivePrivateMessagesFromLinkedPeers,observeState), which runs on every 10 second poll (:1316). After the release, presentation runs only insideresult.onSuccess(:1190), so a failed refresh leaves the tapped request unopened. At 8a7b309 the hold covered only the local allowance listing andpayCovered. 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()atAppViewModel.kt:1191is 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 wheneverpayCoveredstarted an automatic payment. Suppose contact Y's allowance acceptance and a new request from Y land in that intake. ThenonPendingPaykitRequestsChangedand the call at:1192present the request against Y's stale PROPOSED allowance, becauseisUpdatingis false andcoversRequestis false. The open sheet then puts the request inmanualIds(: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
lightningPaymentSettledonly afterautoPayreturns, so it never reaches the interleaving its name describes. By thenfailUnsenthas recorded FAILED.lightningPaymentSettledmatches onlySETTLEABLE_STAGES, so it finds no entry, andverify(payer, times(1))holds with or without the take. Only theassertNullon the journal'spaymentListVersionfails at 8a7b309. The order d6b83b5 describes is an event handled betweenfailUnsent's release and itsrecord, and no test covers it. The 480ae3a test covers the reverse order. Settling the failure from inside areleasePaymentListanswer would reach it.
|
@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. |
|
@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? |
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.Out of Scope
PaykitAllowanceExecutor.ktrecovery: a send stopped by a process kill stays at SENDING, so its request neither pays nor asks; follow-upPaykitAllowanceExecutor.ktrecovery: it settles every open attempt in the identity-wide ledger, which assumes Bitkit is the only app paying from it; follow-up before another app shares the ledgerLightningRepo.ktsync(): a cancellation is recorded as a sync error and starts the retry loop instead of being rethrown; follow-upPaykitAllowanceStore.ktbackup: the allowance journal and automatic-payment history are not in the wallet backup, so a restore loses them; tracked in bug: allowance history and open attempts are lost after a wallet restore #1583 (thread)PaykitAllowanceExecutor.ktjournal trim: totals and Auto-paid tags are rebuilt from the last 200 journal entries; tracked in bug: older auto-paid totals and tags disappear after 200 allowance payments #1584 (thread)PaykitAllowanceExecutor.kton-chain auto-pay: an on-chain request stays manual until fees count against the allowance limits; tracked in feat: pay allowance requests on-chain once fees count against the limits #1592 (thread)PaykitAllowanceExecutor.ktrecovery: a manual on-chain attempt whose send was released stays unknown in the ledger, since a release leaves no durable record to read; tracked in bug: a released on-chain allowance payment keeps holding allowance capacity #1585 (thread)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
side-by-side.mp4
QA Notes
Journeys
set-and-accept.xml— the offer opens on the payee by itself and both rows turn Activeauto-pay-under-limit.xml— a $2 request pays itself with only a Payment Executed notificationabove-limit-asks.xml— a $20 request arrives as an ordinary Payment Requestmonthly-cap-reached.xml— the third $4 request on a $10 cap raises Limit Reached and asksend-stops-auto-pay.xml— after either side ends it, the next request asksrestart-never-pays-twice.xml— a kill right after the hand-off never leads to a second paymentpin-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 askAutomated Checks
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 setPaykitAllowanceExecutorTest.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 evidencePaykitAllowancePayerTest.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 heldPaykitAllowanceRepoTest.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 setPrivatePaykitAllowancePaymentTest.kt— resolving the payee's request-bound private endpoint for an automatic paymentAllowancesViewModelTest.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 askPaykitPaymentRequestRepoTest.kt— an automatic acceptance saves this install as the owner and drops it on a definite failureAppViewModelSendFlowTest.kt— the send flow with the allowance repo wired inPaykitPaymentProofRepoTest.kt— payment proofs carry the allowance id and the paying appPaykitPaymentStateBackupTest.kt— a pending proof keeps its allowance id through backup and restoreset-and-accept.xml,auto-pay-under-limit.xml,above-limit-asks.xmlandend-stops-auto-pay.xmlon two Android 15 emulators with fresh wallets against the Blocktank staging regtest LSP and the staging homegate, on0b1f2664a, 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" whereend-stops-auto-pay.xmlexpects "Ended · $0.00 paid automatically"; the journey text is not updatedmonthly-cap-reached.xmlandrestart-never-pays-twice.xml; the cap and the restart rules are covered byPaykitAllowanceExecutorTest.kt; not run yet:pin-gates-allowance-changes.xml, whose gate logic is covered byAllowancesViewModelTest.ktbut whose PIN pad over the sheet is not checked on a device