[Feature] Fixed issues with max rewards + improved rewards section for audits #549
Conversation
|
WalkthroughThis update enhances the rewards section for audits, adds a maximum reward cap display for severity if applicable, and improves code modularity by moving certain functions to useCallback hooks. It also refines calculations related to reward splits, fees, and factors, and makes styling adjustments for responsiveness. Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (8)
- packages/web/src/components/VaultCard/VaultCard.tsx (1)
- packages/web/src/components/VaultSeverityRewardCard/VaultSeverityRewardCard.tsx (2)
- packages/web/src/components/VaultSeverityRewardCard/styles.ts (4)
- packages/web/src/hooks/severities/useSeverityRewardInfo.ts (2)
- packages/web/src/hooks/subgraph/vaults/parser.ts (4)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultRewardsSection/VaultRewardsSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultRewardsSection/styles.ts (3)
- packages/web/src/pages/VaultEditor/VaultEditorFormPage/VaultEditorFormPage.tsx (3)
Files ignored due to filter (1)
- packages/web/src/languages/en.json
Files skipped from review due to trivial changes (4)
- packages/web/src/components/VaultSeverityRewardCard/styles.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultRewardsSection/VaultRewardsSection.tsx
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultRewardsSection/styles.ts
- packages/web/src/pages/VaultEditor/VaultEditorFormPage/VaultEditorFormPage.tsx
Review comments generated (11)
- Review: 9
- LGTM: 2
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -267,7 +267,7 @@ export const VaultCard = ({ | |||
| ? millify(totalPaidOutOnAudit?.usd ?? 0) | |||
| : showIntended | |||
| ? millify(vault.amountsInfo?.competitionIntendedAmount?.deposited.usd ?? 0) | |||
| : millify(vault.amountsInfo?.depositedAmount.usd ?? 0)} | |||
| : millify(vault.amountsInfo?.maxRewardAmount.usd ?? 0)} | |||
There was a problem hiding this comment.
The change from depositedAmount to maxRewardAmount seems appropriate given the context. However, it's important to ensure that maxRewardAmount is always defined and has a valid value before this operation to avoid potential runtime errors or incorrect calculations.
- : millify(vault.amountsInfo?.maxRewardAmount.usd ?? 0)}
+ : vault.amountsInfo?.maxRewardAmount ? millify(vault.amountsInfo.maxRewardAmount.usd) : 0}This change ensures that millify function is only called when maxRewardAmount is not null or undefined.
| if (!vault || !vault.description) | ||
| return { rewardPrice: 0, rewardPercentage: 0, rewardCap: 0, rewardColor: INITIAL_SEVERITY_COLOR }; | ||
|
|
||
| const isAudit = vault.description && vault.description["project-metadata"].type === "audit"; | ||
| const showIntendedAmounts = vault.amountsInfo?.showCompetitionIntendedAmount ?? false; | ||
| const SEVERITIES_COLORS = getSeveritiesColorsArray(vault); | ||
|
|
||
| if (vault.version === "v2") { | ||
| const severity = vault.description.severities[severityIndex]; | ||
| if (!severity) return { rewardPrice: 0, rewardPercentage: 0, rewardColor: INITIAL_SEVERITY_COLOR }; | ||
| if (!severity) return { rewardPrice: 0, rewardPercentage: 0, rewardCap: 0, rewardColor: INITIAL_SEVERITY_COLOR }; | ||
|
|
||
| const sumTotalPrices = Object.values(totalPrices).reduce((a, b = 0) => a + b, 0); | ||
| // const maxBountyPercentage = Number(vault.maxBounty) / 10000; // Number between 0 and 1; | ||
| // TODO: remove this when we have the new vault contract version | ||
| const maxBountyPercentage = Number(isAudit ? 10000 : vault.maxBounty) / 10000; | ||
| const rewardPercentage = +severity.percentage * maxBountyPercentage; | ||
| const rewardPercentage = +severity.percentage; | ||
|
|
||
| let rewardPrice: number = 0; | ||
| let rewardCap: number = 0; | ||
| if (vault.multipleVaults && sumTotalPrices) { | ||
| rewardPrice = sumTotalPrices * (rewardPercentage / 100); | ||
| } else if (vault.amountsInfo?.tokenPriceUsd) { | ||
| rewardPrice = | ||
| (showIntendedAmounts | ||
| ? vault.amountsInfo.competitionIntendedAmount?.deposited.tokens ?? 0 | ||
| : Number(formatUnits(vault.honeyPotBalance, vault.stakingTokenDecimals))) * | ||
| : vault.amountsInfo.maxRewardAmount.tokens) * | ||
| (rewardPercentage / 100) * | ||
| vault.amountsInfo?.tokenPriceUsd; | ||
| rewardCap = (severity.capAmount ?? 0) * vault.amountsInfo?.tokenPriceUsd; | ||
| } | ||
|
|
||
| const orderedSeverities = vault.description.severities.map((severity) => severity.percentage).sort((a, b) => a - b); | ||
| const rewardColor: string = SEVERITIES_COLORS[orderedSeverities.indexOf(severity.percentage) ?? 0]; | ||
|
|
||
| return { rewardPrice, rewardPercentage, rewardColor }; | ||
| return { rewardPrice, rewardPercentage, rewardCap, rewardColor }; | ||
| } else { |
There was a problem hiding this comment.
The changes in this section seem to be well thought out. The addition of the rewardCap property and its calculation based on the severity's cap amount is a good improvement. However, there seems to be a potential issue with the calculation of rewardPrice. If vault.multipleVaults is false and vault.amountsInfo?.tokenPriceUsd is undefined, rewardPrice will remain 0. This could lead to incorrect calculations downstream. Consider adding error handling or a default value for vault.amountsInfo?.tokenPriceUsd.
- let rewardPrice: number = 0;
+ let rewardPrice: number = vault.amountsInfo?.tokenPriceUsd ?? DEFAULT_TOKEN_PRICE;| const severity = vault.description.severities[severityIndex]; | ||
| if (!severity) return { rewardPrice: 0, rewardPercentage: 0, rewardColor: INITIAL_SEVERITY_COLOR }; | ||
| if (!severity) return { rewardPrice: 0, rewardPercentage: 0, rewardCap: 0, rewardColor: INITIAL_SEVERITY_COLOR }; | ||
|
|
||
| const sumTotalPrices = Object.values(totalPrices).reduce((a, b = 0) => a + b, 0); | ||
| const rewardPercentage = (Number(vault.rewardsLevels[severity.index]) / 10000) * 100; | ||
|
|
||
| let rewardPrice: number = 0; | ||
| let rewardCap: number = 0; | ||
| if (vault.multipleVaults && sumTotalPrices) { | ||
| rewardPrice = sumTotalPrices * (rewardPercentage / 100); | ||
| } else if (vault.amountsInfo?.tokenPriceUsd) { | ||
| rewardPrice = | ||
| (showIntendedAmounts | ||
| ? vault.amountsInfo.competitionIntendedAmount?.deposited.tokens ?? 0 | ||
| : Number(formatUnits(vault.honeyPotBalance, vault.stakingTokenDecimals))) * | ||
| : vault.amountsInfo.maxRewardAmount.tokens) * | ||
| (rewardPercentage / 100) * | ||
| vault.amountsInfo?.tokenPriceUsd; | ||
| } | ||
|
|
||
| const orderedSeverities = vault.description.severities.map((severity) => severity.index).sort((a, b) => a - b); | ||
| const rewardColor: string = SEVERITIES_COLORS[orderedSeverities.indexOf(severity.index) ?? 0]; | ||
|
|
||
| return { rewardPrice, rewardPercentage, rewardColor }; | ||
| return { rewardPrice, rewardPercentage, rewardCap, rewardColor }; |
There was a problem hiding this comment.
Similar to the previous comment, consider adding error handling or a default value for vault.amountsInfo?.tokenPriceUsd when calculating rewardPrice.
- let rewardPrice: number = 0;
+ let rewardPrice: number = vault.amountsInfo?.tokenPriceUsd ?? DEFAULT_TOKEN_PRICE;| const { rewardPercentage, rewardPrice, rewardCap, rewardColor } = useSeverityRewardInfo(vault, severityIndex); | ||
|
|
||
| const severityName = severity?.name.toLowerCase().replace("severity", "") ?? ""; | ||
| const showCap = vault.version === "v2" && vault.description?.severities.some((sev) => !!sev.capAmount); |
There was a problem hiding this comment.
The useSeverityRewardInfo hook now returns a new property rewardCap. This is used to display the maximum reward cap for each severity level. The condition showCap checks if the vault version is "v2" and if there's a cap amount defined for any severity. If true, an additional column will be displayed in the UI to show the cap amount. This is a good approach to handle different versions of the vault and only show relevant information based on the version.
However, I noticed that you're using optional chaining (?.) on severity, which means it could potentially be undefined. If severity is undefined, severityName would be an empty string. Make sure this case is handled properly in your UI.
| return ( | ||
| <StyledVaultSeverityRewardCard color={rewardColor} noNft={noNft}> | ||
| <Pill isSeverity transparent textColor={rewardColor} text={severityName} /> | ||
| <StyledVaultSeverityRewardCard columns={2 + (noNft ? 0 : 1) + (showCap ? 1 : 0)} color={rewardColor}> | ||
| <div className="severity-name"> | ||
| <Pill isSeverity transparent textColor={rewardColor} text={severityName} /> | ||
| </div> | ||
| <div className="severity-prize"> | ||
| <div> | ||
| <span>{`${rewardPercentage.toFixed(2)}%`}</span> | ||
| <span className="tiny"> {t("ofVault")} </span> | ||
| <span className="tiny"> {t("ofRewards")} </span> | ||
| </div> | ||
| <span className="price">~{`$${formatNumber(rewardPrice)}`}</span> | ||
| </div> | ||
| {showCap && ( | ||
| <> | ||
| {(severity as IVulnerabilitySeverityV2).capAmount ? ( | ||
| <WithTooltip text={t("maxRewardCapExplanation")}> | ||
| <div className="severity-prize"> | ||
| <div className="title-container"> | ||
| <span className="tiny">{t("maxRewardCap")}</span> | ||
| <InfoIcon fontSize="small" /> | ||
| </div> | ||
| <span className="price">~{`$${formatNumber(rewardCap)}`}</span> | ||
| </div> | ||
| </WithTooltip> | ||
| ) : ( | ||
| <div /> | ||
| )} | ||
| </> | ||
| )} |
There was a problem hiding this comment.
This section of code handles the rendering of the severity reward card. It includes the severity name, reward percentage, reward price, and optionally the reward cap and NFT reward card.
The reward cap is only shown if showCap is true, which is determined by the vault version and whether a cap amount is defined for any severity. The tooltip with the explanation of the maximum reward cap is a nice touch for user understanding.
One potential issue here is the casting of severity to IVulnerabilitySeverityV2 at line 37. This assumes that if showCap is true, severity must be of type IVulnerabilitySeverityV2. However, TypeScript does not enforce this at runtime, so there could be a potential type error if severity is not actually of type IVulnerabilitySeverityV2. You might want to add a runtime check before casting to avoid potential errors.
- {(severity as IVulnerabilitySeverityV2).capAmount ? (
+ {('capAmount' in severity && severity.capAmount) ? (This change uses the in operator to check if the capAmount property exists in severity before accessing it, providing an extra layer of safety.
| const isAudit = vault.description?.["project-metadata"].type === "audit"; | ||
|
|
||
| const maxRewardFactor = vault.version === "v1" ? +vault.rewardsLevels[vault.rewardsLevels.length - 1] : +vault.maxBounty; | ||
| console.log(vault); | ||
|
|
||
| const governanceSplit = BigNumber.from(vault.governanceHatRewardSplit).eq(ethers.constants.MaxUint256) | ||
| ? vault.master.defaultGovernanceHatRewardSplit | ||
| : vault.governanceHatRewardSplit; | ||
| const hackerHatsSplit = BigNumber.from(vault.hackerHatRewardSplit).eq(ethers.constants.MaxUint256) | ||
| ? vault.master.defaultHackerHatRewardSplit | ||
| : vault.hackerHatRewardSplit; | ||
|
|
||
| // In v2 vaults the split sum (immediate, vested, committee) is 100%. So we need to calculate the split factor to get the correct values. | ||
| // In v1 this is not a probem. So the factor is 1. | ||
| const splitFactor = vault.version === "v1" ? 1 : (10000 - Number(governanceSplit) - Number(hackerHatsSplit)) / 100 / 100; | ||
|
|
||
| const governanceFee = Number(governanceSplit) / 100 / 100; | ||
| const committeeFee = (Number(vault.committeeRewardSplit) / 100 / 100) * splitFactor; | ||
|
|
||
| let maxRewardFactor = 1; | ||
| if (isAudit) { | ||
| // For audits the max bounty is 100% - governanceFee - committeeFee | ||
| maxRewardFactor = maxRewardFactor - governanceFee - committeeFee; | ||
| } else { | ||
| // maxRewardFactor = vault.version === "v1" ? 10000 : +vault.maxBounty; | ||
| maxRewardFactor = maxRewardFactor - governanceFee - committeeFee; | ||
| } |
There was a problem hiding this comment.
The logic for calculating maxRewardFactor has been updated. Previously, it was directly taken from the vault's rewards levels or max bounty. Now, it's calculated based on whether the vault is an audit or not, and the governance fee and committee fee are subtracted from it. This change seems to be intended to correctly calculate the maximum reward factor considering the fees. However, there's a redundancy in the if-else block as both conditions result in the same calculation. This can be simplified.
- let maxRewardFactor = 1;
- if (isAudit) {
- // For audits the max bounty is 100% - governanceFee - committeeFee
- maxRewardFactor = maxRewardFactor - governanceFee - committeeFee;
- } else {
- // maxRewardFactor = vault.version === "v1" ? 10000 : +vault.maxBounty;
- maxRewardFactor = maxRewardFactor - governanceFee - committeeFee;
- }
+ let maxRewardFactor = 1 - governanceFee - committeeFee;
Deploying with
|
| Latest commit: |
9123729
|
| Status: | ⚡️ Build in progress... |
There was a problem hiding this comment.
Files selected (1)
- packages/web/src/hooks/subgraph/vaults/parser.ts (4)
Files ignored due to filter (1)
- packages/web/src/languages/en.json
Review comments generated (4)
- Review: 4
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -1,6 +1,6 @@ | |||
| import { formatUnits } from "@ethersproject/units"; | |||
| import { IMaster, IPayoutGraph, IUserNft, IVault } from "@hats-finance/shared"; | |||
| import { BigNumber } from "ethers"; | |||
| import { BigNumber, ethers } from "ethers"; | |||
There was a problem hiding this comment.
The import statement has been modified to include ethers from "ethers" package. This is a good change as it allows the use of ethers.constants.MaxUint256 in the code, which is a constant that represents the maximum possible value for an unsigned 256-bit integer. This is used later in the code to check if the governance and hacker reward splits are set to their maximum values.
- import { BigNumber } from "ethers";
+ import { BigNumber, ethers } from "ethers";| const isAudit = vault.description?.["project-metadata"].type === "audit"; | ||
|
|
||
| const maxRewardFactor = vault.version === "v1" ? +vault.rewardsLevels[vault.rewardsLevels.length - 1] : +vault.maxBounty; | ||
| console.log(vault); | ||
|
|
||
| const governanceSplit = BigNumber.from(vault.governanceHatRewardSplit).eq(ethers.constants.MaxUint256) | ||
| ? vault.master.defaultGovernanceHatRewardSplit | ||
| : vault.governanceHatRewardSplit; | ||
| const hackerHatsSplit = BigNumber.from(vault.hackerHatRewardSplit).eq(ethers.constants.MaxUint256) | ||
| ? vault.master.defaultHackerHatRewardSplit | ||
| : vault.hackerHatRewardSplit; | ||
|
|
||
| // In v2 vaults the split sum (immediate, vested, committee) is 100%. So we need to calculate the split factor to get the correct values. | ||
| // In v1 this is not a probem. So the factor is 1. | ||
| const splitFactor = vault.version === "v1" ? 1 : (10000 - Number(governanceSplit) - Number(hackerHatsSplit)) / 100 / 100; | ||
|
|
||
| const governanceFee = Number(governanceSplit) / 100 / 100; | ||
| const committeeFee = (Number(vault.committeeRewardSplit) / 100 / 100) * splitFactor; | ||
|
|
||
| const maxRewardFactor = 1 - governanceFee - committeeFee; | ||
|
|
||
| return { | ||
| ...vault, | ||
| amountsInfo: { | ||
| showCompetitionIntendedAmount: | ||
| vault.description?.["project-metadata"].type === "audit" && | ||
| isAudit && | ||
| vault.description && |
There was a problem hiding this comment.
The logic for determining whether to show the competition intended amount has been updated. Previously, it was based solely on whether the vault type was "audit". Now, it also checks if the start time of the project is more than 48 hours in the future and if there is an intended competition amount defined. This makes the condition more robust and ensures that the competition intended amount is only shown when it is relevant.
- showCompetitionIntendedAmount:
- vault.description?.["project-metadata"].type === "audit" &&
+ showCompetitionIntendedAmount:
+ isAudit &&
vault.description &&
vault.description["project-metadata"].starttime &&
vault.description["project-metadata"].starttime > new Date().getTime() / 1000 + 48 * 3600 && // 48 hours
!!vault.description?.["project-metadata"].intendedCompetitionAmount &&| tokens: +vault.description?.["project-metadata"].intendedCompetitionAmount * maxRewardFactor, | ||
| usd: +vault.description?.["project-metadata"].intendedCompetitionAmount * tokenPrice * maxRewardFactor, |
There was a problem hiding this comment.
The calculation of the maximum reward in tokens and USD has been updated. Previously, the maxRewardFactor was divided by 100 twice, effectively dividing it by 10,000. This was likely done because the maxRewardFactor was expressed as a percentage with two decimal places. However, the new code calculates maxRewardFactor differently, so it no longer needs to be divided by 10,000. This change is correct assuming the new calculation of maxRewardFactor is accurate.
- tokens: +vault.description?.["project-metadata"].intendedCompetitionAmount * (maxRewardFactor / 100 / 100),
- usd:
- +vault.description?.["project-metadata"].intendedCompetitionAmount * tokenPrice * (maxRewardFactor / 100 / 100),
+ tokens: +vault.description?.["project-metadata"].intendedCompetitionAmount * maxRewardFactor,
+ usd: +vault.description?.["project-metadata"].intendedCompetitionAmount * tokenPrice * maxRewardFactor,| tokens: depositedAmountTokens * maxRewardFactor, | ||
| usd: depositedAmountTokens * tokenPrice * maxRewardFactor, |
There was a problem hiding this comment.
The calculation of the maximum reward amount in tokens and USD has been updated in the same way as the maximum reward. The changes are correct given the new calculation of maxRewardFactor.
- tokens: depositedAmountTokens * (maxRewardFactor / 100 / 100),
- usd: depositedAmountTokens * tokenPrice * (maxRewardFactor / 100 / 100),
+ tokens: depositedAmountTokens * maxRewardFactor,
+ usd: depositedAmountTokens * tokenPrice * maxRewardFactor,
Summary by CodeRabbit
VaultSeverityRewardCardcomponent, enhancing user understanding of reward limits.populateVaultsWithPricingfunction, providing more accurate data to users.VaultEditorFormPagecomponent by moving logic into reusable hooks, enhancing performance and maintainability.StyledVaultSeverityRewardCardandStyledRewardsSectioncomponents for better responsiveness and readability.