Rpc fix clean pr#642
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>
![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 2 issues, and left some high level feedback:
- In Web3StatusInner,
useEthersis cast toanyandChainOptionprops are typed asany; consider tightening these types (e.g., defining explicit interfaces for the hook return and ChainOption props) to catch integration issues at compile time. - The Jest-related dependencies mix major versions (
jest29 withjest-environment-jsdom30.4.1); aligning them to the same major version will avoid subtle runtime/test-environment inconsistencies. - In the native balance fallback logic,
parseFloat(displayNativeBalance!)assumes a valid numeric string; adding a simple guard (e.g., checking!isNaN(Number(displayNativeBalance))) would make the rendering more robust against unexpected values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In Web3StatusInner, `useEthers` is cast to `any` and `ChainOption` props are typed as `any`; consider tightening these types (e.g., defining explicit interfaces for the hook return and ChainOption props) to catch integration issues at compile time.
- The Jest-related dependencies mix major versions (`jest` 29 with `jest-environment-jsdom` 30.4.1); aligning them to the same major version will avoid subtle runtime/test-environment inconsistencies.
- In the native balance fallback logic, `parseFloat(displayNativeBalance!)` assumes a valid numeric string; adding a simple guard (e.g., checking `!isNaN(Number(displayNativeBalance))`) would make the rendering more robust against unexpected values.
## Individual Comments
### Comment 1
<location path="package.json" line_range="120-121" />
<code_context>
"graphql": "^15.5.1",
"husky": "^8.0.2",
"jazzicon": "^1.5.0",
+ "jest": "29",
+ "jest-environment-jsdom": "^30.4.1",
"jotai-immer": "^0.2.0",
"jsbi": "^3.1.5",
</code_context>
<issue_to_address>
**issue (bug_risk):** The Jest core version and `jest-environment-jsdom` major versions are out of sync.
`jest` is pinned to 29 while `jest-environment-jsdom` is ^30.4.1. Jest environments are expected to match Jest’s major version, so this mismatch can cause subtle test failures or runtime issues. Please either use a 29.x `jest-environment-jsdom` or upgrade Jest to 30.x to keep the majors aligned.
</issue_to_address>
### Comment 2
<location path="src/components/Web3Status/index.tsx" line_range="40" />
<code_context>
+
+ // added a timed-out fallback for when native-balance does not seem to complete (happening on mainnet)
+ // it uses a timeout to avoid mis-formatted amounts based on old-chain decimals.
+ useEffect(() => {
+ let cancelled = false
+ setShowBalance(false)
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new balance timeout/fallback logic into a dedicated hook so Web3StatusInner is responsible only for rendering and not side‑effect orchestration.
You can keep the new behavior while reducing `Web3StatusInner`’s responsibilities by extracting the fallback logic into a dedicated hook. That moves the timeout / cancellation / formatting concerns out of the view layer and keeps this component focused on rendering.
### 1. Extract a `useNativeBalanceWithFallback` hook
Move the effect, extra state, and merging logic 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, chainId?: number) {
const { library } = useEthers() as any
const nativeBalance = useNativeBalance()
const [directNativeBalance, setDirectNativeBalance] = useState<string>()
const [showBalance, setShowBalance] = useState(false)
useEffect(() => {
let cancelled = false
setShowBalance(false)
setDirectNativeBalance(undefined)
const timeoutId = window.setTimeout(() => {
if (!cancelled) setShowBalance(true)
}, 500)
async function readBalance() {
if (!address || !library?.getBalance) return
try {
const balance = await library.getBalance(address)
if (!cancelled) {
setDirectNativeBalance(formatUnits(balance.toString(), 18))
}
} catch {
if (!cancelled) {
setDirectNativeBalance(undefined)
}
}
}
void readBalance()
return () => {
cancelled = true
window.clearTimeout(timeoutId)
}
}, [address, library, chainId])
const balance = nativeBalance ?? directNativeBalance
const loading = !showBalance || !balance
return { balance, loading }
}
```
### 2. Simplify `Web3StatusInner` to consume the hook
`Web3StatusInner` no longer needs the effect/state/spinner logic:
```tsx
import { Spinner, Text, HStack } from 'native-base'
import { Currency } from '@sushiswap/sdk'
import { useAppKitAccount, useAppKitNetwork } from '@reown/appkit/react'
import { useNativeBalanceWithFallback } from '../../hooks/useNativeBalanceWithFallback'
function Web3StatusInner() {
const { i18n } = useLingui()
const sendData = useSendAnalyticsData()
const { address } = useAppKitAccount()
const { chainId } = useAppKitNetwork()
const { balance: displayNativeBalance, loading: balanceLoading } =
useNativeBalanceWithFallback(address ?? undefined, chainId ?? 1)
// ...transactions logic unchanged...
return (
<HStack space={8} flexDirection="row">
{address && (
<div className="flex flex-row gap-4">
{balanceLoading ? (
<Spinner size="sm" color="gdPrimary" />
) : (
displayNativeBalance && (
<Text fontSize="sm" fontFamily="subheading" fontWeight="normal" color="gdPrimary">
{parseFloat(displayNativeBalance).toFixed(4)}{' '}
{Currency.getNativeCurrencySymbol(+(chainId ?? 1))}
</Text>
)
)}
{/* pending tx / account click UI unchanged */}
</div>
)}
</HStack>
)
}
```
This keeps all existing behavior (including the timeout, fallback `getBalance`, and spinner) but:
- Removes the multi-purpose `useEffect` from the component.
- Reduces local state in `Web3StatusInner`.
- Encapsulates the timing/cancellation concerns in a single, 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.
| "jest": "29", | ||
| "jest-environment-jsdom": "^30.4.1", |
There was a problem hiding this comment.
issue (bug_risk): The Jest core version and jest-environment-jsdom major versions are out of sync.
jest is pinned to 29 while jest-environment-jsdom is ^30.4.1. Jest environments are expected to match Jest’s major version, so this mismatch can cause subtle test failures or runtime issues. Please either use a 29.x jest-environment-jsdom or upgrade Jest to 30.x to keep the majors aligned.
|
|
||
| // added a timed-out fallback for when native-balance does not seem to complete (happening on mainnet) | ||
| // it uses a timeout to avoid mis-formatted amounts based on old-chain decimals. | ||
| useEffect(() => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the new balance timeout/fallback logic into a dedicated hook so Web3StatusInner is responsible only for rendering and not side‑effect orchestration.
You can keep the new behavior while reducing Web3StatusInner’s responsibilities by extracting the fallback logic into a dedicated hook. That moves the timeout / cancellation / formatting concerns out of the view layer and keeps this component focused on rendering.
1. Extract a useNativeBalanceWithFallback hook
Move the effect, extra state, and merging logic 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, chainId?: number) {
const { library } = useEthers() as any
const nativeBalance = useNativeBalance()
const [directNativeBalance, setDirectNativeBalance] = useState<string>()
const [showBalance, setShowBalance] = useState(false)
useEffect(() => {
let cancelled = false
setShowBalance(false)
setDirectNativeBalance(undefined)
const timeoutId = window.setTimeout(() => {
if (!cancelled) setShowBalance(true)
}, 500)
async function readBalance() {
if (!address || !library?.getBalance) return
try {
const balance = await library.getBalance(address)
if (!cancelled) {
setDirectNativeBalance(formatUnits(balance.toString(), 18))
}
} catch {
if (!cancelled) {
setDirectNativeBalance(undefined)
}
}
}
void readBalance()
return () => {
cancelled = true
window.clearTimeout(timeoutId)
}
}, [address, library, chainId])
const balance = nativeBalance ?? directNativeBalance
const loading = !showBalance || !balance
return { balance, loading }
}2. Simplify Web3StatusInner to consume the hook
Web3StatusInner no longer needs the effect/state/spinner logic:
import { Spinner, Text, HStack } from 'native-base'
import { Currency } from '@sushiswap/sdk'
import { useAppKitAccount, useAppKitNetwork } from '@reown/appkit/react'
import { useNativeBalanceWithFallback } from '../../hooks/useNativeBalanceWithFallback'
function Web3StatusInner() {
const { i18n } = useLingui()
const sendData = useSendAnalyticsData()
const { address } = useAppKitAccount()
const { chainId } = useAppKitNetwork()
const { balance: displayNativeBalance, loading: balanceLoading } =
useNativeBalanceWithFallback(address ?? undefined, chainId ?? 1)
// ...transactions logic unchanged...
return (
<HStack space={8} flexDirection="row">
{address && (
<div className="flex flex-row gap-4">
{balanceLoading ? (
<Spinner size="sm" color="gdPrimary" />
) : (
displayNativeBalance && (
<Text fontSize="sm" fontFamily="subheading" fontWeight="normal" color="gdPrimary">
{parseFloat(displayNativeBalance).toFixed(4)}{' '}
{Currency.getNativeCurrencySymbol(+(chainId ?? 1))}
</Text>
)
)}
{/* pending tx / account click UI unchanged */}
</div>
)}
</HStack>
)
}This keeps all existing behavior (including the timeout, fallback getBalance, and spinner) but:
- Removes the multi-purpose
useEffectfrom the component. - Reduces local state in
Web3StatusInner. - Encapsulates the timing/cancellation concerns in a single, testable hook.
There was a problem hiding this comment.
what are these balance additions?
There was a problem hiding this comment.
Those additions are a fallback path for the native balance display, plus a small loading-state tweak.
Concretely in Web3StatusInner:
const nativeBalance = useNativeBalance()– the original hook-based balance, which sometimes stalls / never resolves on mainnet in your current setup.directNativeBalancestate and theuseEffect– a directlibrary.getBalance(address)call viauseEthers()that kicks in if the hook is slow or stuck.showBalance+ timeout – you show a spinner for ~500ms first. After that:- if
useNativeBalance()has given a value, you use it - otherwise you fall back to the
getBalanceresult
- if
The net behavior:
- Users no longer get a “forever spinner” when
useNativeBalance()doesn’t resolve on mainnet. - You avoid rendering a balance immediately on chain change (before decimals / RPC are correct) because you wait for the timeout + fresh read before showing it.
My review comment wasn’t suggesting changing that behavior, just moving this logic into a small hook (e.g. useNativeBalanceWithFallback) so the component stays more focused on rendering and less on side‑effects.
If you prefer to keep it inline here, the current logic is fine functionally; the suggestion is purely for readability/testability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88102f68bf
ℹ️ 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".
| "jest": "29", | ||
| "jest-environment-jsdom": "^30.4.1", |
There was a problem hiding this comment.
Adding Jest 29 plus jest-environment-jsdom 30 makes the existing CI jobs in .github/workflows/tests.yaml incompatible with their configured node-version: '12' (both the integration and unit-test jobs install devDependencies before building/testing). The new jest.yml uses Node 20, but these older workflows still run on every push/PR to master, so yarn install/yarn test there will fail until those jobs are upgraded or the Jest packages are pinned to versions that support Node 12.
Useful? React with 👍 / 👎.
Deploying goodprotocolui with
|
| Latest commit: |
5241add
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7d4f791a.goodprotocolui.pages.dev |
| Branch Preview URL: | https://rpc-fix-clean-pr.goodprotocolui.pages.dev |
sirpy
left a comment
sirpy
left a comment
There was a problem hiding this comment.
the core fix is wrong. we need to use chainlist not chainid
| run: yarn install --immutable | ||
|
|
||
| - name: Run Jest tests | ||
| run: yarn test --testPathPattern="rpcParsing" --testTimeout=30000 |
There was a problem hiding this comment.
if you want all tests we should work on them in a separate PR as there is a lot of legacy tests that have not been maintained failing and would block this PR from merging and I thought we want to have this fix for rpc handling to resolve.
| import { useLingui } from '@lingui/react' | ||
| import { SupportedChains, useSwitchNetwork } from '@gooddollar/web3sdk-v2' | ||
| import { Text, Link } from 'native-base' | ||
| import { Text, Link, Spinner, HStack } from 'native-base' |
There was a problem hiding this comment.
what are these file changes why are they in this pr?
There was a problem hiding this comment.
I was testing if the RPCs were working and noticed a bug specifically on Ethereum, so I pushed a fix. thought it minor enough that it did not matter so much
|
|
||
| // added a timed-out fallback for when native-balance does not seem to complete (happening on mainnet) | ||
| // it uses a timeout to avoid mis-formatted amounts based on old-chain decimals. | ||
| useEffect(() => { |
There was a problem hiding this comment.
what are these balance additions?
There was a problem hiding this comment.
The whole thing is wrong.
chainid.network doesnt include the latest rpcs.
need to revert to parsing chainlist.js file
|
Description
Includes all changes from pull-request: #638
but with a proper formatted package.json to surface the exact changes applied
Summary by Sourcery
Improve RPC handling, balance display, and network switching UX while adding Jest-based testing infrastructure for RPC parsing.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: