Repository navigation
Conversation
…t/allowance-auto-accept
…t/allowance-auto-accept # Conflicts: # app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
…t/allowance-auto-accept
|
Pushed 528fb7b: merged the updated
|
|
Added the device result and media to the description.
|
…t/allowance-auto-accept
|
Pushed e7a8bd3: merged the updated
|
|
Pushed df5105a: merged the updated
|
…t/allowance-auto-accept # Conflicts: # journeys/allowances/README.md
…t/allowance-auto-accept # Conflicts: # app/src/test/java/to/bitkit/repositories/PaykitAllowanceRepoTest.kt
…t/allowance-auto-accept
|
Pushed 211d4c2: merged the updated
|
…t/allowance-auto-accept
…t/allowance-auto-accept
|
Pushed 1b61baa: merged the updated
|
…t/allowance-auto-accept
|
Pushed ba1c9bc: merged the updated
|
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. |
 [Medium impact] The changes affect the Paykit allowance workflow between linked contacts. The PR appears safe to merge, with a non-blocking issue in fallback announcements for multiple offers. Findings
SummaryAutomatically accepts received allowance offers when this wallet is the receiver.
English-only strings were explicitly declared out of scope by Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Refresh allowance records] --> B{Received unanswered proposal?}
B -->|No| C[Leave unchanged]
B -->|Yes| D{This wallet receives the allowance?}
D -->|Yes| E[Accept automatically]
E --> F{Acceptance succeeded?}
F -->|Yes| G[Post notification or toast]
F -->|No| H[Retry on a later refresh]
D -->|No| I[Show review with Accept and Decline]
Reviews (1) · Last reviewed commit: "Merge remote-tracking branch 'origin/fea..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba1c9bc209
ℹ️ 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".
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. The allowee's wallet now accepts a received proposal where the proposer took the allower role, posts "Allowance Added", and keeps the review sheet for an allowee's ask. Reviewed ba1c9bc, full tier, reasoned from the code and CI: on this track the branch is not built here.
What I checked, and 5 candidates I ruled out
Read: PaykitAllowance.kt, PaykitAllowanceRepo.kt (lifecycle, accept/respond, presentation, publish), AppViewModel.kt around refreshIncomingPaykitPaymentRequests and presentNextIncomingPaykitPaymentRequest, the new tests, and the twin bitkit-ios#841 for parity.
Call sites traced: acceptOffersFromAllowers (one caller, after paykitAllowanceRepo.refresh()), proposalForPresentation, isAnswerable (the Subscriptions row tap), every refreshIncomingPaykitPaymentRequests caller (10 s poll, foreground, stored refresh).
CI: build with testDevDebugUnitTest, lint and detekt green; e2e shards still running at review time.
Ruled out
- Double accept from two overlapping refreshes: the
isAcceptingOffersCAS lets one run through, and a run that slips past it on a stale snapshot still goes throughrespond, which re-readsentry(entryId)and throwsUnavailableonce the record is no longer answerable, soacceptAllowanceis not called twice and nothing is announced twice. - Mixed entry (an offer and an ask in one group): excluded from auto-accept by the
all { isOfferFromAllower }check and still presented byproposalForPresentation, so the user decides the ask. - Unbounded terms auto-accepted:
PaykitAllowance.fromdrops unbounded terms only for the allower, but on an offer this wallet is the allowee and pays nothing, so accepting one exposes no funds. - Accept during profile deletion: unlike iOS, Android passes no
canContinueinto the accept loop, so a deletion that starts mid-poll does not stop it;processAllowanceRequestsright after it has the same shape on the base branch, so this is not new to this diff. - Identity switch mid-accept, row tap opening Review for an offer, toasts replacing each other: already raised inline by other reviewers on this head; not repeated.
Merge confidence: 4/5, the e2e shards had not finished and pin-gates-allowance-changes.xml was not driven on a device.
jvsena42
left a comment
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 pays an offer.
isOfferFromAllowerrequiresrole == ALLOWEEfrom the SDK'slocalRoleon a received proposal, andcoversRequestpays only onisAllower. - Removed contacts.
removeContactblocks the peer, andlist_allowancesdrops blocked peers. - Cancellation.
isAcceptingOffersresets infinally, including on cancellation. - No shared progress flag, so the
isWorkingissue from synonymdev/bitkit-ios#841 doesn't apply. - Identity switch. An identity-A offer isn't accepted under B, because
respondlooks it up in B's entries and failsUnavailable. Announcing A's already-accepted offers in B's profile is codex's open thread at:252, so I'm not repeating it. The iOScanContinuefence (c5ac982) is still worth porting.
set-and-accept.xml and pin-gates-allowance-changes.xml match iOS #841 apart from ids.
Perf (repeated work): acceptOffersFromAllowers runs on every request refresh, including the 10 s poll. An offer whose accept keeps failing retries every poll, costing one SDK call and a warn line each time.
Device gate (partial): not run yet. Pending: the two allowance journeys, cross-device with #841.
|
Pushed 6d5010f: the automatic offer accept now answers only what it checked, and only while its session lasts.
|
pwltr
left a comment
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 6d5010f. I ran the 51 targeted allowance unit tests, all passing. On two Android emulators, I verified automatic offer acceptance and notifications, PIN confirmation/cancellation when saving and ending allowances, and a $2 regtest automatic payment with matching receipts, history and allowance totals.
One observation: the automatic-payment success notification appeared roughly 40 seconds after sending the request, with a pending-request bell visible during processing. I haven’t isolated the cause of that delay or compared its timing against the base branch.
No additional blocking findings from my review. Greptile’s deferred fallback-toast issue is tracked in #1599.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 6d5010f: no issues found in 6d5010f. Reviewed as a funds change. My earlier LOW is fixed: acceptOffersFromAllowers calls respond(offer) on the checked snapshot (:270), and respond iterates that snapshot (:305).
Checked and clean:
- The snapshot also pins
proposalStreamItemId(:245). isCurrentruns before each offer and again before returning (:258, :268, :274).presentedProposalIdsis written only while the identity is unchanged (:312), which matches iOSc5ac982.canContinue(AppViewModel.kt:1161-1164) applies the same profile-deletion and polling-stopped checks as the refresh (:1140-1147).isAcceptingOffersis still released infinally.
Inherited from the base: the manual Accept answers every member of an entry, including another contact's proposal that shares the id. It is the same bug as synonymdev/bitkit-ios#799 (review) and has an Android reproduction on #1340's branch. This PR doesn't make it worse, so I'll raise it on #1340 once it's rebased.
Perf: not relevant.
Device gate (partial): 6d5010f — not run yet. Pending: allowances/set-and-accept.xml and allowances/pin-gates-allowance-changes.xml cross-device with synonymdev/bitkit-ios#841. Device results will follow in a separate review.
…t/allowance-auto-accept # Conflicts: # app/src/main/java/to/bitkit/repositories/PaykitAllowanceRepo.kt # app/src/test/java/to/bitkit/repositories/PaykitAllowanceRepoTest.kt # journeys/allowances/README.md
|
Pushed 53540d2: merged the updated
|
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta at 53540d2: no issues found. The merge of feat/paykit-allowances takes the contact-scoped it.key in respond (keeping the activeIdentity guard), proposalForPresentation (keeping the !isOfferFromAllower filter), markProposalPresented and the grouping. acceptOffersFromAllowers still answers only the entry it checked. The re-aimed cross-contact test still proves the original contact is never answered.
Minor, no behaviour impact: the merge left a duplicated verify(sdk, never()).acceptAllowance(any(), any(), any()) at PaykitAllowanceRepoTest.kt:505-506.
Device gate (partial): 53540d2 — not run yet. Pending: allowances/set-and-accept.xml and allowances/pin-gates-allowance-changes.xml in the auto-accept form, plus the other five allowance journeys, cross-device with synonymdev/bitkit-ios#841. Device results will follow in a separate review.
…t/allowance-auto-accept
|
Pushed dea7822: merged the updated |
This branch has not been deployed
4 participants
Closes #1353
Twin: synonymdev/bitkit-ios#841
Stacked on #1340
Refs: #1351
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
ba1353-set-and-accept-1b61baa-2x.mp4
QA Notes
Journeys
set-and-accept.xml— the allowee's wallet accepts the offer with no review sheet, posts "Allowance Added", and both rows turn Active1b61baa, Paykit0.1.0, recorded side by side above (payer left, allowee right, 2x): no review sheet opened, "Allowance Added" posted once, the payer's row read "Waiting for an answer" and then "Active · $5 a payment", and the allowee's row read "Active · $5 a payment". The proposal went out 11 s after Save and the allowee's wallet accepted it and posted "Allowance Added" 17 s later (28 s from Save). The later headba1c9bconly adds base changes outside the offer accept.1b61baa).pin-gates-allowance-changes.xml— with a payment PIN on the payer, the payee's wallet still accepts the offer by itself: "Allowance Added" arrives with no review sheet, no tap and no PINManual Tests
N/A
Automated Checks
PaykitAllowanceTest.kt— only a received proposal from the allower counts as an offerPaykitAllowanceRepoTest.kt— an allower's offer is accepted without the sheet and not announced twice; a failed accept is retried and announced once; an allowee's ask still goes to the sheet; sent and answered proposals are never accepted; an ask that joins an offer's entry mid-accept is never accepted; nothing is accepted or announced once the session or identity changedAppViewModelSendFlowTest.kt— the send flow stubs the new acceptance call./gradlew testDevDebugUnitTest detektatdea7822(Paykit0.1.0, merged with the updatedfeat/paykit-allowances): unit tests pass and detekt is clean