add: verify rpcs and fallback for reown/viem #617
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
fetchAndTestRpcslogic mixes chain IDs and named keys:rpcsByChainis initialized withMAINNET_RPC/FUSE_RPC/...but later you assignrpcsByChain[chainId] = validRpcs, which means the named keys never get populated—this should probably usechainMapping[chainIdNum]as the key when storing results. - The
testRpcsuccess checkdata.result !== undefined || data.result !== nullis always true whendata.resultexists (even if null); use an&&or a more explicit type/shape check to avoid accepting invalid responses as healthy RPCs. - The
useEffectinAppKitProvideruses[testedRpcs === null]as its dependency, which will never rerun when the actual RPC list changes; the dependency should betestedRpcsitself (and ideally guard inside the effect) so that AppKit is initialized once the tested RPCs are available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `fetchAndTestRpcs` logic mixes chain IDs and named keys: `rpcsByChain` is initialized with `MAINNET_RPC/FUSE_RPC/...` but later you assign `rpcsByChain[chainId] = validRpcs`, which means the named keys never get populated—this should probably use `chainMapping[chainIdNum]` as the key when storing results.
- The `testRpc` success check `data.result !== undefined || data.result !== null` is always true when `data.result` exists (even if null); use an `&&` or a more explicit type/shape check to avoid accepting invalid responses as healthy RPCs.
- The `useEffect` in `AppKitProvider` uses `[testedRpcs === null]` as its dependency, which will never rerun when the actual RPC list changes; the dependency should be `testedRpcs` itself (and ideally guard inside the effect) so that AppKit is initialized once the tested RPCs are available.
## Individual Comments
### Comment 1
<location> `src/hooks/useWeb3.tsx:69-70` </location>
<code_context>
+
+ if (!response.ok) return false
+
+ const data = await response.json()
+ return !data.error && (data.result !== undefined || data.result !== null)
+ } catch {
+ return false
</code_context>
<issue_to_address>
**issue (bug_risk):** The RPC validity check always returns true when `response.ok` is true due to the `||` condition.
Because `data.result !== undefined || data.result !== null` is always true, any successful response without `data.error` is treated as valid even when `result` is absent. This breaks the RPC validation. Use `&&` instead, or perform a stricter shape check on `data.result`.
</issue_to_address>
### Comment 2
<location> `src/hooks/useWeb3.tsx:77-81` </location>
<code_context>
+}
+
+async function fetchAndTestRpcs(): Promise<Record<string, string[]>> {
+ const rpcsByChain: Record<string, string[]> = {
+ MAINNET_RPC: [],
+ FUSE_RPC: [],
+ CELO_RPC: [],
+ XDC_RPC: [],
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Initial RPC cache keys don’t match the numeric chain-id keys used later, which makes the object shape confusing and potentially error-prone.
You’re initializing `rpcsByChain` with semantic keys (`MAINNET_RPC`, `FUSE_RPC`, etc.), but later you assign values using numeric string keys (`rpcsByChain[chainId] = validRpcs`, where `chainId` is `'1' | '122' | ...`). This mixes two key styles in the same object, while only the numeric keys are actually used (e.g. in `useNetwork`). Consider initializing `rpcsByChain` directly with numeric keys to match its usage and return type, and avoid misleading, unused keys.
Suggested implementation:
```typescript
async function fetchAndTestRpcs(): Promise<Record<string, string[]>> {
// Keep keys numeric to match chainId usage (e.g. rpcsByChain[chainId])
const rpcsByChain: Record<string, string[]> = {}
type RpcCacheEntry = {
```
If any code relies on the semantic keys (e.g. `rpcsByChain.MAINNET_RPC`), it should be updated to use numeric string chain-ids instead (e.g. `rpcsByChain['1']`). Ensure that all assignments use numeric keys consistently: `rpcsByChain[chainId] = validRpcs`.
</issue_to_address>
### Comment 3
<location> `src/hooks/useWeb3.tsx:92-99` </location>
<code_context>
+ const text = await response.text()
+ console.log('[fetchAndTestRpcs] Chainlist fetched, parsing extraRpcs...')
+ // Parse "export const extraRpcs" from the JS file
+ const match = text.match(/export\s+const\s+extraRpcs\s*=\s*(\{[\s\S]*?\n\})/m)
+ if (!match) throw new Error('Could not parse extraRpcs from chainlist')
+
+ // Create a mock privacyStatement object for eval context
+ const privacyStatement = {}
+
+ // Safe evaluation of the RPC object with privacyStatement in scope
+ const extraRpcs = eval(
+ `(function() { const privacyStatement = ${JSON.stringify(privacyStatement)}; return ${match[1]}; })()`
+ )
</code_context>
<issue_to_address>
**🚨 issue (security):** Using `eval` on remote content from GitHub is a security and reliability risk.
This downloads JS from GitHub and `eval`s a substring of it, which is equivalent to executing arbitrary remote code in the client. A repo compromise, MITM, or even a benign upstream change could lead to code execution or breakage. Please replace this with a safer approach (e.g., a JSON endpoint, build-time/server-side processing, or structured parsing) and avoid `eval` on network data entirely.
</issue_to_address>
### Comment 4
<location> `src/hooks/useWeb3.tsx:41` </location>
<code_context>
// 50: { maxFeePerGas: BigNumber.from(12.5e9).toHexString() }, // eip-1559 is only supported on XDC testnet. Last checked 15 november 2025.
}
+const CHAINLIST_URL = 'https://raw.githubusercontent.com/DefiLlama/chainlist/refs/heads/main/constants/extraRpcs.js'
+const RPC_CACHE_KEY = 'GD_RPC_CACHE'
+const CACHE_DURATION_MS = 24 * 60 * 60 * 1000 // 24 hours
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the RPC discovery, testing, and caching logic into a dedicated module and standardizing on chain ID–based types to make `useNetwork` and the provider setup simpler and easier to follow.
You can keep all the new behavior while reducing complexity by isolating the RPC discovery/caching and normalizing types. Two focused refactors would help a lot:
---
### 1. Extract RPC discovery + caching out of `useNetwork`
Move `fetchAndTestRpcs`, `testRpc`, cache helpers, and the singleton into a separate module (e.g. `rpcDiscovery.ts`). The hook then only consumes a simple API and doesn’t know about regex/eval/caching details.
```ts
// rpcDiscovery.ts
const CHAINLIST_URL = 'https://raw.githubusercontent.com/DefiLlama/chainlist/refs/heads/main/constants/extraRpcs.js'
const RPC_CACHE_KEY = 'GD_RPC_CACHE'
const CACHE_DURATION_MS = 24 * 60 * 60 * 1000
let rpcInitializationPromise: Promise<Record<string, string[]>> | null = null
export type TestedRpcsByChainId = Record<string, string[]>
async function testRpc(rpcUrl: string): Promise<boolean> {
// ... existing test logic (moved here) ...
}
function parseExtraRpcs(text: string): any {
const match = text.match(/export\s+const\s+extraRpcs\s*=\s*(\{[\s\S]*?\n\})/m)
if (!match) throw new Error('Could not parse extraRpcs from chainlist')
const privacyStatement = {}
return eval(
`(function() { const privacyStatement = ${JSON.stringify(privacyStatement)}; return ${match[1]}; })()`
)
}
async function fetchAndTestRpcs(): Promise<TestedRpcsByChainId> {
// ... your existing fetch + test logic, but using parseExtraRpcs(...) ...
}
async function getRpcCache(): Promise<TestedRpcsByChainId | null> {
// ... existing cache read logic ...
}
async function setRpcCache(rpcs: TestedRpcsByChainId): Promise<void> {
// ... existing cache write logic ...
}
export async function initializeRpcs(): Promise<TestedRpcsByChainId> {
if (rpcInitializationPromise) return rpcInitializationPromise
rpcInitializationPromise = (async () => {
let cachedRpcs = await getRpcCache()
if (!cachedRpcs) {
cachedRpcs = await fetchAndTestRpcs()
if (Object.values(cachedRpcs).some((arr) => arr.length > 0)) {
await setRpcCache(cachedRpcs)
}
}
return cachedRpcs
})()
return rpcInitializationPromise
}
```
Then `useNetwork` becomes much easier to read and test:
```ts
// useWeb3.tsx
import { initializeRpcs, type TestedRpcsByChainId } from './rpcDiscovery'
type NetworkSettings = {
currentNetwork: string
rpcs: { 1: string; 122: string; 42220: string; 50: string }
testedRpcs: TestedRpcsByChainId | null
}
export function useNetwork(): NetworkSettings {
const [testedRpcs, setTestedRpcs] = React.useState<TestedRpcsByChainId | null>(null)
const celoRpcList = sample(process.env.REACT_APP_CELO_RPC?.split(',')) ?? 'https://forno.celo.org'
const fuseRpcList = sample(process.env.REACT_APP_FUSE_RPC?.split(',')) ?? 'https://rpc.fuse.io'
const xdcRpcList = sample(process.env.REACT_APP_XDC_RPC?.split(',')) ?? 'https://rpc.xinfin.network'
const mainnetList = sample(['https://eth.llamarpc.com', 'https://1rpc.io/eth']) ?? 'https://eth.llamarpc.com'
const [currentNetwork, rpcs] = useMemo(() => {
const selectedRpcs = {
1: sample(testedRpcs?.['1'] || []) || mainnetList,
122: sample(testedRpcs?.['122'] || []) || fuseRpcList,
42220: sample(testedRpcs?.['42220'] || []) || celoRpcList,
50: sample(testedRpcs?.['50'] || []) || xdcRpcList,
}
return [process.env.REACT_APP_NETWORK || 'fuse', selectedRpcs]
}, [testedRpcs, mainnetList, fuseRpcList, celoRpcList, xdcRpcList])
useEffect(() => {
void initializeRpcs().then(setTestedRpcs)
}, [])
useEffect(() => {
AsyncStorage.safeSet('GD_RPCS', rpcs)
}, [rpcs])
return { currentNetwork, rpcs, testedRpcs }
}
```
This keeps all current functionality but clearly separates responsibilities:
- `rpcDiscovery.ts`: parsing, `eval`, testing, caching, singleton.
- `useNetwork`: “select an RPC per chain” and expose it to React.
---
### 2. Normalize key types and naming
Right now you’re mixing chain IDs (`1`, `122`, …) and legacy names (`MAINNET_RPC`, `FUSE_RPC`) in the same flow. Normalizing on chain IDs in the hook and doing a single mapping at the context boundary reduces cognitive overhead:
```ts
// Web3ContextProvider
if (!rpcs) return <></>
const contextRpcs = {
MAINNET_RPC: rpcs['1'],
FUSE_RPC: rpcs['122'],
// If you need them, add CELO_RPC and XDC_RPC here:
// CELO_RPC: rpcs['42220'],
// XDC_RPC: rpcs['50'],
}
return (
<GdSdkContext.Provider value={{ web3, contractsEnv, rpcs: contextRpcs }}>
<Web3Provider
web3Provider={webprovider}
env={contractsEnvV2}
config={{
pollingInterval: 15000,
networks: [Mainnet, Fuse, Celo, Xdc],
readOnlyChainId: undefined,
readOnlyUrls: rpcs, // pure chainId -> url map
}}
>
{children}
</Web3Provider>
</GdSdkContext.Provider>
)
```
Also consider renaming `testifiedRpcs` to `testedRpcs` everywhere (as already reflected above) for clarity; this is a non‑behavioral change that makes the code easier to understand.
</issue_to_address>
### Comment 5
<location> `src/reown/reownprovider.tsx:150` </location>
<code_context>
- featuredWalletIds: [...APPKIT_FEATURED_WALLET_IDS],
-})
-
+let wagmiAdapter: WagmiAdapter = null as any
export function AppKitProvider({ children }: { children: React.ReactNode }) {
+ const [initialized, setInitialized] = React.useState(false)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring AppKitProvider to create a local WagmiAdapter instance from testedRpcs instead of using a global mutable variable and manual initialization state.
You can keep the new behavior while simplifying the lifecycle and removing mutation / global state.
### 1. Make `wagmiAdapter` local React state (no global `let` + `initialized`)
```ts
export function AppKitProvider({ children }: { children: React.ReactNode }) {
const [wagmiAdapter, setWagmiAdapter] = React.useState<WagmiAdapter | null>(null)
const { testedRpcs } = useNetwork()
useEffect(() => {
if (!testedRpcs) return
const configuredNetworks = networks.map(network => {
const rpcUrls = testedRpcs[network.id]
if (!rpcUrls) return network
console.log(`Reown: Updated RPC for ${network.name} to ${rpcUrls}`)
return {
...network,
rpcUrls: {
...network.rpcUrls,
default: {
...network.rpcUrls?.default,
http: rpcUrls,
},
},
}
})
const transports = Object.fromEntries(
Object.entries(testedRpcs).map(([id, rpcs]) => [
Number(id),
fallback(rpcs.map(url => http(url))),
]),
)
const adapter = new WagmiAdapter({
networks: configuredNetworks,
projectId,
ssr: true,
connectors,
transports,
})
console.log(`wagmiAdapter.wagmiConfig:`, adapter.wagmiConfig)
createAppKit({
adapters: [adapter],
networks: configuredNetworks,
projectId,
metadata,
features: {
analytics: true,
socials: APPKIT_SOCIAL_PROVIDER_IDS as any,
},
featuredWalletIds: [...APPKIT_FEATURED_WALLET_IDS],
})
setWagmiAdapter(adapter)
}, [testedRpcs]) // depend on the actual value, not `testedRpcs === null`
if (!wagmiAdapter) {
return null
}
return (
<WagmiProvider config={wagmiAdapter.wagmiConfig}>
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
</WagmiProvider>
)
}
```
**Benefits:**
- Removes global mutable `wagmiAdapter` and `initialized` state; the component is self‑contained.
- Uses `testedRpcs` directly as the effect dependency (no `testedRpcs === null` trick).
- Avoids mutating the imported `networks` via `lodash.set`; builds a new `configuredNetworks` array instead.
- Clearly separates:
- `configuredNetworks` (used by `WagmiAdapter` + `createAppKit`)
- `transports` (derived from the same `testedRpcs`), making the configuration path explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+77
to
+81
| const rpcsByChain: Record<string, string[]> = { | ||
| MAINNET_RPC: [], | ||
| FUSE_RPC: [], | ||
| CELO_RPC: [], | ||
| XDC_RPC: [], |
There was a problem hiding this comment.
suggestion: Initial RPC cache keys don’t match the numeric chain-id keys used later, which makes the object shape confusing and potentially error-prone.
You’re initializing rpcsByChain with semantic keys (MAINNET_RPC, FUSE_RPC, etc.), but later you assign values using numeric string keys (rpcsByChain[chainId] = validRpcs, where chainId is '1' | '122' | ...). This mixes two key styles in the same object, while only the numeric keys are actually used (e.g. in useNetwork). Consider initializing rpcsByChain directly with numeric keys to match its usage and return type, and avoid misleading, unused keys.
Suggested implementation:
async function fetchAndTestRpcs(): Promise<Record<string, string[]>> {
// Keep keys numeric to match chainId usage (e.g. rpcsByChain[chainId])
const rpcsByChain: Record<string, string[]> = {}
type RpcCacheEntry = {If any code relies on the semantic keys (e.g. rpcsByChain.MAINNET_RPC), it should be updated to use numeric string chain-ids instead (e.g. rpcsByChain['1']). Ensure that all assignments use numeric keys consistently: rpcsByChain[chainId] = validRpcs.
Comment on lines
+92
to
+99
| const match = text.match(/export\s+const\s+extraRpcs\s*=\s*(\{[\s\S]*?\n\})/m) | ||
| if (!match) throw new Error('Could not parse extraRpcs from chainlist') | ||
|
|
||
| // Create a mock privacyStatement object for eval context | ||
| const privacyStatement = {} | ||
|
|
||
| // Safe evaluation of the RPC object with privacyStatement in scope | ||
| const extraRpcs = eval( |
There was a problem hiding this comment.
🚨 issue (security): Using eval on remote content from GitHub is a security and reliability risk.
This downloads JS from GitHub and evals a substring of it, which is equivalent to executing arbitrary remote code in the client. A repo compromise, MITM, or even a benign upstream change could lead to code execution or breakage. Please replace this with a safer approach (e.g., a JSON endpoint, build-time/server-side processing, or structured parsing) and avoid eval on network data entirely.
| featuredWalletIds: [...APPKIT_FEATURED_WALLET_IDS], | ||
| }) | ||
|
|
||
| let wagmiAdapter: WagmiAdapter = null as any |
There was a problem hiding this comment.
issue (complexity): Consider refactoring AppKitProvider to create a local WagmiAdapter instance from testedRpcs instead of using a global mutable variable and manual initialization state.
You can keep the new behavior while simplifying the lifecycle and removing mutation / global state.
1. Make wagmiAdapter local React state (no global let + initialized)
export function AppKitProvider({ children }: { children: React.ReactNode }) {
const [wagmiAdapter, setWagmiAdapter] = React.useState<WagmiAdapter | null>(null)
const { testedRpcs } = useNetwork()
useEffect(() => {
if (!testedRpcs) return
const configuredNetworks = networks.map(network => {
const rpcUrls = testedRpcs[network.id]
if (!rpcUrls) return network
console.log(`Reown: Updated RPC for ${network.name} to ${rpcUrls}`)
return {
...network,
rpcUrls: {
...network.rpcUrls,
default: {
...network.rpcUrls?.default,
http: rpcUrls,
},
},
}
})
const transports = Object.fromEntries(
Object.entries(testedRpcs).map(([id, rpcs]) => [
Number(id),
fallback(rpcs.map(url => http(url))),
]),
)
const adapter = new WagmiAdapter({
networks: configuredNetworks,
projectId,
ssr: true,
connectors,
transports,
})
console.log(`wagmiAdapter.wagmiConfig:`, adapter.wagmiConfig)
createAppKit({
adapters: [adapter],
networks: configuredNetworks,
projectId,
metadata,
features: {
analytics: true,
socials: APPKIT_SOCIAL_PROVIDER_IDS as any,
},
featuredWalletIds: [...APPKIT_FEATURED_WALLET_IDS],
})
setWagmiAdapter(adapter)
}, [testedRpcs]) // depend on the actual value, not `testedRpcs === null`
if (!wagmiAdapter) {
return null
}
return (
<WagmiProvider config={wagmiAdapter.wagmiConfig}>
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
</WagmiProvider>
)
}Benefits:
- Removes global mutable
wagmiAdapterandinitializedstate; the component is self‑contained. - Uses
testedRpcsdirectly as the effect dependency (notestedRpcs === nulltrick). - Avoids mutating the imported
networksvialodash.set; builds a newconfiguredNetworksarray instead. - Clearly separates:
configuredNetworks(used byWagmiAdapter+createAppKit)transports(derived from the sametestedRpcs), making the configuration path explicit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description