fix: staking balances get undefined in useBalances #1609
Conversation
WalkthroughThe pull request introduces modifications to three files primarily focused on the balance management logic within the application. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/hooks/useBalances.ts (1)
48-48: Consider implementing comprehensive asset storage.The TODO comment indicates that only native assets are saved in local storage. Given the staking balance issues, consider implementing a more comprehensive storage solution that includes all asset types (native, staking, pooled) to prevent undefined values during state updates.
Consider:
- Extending the
SavedBalancestype to include dedicated fields for staking and pooled balances- Implementing a caching strategy with TTL for non-native assets
- Adding a migration mechanism for existing stored balances
packages/extension-polkagate/src/popup/account/index.tsx (1)
231-234: Fix indentation in staking balance section.The indentation is inconsistent in the staking balance conditional blocks. Consider aligning them for better readability.
{supportStaking && <> - {balances?.soloTotal && !balances?.soloTotal?.isZero() && - <LabelBalancePrice address={address} balances={balances} label={'Solo Stake'} onClick={goToSoloStaking} title={t('Solo Stake')} />} - {balances?.pooledBalance && !balances?.pooledBalance?.isZero() && - <LabelBalancePrice address={address} balances={balances} label={'Pool Stake'} onClick={goToPoolStaking} title={t('Pool Stake')} /> - } + {balances?.soloTotal && !balances?.soloTotal?.isZero() && + <LabelBalancePrice address={address} balances={balances} label={'Solo Stake'} onClick={goToSoloStaking} title={t('Solo Stake')} /> + } + {balances?.pooledBalance && !balances?.pooledBalance?.isZero() && + <LabelBalancePrice address={address} balances={balances} label={'Pool Stake'} onClick={goToPoolStaking} title={t('Pool Stake')} /> + } </> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/extension-polkagate/src/hooks/useBalances.ts (4 hunks)
- packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (3 hunks)
- packages/extension-polkagate/src/popup/account/index.tsx (4 hunks)
🔇 Additional comments (9)
packages/extension-polkagate/src/hooks/useBalances.ts (3)
81-83: LGTM! Improved fallback handling.The modified return logic provides better fallback handling by checking the
balancesobject whenoverallis not available or doesn't match the current chain. This should help prevent undefined values from being displayed.
12-12: Verify staking balance handling in useNativeAssetBalances.The removal of
useStakingAccountdependency suggests that staking balances are now handled throughuseNativeAssetBalances. Let's verify this implementation to ensure it properly addresses the staking balance undefined issue.
Line range hint
14-83: Verify the fix resolves the hide/show effect.While the changes look good and should help prevent undefined staking balances, we should verify that this resolves the hide/show effect mentioned in the PR objectives.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to better understand the balance update patterns.
The first script will help us understand how native balances are fetched, the second will show balance-related state management patterns, and the third will reveal any remaining staking-balance dependencies. This will give us a clearer picture of whether the hide/show effect has been properly addressed.
Let me check one more aspect to understand the balance update mechanism better.
These scripts will help us understand the balance update mechanism and potential sources of flickering.
Based on the analysis of the code, I can now provide a final response about the hide/show effect.
The balance flickering issue has been properly addressed
The code shows several safeguards against balance flickering:
- The
useBalanceshook now has proper synchronization between native balances, pooled balances, and genesis hash checks:if (balances && isUpToDate(balances?.date) && pooledBalance && apiGenesisHash === chainGenesisHash && apiGenesisHash === balances?.genesisHash && apiGenesisHash === pooledBalance.genesisHash) { setOverall({...balances, pooledBalance: pooledBalance.balance}); } else { setOverall(undefined); }
The
useNativeAssetBalanceshook includes proper state management with validation checks before updates, preventing intermediate invalid states.The removal of direct staking account dependency and the addition of proper chain/genesis hash validation ensures that balances are only displayed when they are fully synchronized with the current chain context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential race conditions or timing issues in balance updates # Look for other components that might cause balance flickering rg -A 5 'useBalances.*staking|staking.*useBalances' packages/extension-polkagate/src/ # Check for any setState or update patterns that might cause unnecessary rerenders ast-grep --pattern 'setState($$$)'Length of output: 4637
Script:
#!/bin/bash # Check the implementation of useNativeAssetBalances and related hooks ast-grep --pattern 'function useNativeAssetBalances($$$)' # Look for balance-related state updates in the component ast-grep --pattern 'const [overall, setOverall] = useState($$$)' # Check for any direct dependencies on staking in the balances hook rg -A 5 'staking.*balance|balance.*staking' packages/extension-polkagate/src/hooks/Length of output: 936
Script:
#!/bin/bash # Check the implementation of useNativeAssetBalances to understand the update logic rg -A 20 'export default function useNativeAssetBalances' packages/extension-polkagate/src/hooks/ # Look for any balance-related effects that might cause flickering rg -A 10 'useEffect.*balance|balance.*useEffect' packages/extension-polkagate/src/hooks/useBalances.ts # Check for any race conditions in balance updates rg -A 10 'setOverall|setBalances' packages/extension-polkagate/src/hooks/Length of output: 18058
packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (2)
20-20: LGTM: Improved staking account data freshness controlThe addition of the
onlyNewparameter touseStakingAccounthelps ensure fresh staking data is used when needed, which aligns with fixing the staking balances undefined issue.
54-55: Verify type safety of stakingLedger.total castingWhile preserving the pooledBalance and adding soloTotal improves balance tracking, the direct type casting of
stakingAccount?.stakingLedger?.totaltoBNcould be unsafe.Consider adding a type check:
-soloTotal: stakingAccount?.stakingLedger?.total as unknown as BN, +soloTotal: stakingAccount?.stakingLedger?.total instanceof BN ? stakingAccount.stakingLedger.total : BN_ZERO,packages/extension-polkagate/src/popup/account/index.tsx (4)
13-13: LGTM: Import cleanup aligns with simplified balance handling.The removal of
BalancesInfoimport reflects the shift to using thebalancesobject directly fromuseBalanceshook, which helps prevent undefined staking values.
59-60: LGTM: Improved staking balance checks.The updated logic properly handles potential undefined values using optional chaining and directly checks staking balances from the
balancesobject, addressing the hide/show effect issue mentioned in the PR objectives.
247-247: LGTM: Consistent balance prop passing.The direct passing of the
balancesobject toStakingOptioncomponent maintains consistency with the new balance handling approach.
Line range hint
206-242: Verify the fix for staking balance hide/show effect.The changes look good and should resolve the staking balance issues. Let's verify that the hide/show effect is fixed by checking for any remaining places where staking balances might become undefined.
✅ Verification successful
Staking balance hide/show effect has been properly fixed
The verification shows that all staking balance accesses are properly guarded:
- All direct accesses to
soloTotalandpooledBalanceuse optional chaining (?.)- Zero-value checks are consistently implemented with
isZero()- No unguarded balance state management was found
The changes in the PR effectively prevent the hide/show effect by:
- Using optional chaining for all staking balance accesses
- Properly checking for both undefined and zero values
- Consistent implementation across all components (account details, staking options)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential places where staking balances might still become undefined # Search for direct access to staking-related balances without optional chaining rg -n "balances\.(soloTotal|pooledBalance)[^?]" --type typescript # Search for setBalance calls that might affect staking values rg -n "setBalance.*staking|staking.*setBalance" --type typescriptLength of output: 205
Script:
#!/bin/bash # Let's try without typescript file type and use more specific patterns # Check for direct access to staking balances rg -n "balances\.(soloTotal|pooledBalance)[^?]" # Look for balance state management patterns rg -n "setBalance.*staking|staking.*setBalance" # Check for any undefined checks on these properties rg -n "(soloTotal|pooledBalance)\s*&&\s*!" --multilineLength of output: 2311
| @@ -22,7 +20,7 @@ export default function useBalances (address: string | undefined, refresh?: bool | |||
| const balances = useNativeAssetBalances(address, refresh, setRefresh, onlyNew); | |||
| const maybeBalancesOnAssetHub = useBalancesOnAssethub(address, maybeNonNativeAssetId); | |||
| const maybeBalancesOnMultiChainAssets = useBalancesOnMultiAssetChain(address, maybeNonNativeAssetId); | |||
| const pooledBalance = usePoolBalances(address, refresh); | |||
| const pooledBalance = usePoolBalances(address, refresh); // can move it inside useNativeAssetBalances hook and then remove overall state var | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating balance hooks.
The TODO comment correctly suggests that usePoolBalances could be moved inside useNativeAssetBalances. This would:
- Reduce the number of state variables
- Centralize balance fetching logic
- Simplify the dependency array in useEffect
Consider refactoring the hooks structure like this:
// In useNativeAssetBalances.ts
export function useNativeAssetBalances(address: string, refresh?: boolean, ...) {
const pooledBalance = usePoolBalances(address, refresh);
// Combine all balances including pooled balance in one place
return {
...balances,
pooledBalance: pooledBalance?.balance
};
}
// Then simplify useBalances.ts
export default function useBalances(...) {
const balances = useNativeAssetBalances(address, refresh, setRefresh, onlyNew);
// Remove pooledBalance and overall state...
}
## [0.20.1](v0.20.0...v0.20.1) (2024-10-27) ### Bug Fixes * staking balances get undefined in useBalances ([#1609](#1609)) ([2d97e9a](2d97e9a))
Since
useBalancesfetches new data, it does not incorporate staking values. As a result, we observe a hide/show effect for those amounts in the extension mode account detailsSummary by CodeRabbit
New Features
Bug Fixes
AccountDetailscomponent, reducing potential discrepancies.Refactor
Documentation