Conversation
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR updates ramp URL/search-param handling to better support dynamically loaded EVM tokens (e.g., WETH/WBTC) by making the frontend react to the “dynamic tokens loaded” state, and loosens route search validation to accept a wider range of URL values.
Changes:
- Added an external-store style subscription API in shared dynamic EVM token state to support React
useSyncExternalStore. - Updated
useRampUrlParamsto re-evaluatecryptoLockedonce dynamic EVM tokens finish loading and apply the token to the quote form store. - Relaxed TanStack Router search-param schema for
paymentMethod/rampTypefrom enum validation to plain strings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/shared/src/tokens/evm/dynamicEvmTokens.ts | Adds listeners + snapshot/subscribe helpers and notifies subscribers when token loading completes. |
| apps/frontend/src/types/searchParams.ts | Adjusts route search schema to accept string params for paymentMethod and rampType. |
| apps/frontend/src/hooks/useRampUrlParams.ts | Subscribes to dynamic token loaded state and re-applies cryptoLocked after token config becomes available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isNetworkEVM(networkType as Networks)) { | ||
| const dynamicConfig = getEvmTokenConfig(); | ||
| const networkTokens = dynamicConfig[networkType as EvmNetworks]; | ||
| if (networkTokens && tokenStr in networkTokens) { | ||
| return tokenStr as OnChainToken; | ||
| } | ||
| } |
There was a problem hiding this comment.
findOnChainToken returns tokenStr as OnChainToken for dynamically loaded EVM tokens (e.g. WETH/WBTC), but OnChainToken is currently EvmToken | AssetHubToken in shared, so this cast makes the type inaccurate and can hide downstream logic errors. Consider widening the type used for URL params/store state (e.g. string token symbol) or updating the shared token types to represent dynamic EVM symbols explicitly.
| network: z.string().optional(), | ||
| partnerId: z.string().optional(), | ||
| paymentMethod: z.nativeEnum(EPaymentMethod).optional().catch(undefined), | ||
| paymentMethod: z.string().optional(), | ||
| quoteId: z.string().optional(), | ||
| rampType: z.nativeEnum(RampDirection).optional().catch(undefined), | ||
| rampType: z.string().optional(), | ||
| walletAddressLocked: z.string().optional() |
There was a problem hiding this comment.
Changing paymentMethod and rampType to z.string() removes validation/normalization. This can cause invalid values (e.g. rampType=foo) to be treated as “present” and make hasAllQuoteRefreshParams skip the quote form incorrectly. Keep schema validation but make it case-insensitive (e.g., preprocess to upper/lower then z.nativeEnum(...).optional().catch(undefined)).
| for (const listener of evmTokenListeners) listener(); | ||
| } catch (err) { | ||
| console.error("[DynamicEvmTokens] Failed to fetch tokens from SquidRouter, using fallback:", err); | ||
|
|
||
| state.tokensByNetwork = buildFallbackFromStaticConfig(); | ||
| state.priceBySymbol = buildPriceLookup(state.tokensByNetwork); | ||
| state.isLoaded = true; | ||
| for (const listener of evmTokenListeners) listener(); | ||
| } |
There was a problem hiding this comment.
Calling listeners inside the try block means any exception thrown by a subscriber will be caught by this try/catch and incorrectly trigger the fallback path, overwriting successfully fetched token data. Wrap listener invocations so subscriber errors can’t affect initialization (e.g., notify in a separate try/finally or catch per-listener).
| for (const listener of evmTokenListeners) listener(); | |
| } catch (err) { | |
| console.error("[DynamicEvmTokens] Failed to fetch tokens from SquidRouter, using fallback:", err); | |
| state.tokensByNetwork = buildFallbackFromStaticConfig(); | |
| state.priceBySymbol = buildPriceLookup(state.tokensByNetwork); | |
| state.isLoaded = true; | |
| for (const listener of evmTokenListeners) listener(); | |
| } | |
| } catch (err) { | |
| console.error("[DynamicEvmTokens] Failed to fetch tokens from SquidRouter, using fallback:", err); | |
| state.tokensByNetwork = buildFallbackFromStaticConfig(); | |
| state.priceBySymbol = buildPriceLookup(state.tokensByNetwork); | |
| state.isLoaded = true; | |
| } | |
| for (const listener of evmTokenListeners) { | |
| try { | |
| listener(); | |
| } catch (listenerErr) { | |
| logger.error("[DynamicEvmTokens] Error in EVM token listener", listenerErr); | |
| } | |
| } |
| state.tokensByNetwork = buildFallbackFromStaticConfig(); | ||
| state.priceBySymbol = buildPriceLookup(state.tokensByNetwork); | ||
| state.isLoaded = true; | ||
| for (const listener of evmTokenListeners) listener(); | ||
| } |
There was a problem hiding this comment.
In the fallback path, if any listener throws here it will cause initializeEvmTokens() to reject and prevent other listeners from being notified. Consider isolating notifications (catch per-listener) so one bad subscriber can’t break initialization.
| return { | ||
| apiKey: apiKeyParam || undefined, | ||
| callbackUrl: callbackUrlParam || undefined, | ||
| countryCode: countryCodeParam || undefined, | ||
| cryptoLocked, | ||
| evmTokensLoaded, | ||
| externalSessionId: externalSessionIdParam || undefined, |
There was a problem hiding this comment.
useRampUrlParams is declared to return RampUrlParams, but the returned object includes evmTokensLoaded, which is not part of the RampUrlParams interface. This will fail excess-property checks; either add evmTokensLoaded to the interface (if it’s intentionally part of the public shape) or don’t include it in the returned object and only use it as a dependency to force recomputation.
| try { | ||
| listener(); | ||
| } catch (listenerErr) { | ||
| console.error("[DynamicEvmTokens] Error in EVM token listener", listenerErr); |
There was a problem hiding this comment.
Nice. Just one final nitpick: Let's use our logger instance so that these logs are automatically formatted etc. Please replace the console.error calls with logger.current.error and import the logger instance from logger.ts.
2 participants
We could create a zustand store for storing the tokens data but we need the data also in the
apps/apifor thegetTokenUSDPrice. That's why we use an object to store the data anduseSyncExternalStoreon the frontend to reactively subscribe to it.