Repository navigation
Conversation
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. |
![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: 1568abb741
ℹ️ 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 changes reach Android's Paykit payment-request confirmation flow. Fix Back after asset switching and the endless loader after warning cancellation before merging. FindingsSummaryFixed payment requests now open on the selected asset's confirmation, support switching between bitcoin and USDT, and refresh expired USDT fee quotes in place.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Fixed payment request] --> Bind{Bind selected asset terms}
Bind -->|Cannot bind| Amount[Amount]
Bind -->|Bitcoin| BTC[Bitcoin confirmation]
Bind -->|USDT| USDT[USDT review]
BTC -->|Pay From: bind USDT| USDT
USDT -->|Pay From: bind bitcoin| BTC
USDT -->|Fee quote expired| Quote[Get replacement quote]
Quote --> USDT
Reviews (1) · Last reviewed commit: "fix: keep fixed paykit requests on confi..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
Pushed a89d712: the automatic requote on an expired USDT fee now runs once per swipe instead of retrying without limit.
The two Greptile findings are being fixed next: Back after an asset switch and Cancel on the payment warning. |
|
Pushed 14740ca: switching asset on a payment request now replaces the confirmation, and cancelling the payment warning keeps the reviewed quote.
|
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
2 findings (2 P1). Keeps a fixed Paykit request on the confirmation of its preselected asset, adds a bitcoin/USDT switch on Pay From, a USDT loading confirmation and in-place requote of an expired quote. Reviewed 1568abb, full tier, reasoned from the code: per the repo's Bitkit rules the branch was not built here.
What I checked, and 4 candidates I ruled out
Read in full: PaykitUsdtReview.kt, UsdtViewModel.kt, the changed paths of UsdtSendContent.kt, SendSheet.kt and AppViewModel.kt (openPaykitEndpoints, openPaykitRequestConfirmation, bindUsdtRequestTerms, switchPaykitRequestAsset, onPaymentMethodSwitch, reviewPaykitAmount, onConfirmPay/proceedWithPayment, paymentProofPreparation, sendPaykitUsdt)
Call sites traced: SendEvent.PaymentMethodSwitch (Confirm and USDT review), SendEffect.ShowPaykitConfirmation, UsdtViewModel.edit/requote
CI: build-local green; build, lint, e2e and Greptile still pending at review time
Ruled out
- Requote loop on a second expiry:
requoteonly runs onusdt__error_expiredwith no quote, and a successful requote replaces the error with the notice, so it cannot spin. - Subscription periods routed to the new confirm:
openPaykitRequestConfirmationreturns early forbillingPeriod != nullwith a USDT option. - USDT shown in
fundingOptionswith USDT disabled:availableFundingSourcesadds USDT only with ausdtRecipient, whichopenPaykitEndpointssets only when USDT is enabled or a binding exists. - Cold reader: the stuck-loading finding stood (it rated it P2; I kept P1 because the screen has no way forward). The back-stack finding got no answer because the reader timed out, so that doubt stands.
Merge confidence: 2/5, two P1s on the new confirm paths.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 14740ca: one LOW, inline with a failing test. Nothing above LOW. Reviewed as a funds change.
Checked and clean (amount, fee, PIN):
- Terms bound early: they are bound before the review opens.
- Rechecked before paying:
validateQuoteandauthorizeUsdtExecutioncheck the amount, terms, quote and validity again before executing. A switch during a send clears the reviewed amount, so the payment throws instead of going out. - Requote limits: a requote happens at most once per swipe. It resets the confirmed warnings and the PIN state, so both are asked again, and it never sends on its own.
- Back can't reach stale terms:
popUpToreplaces the other asset's confirmation. - Subscriptions: periods paid in USDT keep the Amount step.
Parity: iOS synonymdev/bitkit-ios#1014 is still open and keeps the Amount step when a real BTC/USDT choice exists. The PR's Out of Scope already notes the difference.
New UI: the loading state while the USDT quote runs has no Figma frame.
Device gate (partial): 14740ca — not run yet. Pending:
- BTC/USDT fixed request: it lands on the preselected confirmation, the amount equals the terms, switching works both ways, and Back;
- quote expiry: swipe, see the notice, warnings and PIN asked again, one payment;
- Cancel on a warning returns the knob to the start;
- the chip from the inline finding.
Device results will follow in a separate review.
This branch has not been deployed
3 participants
Closes #1623, closes #1630, closes #1631, closes #1632
Refs:
This PR keeps a fixed-amount payment request, such as a Pubky Market Shop order, on its confirmation step. On 3.0.1 and on master, a request whose payee offers both bitcoin and USDT opened a loading Payment Request screen, then moved to the Amount step on its own (#1623). From there, the USDT review showed a second Amount screen while the fee was quoted (#1632), sent the user back to Amount when the fee quote expired (#1630), and had a Pay From chip that did nothing (#1631). These issues share one cause: a request with a BTC/USDT choice had no confirmation step of its own, so the asset choice lived only on Amount.
Description
openPaykitEndpointstook any request with a USDT option to Amount, because fix: confirm fixed Paykit requests directly #1581's direct confirmation (openBitcoinRequestConfirmation) stopped wheneverusdtRecipientwas set. Amount remains the fallback when the terms cannot be bound, for example with too little USDT, a missing rate or a Lightning amount below the minimum. Subscription periods keep their existing Amount review.Out of Scope
PaymentNavigationHelperand needs its own change.Design
N/A — no design available. Reuses the existing Confirm, USDT review and Pay From chip; the USDT loading state follows the bitcoin Confirm's preparing placeholder.
Preview
Not captured: these screens need a live Paykit request with both bitcoin and USDT offered, which the local test environment cannot provide.
QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Automated Checks
AppViewModelSendFlowTest.kt— a fixed request switches asset on its confirmation instead of returning to AmountAppViewModelSendFlowTest.kt— a fixed USDT request without enough USDT opens AmountAppViewModelSendFlowTest.kt— a contact USDT review without a payment request keeps Pay From disabledUsdtViewModelTest.kt— an expired quote is replaced on review with the notice, at most once per swipe; other errors are left for the userUsdtViewModelTest.kt— declining a warning keeps the reviewed quote and warns again on the next swipeAppViewModelSendFlowTest.kt— a fixed dollar request opens confirmation whether or not USDT is enabled; an automatic USDT request finishes preparation on its review