Skip to content

Comments

feat: move collectData to per-PaymentOption and update IC flow#293

Merged
jakubuid merged 4 commits intodevelopfrom
feat/per-option-collect-data
Feb 18, 2026
Merged

feat: move collectData to per-PaymentOption and update IC flow#293
jakubuid merged 4 commits intodevelopfrom
feat/per-option-collect-data

Conversation

@jakubuid
Copy link
Collaborator

Summary

  • Moves collectData from top-level PaymentOptionsResponse to each individual PaymentOption, enabling different payment options to have different data collection requirements
  • Removes the "get started" Intro screen (now part of the webview flow)
  • Adds a Summary screen after IC completion for payment confirmation
  • Adds a "Why info required?" explanation dialog
  • Redesigns payment options as a flat card list with per-option "Info required" badges
flowchart TD
    A[Payment Options] -->|Option has no collectData| B[Processing / Sign & Confirm]
    A -->|Option has collectData.url| C[WebView IC]
    A -->|Option has collectData.fields| D[Field-by-field IC]
    C --> E[Summary Screen]
    D --> E
    E -->|Confirm| B
    A -->|Why info required?| F[Explanation Dialog]
    F -->|Got it!| A
    B --> G[Success / Error]
Loading

Model changes

  • Pay.PaymentOption and Wallet.Model.PaymentOption: added collectData: CollectDataAction? = null
  • Updated mappers in both Pay SDK and WalletKit to propagate per-option collectData
  • Bumped yttrium-wcpay to 0.10.41 for backend support

UI changes

  • Removed IntroContent, IntroStepsTimeline, IntroStepItem composables
  • Removed dropdown-style option selector (PayWithDropdown)
  • Added PaymentOptionCard — flat cards with blue border selection + gray "Info required" badge
  • Added SummaryContent — post-IC confirmation with "Pay with" row and confirm button
  • Added WhyInfoRequiredContent — regulatory compliance explanation
  • Added shared MerchantIcon and PaymentTitle helper composables
  • Button text: "Pay $X" for direct options, "Continue" for IC options

Test plan

  • ./gradlew :product:pay:testDebugUnitTest — passes
  • ./gradlew :product:walletkit:testDebugUnitTest — passes
  • ./gradlew :sample:wallet:assembleDebug — builds successfully
  • Manual: scan payment link → verify Options list with IC badges
  • Manual: tap "Why info required?" → see explanation → "Got it!" returns to options
  • Manual: select non-IC option → "Pay" → processing → success
  • Manual: select IC option → "Continue" → WebView → Summary → "Pay" → processing

🤖 Generated with Claude Code

jakubuid and others added 2 commits February 13, 2026 12:27
Move collectData from the top-level PaymentOptionsResponse to each
individual PaymentOption, enabling different payment options to have
different data collection requirements.

- Add collectData field to Pay.PaymentOption and Wallet.Model.PaymentOption
- Update mappers to propagate per-option collectData
- Remove Intro screen (now handled by webview)
- Add Summary screen (post-IC confirmation before payment)
- Add "Why info required?" explanation dialog
- Redesign options as flat card list with "Info required" badges
- Button shows "Continue" for IC options, "Pay" for direct options
- Bump yttrium-wcpay to 0.10.41

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 16, 2026

Claude finished @jakubuid's task in 1m 35s —— View job


Code Review Complete

PR Size: 7 files, +501/-702 lines (net: -201 lines) ✅

Found 3 issue(s)

Issue 1: Race condition in payment confirmation with collected data

ID: paymentviewmodel-collectdata-race-a3f2
File: sample/wallet/src/main/kotlin/com/reown/sample/wallet/ui/routes/dialog_routes/payment/PaymentViewModel.kt:347-353
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: collectedValues map is accessed from executePayment() which can be triggered from multiple paths (direct payment, post-WebView, post-field collection)
  • Risk: When user selects option without collectData, then later switches flows, stale data from previous sessions may be included
  • Impact: Payment confirmation could send incorrect/stale user data to backend
  • Trigger: Select option with IC → go back → select option without IC → confirm

Recommendation: Clear collectedValues when option selection changes, or scope collected data per optionId:

fun onOptionSelected(optionId: String) {
    val option = storedPaymentOptions.find { it.id == optionId } ?: return
    selectedOptionId = optionId
    
    // Clear stale collected data when changing options
    if (option.collectData == null) {
        collectedValues.clear()
    }
    
    val collectData = option.collectData
    // ...rest of logic
}

Issue 2: estimatedTxs coercion silently loses data

ID: mappers-eta-truncation-b7d1
File: product/pay/src/main/java/com/walletconnect/pay/Mappers.kt:58
Severity: LOW
Category: code_quality

Context:

  • Pattern: Line 58 converts etaS (ULong) to Int with coerceAtMost(Int.MAX_VALUE.toULong().toLong()).toInt()
  • Risk: Values exceeding Int.MAX_VALUE (2.1B seconds ≈ 68 years) are silently clamped
  • Impact: Misleading UI if backend ever returns extremely large eta values
  • Trigger: Backend returns etaS > 2147483647

Recommendation: Since field name is etaS (seconds) and values >68 years are unrealistic, add explicit null check instead:

estimatedTxs = option.etaS.let { eta ->
    if (eta <= Int.MAX_VALUE.toULong()) eta.toInt() else null
}

Issue 3: Unconstrained image URLs from payment response

ID: paymentroute-asyncimage-xss-4c9a
File: sample/wallet/src/main/kotlin/com/reown/sample/wallet/ui/routes/dialog_routes/payment/PaymentRoute.kt:477-503
Severity: LOW
Category: security

Context:

  • Pattern: AsyncImage loads merchant icons and token icons from backend-provided URLs without validation
  • Risk: Malicious URLs (e.g., javascript:, data:, file paths) could be injected if backend is compromised
  • Impact: Coil library mitigates most XSS vectors, but no explicit URL scheme whitelist
  • Trigger: Compromised backend or MITM attack modifying payment response

Recommendation: Validate icon URL schemes before loading:

@Composable
private fun MerchantIcon(paymentInfo: Wallet.Model.PaymentInfo?, size: Dp) {
    val iconUrl = paymentInfo?.merchant?.iconUrl?.takeIf { url ->
        url.startsWith("https://", ignoreCase = true) || url.startsWith("http://", ignoreCase = true)
    }
    iconUrl?.let { validUrl ->
        AsyncImage(model = validUrl, ...)
    } ?: run {
        // fallback placeholder
    }
}

Summary

Architecture: Per-option collectData migration is well-executed. Mappers correctly propagate nullable fields. UI flow properly handles 3 branches (no IC, WebView IC, field-by-field IC).

State Management: ViewModel uses MutableStateFlow correctly. No suspend fun leaks. viewModelScope used appropriately.

Critical Checks:

  • PR size: 7 files, reasonable scope ✅
  • External domains: Only reown.com/walletconnect domains ✅
  • Payment idempotency: Not applicable (read-only UI changes) ✅
  • Breaking changes: collectData moved from response to per-option (requires backend support via yttrium-wcpay 0.10.41 bump) ⚠️

Manual Testing Required:
Per test plan checklist - especially IC WebView → Summary → Pay flow with per-option collectData.

jakubuid and others added 2 commits February 17, 2026 12:48
…h navigation

When navigating back and forth in the payment flow before completing a payment,
the success modal would not dismiss on "Got it" or X click. Root causes:

1. Missing launchSingleTop on payment navigation caused duplicate dialog entries
   to stack on the backstack
2. popBackStack(Route.Connections.path) could silently fail if the backstack was
   in an unexpected state, leaving the dialog visible
3. SharedFlow replay=1 leaked stale payment data to duplicate ViewModel instances

Fixes: WCP4-41

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ccess

fix: payment success modal stuck after back/forth navigation
@claude
Copy link

claude bot commented Feb 17, 2026

Claude finished @jakubuid's task in 2m 14s —— View job


🔄 Incremental Review:

Previous Issues Status

Issue 1 (paymentviewmodel-collectdata-race-a3f2) - RESOLVED
The race condition has been fixed. The code now clears collectedValues at line 116 when entering field-by-field IC mode, ensuring stale data doesn't persist across option changes.

Issue 2 (mappers-eta-truncation-b7d1) - STILL EXISTS
Line 58 in Mappers.kt still coerces etaS to Int.MAX_VALUE without null handling for unrealistic values.

Issue 3 (paymentroute-asyncimage-xss-4c9a) - STILL EXISTS
Lines 483-489 and 460-466 in PaymentRoute.kt still load image URLs from backend without scheme validation.

New Issues

No new code quality or security issues found in this update.

Breaking Changes

Found 2 breaking change(s)

Issue 1: New parameter added to PaymentOption data class constructor

ID: brk-pay-paymentoption-collectdata-field-a8c3
File: product/pay/src/main/java/com/walletconnect/pay/Pay.kt:40
Severity: LOW
Category: breaking_change

Context:

  • Pattern: Data class constructor signature changed - new parameter collectData: CollectDataAction? = null added as 5th parameter to Pay.PaymentOption
  • Risk: Consumers who construct PaymentOption instances using positional parameters will face compilation errors. Kotlin data class .copy() calls are backward compatible due to default value.
  • Impact: Minimal - PaymentOption is typically returned by the SDK's getPaymentOptions() API, not constructed by consumers. Consumers who mock or test this class may need updates.
  • Trigger: Compilation time for consumers who construct PaymentOption with positional parameters, or consumers using Java interop without named parameters.

Recommendation:

  • Document in release notes that PaymentOption now includes per-option collectData
  • Mark as minor version bump since the field has a default value and is unlikely to break typical usage
  • Consider adding @JvmOverloads if Java consumers exist to maintain backward compatibility

Issue 2: New parameter added to Wallet.Model.PaymentOption data class constructor

ID: brk-wallet-paymentoption-collectdata-field-b2f7
File: product/walletkit/src/main/kotlin/com/reown/walletkit/client/Wallet.kt:530
Severity: LOW
Category: breaking_change

Context:

  • Pattern: Data class constructor signature changed - new parameter collectData: CollectDataAction? = null added as 5th parameter to Wallet.Model.PaymentOption
  • Risk: Same as Issue 1 - consumers constructing this class with positional parameters will face compilation errors.
  • Impact: Minimal - Wallet.Model.PaymentOption is returned by WalletKit's payment APIs, not typically constructed by consumers. Mocking/testing code may need updates.
  • Trigger: Compilation time for consumers who construct Wallet.Model.PaymentOption with positional parameters.

Recommendation:

  • Document in release notes as part of the payment API enhancement
  • Mark as minor version bump since default value maintains backward compatibility for named parameter usage
  • Ensure sample app and integration tests demonstrate the new per-option collectData usage

Automated Checks

PR Size: 8 files, +522/-711 lines (under threshold)
External Domains: No new external domain URLs detected
Static Resources: N/A (no static resource changes)
GitHub Actions: N/A (no workflow changes)
WalletConnect Pay: Architecture properly maintained - per-option collectData correctly scoped

Summary

This incremental review confirms that the previous race condition (Issue 1) has been resolved. The two remaining issues from the last review (ETA truncation and image URL validation) still exist but are low severity. Two new breaking changes were detected - both are low severity constructor signature changes with default values that maintain backward compatibility for typical SDK usage patterns.

The migration path is straightforward: the top-level collectDataAction field was preserved in PaymentOptionsResponse, providing backward compatibility while enabling per-option data collection for enhanced flexibility.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@jakubuid jakubuid merged commit d4f8c5b into develop Feb 18, 2026
13 of 15 checks passed
@jakubuid jakubuid deleted the feat/per-option-collect-data branch February 18, 2026 07:37
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants