feat: enhance isPaymentLink to support WC URI with pay parameter #317
Conversation
- Update isPaymentLink to detect payment links embedded in WC URIs - Handle both URL-encoded and non-encoded pay= parameters - Bump @walletconnect/react-native-compat to 2.23.2-canary.4
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ganchoradkov's task in 1m 5s —— View job Code Review CompleteAnalyzed changes to payment link detection logic. Tasks
Found 1 issue(s)Issue 1: Redundant URL decoding may cause incorrect behaviorID: walletkitutil-redundant-decode-a3f2 // Replace this:
const decodedPayUrl = decodeURIComponent(payParam);
return isPaymentUrl(decodedPayUrl);
// With this:
return isPaymentUrl(payParam);Automated Checks✅ No workflow security issues detected |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const params = new URLSearchParams(queryString); | ||
| const payParam = params.get('pay'); | ||
| if (payParam) { | ||
| const decodedPayUrl = decodeURIComponent(payParam); |
There was a problem hiding this comment.
🤖 Auto Review Issue: Redundant URL decoding may cause incorrect behavior
Severity: MEDIUM
Category: code_quality
Tool: Claude Auto Review
Context: URLSearchParams.get() already automatically decodes URL-encoded parameter values. Calling decodeURIComponent() on the result performs a second decode, which could fail or produce incorrect results if the URL contains characters that look like encoded sequences (e.g., a literal %20 in the URL would be decoded twice).
Recommendation: Remove the redundant decodeURIComponent() call:
// Replace this:
const decodedPayUrl = decodeURIComponent(payParam);
return isPaymentUrl(decodedPayUrl);
// With this:
return isPaymentUrl(payParam);There was a problem hiding this comment.
Pull request overview
This PR enhances the isPaymentLink function to detect WalletConnect payment links embedded within WC URIs as query parameters, in addition to detecting direct payment URLs. It also bumps the @walletconnect/react-native-compat package to version 2.23.2-canary.4.
Changes:
- Enhanced
isPaymentLinkto parse WC URIs and check forpay=query parameters - Extracted payment URL validation logic into a separate
isPaymentUrlhelper function - Updated
@walletconnect/react-native-compatfrom2.23.2-canary.1to2.23.2-canary.4
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| wallets/rn_cli_wallet/src/utils/WalletKitUtil.ts | Added logic to detect and validate payment links in WC URIs; refactored validation into helper function |
| wallets/rn_cli_wallet/package.json | Bumped @walletconnect/react-native-compat to 2.23.2-canary.4 |
| wallets/rn_cli_wallet/yarn.lock | Updated lockfile checksums for dependency version change |
Comments suppressed due to low confidence (1)
wallets/rn_cli_wallet/src/utils/WalletKitUtil.ts:66
- The enhanced
isPaymentLinkfunction introduces new logic for parsing WC URIs with pay parameters, but there are no tests covering this new behavior. Since the codebase has a jest configuration and a test directory, consider adding unit tests to verify:
- Direct payment URLs are correctly identified
- WC URIs with URL-encoded pay parameters are correctly handled
- WC URIs with non-encoded pay parameters are correctly handled
- Invalid WC URIs return false
- Edge cases like missing query strings or invalid URLs are handled properly
export function isPaymentLink(uri: string): boolean {
// Handle WC URI with pay= parameter (e.g., wc:...&pay=https://pay.walletconnect.com/...)
if (uri.startsWith('wc:')) {
const queryStart = uri.indexOf('?');
if (queryStart === -1) {
return false;
}
const queryString = uri.substring(queryStart + 1);
const params = new URLSearchParams(queryString);
const payParam = params.get('pay');
if (payParam) {
const decodedPayUrl = decodeURIComponent(payParam);
return isPaymentUrl(decodedPayUrl);
}
return false;
}
// Handle direct payment URL
return isPaymentUrl(uri);
}
function isPaymentUrl(url: string): boolean {
try {
const parsed = new URL(url);
const hostname = parsed.hostname.toLowerCase();
const isPayHost =
hostname === 'pay.walletconnect.com' ||
hostname === 'www.pay.walletconnect.com';
return isPayHost && parsed.searchParams.has('pid');
} catch {
return false;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const decodedPayUrl = decodeURIComponent(payParam); | ||
| return isPaymentUrl(decodedPayUrl); |
There was a problem hiding this comment.
The URLSearchParams.get method already returns a decoded value, so calling decodeURIComponent on it will result in double-decoding. This could cause issues if the payment URL contains percent-encoded characters that should remain encoded, or if it contains characters like % that aren't part of a valid percent-encoding sequence.
Remove the decodeURIComponent call and use the payParam value directly, as URLSearchParams already handles the decoding.
| const decodedPayUrl = decodeURIComponent(payParam); | |
| return isPaymentUrl(decodedPayUrl); | |
| return isPaymentUrl(payParam); |
| // Handle direct payment URL | ||
| return isPaymentUrl(uri); | ||
| } | ||
|
|
There was a problem hiding this comment.
The newly introduced isPaymentUrl helper function lacks documentation. Since it's an internal helper function used by isPaymentLink, consider adding a JSDoc comment explaining its purpose, parameters, and return value to improve code maintainability.
| /** | |
| * Internal helper that determines whether a given URL string | |
| * represents a valid WalletConnect Pay payment URL. | |
| * | |
| * A valid payment URL must: | |
| * - Use the `pay.walletconnect.com` (or `www.pay.walletconnect.com`) host, and | |
| * - Include a `pid` query parameter. | |
| * | |
| * Invalid or non-parseable URLs will return `false`. | |
| * | |
| * @param url - The URL string to validate as a WalletConnect Pay payment link. | |
| * @returns `true` if the URL is a WalletConnect Pay payment URL, otherwise `false`. | |
| */ |
| // Check if a URI is a WalletConnect Pay payment link | ||
| export function isPaymentLink(uri: string): boolean { | ||
| // Handle WC URI with pay= parameter (e.g., wc:...&pay=https://pay.walletconnect.com/...) | ||
| if (uri.startsWith('wc:')) { | ||
| const queryStart = uri.indexOf('?'); | ||
| if (queryStart === -1) { | ||
| return false; | ||
| } | ||
| const queryString = uri.substring(queryStart + 1); | ||
| const params = new URLSearchParams(queryString); | ||
| const payParam = params.get('pay'); | ||
| if (payParam) { | ||
| const decodedPayUrl = decodeURIComponent(payParam); | ||
| return isPaymentUrl(decodedPayUrl); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // Handle direct payment URL | ||
| return isPaymentUrl(uri); | ||
| } | ||
|
|
There was a problem hiding this comment.
When a WC URI with an embedded payment link is detected, the full WC URI is passed to handlePaymentLink, but this function expects to receive only the payment URL. The Pay SDK's getPaymentOptions method will likely fail when given a WC URI instead of the actual payment URL.
Consider adding a helper function to extract the payment URL from a WC URI, and ensure that handlePaymentLink always receives the actual payment URL regardless of whether the input was a direct payment URL or a WC URI with a pay parameter.
| // Check if a URI is a WalletConnect Pay payment link | |
| export function isPaymentLink(uri: string): boolean { | |
| // Handle WC URI with pay= parameter (e.g., wc:...&pay=https://pay.walletconnect.com/...) | |
| if (uri.startsWith('wc:')) { | |
| const queryStart = uri.indexOf('?'); | |
| if (queryStart === -1) { | |
| return false; | |
| } | |
| const queryString = uri.substring(queryStart + 1); | |
| const params = new URLSearchParams(queryString); | |
| const payParam = params.get('pay'); | |
| if (payParam) { | |
| const decodedPayUrl = decodeURIComponent(payParam); | |
| return isPaymentUrl(decodedPayUrl); | |
| } | |
| return false; | |
| } | |
| // Handle direct payment URL | |
| return isPaymentUrl(uri); | |
| } | |
| // Extract a WalletConnect Pay payment URL from a given URI (WC URI or direct payment URL) | |
| export function extractPaymentUrl(uri: string): string | null { | |
| if (!uri) { | |
| return null; | |
| } | |
| // Handle WC URI with pay= parameter (e.g., wc:...&pay=https://pay.walletconnect.com/...) | |
| if (uri.startsWith('wc:')) { | |
| const queryStart = uri.indexOf('?'); | |
| if (queryStart === -1) { | |
| return null; | |
| } | |
| const queryString = uri.substring(queryStart + 1); | |
| const params = new URLSearchParams(queryString); | |
| const payParam = params.get('pay'); | |
| if (!payParam) { | |
| return null; | |
| } | |
| const decodedPayUrl = decodeURIComponent(payParam); | |
| return isPaymentUrl(decodedPayUrl) ? decodedPayUrl : null; | |
| } | |
| // Handle direct payment URL | |
| return isPaymentUrl(uri) ? uri : null; | |
| } | |
| // Check if a URI is a WalletConnect Pay payment link | |
| export function isPaymentLink(uri: string): boolean { | |
| return extractPaymentUrl(uri) !== null; | |
| } |
| } | ||
| } | ||
|
|
||
| // Check if a URI is a WalletConnect Pay payment link |
There was a problem hiding this comment.
The comment describes the function as checking if a URI is a WalletConnect Pay payment link, but it doesn't mention that the function also validates the hostname and pid parameter. Consider updating the comment to be more descriptive: "Check if a URI is a valid WalletConnect Pay payment link (either a direct payment URL or a WC URI with a pay parameter)"
| // Check if a URI is a WalletConnect Pay payment link | |
| // Check if a URI is a valid WalletConnect Pay payment link (either a direct payment URL or a WC URI with a pay parameter) |
Summary
isPaymentLinkfunction to detect payment links embedded in WalletConnect URIspay=query parameters in WC URIs@walletconnect/react-native-compatto2.23.2-canary.4Supported payment link formats:
Direct payment URL:
WC URI with pay parameter (non-encoded):
WC URI with pay parameter (URL-encoded):
Test plan