Repository navigation
fix: authorize initial USDT subscriptions once - #1006
ben-kaufman wants to merge 15 commits into
Conversation
 [Critical impact] This reaches a narrow subscription flow that can move USDT. Fix the mixed-method eligibility check before merging so only USDT-only requests skip payment review. FindingsSummaryThis PR continues eligible initial USDT subscription payments after Subscribe and reuses subscription progress and success views.
Intentional behavior stated by ben-kaufman: eligible initial payments no longer require a second payment swipe. Review checks were static. Tests and iOS visual checks were not run in this Linux environment. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Subscribe] --> B[Resolve payable endpoints]
B --> C{Eligible initial USDT payment?}
C -->|Yes| D[Prepare USDT quote]
C -->|No| E[Amount and payment review]
D --> F[Warnings and configured authentication]
F --> G[Recheck request and send once]
G --> H[Track transfer]
H --> I[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}
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
Reviewed at e369308 from the code. Nothing was compiled or run as part of this review. Run Tests, Run Integration Tests and build-local were still pending when I read the checks.
No blocking findings.
What I checked:
initialUsdtSubscriptionPaymentreturns terms only for an initial, unexpired, period-bound request with USDT-only endpoints, where the asset is USDT with no conversion or USD with a fixed rate equal to 1. When bothusdt-arbitrumandusdtrates are present,usdt-arbitrumis the one checked, and the test covers a conflicting pair. A per-period conversion and any BTC request fall back to the existing review routes.- The auto-continue in
UsdtSendViewruns once (hasStartedInitialPayment), only when the prepared amount equals the eligible terms. It goes throughconfirmPayment(), so the balance, size and fee warnings, PIN or biometric authentication andensurePaymentAllowedinsendPaymentall still apply. A cancelled authentication leaves the quote unsent, with no retry loop. - A failed quote sets
errorand clearsbusy, so insufficient funds stops before any send.PaykitUsdtPaymentService.sendstill clearspaymentStartedwhen no transfer was stored. - The submitted-status polling
.taskmoved fromcontentto the outerGroup. That is needed: theSubscriptionSuccessViewbranch replacescontent, which would otherwise cancel the polling and the finalreconcile. onBackcloses the sheet when the USDT route is the embedded root (emptynavigationPath), which is the Back path the updated journey uses.- Both entry points,
SubscriptionSheeton accept andretryEmbeddedInitialSubscriptionPayment, setpaykitPaymentTermsandpaykitReviewedAmountbefore the.usdtroute.performPaymentValidationAfterSyncreturns.readyfor USDT, so the retry route is not held back by the Bitcoin balance checks.
The Android port appears to be #1568 on bitkit-android. The journey change here (review-and-subscribe.xml) should land on that side as well to keep the two specs diffable.
|
The failed E2E shard timed out in the hardware-wallet helper while waiting for |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
hasStartedInitialPaymentis per-view@State, and nothing rebuilds the view without a user action. The sync overlay is off for USDT,offlineSheetOverlayis an overlay, androotRouteGenerationchanges only on retry. It is equivalent to Android's view-model flag.- iOS doesn't compare the recomputed terms before auto-confirming, but
PaykitUsdtPaymentService.prepare/sendcheckpaykitReviewedAmountandpaymentTerms, andensurePaymentAllowedstill runs. - Confirm runs only
if automaticPayment, !Task.isCancelled, quote != nil. A cancelled biometric leaves the quote unsent, and a failed send clears the quote without retrying. - The
quotePaymentandauthenticatePaymentseams have no production caller.
Device gate (partial): not run yet. Pending: subscriptions/review-and-subscribe.xml on the simulator against the local USDT fixture, plus timing from Subscribe to Subscribed. The device result follows in a second review.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at ba31357. The code scan from my earlier review stands; this review only adds the device result.
Device gate: ba31357 — journeys: subscriptions/review-and-subscribe.xml 23/23 pass on the iPhone 17 simulator (iOS 26.5) as payer, with synonymdev/bitkit-android#1568 on Android as the creator; perf: USDT subscribe, swipe → Subscribed 68.9 s with one swipe vs master's swipe → USDT Sent 70.0 s with Amount, Continue, a second swipe and PIN (automated; single samples, network-bound on the payment-list wait); l10n: no UI text changed.
Covered:
- BTC first period: Subscribed 69 s after the swipe, with no second swipe and the layout kept on Review & Subscribe.
- USDT first period: paid after the fee warning, with no amount entry.
- PIN on: swipe, then Back on the PIN prompt; nothing was paid and the balance stayed 63.222109. Paying the period from the bell paid once (60.546773).
- $100 over balance: "Not enough USDT to cover the amount and fees." and the period stays offered.
- Overview: MONTHLY COST is a single total aligned with ACTIVE/CREATED.
Observation, not a finding (not compared with master): after Close on the BTC subscription, the ACTIVE row read "Renews 19 October" (today) for over a minute while the detail said 19 November. A later visit showed 19 November.
PaykitPaymentRequestServiceTests and the code trace.
CI at ba31357: green.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ⛔️ Request changes: on the manual initial USDT route, the payer sees the subscription being paid before authorizing anything.
Findings:
- MEDIUM
Bitkit/Views/Wallets/UsdtSendView.swift:147: the paying overlay covers the manual USDT route before authorization.InitialSubscriptionPaymentProgressshows Review & Subscribe with the swipe already completed wheneverisInitialSubscriptionPayment && busy.- A manual initial USDT payment reaches this view from Amount. Examples are a request accepting Bitcoin and USDT, and one with a per-period or non-1:1 conversion.
busyis true there while the quote is prepared. - In Test 4, Continue showed Review & Subscribe at $2.00 with the swipe spinning for about 5 s. Only then did the real $2.02 review and its Swipe To Pay appear.
SendConfirmationView.swift:279shows the same overlay only whenshouldAutomaticallyPay. Gating this one on the automatic payment keeps the manual review as it was before this PR.
- LOW
Bitkit/Views/Wallets/UsdtSendView.swift:123: the USDT Subscribed screen keeps the navigation bar on the manual route.- Only
contentcarries.navigationBarHidden(true)(line 194), and the new branch replaces it. - After the push from
SendAmountView.swift:349, Subscribed shows a system Back button. In Test 4, Back slid to the Amount screen of the period just paid while the sheet closed. SendSuccesswraps the same view in.navigationBarHidden(true)and.allowSwipeBack(false). The automatic route shows no bar.
- Only
QA for ba31357:
Two iPhone 17 Pro simulators (iOS 26.5), one creating and one paying subscriptions, on a local Arbitrum fork with test USDT.
Test 1: expired fee authorization, then explicit retry ✅ Automatic preparation failed without sending or retrying, and the explicit retry paid once.
- The fee authorization was set to expire before the payer swiped to subscribe to a 2 USD USDT-only proposal.
- The payment stopped on Amount with "The fee quote expired. Review the payment again." The screen stayed there for 30 s with Available $18.75 unchanged.
- With the authorization working again, Continue prepared a new quote and waited at Swipe To Pay.
- One swipe paid once (−2.68 USDT in activity) and Subscribed appeared.
t1-expired.2x.mp4
Test 2: automatic payment pending before inclusion ✅ The pending payment showed its submitted state and moved to Subscribed once included.
- Inclusion was held back, and the payer swiped to subscribe to a 2 USD USDT-only proposal.
- After the fee warning, the screen read "Payment submitted. Your payment is being processed", with Details and Close.
- Once the operation was included, it switched to Subscribed within about 20 s.
- Activity shows one payment (−2.68 USDT).
t2-pending.2x.mp4
Test 3: plain USDT send ✅ A 1 USDT send to an address completed, showed its success screen and closed.
- USDT Sent showed Details and Close.
- Close returned to the USDT wallet with one Sent row (−1.83 including the fee) and a $23.17 balance.
t3-plain-send.2x.mp4
Test 4: manual initial USDT payment ❌ The paying overlay appeared before review, and Subscribed showed a Back button.
- The creator proposed 2 USD, USDT only, at a fixed 1.01 USDT rate. Bitkit's own creator does not send that rate; it stands in for the mixed-method and non-1:1 requests that take this route.
- Swipe To Subscribe opened Amount ($2.02, USDT).
- Continue showed Review & Subscribe at $2.00 with the swipe spinning for about 5 s, then the Payment Request review of $2.02.
- After Swipe To Pay and the fee warning, Subscribed showed a system Back button.
- Back slid to the Amount screen while the sheet closed.
- One payment went out (−2.75 USDT, checked in activity after the recording).
t4-manual.2x.mp4
Not driven:
- Paykit quote terms expiring or being replaced during preparation (this PR's Manual Test). Test 1 expires the USDT fee authorization, not the request's quote terms, so this rests on
PaykitPaymentRequestServiceTests. - The USDT branch of
retryEmbeddedInitialSubscriptionPaymentinSendSheet. It needs a failed submission inside the subscription sheet, which no case reached.
Relied on:
- @jvsena42's device run at ba31357 of
review-and-subscribe.xml(23/23), which covers the first USDT period, PIN cancel with a retry from the bell, and the over-balance error. I did not re-drive those.
Pair PR synonymdev/bitkit-android#1568 matches in eligibility and in its review-and-subscribe.xml edit. Its AutoPayOverlay shows on state.busy for every initial subscription payment, the same condition as the first finding.
Full review of 7b3f093...ba31357, 7 files.
|
Applied the matching locked-amount fix from the Android review in 882ffbd. Tapping the fixed amount now keeps the payment review open; explicit Back still closes it, and normal editable sends retain amount navigation. Both platforms’ subscription journey includes this check. Also fixed the new CI failure in 72814ca: screenshot OCR read “Subscribed” as “SubscribedR”. The regression now checks the real pushed navigation stack, hidden navigation bar and disabled swipe-back state, rather than matching OCR text. All 2,685 local unit tests pass with the same six exclusions as CI, along with SwiftFormat and translation validation. Compiler warnings are on unchanged master lines. A funded journey was not rerun; the isolated iOS tap probe could not expose automation targets, so I am not claiming device-tap verification. Android’s eight component tests include the actual amount and Back taps. All five PR commits are signed and verified locally and on GitHub. Re-requesting review. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since my approval at ba31357 (2e7865d, 882ffbd, 72814ca): no findings.
automaticPaymentis now@State. Confirm is still gated byautomaticPayment, !Task.isCancelled, quote != niland the per-viewhasStartedInitialPayment.- The overlay shows only
if automaticPayment, busy, so it never covers the warning alert or the PIN push. - The
.taskre-run after the PIN screen pops returns before writing the flag. - In embedded mode the amount header can't be tapped; non-embedded behaviour is unchanged.
SubscriptionSuccessViewhides the nav bar and disables swipe-back, andAllowSwipeBackModifierrestores swipe-back on disappear.
Non-blocking: testInitialUsdtProgressRequiresAutomaticAuthorization still asserts through Vision OCR (labels.contains("Review & Subscribe")). That's the mechanism 72814ca removed from the success test after the "SubscribedR" flake, and its "absent" assertion also passes on a misread.
Device gate (partial): ba31357 ran the full review-and-subscribe.xml. Not run yet at 72814ca: the two changed actions, on the simulator against synonymdev/bitkit-android#1568. The device result will follow.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. Both earlier findings are fixed.
QA for 72814ca:
iPhone 17 Pro simulator, iOS 26.5, with local-fork test USDT. The payer ran the unmodified PR build.
Test 1: automatic initial payment and success navigation ✅ One Subscribe swipe paid the first period, and Subscribed could not return to the paid amount.
After the fee warning, Subscribed appeared without another amount entry or payment swipe. It had no Back button, and an edge swipe stayed there. Close dismissed it; the subscription detail showed one payment and renewal on 19 November.
t1-auto.2x.mp4
Test 2: PIN cancellation, locked amount and explicit retry ✅ Cancelled authentication sent nothing; amount taps kept review open, Back closed it, and the explicit retry paid once.
The local chain balance stayed at 19.475473 USDT across PIN cancellation and the amount tap. Reopening the unpaid period showed normal preparation and waited at Swipe To Pay without automatic-payment progress. The explicit swipe and PIN paid once; history showed one payment for that period.
t2-pin.2x.mp4
Test 3: manual initial payment ✅ Continue used normal preparation, and Subscribed had no Back button or working swipe-back.
A creator-side test hook issued a 2 USD subscription at a fixed 1.01 USDT rate. On the payer, Continue from 2.02 USDT showed the normal spinner, then waited at Payment Request and Swipe To Pay. After the swipe, warning and PIN, Subscribed stayed open on an edge swipe. The subscription detail showed one payment.
t3-manual.2x.mp4
Test 4: quote terms expire during preparation, then explicit retry ✅ Expired terms cancelled payment without sending; a fresh quote and explicit retry paid once.
The signed conversion quote expired during successful USDT preparation, checked against its signed expiry and the delayed RPC timestamps. After Swipe To Pay and PIN, Bitkit showed “The payment details did not match the request. Payment cancelled.” The local chain balance stayed at 16.800137 USDT. A fresh signed quote and reopening the unpaid period required an explicit payment swipe and PIN, then produced one payment in the subscription history.
t4-expiry.2x.mp4
Test 5: ordinary send amount editing ✅ Tapping the quoted amount returned to editable Amount.
A 1 USDT address send waited at Swipe To Pay. Tapping its amount opened Amount, where I changed it to 2 USDT, then closed without paying.
t5-editable.2x.mp4
Retained earlier coverage:
- @jvsena42's device report at ba31357: the unchanged Bitcoin first period, insufficient-funds handling and monthly overview total.
- The earlier review at ba31357: expired fee authorization, pending inclusion and ordinary send completion; the quote, submission and polling code is unchanged.
Pair PR synonymdev/bitkit-android#1568 matches the automatic-progress gating, locked amount behavior and shared journey updates.
Delta review of ba31357 to 72814ca, 3 files. The unchanged embedded SendSheet failure-retry branch was inspected but not separately driven.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
ovitrif
left a comment
ovitrif
left a comment
There was a problem hiding this comment.
Approved per the ovi-reviewer review.
|
The Android device review found that an unpaid subscription period was shown as “Unavailable”. The same display path existed here, so a3d15fb now shows “Waiting for payment” for payable periods and “Expired” after their deadline, preserving unavailable and proof-submitted states. All 275 payment-request service tests pass, including the new status regression; final SwiftFormat and translation checks pass with no introduced warnings. The shared retry journey now correctly describes Amount → Continue → payment review after reopening a cancelled initial payment. A manual retry does not reuse the completed Subscribe authorization. The initial Subscribe flow is unchanged. Re-requested review. |
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — expired unpaid periods still disappear instead of showing Expired.
Findings:
- MEDIUM
Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:23: retain expired unpaid periods so the new status can render.discardExpiredRequestsremoves them frompendingRequests, while recurring history keeps only submitted/completed payments. The detail lookup then returns no request and shows only Unavailable; the period also disappears from Payments. Test 2 reproduces this at the signed deadline. Preserve the expired period as non-actionable history. The new helper-only test bypasses this pruning.
QA for a3d15fb:
iPhone 17 Pro simulators, iOS 26.5, using local-fork test USDT. The payer ran the unmodified head. A creator-only test hook supplied the short signed deadline in Test 2. Videos play at 2× speed.
Test 1: payable unpaid period and explicit retry ✅ Waiting for payment appeared, and the explicit retry paid once.
Cancelling PIN sent nothing, checked against the unchanged 25 USDT fork balance. Tapping the locked amount kept review open; Back closed it. From Payments, the detail showed Waiting for payment. Pay opened fixed Amount, and Continue showed the ordinary preparation spinner before waiting at Swipe To Pay. The explicit swipe and PIN completed one payment. History then showed Proof submitted without Pay.
t1-unpaid-retry.2x.mp4
Test 2: unpaid period after its deadline ❌ The detail became Unavailable and the period disappeared from Payments.
The signed subscription used a 180-second period-start payment deadline. Insufficient funds prevented its initial 100 USDT payment. Before the deadline, the detail showed Waiting for payment and Pay. After expiry, it showed only Unavailable; returning to Payments showed no row for the unpaid period. No transfer occurred, checked against the unchanged 22.170809 USDT fork balance.
t2-expired-period.2x.mp4
Test 3: embedded first-payment failure and retry ✅ Retry Payment recovered through fresh preparation and authentication, then paid once.
Creator is left, payer is right. Disabling the creator's contact-payment offers produced First Payment Failed. Restoring the offers left that screen waiting for Retry Payment. The fork balance stayed unchanged through the failure and until PIN authorization. Retry reloaded payment preparation, showed the fee warning and PIN, and reached Subscribed. The payer then showed one payment for that period, with Proof submitted and no Pay action.
t3-embedded-retry.2x.mp4
Retained coverage for unchanged code:
- The earlier review at 72814ca: automatic initial payment and success navigation, manual conversion, quote expiry with explicit recovery, and ordinary send editing.
- @jvsena42's device report at ba31357: Bitcoin first period, insufficient funds and monthly overview total.
- The earlier review at ba31357: expired fee authorization, pending inclusion and ordinary send completion.
Twin synonymdev/bitkit-android#1568 fix: authorize initial USDT subscriptions once matches the status conditions and updated retry journey in source; Android was not driven in this delta run.
Delta review of 72814ca…a3d15fb, four files. The status changes and clarified retry route were driven; the changelog wording has no separate device case. No cases remain unrun.
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 review of 78cfdb7, descendant of 9450bdb, using the completed baseline QA review. This pass inspected subscription-detail payment selection, expired unpaid history through refresh, restart, rollover, and cancellation, the updated expiry step in journeys/subscriptions/review-and-subscribe.xml, and the same selection on bitkit-android#1568 at 19e3a31. Inherited coverage is initial USDT eligibility, Subscribe routing, manual payment review, Subscribed navigation, and the unchanged shared payment contracts from that baseline.
No new actionable code findings.
Payer subscription details include a history row only when it belongs to the subscription and its lifecycle is proof submitted. Creator details still list received payment proofs. An expired unpaid period remains in Payments with status Expired, stays unpaid across refresh and restart, and is omitted from the subscription Payments section. That is the behavior reported in the subscription-detail thread. The journey file matches the Android pin.
SwiftFormat and build-local passed on this revision. Run Tests and Run Integration Tests were still pending. This review inspected the subscriptionPayments assertions in PaykitPaymentRequestServiceTests.swift and did not execute them. Device testing: not performed in this review.
Recommended before device testing: wait for Run Tests and Run Integration Tests on this revision.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 78cfdb7: the MEDIUM is fixed. subscriptionPayments lists only periods with lifecycleState == .proofSubmitted on the payer side and keeps receivedPaymentRequests() for the creator. PaykitPaymentRequestServiceTests asserts 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): 78cfdb7 — not run yet. Pending: subscriptions/review-and-subscribe.xml, including the new expiry and relaunch action, cross-device with the Android 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 a debit is fixed: payer subscription details now require a submitted payment proof, while creator receipts retain their existing selection.
QA for 78cfdb7:
I reran the previously failing subscription-detail case on an iPhone 17 Pro simulator, iOS 26.5, and retained passing reports for the unchanged payment flows.
Test 1: subscription journey and expired history ✅ Retained passing coverage at 9450bdb; the payment and request-history paths are unchanged.
@piotr-iohk’s device report covers initial Bitcoin and USDT payments, no-payment-due, insufficient funds, PIN and warning cancellation, explicit retry, locked amount and Back, manual conversion, mixed methods, ordinary amount editing and Monthly Cost. It also confirms automatic expiry, one non-payable history row after relaunch and a separate next payable period. This delta changes subscription-detail selection only.
Test 2: signed quote expiry and explicit recovery ✅ Retained passing coverage at 9450bdb; stale terms sent nothing and a fresh explicit retry paid once.
The same device report delayed a successful, unchanged token-quotes response beyond the signed deadline and independently checked the single received payment proof. Quote validation, preparation and submission are unchanged by this delta.
Test 3: embedded first-payment failure and retry ✅ Retained passing coverage at a3d15fb; recovery still requires explicit Retry Payment and fresh authentication.
The earlier recording and result shows First Payment Failed waiting after creator offers were restored, then a fresh preparation, warning and PIN producing one payment. This delta leaves that route unchanged.
Test 4: expired unpaid period in subscription details ✅ The production detail omits the unpaid debit; manager regressions preserve paid payer rows and creator receipts.
The earlier failing hosted probe now passes. An isolated SDK fixture creates an active 2 USDT subscription, advances its injected clock past the period-start deadline and opens the production SubscriptionDetailView. The expired unpaid period remains in request history, but the detail has no Payments section or sent amount. App sources are unchanged, and the fixture performs no transfer. The recording shows this detail at 2× speed.
I also executed the changed manager regressions: unpaid periods stay excluded after expiry, refresh, manager restoration and rollover; the next period remains separately payable; paid payer rows and creator receipts remain selected. These lifecycle and receipt checks come from assertions, rather than the recording.
expired-detail-2x.2x.mp4
No required cases remain unrun. This pass used an isolated hosted fixture; funded payment coverage is retained from the linked reports.
Twin synonymdev/bitkit-android#1568 fix: authorize initial USDT subscriptions once at 19e3a31 matches the proof-only payer selection and unchanged creator receipts; the journey files are identical. Android was not driven in this delta run.
Delta review of 9450bdb…78cfdb7, including the previously open debit finding. Changelog wording has no separate device case.
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.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full pull request at c1a21f38 after the comparison base changed from cf2b97e to 43c06a43. The merge base is that same base commit. This pass covers the current diff, including initial USDT authorization, one-shot preparation and authentication, locked review navigation, expired unpaid history, and paid-only subscription details. No earlier coverage was inherited.
No new actionable code findings.
Eligible USDT-only initial payments with an unchanged amount go straight into quote preparation and still pass warning, authentication, deadline, and duplicate-payment checks before send. A failed quote or cancelled authentication does not submit a payment or authorize again on its own. Paying that period later from Payments stays on Amount, Continue, and an explicit swipe. Expired unpaid periods stay in payment history without a Pay action, and subscription details list only submitted payments. The six existing review threads are resolved, and those fixes are still present at this head.
The same contracts were compared with bitkit-android#1568 at 19e3a311. The shared journey text matches, as do eligibility, one-shot authorization, expired-history retention, and paid-only subscription details. That comparison is not a review of the Android pull request.
Recommended before device testing: unit tests and integration tests are still pending on this revision.
Device testing: not performed in this review. Device receipts linked from the pull request are for 78cfdb75, not this merge. J1 and manual case 1 were not executed here.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The merge preserves embedded subscription Back handling, one-use authorization, terminal success navigation and the previously resolved fixes.
QA for c1a21f3:
I built this head and checked the merged send navigation on an iPhone 17 Pro simulator, iOS 26.5. The production app sources were unchanged. Navigation probes hosted the real send view with controlled quote/authentication responses and counted parent callbacks; they performed no transfers. Recordings play at 2×.
Test 1: J1 — cancelled automatic subscription review ✅ The amount stayed locked, and Back invoked the parent action without sending.
After the controlled authentication cancellation, tapping the amount kept Payment Request review open. Back invoked the embedded parent action. Probe assertions confirmed one quote, one cancelled authorization, zero sends and exactly one parent callback; the recording shows navigation, not a real PIN prompt.
automatic-2x.2x.mp4
Hosted automatic subscription review.
Test 2: J1 — manual subscription review ✅ Mixed-method preparation waited for explicit payment approval; amount taps stayed locked and Back invoked the parent action.
The production view received a mixed-method subscription context. The probe asserted one quote, no automatic authentication and no send. Tapping the amount kept review open; Back invoked the parent action once.
manual-2x.2x.mp4
Hosted manual subscription review.
Test 3: ordinary hosted address navigation ✅ Back returned to editable Amount first, then invoked the parent action.
From ordinary address review, the first Back opened Amount. I changed 1 to 12 USDT with the keypad. The second Back invoked the parent action. Probe assertions confirmed that only the second Back reached that callback and no authentication or send occurred.
hosted-2x.2x.mp4
Hosted ordinary address review and amount editing.
Test 4: hosted authorization and success regressions ✅ Existing assertions passed for one-use authorization, failure recovery, cancellation, progress and terminal navigation.
I executed the existing hosted tests for successful/cancelled authentication, insufficient balance and expired-quote recovery without automatic reauthorization, closing during preparation, automatic-only progress, and Subscribed navigation. The success test restores an isolated confirmed transfer and asserts the actual navigation stack, hidden navigation bar and disabled swipe-back. These guarantees come from the assertions; the recording shows the hosted screens.
regressions-2x.2x.mp4
Hosted regression sequence, including fixture errors and Subscribed.
Check 1: J1 — retained funded journey coverage ✅ The proven payment, expiry and overview paths are unchanged.
I retained @piotr-iohk’s passing device report at 9450bdb for initial Bitcoin/USDT payments, no-payment-due, insufficient funds, PIN/warning cancellation and explicit retry, mixed methods, manual conversion, expiry/relaunch/rollover and Monthly Cost. The overlapping send-navigation changes were exercised above. Core rc7→rc9 changes bridge/deposit handling; direct Arbitrum subscription submission is unchanged.
Check 2: manual case 1 — signed quote expiry ✅ The previously proven stale-quote rejection and explicit recovery paths are unchanged.
The same device report delayed a successful signed-quote response past its deadline, verified no stale transfer, then verified exactly one received proof after a fresh explicit retry. Quote validation, preparation and direct submission are unchanged.
Check 3: embedded first-payment failure and retry ✅ The previously proven explicit Retry Payment route is unchanged.
The passing recording at a3d15fb shows First Payment Failed waiting after offers were restored, followed by fresh preparation, warning and PIN, and one payment. The merge preserves that route.
Check 4: expired unpaid subscription details ✅ The previously verified paid-only selection and history lifecycle are unchanged.
The passing hosted detail check at 78cfdb7 confirms that an expired unpaid period produces no sent debit, while manager assertions preserve paid payer rows and creator receipts through refresh, restoration and rollover. Those sources and regressions are unchanged.
No required case remains unrun. Funded end-to-end coverage is retained from the linked reports; this pass exercised isolated hosted views.
Twin synonymdev/bitkit-android#1568 fix: authorize initial USDT subscriptions once at 19e3a31 matches the subscription contracts and has the identical journey file. Android was not re-driven here.
Delta review of 78cfdb7…c1a21f3, including imported master changes and merge resolutions. Imported scanner routing, receive UI, server configuration, localization, CI and release changes were code-reviewed without separate device cases.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The six resolved review findings remain fixed.
QA for ac7e2fd:
I built this head and tested on an iPhone 17 Pro simulator, iOS 26.5. The app sources were unchanged. The new hosted probes use the production warnings and PIN screen with controlled quotes and a counted send callback; they perform no transfers. Recordings play at 2×.
Test 1: J1 — real PIN approval after successive warnings ✅ Both fee warnings and correct PIN entry completed one uncancelled send callback.
A deliberately high fixture fee triggered the over-$10 and over-50% warnings in sequence. After accepting both, I entered the test PIN on the actual keypad. Assertions confirmed one quote, one send and an uncancelled task after returning from PIN navigation.
approve.2x.mp4
Successive warnings and real PIN entry; submission is counted by the fixture.
Test 2: J1 — PIN cancellation and explicit retry ✅ Back from PIN sent nothing; an explicit payment swipe and correct PIN reached the send callback once.
Cancelling the actual PIN screen returned to payment review. Retrying required a new Swipe To Pay and PIN entry. Assertions confirmed zero sends before approval, one prepared quote and exactly one send afterward.
pin-cancel.2x.mp4
PIN cancellation, explicit swipe and PIN approval.
Test 3: J1 — warning cancellation and explicit retry ✅ Cancelling the warning left review idle; explicit swipe, warning approval and PIN reached the send callback once.
After Cancel, the fixture asserted zero sends and no second quote. I then swiped explicitly, accepted the warning and entered the test PIN. The final count remained one send.
warning-cancel.2x.mp4
Warning cancellation and explicit recovery through the real controls.
Test 4: hosted authorization and merged expiry regressions ✅ Exact-head assertions passed for navigation, one-use authorization, cancellation, recovery and expiry/history retention.
The existing hosted tests verify successful and cancelled authorization across navigation, preparation failure/recovery, closing during preparation, automatic-only progress and terminal success navigation. Manager assertions verify payment-deadline revalidation, one-time expiry notification deduplication, and unpaid subscription history through expiry, refresh, restoration and rollover, including exclusion from paid subscription details. The recording shows hosted screens; these guarantees come from assertions.
regression.2x.mp4
Existing hosted regression sequence and manager checks.
Check 1: J1 — retained funded journey coverage ✅ The earlier proven settlement, eligibility and overview paths are unchanged.
I retained @piotr-iohk’s passing device report at 9450bdb for Bitcoin/USDT first-period settlement, no payment due, insufficient funds, mixed methods, manual conversion, expiry/relaunch/rollover and Monthly Cost. The changed warning/PIN continuation is exercised above. The paid-only detail result at 78cfdb7 remains applicable; its selection is unchanged and the lifecycle assertions passed again.
Check 2: manual case 1 — signed quote expiry ✅ The proven stale-quote rejection and explicit fresh-quote retry remain applicable.
The same funded device report delayed a successful response beyond the signed deadline, verified no stale transfer, then independently verified one received proof after explicit retry. Quote preparation, request validation and direct submission are unchanged. Deadline revalidation after authorization also passed in this run’s assertions.
Check 3: embedded first-payment failure and retry ✅ The earlier proven Retry Payment route remains unchanged.
The passing device recording at a3d15fb shows First Payment Failed waiting after creator offers recovered, followed by explicit Retry Payment, fresh preparation and authentication, and one payment. The embedded retry route has no changes in this delta.
No required case remains unrun. Funded settlement coverage is retained from the linked reports; physical-device biometrics were not exercised. The reported LNURL E2E failure remains open; I did not rerun that suite.
Twin synonymdev/bitkit-android#1568 fix: authorize initial USDT subscriptions once at 844d295 matches eligibility, unpaid-history retention and paid-only detail selection; the subscription journey is identical. Android was not driven here.
Delta review of c1a21f3…ac7e2fd, all 20 changed files and both merge resolutions. Imported unlock, requester-label and amount/review layout behavior had no separate device cases; one-time expiry notification behavior was checked through manager assertions.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
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 pull request at ac7e2fdb after the comparison base changed from 43c06a43 to e7a21e8e. The merge base is that same base. No earlier coverage was inherited.
No new actionable code findings.
Eligible initial USDT subscriptions continue from Subscribe only for an unexpired USDT-only request whose amount is unchanged: USDT with no conversion, or USD with an explicit fixed 1:1 USDT rate. Quote preparation, warnings, PIN or biometrics, deadline checks, and the one-use guard still sit on that path. Cancelling authorization or closing during preparation does not send, and a failed quote does not authorize again by itself. Manual mixed-method preparation clears the automatic-progress state before review. Expired unpaid periods of an active subscription move into payment history on refresh and when the deadline timer fires, stay unpayable, and are omitted from payer subscription details; submitted proofs still take precedence. The merged one-time expiry handling still runs through the same expiry pass.
Compared those contracts with bitkit-android#1568 at 844d2955. That was not a full review of the Android pull request.
Device testing: not performed in this review. Unit tests, integration tests, and lint succeeded on this revision; they were not rerun here. E2E failed on this revision. The changed send routes are USDT back handling and initial-subscription routing. This review did not re-diagnose that E2E failure.
Ready for device testing. J1 and manual case 1 were not executed on this revision in this review.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan of 78cfdb7..ac7e2fd: no findings.
- The PR's own change since my review is test-only (
cdaf001). The behavioural change came in mergea99b428: the automatic route'sawait confirmPayment()becameTask { @MainActor in try? await confirmPayment() }(UsdtSendView.swift:203-206), because master's #1064 moved PIN and warnings into continuations and the PIN push cancels the view task. - Traced clean: the unstructured semantics match SwipeButton's swipe action, the one-shot guards still hold,
try?swallows onlyCancellationError, every warning, PIN and biometric cancel path sends nothing, and the sheet can't be dismissed interactively while PIN is up. - From the expiry merge:
recordSilentExpirationsexcludesbillingPeriodrequests, so the PR's deadline timer never toasts. - My earlier thread (expired period shown as a sent payment) stays fixed:
SubscriptionsView.swift:233-237filters payer history on.proofSubmitted. - The Android twin synonymdev/bitkit-android#1568 confirms through
viewModelScope, so the detached-task change isn't needed there. It has conflicts and wasn't reviewed this pass.
Device gate (partial): ac7e2fd — not run yet; pending review-and-subscribe.xml on the automatic route with PIN on and approved (one payment, then Subscribed). That is the case a99b428 fixes, and it hasn't been driven funded at any head. The cross-device run waits for the Android twin's rebase. Device results will follow in a separate review.
|
The Android twin’s master conflict is now resolved in #1568 at This removes the source-conflict blocker noted in review; it does not complete the funded cross-device subscription/PIN gate. The iOS head remains |
5 participants
Fixes #981
Twin: synonymdev/bitkit-android#1568
Description
Continues eligible initial USDT payments directly from Subscribe and reuses subscription progress and success views. The completed screen cannot navigate back to the paid amount. 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.
Keeps initial-payment authorization active across PIN navigation and continues only after approval; cancellation and the one-use guard still prevent unintended or duplicate sends.
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. Payment deadlines update the screen automatically; 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
Setup
Use the pinned Core
0.8.0-rc9, Paykit0.1.0and LDK Node0.7.0-rc.76fromPackage.resolved. Build a simulator E2E app using the journey setup and prepare two linked creator/payer identities. Runcreate-and-propose.xmlbefore J1. Bitcoin steps require local regtest savings above 5,000 sats plus fees and no usable Lightning balance.USDT payment steps additionally need an explicitly configured local gateway/RPC/bundler and owned local test funds; Bitcoin regtest does not fund USDT. Follow the USDT fixture requirements. The standard journey environment does not provide billing-clock/quote-expiry controls; those prerequisites must be supplied for manual case 1. Do not substitute mainnet funds or assume standard staging provides this setup.
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
PaykitPaymentRequestServiceTests.swift— initial-only eligibility, explicit fixed rates, rail-specific overrides, expired deadlines and mixed accepted/resolved-method rejection. Hosted send-view tests exercise preparation, success, authentication cancellation, insufficient balance/expired quote recovery, screen closure and one-use authorization, plus rendered manual/automatic progress and terminal success navigation. The success test checks the actual navigation stack and swipe-back state rather than relying on screenshot text recognition.PaykitPaymentRequestServiceTests.swift— production subscription-detail selection excludes expired unpaid periods after expiry, refresh, restart and rollover, while preserving paid payer rows and creator receipts.Evidence
ac7e2fdbbuilt unchanged app sources and exercised the production warning dialogs and PIN keypad on an iPhone 17 Pro simulator. Accepting both fee warnings and entering the PIN produced one uncancelled send callback. Back from PIN or cancelling a warning produced zero sends; each explicit swipe/PIN retry produced exactly one callback with one prepared quote. These controlled hosted probes perform no transfers. The existing authorization, expiry-notification and unpaid-history assertions also passed in that review; no new findings were reported.9450bdb, including signed-quote expiry/retry, and the embedded retry result ata3d15fb. Those are historical tested revisions, not a fresh funded run onac7e2fdb.ac7e2fdbintegrates mastere7a21e8e, preserving both one-time request expiry notifications and expired unpaid subscription history. Independent review checked the exact resolved service file and both parent revisions, including all upstream expiry tests, and found no introduced issue.ec30b3b0: the real navigation stack hides/restores the send view while a suspended authorization seam returns approval or cancellation; exactly-once continuation is checked. This does not claim actual PIN entry or biometrics were exercised.ac7e2fdbwith the CI exclusions: 2,744 unit executions and 43 UI executions (39 passed, four explicit fixture skips), zero failures. The navigation regression passed again in this final suite. Complete diagnostics and the xcresult summary were inspected: no compiler warnings/errors; runtime QoS diagnostics refer to test files that match current master byte-for-byte. The unavailable Trezor-emulator and local-Arbitrum-fork cases remain skipped, not passed.e7a21e8ebyte-for-byte.Historical evidence:
c1a21f38integrates master43c06a43and retains both the hosted USDT address route and the initial-subscription safe-back/success flow. Independent review found no introduced issue.c1a21f38with the CI unit exclusions: 2,726 unit executions and 43 UI executions (39 passed, four explicit fixture skips), zero failures. The four skips require the Trezor emulator or local Arbitrum fork. Complete build diagnostics and the result bundle were inspected; only AppIntents metadata-extraction notices were emitted during the build.43c06a43.78cfdb75, unit CI and the integration rerun passed. The original integration attempts encountered wallet-operation timeouts; those historical results are not new-head evidence.78cfdb75: Ovi's subscription-detail check and Piotr's review. These receipts retain their original tested revision.Gaps
The full funded J1 journey and manual case 1 were not rerun on
ac7e2fdb; their retained external funding/quote-expiry evidence belongs to the linked historical revisions. Actual PIN entry and warning/PIN cancellation with explicit retry are now covered by the exact-head external simulator probes above, using a counted send callback rather than transfers. Physical-device biometrics remain unverified. No new funded/provider end-to-end pass or wallet reset is claimed. Current-head unit, integration and lint checks passed. E2E run 39655058906 passed 13 of 14 cases, but the fixed-msat LNURL case failed on all three built-in attempts with a blank Send sheet and missingReviewAmount-primary. Screenshots and complete logs were inspected; no nil-unwrapping fatal was found in these simulator logs. This matches the symptom tracked in #1087; its cause remains unconfirmed. Exact CI merge31cfc5fbversus basee7a21e8epreserves the ordinary LNURL paths and cleanup; this PR's SendSheet differences are USDT Back handling and embedded initial-subscription routing. Evidence is recorded in #1087. No workflow rerun was requested, and full E2E success is not claimed.Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)Model and effort were verified from the contributing author session records and the independent merge review's execution metadata. Earlier review rounds are not included as new validation evidence.