-
Notifications
You must be signed in to change notification settings - Fork 45
fix: user pool balances not loading correctly #1763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| } | ||
|
|
||
| /** Hook to fetch balances for multiple tokens */ | ||
| export function useTokenBalances( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I let Claude vibe code this by looking at useTokenUsdRates from token-usd-rates.ts 🙈
| export function useTokenBalance({ chainId, userAddress, tokenAddress }: FieldsOf<TokenBalanceQuery>) { | ||
| export function useTokenBalance( | ||
| { chainId, userAddress, tokenAddress }: FieldsOf<TokenBalanceQuery>, | ||
| enabled: boolean = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we normally use validation suites rather than enabled, but not using queryFactory (because of wagmi) makes it a big more complex and I wanted to know if my refactor was working at all. So I opted for this (anti?)-pattern that we've used for some other hooks as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabling a query is fine, but IMO we can also add a validation suite here such that enabled=enabled&&valid
| get()[sliceKey].setStateByActiveKey('userShare', userPoolActiveKey, userShare) | ||
| get()[sliceKey].setStateByActiveKey('userLiquidityUsd', userPoolActiveKey, liquidityUsd) | ||
|
|
||
| return fetchedWalletBalances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return value was never used
| enabled = true, | ||
| ) { | ||
| const { curveApi, isHydrated } = useCurve() | ||
| const pool = chainId && userAddress && poolId && curveApi && isHydrated ? curveApi.getPool(poolId) : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hydration is required, because if it isn't getPool will throw an error it can't find the poolId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of checks that aren't really related to the pool, they should be handled by the query's validation not manually here imo
chainId: is not used hereuserAddress: has nothing to do with poolpoolId: can it really be undefined after hydration?curveApi: cannot be undefined whenisHydrated
| const pool = chainId && userAddress && poolId && curveApi && isHydrated ? curveApi.getPool(poolId) : undefined | |
| const pool = useMemo(() => isHydrated && curveApi.getPool(poolId), [curveApi, isHydrated, poolId]) |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [chainId, poolId, signerAddress, formValues, formStatus, slippage.isHighSlippage, slippageConfirmed, maxSlippage]) | ||
| }, [ | ||
| config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of times exhaustive deps was turned off. I didn't dare to remove it, afraid of breaking anything. I just added config and called it a day.
| balance={balLpToken} | ||
| balanceLoading={balancesLoading} | ||
| hasError={haveSigner ? new BigNumber(formValues.lpToken).isGreaterThan(balLpToken as string) : false} | ||
| balance={lpTokenBalance ?? '0'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was there already, but omg it looks so bad when your balance is 0 during loading.
| const userToBalance = userPoolBalances?.[formValues.toAddress] | ||
| const config = useConfig() | ||
| const { address: userAddress } = useConnection() | ||
| const { data: userFromBalance, isLoading: userFromBalanceLoading } = useTokenBalance({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of a version that allows an array or tuple of addresses via useQueries?
| balance={haveSigner ? balGauge : ''} | ||
| hasError={+formValues.stakedLpToken > +balGauge} | ||
| balanceLoading={gaugeTokenLoading} | ||
| balance={gaugeTokenBalance ?? '0'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the existing code did not have ?? 0 but as string
| !seed.loaded || | ||
| walletBalancesLoading | ||
|
|
||
| const hasWalletBalances = Object.keys(wrappedCoinsBalances).length && Object.keys(underlyingCoinsBalances).length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use usePoolTokenBalances(..).error for this, I don't see an use case for generic error
| const { curveApi, isHydrated } = useCurve() | ||
| const pool = chainId && userAddress && poolId && curveApi && isHydrated ? curveApi.getPool(poolId) : undefined | ||
|
|
||
| const { data: wrappedCoinsBalances, isLoading: wrappedCoinsLoading } = useTokenBalances( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be extracting the error here, too
| export function fetchPoolLpTokenBalance(config: Config, curve: CurveApi, poolId: string) { | ||
| const pool = requireLib('curveApi').getPool(poolId) | ||
|
|
||
| return fetchTokenBalance(config, { | ||
| chainId: curve?.chainId, | ||
| userAddress: curve.signerAddress as Address, | ||
| tokenAddress: pool.lpToken as Address, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call requireLib if you are passing curve here? 😅
| export function fetchPoolLpTokenBalance(config: Config, curve: CurveApi, poolId: string) { | |
| const pool = requireLib('curveApi').getPool(poolId) | |
| return fetchTokenBalance(config, { | |
| chainId: curve?.chainId, | |
| userAddress: curve.signerAddress as Address, | |
| tokenAddress: pool.lpToken as Address, | |
| }) | |
| } | |
| export const fetchPoolLpTokenBalance = async (config: Config, curve: CurveApi, poolId: string) => | |
| await fetchTokenBalance(config, { | |
| chainId: curve.chainId, | |
| userAddress: curve.signerAddress as Address, | |
| tokenAddress: curve.getPool(poolId).lpToken as Address, | |
| }) |
| export function useTokenBalance({ chainId, userAddress, tokenAddress }: FieldsOf<TokenBalanceQuery>) { | ||
| export function useTokenBalance( | ||
| { chainId, userAddress, tokenAddress }: FieldsOf<TokenBalanceQuery>, | ||
| enabled: boolean = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabling a query is fine, but IMO we can also add a validation suite here such that enabled=enabled&&valid
|
|
||
| /** Get query options for a token balance (handles both native and ERC-20) */ | ||
| const getTokenBalanceQueryOptions = (config: Config, query: TokenBalanceQuery) => { | ||
| if (isNative(query)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ternary + inline arrow would be nice
| const config = useConfig() | ||
|
|
||
| const isEnabled = enabled && chainId != null && userAddress != null | ||
| const uniqueAddresses = useMemo(() => Array.from(new Set(tokenAddresses)), [tokenAddresses]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't wagmi unify them already?
| ) { | ||
| const config = useConfig() | ||
|
|
||
| const isEnabled = enabled && chainId != null && userAddress != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the validation group here, too
Depends on #1757
The Zustand rot is real; like 80% of this PR is just me passing a wagmi config to all the imperative Zustand functions, just to be able to use the imperative versions of the new wagmi native token balance hooks.
Please don't judge too hardly about that plumbing, the main focus should be on:
token-balance.query.tsusePoolTokenDepositBalances.tsusePoolTokenBalances.tsThe harsh reality here is that there's no automated tests for now, so all we can do is test manually 😭