Repository navigation
feat: accept allowance offers automatically on the allowee's wallet - #841
Conversation
1a0051f to
8f0449c
Compare
b0c583e to
80f5c0f
Compare
|
Pushed 0b8e352: this PR now sits on the updated #799 (merge 8f0449c, Paykit SDK rc59 with identity-wide shared state).
|
|
Pushed 14307a0: merged the updated #799 (a19ea87: the request-presentation and covered-request fixes and more of Ben's shared-runtime merges) into this PR; the merge was clean and nothing in the auto-accept code changed. Local verification: the Paykit and allowance suites on an iPhone 17 simulator, 382 tests, 0 failures. |
|
Pushed 5f65a58: merged the current #799 head (2073233) into this PR, which brings the shared Paykit runtime merges and the subscription-notification and contact-loading fixes. The merge was clean.
|
|
Pushed cebcc55: merged the updated #799 (ea65e76, now level with
|
|
Pushed 8de56f4: merged the updated #799 (47ed839, which counts an automatic on-chain allowance payment only once it is accepted). The merge was clean; this PR's offer acceptance does not touch the changed payer interface.
|
|
Pushed 5143187: merged the updated #799 (10abf2e: the payment PIN gate for granting and ending an allowance, automatic allowance payments over Lightning only, and both limits required), plus a51aca5 for
|
QA:
|
|
Pushed 8e476fc: merged the updated #799 (b18cb79:
|
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 10 new commits (da90209, 7cb14fb, fd4c701, and 7 more). Nothing blocking remains.
addressed
- Offer accepted as the scene goes inactive is never announced (Bitkit/Services/PaykitAllowanceManager.swift:276). Fixed:
PaykitAllowanceManager.swift:268now breaks out of the loop instead of returning[].:277gates the result only on identity andcanAnnounce, andAppScene.swift:1425-1429passescanAnnounce: isAutomaticSession(session and deletion, nocanRefresh()), so scene inactivity or poll cancellation no longer drops the notice. I also checked thatwithStateRevisionTracking(PubkyService.swift:1562-1640) does not throw on cancellation after the write, and thatPaykitAllowanceNotifier.postruns in an unstructuredTask. Code reading only: no Xcode tooling was available to run the tests. - Allowances list still opens Accept/Decline for an unanswered offer (Bitkit/Views/Subscriptions/AllowancesView.swift:27). Fixed:
AllowancesView.swift:28-29opens the review sheet only for an entry holding an ask, matchingproposalForPresentationatPaykitAllowanceManager.swift:312; the auto-present path atAppScene.swift:224uses that same check. An offer now opensAllowanceDetailView, whose swipe needscanEnd, and that is false for a received proposal (PaykitAllowance.swift:143-149). So nothing races the background accept. - New strings ship English only despite the same-PR translation rule (Bitkit/Resources/Localization/en.lproj/Localizable.strings:1824). Fixed: Both
subscriptions__allowance_offer_accepted_*keys are present in all 14 non-EnglishLocalizable.strings, using the allowance terms the base introduced.node scripts/validate-translations.jsreports 0 errors.
still open
- Automatic acceptance can write after profile deletion begins (Bitkit/Services/PaykitAllowanceManager.swift:270). An offer can be accepted after profile deletion starts. The session predicate is checked before
accept, but the SDK call can suspend on its lock or backup revision read before writing. A standalone test using these methods and a delayed revision read still performed acceptance after the deletion flag became true. The final check suppresses only the announcement. The response agrees the race is real, and the code is unchanged. Saying it belongs in #799 or the SDK defers it rather than showing it does not apply, and the PR body still says acceptance never happens while the profile is being deleted. An in-app fence looks feasible: checkPrivatePaykitService.shared.isDeletingProfileinside the SDK lock, asWalletViewModel.swift:1418does, sincebeginProfileDeletion()sets it before deletion's SDK work. Otherwise track it asCLAUDE.mdasks: an issue plus an Out of Scope bullet.
new since the last review
worth doing, does not block
- Offer detail says "Waiting for your answer" but offers no way to answer (Bitkit/Views/Subscriptions/AllowancesView.swift:766). Since 8e47d17, tapping an unaccepted offer opens
AllowanceDetailView, which reads "Waiting for your answer" yet shows no Accept, Decline or swipe. The text comes fromexplanationatBitkit/Views/Subscriptions/AllowancesView.swift:766, and the list row shows the same string at:119. The swipe is hidden becausecanEndis false for a received proposal (PaykitAllowance.swift:143-149). The state shows while an offer waits for its first accept pass, and for as long as the background accept keeps failing; the PR notes the first private link can delay it by a minute or two. If an accept fails every time, the allowee can no longer refuse or remove the row, because Decline is gone and the row is retried on every poll. I did not find an SDK error that fails permanently, so that last case is unconfirmed. A status forisOfferFromAllowerthat says the wallet is setting the allowance up, rather than asking for an answer, would match the new flow. Any new string needs translating into all 14 languages. - Recovery can duplicate the payment notification (Bitkit/Services/PaykitAllowanceExecutor.swift:389). An automatic payment that settles during recovery can get two "Payment Executed" notices.
wasSettledis captured beforerecordawaits, so the settlement handler can select the same unfinished entry and both paths can announce it. Pausing recovery's outcome write and delivering settlement reproduced two events on HEAD versus one at97d86f86, using production methods and an idempotent SDK mock. The running app was not exercised.
|
Pushed 2782494: merged the updated #799 (efe4aa2: automatic and manual payments of one request stay apart). The one conflict, in
|
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 2 new commits (efe4aa2, 2782494). Nothing blocking remains.
addressed
- Recovery can duplicate the payment notification (Bitkit/Services/PaykitAllowanceExecutor.swift:389). Fixed: The merged base commit efe4aa2 moves the announcement into
applyJournalOutcome(PaykitAllowanceExecutor.swift:474-493).recordand the recovery.succeededbranch at :390 both call it. It decides inside the synchronousupdateLocalStateclosure (:313) whether this call moved the entry to.succeeded, with no suspension between check and set. So of two concurrentrecordcalls only the first to resume on the actor announces. I confirmed this by reading the code, not by running it, since this machine has no Xcode.
still open
- Automatic acceptance can write after profile deletion begins (Bitkit/Services/PaykitAllowanceManager.swift:270). An offer can be accepted after profile deletion starts. The session predicate is checked before
accept, but the SDK call can suspend on its lock or backup revision read before writing. A standalone test using these methods and a delayed revision read still performed acceptance after the deletion flag became true. The final check suppresses only the announcement.acceptOffersFromAllowersatPaykitAllowanceManager.swift:276-286is unchanged. Deletion is still only checked beforeacceptsuspends and again before announcing, and no response since the review addresses this. The point stands as a non-blocking race. - Offer detail says "Waiting for your answer" but offers no way to answer (Bitkit/Views/Subscriptions/AllowancesView.swift:766). Since 8e47d17, tapping an unaccepted offer opens
AllowanceDetailView, which reads "Waiting for your answer" yet shows no Accept, Decline or swipe. The text comes fromexplanationatBitkit/Views/Subscriptions/AllowancesView.swift:766, and the list row shows the same string at:119. The swipe is hidden becausecanEndis false for a received proposal (PaykitAllowance.swift:143-149). The state shows while an offer waits for its first accept pass, and for as long as the background accept keeps failing; the PR notes the first private link can delay it by a minute or two. If an accept fails every time, the allowee can no longer refuse or remove the row, because Decline is gone and the row is retried on every poll. I did not find an SDK error that fails permanently, so that last case is unconfirmed. A status forisOfferFromAllowerthat says the wallet is setting the allowance up, rather than asking for an answer, would match the new flow. Any new string needs translating into all 14 languages. A received offer still maps to.awaitingMyAnswer(PaykitAllowance.swift:121). The row (AllowancesView.swift:119-120) and detail (AllowancesView.swift:766-767) still say "Waiting for your answer", but the detail shows no Accept, Decline or swipe becausecanEndis false for a received proposal (PaykitAllowance.swift:144-147). No status forisOfferFromAllowerhas been added.
|
@coreyphillips thanks for the re-check. All three open points are fixed as of 60444d8.
Fixed in 26f826e with the fence you suggested.
Fixed in 8c06355. An allower's offer that is still being accepted now reads "Setting up the allowance" in both the list row and the detail sheet. An ask still reads "Waiting for your answer". The new string is translated into all 14 languages, and
Fixed in #799's efe4aa2, which this PR has merged since your review of b64f755: Payment Executed is now raised only where the executor moves an automatic entry to succeeded, so settlement and recovery cannot both announce it, and
|
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 5 new commits (69da189, 26f826e, 8c06355, and 2 more). One point still blocks a merge.
addressed
- Automatic acceptance can write after profile deletion begins (Bitkit/Services/PaykitAllowanceManager.swift:270). Fixed:
PubkyService.swift:1241now checksPrivatePaykitService.shared.isDeletingProfileinside thewithStateRevisionTrackingclosure. That closure runs underoperationLock.withLock(:1525) and after the revision read (:1603-1609).performProfileDeletion(PubkyProfileManager.swift:986) sets the flag before deletion's SDK work, and that work (deletePaykitProfile,PubkyService.swift:807) needs the same lock. So an accept either writes before deletion's SDK work or is refused. I traced this ordering in the code. I did not run a test for it. - Offer detail says "Waiting for your answer" but offers no way to answer (Bitkit/Views/Subscriptions/AllowancesView.swift:766). Fixed:
AllowancesView.swift:119-122(row) and:768-771(detail) now showsubscriptions__allowance_status_setting_up("Setting up the allowance") for an allower's offer. An ask still shows "Waiting for your answer". The key exists in en and all 14 other locales, andnode scripts/validate-translations.jspasses with 0 errors.
new since the last review
needs changing before merge
- Failure cleanup can release another payment's list consumption (Bitkit/Services/PrivatePaykitService+Payments.swift:355). Failure cleanup can release a list that a later payment has already consumed. The pre-broadcast failure path first releases the list at
PaykitAllowanceExecutor.swift:728, then awaitsrecord(), which releases the same version again. If another payment consumes that version between these releases, this version-only guard clears its consumption too. I reproduced this with the extracted production methods: A releases version 7, B consumes 7, A's delayed rollback resets the marker to 4, and a third consumption of 7 succeeds while B still owns it.
nits
- A refused accept is counted as a failed write and triggers a backup (Bitkit/Services/PubkyService.swift:1241). The deletion fence throws from inside the
operationclosure, sowithBackupStateRevisionTrackingtreats the refusal as a write that failed partway. When the operation throws, the catch atPubkyService.swift:1613-1617clearscachedBackupStateand callsmarkWalletBackupDataChanged(). So every accept refused during profile deletion bumpsbackupStateVersion, sendswalletBackupDataChangedSubject, and forces the next SDK call to read the backup revision again, although nothing was written. ThebeforeOperationhook (:1608-1609, "Admission can be deferred after the revision read without marking an unattempted write as uncertain") exists for this kind of refusal. The private wrapper at:1581-1586only uses that hook fordeferDuringPayment. I found this by reading the code; the only cost is one spurious backup trigger per refused accept.
Fixed in 86c8cb4. The deletion check moved out of the operation closure into the
Fixed in #799's 4a1e391, merged here in 6ee409c. A list released before broadcast now has its version cleared from the journal entry before the in-memory release, so The head is now 6ee409c. It also merges the rest of the updated #799: 98824b6 (bitcoin requests that also price a USDT rail can be paid automatically) and 723cdf5 (a held request is presented as soon as allowances settle). |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 6 new commits (98824b6, 8ca97b4, 86c8cb4, and 3 more). Nothing blocking remains.
addressed
- A refused accept is counted as a failed write and triggers a backup (Bitkit/Services/PubkyService.swift:1241). Fixed: The deletion check now runs in the
admitclosure, whichwithStateRevisionTrackingcalls frombeforeOperation(PubkyService.swift:1588).withBackupStateRevisionTrackingawaits that at:1612, before thedo/catchthat callsonSnapshot(nil)andonChange()(:1618). So a refused accept never reaches the failed-write path. I confirmed this by reading the code: the refusal works like the existingdeferDuringPaymentdeferral and is still checked under the SDK lock. - Failure cleanup can release another payment's list consumption (Bitkit/Services/PrivatePaykitService+Payments.swift:355). Fixed: For
.definitePreBroadcastFailure,releasePaymentListnow clears the entry'spaymentListVersionandpreviousPaymentListVersionbefore the in-memory release (PaykitAllowanceExecutor.swift:758-765). All three pre-broadcast failure sites pass the journal attempt id (:734,:785,:811). The laterrecord(.failed), and the recover branch for an attempt reconciliation already failed, then reach theguard ... let version = entry.paymentListVersionat:502and return without a version-only release. The.uncertainpaths keep the version, which is safe becauseconsumePrivatePaymentListrefuses a version at or below the one still consumed, so no other payment can take it before the durable release.testPaymentListReleasedBeforeBroadcastIsNeverReleasedAgain(BitkitTests/PaykitAllowanceExecutorTests.swift:621) asserts that no second release happens. I confirmed this by tracing each failure path; I did not run the tests.
new since the last review
worth doing, does not block
- Cached recovery stages permit a second list rollback (Bitkit/Services/PaykitAllowanceExecutor.swift:417). The cached journal stage can make recovery release a list that another payment has consumed. With two failed ledger attempts still open locally, suspend recovery during the first rollback. Let a concurrent settlement release the second attempt's list, then consume that list in another payment. Recovery resumes using the stale stage and releases it again. I reproduced this with extracted production methods: version 7 reverted to 4. Reading the current stage preserved version 7.
nits
- isAllowancePayable doc comment still says a conversion keeps a request manual (Bitkit/Services/PaykitAllowance.swift:338). The doc comment on
isAllowancePayable(PaykitAllowance.swift:338) still reads "Allowances cover only requests priced in bitcoin without a conversion; any other request always asks". After 98824b6 the property returnsamount.asset == .btcand ignorespricing.conversion, and the inline comment below it says the opposite. Someone reading only the doc comment would think a bitcoin request that also prices a USDT rail stays manual, buttestManualPaymentOverTheUsdtRailStaysOutOfTheLedgerand the updatedcoversRequestassertion show it is now covered.
|
@coreyphillips both new points from your approval were in #799's code and are fixed there in d500fab, merged here in 07b24a4:
Recovery now moves an attempt only if its current stage is still open, checked in the same synchronous journal update, and releases the list only when it made that move. The cached stages remain only as a pre-filter.
The doc comment now matches the code.
|
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 2 new commits (d500fab, 07b24a4). Nothing blocking remains.
addressed
- isAllowancePayable doc comment still says a conversion keeps a request manual (Bitkit/Services/PaykitAllowance.swift:338). Fixed:
Bitkit/Services/PaykitAllowance.swift:338-339now says a conversion only prices other rails and does not keep a request manual. That matchesamount.asset == .btcat :341. Checked by reading the file at HEAD. - Cached recovery stages permit a second list rollback (Bitkit/Services/PaykitAllowanceExecutor.swift:417). Fixed:
PaykitAllowanceExecutor.swift:419-424now passesonlyIfOpen: true.applyJournalOutcome(:524-526) checks the current stage inside the synchronousupdateLocalStateand returns nil once a settlement has failed the attempt, so no release follows. I tracedtestRecoveryNeverReleasesAListASettlementReleasedWhileItWaited(PaykitAllowanceExecutorTests.swift:689): the old code releases v7 twice and the new code once. I could not run it because there is no build cache here.
|
@coreyphillips this PR now targets
|
Closes #807
Twin: synonymdev/bitkit-android#1385
Stacked on #799
Refs: #805
This PR makes the allowee's wallet accept an allowance the allower offers, so the allowee taps nothing and the allowance is active as soon as the offer arrives.
Description
Out of Scope
Design
N/A — no design available.
Preview
set-and-accept-ios.mp4
QA Notes
Journeys
set-and-accept.xmlpin-gates-allowance-changes.xml: the payee's wallet accepts the offer by itself once the payer's PIN is enteredManual Tests
regression:payee: allowance detail → Swipe To End Allowance: ends on both sides.PaykitAllowanceExecutorTests.swift.Automated Checks
PaykitAllowanceTests.swift— only a received proposal from the allower counts as an offerPaykitAllowanceExecutorTests.swift— an allower's offer is accepted without the sheet and not announced twice; an allowee's ask still goes to the sheet; sent and answered proposals are never accepted; a failed accept is retried and announced once; an accept made as the app goes inactive is still announced; nothing is announced once the session changes; a background accept leaves Save disabled while a proposal is in flight; accepting one contact's offer leaves another contact's same-id ask on the review sheetxcodebuild test -only-testing:BitkitTestson an iPhone 17 simulator (iOS 26.5) with Paykit 0.1.0 after merging the updated feat: add allowances for automatic paykit payments #799 — 2,770 tests, 12 skipped; the only failures areSendConfirmationLifecycleTests.testCustomFeeWalletSwitchSurvivesFeeScreenNavigation, which fails the same way onmasterset-and-accept.xmlon two simulators against the staging regtest backend on the final head: the payee's wallet accepted by itself 14 s after the payer published the proposal, no review sheet opened, the toast "Allowance Added" showed, and both rows turned Active (the system notification was not exercised, notifications were not granted)