Repository navigation
fix: preserve payment guards and usdt feedback - #1506
Conversation
 [Medium impact] The changes affect payment-request and USDT wallet workflows. Fix the nullable Findings
SummaryThis PR keeps payment guards tied to active attempts and queues incoming USDT notices until they can appear.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Payment attempt starts] --> B[Add attempt token]
B --> C[Pause background Paykit work]
C --> D[Attempt finishes or cancellation completes]
D --> E[Remove this attempt token]
E --> F{Any tokens remain?}
F -->|Yes| C
F -->|No| G[Resume background work]
Reviews (1) · Last reviewed commit: "fix: preserve payment guards and usdt fe..." · 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). |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Turns the payment-submission flag into a per-attempt token set held for the whole software or hardware attempt (so sign-out and sheet dismissal no longer drop it), moves incoming USDT receipts from a fire-and-forget SharedFlow into state consumed once when a sheet can show, and brings bridge status text and the preparing accent in line with iOS. Reviewed 643ada4, full tier.
What I checked, and 6 candidates I ruled out
Read in full: the diff of every app/src/main file; HwSendViewModel.signAndBroadcast/cancel, UsdtRefreshEffect, UsdtRepo.refresh, AppViewModel.onConfirmPay/sendPaykitUsdt/showTransactionSheet
Call sites traced: setPaymentSubmissionActive (now only from beginPaymentSubmission), isPaymentSubmissionActive readers (PaykitSdkService outbound send, scheduleAcceptedResponse, BackupRepo, observePaymentProofChanges), UsdtRefreshEffect (Home, USDT wallet, deposit receive)
CI: build (includes testDevDebugUnitTest), build-local, lint, detekt, Greptile green; the local e2e shards were still running. Not built here: an auto review does not run a branch's Gradle code on this box, so CI is the evidence.
Ruled out
- A held guard that never releases: every
beginPaymentSubmission()is paired with afinally; on hardware it lives insidesigningJob, so acancel()that returns early on an unresolved broadcast still releases when the job ends orviewModelScopeis cleared. - Removing the reset from
clearStateLocked: it is the point of the change, and no other path relied on it; the background readers only wait for the flag to drop, which the token set now does when the last attempt ends. - Hardware double-guard:
onConfirmPaysetsisSubmittingPaymentRequestand navigates without taking a token; the token comes frombeginHardwarePaymentSubmissionper signing attempt, and the passphrase retry now goes through the samesignAndBroadcastlambda, so it takes one too. - Two screens presenting one receipt: Home and the USDT wallet both run
UsdtRefreshEffect, butconsumeReceiptis a singlegetAndUpdateon the state, so only oneshouldShowwins. - Receipt lost while blocked:
shouldShowruns after the restore, critical-update and sheet-enabled checks, so a blocked attempt leaves the receipt queued;AppViewModelSendFlowTestcovers exactly that ordering. - Stale receipts across wallets:
pendingReceiptsis cleared on reset and on an address change, which rebuildsUsdtWalletState.
Merge confidence: 4/5, the payment-guard lifetime is a money path and the device e2e shards had not finished when I looked.
This comment has been minimized.
This comment has been minimized.
643ada4 to
677961b
Compare
|
Rebased onto Validation: compile passed, all 4,243 unit tests passed, and Detekt/formatting diagnostics were compared with a clean copy of the current base. No new violations; the existing |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
|
Aligned the amount-toggle journey with the iOS J1 review: the entered denomination stays exact, while the converted BTC amount uses the current fresh rate. Both implementations reproduced 12,139 → 12,130 sats after a controlled rate refresh, with the original 10 USD unchanged. At an unchanged rate, repeated toggles remain stable. Updated the shared journey and README in 0548200, paired with synonymdev/bitkit-ios#948. Production pricing is unchanged. Amount checks and XML validation passed; Detekt/formatting diagnostics are unchanged from the previously validated report. |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 2 files.
No new findings; the rest is in the review.
Twin: synonymdev/bitkit-ios#948 identical at f2e30b8
QA:
Tests running.
J1 not tested because the covered-balance case needs working Arbitrum access and disposable USDT funds.
J2, J3 not tested because they need controlled USDT receipts, bridge history, and working Arbitrum access.
J4 not tested because the first review needs working Arbitrum access and disposable USDT funds.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
This comment has been minimized.
This comment has been minimized.
|
The iOS J1 clarification identified an amount-screen title issue, fixed in synonymdev/bitkit-ios#948. Android already chooses |
There was a problem hiding this comment.
Verdict: ✅ All 5 device cases passed at da63d6e; one LOW.
Reaudit: diff 1 file. The new title check holds: SendAmountScreen.kt:229 uses usdt__amount, the same title as direct USDT Send, so German never shows "Bitcoin Betrag".
Findings:
- LOW
app/src/main/java/to/bitkit/ui/screens/wallets/send/SendConfirmScreen.kt:362: a USDT-only request still shows a Bitcoin confirmation for about 0.2 s as Preparing ends (₿ 0, orange From/For icons, orange Swipe To Pay), then slides to the USDT Amount screen.navigateToSendRoute(AppViewModel.kt:4965) clearspreparingRequestbeforeNavigateToAmountruns, so the Confirm route briefly renders the plain send state. Preparing itself is teal now, but this is the same Bitcoin accent #1504 reports. Keeping the preparing content until the route has changed would remove it.
QA for da63d6e:
Tested on two Android 15 emulators (dev debug build, German app locale for J1) against the bitkit-docker USDT fork fixture.
Test J1: USDT-only request preparation and amount toggles ✅ Preparing is teal and cannot submit, the payer's title is "Amount", and 10 USD survives every toggle.
- On the requester, 10 USD came back as exactly 10 on every USD/BTC switch. BTC read 12 118 once and 12 128 after a rate refresh.
- A USDT-only 150,50 USD request showed $150,50 on the payer.
- Preparing used teal From/For icons and a teal loading swipe. A swipe during Preparing submitted nothing.
- The Amount screen was titled "Amount", not "Bitcoin Betrag". It held 150.5 USDT, offered only USDT, and ignored keypad input.
- Back closed straight to the request list. Reopening kept the amount and the USDT method, and the balance did not change.
- The Bitcoin flash in the finding shows as Preparing ends (second image).
 |  |  |
j1-payer.2x.mp4 | j1-requester.2x.mp4 |
Test J2: receipt during deposit details ✅ The receipt waited behind deposit details and showed once back on the wallet.
- A 2 USDT transfer confirmed while a deposit's details were open. No sheet appeared there for 45 s.
- Back on the USDT wallet, Received USDT 2.00 showed once. Revisiting deposit details and returning did not show it again.
- A 1 USDT receipt that landed while a transfer's detail was open stayed queued and showed once when the detail closed.
 | j2.2x.mp4 |
Test J3: readable bridge status and copied diagnostics ✅ No raw code on screen; copied details carry both the readable and the technical status.
- A Polygon deposit held with
standing_route_unavailableshowed Needs attention. Its Details showed the readable support message and no raw code. - Its Copy Details carried
Status: Needs attention (held)andStatus code: standing_route_unavailable, plus the deposit reference and source transaction. - An outbound Orchestra transfer to Base in needs attention copied
Status: Bridge needs attention (BRIDGE_NEEDS_ATTENTION), with the bridge reference and source transaction. - Contact Support opened the report form prefilled with the same text and stayed unsent.
 |  | j3.2x.mp4 |
Test J4: full-balance USDT send ✅ The full balance was rejected with the fee message, and a smaller amount reached review.
- With 5.00 USDT available, sending the whole 5 to another owned wallet kept review closed and showed "Unable to pay the transaction fee in USDT. Try a smaller amount."
- 1 USDT reached Swipe To Pay, and review closed unsigned with the balance unchanged.
- Android reached the fee-collection message here. The new journey description says only iOS reaches it first.
 | j4.2x.mp4 |
Test M1: mixed request preparation ✅ A Savings and USDT request prepares with a neutral accent.
- This checks the PR's neutral-accent change, which no journey covers. A 10 USD request accepting Savings and USDT showed Preparing with grey From/For icons and a grey loading swipe.
- The Amount screen then opened with Bitcoin selected, since the USDT balance does not cover 10 USD.
 | m1-payer.2x.mp4 |
Journey steps outside this PR's QA lines were not re-driven: subscriptions, link and scan routing, and refund requests. The J2 injection was not repeated with refund review open.
There was a problem hiding this comment.
Verdict: ✅ Approve. The Bitcoin flash is gone and both re-driven cases passed at fec209b. Two LOW navigation findings.
Reaudit: diff 6 files. The earlier LOW is fixed: on every open, Preparing cuts straight to Amount with no Bitcoin confirmation frame in between.
Findings:
- LOW
app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt:111: after a payment method change from Confirm, Back from the re-review Amount screen lands on that Confirm again, Swipe To Pay included.- A prepared request's sheet now starts at Amount (
AppViewModel.kt:4965).navigateToSendAmountruns itspopUpTo(SendRoute.Confirm)only for a sheet that started at Confirm, so it now skips it. - Path: on a USD-priced request, Amount → Continue → Confirm, then a Savings/Spending switch (From, Instant on the fee screen, or a speed after Instant) runs
reviewPaykitMethodChange(AppViewModel.kt:3594). - The back stack becomes [Amount, Confirm, Amount] instead of [Amount].
- A swipe there still runs
validateIncomingPaymentRequest(AppViewModel.kt:5252), which matches the amount to the live quote for the selected method on Savings and Spending alike. So this is navigation only. SendSheetNavigationTeststill starts prepared requests at Confirm.- Calling
popBackStack(SendRoute.Amount, inclusive = false)before navigating would keep a single Amount. - I found this in the code; no device case reached it.
- A prepared request's sheet now starts at Amount (
- LOW
app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt:344: the prepared request's Amount screen no longer has its top-bar Back arrow.canGoBack = startDestination != SendRoute.Amounthides it now that the sheet starts at Amount.- At da63d6e the arrow closed the sheet (c5c25b1). The J1 step edited in this push still says "Tap Back and verify the send sheet closes".
- System Back and swipe-down still close the sheet.
- Keep the arrow for prepared requests, or reword the step.
QA for fec209b:
Tested on two Android 15 emulators (dev debug build) against the bitkit-docker USDT fork fixture.
Test J1: USDT-only request, Preparing to Amount ✅ On all three opens, teal Preparing cut straight to the USDT Amount screen, with no Bitcoin confirmation in between.
- A 2 USD USDT-only request opened by itself when it arrived, then twice more from Pay.
- Checked frame by frame, the last teal Preparing frame is followed directly by Amount ($2, USDT, Arbitrum One). No frame of the send sheet has Bitcoin orange. The still shows the two frames of the first cut.
- A swipe during Preparing submitted nothing, and the keypad left the amount at $2.
- Continue opened USDT review at $2.00, and Back returned to Amount.
- System Back and swipe-down each closed the sheet without a confirmation screen.
- A swipe-down during Preparing closed the sheet. The next open went cleanly from Preparing to Amount.
- No sheet reopened by itself, and the USDT balance stayed at 10.00.
- The Amount screen shows no top-bar Back arrow (second finding).
j1-payer.2x.mp4
Test M1: mixed Savings and USDT request ✅ Neutral Preparing cut straight to Amount, with USDT preselected.
- A 3 USD request accepting Savings and USDT opened by itself. The payer's 10 USDT covers it.
- Preparing used the grey loading swipe, then cut directly to Amount with USDT selected. The still shows the cut.
- The source picker offered Savings, showed Spending disabled and had USDT checked.
- Choosing Savings showed Bitcoin Amount ₿ 3 658 ($3.00), and switching back restored $3 in USDT. System Back closed the sheet.
- In the second video, with Savings funded, Savings → Continue reached Confirm (₿ 3 658). Back returned to Amount, and system Back closed the sheet. The request stayed unpaid.
m1-payer.2x.mp4 | r1-payer.2x.mp4 |
These cases passed at da63d6e in the earlier review, and this push leaves them untouched:
- J2, whose checks are on USDT wallet and deposit screens the delta does not change.
- J3, whose checks are on bridge details and support screens the delta does not change.
- J4. A USDT link opens a new send sheet, so the route key changes nothing there. The journey edit only widens which error counts, and Android showed the fee error.
- J1's requester toggles and German title.
The review covered the delta da63d6e..fec209b. No device case drove the first finding's path, a Savings/Spending switch from Confirm, because it needs a Spending balance on the payer.
Twin: synonymdev/bitkit-ios#948 at a08e4c1 has the same journey text and full-balance README section. Its latest commit changes only those docs, so the route swap is Android-only.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against its merge base 08011881, at b735f400.
No new actionable code findings.
Each payment attempt now holds its own guard until that attempt finishes. Sign-out and sheet dismissal leave it in place during a software USDT send and during an unresolved hardware broadcast, and overlapping attempts release it only when the last one ends.
Prepared payment requests replace the send sheet route directly. A USDT-only request opens Amount without an empty Bitcoin confirmation; Preparing uses the USDT accent for a USDT-only request and a neutral accent for a mixed request. Changing Savings or Spending returns to the existing Amount screen, and that screen's Back arrow closes the request.
Incoming USDT receipts stay queued until the wallet can show them, and each receipt is consumed once. Deposit details show a readable assistance message instead of a raw provider code, while copied diagnostics keep both the readable status and the technical value.
Shared preparing accent, support text, amount back navigation, and journey wording were checked against bitkit-ios#948 at 37749e5. That was not a review of the iOS pull request. iOS already queues receipts until presentation.
Validation: The build job passed at this revision, including testDevDebugUnitTest. Those tests were not rerun locally. Device testing: not performed in this review.
Ready for device testing.
Suggested additional test cases
- Android, USDT wallet with one transfer's detail open: receive a confirmed USDT transfer. The celebration does not appear over the detail. Close the detail. The celebration appears once. Open that detail again and return to the wallet. The same receipt does not appear again.
- Android, incoming request that accepts both Bitcoin and USDT, while Preparing is visible: the icons and disabled swipe use a neutral accent, not Bitcoin orange, and a swipe does not pay. When Preparing ends, Amount opens without an empty Bitcoin confirmation.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up. Re-reviewed changes since b735f400, including the quoted payment-method review journey and the Android and iOS paths it describes, at b66d83f6. Inherited the completed baseline review for the payment-submission guard, prepared payment-request route replacement and Amount Back, USDT preparing accent, USDT receipt queue, and bridge support diagnostics. The comparison base and merge base are unchanged.
No new actionable code findings.
The new commit updates paykit-quoted-usdt.xml and the journeys README. On Android, a quoted request returns to Amount when a Savings or Spending change selects a different payment method, and that Amount screen's Back arrow closes the sheet. On iOS, confirmation stays open when the recomputed amount, quote, and validity window still match and remain valid; changed or expired terms return to Amount. The shared journey wording matches bitkit-ios#948 at ab313b1e. That inspection covered only this payment-method review contract.
Validation: The CI build passed at this revision, including testDevDebugUnitTest. This review did not rerun those tests. Device testing: not performed in this review.
Ready for device testing.
Android navigation retest — passedFocused Android navigation retest passed at b66d83f using the existing CI devDebug APK on Pixel 6 / Android 13, regtest.
Both requests remain unpaid; balances were unchanged. No payment was authorized and no build was triggered. This is a targeted Android navigation retest; it does not claim a fresh full-suite pass or an iOS device run. Existing artifact: CI run 38865298271, This rechecks the two navigation findings in the earlier review. Current code-review scope remains in the b66d83f6 follow-up. Savings Confirm → Spending Amount → Backmixed-direction1.mp4Spending Confirm → Savings Amount → Backmixed-direction2.mp4Three method-change/review cycles, dismissal and reopenrepeat-review.mp4USDT-only Preparing → Amount, Back and reopencurrent-usdt.mp4Final reopened USDT Amount and both requests still unpaid
|
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approved based on the code review, verified baseline evidence, and completed Android navigation retest: #1506 (comment). No outstanding blocking findings in the reviewed scope.
There was a problem hiding this comment.
Verdict: ✅ Approve. Both LOW findings from the last review are fixed in b735f40, and the delta adds none.
Posted as a comment: Ovi holds approvals on this PR.
Reaudit: diff 5 files.
- Back after a method change is fixed.
navigateToSendAmount(SendSheet.kt:112) now returns to the existing Amount, so a Savings/Spending change from Confirm leaves a single Amount. The same pop also clears the fee screens when the change comes from there (Instant, or a speed after it). - The missing Back arrow is fixed. Payment requests show it again (
SendSheet.kt:345). On a request's first Amount, the pop empties the back stack and returns false, so the arrow closes the sheet.
Findings: N/A
QA for b66d83f:
No device run here. Every case passed at this head, or at an earlier head whose later pushes leave it untouched.
- Piotr's retest drove both fixes at b66d83f:
- A Savings/Spending change from Confirm, in each direction, returned to Amount. Its top-bar Back closed the sheet without the old Confirm.
- Three change-and-review cycles left no stacked confirmations.
- A USDT-only request went from teal Preparing straight to Amount.
- J1's other payer checks passed at fec209b in the last review: a swipe during Preparing submits nothing, and Continue opens USDT review, whose Back returns to Amount. The delta does not change USDT review's Back.
- M1, the mixed Savings and USDT request, passed at fec209b. A source switch on Amount now reaches the new pop, which finds Amount on top and removes nothing, so it navigates as before. The arrow that request now shows is the one Piotr drove.
- J1's requester checks, J2, J3 and J4 passed at da63d6e in the review before. The delta changes only the send sheet's Amount navigation. A USDT link is not a payment request, so J4 gets no arrow and has no Amount underneath.
The review covered fec209b..b66d83f. No device case drove a method change from the fee screen (Instant, or a speed after it); it reaches the same pop, checked in the code.
Twin: synonymdev/bitkit-ios#948 at ab313b1 has the identical journey and README section; its last two commits change only those docs.
5 participants
Closes #1498, closes #1502, closes #1503, closes #1504
Twin: synonymdev/bitkit-ios#948
This PR fixes payment lifetime and USDT feedback issues.
Description
Out of Scope
Design
Payment requests · Bridge technical details
Preview
QA Notes
Journeys
paykit-quoted-usdt.xml— USDT-only request preparation stays teal, opens Amount without an intermediate Bitcoin confirmation, and cannot submit before review; denomination switches restore the entered amount, with identical BTC equivalents expected only at an unchanged rate. USDT amount titles match direct Send, including in German; the smaller BTC figure is a wallet-rate equivalent. For the optional funded regtest check, Android reopens Amount after Savings/Spending changes; iOS keeps confirmation open when the exact payment terms remain unchanged and valid. When Amount is reopened, its Back arrow closes the request without revisiting confirmation.usdt-bridge-details.xml— a receipt arriving during deposit details appears once after returning to the wallet.usdt-bridge-support.xml— bridge problems use readable messages; copied diagnostics retain readable and technical status.usdt-payment-links-and-scans.xml— full-balance preparation accepts either actionable fee or balance error without opening review.Manual Tests
N/A
Automated Checks
AppViewModelSendFlowTest.kt— a held USDT attempt survives sign-out and sheet dismissal; receipts remain queued when presentation is blocked.PaykitPaymentRequestRepoTest.kt— clearing requests leaves waiting work blocked until the active payment ends.SendConfirmScreenTest.kt— a USDT-only preparing request uses the USDT accent despite a previous Lightning selection and cannot pay.AppViewModelSendFlowTest.kt— preparation resolves directly to Amount and Continue still opens USDT review.SendSheetNavigationTest.kt— repeated amount review removes stale confirmations while contact payments retain their previous screen.AppViewModelSendFlowTest.ktandHwSendViewModelTest.kt— each hardware attempt releases only its own guard, including cancellation during an unresolved broadcast.UsdtRepoTest.kt— receipt retention, one-time consumption, first-sync suppression and wallet reset.UsdtSupportDetailsTest.ktandUsdtBridgeDetailsTest.kt— readable status in UI and copied details, with raw codes kept only in diagnostics.SendConfirmScreenTest.ktandUsdtBridgeDetailsTest.kton a disposable Android emulator — 9 tests passed.The full local unit suite passed (4,243 tests). The preparation transition also passed all 443 focused send-flow tests. No funded mainnet payments were needed; the changed journeys remain listed for reviewer device verification.