Fix voting power to dev #1168
Fix voting power to dev #1168
Conversation
fix(components): chart voting power
WalkthroughThe changes in this pull request primarily involve the introduction of a new utility function Changes
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/lib/utils/validator.ts (1)
34-52: LGTM: NewgetStakingAssetInfofunction implemented correctly.The function is well-implemented and serves its purpose of retrieving asset information based on a staking denomination. The use of optional parameters and return type is appropriate.
A couple of minor points to consider:
- The comment on line 39 mentions "initia case". Is this a typo for "initial case", or is it specific to your project?
- The default precision value is set to 6. Please verify if this is the correct default value for your project.
Consider clarifying the comment on line 39 and double-checking the default precision value to ensure it aligns with your project's requirements.
src/lib/pages/validator-details/components/bonded-token-changes/VotingPowerChartDetails.tsx (1)
50-61: LGTM: Improved performance and precision withuseMemoThe implementation of
useMemoforcompareVotingPowercalculation is a good optimization. The new logic accurately considers 24 hours of historical data when available, with a safe fallback to zero.Consider adding a check for the array length before accessing the 25th last item to prevent potential errors with shorter historical data:
const yesterdayVotingPower = historicalPowers.items.length >= 25 ? historicalPowers.items[historicalPowers.items.length - 25].votingPower : big(0);This change would make the code more robust against potential edge cases with limited historical data.
src/lib/pages/validators/components/validators-table/ValidatorsTableRow.tsx (2)
30-30: LGTM: Props interface updated correctly.The change from
denomToken: Option<TokenWithValue>toassetInfo: Option<AssetInfo>is consistent with the overall refactoring. This update improves type safety and aligns with the new asset information handling approach.Consider adding a comment explaining the purpose of
assetInfofor better code documentation:/** Information about the asset used for voting power calculations */ assetInfo: Option<AssetInfo>;
96-101: LGTM: JSX updated correctly to useassetInfo.The changes in the JSX are consistent with the shift from
denomTokentoassetInfo. The use of optional chaining is maintained, which is good for handling potential null values. ThegetTokenLabelfunction now correctly receivesassetInfo.idandassetInfo.symbol.For consistency, consider using optional chaining for
assetInfo.symbolas well:? ` ${getTokenLabel(assetInfo.id, assetInfo?.symbol)}`This ensures that the code handles cases where
symbolmight be undefined, maintaining consistency with howidis accessed.src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsx (1)
27-27: LGTM: Props interface updated correctly.The change from
denomToken: Option<TokenWithValue>toassetInfo: Option<AssetInfo>is consistent with the overall refactoring. This update improves type safety and aligns with the new asset information structure.Consider adding a comment explaining the
AssetInfotype for better code documentation:/** Asset information including precision, symbol, and other relevant details */ assetInfo: Option<AssetInfo>;src/lib/pages/validator-details/components/validator-overview/VotingPowerOverview.tsx (1)
102-102: LGTM: Improved asset info retrieval.The use of
getStakingAssetInfosimplifies the code and potentially improves error handling. This change aligns with the goal of streamlining asset information retrieval across components.Consider adding a comment explaining the purpose of
getStakingAssetInfofor better code documentation:// Retrieve staking asset info using the new utility function const assetInfo = getStakingAssetInfo(singleStakingDenom, assetInfos);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/lib/pages/validator-details/components/bonded-token-changes/VotingPowerChart.tsx (2 hunks)
- src/lib/pages/validator-details/components/bonded-token-changes/VotingPowerChartDetails.tsx (4 hunks)
- src/lib/pages/validator-details/components/validator-overview/VotingPowerOverview.tsx (2 hunks)
- src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsx (3 hunks)
- src/lib/pages/validators/components/validators-table/ValidatorsTableRow.tsx (4 hunks)
- src/lib/pages/validators/components/validators-table/index.tsx (4 hunks)
- src/lib/utils/validator.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (22)
src/lib/utils/validator.ts (2)
4-4: LGTM: Import statement updated correctly.The import statement has been properly updated to include the new types (AssetInfo, AssetInfos, and Option) required for the new
getStakingAssetInfofunction. This change is consistent with the function's implementation and improves type safety.
Line range hint
1-52: Overall, the changes in this file are well-implemented and improve code reusability.The introduction of the
getStakingAssetInfofunction centralizes asset information retrieval, which aligns with the PR's objective of fixing voting power-related issues. This change will likely improve maintainability and reduce code duplication across the project.The implementation is type-safe, handles edge cases appropriately, and doesn't modify existing code, minimizing the risk of introducing new bugs. These changes provide a solid foundation for addressing the voting power issues mentioned in the PR description.
src/lib/pages/validator-details/components/bonded-token-changes/VotingPowerChartDetails.tsx (3)
3-3: LGTM: Import ofuseMemoaddedThe addition of
useMemoimport is appropriate for the performance optimization implemented in this component.
Line range hint
40-47: LGTM: Variable renamed for clarityThe renaming of
currentPricetocurrentVotingPowerimproves code readability by more accurately reflecting the nature of the stored data. The calculation logic remains correct and consistent with the previous implementation.
76-76: LGTM: Consistent variable usage in component outputThe update from
currentPricetocurrentVotingPowerin the component's output is consistent with the earlier variable renaming, ensuring that the correct value is displayed in the UI.src/lib/pages/validators/components/validators-table/ValidatorsTableRow.tsx (3)
9-9: LGTM: Import statement updated correctly.The addition of
AssetInfoto the import statement is consistent with the changes in the component's props and aligns with the overall refactoring to useAssetInfoinstead ofTokenWithValue.
40-40: LGTM: Component parameter updated correctly.The change from
denomTokentoassetInfoin the destructured parameters is consistent with the updated props interface. This ensures that the component function signature matches the new props structure.
Line range hint
1-134: Summary: Successful refactoring to useassetInfoinstead ofdenomToken.The changes in this file successfully refactor the
ValidatorsTableRowcomponent to useassetInfoinstead ofdenomToken. This modification aligns with the PR objectives and the changes described in the AI-generated summary. The refactoring improves the component by using a more appropriate data structure for asset information, which should enhance maintainability and consistency across the application.Key points:
- Import statements, props interface, and component parameters have been updated correctly.
- JSX changes are consistent with the new
assetInfostructure.- Type safety is maintained through the use of optional chaining and nullish coalescing.
The suggested minor improvements (adding a comment for
assetInfoprop and using optional chaining forassetInfo.symbol) would further enhance code quality and consistency.Overall, these changes contribute positively to the "Fix voting power to dev" objective of the pull request.
src/lib/pages/validators/components/validators-table/index.tsx (5)
14-14: LGTM: New import statement is appropriate.The addition of the
getStakingAssetInfoimport is consistent with the changes in the component logic and follows good practices by importing utility functions from a centralized location.
69-69: Great refactoring: Improved code clarity and maintainability.The introduction of
assetInfousing thegetStakingAssetInfofunction is a positive change. It encapsulates the logic for obtaining asset information, simplifying the component and improving code readability. This refactoring aligns well with the principle of keeping components focused on their primary responsibilities.
Line range hint
1-126: Overall: Excellent refactoring that improves code quality.The changes in this file significantly improve code clarity and maintainability by introducing the
getStakingAssetInfoutility function and consistently updating its usage across the component and its children. These modifications align well with the PR objectives of fixing the voting power in the chart component.To ensure full compatibility:
- Verify that the ValidatorsTableMobileCard component has been updated to use
assetInfo.- Verify that the ValidatorsTableRow component has been updated to use
assetInfo.Great job on this refactoring!
108-108: Prop update is consistent: Verify ValidatorsTableRow implementation.The change from
denomTokentoassetInfoin the ValidatorsTableRow props is consistent with the earlier modifications. This ensures that the table row component receives the correct data structure.To ensure full compatibility, please verify that the ValidatorsTableRow component has been updated to use
assetInfoinstead ofdenomToken. Run the following script to check the implementation:#!/bin/bash # Description: Verify ValidatorsTableRow implementation # Test: Check if ValidatorsTableRow uses assetInfo prop rg -A 5 'interface ValidatorsTableRowProps' src/lib/pages/validators/components/validators-table/ValidatorsTableRow.tsx rg 'assetInfo:' src/lib/pages/validators/components/validators-table/ValidatorsTableRow.tsx
83-83: Prop update is consistent: Verify ValidatorsTableMobileCard implementation.The change from
denomTokentoassetInfoin the ValidatorsTableMobileCard props is consistent with the earlier modifications. This ensures that the mobile card component receives the correct data structure.To ensure full compatibility, please verify that the ValidatorsTableMobileCard component has been updated to use
assetInfoinstead ofdenomToken. Run the following script to check the implementation:✅ Verification successful
Validation Successful:
ValidatorsTableMobileCardusesassetInfoprop.The
assetInfoprop inValidatorsTableMobileCard.tsxis correctly implemented, ensuring the component receives the appropriate data structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ValidatorsTableMobileCard implementation # Test: Check if ValidatorsTableMobileCard uses assetInfo prop rg -A 5 'interface ValidatorsTableMobileCardProps' src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsx rg 'assetInfo:' src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsxLength of output: 580
Script:
#!/bin/bash # Description: Verify ValidatorsTableMobileCard implementation with corrected file path # Test 1: Check if ValidatorsTableMobileCard.tsx exists if [ -f src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsx ]; then echo "ValidatorsTableMobileCard.tsx exists." else echo "ValidatorsTableMobileCard.tsx does not exist." exit 1 fi # Test 2: Check if ValidatorsTableMobileCard uses assetInfo prop rg 'assetInfo:' src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsxLength of output: 311
src/lib/pages/validators/components/validators-table/ValidatorsTableMobileCard.tsx (4)
8-8: LGTM: Import statement updated correctly.The addition of
AssetInfoto the import statement is consistent with the changes in the component props and aligns with the overall refactoring to useAssetInfoinstead ofTokenWithValue.
35-35: LGTM: Props destructuring updated correctly.The change from
denomTokentoassetInfoin the props destructuring is consistent with the interface update and ensures that the component uses the newassetInfoprop correctly.
Line range hint
1-160: Overall: Changes look good, with minor suggestions for improvement.The modifications to
ValidatorsTableMobileCard.tsxare consistent with the PR objectives and the AI-generated summary. The changes successfully refactor the component to useAssetInfoinstead ofTokenWithValue, improving consistency across the application.Key points:
- Import statements, props interface, and component implementation are all updated correctly.
- Type safety and null-safety are maintained throughout the changes.
- The refactoring aligns well with the broader changes mentioned in the AI-generated summary.
Minor suggestions:
- Consider adding a comment to explain the
AssetInfotype in the props interface.- Verify that the
getTokenLabelfunction correctly handles the newassetInfo.idandassetInfo.symbolarguments.These changes contribute to a more consistent and maintainable codebase.
95-101: LGTM: JSX updated correctly, but verifygetTokenLabelusage.The changes from
denomTokentoassetInfoin the JSX are consistent with the overall refactoring. The null-safety is maintained with the use of optional chaining.Please verify that the
getTokenLabelfunction correctly handlesassetInfo.idandassetInfo.symbolas arguments, as they might have different semantics than the previousdenomToken.denomanddenomToken.symbol.✅ Verification successful
LGTM:
getTokenLabelis consistently used withassetInfo.idandassetInfo.symbolacross the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of getTokenLabel function # Test: Search for the getTokenLabel function definition rg -A 10 'function getTokenLabel' # Test: Search for other usages of getTokenLabel in the codebase rg 'getTokenLabel\('Length of output: 4696
src/lib/pages/validator-details/components/bonded-token-changes/VotingPowerChart.tsx (3)
17-17: LGTM: New utility function import.The import of
getStakingAssetInfofrom 'lib/utils' is correctly placed and seems relevant to the component's functionality.
45-45: LGTM: Improved asset info retrieval.The use of
getStakingAssetInfosimplifies the code and improves maintainability by centralizing the logic for obtaining asset information. This change is consistent with the component's previous behavior and doesn't require any further adjustments in the component.
Line range hint
1-180: Summary: Improved asset info retrieval in VotingPowerChartThe changes in this file enhance the
VotingPowerChartcomponent by introducing thegetStakingAssetInfoutility function. This modification improves code maintainability and readability without altering the component's core functionality. The change aligns with the PR objective of addressing voting power in the chart component by refining the asset information retrieval process.Some suggestions for further improvement:
- Consider adding a comment explaining the purpose of
getStakingAssetInfofor better documentation.- Ensure that error handling in
getStakingAssetInfois robust, as the component relies on its output.To ensure the changes don't introduce any regressions, please run the following verification script:
src/lib/pages/validator-details/components/validator-overview/VotingPowerOverview.tsx (2)
22-22: LGTM: New utility function import added.The addition of
getStakingAssetInfoto the import list is consistent with the changes described in the AI summary. This new utility function will help streamline the retrieval of asset information across components.
Line range hint
1-224: Overall assessment: Positive improvement in code quality.The changes in this file successfully introduce the new
getStakingAssetInfoutility function and integrate it into theVotingPowerOverviewcomponent. This modification enhances code clarity and maintainability by centralizing the logic for retrieving asset information.Key points:
- The changes align with the PR objectives of addressing voting power-related issues.
- The core functionality of the component remains intact.
- Code readability and potential error handling have been improved.
No potential issues or regressions are apparent from these changes. The refactoring appears to be a solid improvement to the codebase.
evilpeach
force-pushed
the
fix/voting-power-dev
branch
from
a1fa610 to
2b55b80
Compare
fix(components): chart voting power
Summary by CodeRabbit
Release Notes
New Features
VotingPowerChartDetailscomponent to better represent voting power changes over time.Improvements
VotingPowerChartDetailscomponent through memoization.Bug Fixes
VotingPowerOverviewcomponent.Documentation