feat: add support for pool migration #1702
Conversation
WalkthroughThe changes introduce pooled balance handling for nomination pools. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/extension-polkagate/src/hooks/useReservedDetails.ts(3 hunks)packages/extension-polkagate/src/popup/account/util.ts(3 hunks)packages/extension-polkagate/src/popup/home/news.ts(2 hunks)packages/extension-polkagate/src/util/constants.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/util/constants.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension-polkagate/src/popup/account/util.ts
[error] 28-28: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/extension-polkagate/src/popup/account/util.ts (1)
30-32: LGTM! Balance calculation logic aligns with pool migration.The total balance calculation correctly excludes pooled balance for migrated chains (Kusama and Westend) where it's now part of reserved balance.
packages/extension-polkagate/src/popup/home/news.ts (1)
12-17: LGTM! Clear and informative news entry.The news entry accurately describes the pool migration support and its impact on governance voting capabilities.
packages/extension-polkagate/src/hooks/useReservedDetails.ts (3)
20-20: LGTM! Type definition updated for pool migration.The Item type correctly includes pooledBalance as a possible reserved detail.
30-32: LGTM! Efficient memoization of pooled balance.The pooled balance is correctly memoized with appropriate dependencies.
284-293: LGTM! Proper handling of pooled balance as reserved.The implementation correctly:
- Checks if the chain is in MIGRATED_NOMINATION_POOLS_CHAINS
- Verifies non-zero pooled balance
- Updates reserved state with the pooled balance
Nick-1979
changed the title
Nick-1979
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/extension-polkagate/src/util/workers/utils/balancify.js (1)
19-20: LGTM! Consider adding documentation.The new votingBalance calculation correctly handles pooled balances being part of reserved balance after pool migration. The fallback to BN_ZERO ensures safe handling of undefined reserved balances.
Consider adding a JSDoc comment to document this behavior change:
+ /** + * Calculate voting balance as free + reserved. + * After pool migration, pooled balances are part of reserved balance on certain chains, + * so we no longer use the API's votingBalance directly. + */ votingBalance: String(balances.free.add(balances?.reserved || BN_ZERO))packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (1)
59-60: Consider updating the dependency array.The votingBalance calculation depends on allBalances, but it's not included in the dependency array of the useCallback hook.
Add allBalances to the dependency array:
}, [api, genesisHash, chainName, decimal, stakingAccount, token, isFetchingNativeTokenOfAssetHub, balances, formatted, isFetching.fetching[String(formatted)]?.['length'], setRefresh + , allBalances ]);packages/extension-polkagate/src/fullscreen/governance/delegate/partial/AlreadyLockedTooltipText.tsx (1)
34-44: Consider using for...of loop for better readability.Instead of disabling the ESLint rule, consider refactoring the loop to use the more readable for...of syntax.
- // eslint-disable-next-line @typescript-eslint/prefer-for-of - for (let i = 0; i < LOCKS_ORDERED.length; i++) { - const lockName = LOCKS_ORDERED[i]; + for (const lockName of LOCKS_ORDERED) { if (a.id.eq(lockName)) { return -1; } else if (b.id.eq(lockName)) { return 1; } - } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/AlreadyLockedTooltipText.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/manageIdentity/Review.tsx(2 hunks)packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts(3 hunks)packages/extension-polkagate/src/util/types.ts(2 hunks)packages/extension-polkagate/src/util/workers/utils/balancify.js(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/extension-polkagate/src/util/types.ts
- packages/extension-polkagate/src/fullscreen/manageIdentity/Review.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/extension-polkagate/src/util/workers/utils/balancify.js (1)
1-5: LGTM!The copyright year update and BN_ZERO import are appropriate for the new votingBalance calculation.
packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (1)
5-5: LGTM!The Balance type import and votingBalance calculation using api.createType ensure type safety and consistent behavior.
Also applies to: 46-46
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/AlreadyLockedTooltipText.tsx (3)
7-9: LGTM!The import changes appropriately support the pool migration changes by updating to the new
BalancesInfotype and adding necessary type imports.
24-24: LGTM!The function signature has been appropriately updated to use the new
BalancesInfotype while maintaining the same functionality.
85-85: LGTM!Good addition of the optional chaining operator to safely handle potentially undefined
refIdvalues.
.../extension-polkagate/src/fullscreen/governance/delegate/partial/AlreadyLockedTooltipText.tsx
Show resolved
Hide resolved
Nick-1979
requested review from
AMIRKHANEF
and removed request for
AMIRKHANEF
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (1)
29-30: LGTM! Correctly implements pooled balance handling.The implementation aligns with PR objectives by excluding pooled balance from total balance calculation for migrated chains (Kusama and Westend) where it's part of reserved balance.
Consider extracting the total balance calculation to a separate function for better readability:
+ const calculateTotalBalance = (freeBalance, reservedBalance, pooledBalance, isMigrated) => { + return freeBalance.add(reservedBalance).add(isMigrated ? BN_ZERO : pooledBalance); + }; + balanceInfo.forEach(({ address, balances, pooledBalance, soloTotal }) => { const genesisHash = api.genesisHash.toString(); const isMigrated = MIGRATED_NOMINATION_POOLS_CHAINS.includes(genesisHash); - const totalBalance = balances.freeBalance.add(balances.reservedBalance).add(isMigrated ? BN_ZERO : pooledBalance); + const totalBalance = calculateTotalBalance(balances.freeBalance, balances.reservedBalance, pooledBalance, isMigrated);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (1)
4-6: LGTM! Imports align with PR objectives.The new imports support the pooled balance migration feature:
BN_ZEROfor zero value when excluding pooled balanceMIGRATED_NOMINATION_POOLS_CHAINSfor checking chain migration status
# [0.35.0](v0.34.0...v0.35.0) (2025-02-22) ### Features * add support for pool migration ([#1702](#1702)) ([55c57b8](55c57b8))
Pooled balance is part of reserved on kusama and westend, hence no need to add them to total balance anymore.
Summary by CodeRabbit