Repository navigation
fix: preserve payment guards and usdt feedback - #1506
ben-kaufman wants to merge 6 commits into
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.
|
|
||
| val accentColor = when { | ||
| isPreparing -> Colors.Brand | ||
| preparingRequest != null -> if (preparingRequest.acceptedPaymentEndpointIdentifiers.toSet() == |
There was a problem hiding this comment.
As Preparing ends for a USDT-only request, the sheet shows a Bitcoin confirmation for about 0.2 s: ₿ 0, orange From/For icons and an orange Swipe To Pay. Then it slides to the USDT Amount screen. This happened on both the first open and the reopen. navigateToSendRoute (AppViewModel.kt:4965) sets preparingRequest = null before NavigateToAmount is handled, so the Confirm route renders the empty send state for a moment, and this accent falls through to Colors.Brand. That is the Bitcoin accent #1504 reports, now brief. Setting the effect before clearing preparingRequest, or keeping the preparing branch until the route has changed, would remove it.
There was a problem hiding this comment.
Fixed in fec209b. Preparing now resolves directly to the appropriate root screen, so USDT requests open Amount without retaining or briefly rendering the empty Bitcoin confirmation. The existing ownership and cancellation checks remain in place.
Extended the existing send-flow test to check the direct transition and that Continue still opens USDT review. All 4,243 unit tests pass, including 443 focused send-flow cases; compile and lint/format checks pass with no new diagnostics. Please recheck J1’s transition on the new build.
Also corrected the paired journey and README: the full-balance fee-collection error can occur on either platform. The matching iOS documentation is in #948.
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.
| if (!paymentRequestPreparation.compareAndSet(preparation, null)) return | ||
| _currentSheet.update { | ||
| if (it === preparation.sheet) preparation.sheet.copy(preparingRequest = null) else it | ||
| if (it === preparation.sheet) preparation.sheet.copy(route = route, preparingRequest = null) else it |
There was a problem hiding this comment.
With the sheet now starting at Amount, navigateToSendAmount (SendSheet.kt:111) skips its popUpTo(SendRoute.Confirm), which runs only when the sheet started at Confirm. On a USD-priced request, Amount → Continue → Confirm, then a Savings/Spending switch runs reviewPaykitMethodChange. The back stack becomes [Amount, Confirm, Amount] instead of [Amount], so Back from the re-review lands on that Confirm with Swipe To Pay. A swipe there still runs validateIncomingPaymentRequest (line 5252), which matches the amount to the live quote for the selected method on Savings and Spending alike, so this is navigation only. Calling popBackStack(SendRoute.Amount, inclusive = false) before navigating would keep a single Amount. SendSheetNavigationTest still models prepared requests as starting at Confirm.
There was a problem hiding this comment.
Fixed in b735f40. Amount navigation now returns to an existing Amount entry before creating a new one, removing the intervening confirmation. Extended SendSheetNavigationTest to cover the actual Amount-root flow, repeated re-review, and preserving the contact screen beneath Amount. It reproduced the stale Confirm before the fix and passes now; all 4,243 unit tests pass with no new lint/format diagnostics.
Agreed this is a navigation issue: send-time quote validation is unchanged. iOS already clears its path when Amount is the root; its paired journey now includes the method-change/Back check too.
| preparingRequest = sheet.preparingRequest, | ||
| hwSendViewModel = hwSendViewModel, | ||
| ) | ||
| key(sheet.route) { |
There was a problem hiding this comment.
Rebuilding SendSheet at startDestination = Amount also hides the Amount screen's top-bar Back: canGoBack = startDestination != SendRoute.Amount (SendSheet.kt:344). At da63d6e that arrow closed the sheet (c5c25b1), and the J1 step edited in this push still says "Tap Back and verify the send sheet closes". System Back and swipe-down still close it. Keep the arrow for prepared requests, or reword the step.
There was a problem hiding this comment.
Fixed in b735f40. Payment requests now retain the Amount top-bar Back arrow even when Amount is the root. It uses the existing handler to close the sheet when there is no earlier screen. The paired journey explicitly says to tap that arrow and also checks Back after a Savings/Spending re-review. iOS already displays the arrow. Please recheck on the updated build.
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.
This branch has not been deployed
4 participants
Fixes #1498
Fixes #1502
Fixes #1503
Fixes #1504
This PR fixes payment lifetime and USDT feedback issues. Paired iOS fixes: synonymdev/bitkit-ios#948.
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.