refactor: use useSWR in useUpdateUsdcBalances #2164
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| import { useBalances } from '../useBalances' | ||
| import { getProviderForChainId } from '@/token-bridge-sdk/utils' | ||
|
|
||
| export async function childChainUsdcAddressFetcher([ |
There was a problem hiding this comment.
Could we do something like this, also renaming the method? I see that it's easier to pass directly from SWR with an array, but it's just harder to read when calling the method.
getChildUsdcAddress({
parentUsdcAddress,
parentChainId,
childChainId
)}nit: I don't think we need to usd chain in variable names, it's implied. We've been using it without chain on the bridge already and everywhere in the sdk. Might wanna follow the same structure.
| }) | ||
| } | ||
|
|
||
| export function useParentChainUsdcAddress() { |
There was a problem hiding this comment.
IF you decide to go with variable names without 'chain', can rename this to useParentUsdcAddress. Otherwise keep consistent.
| export function useUpdateUsdcBalances({ | ||
| walletAddress | ||
| }: { | ||
| walletAddress: string | undefined |
There was a problem hiding this comment.
nit
| walletAddress: string | undefined | |
| walletAddress: Address | undefined |
| getL2ERC20Address: jest.fn() | ||
| })) | ||
|
|
||
| const xaiTestnetChainId = 37714555429 |
There was a problem hiding this comment.
nit: you could cast all of these as ChainId here and won't have to do it every time later
| ]) | ||
|
|
||
| expect(result).toEqual(CommonAddress.ArbitrumOne.USDC) | ||
| expect(result).not.toEqual(CommonAddress.ArbitrumOne['USDC.e']) |
There was a problem hiding this comment.
nit: we don't need to check for USDC.e, if USDC passed then USDC.e must fail (unless we change ArbitrumOne['USDC.e'] to the same value)
| import { getProviderForChainId } from '@/token-bridge-sdk/utils' | ||
|
|
||
| export async function childChainUsdcAddressFetcher([ | ||
| _parentChainUsdcAddress, |
There was a problem hiding this comment.
I wonder if we can do something about _parentChainUsdcAddress, it should not be required for non-Orbit chains, so we should be able to just call the method without specifying parent USDC address and rely on chain ID only.
Maybe we don't need useParentChainUsdcAddress, we could just make it a util method and use it here as well?
| const [networks] = useNetworks() | ||
| const { parentChain } = useNetworksRelationship(networks) | ||
|
|
||
| return useMemo(() => { |
There was a problem hiding this comment.
It's a string, We don't need useMemo here
| // we don't have native USDC addresses for Orbit chains, we need to fetch it | ||
| const { | ||
| data: childChainUsdcAddress, | ||
| error, // can be unbridged to Orbit chain so no address to be found |
There was a problem hiding this comment.
We never return this error
| updateUsdcBalances() | ||
| return | ||
| } | ||
|
|
||
| latestTokenBridge?.current?.token?.updateTokenData(selectedToken.address) |
There was a problem hiding this comment.
Just thinking here, can we maybe call updateUSDCBalances in updateTokenData directly?
There was a problem hiding this comment.
supposedly, i think it requires some exploration and so i haven't created any tickets for it or dealt with it on this PR as it's out of scope
| const _walletAddress: Address | undefined = | ||
| walletAddress && isAddress(walletAddress) ? walletAddress : undefined |
There was a problem hiding this comment.
NIT: I don't think we need to type it here?
Also, can we reuse walletAddress directly? Can we have invalid address here?
| jest.mock('../useNetworks', () => ({ | ||
| useNetworks: jest.fn() | ||
| })) |
There was a problem hiding this comment.
If we need to mock useNetworks maybe it's a sign we can pass networks as a parameter to useUpdateUsdcBalances?
There was a problem hiding this comment.
I’d rather just mock it in the tests and call useNetworks in useUpdateUsdcBalances
I don’t think we should design the function to facilitate test writing
| const xaiTestnet = orbitTestnets[xaiTestnetChainId] | ||
|
|
||
| if (!xaiTestnet) { | ||
| throw new Error(`Could not find Xai Testnet in the Orbit chains list.`) | ||
| } | ||
|
|
||
| registerCustomArbitrumNetwork(xaiTestnet) | ||
|
|
||
| const polterTestnet = orbitTestnets[polterTestnetChainId] | ||
|
|
||
| if (!polterTestnet) { | ||
| throw new Error(`Could not find Polter Testnet in the Orbit chains list.`) | ||
| } | ||
|
|
||
| registerCustomArbitrumNetwork(polterTestnet) | ||
|
|
||
| const geistMainnet = orbitMainnets[geistMainnetChainId] | ||
|
|
||
| if (!geistMainnet) { | ||
| throw new Error(`Could not find Geist Mainnet in the Orbit chains list.`) | ||
| } | ||
|
|
||
| registerCustomArbitrumNetwork(geistMainnet) |
There was a problem hiding this comment.
Maybe we can create an helper for that? We have the same pattern in isDepositMode and networks test
| @@ -4,6 +4,7 @@ import { twMerge } from 'tailwind-merge' | |||
| import { utils } from 'ethers' | |||
| import { Chain, useAccount } from 'wagmi' | |||
| import { useMedia } from 'react-use' | |||
| import { isAddress } from 'ethers/lib/utils.js' | |||
There was a problem hiding this comment.
| import { isAddress } from 'ethers/lib/utils.js' | |
| import { isAddress } from 'ethers/lib/utils |
We don't need to fetch the USDC address on the child chain every time we want to update the USDC balances because it will never change
Closes FS-1082