Skip to content

feat: accept allowance offers automatically on the allowee's wallet - #841

Merged
ovitrif merged 37 commits into
masterfrom
feat/allowance-auto-accept
Oct 22, 2026
Merged

ovitrif merged 37 commits into
masterfrom
feat/allowance-auto-accept

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Adds automatic acceptance of a received allowance proposal that makes this wallet the allowee, so the allower's decision alone activates the allowance.
  • Adds an "Allowance Added" notification, "{name} set up an allowance for you", with a toast fallback, so the allowee learns about the new allowance.
  • Keeps the review sheet with Accept and Decline for an allowee's ask, because there the money is this wallet's.
  • Retries a failed acceptance on the next refresh, and never touches a proposal this wallet sent or one already answered.
  • Accepts and announces an offer only for the profile session that found it, and never while the profile is being deleted; an offer accepted as the app goes inactive is still announced.
  • Opens the allowance detail, not Accept and Decline, when the user taps an offer in the Allowances list, where an offer still being accepted reads "Setting up the allowance".
  • Refuses an allowance accept once profile deletion has started, checked under the SDK lock right before the write.
  • Translates the "Allowance Added" notice and the new status into every supported language.

Out of Scope

  • Allowee-authored asks: this PR only changes what the allowee's wallet does with a received offer; the iOS app proposes allowances only as allower, so the review sheet for an ask is covered by unit tests, not on a device.

Design

N/A — no design available.

Preview

Payer waits Payee: Allowance Added Payee: Active Payer: Active
Recording
set-and-accept-ios.mp4

QA Notes

Journeys

  • updated set-and-accept.xml
  • updated pin-gates-allowance-changes.xml: the payee's wallet accepts the offer by itself once the payer's PIN is entered

Manual Tests

  • 1. Payer: Allowances → Add Allowance → contact → Save: the payee's wallet shows no review sheet, posts "Allowance Added", and both rows turn Active — the first private link between two new contacts can delay the accept by a minute or two.
  • 2. regression: payee: allowance detail → Swipe To End Allowance: ends on both sides.
  • Allowee-authored ask → review sheet on the allower — not reachable on iOS, which proposes only as allower; covered by PaykitAllowanceExecutorTests.swift.

Automated Checks

  • added PaykitAllowanceTests.swift — only a received proposal from the allower counts as an offer
  • added PaykitAllowanceExecutorTests.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 sheet
  • ran xcodebuild test -only-testing:BitkitTests on 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 are SendConfirmationLifecycleTests.testCustomFeeWalletSwitchSurvivesFeeScreenNavigation, which fails the same way on master
  • ran the allowance and payment proof suites again after merging the updated feat: add allowances for automatic paykit payments #799 (PIN gate, Lightning-only automatic payments, payer-side proposal checks) and the review fixes, with warnings as errors — 168 tests, 0 failures
  • ran set-and-accept.xml on 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)

@ovitrif
ovitrif force-pushed the feat/allowance-auto-accept branch from b0c583e to 80f5c0f Compare October 1, 2026 21:52
@ovitrif

ovitrif commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 0b8e352: this PR now sits on the updated #799 (merge 8f0449c, Paykit SDK rc59 with identity-wide shared state).

  • Rebased only this PR's two feature commits onto the new feat: add allowances for automatic paykit payments #799, whose old base commits are gone, and resolved the conflicts in AppScene.swift and the two allowance test files. Allowances are bound to identities now, so the offer rule is unchanged: a received proposal is accepted automatically only when this wallet is the allowee.
  • README: the journeys README no longer promises a one-second accept; the offer is accepted once it has arrived, which can take a minute or two between contacts that have never exchanged a private message.
  • Local verification: BitkitTests on an iPhone 17 simulator, 1,625 tests, 12 skipped, 0 failures; set-and-accept.xml on two simulators against staging regtest passed, the payee turned Active with no tap and no review sheet.
  • The description now carries the screenshots and a recording, and its QA Notes record that the allowee-ask review sheet is not reachable on iOS.

@ovitrif

ovitrif commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@jvsena42
jvsena42 added this pull request to stack #866 October 2, 2026 16:44
@ovitrif
ovitrif removed this pull request from stack #866 October 5, 2026 15:39
@ovitrif

ovitrif commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

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.

  • xcodebuild test -only-testing:BitkitTests on an iPhone 17 simulator (iOS 26.5) with Paykit rc59: 1,685 tests, 12 skipped, 0 failures.
  • feat: add allowances for automatic paykit payments #799 still has to take the latest master (Paykit 0.1.0); this PR follows once it does, and set-and-accept.xml is rerun on that head.

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 19, 2026
@ovitrif

ovitrif commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed cebcc55: merged the updated #799 (ea65e76, now level with master and Paykit 0.1.0) into this PR.

  • One conflict, in AppScene.swift: the incoming request refresh now runs automatic work only for the current automatic Paykit session and never while the profile is being deleted. Accepting an allower's offer and posting "Allowance Added" now sit inside that same guard, right after the allowance refresh, so an offer is never accepted for a stale session or during profile deletion.
  • xcodebuild test -only-testing:BitkitTests on an iPhone 17 simulator (iOS 26.5): 2,770 tests, 12 skipped, 2 failing, neither in allowance code:
    • SendConfirmationLifecycleTests.testCustomFeeWalletSwitchSurvivesFeeScreenNavigation fails the same way on master.
    • PaymentFlowTests.testchannelPurchaseFlow failed at the regtest deposit step (the balance never rose after funding); it passed on the previous head and gets a rerun.
  • set-and-accept.xml is rerun on this head next.

@ovitrif

ovitrif commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

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.

  • xcodebuild test for PaykitAllowanceTests, PaykitAllowanceExecutorTests, PaykitPaymentProofServiceTests and PaymentFlowTests.testchannelPurchaseFlow: 144 tests, 0 failures; the channel purchase test that failed at its regtest deposit step on cebcc55 passes.

@ovitrif

ovitrif commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

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 pin-gates-allowance-changes.xml.

  • The PIN journey now expects the payee's wallet to accept the offer by itself: no "Allowance Added" while the payer's PIN is cancelled, and once the PIN is entered, "Allowance Added" arrives with no review sheet, no tap and no PIN. It matches the Android copy word for word apart from the identifier annotations.
  • xcodebuild test for PaykitAllowanceTests, PaykitAllowanceExecutorTests and PaykitPaymentProofServiceTests: 144 tests, 0 failures.

@ovitrif

ovitrif commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA: set-and-accept.xml on 5143187

Two iPhone 17 simulators (iOS 26.5), fresh wallets with Pubky profiles on staging, saved as each other's contacts, regtest Debug build. Pass on every action.

  • The payer saved $5 a payment and $50 a month; its row read "Waiting for an answer" with "$ 50.00" over "Monthly limit".
  • The payee's wallet accepted by itself 14 s after the payer published the proposal: no review sheet opened and nothing was tapped. Notifications were not granted, so the "Allowance Added" toast showed: "Alwpayer807 set up an allowance for you".
  • The payee's row reads "Active · $5.00 a payment" with no Accept or Decline step, and the payer's row turned "Active · $5 a payment".

Payer on the left, payee on the right, at 2x:

set-and-accept-ios.mp4

@ovitrif

ovitrif commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 8e476fc: merged the updated #799 (b18cb79: master with the USDT feedback fixes, capped routing fees on automatic allowance payments, and dropping proposals whose limits the payer can't show). The merge was clean. Dropping such proposals applies only when this wallet is the payer, so the payee's automatic accept is unchanged.

  • xcodebuild test for PaykitAllowanceTests, PaykitAllowanceExecutorTests and PaykitPaymentProofServiceTests: 146 tests, 0 failures.
  • set-and-accept.xml passed on 5143187, one merge earlier (results above); none of the commits merged since touch the offer or its acceptance.

@ovitrif
ovitrif marked this pull request as ready for review October 19, 2026 19:21
@ovitrif
ovitrif requested review from a team and ben-kaufman October 19, 2026 19:21

@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 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:268 now breaks out of the loop instead of returning []. :277 gates the result only on identity and canAnnounce, and AppScene.swift:1425-1429 passes canAnnounce: isAutomaticSession (session and deletion, no canRefresh()), so scene inactivity or poll cancellation no longer drops the notice. I also checked that withStateRevisionTracking (PubkyService.swift:1562-1640) does not throw on cancellation after the write, and that PaykitAllowanceNotifier.post runs in an unstructured Task. 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-29 opens the review sheet only for an entry holding an ask, matching proposalForPresentation at PaykitAllowanceManager.swift:312; the auto-present path at AppScene.swift:224 uses that same check. An offer now opens AllowanceDetailView, whose swipe needs canEnd, 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-English Localizable.strings, using the allowance terms the base introduced. node scripts/validate-translations.js reports 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: check PrivatePaykitService.shared.isDeletingProfile inside the SDK lock, as WalletViewModel.swift:1418 does, since beginProfileDeletion() sets it before deletion's SDK work. Otherwise track it as CLAUDE.md asks: 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 from explanation at Bitkit/Views/Subscriptions/AllowancesView.swift:766, and the list row shows the same string at :119. The swipe is hidden because canEnd is 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 for isOfferFromAllower that 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. wasSettled is captured before record awaits, 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 at 97d86f86, using production methods and an idempotent SDK mock. The running app was not exercised.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 2782494: merged the updated #799 (efe4aa2: automatic and manual payments of one request stay apart). The one conflict, in PaykitAllowanceManager.swift, keeps this PR's offer-accept flag next to the base's new update counter.

xcodebuild test with warnings as errors for PaykitAllowanceTests, PaykitAllowanceExecutorTests and PaykitPaymentProofServiceTests: 162 tests, 0 failures.

@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 (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). record and the recovery .succeeded branch at :390 both call it. It decides inside the synchronous updateLocalState closure (:313) whether this call moved the entry to .succeeded, with no suspension between check and set. So of two concurrent record calls 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. acceptOffersFromAllowers at PaykitAllowanceManager.swift:276-286 is unchanged. Deletion is still only checked before accept suspends 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 from explanation at Bitkit/Views/Subscriptions/AllowancesView.swift:766, and the list row shows the same string at :119. The swipe is hidden because canEnd is 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 for isOfferFromAllower that 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 because canEnd is false for a received proposal (PaykitAllowance.swift:144-147). No status for isOfferFromAllower has been added.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips thanks for the re-check. All three open points are fixed as of 60444d8.

Automatic acceptance can write after profile deletion begins

Fixed in 26f826e with the fence you suggested. PubkyService.acceptAllowance now checks PrivatePaykitService.shared.isDeletingProfile inside withStateRevisionTracking, under the SDK lock and right before the write, and throws sessionNotActive when deletion has begun. beginProfileDeletion() sets the flag before deletion's own SDK work, so an accept either writes before deletion starts or is refused. The manual Accept goes through the same call, so it is fenced too. The PR body's claim now holds.

Offer detail says "Waiting for your answer" but offers no way to answer

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 validate-translations.js passes.

Recovery can duplicate the payment notification

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 testSettlementAndRecoveryAnnounceOnePaymentOnce covers it. Details: #799 (comment)

xcodebuild test with warnings as errors for PaykitAllowanceTests, PaykitAllowanceExecutorTests and PaykitPaymentProofServiceTests: 165 tests, 0 failures. 60444d8 also merges the updated #799 (69da189: the payee's payment list is freed when an automatic payment fails).

@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 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:1241 now checks PrivatePaykitService.shared.isDeletingProfile inside the withStateRevisionTracking closure. That closure runs under operationLock.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 show subscriptions__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, and node scripts/validate-translations.js passes 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 awaits record(), 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 operation closure, so withBackupStateRevisionTracking treats the refusal as a write that failed partway. When the operation throws, the catch at PubkyService.swift:1613-1617 clears cachedBackupState and calls markWalletBackupDataChanged(). So every accept refused during profile deletion bumps backupStateVersion, sends walletBackupDataChangedSubject, and forces the next SDK call to read the backup revision again, although nothing was written. The beforeOperation hook (: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-1586 only uses that hook for deferDuringPayment. I found this by reading the code; the only cost is one spurious backup trigger per refused accept.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips

A refused accept is counted as a failed write and triggers a backup

Fixed in 86c8cb4. The deletion check moved out of the operation closure into the beforeOperation hook. withStateRevisionTracking takes a new admit closure, and acceptAllowance passes the isDeletingProfile check through it. The check still runs under the SDK lock and after the revision read, but a refused accept is now treated as never attempted. Nothing clears cachedBackupState or calls markWalletBackupDataChanged() for it.

Failure cleanup can release another payment's list consumption

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 record(.failed) has nothing to release a second time. A durable release happens only for an attempt whose list was never released, and testPaymentListReleasedBeforeBroadcastIsNeverReleasedAgain covers it.

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). xcodebuild test with warnings as errors for PaykitAllowanceTests, PaykitAllowanceExecutorTests and PaykitPaymentProofServiceTests: 168 tests, 0 failures.

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

@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 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 admit closure, which withStateRevisionTracking calls from beforeOperation (PubkyService.swift:1588). withBackupStateRevisionTracking awaits that at :1612, before the do/catch that calls onSnapshot(nil) and onChange() (:1618). So a refused accept never reaches the failed-write path. I confirmed this by reading the code: the refusal works like the existing deferDuringPayment deferral 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, releasePaymentList now clears the entry's paymentListVersion and previousPaymentListVersion before the in-memory release (PaykitAllowanceExecutor.swift:758-765). All three pre-broadcast failure sites pass the journal attempt id (:734, :785, :811). The later record(.failed), and the recover branch for an attempt reconciliation already failed, then reach the guard ... let version = entry.paymentListVersion at :502 and return without a version-only release. The .uncertain paths keep the version, which is safe because consumePrivatePaymentList refuses 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 returns amount.asset == .btc and ignores pricing.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, but testManualPaymentOverTheUsdtRailStaysOutOfTheLedger and the updated coversRequest assertion show it is now covered.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips both new points from your approval were in #799's code and are fixed there in d500fab, merged here in 07b24a4:

Cached recovery stages permit a second list rollback

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. testRecoveryNeverReleasesAListASettlementReleasedWhileItWaited holds the first rollback open while a settlement fails the second attempt, and asserts that list is released once.

isAllowancePayable doc comment still says a conversion keeps a request manual

The doc comment now matches the code.

xcodebuild test with warnings as errors for PaykitAllowanceTests, PaykitAllowanceExecutorTests and PaykitPaymentProofServiceTests: 169 tests, 0 failures.

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 (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-339 now says a conversion only prices other rails and does not keep a request manual. That matches amount.asset == .btc at :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-424 now passes onlyIfOpen: true. applyJournalOutcome (:524-526) checks the current stage inside the synchronous updateLocalState and returns nil once a settlement has failed the attempt, so no release follows. I traced testRecoveryNeverReleasesAListASettlementReleasedWhileItWaited (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.

@ovitrif
ovitrif changed the base branch from feat/paykit-allowances to master October 21, 2026 18:41
@ovitrif
ovitrif dismissed coreyphillips’s stale review October 21, 2026 18:41

The base branch was changed.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Retargeted onto master ahead of merging #799, so this PR stays open when its base branch is deleted. It merges after #799; until then the diff also shows that PR's commits.

@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips this PR now targets master, since #799 merged as 35655ae. Retargeting is what dismissed your approval. 8bf9f68 only merges master in, and the feature diff is unchanged from 07b24a4, the head you approved. Could you re-approve?

  • swiftformat --lint passes on the changed Swift files, and validate-translations.js passes.
  • The unit test selection from unit-tests.yml ran on an iPhone 17 simulator (iOS 26.5). Two tests failed, neither in code this PR touches:
    • SendConfirmationLifecycleTests.testCustomFeeWalletSwitchSurvivesFeeScreenNavigation, which also fails on master.
    • UsdtWalletUITests.testReceiveAndInvalidPaymentStaySeparateFromBitcoin.

@ovitrif
ovitrif requested a review from coreyphillips October 21, 2026 19:23
@ovitrif
ovitrif merged commit 2464865 into master Oct 22, 2026
18 of 19 checks passed
@ovitrif
ovitrif deleted the feat/allowance-auto-accept branch October 22, 2026 03:08
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.

feat: accept allowance offers automatically on the allowee's wallet

4 participants