Repository navigation
fix: keep fixed paykit requests on confirmation with an asset switch - #1638
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.
…quest-asset-confirm # Conflicts: # app/src/main/java/to/bitkit/ui/screens/wallets/usdt/UsdtSendContent.kt
|
Merged master into this branch (bf9aaac) to resolve the conflict in |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan of 14740ca..bf9aaac: no findings.
17e1b0cfixes the Pay From chip on a contact USDT review without a request (AppViewModel.kt:8390-8395, with the test atAppViewModelSendFlowTest.kt:14658-14690).- The master merge
bf9aaacresolves its one conflict inUsdtSendContent.ktcorrectly (UsdtQuoteDetails(quote, contact, isMerchantRequest, onSwitchAsset),:698and:711-716). - Re-traced the funds path after the merge into
AppViewModel:- terms are cleared before the rebind and re-validated in
prepare/validateQuote; - there's no new auto-start, because a USDT-payable request always has a conversion, so
shouldAutomaticallyPayis false; - Back and cancel from the asset switch land on Amount or close the sheet, never on stale terms.
- terms are cleared before the rebind and re-validated in
Device gate (partial): bf9aaac — not run yet; pending a fixed BTC/USDT request switched between assets on confirmation, and l10n for the new usdt__error_quote_refreshed. Device results will follow in a separate review.
|
@piotr-iohk this needs a device run on dd0c69f before the 3.1.0 cut, since #1713 is stacked on it. The Manual Tests section lists the cases: a fixed request with both BTC and USDT opens on its confirmation, Pay From switches between the assets, and an expired USDT quote is requoted in place. The regtest and local USDT fixture setup you used for #1635 covers them. dd0c69f only merges master in. It had no conflicts, and the send-flow and USDT tests plus detekt pass. |
|
QA for dd0c69f: Manual Tests 1, 2 and 3 pass. Tested on an Android 16 emulator: BTC on staging regtest, USDT on the local Arbitrum fork fixture. The request came from the Paykit fixture peer: a fixed 5 USD request (4 USD in Test 3) accepting both bitcoin and USDT, which stands in for a Shop order. 🟢 Tests 1, 2, 3Test 1 The request opens on the USDT review of the preselected asset, after the quote loader, and never moves to Amount. With no USDT balance loaded, an earlier request opened on the bitcoin confirmation instead.
Test 2 Pay From switches USDT → bitcoin → USDT → bitcoin on the confirmations, and the swipe pays 10,000 sats in bitcoin to the request's address. The request then leaves Payment Requests.
Test 3 After 130 s on the USDT review (the quote lives 120 s), the first swipe replaces the quote on the same screen with "The fee quote expired and was updated." The second swipe pays. The recipient received exactly 4.000000 USDT, a single transfer.
|
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the PR diff against merge base 69e98773, at dd0c69fd. A direct Pay From switch replaces the other confirmation, and an expired USDT fee quote is refreshed once per swipe. The Amount fallback after that replacement still leaves the current confirmation on the back stack, and a later Pay From toward USDT does not use that fallback when the conversion rate is unavailable.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
Earlier inline threads were rechecked at this head and do not reproduce: the requote loop (discussion), stacking the other confirmation on a direct switch (discussion), the warning Cancel loader (discussion), and Pay From on a contact USDT review without a request (discussion). No matching open issue was found for the Amount-fallback back stack.
GitHub CI, Lint, and local E2E succeeded on this commit. This review did not run those jobs or the new unit tests; the new test code was inspected. The author reports manual tests 1–3 passed on this commit (report); that run does not cover the fallback below. iOS source was not checked out here. The PR states the matching Amount-first route remains in iOS PaymentNavigationHelper and is out of scope.
Device testing: not performed in this review.
Suggested additional test cases
- Android. Fixed USD request that accepts lightning, on-chain, and USDT, opened on the USDT review, then switched once to bitcoin. After the conversion quote is no longer valid, tap Pay From to move between lightning and on-chain, then use the in-sheet Back and the system Back. Amount should be the only send step; Back should not open a bitcoin confirmation whose request terms were cleared, and swipe on that confirmation should not dismiss the request.
- Android. Fixed request with on-chain and USDT only, sitting on the bitcoin confirmation. After the conversion quote expires, tap Pay From toward USDT. The sheet should show Amount or a visible rate error, and the bitcoin confirmation should not ignore the tap.
Findings
- [MEDIUM] Amount fallback leaves the unbound confirmation underneath — inline at
app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt:124. - [LOW] Pay From toward USDT does nothing when the rate is unavailable — inline at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:4072.
Reviewer model: grok-4.7
Reasoning effort: high
|
QA for 56b680c: Manual Tests 1, 2 and 3 pass on the head with both review fixes. Tested on an Android 16 emulator: BTC on staging regtest, USDT on the local Arbitrum fork fixture. The request came from the Paykit fixture peer: a fixed 5 USD request (4 USD in Test 3) accepting both bitcoin and USDT, which stands in for a Shop order. 🟢 Tests 1, 2, 3Test 1 The request opens on the USDT review of the preselected asset, after the quote loader, and never moves to Amount.
Test 2 Pay From switches USDT → bitcoin → USDT → bitcoin on the confirmations, and the swipe pays 10,000 sats in bitcoin. The request then leaves Payment Requests.
Test 3 After 132 s on the USDT review (the quote lives 120 s), the first swipe replaces the quote on the same screen with "The fee quote expired and was updated." The second swipe pays. The recipient received exactly 4.000000 USDT.
|
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of 56b680c8 since dd0c69fd, using the completed baseline review. The merge base is still 69e98773. This pass inspected the amount-fallback delta and its callers. Inherited baseline coverage is the fixed-request open path, successful Pay From cycling and the confirm chip, USDT loading and quote-expiry handling, displayed-versus-executed binding, the previously reviewed unit tests, and the fee-quote copy.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Pay From toward an asset the request cannot quote clears the reviewed confirmation and opens Amount on the asset that is still selected. Amount then requotes that asset, so the rate-unavailable notice does not stay on screen.
Validation: the new SendSheetNavigationTest and AppViewModelSendFlowTest cases were inspected and not executed. Device testing: not performed in this review. iOS was not compared; this pull request already treats the matching iOS routing as separate work.
Suggested additional test cases
- Android. Setup: a fixed USD Paykit request with bitcoin and USDT endpoints and a conversion rate for bitcoin only; USDT enabled and enough on-chain balance to pay the bitcoin quote. Action: open the request so bitcoin Confirm is showing, then tap Pay From. Expected: the bitcoin confirmation stays swipeable, or Amount opens on USDT with “Payment rates are unavailable or expired. Try again when online.” and Continue disabled. Back must not close the sheet while that payable bitcoin quote is still available.
Earlier findings reconciled at this revision: 6 verified fixed.
Findings
- [MEDIUM] No-rate Pay From drops the payable confirmation — inline at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:4012.
Reviewer model: grok-4.7
Reasoning effort: high
|
QA for ee6edc3: Manual Test 4 passes. Tested on an Android 16 emulator: BTC on staging regtest, USDT on the local Arbitrum fork fixture with a 25 USDT balance. The request came from the Paykit fixture peer: a fixed 5 USD request accepting bitcoin and USDT, priced with a bitcoin rate only. Tests 1–3 passed on 56b680c in the earlier QA comment; this commit changes only the no-rate branch of Pay From. 🟢 Test 4Test 4 The request opens on the bitcoin confirmation (10,000 sats). Pay From toward USDT keeps that confirmation, still on Savings, and shows "Payment rates are unavailable or expired. Try again when online." A swipe then pays in bitcoin and shows Bitcoin Sent. The clip joins two recordings: the first swipe attempt by the test driver was too fast for the slider.
|
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of ee6edc3d since 56b680c8, with merge base 69e98773. Inherited baseline: pullrequestreview-5680171892. This pass covered the no-rate Pay From change and the confirmation funding-source cycle it stays on. Unchanged request open, successful Pay From rebinding, USDT quote expiry, displayed-versus-executed terms, fee-quote copy, and amount-fallback navigation remain inherited.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
A reviewed confirmation that cannot price the next Pay From source keeps that source selected, so a later priceable source is unreachable from the cycle.
Validation: the updated no-USDT-rate case in AppViewModelSendFlowTest was inspected and not executed. It selects USDT directly from an on-chain-only confirmation. Device testing: not performed in this review.
Suggested additional test cases
- Android. Setup: a fixed payment request with Lightning, on-chain, and USDT endpoints, a BTC conversion rate, and no USDT rate, where both Spending and Savings can fund the amount. Action: open the request so it starts on the Lightning confirmation, tap Pay From once, then tap Pay From again. Expected: Pay From reaches the other bitcoin source instead of staying on Savings with only the rate toast, and swipe pays the bitcoin source left selected.
Earlier findings reconciled at this revision: 5 verified fixed.
Findings
- [MEDIUM] Pay From stops cycling when the next asset has no rate — inline at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:4011.
Reviewer model: grok-4.7
Reasoning effort: high
|
QA for 17b60ca: Manual Test 4 passes again on this head. Tested on an Android 16 emulator: BTC on staging regtest, USDT on the local Arbitrum fork fixture with a 25 USDT balance. The request came from the Paykit fixture peer: a fixed 5 USD request accepting on-chain bitcoin and USDT, priced with a bitcoin rate only. Test 5 was not run on a device: the fixture peer cannot add a Lightning endpoint to a request. 🟢 Test 4Test 4 On the bitcoin confirmation (10,000 sats, Savings), Pay From has no other priced source, so the confirmation stays and the toast "Payment rates are unavailable or expired. Try again when online." shows. A swipe pays in bitcoin and shows Bitcoin Sent.
|
|
QA for 5e988c3: Manual Tests 1, 2, 3, 4 and 6 pass. Tested on Android 16 emulators: BTC on staging regtest, USDT on the local Arbitrum fork fixture with a 25 USDT balance. Requests in Tests 1–4 came from the Paykit fixture peer and accepted on-chain bitcoin and USDT: a fixed 5 USD request (4 USD in Test 3), priced in USDT and bitcoin, except in Test 4, which has a bitcoin rate only. Test 6 used a second Bitkit wallet on another emulator as the payee contact. Test 5 was not run on a device: the fixture peer cannot add a Lightning endpoint to a request. 🟢 Tests 1, 2, 3, 4, 6Test 1 The request opens on the USDT review at 5.00 after the quote loader and never moves to Amount.
Test 2 On the same request, Pay From switches USDT → bitcoin → USDT → bitcoin on the confirmations, and the swipe pays 10,000 sats in bitcoin (Bitcoin Sent).
Test 3 After 130 s on the USDT review (the quote lives 120 s), the first swipe replaces the quote on the same screen with "The fee quote expired and was updated." The second swipe pays. The recipient's balance went from 0 to exactly 4.000000 USDT.
Test 4 The request opens on the bitcoin confirmation (10,000 sats, Savings). Pay From has no other source with a rate, so the confirmation stays and the toast "Payment rates are unavailable or expired. Try again when online." shows. A swipe pays in bitcoin.
Test 6 Paying the second wallet's contact without a request: on Amount, USDT with 5 entered (6,141 sats), then Savings, then USDT again shows 5.00 USDT.
|
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to 5e988c3 (Pay From cycle commits). No HIGH or MEDIUM. One LOW inline: the confirmation's fee refresh can be left stuck loading when Pay From has no rate.
Checked:
hasPaymentRatematchespaymentAmountForSourceexactly and is pure.popUpTo(graph.id)is a safe superset.- Unpriced sources are skipped in the cycle, and the amount fallback replaces a switched confirmation correctly.
This PR and #1725 will conflict in onPaymentMethodSwitch, in the refresh-cancel lines.
Device gate (partial): 5e988c3, not run yet. You already ran tests 1–4 and 6 on this head. Pending: tapping Pay From while the fee cell is still spinning.
|
@ovitrif please resolve conflicts. |
…quest-asset-confirm # Conflicts: # app/src/main/java/to/bitkit/ui/screens/wallets/usdt/UsdtViewModel.kt
|
@piotr-iohk conflicts are resolved: master is merged in 82d043f. Device tests on the new head are running now, and I'll post the results here. |
|
QA for 27adb96: Manual Tests 1, 2, 3, 4 and 6 pass. Tested on Android 16 emulators: BTC on staging regtest, USDT on the local Arbitrum fork fixture with a 25 USDT balance. Requests in Tests 1–4 came from the Paykit fixture peer and accepted on-chain bitcoin and USDT: a fixed 5 USD request (4 USD in Test 3), priced in USDT and bitcoin, except in Test 4, which has a bitcoin rate only. Test 6 used a second Bitkit wallet on another emulator as the payee contact. Test 5 was not run on a device: the fixture peer cannot add a Lightning endpoint to a request. 🟢 Tests 1, 2, 3, 4, 6Test 1 The request opens on the USDT review at 5.00 after the quote loader and never moves to Amount.
Test 2 On the same request, Pay From switches USDT → bitcoin → USDT → bitcoin on the confirmations, and the swipe pays 10,000 sats in bitcoin (Bitcoin Sent).
Test 3 After 130 s on the USDT review (the quote lives 120 s), the first swipe replaces the quote on the same screen with "The fee quote expired and was updated." The second swipe pays. The recipient's balance went from 0 to exactly 4.000000 USDT.
Test 4 The request opens on the bitcoin confirmation (10,000 sats, Savings). Pay From has no other source with a rate, so the confirmation stays and the toast "Payment rates are unavailable or expired. Try again when online." shows. The fee row keeps its estimate (Normal, ₿ 141) after the toast, and a swipe pays in bitcoin.
Test 6 Paying the second wallet's contact without a request: on Amount, USDT with 5 entered (6,202 sats), then Savings, then USDT again shows 5.00 USDT.
|
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to 27adb96. No new findings.
27adb96 fixes my LOW at AppViewModel.kt:4018. The early return for a reviewed quote with no rate now happens before ++sendStateGeneration and onchainSendRefreshJob?.cancel(), so the toast path keeps the confirmation's fee refresh running. The other branches still cancel it as before. The new test no-rate pay from toast keeps the confirmation fee refresh running in AppViewModelSendFlowTest.kt covers it.
The conflict with #1725 in onPaymentMethodSwitch still stands, so whichever lands second needs a rebase.
Device gate: 27adb96. paykit-fixture-sender can send, so it is left to e2e CI. A Bitkit-made request always prices every endpoint asset, so the app can't build this precondition itself. Covered by the unit test above and your Test 4 on this head.
|
@jvsena42 Merging this PR on your approval of head 27adb96, with the device run on that head passing, checks green and no open threads. @ben-kaufman, #1725 conflicts with this in onPaymentMethodSwitch, so it needs a rebase on master once this lands. |
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 disabledAppViewModelSendFlowTest.kt— Pay From toward USDT with no USDT rate keeps the confirmation and shows the rate errorAppViewModelSendFlowTest.kt— the Pay From cycle skips USDT when the request has no USDT rateAppViewModelSendFlowTest.kt— switching from USDT to Savings and back restores the entered USDT amountAppViewModelSendFlowTest.kt— Pay From with no rate keeps the confirmation's fee refresh runningSendSheetNavigationTest.kt— the Amount fallback after a Pay From switch leaves no confirmation behind itUsdtViewModelTest.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