Repository navigation
fix: authorize initial USDT subscriptions once - #1568
ben-kaufman wants to merge 9 commits into
Conversation
 [Critical impact] The change reaches the wallet's USDT payment flow for initial subscriptions. The PR appears safe to merge, with a non-blocking test gap in the automatic payment handoff. Findings
SummaryEligible initial USDT subscription payments now continue from Subscribe without a second payment swipe.
Tests were inspected, not run. No device UI capture was made. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Swipe Subscribe] --> B{Eligible initial USDT payment?}
B -->|No| C[Existing amount and payment review]
B -->|Yes| D[Open USDT review]
D --> E[Consume one-use authorization]
E --> F[Prepare quote]
F -->|Failed| G[Show error; require explicit retry]
F -->|Ready and authorized| H[Confirm payment]
H --> I[Required warnings and authentication]
I --> J[Check terms, deadlines, and previous attempts]
J --> K[Send payment]
K --> L{Transfer confirmed?}
L -->|No| M[Pending or failed payment view]
L -->|Yes| N[Subscribed]
Reviews (1) · Last reviewed commit: "fix: reuse initial USDT subscription aut..." · 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). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. An initial USDT subscription payment now goes straight from Subscribe to the USDT review and confirms itself once. It only does that when the terms leave the amount unchanged: USDT with no conversion, or USD with a fixed 1:1 USDT rate. Reviewed 2c486a9, full tier.
What I checked, and 6 candidates I ruled out
Read in full: PaykitUsdtReview.kt, the UsdtReview route in SendSheet.kt, UsdtViewModel.kt (quote, confirm, send, waitForTransfer), ContactPaymentContext.initialUsdtSubscriptionPayment, openPaykitEndpoints, takeInitialUsdtSubscriptionAuthorization, sendPaykitUsdt / authorizeUsdtExecution, PaykitUsdtPaymentRepo.prepare / validateQuote / validateRequestTerms, PaykitRequestPricing.payment
Call sites traced: initialSubscriptionPaymentAutoStartPending (set in resetSendState, consumed by onStartInitialSubscriptionPayment on the Bitcoin path and by the new taker on the USDT path), isInitialSubscriptionPayment = true (accept, failure, in-sheet retry), AutoPayOverlay, TransactionConfetti
CI: green: build (with testDevDebugUnitTest), lint, detekt, all seven e2e-tests-local shards, e2e-status
Not built here: this review was reasoned from the code and from CI.
Ruled out
- Auto-pay runs again after a rotation or recomposition:
LaunchedEffect(Unit)does re-run, but the taker clears the pending flag before it returns, and the second call returns false. The new test asserts that. - Auto-pay skips the PIN or the sanity warnings:
preparePaymentcalls the sameconfirm()as the swipe, sowarnings()andisPinForPaymentsEnabledstill gatesend(). - Auto-pay with terms that differ from the ones checked: the taker recomputes the terms from
activeContactPaymentContextand compares them withpaykitPaymentTermsandpaykitReviewedAmount.validateRequestTermscompares them again at submission. If they differ, it falls back to a manual swipe, never to a different amount. - Rate selection disagrees with
payment(): both look upusdt-arbitrum, thenusdt. The test also covers a 1:1usdtrate shadowed by a 2:1usdt-arbitrumone, which is rejected. - Hardware wallet or amount review paths auto-pay:
shouldAutomaticallyPayexcludeshardwareWalletIdandrequiresAmountReview. The non-eligible route still goes through Amount, and the taker returns false there. - Back is stuck when
UsdtReviewis the start destination: the newpopBackStack()-or-hideSheet()fallback matches the pattern already used atSendSheet.kt:515andPinSheet.kt:52.
Merge confidence: 4/5. The auto-confirm call in preparePayment moves money without a swipe and has no test of its own: the tests cover eligibility and the one-use taker, not the composable that calls confirm().
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 21fd91e. This is the first assessment of pull request 1568.
No new actionable code findings.
An eligible initial USDT subscription quotes and confirms once after Subscribe. Authorization is consumed before confirm and must still match the stored terms, reviewed amount, expiry, and automatic-pay guards. Quote failure, authentication cancellation, and leaving while preparation is in flight do not submit a payment or start another automatic attempt. A quote whose amount or terms differ from the authorized payment is rejected at send. Back closes the sheet when USDT review is the first destination. A confirmed transfer shows Subscribed with the existing USDT confetti, and the subscription progress layout stays up while the payment is busy.
The shared eligibility rules and journeys/subscriptions/review-and-subscribe.xml match bitkit-ios#1006 at ba31357. That comparison covers only those shared contracts.
The open Greptile thread asked for coverage of the automatic confirm path. At this revision, UsdtViewModelTest drives preparePayment through quote completion, authentication cancellation, explicit retry, failed recovery, in-flight cancellation, and a single submission. I inspected those tests and did not re-run them. The CI build on this commit completed testDevDebugUnitTest successfully. Journey files are not executed by that workflow.
Device testing: not performed in this review.
Ready for device testing.
Suggested additional test cases
- Android, dev build with the local USDT stack. Create a subscription that accepts both Bitcoin and USDT, or a USD subscription whose fixed USDT rate is not 1:1, with the first period due. Swipe Subscribe. Expected: the amount screen remains, no USDT payment is sent, and payment still requires an explicit review and swipe.
- Android, payer with payment warnings enabled and a balance that triggers an amount or fee warning. Swipe Subscribe on an eligible USDT-only proposal. When the safety dialog appears, tap Cancel. Expected: no payment is recorded, preparation does not start again on its own, and the unpaid period can still be paid by a later explicit authorization.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- One-shot authorization:
takeInitialUsdtSubscriptionAuthorizationclears the flag before returning. It returns true only when the recomputed terms equalpaykitPaymentTermsand the amount equalspaykitReviewedAmount. Reopening a period from Payments is not marked initial, so it never auto-pays. - Changed amount: a refreshed request with a different amount falls back to a manual swipe, and
validateQuoteandauthorizeUsdtExecutioncheck quote = reviewed = terms again at send. - Dismiss:
hideSheetclears the context,_sendEffecthas no replay, and dismissing during the quote cancels theLaunchedEffectbeforeconfirm(). - Double pay:
validateReplacementthrowsOperationInProgresson an unresolved attempt, andisSubmittingPaymentRequestguards resubmission.
review-and-subscribe.xml is byte-identical to synonymdev/bitkit-ios#1006.
Device gate (partial): not run yet. Pending: subscriptions/review-and-subscribe.xml on a funded local USDT stack, plus timing from the Subscribe swipe to Subscribed (perf-relevant: a new user-facing wait under the progress overlay, with Back blocked) against master. The device result follows in a second review.
|
Outside this PR's scope; no change needed here. The USDT payment sheet ignores the bottom navigation-bar inset. On the device gate, after cancelling the PIN on an initial USDT subscription payment, the "Payment Request" review came back with Swipe To Pay under the gesture handle: 
Cause: It's already fixed in #1564, which adds |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 21fd91e. The code scan from my earlier review stands; this review only adds the device result.
Device gate: 21fd91e — journeys: subscriptions/review-and-subscribe.xml 23/23 actions pass on Android (payer) against synonymdev/bitkit-ios#1006 as the creator; action 13 failed once and passed on re-run, see below; perf: USDT subscribe, swipe → Subscribed 80.3 s with one swipe vs master's swipe → Sent 102.4 s with Amount, Continue and a second swipe (automated taps; both wait ~35 s for the private payment list after acceptance); Figma: not compared (routing change, no new layout); l10n: no UI text changed.
Setup: Pixel_9 with on-chain sats and no channel, 60 USDT from the local fixture. The creator was the iOS twin on the iPhone 17 simulator, the two linked as contacts. Proofs reached the creator for every paid subscription ("1 subscriber, 1 payment"; "0 payments" for the unaffordable one).
Covered:
- BTC first period paid without a second swipe (Subscribed 95 s after the swipe).
- USDT first period paid through the fee warning with no amount entry.
- PIN on: swipe, then cancel the PIN; nothing was paid, Back closed the sheet, and reopening the unpaid period and authorizing paid once (balance 57.222109 → 54.546773).
- $100 over balance: "Not enough USDT to cover the amount and fees." with the period still offered and the balance unchanged.
- Overview: one MONTHLY COST total across BTC and USD subscriptions.
Action 13 ("billing period no longer offered"): right after Close on the BTC subscription, the Payments tab still listed the period with Pay for a few seconds. It was gone on re-run ~30 s later. This PR doesn't change the BTC path, so I'm treating it as refresh lag, not a finding.
UsdtViewModelTest and the code trace in my earlier review.
Out of scope, tracked in #1571: after the PIN cancel, the USDT "Payment Request" review puts Swipe To Pay under the gesture handle. It's the same on master, and #1564 fixes it.
CI at 21fd91e: green.
|
Fixed the matching manual-preparation issue found in the iOS review in 1711f13. Review & Subscribe progress now appears only when the initial subscription authorization was actually consumed; manual quote preparation and explicit retries keep the normal payment review. Quote, fee and authentication checks are unchanged. Added delayed-preparation coverage through the real review helper and UsdtViewModel, and extended the cancellation/re-entry test. All three PR commits are signed and verified locally and by GitHub. Re-requesting review. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 💬 Comment: one LOW, reproduced on device. The automatic, manual and failed-preparation routes paid at most once.
The PR moved to 1711f13 after this run; these results are for 21fd91e.
Findings:
- LOW
app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt:338: tapping the locked amount on the direct USDT review closes the payment sheet.- On the new direct route (
AppViewModel.kt:3564),UsdtReviewis the only entry in the sheet's back stack. - The review's amount header is still tappable while the payment is not busy (
UsdtSendContent.kt:547). ItsonPageChangeisback()(PaykitUsdtReview.kt:116), sopopBackStack()returns false andhideSheet()closes the sheet. - In Test 4, after a cancelled PIN, one tap on $2.00 closed the sheet. Nothing was paid, and the period stayed payable from Payment Requests.
- Before this PR the same review sat on top of the locked Amount screen, and the tap went back there. The amount is not editable on this route, so the tap can do nothing here.
- The code is unchanged at 1711f13.
- On the new direct route (
QA for 21fd91e:
Two Android 15 emulators, one creating and one paying subscriptions, on a local Arbitrum fork with test USDT. Balances were checked on the fork before and after each case.
Test 1: safety warning cancelled on an eligible initial payment ✅ Cancel sent nothing and started nothing again; a later explicit payment paid once.
- The creator proposed $6 monthly, USDT only. The payer held 10 USDT.
- Swipe To Subscribe kept the Review & Subscribe layout while it prepared, then showed the Payment Request review with the over-50% warning. There was no Amount step.
- Cancel left the locked Amount screen. Nothing changed for 40 s, and the balance on the fork stayed at 10 USDT.
- Continue, Swipe To Pay and Yes, Send reached Subscribed. That step fell in a gap between recordings; the second clip starts after it.
- The second clip shows a balance of 3.17 USDT, one −6.83 row in activity and one payment on the subscription. The creator's list shows "1 subscriber, 1 payment".
https://github.com/user-attachments/assets/81bb341e-6fef-4f4d-8ec2-0f774be5b5c0
https://github.com/user-attachments/assets/afb9cd70-4bf2-4dbc-b7da-93199ab69bf9
Test 2: USD subscription with a 2:1 USDT rate ✅ The payer got the manual Amount and review steps, and nothing was paid without a swipe.
- The creator proposed $1 with a fixed rate of 2 USDT per USD. Bitkit's own creator never sends that rate, so a test build of the creator stood in for an external issuer.
- Swipe To Subscribe opened Amount at $2, USDT on Arbitrum One, with Continue.
- Continue opened the $2.00 Payment Request review, which waited at Swipe To Pay with no warning or PIN prompt. I closed it without paying. The balance stayed at 28.17 USDT, and the creator shows 0 payments (creator's list under Test 1).
- Between Continue and the review, Review & Subscribe at $1.00 with a spinning swipe showed for about 5 s. This is the paying overlay on the manual route that the pair PR's review raised, which 1711f13 now limits to automatic payments. I did not retest it.
T2-2x.2x.mp4
Test 3: failed preparation, then an explicit retry ✅ Preparation failed without sending or retrying, and the explicit retry paid once.
- The fork's fee provider was set to issue fee authorizations that had already expired. Then the payer swiped to subscribe to a $2 USDT-only proposal.
- The payment stopped on Amount with "The fee quote expired. Review the payment again." It stayed there for 40 s, and the balance stayed at 28.17 USDT.
- With the provider working again, Continue prepared a new quote and waited at Swipe To Pay. Swipe, the fee warning and the PIN led to Subscribed.
- The balance went from 28.170809 to 25.495473 USDT, which is one $2 payment plus fee. The subscription lists one payment, and the creator shows "1 subscriber, 1 payment" (creator's list under Test 1).
- This manual payment also showed the Review & Subscribe overlay while sending, the same case Test 2 notes.
T3-2x.2x.mp4
Test 4: amount tapped after a cancelled PIN ❌ The tap closed the whole payment sheet.
- PIN for payments was on. The creator proposed $2 monthly, USDT only.
- Swipe To Subscribe, then Yes, Send on the fee warning, opened the PIN screen. Back cancelled it, and the Payment Request review came back with Swipe To Pay.
- Tapping $2.00 closed the sheet and returned to the USDT wallet.
- Nothing was paid: the balance stayed at 28.17 USDT, and the creator shows 0 payments (creator's list under Test 1). The period was still listed under Payment Requests with Pay and Dismiss.
T4-2x.2x.mp4
Not driven:
- Paykit quote terms expiring or being replaced during preparation (this PR's Manual Test). Test 3 expires the USDT fee authorization, not the request's quote terms, so this case rests on
UsdtViewModelTest. - A request accepting both Bitcoin and USDT. Bitkit's creator cannot send one, so that half of @piotr-iohk's first case rests on
AppViewModelSendFlowTest.
Relied on:
- @jvsena42's device run at 21fd91e of
review-and-subscribe.xml(23/23). It covers the first USDT period paid automatically through to Subscribed, PIN cancel with a retry, the over-balance error and the BTC first period. I did not re-drive those. - Test 1 is @piotr-iohk's second suggested case, and Test 2 is the non-1:1 half of the first.
Pair PR synonymdev/bitkit-ios#1006 matches in eligibility: accepted methods must be exactly usdt-arbitrum, and the usdt-arbitrum, then usdt, rate must be 1:1.
Full review of 402aecb...21fd91e, 10 files. This PR moved to 1711f13 during the run. That commit is not tested here; I read its diff only for the two notes above.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of f6f2c631 since 21fd91e, inheriting the completed baseline review. This pass covered the two later commits: automatic-payment progress only after a fresh takeInitialUsdtSubscriptionAuthorization result, locked review amounts staying on the payment review while Back still closes the sheet and ordinary send amounts stay editable, submitted-payment reconciliation, and the updated review-and-subscribe.xml steps. Unchanged eligibility, direct USDT routing and one-use authorization, send-time quote checks, and duplicate-submit guards remain covered by that baseline.
No new actionable code findings.
Manual preparation quotes and leaves confirmation to an explicit swipe, so the subscription progress overlay stays on an authorized automatic payment. A locked amount tap on the direct USDT review leaves that review open. Back still closes a sheet whose first destination is USDT review. After a confirmed initial transfer, Subscribed has no back button, and system back closes the sheet. journeys/subscriptions/review-and-subscribe.xml is byte-identical to bitkit-ios#1006 at 72814ca6.
The amount-tap dismissal reported on 21fd91e is addressed at this revision. I inspected UsdtViewModelTest and UsdtPaymentContentTest and did not run them. The build workflow passed on this commit; journey files are not part of that workflow. Device results posted for 21fd91e predate these commits.
Device testing: not performed in this review.
Ready for device testing.
Suggested additional test cases
- Android, dev build with the local USDT stack. Propose a USD subscription whose fixed USDT rate is not 1:1, with the first period due. Swipe Subscribe, then Continue from the amount screen. Expected: while the quote is prepared, the Review and Subscribe progress overlay stays hidden; the payment review waits for an explicit swipe, and nothing is sent until that swipe.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since my approval at 21fd91e (1711f13, f6f2c63): no findings.
- The overlay is now
AutoPayOverlay(isVisible = automaticPayment && state.busy). The flag comes only from the one-shot taker and is cleared by an explicit Continue or swipe. - Warning Cancel, PIN cancel and quote failure all leave
busyfalse. The fee warning andUsdtAuthenticationare never covered. - A manual initial payment and a period reopened from Payments no longer show "Review & Subscribe" before the user authorizes.
amountEditable = falsemakes the locked amount a no-op only on this route. Ordinary USDT sends keep amount navigation.- No payment trigger changed:
preparePaymentstill confirms only ontakeInitialUsdtSubscriptionAuthorization()with a quote.
#1571 still applies: PaykitUsdtReview line 98 still has no navigationBarsPadding().
Device gate (partial): 21fd91e ran the full review-and-subscribe.xml. Not run yet at f6f2c63: the two changed actions in subscriptions/review-and-subscribe.xml (overlay only during the automatic payment; locked amount on the manual route), on Android against synonymdev/bitkit-ios#1006. The device result will follow.
|
Resolved the master conflict in 47030f9, retaining the locked-amount/Back regression alongside master's decimal-entry and enlarged-header tests. Existing authorization, quote and duplicate-payment safeguards are unchanged.
|
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The locked-amount dismissal is fixed and verified in Test 2.
Reaudit: 21fd91e…47030f9, 74 changed files including the master merge; 12 files differ from the current base.
Pair PR: synonymdev/bitkit-ios#1006 fix: authorize initial USDT subscriptions once at 72814ca matches the eligibility rules, manual progress behavior, locked-amount fix and shared journey. I did not drive iOS in this run.
QA for 47030f9:
Tested on two Android 15 / API 35 Pixel 9 emulators using test USDT on a local Arbitrum fork. Creator is left, payer is right; videos play at 2× speed. Tests 1, 2, 3 and 5 used the unmodified head on the payer. Disposable issuer hooks supplied the non-1:1 and mixed-method proposals.
Test 1: Automatic initial payment ✅ One Subscribe swipe reached Subscribed; Back dismissed it.
The fee warning remained actionable, and no payment swipe was required after Subscribe. Subscribed had no Back arrow. The period appeared once in payment history without Pay or Dismiss. Fork balance confirmed one 2 USDT payment plus its fee.
T1.2x.mp4
Test 2: PIN cancellation and manual retry ✅ The locked amount kept review open; explicit retry paid once.
Canceling PIN sent nothing, checked against the fork balance. Tapping the locked amount retained Payment Request; Back closed it. Reopening the unpaid period required Continue and a manual payment swipe. Preparation stayed on normal payment UI, and PIN authorization reached USDT Sent. Payer history and creator showed one payment.
T2.2x.mp4
Test 3: Non-1:1 conversion ✅ A 2:1 rate required manual confirmation without subscription progress.
The $2 proposal opened Amount at 4 USDT. Continue prepared the normal Payment Request and waited at Swipe To Pay. Closing unpaid left the fork balance unchanged and the creator at zero payments.
T3.2x.mp4
Test 4: Conversion quote expiry and recovery ✅ Expired terms prevented sending; explicit reopening paid once.
A disposable payer debug hook injected a per-period conversion quote with 10-second validity and held preparation for 15 seconds; payment guards remained unchanged. After the manual swipe, fee warning and PIN, the app rejected the request. Diagnostics confirmed expired Paykit terms, a valid native USDT quote and matching amounts; the fork balance was unchanged. Reopening loaded the original valid fixed terms and required manual authorization, then showed USDT Sent and one history row without Pay. The fork debit was one 2 USDT payment plus its fee; the creator showed one incoming row pending proof reconciliation. Quote replacement was not separately exercised.
T4.2x.mp4
Test 5: Mixed accepted methods ✅ Accepting Bitcoin and USDT kept the payment manual when only USDT resolved.
The issuer declared both methods while publishing only USDT private endpoints. The payer exposed only USDT at Amount, then prepared normal Payment Request and waited for Swipe To Pay. Closing unpaid left the fork balance unchanged and the creator at zero payments.
T5.2x.mp4
Unchanged cases were not repeated:
- Earlier device report at 21fd91e: BTC first period, insufficient funds and monthly cost overview.
- Earlier review and device report at 21fd91e: safety-warning cancellation and failed fee-authorization recovery.
- Author's component report at f6f2c63: ordinary editable-amount navigation. I did not repeat that control on device; the merged component tests retain it.
Focused authorization, request-pricing and USDT repository/view-model checks passed on the unmodified head. Master integration received source review and these checks; the device cases above cover the changed subscription paths.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Resolved the conflict with the merged fixed-quote confirmation flow. Eligible initial USDT subscriptions retain their one-use authorization; fixed Bitcoin requests use the new direct confirmation path. Added scoped coroutine opt-ins required by master’s stricter compiler checks. Validation: 4,380 unit tests passed, Detekt reported zero findings, and Android Lint reported no new violations (the unchanged master baseline was applied). The complete PR commit range is signed and verified locally and on GitHub. Re-requesting review of the integration update. |
|
Device gate (partial):
Deviation: reopening the unpaid period from Payments landed on a locked Amount step ($2.00, keypad greyed, Continue), not directly on the payment review as the updated action 21 says, and the row detail showed status "Unavailable". One Continue led to the review and the payment went through. It wasn't re-run. Either the journey text or the reopen route needs to change; can you confirm which is intended? Perf: 55–58 s from the Subscribe swipe to Pending: the iOS synonymdev/bitkit-ios#1006 payer direction. The approval waits for it and for the deviation above. |
|
Thanks for running the journey. The reopened unpaid period deliberately uses the normal Amount → Continue → payment review flow: the completed Subscribe authorization is not reused for a manual retry. I corrected that step in both apps’ shared journey. The “Unavailable” label was a real display bug on both platforms. Payable unpaid periods now show “Waiting for payment”; periods whose payment deadline has passed show “Expired”, even before list refresh. This changes only presentation; fee/quote checks, authentication, one-use authorization and duplicate-payment safeguards remain unchanged. Regression coverage checks payable, expired, unavailable and proof-submitted states. The matching iOS fix is in #1006 (synonymdev/bitkit-ios#1006). The reported private-list wait also occurred on master in your run. The separate payment-latency work remains responsible for that investigation; this PR does not make a speculative latency change. Pushed as d5ae1af. All 4,381 unit tests passed, followed by a focused final rerun after formatting cleanup; compilation, Detekt (0 findings) and Android Lint (unchanged baseline only) pass. All PR commits are signed and verified locally and on GitHub. Re-requested review. |
|
Delta since
Device gate (partial): pending at |
|
Fixed the same expiry/history gap reported in iOS #1006 in 1f55d03. Expired unpaid periods of active subscriptions remain visible in Payments after expiry and refresh/restoration, without a Pay action. The next billing period remains separate and payable; paid proofs take precedence and canceled subscriptions do not generate unpaid periods. Validated through the real repository expiry job and refresh/restoration paths: all 48 subscription repository tests and all 4,382 unit tests pass. Compilation, formatting and Detekt pass; Android Lint reports no new findings, with its baseline unchanged. The shared journey now includes expiry and relaunch checks. |
There was a problem hiding this comment.
Verdict: ⛔️ Request changes at d5ae1af — expired unpaid periods disappear.
The PR moved to 1f55d03 after this run; these results are for d5ae1af.
The PR advanced to 1f55d03 during this run. The author reports the expiry/history fix in that push; these results cover d5ae1af, and do not verify the newer head.
Findings:
- MEDIUM
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt:507: the Expired status cannot retain an unpaid period after its deadline. The expiry job removes it from pending requests, while recurring history retains only submitted/completed periods. The open detail becomes Unavailable and Payments loses the row. Test 3 reproduces this on the unmodified reviewed head. Retaining the expired period as non-actionable history is needed for this status to render.
QA for d5ae1af:
Two Android 15 / API 35 emulators, with local-fork test USDT and regtest Bitcoin. Creator is left, payer is right; recordings play at 2×. Tests 1–3 and 5 used the unmodified payer build. Test 3 used a creator-only deadline hook; Test 4 used a disposable payer terms-replacement hook, with payment guards unchanged.
Test 1: Automatic initial USDT payment ✅ One Subscribe swipe reached Subscribed and recorded one payment.
The fee warning remained actionable; no second payment swipe was required. Subscribed had no Back button, and system Back dismissed it. Subscription detail showed one payment and November 20 renewal; history showed Proof submitted without Pay. The fork debit was one 2 USDT payment plus its fee.
T1.2x.mp4
Test 2: PIN cancellation and unpaid-period retry ✅ Waiting for payment appeared, and explicit retry paid once.
Cancelled PIN sent nothing, checked against the unchanged fork balance. Tapping the locked amount kept review open; Back closed it. From Payments, the detail showed Waiting for payment. Pay opened fixed Amount; Continue used ordinary preparation and waited for a swipe and PIN. The payment reached USDT Sent, then Proof submitted without Pay. The fork debit was one payment plus fee.
T2.2x.mp4
Test 3: Unpaid period after its deadline ❌ The detail became Unavailable and the period disappeared from Payments.
A creator-only hook supplied a signed 180-second period-start deadline. Insufficient funds prevented the initial 100 USDT payment. Before expiry, the detail showed Waiting for payment and Pay. At the cutoff, the open detail became Unavailable; returning to Payments showed no row for that period. No transfer occurred, checked against the unchanged 44.495563 USDT fork balance.
T3.2x.mp4
Test 4: Quote replacement and explicit recovery ✅ Stale selected terms were rejected; reopening with fresh terms paid once.
A disposable in-memory hook selected valid 1:1 per-period terms, held preparation, then replaced the active quote ID without changing the amount. Swipe and PIN rejected payment. Diagnostics confirmed a valid native quote, valid selected terms, and differing selected/current Paykit quote IDs; the fork balance was unchanged. Reopening loaded the original valid fixed terms and required manual authorization, reaching USDT Sent and Proof submitted without Pay. The fork debit was one 2 USDT payment plus fee. This exercised the real send guards, rather than a signed replacement arriving over the wire.
T4.2x.mp4
Test 5: Bitcoin first payment ✅ One Subscribe swipe and PIN completed one Savings payment.
This checks integration with synonymdev/bitkit-android#1581 fix: confirm fixed Paykit requests directly. The 5,000-sat subscription went from Subscribe to payment PIN, then Subscribed without a second payment swipe. The app logged one accepted on-chain send. After Close, subscription detail showed one payment and November 20 renewal; Payments showed Proof submitted without Pay.
T5.2x.mp4
Retained coverage for unchanged paths:
- The earlier review at 47030f9: non-1:1 conversion, mixed accepted methods, and expired conversion terms with explicit recovery.
- The earlier device report at 21fd91e: monthly cost overview and insufficient-funds handling.
- The earlier review at 21fd91e: safety-warning cancellation and failed fee-authorization recovery.
No planned cases remain unrun. Focused routing, USDT view-model/recovery and presentation checks passed. Delta review covered 47030f9…d5ae1af, including the master merge; merged hardware, backup/wipe and build/lint changes received source review without separate device cases.
Twin synonymdev/bitkit-ios#1006 fix: authorize initial USDT subscriptions once at a3d15fb has the identical retry journey and the same reported expiry failure.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 1f55d03 after the comparison base changed since the saved assessment. No baseline coverage was supplied, so this pass covers the whole diff.
No new actionable code findings.
Eligible initial USDT subscriptions open review and confirm once: USDT with no conversion, or USD with a fixed 1:1 USDT rate, and USDT as the only method. The authorization is consumed before the quote returns. A failed, cancelled, or replaced quote stays on explicit retry. Send still checks the quoted amount, payment terms, deadline, warnings, and PIN or biometrics. Tapping the locked amount stays on review; Back closes the sheet.
Expired unpaid periods of an active subscription leave the payable list and remain in Payments without Pay, including after refresh and restore. The next period stays separate. Paid proofs still take precedence, and cancellation drops the unpaid expired period. That is the gap reproduced at d5ae1af (review thread); the current source retains those periods.
Compared the shared eligibility, history, and journey contract with bitkit-ios#1006 at 9450bdb. The journey text matches and those rules agree. This was a comparison of that contract only.
Validation: build and lint succeeded on this revision. The new unit tests were inspected and were not executed in this review.
Device testing: not performed in this review. Reported journey passes at 21fd91e and 47030f9 do not cover expiry retention at this revision.
Ready for device testing.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Device QA — passed
Tested 1f55d0317d1074fe57449710cd4c3e7856cf0cb8 on Pixel 6 Android emulator, using a build of this exact revision, staging Bitcoin regtest, and the local bitkit-docker USDT fork fixture with a controlled recipient and signed staging test subscriptions.
All 13 planned cases plus exploration passed:
- Bitcoin first-period payment; USDT-only and USD fixed 1:1 initial payment with one Subscribe authorization.
- No initial payment due; insufficient funds; PIN cancellation followed by an explicit retry; safety-warning cancellation without automatic retry.
- Locked amount and Back behavior; manual non-1:1 conversion; mixed accepted methods; ordinary USDT amount editing.
- Unpaid period changes automatically to Expired, retains one non-payable Payments row after relaunch, and stays separate from the next payable period.
- Monthly Cost agrees with an independent calculation using the app's cached rate.
- Signed conversion quote expires during successful preparation: stale terms are rejected without transfer; an explicit fresh-quote retry pays once.
The quote-expiry trigger was exercised with a transparent timing proxy delaying a successful token-quotes response until after the signed quote deadline. Responses were not changed. The stale attempt transferred nothing; the fresh retry produced exactly one received payment proof, independently verified by Corewallet.
Each platform completed four 2-USDT test payments and one confirmed 5,000-sat Bitcoin payment, with no duplicate payments. The shared USDT recipient received exactly 16 USDT across the paired run. After approved pending-request cleanup, the wallet bell cleared on both platforms. Temporary payment PIN settings were restored to Disabled and the subscription scheduling clock to Off.
No confirmed product defects or blocked cases in this scope. Physical-device camera and biometric behavior were outside this emulator/simulator run. Current head and comparison base still match the saved full code assessment; applicable CI checks pass. The earlier expired-history concern was verified fixed through expiry, relaunch and next-period checks on this revision.
Approved based on the completed code review and this full device run.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
One MEDIUM inline, introduced in 1f55d03: an expired unpaid subscription period now shows as a sent payment on the payer's subscription detail. No funds move.
Everything else in the delta is clean:
- Expired periods can't be paid: the refresh and per-subscription pending filters exclude them, and
ensurePaymentAllowed/updateRequest/checkPaymentDeadlinethrowRequestExpired. retryOriginalOnchainSendfinds expired periods but is still rejected, so nothing is broadcast.- Paid proofs win over expiry, and canceled subscriptions add no unpaid history.
- Expiry dedupes by id, and an expired period counts as handled, so no reminder fires for it.
- The Expired / Waiting labels are correct, and there is no persisted format change.
Perf: not relevant. The new deadline wakes are bounded, since the request is removed before the timer reschedules.
Device gate: waits for the fix.
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — an expired unpaid period appears as a sent subscription payment.
Findings: No new findings. I confirmed the existing MEDIUM unpaid period shown as sent: PaykitPaymentRequestRepo.kt:1419 adds expired unpaid periods to history, and SubscriptionsScreen.kt:508 selects them for PAYMENTS with isOutgoingPayment = true. That gives the row a minus amount and sent arrow without payment evidence. No funds move.
The earlier disappearing-period finding is fixed: expiry retains the period, and refresh/restoration reconstructs it without making it payable.
QA for 1f55d03:
Test 1: Expired-period retention ✅ The repository retains the expired period through refresh/restoration and isolates the next payable period.
The existing repository regression drives the real expiration job, refresh, state restoration and next period; it also checks that payment is rejected with RequestExpired. I ran it against the unmodified head. Expiry, relaunch and next-period device coverage is supplied by the completed device report at this exact head.
Test 2: Unpaid period excluded from sent subscription payments ❌ A repository reproduction selects an unpaid expired period with a minus amount.
A review-only test expires an accepted period through the real timer, refreshes, checks that it cannot be paid, then applies the subscription detail's existing selection and amount-sign helper. Expected no unpaid sent rows; actual result is one - row. I checked the composable's outgoing flag and sent-arrow selection in source; this is a repository/presentation reproduction, not a new device capture.
I retained the 13-case device report at 1f55d03, including expiry/relaunch, single authorization, cancellation/retry, conversion guards and signed quote expiry. The separate quote-replacement case remains covered by the earlier device report at d5ae1af; the delta leaves those send guards unchanged. No device cases were repeated in this pass, and no planned cases remain unrun.
Twin synonymdev/bitkit-ios#1006 fix: authorize initial USDT subscriptions once at 9450bdb matches the expired-history retention rules and the shared journey text. I compared that contract in source.
Delta review covers d5ae1af…1f55d03, all four changed files and their history consumers. The changelog received source review only.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at 19e3a31, since 1f55d03, using the completed baseline review. This pass checked payer subscription details, which now list only belonging history rows with a submitted payment proof, while creator details still use received payments. It also checked the updated repository assertions, the new review-and-subscribe journey sentence, and the same filter on iOS pull request 1006 at 78cfdb7. Inherited coverage is initial USDT eligibility and one-use authorization, quote preparation and payment guards, locked-amount navigation, subscription success, expired-period retention in payment history, payable versus expired status, the remaining author tests and journey actions, and the earlier iOS eligibility comparison at 9450bdb.
No new actionable code findings.
The subscription-detail sent-payment report is addressed at this revision. Expired unpaid periods stay in Payments as Expired, and a passed payment deadline leaves a period with a submitted proof in the subscription detail. The iOS comparison covers this shared listing and journey sentence.
Validation: the new PaykitPaymentRequestRepoSubscriptionTest assertions were inspected. The CI build on this revision completed its unit-test step, and lint and detekt passed. Device testing: not performed in this review. The device pass covers 1f55d03.
Ready for device testing. The targeted check is the updated review-and-subscribe.xml sentence: an expired unpaid period stays in Payments and is absent from the subscription detail, while previously paid periods remain there.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 19e3a31: the MEDIUM is fixed. subscriptionPayments lists only periods with payment evidence on the payer side and keeps receivedPaymentRequests() for the creator. The repo tests assert that an expired unpaid period stays out after the timer, after a refresh and after a reload. Nothing else in the commit changes behaviour.
Device gate (partial): 19e3a31 — not run yet. Pending: subscriptions/review-and-subscribe.xml, including the new expiry and relaunch action, cross-device with the iOS twin and an issuer that sets PeriodStart deadlines. Device results will follow in a separate review.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The expired unpaid period shown as sent is fixed: payer subscription details retain only periods with payment evidence; creator proof rows remain available.
QA for 19e3a31:
Two Android 15 / API 35 emulators with local-fork test USDT. Creator is left, payer is right; recordings are edited and play at 2×.
Test 1: Expired unpaid periods excluded from subscription payments ✅ Expired history and the earlier paid period survive relaunch; rollover adds only a payable new period.
I first made a real 2-USDT payment on the unmodified payer build. Fork balances and the queued-proof log establish that paid control. The deadline checks then used a billing-clock fixture: the repository and status view see controlled billing time, while SDK authentication and the subscription-detail selection remain unchanged.
The unpaid October 21 period remains exactly once in Payments as Expired, with no Pay action. Subscription details show only the paid October 20 period. Both checks hold after process restart. At the October 22 rollover, only the new period offers Pay; October 21 stays Expired, and subscription details still contain just the October 20 payment. UI dumps independently check the row counts, dates and payment actions shown in the recordings.
T1.2x.mp4
Test fixture controls — not pushed
The creator supplies a signed five-minute period-start deadline. The payer fixture advances only billing and expiration time; it does not fabricate SDK records, payment proofs or selection results. The initial payment used the unmodified payer build. The patches used for the subsequent deadline checks are:
diff --git a/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt b/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt
index 1e62b60e0..1dfa669d1 100644
--- a/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt
+++ b/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt
@@ -924,6 +924,7 @@ class PaykitPaymentRequestRepo @Inject constructor(
conversion = rates.takeIf { it.isNotEmpty() }?.let(PaymentConversion::Fixed),
paymentReference = "bitkit-${UUID.randomUUID()}",
proposalExpiresAt = draft.expiresAt.toString(),
+ paymentDeadline = if (name.startsWith("Deadline")) PaymentDeadline.PeriodStart(300uL) else null,
acceptedPaymentEndpointIdentifiers = endpointIdentifiers,
metadataJson = JsonObject(
mapOf(diff --git a/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt b/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt
index 1e62b60e0..4b43ec7c9 100644
--- a/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt
+++ b/app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt
@@ -319,7 +319,7 @@ class PaykitPaymentRequestRepo @Inject constructor(
private val paymentProofRepo: PaykitPaymentProofRepo,
private val usdtPayments: PaykitUsdtPaymentRepo,
private val subscriptionNotificationScheduler: PaykitSubscriptionNotificationScheduler,
- private val clock: Clock,
+ clock: Clock, // test hook
@SubscriptionClock private val subscriptionClock: Clock,
private val lightningRepo: LightningRepo,
private val currencyRepo: CurrencyRepo,
@@ -332,6 +332,8 @@ class PaykitPaymentRequestRepo @Inject constructor(
private const val PAYMENT_REQUEST_DISCOVERY_CONCURRENCY = 8
}
+ private val clock = to.bitkit.utils.ReviewBillingClock.wrap(clock) // test hook
+
private val operationMutex = Mutex()
private val refreshMutex = Mutex()
private val targetDiscoveryMutex = Mutex()
diff --git a/app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt b/app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt
index 38288ffc7..e9844cb2f 100644
--- a/app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt
+++ b/app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt
@@ -504,7 +504,7 @@ internal fun paymentRequestStatus(
internal fun paymentRequestLifecycleStatusRes(request: PaykitPaymentRequest, isPending: Boolean): Int {
val canExpire = request.lifecycleState == PaymentRequestLifecycleState.PROPOSED ||
request.lifecycleState == PaymentRequestLifecycleState.ACTIVE_RECURRING
- if (canExpire && request.isExpired(Clock.System.now())) {
+ if (canExpire && request.isExpired(to.bitkit.utils.ReviewBillingClock.date(Clock.System.now())) /* test hook */) {
return R.string.wallet__payment_request_status_expired
}
diff --git a/app/src/main/java/to/bitkit/utils/SubscriptionClockOffset.kt b/app/src/main/java/to/bitkit/utils/SubscriptionClockOffset.kt
index 21812272b..441d589bb 100644
--- a/app/src/main/java/to/bitkit/utils/SubscriptionClockOffset.kt
+++ b/app/src/main/java/to/bitkit/utils/SubscriptionClockOffset.kt
@@ -44,8 +44,8 @@ object SubscriptionClockOffset {
): Instant {
if (!isAvailable) return date
val days = clampedOffsetDays(offsetDays)
- if (days == 0) return date
- return date + days.days
+ if (days == 0) return ReviewBillingClock.date(date) // test hook
+ return ReviewBillingClock.date(date + days.days) // test hook
}
fun subscriptionNow(clock: Clock = Clock.System): Instant = subscriptionDate(clock.now())
diff --git a/app/src/main/java/to/bitkit/utils/ReviewBillingClock.kt b/app/src/main/java/to/bitkit/utils/ReviewBillingClock.kt
new file mode 100644
index 000000000..b9ae6adb9
--- /dev/null
+++ b/app/src/main/java/to/bitkit/utils/ReviewBillingClock.kt
@@ -0,0 +1,18 @@
+package to.bitkit.utils
+
+import to.bitkit.BuildConfig
+import java.io.File
+import kotlin.time.Clock
+import kotlin.time.Duration.Companion.milliseconds
+import kotlin.time.Instant
+
+// test hook: changes only Paykit billing/expiration time; SDK authentication keeps real time.
+internal object ReviewBillingClock {
+ fun date(date: Instant): Instant = if (BuildConfig.DEBUG) date + offset().milliseconds else date
+ fun wrap(base: Clock): Clock = object : Clock {
+ override fun now(): Instant = date(base.now())
+ }
+ private fun offset(): Long = runCatching {
+ File("/data/user/0/${BuildConfig.APPLICATION_ID}/files/review-billing-offset-ms").readText().trim().toLong()
+ }.getOrDefault(0)
+}Test 2: Creator submitted-proof history ✅ The creator retains one +$2.00 proof row after restart.
The October 20 row remains exactly once after process restart, labeled pending verification. The fork independently confirms 2 USDT received. This checks retained proof history, not a change to receipt verification. The creator uses only the proposal-deadline fixture shown above; its receipt selection and detail screen are unchanged.
T2.2x.mp4
Retained coverage for paths this delta leaves unchanged:
- The 13-case device report at 1f55d03 covers initial authorization, cancellation/retry, insufficient funds, conversion guards, signed quote expiry, locked amounts, expiry retention and monthly cost.
- The quote-replacement device check at d5ae1af covers rejection of replaced terms and explicit recovery.
No planned cases remain unrun. The current-head repository expiry/restoration/rollover and presentation regressions also pass on unmodified sources.
Twin synonymdev/bitkit-ios#1006 fix: authorize initial USDT subscriptions once at 78cfdb7 has the equivalent paid-only filter and byte-identical journey. I checked that contract in source; iOS devices were not repeated in this delta pass.
Delta review covers 1f55d03…19e3a31, all four changed files and their history consumers. The changelog received source review only.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approving the current revision based on the completed code review, full paired 13-case device baseline, and current-head fix/regression retests. The paid-only subscription-history fix is verified, prior findings are addressed, and applicable build/unit/integration/lint checks pass. E2E is non-blocking for this approval.
The observed clock-skew subscription sequence is a separate, unconfirmed follow-up: one payment was verified on both wallets, with no duplicate charge. Controlled skew and base-branch comparison will be investigated separately. This approval does not claim that follow-up is fixed.
This branch has not been deployed
5 participants
Fixes #1540
Description
Continues eligible initial USDT payments directly from Subscribe and reuses subscription progress and success views. Eligibility is limited to USDT-only requests with unchanged amounts: USDT without conversion or USD with an explicit fixed 1:1 USDT rate.
Retains quote preparation, fee checks, payment authentication, warnings, deadline validation and duplicate-payment protection. Other conversion terms still require amount/payment review; failed or cancelled preparation never automatically retries. Manual preparation and retries keep the normal Amount → Continue → payment review flow without automatic-payment progress. Tapping a locked amount keeps review open; Back still closes it, and ordinary send amounts remain editable.
Shows Waiting for payment for payable unpaid periods. Expired unpaid periods of active subscriptions stay in Payments and retain their details across refresh and restart, without a Pay action. Paid history and cancellation behavior are preserved. Subscription details list only paid periods, so an expired unpaid request never appears as a sent payment.
Out of Scope
Design
Reuses Review & Subscribe and Subscribed, with existing USDT styling.
Preview
Existing subscription layouts; this removes the extra payment swipe. No funded device recording was made for this change.
QA Notes
Journeys
review-and-subscribe.xml— initial USDT payment, authentication cancellation and locked-amount navigation, insufficient funds, an unpaid-period retry, expiry/history restoration, and exclusion of unpaid periods from subscription payments using local test funds.Manual Tests
Automated Checks
AppViewModelSendFlowTest.kt— initial-only eligibility, conversion/deadline rejection, direct routing and one-use authorization; mixed accepted methods still require selection when only USDT resolves.UsdtViewModelTest.ktcases through the actual review preparation helper: quote-before-authorization, authentication cancellation/manual retry, insufficient balance/expired quote recovery, cancelled preparation and duplicate prevention. Delayed manual preparation stays unauthorised and does not show automatic-payment progress.PaykitPaymentRequestRepoSubscriptionTest.kt— real expiry timer, refresh/restoration, next-period isolation, and the production subscription-detail selection: unpaid history is excluded, paid payer rows and creator receipts are retained. All 48 repository subscription tests pass.PaymentRequestPresentationTest.kt— payable and expired subscription periods, unavailable periods and submitted proofs.just compile,just test— 4,382 tests passed; existing USDT tests cover expiry, insufficient funds, authentication cancellation, retries and duplicate prevention.UsdtPaymentContentTestinstrumented tests before the history-only follow-up, including locked-amount taps, explicit Back and editable amount navigation.just lint— Detekt reports no findings; Android Lint reports no new findings (280 existing entries remain in this branch’s committed baseline). Full diagnostics were inspected.