feat: show validators APY #1633
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several changes across multiple components and hooks in the codebase. Key modifications include reordering parameters in 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: 13
🧹 Outside diff range and nitpick comments (9)
packages/extension-polkagate/src/components/ShowValue.tsx (1)
4-7: Enhance component documentationWhile the JSDoc comment provides basic information, consider expanding it to:
- Document all supported props (value, height, unit, width)
- Provide usage examples
- List all pages where this component is used, not just contributeToCrowdloan
/** - * @description this component is used to show an account balance in some pages like contributeToCrowdloan + * @description A reusable component for displaying values with optional units and loading states + * @param {number|string|BN|null|undefined} value - The value to display + * @param {number} [height=20] - Height of the skeleton loader + * @param {string} [unit] - Optional unit to display after the value + * @param {string} [width='90px'] - Width of the skeleton loader + * @example + * <ShowValue value={balance} unit="DOT" /> + * <ShowValue value={undefined} height={30} width="120px" /> */packages/extension-polkagate/src/hooks/useCurrentEraIndex.ts (1)
16-18: Consider using dot notation for better type safety.While the optional chaining improves null safety, using bracket notation (
['staking']) instead of dot notation (staking) bypasses TypeScript's property checking. This could make typos harder to catch during development.Consider this alternative:
- api?.query['staking']?.['currentEra']().then((i) => { + api?.query.staking?.currentEra().then((i) => { setIndex(Number(i?.toString() || '0')); }).catch(console.error);packages/extension-polkagate/src/components/ShortAddress.tsx (1)
Line range hint
26-57: Consider performance optimizations.The component could benefit from some performance improvements:
- Memoize the decreaseCharactersCount callback with all its dependencies
- Consider using ResizeObserver directly instead of a custom ObserveResize utility for better performance
Here's how to optimize the callback:
- const decreaseCharactersCount = useCallback(() => clipped && setCharactersCount(charactersCount - 1), [charactersCount, clipped]); + const decreaseCharactersCount = useCallback(() => { + if (clipped) { + setCharactersCount((prev) => prev - 1); + } + }, [clipped]);packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx (1)
40-44: Consider enhancing the Div component for better reusabilityWhile the implementation is clean, consider making this component more reusable:
- Move it to a shared components folder if it's used elsewhere
- Add props for customizing height, margin, and color
-const Div = () => ( +interface DivProps { + height?: string; + margin?: string; + color?: string; +} + +const Div = ({ color = 'divider', height = '15px', margin = '3px 10px' }: DivProps) => ( <Grid alignItems='center' item justifyContent='center'> - <Divider orientation='vertical' sx={{ bgcolor: 'divider', height: '15px', m: '3px 10px', width: '1px' }} /> + <Divider orientation='vertical' sx={{ bgcolor: color, height, m: margin, width: '1px' }} /> </Grid> );packages/extension-polkagate/src/util/types.ts (1)
76-76: LGTM! Consider adding JSDoc comments.The optional
apyproperty withstring | nulltype is well-designed for handling various states (loading, not applicable, and actual values). The change is non-breaking and follows TypeScript best practices.Consider adding JSDoc comments to document:
- The purpose of this property
- The format of the APY string (e.g., percentage with decimal places)
- The meaning of null vs undefined
export interface ValidatorInfo extends DeriveStakingQuery { exposure: { own: BN, total: BN, others: Other[] }; accountInfo?: DeriveAccountInfo; + /** Annual Percentage Yield (APY) of the validator + * @remarks + * - undefined: APY is still loading + * - null: APY is not applicable + * - string: APY value as a percentage with decimal places + */ apy?: string | null;packages/extension-polkagate/src/hooks/useValidatorApy.ts (4)
94-96: Add dependency tocalculateValidatorAPYinuseEffectThe
useEffecthook usescalculateValidatorAPY, but it's not included in the dependency array. This might lead to stale closures ifcalculateValidatorAPYchanges.Apply this diff to add the missing dependency:
useEffect(() => { if (!api || !isElected) { return; } calculateValidatorAPY(validatorAddress).catch(console.error); -}, [api, isElected, validatorAddress]); +}, [api, calculateValidatorAPY, isElected, validatorAddress]);
6-6: Remove unnecessary@ts-ignorecommentThe
@ts-ignorecomment suppresses TypeScript errors, which might hide potential issues. If the import is correct and no errors are present, this comment can be removed.Apply this diff to remove the comment:
-//@ts-ignore
14-14: Initialize state with a defined typeThe state
apyis initialized without a default value, which can beundefined. For better type safety, consider initializing it explicitly.Apply this diff to initialize
apywithnull:-const [apy, setApy] = useState<string | null>(); +const [apy, setApy] = useState<string | null>(null);
16-91: Optimize API calls by batching requestsInside the loop, multiple asynchronous API calls are made sequentially, which can be slow. Consider batching these calls to improve performance.
As a follow-up, you might:
- Use
Promise.allto fetcheraReward,eraPoints, andvalidatorExposurein parallel.- Collect all required eras and make bulk requests if the API supports it.
This can significantly reduce the total execution time of the APY calculation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/extension-polkagate/src/components/ShortAddress.tsx(2 hunks)packages/extension-polkagate/src/components/ShowValue.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx(4 hunks)packages/extension-polkagate/src/hooks/getValidatorApy.ts(1 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useCurrentEraIndex.ts(1 hunks)packages/extension-polkagate/src/hooks/useValidatorApy.ts(1 hunks)packages/extension-polkagate/src/hooks/useValidators.ts(4 hunks)packages/extension-polkagate/src/util/ObserveResize.tsx(1 hunks)packages/extension-polkagate/src/util/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/util/ObserveResize.tsx
🔇 Additional comments (10)
packages/extension-polkagate/src/components/ShowValue.tsx (1)
19-19: LGTM!
The function signature formatting change follows common React/TypeScript conventions.
packages/extension-polkagate/src/hooks/useCurrentEraIndex.ts (1)
Line range hint 11-21: Verify error handling and state updates.
The hook's implementation looks good overall, but there are a few considerations:
- The fallback to '0' when
iis undefined might not be the best default for all cases - The error is logged but the state isn't updated on error
- The hook doesn't clean up subscriptions on unmount
Let's verify the error handling pattern in other similar hooks:
packages/extension-polkagate/src/components/ShortAddress.tsx (2)
26-26: Verify updates to component usage after parameter reordering.
The ShortAddress component's parameter order has changed. Ensure all component usages are updated accordingly.
Let's check for any instances that might need updates:
#!/bin/bash
# Description: Find all usages of ShortAddress component
# to verify they're updated with the new parameter order
rg -l 'ShortAddress.*clipped.*charsCount' 3-5:
Avoid disabling TypeScript checks.
Adding @ts-nocheck removes valuable type safety checks. Consider fixing the underlying type issues instead of disabling TypeScript verification completely.
Let's check if there are any TypeScript errors that need to be addressed:
packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx (1)
17-18: LGTM: Import statements are well-structured
The new imports for ShowValue and useValidatorApy are properly organized and align with the new APY functionality.
packages/extension-polkagate/src/hooks/useValidators.ts (4)
108-108: LGTM: Improved error handling
Good addition of the null check for currentEraIndex. This prevents potential runtime errors and makes the code more robust.
Line range hint 155-185: LGTM: Improved validator processing logic
The refactored loop using for...of improves readability and maintainability. The separation of current and waiting validators is now clearer.
157-162:
Complete APY calculation implementation
The commented-out APY calculation code suggests an incomplete feature implementation. This seems to be part of the PR's objective to show validator APY.
- // const apy = await getValidatorApy(api, v, currentEraValidatorsOverview[v].total, validatorPrefs[v].commission, currentEraIndex);
+ const apy = await getValidatorApy(api, v, currentEraValidatorsOverview[v].total, validatorPrefs[v].commission, currentEraIndex);
current.push(
{
accountId: v as unknown as AccountId,
- // apy,
+ apy,#!/bin/bash
# Check if getValidatorApy function is implemented
rg -A 5 "function getValidatorApy|const getValidatorApy"
# Check if APY is used in the UI
rg -A 5 "apy.*validator"6-7: Verify if @ts-ignore can be avoided
The use of @ts-ignore suppresses TypeScript type checking. Consider investigating if this can be resolved by:
- Updating @polkadot/types package
- Using proper type imports
- Adding necessary type declarations
packages/extension-polkagate/src/hooks/index.ts (1)
119-119: LGTM! The export follows established patterns.
The new useValidatorApy hook export is properly placed alphabetically among other validator-related hooks and follows the established naming conventions.
Let's verify the hook implementation exists:
✅ Verification successful
✓ Hook implementation verified and properly structured
The useValidatorApy hook exists and follows React hooks conventions:
- Exports a default function with the correct name
- Uses React's useState and useCallback hooks
- Takes appropriate parameters (api, validatorAddress, isElected)
- Returns expected APY value type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the useValidatorApy hook implementation exists
# Expected: The hook file should exist and export a default function
# Check if the hook file exists
fd -t f "useValidatorApy.ts" packages/extension-polkagate/src/hooks/
# Verify the hook implementation
ast-grep --pattern 'export default function useValidatorApy' packages/extension-polkagate/src/hooks/useValidatorApy.ts
Length of output: 247
Script:
#!/bin/bash
# Let's verify the actual implementation of the hook
rg -A 5 "export default function useValidatorApy" packages/extension-polkagate/src/hooks/useValidatorApy.ts
Length of output: 419
packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/hooks/useValidatorApy.ts (1)
115-125: Improve error handling and add cleanup.The current implementation only logs errors to console. Consider:
- Adding proper error handling
- Adding a cleanup function to cancel pending operations
Apply this diff:
useEffect(() => { + let mounted = true; + if (!api || isElected === undefined) { return; } if (isElected === false) { return setApy(null); } - calculateValidatorAPY(validatorAddress).catch(console.error); + calculateValidatorAPY(validatorAddress) + .then((result) => { + if (mounted) { + // Handle successful calculation + } + }) + .catch((error) => { + console.error('Failed to calculate validator APY:', error); + if (mounted) { + setApy(null); + } + }); + + return () => { + mounted = false; + }; }, [api, calculateValidatorAPY, isElected, validatorAddress]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-polkagate/src/hooks/useValidatorApy.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/hooks/useValidatorApy.ts
[error] 108-108: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
[error] 108-108: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-polkagate/src/hooks/useValidatorApy.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/hooks/useValidatorApy.ts
[error] 110-110: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
[error] 110-110: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
packages/extension-polkagate/src/hooks/useValidatorApy.ts (2)
6-7:
Remove @ts-ignore directive and properly import types.
The @ts-ignore directive suppresses TypeScript errors and should be avoided. Instead, properly import the required types.
Apply this diff:
-//@ts-ignore
-import type { PalletStakingActiveEraInfo, PalletStakingEraRewardPoints, PalletStakingValidatorPrefs, SpStakingPagedExposureMetadata } from '@polkadot/types/lookup';
+import type {
+ PalletStakingActiveEraInfo,
+ PalletStakingEraRewardPoints,
+ PalletStakingValidatorPrefs,
+ SpStakingPagedExposureMetadata
+} from '@polkadot/types/lookup';Likely invalid or redundant comment.
106-108:
Prevent precision loss in APY calculation.
Converting BN instances to numbers using toNumber() can lead to precision loss with large values. Perform calculations using BN methods instead.
Apply this diff:
-const dailyReturn = dailyReward.toNumber() / totalStaked.toNumber();
-
-const APY = (dailyReturn * 365 * 100).toFixed(2);
+if (!totalStaked.isZero()) {
+ const dailyReturn = dailyReward.mul(BN_HUNDRED).muln(365).div(totalStaked);
+ const APY = dailyReturn.toString();
+ setApy(APY);
+} else {
+ setApy('0');
+}Likely invalid or redundant comment.
# [0.24.0](v0.23.1...v0.24.0) (2024-11-10) ### Features * show validators APY ([#1633](#1633)) ([cdc4b37](cdc4b37))
Summary by CodeRabbit
Release Notes
New Features
useValidatorApyto calculate the annual percentage yield (APY) for validators.ShowValidatorcomponent to display APY based on validator status.pxproperty to theDraggableModalfor customizable horizontal padding.Improvements
ShowValueanduseCurrentEraIndexcomponents by removing// @ts-nocheckdirective.useValidatorshook to ensure API calls are made only whencurrentEraIndexis defined.apyproperty to theValidatorInfointerface for better data representation.Bug Fixes
ShowValidatorfor determining validator election status.Style
ValidatorInfoPagefor better layout.