fix: add unbonding period time to staking #1443
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications across multiple components and a custom hook within the frontend application. Key changes include replacing hardcoded unbonding period values with dynamic values retrieved from a custom hook, ensuring accurate display of staking information. Additionally, minor adjustments were made to method names and UI elements to enhance user experience and functionality. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Outside diff range and nitpick comments (10)
frontend/src/app/(routes)/staking/components/DelegatePopup.tsx (2)
124-126: Approve: Dynamic unbonding period displayThe replacement of the hardcoded "21 days" with a dynamic value from
singleStake.getChainUnbondingPeriod(chainID)is a good improvement. It allows the component to accurately display the unbonding period for different chains.Consider adding a unit (e.g., "days") to the displayed value for clarity:
- Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days. + Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days.This assumes that the
getChainUnbondingPeriodmethod returns a number. If it returns a formatted string including the unit, this change is not necessary.
128-130: Approve: Consistent dynamic unbonding period displayThe replacement of the hardcoded "21 days" with
{singleStake.getChainUnbondingPeriod(chainID)}in the undelegation information is consistent with the previous change and improves the accuracy of the displayed information.As suggested for the previous instance, consider adding a unit (e.g., "days") to the displayed value for clarity:
- To make your staked assets liquid, undelegation will take {singleStake.getChainUnbondingPeriod(chainID)} days. + To make your staked assets liquid, undelegation will take {singleStake.getChainUnbondingPeriod(chainID)} days.Again, this assumes that the
getChainUnbondingPeriodmethod returns a number. If it returns a formatted string including the unit, this change is not necessary.frontend/src/app/(routes)/staking/components/UndelegatePopup.tsx (3)
122-122: Approve: Dynamic unbonding period displayThe change from a hardcoded value to
singleStake.getChainUnbondingPeriod(chainID)improves the flexibility of the component by displaying chain-specific unbonding periods. This aligns well with the PR objective.Consider adding a unit (e.g., "days") to the output for clarity:
-Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days. +Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days.This assumes that
getChainUnbondingPeriodreturns a number. If it returns a formatted string including the unit, disregard this suggestion.
126-126: Approve: Consistent use of dynamic unbonding periodThis change is consistent with the previous one, ensuring that both instances of the unbonding period information use the dynamic
getChainUnbondingPeriodmethod.Consider extracting the unbonding period into a variable to avoid multiple calls to
getChainUnbondingPeriod:+const unbondingPeriod = singleStake.getChainUnbondingPeriod(chainID); ... -Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days. +Staking will lock your funds for {unbondingPeriod} days. ... -To make your staked assets liquid, undelegation will take {singleStake.getChainUnbondingPeriod(chainID)} days. +To make your staked assets liquid, undelegation will take {unbondingPeriod} days.This would slightly improve readability and potentially performance if
getChainUnbondingPeriodis a computationally expensive operation.
Consistency Check Needed for Unbonding Periods
While the update in
UndelegatePopup.tsxcorrectly implements dynamic unbonding periods, the search results indicate that there are other instances of hardcoded day values in the codebase, particularly within governance and staking-related components. To ensure consistency and maintainability, it's recommended to refactor these instances to utilize dynamic unbonding periods similar to the changes made inUndelegatePopup.tsx.🔗 Analysis chain
Line range hint
1-146: Summary: Successful implementation of dynamic unbonding periodThe changes in this file successfully implement a dynamic unbonding period display, replacing hardcoded values with chain-specific ones. This improves the accuracy and flexibility of the
UndelegatePopupcomponent.To ensure consistency across the codebase, it would be beneficial to verify if similar updates are needed in other components. Run the following script to check for other occurrences of hardcoded unbonding periods:
This will help identify any other places where similar updates might be necessary to maintain consistency in displaying unbonding periods across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential hardcoded unbonding periods in other files # Test: Search for occurrences of "21 days" or similar in TypeScript/JavaScript files rg --type-add 'web:*.{ts,tsx,js,jsx}' -t web '(\d+\s*(day|days))' # Test: Search for other components or files that might need similar updates rg --type-add 'web:*.{ts,tsx,js,jsx}' -t web -i '(undelegate|unbonding|staking)'Length of output: 92916
frontend/src/app/(routes)/staking/components/NewDelegationDialog.tsx (3)
122-123: Approve: Dynamic unbonding period displayGreat job on replacing the hardcoded value with a dynamic one! This change improves the flexibility and accuracy of the staking information displayed to users.
Consider adding the word "approximately" before the unbonding period to account for potential slight variations:
- Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days. + Staking will lock your funds for approximately {singleStake.getChainUnbondingPeriod(chainID)} days.
126-127: Approve: Consistent use of dynamic unbonding periodExcellent work on maintaining consistency by updating the undelegation period information as well. This ensures that users receive accurate information throughout the dialog.
Consider storing the unbonding period in a variable to avoid repeated method calls:
+ const unbondingPeriod = singleStake.getChainUnbondingPeriod(chainID); // ... - Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days. + Staking will lock your funds for {unbondingPeriod} days. // ... - To make your staked assets liquid, undelegation will take {singleStake.getChainUnbondingPeriod(chainID)} days. + To make your staked assets liquid, undelegation will take {unbondingPeriod} days.This small optimization improves readability and ensures consistent values are displayed.
Method Name Correction Not Applied
The method
getAvaiailableAmountis still present infrontend/src/app/(routes)/staking/components/NewDelegationDialog.tsx. Please rename it togetAvailableAmountto ensure consistency and correctness.🔗 Analysis chain
Line range hint
1-150: Verify: Method name correctionThe AI summary mentions a correction of the method name from
getAvaiailableAmounttogetAvailableAmount. However, this change is not visible in the provided code. Could you please verify if this correction has been made?🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of both method names in the file. rg --type typescript 'getAvaiailableAmount|getAvailableAmount' frontend/src/app/\(routes\)/staking/components/NewDelegationDialog.tsxLength of output: 170
Script:
#!/bin/bash # Description: Check for the presence of both method names in the specified file without using the --type flag. rg 'getAvaiailableAmount|getAvailableAmount' "frontend/src/app/(routes)/staking/components/NewDelegationDialog.tsx"Length of output: 186
frontend/src/app/(routes)/staking/components/ReDelegatePopup.tsx (1)
219-219: Improve accuracy of unbonding period informationThe change from a hardcoded value to
singleStake.getChainUnbondingPeriod(chainID)is a good improvement, as it provides chain-specific unbonding periods to users. This enhances the accuracy of the information presented.However, there are a couple of points to consider:
- Error handling: Consider adding a fallback value or error handling in case
getChainUnbondingPeriodfails or returns an unexpected value.- Consistency: The next line still mentions "21+ days". Consider updating this to be consistent with the dynamic value.
To address these points, consider the following changes:
- Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID)} days + Staking will lock your funds for {singleStake.getChainUnbondingPeriod(chainID) || 21} daysAnd for the next line:
- No staking rewards, cancellation of unbonding, or fund withdrawals - until 21+ days post-undelegation. + No staking rewards, cancellation of unbonding, or fund withdrawals + until {singleStake.getChainUnbondingPeriod(chainID) || 21}+ days post-undelegation.These changes ensure a fallback value and maintain consistency throughout the message.
frontend/src/custom-hooks/useSingleStaking.tsx (1)
65-65: Consider making the default unbonding period a configurable constantThe function returns
21as a default unbonding period when the unbonding time is not available. Consider defining this default value as a constant or fetching it from a configuration to improve maintainability and make it easier to update in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- frontend/src/app/(routes)/staking/components/DelegatePopup.tsx (1 hunks)
- frontend/src/app/(routes)/staking/components/NewDelegationDialog.tsx (1 hunks)
- frontend/src/app/(routes)/staking/components/ReDelegatePopup.tsx (1 hunks)
- frontend/src/app/(routes)/staking/components/UndelegatePopup.tsx (1 hunks)
- frontend/src/custom-hooks/useSingleStaking.tsx (4 hunks)
- frontend/src/utils/util.ts (0 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/utils/util.ts
🔇 Additional comments (2)
frontend/src/app/(routes)/staking/components/DelegatePopup.tsx (1)
124-130: Verify the implementation ofgetChainUnbondingPeriodmethodThe changes improve the component by using a dynamic value for the unbonding period. To ensure the accuracy of the displayed information, it's important to verify the implementation of the
getChainUnbondingPeriodmethod in theuseSingleStakinghook.Please run the following script to check the implementation of
getChainUnbondingPeriod:This script will help us verify:
- The correct implementation of
getChainUnbondingPeriod.- Any remaining instances of hardcoded unbonding periods that might need to be replaced.
Please review the results to ensure the method is implemented correctly and returns the appropriate values for different chain IDs.
frontend/src/app/(routes)/staking/components/NewDelegationDialog.tsx (1)
Line range hint
1-150: Summary: PR Objective AchievementThe changes made in this file successfully address the PR objective of adding unbonding period time to staking. The implementation dynamically retrieves and displays the unbonding period for both staking and undelegation processes, enhancing the accuracy and flexibility of the information presented to users.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
getAvaiailableAmounttogetAvailableAmountfor accurate functionality.Enhancements