Rpc fix with json#643
Conversation
…d add RPC fallbacks (#638) * Initial plan * test: cover chainlist rpc parsing and fallback helpers Agent-Logs-Url: https://github.com/GoodDollar/GoodProtocolUI/sessions/cd79a809-ceb1-4685-8196-2855b4e01243 * fix: harden rpc parser and remove eval usage Agent-Logs-Url: https://github.com/GoodDollar/GoodProtocolUI/sessions/cd79a809-ceb1-4685-8196-2855b4e01243 * fix: simplify fetchAndTestRpcs to use chainid.network JSON, add promise reset on failure * fix: multicall fails for native balance on ethereum mainnet * Add Jest CI workflow for testing * Update package manager to pnpm in jest.yml * Update Jest workflow for package management and test command * Update Jest command to use pnpx instead of pnpm * Fix testPathPattern to testPathPatterns in jest.yml * Update Jest test command to use local binary * Change Jest test command to use pnpm * Update Jest workflow to use pnpm package manager * ci: add jest config and deps for yarn-based test workflow * ci: add Jest workflow using yarn for rpcParsing tests on PRs to master * - replace usage of legacy yarn flag (frozen-lockfile > immutable) - extend test with fallback test case * chore: lingui * fix: formatUnits for native balance fallback, chore: lingui --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Gutopro <nickigber@gmail.com> Co-authored-by: LewisB <laurence@gooddollar.org> Co-authored-by: Nicholas Igber <111025771+Gutopro@users.noreply.github.com>
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
useNetwork, the fallback selection for each chain defaults to an empty string when neithertestifiedRpcsnorfallbackRpcsByChainhave entries; consider avoiding empty strings here and either omitting the key or falling back to a known-good URL to prevent passing an invalid RPC URL downstream. FALLBACK_RPCS_BY_CHAINis exported fromrpcParsing.tsbut not used anywhere in the diff; consider removing this export (or wiring it up where intended) to avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useNetwork`, the fallback selection for each chain defaults to an empty string when neither `testifiedRpcs` nor `fallbackRpcsByChain` have entries; consider avoiding empty strings here and either omitting the key or falling back to a known-good URL to prevent passing an invalid RPC URL downstream.
- `FALLBACK_RPCS_BY_CHAIN` is exported from `rpcParsing.ts` but not used anywhere in the diff; consider removing this export (or wiring it up where intended) to avoid dead code.
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="79-80" />
<code_context>
- 50: 'XDC_RPC',
+ const extraRpcs = await fetchRpcsFromChainlist().catch(() => fallbackRpcsByChain)
+
+ for (const chainId of SUPPORTED_CHAIN_IDS) {
+ const chainRpcs = extraRpcs[chainId] || []
+ const testResults = await Promise.all(
+ chainRpcs.slice(0, 10).map(async (rpcUrl) => ({
</code_context>
<issue_to_address>
**issue:** Avoid falling back to an empty string as an RPC URL when no RPCs are available
In `useNetwork`, `selectedRpcs` uses `sample(fallbackRpcsByChain[chainId]) || ''`, so if both `testifiedRpcs` and `fallbackRpcsByChain[chainId]` are empty, the RPC URL becomes `''`, which can flow into web3 providers as an invalid URL. Instead, consider omitting the key when no RPC is available, using a hard-coded last-resort default, or surfacing an explicit error state rather than defaulting to an empty string.
</issue_to_address>
### Comment 2
<location path="src/components/Web3Status/index.tsx" line_range="29" />
<code_context>
const sendData = useSendAnalyticsData()
const { address } = useAppKitAccount()
const { chainId } = useAppKitNetwork()
+ const { library } = useEthers() as any
const { ENSName } = useENSName(address ?? undefined)
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the balance timeout/fallback logic into a dedicated hook so Web3StatusInner stays mostly presentational and simpler to reason about.
You can keep the new fallback behavior but move the imperative logic out of `Web3StatusInner` into a small dedicated hook. That will reduce responsibilities (and tests) for `Web3StatusInner` without changing functionality.
### 1. Extract balance + timeout + fallback into a hook
```ts
// hooks/useNativeBalanceWithFallback.ts
import { useEffect, useState } from 'react'
import { useNativeBalance } from '@gooddollar/web3sdk-v2'
import { useEthers } from '@usedapp/core'
import { formatUnits } from 'viem'
export function useNativeBalanceWithFallback(
address: string | undefined,
chainId: number | undefined,
{ timeoutMs = 500 } = {}
) {
const { library } = useEthers() as any
const nativeBalance = useNativeBalance()
const [fallbackBalance, setFallbackBalance] = useState<string | undefined>()
const [isReady, setIsReady] = useState(false)
useEffect(() => {
let cancelled = false
setIsReady(false)
setFallbackBalance(undefined)
const timeoutId = window.setTimeout(() => {
if (!cancelled) setIsReady(true)
}, timeoutMs)
async function readBalance() {
if (!address || !library?.getBalance) return
try {
const balance = await library.getBalance(address)
if (!cancelled) {
// NOTE: still using 18 here to preserve current behavior
setFallbackBalance(formatUnits(balance.toString(), 18))
}
} catch {
if (!cancelled) setFallbackBalance(undefined)
}
}
void readBalance()
return () => {
cancelled = true
window.clearTimeout(timeoutId)
}
}, [address, library, chainId, timeoutMs])
const balance = nativeBalance ?? fallbackBalance
const isLoading = !isReady || !balance
return { balance, isLoading }
}
```
### 2. Simplify `Web3StatusInner` to use the hook
```tsx
import { useNativeBalanceWithFallback } from '../../hooks/useNativeBalanceWithFallback'
import { Spinner } from 'native-base'
import { Currency } from '@sushiswap/sdk'
import { useAppKitAccount, useAppKitNetwork } from '@reown/appkit/react'
function Web3StatusInner() {
const { i18n } = useLingui()
const sendData = useSendAnalyticsData()
const { address } = useAppKitAccount()
const { chainId } = useAppKitNetwork()
const { balance, isLoading } = useNativeBalanceWithFallback(
address,
chainId ? +chainId : undefined,
{ timeoutMs: 500 }
)
// ... transactions, pending, etc. unchanged ...
return (
<HStack space={8} flexDirection="row">
{address && (
<div className="flex flex-row gap-4">
{isLoading ? (
<Spinner size="sm" color="gdPrimary" />
) : (
balance && (
<Text
fontSize="sm"
fontFamily="subheading"
fontWeight="normal"
color="gdPrimary"
>
{parseFloat(balance).toFixed(4)}{' '}
{Currency.getNativeCurrencySymbol(+(chainId ?? 1))}
</Text>
)
)}
{/* pending tx UI unchanged */}
</div>
)}
</HStack>
)
}
```
This keeps all existing behavior (timeout, fallback to `library.getBalance`, merging both sources, spinner vs value) but makes `Web3StatusInner` mostly presentational again and isolates the imperative logic into a focused, testable hook.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for (const chainId of SUPPORTED_CHAIN_IDS) { | ||
| const chainRpcs = extraRpcs[chainId] || [] |
There was a problem hiding this comment.
issue: Avoid falling back to an empty string as an RPC URL when no RPCs are available
In useNetwork, selectedRpcs uses sample(fallbackRpcsByChain[chainId]) || '', so if both testifiedRpcs and fallbackRpcsByChain[chainId] are empty, the RPC URL becomes '', which can flow into web3 providers as an invalid URL. Instead, consider omitting the key when no RPC is available, using a hard-coded last-resort default, or surfacing an explicit error state rather than defaulting to an empty string.
| const sendData = useSendAnalyticsData() | ||
| const { address } = useAppKitAccount() | ||
| const { chainId } = useAppKitNetwork() | ||
| const { library } = useEthers() as any |
There was a problem hiding this comment.
issue (complexity): Consider extracting the balance timeout/fallback logic into a dedicated hook so Web3StatusInner stays mostly presentational and simpler to reason about.
You can keep the new fallback behavior but move the imperative logic out of Web3StatusInner into a small dedicated hook. That will reduce responsibilities (and tests) for Web3StatusInner without changing functionality.
1. Extract balance + timeout + fallback into a hook
// hooks/useNativeBalanceWithFallback.ts
import { useEffect, useState } from 'react'
import { useNativeBalance } from '@gooddollar/web3sdk-v2'
import { useEthers } from '@usedapp/core'
import { formatUnits } from 'viem'
export function useNativeBalanceWithFallback(
address: string | undefined,
chainId: number | undefined,
{ timeoutMs = 500 } = {}
) {
const { library } = useEthers() as any
const nativeBalance = useNativeBalance()
const [fallbackBalance, setFallbackBalance] = useState<string | undefined>()
const [isReady, setIsReady] = useState(false)
useEffect(() => {
let cancelled = false
setIsReady(false)
setFallbackBalance(undefined)
const timeoutId = window.setTimeout(() => {
if (!cancelled) setIsReady(true)
}, timeoutMs)
async function readBalance() {
if (!address || !library?.getBalance) return
try {
const balance = await library.getBalance(address)
if (!cancelled) {
// NOTE: still using 18 here to preserve current behavior
setFallbackBalance(formatUnits(balance.toString(), 18))
}
} catch {
if (!cancelled) setFallbackBalance(undefined)
}
}
void readBalance()
return () => {
cancelled = true
window.clearTimeout(timeoutId)
}
}, [address, library, chainId, timeoutMs])
const balance = nativeBalance ?? fallbackBalance
const isLoading = !isReady || !balance
return { balance, isLoading }
}2. Simplify Web3StatusInner to use the hook
import { useNativeBalanceWithFallback } from '../../hooks/useNativeBalanceWithFallback'
import { Spinner } from 'native-base'
import { Currency } from '@sushiswap/sdk'
import { useAppKitAccount, useAppKitNetwork } from '@reown/appkit/react'
function Web3StatusInner() {
const { i18n } = useLingui()
const sendData = useSendAnalyticsData()
const { address } = useAppKitAccount()
const { chainId } = useAppKitNetwork()
const { balance, isLoading } = useNativeBalanceWithFallback(
address,
chainId ? +chainId : undefined,
{ timeoutMs: 500 }
)
// ... transactions, pending, etc. unchanged ...
return (
<HStack space={8} flexDirection="row">
{address && (
<div className="flex flex-row gap-4">
{isLoading ? (
<Spinner size="sm" color="gdPrimary" />
) : (
balance && (
<Text
fontSize="sm"
fontFamily="subheading"
fontWeight="normal"
color="gdPrimary"
>
{parseFloat(balance).toFixed(4)}{' '}
{Currency.getNativeCurrencySymbol(+(chainId ?? 1))}
</Text>
)
)}
{/* pending tx UI unchanged */}
</div>
)}
</HStack>
)
}This keeps all existing behavior (timeout, fallback to library.getBalance, merging both sources, spinner vs value) but makes Web3StatusInner mostly presentational again and isolates the imperative logic into a focused, testable hook.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15646eead8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) : ( | ||
| <Spinner size="sm" color="gdPrimary" /> |
There was a problem hiding this comment.
Stop showing an endless balance spinner
When a connected account has no readable native balance (for example useNativeBalance() stays undefined and library.getBalance is unavailable or the RPC call fails), showBalance flips to true after the timeout but shouldShowBalance is still false, so this branch renders a spinner forever. Before this change the header simply omitted the balance and still showed the address; with the new fallback path, affected users are left with a permanent loading indicator even after the timeout has elapsed.
Useful? React with 👍 / 👎.
Deploying goodprotocolui with
|
| Latest commit: |
15646ee
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a6a8495c.goodprotocolui.pages.dev |
| Branch Preview URL: | https://rpc-fix-with-json.goodprotocolui.pages.dev |
Description
Same fixes as this PR: #642
but then replaced by using the chainlist rpc.json endpoint