fix: infinit loop issue in Edit pool FS, negative number at unstake #1682
Conversation
WalkthroughThe pull request introduces several modifications across multiple components in the PolkaGate extension. Key changes include refactoring function declarations to named exports and wrapping components with Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
🧹 Outside diff range and nitpick comments (10)
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/ShowPoolRole.tsx (1)
44-44: Consider using more descriptive message for missing roleThe message "To be Removed" might be confusing to users. Consider using a more informative message like "No role assigned" or "Role will be removed" to better convey the state.
- {t('To be Removed')} + {t('Role will be removed')}packages/extension-polkagate/src/fullscreen/stake/pool/unstake/index.tsx (1)
Line range hint
134-160: Improve error handling with user feedbackWhile the fee estimation logic is correct, consider enhancing error handling by:
- Providing user feedback when fee estimation fails
- Adding a fallback fee estimation mechanism
if (!api || !formatted || !unbonded || !amountAsBN) { + setEstimatedFee(undefined); + console.warn('Fee estimation skipped: required dependencies not available'); return; } const params = [formatted, amountAsBN]; if (!api?.call?.['transactionPaymentApi']) { + console.warn('TransactionPaymentApi not available, using minimum fee'); return setEstimatedFee(api?.createType('Balance', BN_ONE) as Balance); } poolWithdrawUnbonded && maxUnlockingChunks && unlockingLen !== undefined && unbonded(...params).paymentInfo(formatted).then((i) => { const fee = i?.partialFee; if (unlockingLen < maxUnlockingChunks) { setEstimatedFee(fee); } else { const dummyParams = [1, 1]; poolWithdrawUnbonded(...dummyParams) .paymentInfo(formatted) .then( (j) => setEstimatedFee(api.createType('Balance', fee.add(j?.partialFee || BN_ZERO)) as Balance) ) - .catch(console.error); + .catch((error) => { + console.error('Failed to estimate withdrawal fee:', error); + setEstimatedFee(fee); // Fallback to using only unbond fee + }); } - }).catch(console.error); + }).catch((error) => { + console.error('Failed to estimate unbond fee:', error); + setEstimatedFee(api?.createType('Balance', BN_ONE) as Balance); // Fallback to minimum fee + });packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (3)
Line range hint
82-91: Consider using useLayoutEffect and batch updatesWhile the initialization logic works, there are a few potential improvements:
- The comment about useLayoutEffect is valid - it would prevent potential flicker during initialization
- Consider batching the state updates to reduce render cycles
Here's a suggested improvement:
- useEffect(() => { // maybe it is better to use "useLayoutEffect" + useLayoutEffect(() => { + const initialState = { + poolName: poolName || undefined, + rootAddress: pool?.bondedPool?.roles?.root?.toString() || undefined, + nominatorAddress: pool?.bondedPool?.roles?.nominator?.toString() || undefined, + bouncerAddress: pool?.bondedPool?.roles?.bouncer?.toString() || undefined, + commissionPayee: maybeCommissionPayee || undefined, + commissionValue: commissionValue || undefined + }; + + setNewPoolName(initialState.poolName); + setNewRootAddress(initialState.rootAddress); + setNewNominatorAddress(initialState.nominatorAddress); + setNewBouncerAddress(initialState.bouncerAddress); + setNewCommissionPayee(initialState.commissionPayee); + setNewCommissionValue(initialState.commissionValue); }, []);
Line range hint
126-134: Add validation for negative commission valuesGiven the PR title mentions issues with negative numbers, the commission input should explicitly prevent negative values.
Here's a suggested fix:
const onNewCommission = useCallback((e: { target: { value: any; }; }) => { const value = Number(e.target.value); + if (value < 0) { + setNewCommissionValue(0); + return; + } + if (value !== commissionValue) { setNewCommissionValue(value > 100 ? 100 : value); } else { setNewCommissionValue(undefined); } }, [commissionValue]);
Line range hint
93-106: Prevent potential infinite loops in changes trackingThe changes tracking useEffect might be related to the "infinit loop issue" mentioned in the PR title. The current implementation could cause unnecessary re-renders or loops.
Consider these improvements:
useEffect(() => { + const newChanges = { + commission: { + payee: getChangedValue(newCommissionPayee, maybeCommissionPayee) as string, + value: (newCommissionPayee || maybeCommissionPayee) + ? getChangedValue(newCommissionValue, commissionValue) as number + : undefined + }, + newPoolName: getChangedValue(newPoolName, poolName) as string, + newRoles: { + newBouncer: getChangedValue(newBouncerAddress, poolRoles?.bouncer?.toString()) as string, + newNominator: getChangedValue(newNominatorAddress, poolRoles?.nominator?.toString()) as string, + newRoot: getChangedValue(newRootAddress, poolRoles?.root?.toString()) as string + } + }; + + // Only update if changes actually differ to prevent unnecessary re-renders + if (JSON.stringify(changes) !== JSON.stringify(newChanges)) { setChanges({ - commission: { - payee: getChangedValue(newCommissionPayee, maybeCommissionPayee) as string, - value: (newCommissionPayee || maybeCommissionPayee) ? getChangedValue(newCommissionValue, commissionValue) as number : undefined - }, - newPoolName: getChangedValue(newPoolName, poolName) as string, - newRoles: { - newBouncer: getChangedValue(newBouncerAddress, poolRoles?.bouncer?.toString()) as string, - newNominator: getChangedValue(newNominatorAddress, poolRoles?.nominator?.toString()) as string, - newRoot: getChangedValue(newRootAddress, poolRoles?.root?.toString()) as string - } + ...newChanges }); + } }, [/* dependencies */]);packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Review.tsx (5)
40-48: Simplify conditional logic ingetRolefunctionThe
getRolefunction can be simplified by removing unnecessaryelsestatements afterreturn, enhancing readability.Consider applying this refactor:
const getRole = (role: string | undefined | null) => { if (role === undefined) { return 'Noop'; } if (role === null) { return 'Remove'; } return { set: role }; };
86-86: Define scaling factor as a named constantUsing the magic number
10 ** 7can make the code less clear. Define it as a named constant to improve readability and maintainability.For example:
+const COMMISSION_SCALE = 10 ** 7; ... (params: [ pool.poolId, [ - (changes.commission.value || 0) * 10 ** 7, + (changes.commission.value || 0) * COMMISSION_SCALE, changes.commission.payee || maybeCurrentCommissionPayee ] ])
139-140: Avoid using// @ts-ignore; resolve TypeScript errors directlySuppressing TypeScript errors with
// @ts-ignorecan hide potential issues. Address the underlying type error to maintain type safety and code quality.Investigate and correct the type mismatch instead of ignoring it:
-//@ts-ignore const maybeCurrentCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString() as string | undefined;Ensure that
pool.bondedPool.commission.currenthas the correct type definition.
137-137: Refine type assertion forselectedProxyAddressCasting
selectedProxy?.delegateasunknown as stringmay indicate a type issue. Adjust the type definitions to avoid unnecessary casting and ensure type safety.Consider updating the type of
selectedProxy.delegatetostringif appropriate, eliminating the need for casting:const selectedProxyAddress = selectedProxy?.delegate;
107-111: Handle fee estimation errors gracefullyCurrently, errors during fee estimation are only logged to the console. Consider providing user feedback or fallback behavior to enhance the user experience in case of failures.
For example:
.call(...params).paymentInfo(formatted) .then((i) => { fee = api.createType('Balance', i?.partialFee) as Balance; setEstimatedFee(fee); // ... }) - .catch(console.error); + .catch((error) => { + console.error(error); + // Provide user feedback or set a default fee + setEstimatedFee(api.createType('Balance', BN_ONE) as Balance); + // Optionally, inform the user of the error + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Review.tsx(6 hunks)packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/ShowPoolRole.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/index.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/stake/pool/unstake/index.tsx(4 hunks)
🔇 Additional comments (10)
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/ShowPoolRole.tsx (2)
21-21: LGTM! Clean function declaration with proper typing
The conversion to a named function improves code organization and debugging capabilities.
54-54: Verify the benefit of React.memo in this context
While the use of React.memo is appropriate for this component, let's verify if it's used in a context where memoization provides meaningful performance benefits.
✅ Verification successful
Let's try to understand both the implementation and usage context of this component to verify if React.memo provides meaningful benefits.
React.memo usage is beneficial in this context
The use of React.memo for ShowPoolRole is justified and provides performance benefits because:
- The component is used multiple times within the Review component, which handles complex state management and updates
- The parent Review component manages multiple states (proxies, password errors, transaction preparation) that could trigger re-renders
- ShowPoolRole receives only primitive props (strings and booleans) making memoization effective
- The component is used in a list-like pattern where multiple instances display different roles, making it important to prevent unnecessary re-renders of unchanged items
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ShowPoolRole usage to understand re-render scenarios
rg -A 5 "ShowPoolRole" --type tsx
Length of output: 61
Script:
#!/bin/bash
# Search for ShowPoolRole usage with ripgrep, without file type filter
rg -A 5 "ShowPoolRole"
# Also search for the file containing ShowPoolRole to understand its implementation
fd "ShowPoolRole.tsx"
Length of output: 8966
Script:
#!/bin/bash
# Let's check the implementation of ShowPoolRole to understand its complexity
cat packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/ShowPoolRole.tsx
# Let's also check the parent component Review.tsx to understand the re-render scenarios
cat packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Review.tsx
Length of output: 12322
packages/extension-polkagate/src/fullscreen/stake/pool/unstake/index.tsx (2)
81-81: LGTM: Type safety improvement
The use of toPrimitive() method properly converts the chain constant to a primitive number, improving type safety.
194-199: Verify handling of legacy staking amounts
The code now handles a rare edge case where old stakers have less than the minimum create bond. While the solution is correct, we should verify:
- The number of affected accounts
- The impact on their unstaking process
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient context about the code handling and its implications.
Handling of legacy staking amounts is properly implemented
The code correctly handles an edge case for pool depositors who staked before minCreateBond updates. The implementation is safe because:
- The condition only affects pool depositors (root/creator role)
- It has proper validation checks:
- Only triggers when
poolState !== 'Destroying' || poolMemberCounter !== 1 - Calculates partial amount as
staked.sub(poolConsts.minCreateBond)
- Only triggers when
- The negative case handling allows full unstaking, preventing funds from being locked
- The same logic is consistently implemented in both popup and fullscreen interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find accounts that might be affected by this change
# Expected results: List of pool members with stake less than minCreateBond
# Get all nomination pool members
rg -l "nomination.*pools.*member" | while read -r file; do
echo "Checking $file for potential affected accounts..."
ast-grep --pattern 'stakingLedger.active' "$file" -A 5
done
Length of output: 50
Script:
#!/bin/bash
# Let's try a different approach to find related code and understand the context better
# Search for files containing pool unstaking logic
rg -l "unstake.*pool" --type tsx --type ts
# Look for any references to minCreateBond in the codebase
rg "minCreateBond" -A 3 -B 3
# Search for any negative value handling in pool operations
rg "isNeg\(\)" -A 3 -B 3
Length of output: 17975
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/index.tsx (3)
113-113: Ensure prop stability for effective memoization.
The React.memo wrapper is a good addition for performance optimization. However, its effectiveness depends on prop stability.
Consider adding a custom comparison function to React.memo if complex prop comparisons are needed:
export default React.memo(ManageEditPool, (prevProps, nextProps) => {
// Add custom comparison logic for complex props like pool
return prevProps.pool.id === nextProps.pool.id;
});✅ Verification successful
Props are well-defined and stable for memoization.
Looking at the component's implementation, the props (address, api, chain, onClose, pool, setRefresh) are all well-typed and appear to be stable values:
address,chain, andpoolare likely stable referencesapiis passed as an external dependencyonCloseandsetRefreshare callback functions that should be stable if properly defined by the parent
The current memoization implementation is sufficient as these props are primitive values or stable references. A custom comparison function isn't necessary in this case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how the component is used and if props are stable
rg -A 5 '<ManageEditPool'
Length of output: 27
Script:
#!/bin/bash
# Let's first check the component definition and its props
ast-grep --pattern 'const ManageEditPool = ({ $$$props }) => { $$$ }'
# Also check if ManageEditPool is imported anywhere
rg "import.*ManageEditPool"
# Check for any dynamic usage with spread props
rg "<ManageEditPool.*\.\.\."
Length of output: 132
Script:
#!/bin/bash
# Let's try a different pattern to find the component definition
ast-grep --pattern 'function ManageEditPool($$$) { $$$ }'
# Let's also check the file content directly to understand the component
cat packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/index.tsx
# Check for any usage of this component in tsx/jsx files
rg -t tsx -t jsx "ManageEditPool"
Length of output: 3728
Line range hint 46-113: Verify if the infinite loop issue is fully addressed.
The PR title mentions fixing an infinite loop issue, but the changes only show React.memo optimization. While this might help prevent unnecessary re-renders, please verify if this completely resolves the reported infinite loop.
Line range hint 46-111: Verify state dependencies to prevent infinite re-renders.
While the component structure is clean, the multiple state updates (step, txInfo, changes) could potentially cause unnecessary re-renders or loops. Consider using useCallback for state setters passed as props to child components.
Consider these improvements:
- Memoize callback props using
useCallback - Combine related state into a single reducer if they often update together
- Add dependency arrays to any useEffects in child components
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (3)
33-33: LGTM: Good performance optimization with React.memo
The change to use React.memo is appropriate here given the component's complexity and multiple prop dependencies. This will help prevent unnecessary re-renders when parent components update.
Also applies to: 259-259
120-120: LGTM: Good type safety improvement
The use of Record<string, unknown> for type assertion is a good practice that improves type safety when dealing with object values.
Line range hint 93-106: Verify other instances of infinite loop issues
Let's verify if there are similar patterns in other components that might cause infinite loops.
✅ Verification successful
Let me analyze the code further to check for potential infinite loop issues.
After analyzing the code, I can now provide a final response:
No infinite loop issues found in the codebase
The useEffect implementations in both files (Edit.tsx and index.tsx) are properly structured with:
- An initialization effect with empty dependency array
[] - A changes calculation effect with proper dependency arrays including all referenced variables
- An API query effect with appropriate API dependency
The dependency arrays are complete and the state updates are triggered only when the dependencies actually change, preventing infinite loops.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useEffect hooks with setChanges or similar state updates
ast-grep --pattern 'useEffect(() => {
$$$
setChanges($$$)
$$$
}, [$$$])'
# Search for potential infinite loop patterns in other components
ast-grep --pattern 'const [changes, setChanges] = useState($$$)'
Length of output: 4191
Script:
#!/bin/bash
# Let's check the dependency arrays of useEffect hooks in these files
rg -A 2 -B 2 "useEffect\(" packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/index.tsx
# Let's also check how changes state is used
rg "changes\." packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/index.tsx
Length of output: 4307
| const extraInfo = { | ||
| action: 'Pool Staking', | ||
| fee: String(fee ?? 0), | ||
| subAction: 'Edit Pool' | ||
| }; |
There was a problem hiding this comment.
Ensure extraInfo uses the updated fee value
The fee used in extraInfo may not reflect the actual estimated fee because fee estimation is asynchronous. Assign extraInfo after the fee estimation completes to ensure accurate information.
Modify the code to set extraInfo after fee estimation:
-const extraInfo = {
- action: 'Pool Staking',
- fee: String(fee ?? 0),
- subAction: 'Edit Pool'
-};
...
call(...params).paymentInfo(formatted)
.then((i) => {
fee = api.createType('Balance', i?.partialFee) as Balance;
setEstimatedFee(fee);
+ const extraInfo = {
+ action: 'Pool Staking',
+ fee: String(fee ?? 0),
+ subAction: 'Edit Pool'
+ };
+ setInputs({
+ call,
+ extraInfo,
+ params
+ });
})
.catch(console.error);
...
-setInputs({
- call,
- extraInfo,
- params
-});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Review.tsx (2)
142-146: Add cleanup function to useEffectThe effect sets state but doesn't clean up on unmount, which could lead to memory leaks.
Add a cleanup function:
useEffect((): void => { const fetchedProxyItems = proxies?.map((p: Proxy) => ({ proxy: p, status: 'current' })) as ProxyItem[]; setProxyItems(fetchedProxyItems); + return () => { + setProxyItems(undefined); + }; }, [proxies]);
177-179: Add aria-labels for accessibilityThe role titles should have proper aria-labels for screen readers.
Add aria-labels to the role titles:
- {t('Pool name')} + <span aria-label={t('Pool name as displayed in the pool list')}>{t('Pool name')}</span> - roleTitle={t('Root')} + roleTitle={<span aria-label={t('Root role of the pool')}>{t('Root')}</span>} - roleTitle={t('Nominator')} + roleTitle={<span aria-label={t('Nominator role of the pool')}>{t('Nominator')}</span>} - roleTitle={t('Bouncer')} + roleTitle={<span aria-label={t('Bouncer role of the pool')}>{t('Bouncer')}</span>} - roleTitle={t('Commission payee')} + roleTitle={<span aria-label={t('Address receiving pool commission')}>{t('Commission payee')}</span>}Also applies to: 192-194, 200-202, 208-210, 229-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Review.tsx(7 hunks)
🔇 Additional comments (4)
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Review.tsx (4)
40-48: LGTM: Clear and concise role mapping utility
The getRole helper function effectively maps role states to their corresponding pool configuration format.
282-282: LGTM: Proper performance optimization
Good use of React.memo to prevent unnecessary re-renders of the Review component.
98-117:
Fix race condition in fee estimation
The current implementation has a race condition where extraInfo is created with an initial fee value before the actual fee estimation completes.
Move the extraInfo creation inside the .then() callback to ensure it uses the correct fee:
- let fee: Balance = api.createType('Balance', BN_ONE) as Balance;
+ let fee: Balance;
// Estimate transaction fee
if (!api?.call?.['transactionPaymentApi']) {
fee = api.createType('Balance', BN_ONE) as Balance;
setEstimatedFee(fee);
+ setExtraInfo(fee);
}
call(...params).paymentInfo(formatted)
.then((i) => {
fee = api.createType('Balance', i?.partialFee) as Balance;
setEstimatedFee(fee);
+ setExtraInfo(fee);
})
.catch(console.error);
- const extraInfo = {
+ const setExtraInfo = (fee: Balance) => setInputs(prev => ({
+ ...prev,
+ extraInfo: {
action: 'Pool Staking',
fee: String(fee ?? 0),
subAction: 'Edit Pool'
+ }
+ }));
- };
- setInputs({
+ setInputs(prev => ({
+ ...prev,
call,
- extraInfo,
params
- });
+ }));63-91: Verify commission value calculation
The commission value calculation on line 86 assumes a specific decimal precision:
(changes.commission.value || 0) * 10 ** 7Let's verify if this matches the chain's commission configuration:
✅ Verification successful
Commission value calculation is consistent with codebase usage
The commission calculation using * 10 ** 7 is consistently used throughout the codebase for both reading and writing commission values:
- When reading validator commissions:
Number(v.validatorPrefs.commission) / (10 ** 7) - When setting pool commission:
(changes.commission.value || 0) * 10 ** 7
The codebase shows this is the standard scaling factor used for commission values in both nomination pools and validator contexts, making the calculation in the reviewed code correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the chain's commission configuration
ast-grep --pattern 'const COMMISSION_MAX = $_'
Length of output: 48
Script:
#!/bin/bash
# Let's search for commission-related constants and configurations
rg -i "commission.*max|max.*commission" -A 2
# Also search for any commission-related type definitions or configurations
ast-grep --pattern 'type $_Commission = $_'
# Let's also check the actual usage of commission calculations
rg "commission.*\*\s*10|10\s*\*.*commission" -A 2
Length of output: 13131
| //@ts-ignore | ||
| const maybeCurrentCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString() as string | undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove @ts-ignore and fix type assertion
The @ts-ignore comment suggests a potential type safety issue. Consider properly typing the pool structure.
Replace with proper type checking:
- //@ts-ignore
- const maybeCurrentCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString() as string | undefined;
+ const maybeCurrentCommissionPayee = pool?.bondedPool?.commission?.current &&
+ Array.isArray(pool.bondedPool.commission.current) &&
+ pool.bondedPool.commission.current[1]?.toString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //@ts-ignore | |
| const maybeCurrentCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString() as string | undefined; | |
| const maybeCurrentCommissionPayee = pool?.bondedPool?.commission?.current && | |
| Array.isArray(pool.bondedPool.commission.current) && | |
| pool.bondedPool.commission.current[1]?.toString(); |
Close: #1681
Summary by CodeRabbit
Release Notes
New Features
Performance Improvements
React.memoto optimize rendering and prevent unnecessary re-renders.Bug Fixes
Refactor