Repository navigation
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:
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e476fc97f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
[Medium impact] The change affects the allowance agreement flow between two wallets. The PR appears safe to merge, with non-blocking improvements for session checks and retry tests. FindingsSummaryReceived allowance offers now accept automatically. Requests asking this wallet to pay still go to the review sheet.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Received proposal] --> B{This wallet receives money?}
B -->|Yes| C[Accept automatically]
C --> D{Acceptance succeeds?}
D -->|Yes| E[Announce Allowance Added]
D -->|No| F[Retry on next refresh]
B -->|No| G[Show review sheet]
Reviews (1) · Last reviewed commit: "chore: merge feat/paykit-allowances" · Reviewed by Greptile |
|
Pushed c5ac982: the automatic offer accept now runs only for the session that found the offer.
|
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Makes the allowee's wallet accept an allower's offer by itself and announce it, while an allowee's ask still goes to the review sheet. Reviewed c5ac982, full tier.
What I checked, and 4 candidates I ruled out
Read in full: the diff against feat/paykit-allowances (10 files), PaykitAllowance.swift, PaykitAllowanceManager.swift (accept, respond, presentation, automatic payments), the refreshPaykit path in AppScene.swift, the row routing in AllowancesView.swift
Call sites traced: acceptOffersFromAllowers, isOfferFromAllower, isAnswerable, proposalForPresentation
CI: Run Tests still running at review time; Greptile passed. Not built here: Swift does not build on this box, so this review is reasoned from the code and the tests.
Ruled out
- An allowee's ask accepted without the user:
isOfferFromAllowerneedsrole == .alloweeon a received proposal, so a proposal that makes this wallet the payer never qualifies;testManagerLeavesAnAskFromTheAlloweeForTheReviewSheetcovers it. - A mixed entry auto-accepted: an entry is taken only when every answerable allowance in it is an offer (
allSatisfy), and anything else stays on the sheet path. - Accepting or announcing for a stale session: c5ac982 pins the starting identity, rechecks it with the caller's session check before each accept and before returning, and
respondrecords presented ids only under the identity it started with; both bot threads are answered by that commit and its two tests. - Double accept or double announcement:
isAcceptingOffersdrops a re-entrant pass, and an accepted offer leaves the answerable set afterrespondrefreshes, so the second pass finds nothing;testManagerAcceptsAnOfferFromTheAllowerWithoutAReviewSheetand the retry test assert one accept and one announcement.
Merge confidence: 4/5, the change is covered by the new manager tests but Run Tests had not finished at review time.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no HIGH/MEDIUM. One LOW is inline, with a failing test.
Checked and clean:
- This wallet never ends up paying.
isOfferFromAllowerrequiresrole == .alloweeon a received proposal, andlocalRolecomes from the SDK (recipient_role()). - Terms can't change after the check. The accept passes
expectedProposalStreamItemId, so a re-sent or replaced proposal is refused. - The allowee side has no automatic effects. Every payment path checks
isAllower. - Removed contacts. Offers arrive only over an Encrypted Link,
list_allowancesdrops blocked peers, and deleting a contact blocks the peer, so a removed contact's offer isn't auto-accepted.
Parity: the session/identity fence fixed in c5ac982 needs porting to the Android draft synonymdev/bitkit-android#1385, whose acceptOffersFromAllowers() takes no canContinue.
Perf (repeated work): an offer whose accept keeps failing (recoveryRequired, SDK NotFound) retries on every poll, costing one acceptAllowance call and a warn line each time. I'll measure that on device together with #799.
Device gate (partial): not run yet. Pending: the updated allowances/set-and-accept.xml and pin-gates-allowance-changes.xml, cross-device.
|
Pushed d2c0ca9: an offer accepted in the background no longer clears the allowance sheet's progress state, so Save stays disabled while the user's own proposal is in flight and a second tap can't send a duplicate proposal.
|
|
Pushed 84d9ccc: merged the updated #799 (2e7c71d, which brings
|
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 84d9ccc: no issues found in d2c0ca9 and 84d9ccc. Reviewed as a funds change. My earlier LOW is fixed:
- the background pass calls
accept(offer, showsProgress: false)(:268); respondtouchesisWorkingonly whenshowsProgressis set (:283-290);- covered by
testAutomaticOfferAcceptanceKeepsAManualProposalWorking.
Checked and clean:
showsProgressdefaults to true, so the review sheet andrejectkeep theisWorkingguard.84d9cccis formatting only.- Parity with synonymdev/bitkit-android#1385: both pin the checked entry, check the identity, take
canContinue, and record presented ids only for the starting identity.
Inherited from the base: the shared-id issue posted on #799 (review). This PR's auto-accept leaves offer-plus-ask entries for manual review, and that is the shape the issue produces. The fix belongs in #799 and needs nothing here.
Perf: not relevant.
Device gate (partial): 84d9ccc — not run yet. Pending: allowances/set-and-accept.xml and allowances/pin-gates-allowance-changes.xml cross-device with the Android twin. Device results will follow in a separate review.
|
Pushed e1f6cc8: merged the updated #799 (407c225: allowance entries and presented proposals are keyed by contact as well as allowance id, so two contacts' proposals that share an id stay apart).
|
|
Pushed 86d52d3: merged the updated #799 (b059b8e: each contact's automatic payments count only toward that contact's allowance). The merge was clean.
|
jvsena42
left a comment
There was a problem hiding this comment.
Delta at e1f6cc8: no issues found. The merge of feat/paykit-allowances replaces 80c05bc's change with the scoped-id version from #799. This PR's production diff over its base is unchanged, so only tests moved: the merged test asserts that only C's offer is auto-accepted and M's ask stays presentable.
set-and-accept.xml:20 still says AllowanceRow-<allowanceId>. #799 changed it to <entryId> in a844c40, so this line needs the same wording once you merge the base.
Device gate (partial): e1f6cc8 — not run yet. Pending: allowances/set-and-accept.xml and allowances/pin-gates-allowance-changes.xml cross-device with the Android twin. Device results will follow in a separate review.
|
Pushed 97d86f8: merged the updated #799 (b005f8f, which carries
|
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; nothing is accepted or 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)