feat(pay): dynamic API endpoint routing based on payment link#375
feat(pay): dynamic API endpoint routing based on payment link#375
Conversation
Add tracing debug output to help diagnose API routing issues: - Log which API endpoint (prod/staging) is selected - Log config.base_url and is_custom_base_url check - Log in client_for_env to trace actual client selection Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Claude finished @jakubuid's task in 2m 1s —— View job Code Review CompleteFound 4 issue(s)Issue 1: Duplicate url_decode implementationID: mod-duplicate-url-decode-8a3f Context:
Recommendation: Extract to shared helper function at module level: // Near top of module with other helpers
fn url_decode(s: &str) -> String {
urlencoding::decode(s)
.map(|c| c.into_owned())
.unwrap_or_else(|_| s.to_string())
}Then remove duplicate implementations in both Issue 2: Potential race condition with global stateID: mod-payment-envs-race-4b2c Context:
Recommendation: Pre-initialize the HashMap to avoid runtime initialization: static PAYMENT_API_ENVS: RwLock<HashMap<String, PayApiEnv>> =
RwLock::new(HashMap::new());
fn set_payment_api_env(payment_id: &str, env: PayApiEnv) {
let mut envs = PAYMENT_API_ENVS
.write()
.expect("Payment API envs lock poisoned");
envs.insert(payment_id.to_string(), env);
}
fn get_payment_api_env(payment_id: &str) -> PayApiEnv {
PAYMENT_API_ENVS
.read()
.expect("Payment API envs lock poisoned")
.get(payment_id)
.copied()
.unwrap_or(PayApiEnv::Prod)
}Issue 3: URL parsing uses contains() instead of proper host checkID: mod-detect-env-contains-7e1a Context:
Recommendation: Parse as URL and check host component: fn detect_api_env(payment_link: &str) -> PayApiEnv {
let pay_url = extract_pay_url_for_env(payment_link);
if let Ok(url) = Url::parse(&pay_url) {
if let Some(host) = url.host_str() {
if host == "dev.pay.walletconnect.com"
|| host.ends_with(".dev.pay.walletconnect.com") {
return PayApiEnv::Staging;
}
}
}
PayApiEnv::Prod
}Issue 4: Missing unbounded memory growth protectionID: mod-payment-envs-memory-9f4e Context:
Recommendation: Add bounded LRU cache or TTL-based eviction: use std::collections::HashMap;
use std::time::{Instant, Duration};
struct CachedEnv {
env: PayApiEnv,
inserted_at: Instant,
}
static PAYMENT_API_ENVS: RwLock<HashMap<String, CachedEnv>> =
RwLock::new(HashMap::new());
const MAX_CACHE_SIZE: usize = 10_000;
const CACHE_TTL: Duration = Duration::from_secs(3600);
fn set_payment_api_env(payment_id: &str, env: PayApiEnv) {
let mut envs = PAYMENT_API_ENVS
.write()
.expect("Payment API envs lock poisoned");
// Evict expired entries if cache is large
if envs.len() >= MAX_CACHE_SIZE {
let now = Instant::now();
envs.retain(|_, v| now.duration_since(v.inserted_at) < CACHE_TTL);
}
envs.insert(payment_id.to_string(), CachedEnv {
env,
inserted_at: Instant::now(),
});
}SummaryImplementation adds dynamic API routing based on payment link domain. Core logic functional, tests comprehensive. Main concerns: security issue with substring matching (Issue 3), unbounded memory growth (Issue 4), code duplication (Issue 1), and suboptimal global state pattern (Issue 2). Critical: Fix Issue 3 (URL parsing) before merge. Issues 1, 2, 4 can be addressed post-merge. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| RwLock::new(None); | ||
|
|
||
| /// URL decode helper | ||
| fn url_decode(s: &str) -> String { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Duplicate url_decode implementation
Severity: LOW
Category: code_quality
Tool: Claude Auto Review
Context:
- Pattern: Function
url_decodeat lines 638-642 duplicates identical implementation fromextract_payment_idfunction (lines 1275-1279) - Risk: Code duplication increases maintenance burden and risk of inconsistent behavior if one implementation changes
- Impact: Future bug fixes or improvements must be applied in multiple places
- Trigger: Any change to URL decoding logic requires coordinated updates
Recommendation: Extract to shared helper function at module level:
// Near top of module with other helpers
fn url_decode(s: &str) -> String {
urlencoding::decode(s)
.map(|c| c.into_owned())
.unwrap_or_else(|_| s.to_string())
}Then remove duplicate implementations in both extract_pay_url_for_env (line 638) and extract_payment_id::url_decode (line 1275).
| } | ||
|
|
||
| /// Global storage for payment ID -> API environment mapping | ||
| static PAYMENT_API_ENVS: RwLock<Option<HashMap<String, PayApiEnv>>> = |
There was a problem hiding this comment.
🤖 Auto Review Issue: Potential race condition with global state
Severity: MEDIUM
Category: code_quality
Tool: Claude Auto Review
Context:
- Pattern: Global
PAYMENT_API_ENVSusesRwLock<Option<HashMap<>>>requiring two-step initialization (lines 634, 678) - Risk: Multiple threads calling
set_payment_api_envconcurrently could experience lock contention or initialization races - Impact: Potential performance degradation under concurrent load; not a correctness issue due to RwLock protection
- Trigger: Concurrent payment requests for same or different payment IDs
Recommendation: Pre-initialize the HashMap to avoid runtime initialization:
static PAYMENT_API_ENVS: RwLock<HashMap<String, PayApiEnv>> =
RwLock::new(HashMap::new());
fn set_payment_api_env(payment_id: &str, env: PayApiEnv) {
let mut envs = PAYMENT_API_ENVS
.write()
.expect("Payment API envs lock poisoned");
envs.insert(payment_id.to_string(), env);
}
fn get_payment_api_env(payment_id: &str) -> PayApiEnv {
PAYMENT_API_ENVS
.read()
.expect("Payment API envs lock poisoned")
.get(payment_id)
.copied()
.unwrap_or(PayApiEnv::Prod)
}| let pay_url = extract_pay_url_for_env(payment_link); | ||
|
|
||
| // dev.pay.walletconnect.* -> staging API | ||
| if pay_url.contains("dev.pay.walletconnect") { |
There was a problem hiding this comment.
🤖 Auto Review Issue: URL parsing uses contains() instead of proper host check
Severity: MEDIUM
Category: security
Tool: Claude Auto Review
Context:
- Pattern: Line 667 uses
pay_url.contains("dev.pay.walletconnect")for environment detection - Risk: Substring matching can be fooled by crafted URLs like
https://malicious.com?redirect=dev.pay.walletconnect.comorhttps://dev.pay.walletconnect.com.evil.com - Impact: Could route legitimate payment to wrong API or enable subdomain confusion attacks
- Trigger: User-controlled payment link with substring "dev.pay.walletconnect" anywhere in URL
Recommendation: Parse as URL and check host component:
fn detect_api_env(payment_link: &str) -> PayApiEnv {
let pay_url = extract_pay_url_for_env(payment_link);
if let Ok(url) = Url::parse(&pay_url) {
if let Some(host) = url.host_str() {
if host == "dev.pay.walletconnect.com"
|| host.ends_with(".dev.pay.walletconnect.com") {
return PayApiEnv::Staging;
}
}
}
PayApiEnv::Prod
}| } | ||
|
|
||
| /// Global storage for payment ID -> API environment mapping | ||
| static PAYMENT_API_ENVS: RwLock<Option<HashMap<String, PayApiEnv>>> = |
There was a problem hiding this comment.
🤖 Auto Review Issue: Missing unbounded memory growth protection
Severity: MEDIUM
Category: performance
Tool: Claude Auto Review
Context:
- Pattern: Global
PAYMENT_API_ENVSHashMap grows without bounds as payment IDs accumulate (line 678) - Risk: Long-running processes processing many payments will accumulate entries indefinitely
- Impact: Memory leak in server/daemon scenarios; each payment ID consumes ~48 bytes + string allocations
- Trigger: Processing thousands of unique payment IDs over application lifetime
Recommendation: Add bounded LRU cache or TTL-based eviction:
use std::collections::HashMap;
use std::time::{Instant, Duration};
struct CachedEnv {
env: PayApiEnv,
inserted_at: Instant,
}
static PAYMENT_API_ENVS: RwLock<HashMap<String, CachedEnv>> =
RwLock::new(HashMap::new());
const MAX_CACHE_SIZE: usize = 10_000;
const CACHE_TTL: Duration = Duration::from_secs(3600);
fn set_payment_api_env(payment_id: &str, env: PayApiEnv) {
let mut envs = PAYMENT_API_ENVS
.write()
.expect("Payment API envs lock poisoned");
// Evict expired entries if cache is large
if envs.len() >= MAX_CACHE_SIZE {
let now = Instant::now();
envs.retain(|_, v| now.duration_since(v.inserted_at) < CACHE_TTL);
}
envs.insert(payment_id.to_string(), CachedEnv {
env,
inserted_at: Instant::now(),
});
}Summary
Implementation adds dynamic API routing based on payment link domain. Core logic functional, tests comprehensive. Main concerns: security issue with substring matching (Issue 3), unbounded memory growth (Issue 4), code duplication (Issue 1), and suboptimal global state pattern (Issue 2).
1 participant
Summary
Add dynamic Pay API endpoint selection based on payment link domain:
dev.pay.walletconnect.com→ staging API (https://staging.api.pay.walletconnect.com)staging.pay.walletconnect.com→ prod API (https://api.pay.walletconnect.com)pay.walletconnect.com(prod) → prod APIChanges
PayApiEnvenum for routing (Staging/Prod)detect_api_env()function to parse payment links and determine environmentwc:...?pay=https://...) with URL decodingclient_prod,client_staging) for separate endpointsArchitecture
flowchart TD A[Payment Link] --> B{detect_api_env} B -->|dev.pay.walletconnect.*| C[PayApiEnv::Staging] B -->|staging.pay.*, pay.*, bare ID| D[PayApiEnv::Prod] C --> E[client_staging] D --> F[client_prod] E --> G[staging.api.pay.walletconnect.com] F --> H[api.pay.walletconnect.com]Test plan
detect_api_env()with various URL formatsextract_pay_url_for_env()🤖 Generated with Claude Code