Repository navigation
feat: refine usdt payment and earn screens - #942
Conversation
[Medium impact] The changes reach several wallet workflows, but stay within Paykit, USDT, and Earn screens. The PR appears safe to merge, with non-blocking improvements for translated payment choices and full-address review. FindingsSummaryThis PR updates Earn and USDT payment screens and adds separate receiving-method choices.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["Choose receiving methods"] --> B["Store selected methods in draft"]
B --> C["Filter by asset and availability"]
C --> D{"Any methods left?"}
D -- No --> E["Block proposal"]
D -- Yes --> F["Recheck before sending"]
F --> G["Send proposal with selected methods"]
Reviews (1) · Last reviewed commit: "feat: refine usdt payment and earn scree..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. iOS side of the same change: per-method Savings, Spending and USDT chips for requests and subscriptions, the subscription selection carried into the proposal and its pre-send recheck, bridge amounts moved into expanded details, and the relay line dropped from the Earn intro (it stays in the authorization screen). Reviewed b817d74, full tier.
What I checked, and 5 candidates I ruled out
Read in full: PaykitRequestPricing.swift (subscriptionEndpoints), PaykitPaymentRequestService.swift (proposeSubscription and the endpoint recheck before sending), PaykitAcceptedMethods.swift, CreateSubscriptionView.swift, SubscriptionsView.swift (SubscriptionSheet), CreatePaymentRequestView.swift, PubkyAuthApprovalSheet.swift, UsdtSendView.swift, Localizable.strings hunks
Call sites traced: subscriptionEndpoints (view, propose, recheck), PaykitSubscriptionDraft construction (the new optional field defaults to nil in the memberwise init), relayOrigin (still rendered by relayOriginSection in the authorization screen), every reference to the removed string keys (none left)
CI: Run Tests still running at review time; Greptile passed.
Not built here: iOS does not build on this reviewer's Linux host, so this is reasoned from the code; Run Tests is the gate.
Ruled out
- Recheck before sending disagrees with the first computation: both calls pass the same
draft.acceptedPaymentEndpointIdentifiers, so the guard only trips when availability really changed. - Empty selection still submits: the details button is disabled on
acceptedEndpoints.isEmpty, and the service's guard rejects an empty list. - Stale selection after an asset change on the amount screen:
onContinueresets the selection tonilwhen the asset changes. - Required USDT claim cannot be approved once the switch is hidden:
loadUsdtAddresssetsshareUsdt = truefor a required claim once the address loads. - Relay disclosure lost: removed from the intro only; the authorization screen still shows it.
Merge confidence: 4/5, no findings, but Run Tests had not finished and the Swift is not built here.
e0f1cae to
390fb9c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 87315a3.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Payment-request and subscription method selection, proposal endpoint filtering, USDT review amounts, deposit history, bridge support, Earn authorization, and the funding-source menu match the updated journeys. Bare 0x addresses route through the scanner, Send paste, and deeplink path. Ordinary Send → Enter Manually validates the same address and then does not open USDT; Android #1494 at 2057b38 sends that manual entry through its scan handler. Earlier chip-wrapping and address-expansion comments are present in this head as WrappingHStack and the tap-to-expand address.
CI Run Tests passed on this head and does not execute manual-entry navigation. Run Integration Tests was still pending. This review did not rerun tests or start a simulator.
Device testing: not performed in this review.
Findings
- [MEDIUM] Route bare USDT addresses from Enter Manually — inline at
Bitkit/ViewModels/AppViewModel.swift:1325.
Related primary: synonymdev/bitkit-android#1494.
jvsena42
left a comment
There was a problem hiding this comment.
One MEDIUM and two LOW findings, inline. Reviewed 87315a3. I reviewed this as a funds and remote-input change: it adds bare-address routing, the send confirmation, and payment-request and subscription endpoints. The MEDIUM and the copy LOW were reproduced on the iPhone 17 simulator, built from this head with the local USDT fixture.
Checked and clean:
- Send confirmation. Swipe sends
quoteunchanged (UsdtSendView.swift:634-645). Fee ismaximumFee, Max total debit isamount + maximumFee, and Recipient gets isreceivedAmount. Moving the bridge amounts into expanded details is the agreed design and is not raised. - Bare-address input.
usdtValidateRecipient→ coreparse_addressenforces the0xprefix, length 42, a non-zero address and EIP-55 for mixed case, and it rejects token, OFT and paymaster addresses. No Bitcoin, Lightning or LNURL payload starts with0x. Hardware scope rejects the address (AppViewModel.swift:1280), and the opt-in prompt still shows when USDT is off (UsdtSendView.swift:112). The scanner and clipboard paths route correctly; only Enter Manually does not (inline). - Subscriptions and requests.
subscriptionEndpoints(selected:)intersects the selection with availability and asset. The proposal and its pre-send recheck use the same identifiers, an empty selection disables the button, and the service also rejects it. Amount Continue resets the selection only on an asset change, and the sheet-scoped draft resets on reopen. - Earn consent. A required claim still gates Approve on the loaded address.
approvedUsdtAddressis gated onshareUsdt && isUsdtEnabled, and the relay disclosure stays inrelayOriginSection. - Figma, all 9 linked frames:
- Earn, USDT-only Earn, basic confirmation, technical details, payment requests, subscriptions, deposit-history entry, funding-source menu and small action buttons were checked for spacing, type, tokens, states and icons. That includes the new
arrow-*-rightandpluspaths against the exported frame assets, and the removed-chip 50% opacity with +. - No deltas apart from the expanded-address LOW inline. The middle truncation of the address is kept deliberately: it shows both ends.
- Earn, USDT-only Earn, basic confirmation, technical details, payment requests, subscriptions, deposit-history entry, funding-source menu and small action buttons were checked for spacing, type, tokens, states and icons. That includes the new
- Performance. Not performance-relevant: layout and routing only, with no new polling, waits or locks.
Android twin synonymdev/bitkit-android#1494 already routes a bare address from Enter Manually (onAddressContinue → handleScan → isUsdtPaymentInput), so the MEDIUM is iOS-only. The copy LOW applies to both.
Device gate: not run yet — held until the MEDIUM is fixed. J2/J3 need two linked wallets; J1/J4–J7 run against the local USDT fixture.
83565c1 to
cce9ade
Compare
|
Rebased on current master and retained the reviewed USDT fixes. The new receive-refresh screen had a stale QR component call that prevented compilation; it now uses the current edit callback. The rebased build and all 86 selected tests pass, including receive refresh and manual USDT entry. SwiftFormat passes for all 35 changed Swift files; translation validation reports no errors. Requesting another review on the updated head. |
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 cce9ade.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
The new subscription method field has no initializer default, so the existing PaykitSubscriptionDraft call sites would fail to type-check. Bare-address routing, Earn address sharing, bridge review amounts, and request and subscription endpoint selection were traced in this source. Those contracts were compared with bitkit-android#1494 at 1c37a7e; that is not a review of the Android PR. Android defaults the subscription selection to null, which is what makes an omitted selection mean every available method for the asset.
Earlier comments on Enter Manually, the optional Earn wording, chip wrapping, and the expanded-address divider are addressed in this head.
Validation: tests were not executed in this review. Validate Translations passed on this head. Run Tests and Run Integration Tests were still pending, and the Swift compiler was not available here.
Device testing: not performed in this review.
Findings
- [HIGH] Default the subscription method selection — inline at
Bitkit/Services/PaykitPaymentRequestService.swift:513.
|
The E2E build passed on retry on the same commit, including storyboard compilation and app upload. The failure appears to have been transient; no source change was needed. Local E2E builds also passed, including a forced storyboard rebuild. The iOS unit and integration jobs passed; the downstream E2E tests are now queued. Retry run. |
ovitrif
left a comment
There was a problem hiding this comment.
Code review from our side, no device runs, at cce9ade: one MEDIUM, inline, which is a product call rather than a code bug. Every earlier finding from @jvsena42, @piotr-iohk and Greptile is fixed at this head.
Checked and clean: bare-address routing from scanner, paste, Enter Manually and clipboard (opt-in and hardware-wallet guards intact); the send confirmation's amount, fee and maximum debit; request and subscription method selection enforced in the proposal and the pre-send recheck, with an empty selection blocked and USDT excluded when disabled; Earn approval blocked until the address loads, relay disclosure still on the authorization screen.
jvsena42
left a comment
There was a problem hiding this comment.
One more design delta from the Figma pass.
jvsena42
left a comment
There was a problem hiding this comment.
One more Figma delta on Create Subscription.
|
Mirrored the two journey wording corrections from John's Android device review in f76c499. Both platforms now expect "Not enough USDT to cover the amount and fees." for manually entering the full balance and "Refund in progress" for the pending refund. Checked both against the current strings and error/status mappings; XML and action parity checks pass. Product copy and behavior are unchanged. |
jvsena42
left a comment
There was a problem hiding this comment.
Re-review at f76c4998. No HIGH or MEDIUM. All five earlier findings are fixed:
- MEDIUM, bare
0xin Enter Manually: fixed.SendEnterManuallyView.swift:83routes0xtohandleSendScanafter whitespace is stripped, andtestOrdinaryManualEntryRoutesBareAddressToUsdtcovers it. A malformed0xstring still fails validation and keeps Continue disabled. Bitcoin, BOLT11 and LNURL input is unaffected, and hardware-wallet Send still rejects0x. - Optional-claim copy: fixed (
PubkyAuthApprovalSheet.swift:482-486,pubky_auth__usdt_intro_optional). - Expanded-address divider: fixed (
UsdtSendView.swift:469-489,SendSectionView.swift:35). - Deposit and Choose Recipient floating buttons: fixed (
.safeAreaInsetinUsdtReceiveView.swift:96-106andCreateSubscriptionView.swift:47-59).
Also checked and clean:
- Send: submission logic in
UsdtSendViewis unchanged; the delta is typography only. - Auth approval: consent gating is unchanged.
- Receive:
cce9ade0is the sameonEditadaptation master got in #947. Expect a one-line@ViewBuilderconflict on the next master merge.
One LOW inline: a journey and the QA doc still describe the old deposit button layout.
Device gate (partial): f76c499 — not run yet. Pending a simulator run of:
- Enter Manually
0xrouting - the expanded-address dividers
- the optional USDT auth claim
- both floating buttons
I'll approve once it passes.
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 7f904485.
The simulator gate (iPhone 17, local USDT fixture on an Arbitrum fork) ran at f76c4998. 7f904485 only rewords Docs/USDT-QA.md and the deposit-button journey step, and the gate already checked the behaviour that step now describes. All seven paths pass:
- Enter Manually: a bare
0xaddress, with or without surrounding spaces, typed or pasted, opens USDT Amount on Arbitrum One. Back twice restores the recipient.- A malformed address keeps Continue disabled with the toast.
- A
bcrt1q…address still opens Bitcoin send. - With USDT off, Continue shows the enable prompt: Cancel returns quietly, and Enable resumes USDT Amount.
- Polygon bridge review: the expanded address wraps to two lines and both dividers sit level. Collapse and Hide Details work.
- Pubky auth:
- An optional USDT claim shows "You can share…" with the "Share my USDT address" switch, and the switch stays off after Back.
- The combined claim keeps "Also share…".
- A USDT-only claim keeps the mandatory copy.
- Floating buttons: Cross-Network Deposits and Choose Recipient stay pinned, and the content scrolls behind them and clears them at the end, including at accessibility text sizes. On Create Subscription the button stays above the keyboard with the focused field visible.
- Small screens and sheets: the USDT intro buttons stack at large text. The received celebration's Details and primary buttons work.
One LOW inline on a journey step that asserts the Android error message. Hardware-wallet Send was not run because no hardware wallet was paired.
Device gate: f76c499 — journeys: 5 run (4 partial), 1 failed (usdt-payment-links-and-scans step 18, journey wording, inline); perf: no crash, no hang; Figma: 3 screens compared, 1 delta (compact address middle-truncated, deliberate per earlier review); l10n: English-only, pubky_auth__usdt_intro_optional / pubky_auth__share_usdt not yet translated
Closes #943
This PR refines the USDT payment and Earn screens after #884.
Description
Out of Scope
None.
Design
Home · USDT introduction · Receive fees · Earn · USDT-only Earn · Basic confirmation · Technical details · Payment requests · Subscriptions · Deposit history entry · Funding-source menu · Small action buttons
Subscription methods follow the agreed #1000 discussion: Savings and Spending remain individually selectable, and Bitcoin methods remain mutually exclusive with USDT.
Preview
Native captures were checked against these frames and shared in the local review gallery. They use isolated test data and verify presentation and interactions; they do not represent live payments or bridge settlement.
QA Notes
Journeys
share-receiving-details.xml— short Earn copy, optional USDT sharing and disabled behavior.paykit-quoted-usdt.xml— individual receiving methods and selected request options.create-and-propose.xml— subscriptions accept selected Bitcoin methods or USDT; the floating recipient action preserves scrolling, large text and keyboard access.usdt-payment-links-and-scans.xml— raw addresses and payment links route consistently, with opt-in and clipboard safeguards.usdt-opt-in.xml— readable enable actions at large text sizes.usdt-send-review-and-details.xml— basic confirmation and expanded technical details.usdt-bridge-details.xml— bridge details and a floating deposit-history action, including scrolling and large text.usdt-bridge-support.xml— compact support actions and refund history.Manual Tests
N/A
Automated Checks
UsdtWalletUITests.swift— ordinary manual entry routes a bare address to USDT, preserves it on back navigation, and offers the network chooser.PaykitRequestPricingTests.swift— filters subscription methods by selection, asset and current availability.PaykitPaymentRequestServiceTests.swift— sends only the selected subscription endpoint.UsdtPaymentLinkTests.swift— accepts raw addresses, rejects malformed recipients, preserves hardware scope and suppresses disabled automatic clipboard prompts.Local validation: 86 focused display, support, payment-link, pricing, authorization, receive-refresh and manual-entry tests passed after rebasing on master. The final native render build passed. All 35 changed Swift files pass SwiftFormat; translation validation has no errors, with the previously accepted English fallback for new copy. Subscription footer checks passed at normal and enlarged text sizes, including Name and Description editing above the keyboard. Native captures cover normal and large text, Home balances, send/receive, fees, completion, Earn and expanded address dividers. These checks do not exercise live payments or bridge settlement.