[Feature] Submissions page on vault details + fixed max rewards #554
Conversation
Bumps [graphql](https://github.com/graphql/graphql-js) from 16.6.0 to 16.8.1. - [Release notes](https://github.com/graphql/graphql-js/releases) - [Commits](graphql/graphql-js@v16.6.0...v16.8.1) --- updated-dependencies: - dependency-name: graphql dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>
[Feature] New submissions section on VaultDetailsPage
[Feature] Fixed issues with max rewards + improved rewards section for audits
|
WalkthroughThis comprehensive update introduces new functionalities and enhancements across the application. It modifies the VaultCard component, improves token price fetching in VaultsProvider, adjusts UI presentation and rendering logic in VaultRewardsSection, adds a submissions section to the vault details page, and includes validation changes in the getEditedDescriptionYupSchema function. 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. |
Deploying with
|
| Latest commit: |
8aca204
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84cc8fbf.dapp-a9y.pages.dev |
| Branch Preview URL: | https://develop.dapp-a9y.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (26)
- 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/constants/constants.ts (1)
- packages/web/src/hooks/severities/useSeverityRewardInfo.ts (2)
- packages/web/src/hooks/subgraph/vaults/parser.ts (4)
- packages/web/src/pages/CommitteeTools/SubmissionsTool/SubmissionDetailsPage/SubmissionDetailsPage.tsx (2)
- 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/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/EnvSetupSection/EnvSetupSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/InScopeSection/InScopeSection.tsx (4)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/OutOfScopeSection/OutOfScopeSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/SeverityLevelsSection/SeverityLevelsSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/PublicSubmissionCard/PublicSubmissionCard.tsx (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/PublicSubmissionCard/styles.ts (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/VaultSubmissionsSection.tsx (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/styles.ts (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/index.ts (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/VaultDetailsPage.tsx (5)
- packages/web/src/pages/Honeypots/VaultDetailsPage/hooks.ts (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/savedSubmissionsService.ts (1)
- packages/web/src/pages/Honeypots/VaultDetailsPage/styles.ts (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/types.ts (1)
- packages/web/src/pages/Submissions/SubmissionFormPage/FormSteps/SubmissionSubmit/components/SubmissionReview/SubmissionReview.tsx (2)
- packages/web/src/pages/VaultEditor/VaultEditorFormPage/VaultEditorFormPage.tsx (3)
- packages/web/src/utils/contractsCovered.utils.ts (1)
Files ignored due to filter (1)
- packages/web/src/languages/en.json
Files skipped from review due to trivial changes (10)
- packages/web/src/components/VaultSeverityRewardCard/styles.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultRewardsSection/styles.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/OutOfScopeSection/OutOfScopeSection.tsx
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/PublicSubmissionCard/styles.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/styles.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/index.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/hooks.ts
- packages/web/src/pages/Honeypots/VaultDetailsPage/types.ts
- packages/web/src/pages/VaultEditor/VaultEditorFormPage/VaultEditorFormPage.tsx
- packages/web/src/utils/contractsCovered.utils.ts
Review comments generated (35)
- Review: 31
- LGTM: 4
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 considering the context. It appears that the maximum reward amount is now being displayed instead of the deposited amount, which could provide more accurate information to the user about potential rewards.
- : millify(vault.amountsInfo?.depositedAmount.usd ?? 0)}
+ : millify(vault.amountsInfo?.maxRewardAmount.usd ?? 0)}| @@ -3,7 +3,7 @@ import LinkIcon from "@mui/icons-material/InsertLinkOutlined"; | |||
| import MDEditor from "@uiw/react-md-editor"; | |||
| import { Alert, Button, HatSpinner, WalletButton } from "components"; | |||
| import { useKeystore } from "components/Keystore"; | |||
| import { IPFS_PREFIX, disallowedElementsMarkdown } from "constants/constants"; | |||
| import { IPFS_PREFIX, allowedElementsMarkdown } from "constants/constants"; | |||
There was a problem hiding this comment.
The change from disallowedElementsMarkdown to allowedElementsMarkdown seems to indicate a shift in the approach for handling markdown rendering. This could potentially introduce security vulnerabilities if not all elements in allowedElementsMarkdown are safe. Please ensure that only safe HTML elements are included in allowedElementsMarkdown.
- import { IPFS_PREFIX, disallowedElementsMarkdown } from "constants/constants";
+ import { IPFS_PREFIX, allowedElementsMarkdown } from "constants/constants";| export const allowedElementsMarkdown = [ | ||
| "h1", | ||
| "h2", | ||
| "h3", | ||
| "h4", | ||
| "h5", | ||
| "h6", | ||
| "p", | ||
| "a", | ||
| "ul", | ||
| "ol", | ||
| "li", | ||
| "strong", | ||
| "em", | ||
| "s", | ||
| "code", | ||
| "blockquote", | ||
| "pre", | ||
| "img", | ||
| "hr", | ||
| "br", | ||
| "table", | ||
| "thead", | ||
| "tbody", | ||
| "tr", | ||
| "th", | ||
| "td", | ||
| "caption", | ||
| "div", | ||
| "span", | ||
| ]; |
There was a problem hiding this comment.
The change from disallowedElementsMarkdown to allowedElementsMarkdown is a good security practice. By specifying only the allowed elements, you are implementing a whitelist approach which is generally safer than a blacklist approach. However, please ensure that all necessary HTML elements for your markdown rendering are included in this list. If an element is not included and it's used in the markdown, it won't be rendered correctly.
Also, consider adding some form of error handling or notification if a disallowed element is encountered in the markdown. This could help with debugging and user experience.
+ export const allowedElementsMarkdown = [
+ "h1",
+ "h2",
+ "h3",
+ "h4",
+ "h5",
+ "h6",
+ "p",
+ "a",
+ "ul",
+ "ol",
+ "li",
+ "strong",
+ "em",
+ "s",
+ "code",
+ "blockquote",
+ "pre",
+ "img",
+ "hr",
+ "br",
+ "table",
+ "thead",
+ "tbody",
+ "tr",
+ "th",
+ "td",
+ "caption",
+ "div",
+ "span",
+ ];| <h4 className="value">~${millify(vault.amountsInfo?.maxRewardAmount.usd ?? 0)}</h4> | ||
| )} | ||
| </div> | ||
| </div> | ||
| <div className="division"> | ||
| <div className="card"> | ||
| <h4 className="title">{t("rewardsDivision")}</h4> | ||
| <div className="chart-container"> | ||
| <VaultRewardDivision vault={vault} /> | ||
| {!isAudit && ( | ||
| <div className="division"> | ||
| <div className="card"> | ||
| <h4 className="title">{t("rewardsDivision")}</h4> | ||
| <div className="chart-container"> | ||
| <VaultRewardDivision vault={vault} /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
The logic for displaying the maximum reward amount has been updated. The old code had a TODO comment indicating that the deposited amount should not be displayed, but only the max reward amount. This has been fixed in the new code. However, it's important to ensure that vault.amountsInfo?.maxRewardAmount.usd is always available and correctly calculated to avoid displaying incorrect information.
- ~${millify(vault.amountsInfo?.depositedAmount.usd ?? 0)}
+ ~${millify(vault.amountsInfo?.maxRewardAmount.usd ?? 0)}Also, the condition for displaying the "rewardsDivision" section has been changed to exclude audits. Make sure this change aligns with the business requirements.
- <div className="division">
+ {!isAudit && (
+ <div className="division">| @@ -1,7 +1,7 @@ | |||
| import { IVault } from "@hats-finance/shared"; | |||
| import MDEditor from "@uiw/react-md-editor"; | |||
| import { Pill } from "components"; | |||
| import { disallowedElementsMarkdown } from "constants/constants"; | |||
| import { allowedElementsMarkdown } from "constants/constants"; | |||
There was a problem hiding this comment.
The import statement has been changed from disallowedElementsMarkdown to allowedElementsMarkdown. This change is fine as long as the new constant allowedElementsMarkdown is defined and contains the correct elements that are allowed in markdown rendering. If not, this could lead to unexpected behavior or security issues if unsafe HTML elements are allowed.
- import { disallowedElementsMarkdown } from "constants/constants";
+ import { allowedElementsMarkdown } from "constants/constants";| 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 logic for calculating the reward price and cap seems to be duplicated in both the if and else blocks. This could be refactored to reduce code duplication. Here's a suggestion:
- if (vault.version === "v2") {
- const severity = vault.description.severities[severityIndex];
- 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 = +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
- : 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, rewardCap, rewardColor };
- } else {
- const severity = vault.description.severities[severityIndex];
- 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
- : 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, rewardCap, rewardColor };
- }
+ const severity = vault.description.severities[severityIndex];
+ 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 = vault.version === "v2" ? +severity.percentage : (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
+ : vault.amountsInfo.maxRewardAmount.tokens) *
+ (rewardPercentage / 100) *
+ vault.amountsInfo?.tokenPriceUsd;
+ rewardCap = vault.version === "v2" ? (severity.capAmount ?? 0) * vault.amountsInfo?.tokenPriceUsd : 0;
+ }
+
+ 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, rewardCap, rewardColor };This way, the logic for calculating rewardPrice and rewardCap is only written once, and we just adjust the rewardPercentage and rewardCap based on the vault version.
| import { useMemo, useState } from "react"; | ||
| import { useTranslation } from "react-i18next"; | ||
| import { redirect, useNavigate, useParams } from "react-router-dom"; | ||
| import { useNavigate, useParams } from "react-router-dom"; | ||
| import { HoneypotsRoutePaths } from "../router"; | ||
| import { VaultDepositsSection, VaultRewardsSection, VaultScopeSection } from "./Sections"; | ||
| import { VaultDepositsSection, VaultRewardsSection, VaultScopeSection, VaultSubmissionsSection } from "./Sections"; | ||
| import { useSavedSubmissions } from "./hooks"; |
There was a problem hiding this comment.
The import of useMemo is new in this hunk. It's used to optimize the performance by memorizing the result of expensive function calls and reusing them on subsequent calls if the dependencies haven't changed. The useState hook is also being used here to manage the state of the component. The useSavedSubmissions hook is imported to fetch saved submissions for the vault. These changes seem to be appropriate and well-implemented.
-import { useState } from "react";
+import { useMemo, useState } from "react";
-import { VaultDepositsSection, VaultRewardsSection, VaultScopeSection } from "./Sections";
+import { VaultDepositsSection, VaultRewardsSection, VaultScopeSection, VaultSubmissionsSection } from "./Sections";
+import { useSavedSubmissions } from "./hooks";| const isAudit = vault?.description?.["project-metadata"].type === "audit"; | ||
|
|
||
| const { data: savedSubmissions } = useSavedSubmissions(vault); | ||
|
|
||
| const [openSectionId, setOpenSectionId] = useState(sectionId ?? DETAILS_SECTIONS[0].title); | ||
|
|
||
| const DETAILS_SECTIONS_TO_SHOW = useMemo( | ||
| () => | ||
| DETAILS_SECTIONS.filter((section) => { | ||
| if (section.title === "deposits" && noActions) return false; | ||
| if (section.title === "submissions" && (!isAudit || savedSubmissions?.length === 0)) return false; | ||
| return true; | ||
| }), | ||
| [noActions, isAudit, savedSubmissions] | ||
| ); | ||
|
|
||
| if (allVaults?.length === 0) return <Loading extraText={`${t("loadingVaultDetails")}...`} />; | ||
| if (!vault || !vault.description) { | ||
| redirect("/"); | ||
| return null; | ||
| return <Loading extraText={`${t("loadingVaultDetails")}...`} />; | ||
| } |
There was a problem hiding this comment.
The isAudit variable is now defined earlier in the code, which is a good practice as it improves readability. The useSavedSubmissions hook is used to fetch saved submissions for the vault. A new constant DETAILS_SECTIONS_TO_SHOW is created using useMemo to filter out sections based on certain conditions. This is a good use of useMemo to avoid unnecessary computations on every render. The error handling has been updated to show a loading screen instead of redirecting to the home page if the vault or its description is not available. This seems like a better user experience.
- const isAudit = vault.description["project-metadata"].type === "audit";
+ const isAudit = vault?.description?.["project-metadata"].type === "audit";
+ const { data: savedSubmissions } = useSavedSubmissions(vault);
+ const [openSectionId, setOpenSectionId] = useState(sectionId ?? DETAILS_SECTIONS[0].title);
+ const DETAILS_SECTIONS_TO_SHOW = useMemo(
+ () =>
+ DETAILS_SECTIONS.filter((section) => {
+ if (section.title === "deposits" && noActions) return false;
+ if (section.title === "submissions" && (!isAudit || savedSubmissions?.length === 0)) return false;
+ return true;
+ }),
+ [noActions, isAudit, savedSubmissions]
+ );
+ if (allVaults?.length === 0) return <Loading extraText={`${t("loadingVaultDetails")}...`} />;
+ if (!vault || !vault.description) {
+ return <Loading extraText={`${t("loadingVaultDetails")}...`} />;
+ }| @@ -70,7 +86,7 @@ | |||
| return ( | |||
| <> | |||
| <Seo title={`${vaultName} ${isAudit ? t("auditCompetition") : t("bugBounty")}`} /> | |||
| <StyledVaultDetailsPage className="content-wrapper" isAudit={isAudit}> | |||
| <StyledVaultDetailsPage className="content-wrapper" isAudit={isAudit} tabsNumber={DETAILS_SECTIONS_TO_SHOW.length}> | |||
There was a problem hiding this comment.
The StyledVaultDetailsPage component now receives an additional prop tabsNumber, which represents the number of tabs to be displayed. This allows for dynamic rendering of tabs based on the number of sections to show. This is a good practice as it makes the component more flexible and adaptable to changes in the data.
- <StyledVaultDetailsPage className="content-wrapper" isAudit={isAudit}>
+ <StyledVaultDetailsPage className="content-wrapper" isAudit={isAudit} tabsNumber={DETAILS_SECTIONS_TO_SHOW.length}>| @@ -91,7 +107,7 @@ | |||
| </div> | |||
|
|
|||
| <div className="sections-tabs"> | |||
| {DETAILS_SECTIONS.filter((section) => (noActions ? section.title !== "deposits" : true)).map((section) => ( | |||
| {DETAILS_SECTIONS_TO_SHOW.map((section) => ( | |||
There was a problem hiding this comment.
The DETAILS_SECTIONS_TO_SHOW array is now used to render the section tabs instead of filtering the DETAILS_SECTIONS array directly in the JSX. This is a good practice as it improves readability and performance by avoiding unnecessary computations during rendering.
- {DETAILS_SECTIONS.filter((section) => (noActions ? section.title !== "deposits" : true)).map((section) => (
+ {DETAILS_SECTIONS_TO_SHOW.map((section) => (
…ntsMarkdown Removed disallowedElementsMarkdown
There was a problem hiding this comment.
Files selected (1)
- packages/web/src/components/FormControls/FormMDEditor/FormMDEditor.tsx (2)
Files skipped from review due to trivial changes (1)
- packages/web/src/components/FormControls/FormMDEditor/FormMDEditor.tsx
Review comments generated (0)
- Review: 0
- 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.
…phql-16.8.1 Bump graphql from 16.6.0 to 16.8.1
|
|
allowed elements now in shared lib
There was a problem hiding this comment.
Files selected (11)
- packages/shared/src/constants/mdAllowedElements.ts (1)
- packages/shared/src/index.ts (1)
- packages/web/src/components/FormControls/FormMDEditor/FormMDEditor.tsx (2)
- packages/web/src/constants/constants.ts (1)
- packages/web/src/pages/CommitteeTools/SubmissionsTool/SubmissionDetailsPage/SubmissionDetailsPage.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/EnvSetupSection/EnvSetupSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/InScopeSection/InScopeSection.tsx (5)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/OutOfScopeSection/OutOfScopeSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultScopeSection/SeverityLevelsSection/SeverityLevelsSection.tsx (2)
- packages/web/src/pages/Honeypots/VaultDetailsPage/Sections/VaultSubmissionsSection/PublicSubmissionCard/PublicSubmissionCard.tsx (1)
- packages/web/src/pages/Submissions/SubmissionFormPage/FormSteps/SubmissionSubmit/components/SubmissionReview/SubmissionReview.tsx (2)
Files ignored due to filter (3)
- packages/shared/package.json
- packages/web/package.json
- yarn.lock
Files skipped from review due to trivial changes (4)
- packages/shared/src/constants/mdAllowedElements.ts
- packages/shared/src/index.ts
- packages/web/src/constants/constants.ts
- packages/web/src/pages/Submissions/SubmissionFormPage/FormSteps/SubmissionSubmit/components/SubmissionReview/SubmissionReview.tsx
Review comments generated (17)
- Review: 14
- LGTM: 3
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.
| @@ -20,7 +19,7 @@ export const EnvSetupSection = ({ vault }: EnvSetupSectionProps) => { | |||
| </div> | |||
|
|
|||
| <MDEditor.Markdown | |||
| disallowedElements={disallowedElementsMarkdown} | |||
| allowedElements={allowedElementsMarkdown} | |||
There was a problem hiding this comment.
The change from disallowedElementsMarkdown to allowedElementsMarkdown could potentially introduce security vulnerabilities if the allowed elements include any that can be used for cross-site scripting (XSS) attacks. Please ensure that the list of allowed elements does not contain any such elements.
- allowedElements={allowedElementsMarkdown}
+ disallowedElements={disallowedElementsMarkdown}| @@ -12,7 +11,7 @@ export const OutOfScopeSection = ({ vault }: OutOfScopeSectionProps) => { | |||
| return ( | |||
| <div className="subsection-container"> | |||
| <MDEditor.Markdown | |||
| disallowedElements={disallowedElementsMarkdown} | |||
| allowedElements={allowedElementsMarkdown} | |||
There was a problem hiding this comment.
The change from disallowedElementsMarkdown to allowedElementsMarkdown is a significant one. It changes the approach of markdown rendering from a blacklist approach (disallowing certain elements) to a whitelist approach (only allowing certain elements). This can be a more secure way of handling user-generated content, as it ensures that only known-safe elements are allowed. However, this could potentially break existing markdown that uses elements not included in allowedElementsMarkdown. Please ensure that all necessary elements are included in allowedElementsMarkdown and that this change is thoroughly tested.
- disallowedElements={disallowedElementsMarkdown}
+ allowedElements={allowedElementsMarkdown}| @@ -20,7 +19,7 @@ export const SeverityLevelsSection = ({ vault }: SeverityLevelsSectionProps) => | |||
| <div className="severity" key={idx}> | |||
| <Pill isSeverity transparent text={severity.name} textColor={severityColors[idx]} /> | |||
| <MDEditor.Markdown | |||
| disallowedElements={disallowedElementsMarkdown} | |||
| allowedElements={allowedElementsMarkdown} | |||
There was a problem hiding this comment.
The change from disallowedElementsMarkdown to allowedElementsMarkdown could potentially introduce security vulnerabilities if the allowed elements include any that can be used for cross-site scripting (XSS) attacks. Please ensure that the list of allowed elements does not contain any such elements.
- allowedElements={allowedElementsMarkdown}
+ disallowedElements={disallowedElementsMarkdown}This is just a precautionary measure, please revert back to allowedElementsMarkdown if you are sure about the safety of all the elements in the list.
| @@ -1,9 +1,10 @@ | |||
| import { allowedElementsMarkdown } from "@hats-finance/shared"; | |||
There was a problem hiding this comment.
The import statement has been changed from disallowedElementsMarkdown to allowedElementsMarkdown. This change could potentially affect the rendering of markdown content. Ensure that the new allowedElementsMarkdown constant includes all necessary HTML elements and does not include any potentially unsafe elements that could lead to issues such as cross-site scripting (XSS) attacks.
- import { IPFS_PREFIX, disallowedElementsMarkdown } from "constants/constants";
+ import { allowedElementsMarkdown } from "@hats-finance/shared";| @@ -84,7 +85,7 @@ | |||
| <SubmissionCard submission={submission} noActions /> | |||
|
|
|||
| <MDEditor.Markdown | |||
| disallowedElements={disallowedElementsMarkdown} | |||
| allowedElements={allowedElementsMarkdown} | |||
There was a problem hiding this comment.
This line is using the newly imported allowedElementsMarkdown in place of the old disallowedElementsMarkdown. As mentioned above, ensure that this change doesn't introduce any potential security vulnerabilities or break any existing functionality.
- disallowedElements={disallowedElementsMarkdown}
+ allowedElements={allowedElementsMarkdown}| @@ -37,7 +37,7 @@ | |||
| > | |||
| <MDEditor | |||
| value={value} | |||
| previewOptions={{ disallowedElements: disallowedElementsMarkdown }} | |||
| previewOptions={{ allowedElements: allowedElementsMarkdown }} | |||
There was a problem hiding this comment.
The previewOptions prop of the MDEditor component has been updated to use allowedElementsMarkdown instead of disallowedElementsMarkdown. This change should be fine as long as allowedElementsMarkdown is correctly defined and includes all necessary elements for markdown rendering.
- previewOptions={{ disallowedElements: disallowedElementsMarkdown }}
+ previewOptions={{ allowedElements: allowedElementsMarkdown }}| @@ -50,7 +50,7 @@ | |||
| getRepoContractsList(); | |||
| }, [vault.description?.scope?.reposInformation]); | |||
|
|
|||
| if (!vault.description) return null; | |||
| if (!vault.description) return <Loading fixed extraText={`${t("loading")}...`} />; | |||
There was a problem hiding this comment.
This change improves user experience by showing a loading spinner when the vault description is not available yet, instead of just returning null and showing nothing.
- if (!vault.description) return null;
+ if (!vault.description) return <Loading fixed extraText={`${t("loading")}...`} />;| const [isOpen, setIsOpen] = useState(false); | ||
|
|
There was a problem hiding this comment.
The useState hook is used here to manage the state of isOpen. However, there's no error handling or validation to ensure that the state updates correctly. Consider adding error handling or validation to ensure robustness.
- const [isOpen, setIsOpen] = useState(false);
+ const [isOpen, setIsOpen] = useState<boolean>(false);| const severityIndex = | ||
| submission.severity && | ||
| vault?.description?.severities.findIndex((sev: IVulnerabilitySeverity) => | ||
| sev.name.toLowerCase().includes(submission.severity ?? "") | ||
| ); |
There was a problem hiding this comment.
This block of code finds the index of the severity in the vault's severities array that matches the submission's severity. However, it doesn't handle the case where the severity isn't found (i.e., findIndex returns -1). This could lead to unexpected behavior later on when this index is used. Consider adding a check for this case.
- const severityIndex =
- submission.severity &&
- vault?.description?.severities.findIndex((sev: IVulnerabilitySeverity) =>
- sev.name.toLowerCase().includes(submission.severity ?? "")
- );
+ let severityIndex = -1;
+ if (submission.severity && vault?.description?.severities) {
+ severityIndex = vault.description.severities.findIndex((sev: IVulnerabilitySeverity) =>
+ sev.name.toLowerCase().includes(submission.severity)
+ );
+ if (severityIndex === -1) {
+ // Handle case where severity isn't found
+ }
+ }| source={submission.issueData.issueDescription | ||
| .replace("\n**Submission hash", "\\\n**Submission hash") | ||
| .replace("\n**Severity:**", "\\\n**Severity:**")} |
There was a problem hiding this comment.
In this section, you're replacing certain substrings in the issue description. It would be more maintainable and less error-prone to use a constant for these strings instead of hardcoding them. This way, if the strings need to be changed in the future, you only need to update them in one place.
- source={submission.issueData.issueDescription
- .replace("\n**Submission hash", "\\\n**Submission hash")
- .replace("\n**Severity:**", "\\\n**Severity:**")}
+ const SUBMISSION_HASH = "\n**Submission hash";
+ const SEVERITY = "\n**Severity:**";
+ source={submission.issueData.issueDescription
+ .replace(SUBMISSION_HASH, `\\${SUBMISSION_HASH}`)
+ .replace(SEVERITY, `\\${SEVERITY}`)}
| import { useAccount, useNetwork } from "wagmi"; | ||
| import * as auditDraftsService from "./auditDraftsService"; | ||
|
|
There was a problem hiding this comment.
The import of useAccount is new in this hunk. Ensure that the useAccount hook is properly implemented and tested, as it's crucial for fetching the user's address.
|
|
||
| useEffect(() => { | ||
| const checkGovMember = async () => { | ||
| if (address && chain && chain.id) { | ||
| const chainId = Number(chain.id); | ||
| const govMultisig = appChains[Number(chainId)]?.govMultisig; | ||
|
|
||
| const isGov = await isAddressAMultisigMember(govMultisig, address, chainId); | ||
| setIsGovMember(isGov); | ||
| } | ||
| }; | ||
| checkGovMember(); | ||
| }, [address, chain]); |
There was a problem hiding this comment.
This block introduces a new effect to check if the current user is a member of the governance multisig. This is a good security measure to ensure only authorized users can access certain features. However, consider handling potential errors that might occur during the isAddressAMultisigMember function call.
useEffect(() => {
const checkGovMember = async () => {
if (address && chain && chain.id) {
const chainId = Number(chain.id);
const govMultisig = appChains[Number(chainId)]?.govMultisig;
try {
const isGov = await isAddressAMultisigMember(govMultisig, address, chainId);
setIsGovMember(isGov);
} catch (error) {
console.error('Error checking multisig membership:', error);
}
}
};
checkGovMember();
}, [address, chain]);
|
|
||
| const auditCompetitionsVaults = | ||
| allVaultsOnEnv | ||
| ?.filter((vault) => vault.registered) | ||
| .filter((vault) => vault.description?.["project-metadata"].type === "audit") ?? []; | ||
|
|
||
| const paidPayoutsFromAudits = allPayoutsOnEnv?.filter( | ||
| (payout) => payout.isApproved && payout.payoutData?.vault?.description?.["project-metadata"].type === "audit" | ||
| ); | ||
| .filter((vault) => vault.description?.["project-metadata"].type === "audit") | ||
| .filter((vault) => { | ||
| const isPrivateAudit = vault.description?.["project-metadata"].isPrivateAudit; | ||
| const isUserInvited = vault.description?.["project-metadata"].whitelist?.some( | ||
| (whiteAddress) => whiteAddress.address.toLowerCase() === profileData?.address?.toLowerCase() | ||
| ); | ||
|
|
||
| return opts.private ? isPrivateAudit && (isUserInvited || isGovMember) : !isPrivateAudit; | ||
| }) ?? []; |
There was a problem hiding this comment.
This block filters vaults based on their registration status, type, and privacy settings. It also checks if the user is invited or is a governance member for private audits. The logic seems correct, but remember to handle potential edge cases where vault.description or ["project-metadata"] might be undefined.
|
|
||
| const paidPayoutsFromAudits = allPayoutsOnEnv | ||
| ?.filter((payout) => payout.isApproved) | ||
| .filter((payout) => payout.payoutData?.vault?.description?.["project-metadata"].type === "audit") | ||
| .filter((payout) => { | ||
| const isPrivateAudit = payout.payoutData?.vault?.description?.["project-metadata"].isPrivateAudit; | ||
| const isUserInvited = payout.payoutData?.vault?.description?.["project-metadata"].whitelist?.some( | ||
| (whiteAddress) => whiteAddress.address.toLowerCase() === profileData?.address?.toLowerCase() | ||
| ); | ||
|
|
||
| return opts.private ? isPrivateAudit && (isUserInvited || isGovMember) : !isPrivateAudit; | ||
| }); |
There was a problem hiding this comment.
This block filters payouts based on their approval status, type, and privacy settings. Similar to the previous comment, ensure to handle potential edge cases where payout.payoutData, vault, description, or ["project-metadata"] might be undefined.
| const isPrivateAudit = vault.description?.["project-metadata"].isPrivateAudit; | ||
|
|
||
| // Don't show the private audit, unless the project is already selected | ||
| if (isPrivateAudit && submissionData?.project?.projectId !== vault.id) return undefined; | ||
|
|
There was a problem hiding this comment.
The logic here is checking if the vault is a private audit and if it's not the currently selected project, then it returns undefined. This could potentially lead to an array with undefined values. It would be better to use Array.filter() before mapping to avoid this.
- const vaultsProjects = activeVaults?.map((vault: IVault, index: number) => {
+ const vaultsProjects = activeVaults?.filter((vault: IVault) => !(vault.description?.["project-metadata"].isPrivateAudit && submissionData?.project?.projectId !== vault.id)).map((vault: IVault, index: number) => {
const projectName = vault.description?.["project-metadata"].name;
- const isPrivateAudit = vault.description?.["project-metadata"].isPrivateAudit;
-
- // Don't show the private audit, unless the project is already selected
- if (isPrivateAudit && submissionData?.project?.projectId !== vault.id) return undefined;
-
if (projectName?.toLowerCase().includes(userInput.toLowerCase()) && !vault.liquidityPool && vault.registered) {
return (
<VaultThis way, you filter out the undesired elements first, and then map over the remaining ones. This prevents the creation of an array with undefined values.
| export async function encryptWithHatsKey(dataToEncrypt: string): Promise<string> { | ||
| try { | ||
| const hatsPublicKeyString = await getHatsPublicKey(); | ||
| if (!hatsPublicKeyString) throw new Error("Hats public key not found on server"); | ||
|
|
||
| const hatsPublicKey = await readKey({ armoredKey: hatsPublicKeyString }); | ||
| const encryptedData = await encrypt({ | ||
| message: await createMessage({ text: dataToEncrypt }), | ||
| encryptionKeys: hatsPublicKey, | ||
| }); | ||
|
|
||
| return encryptedData as string; | ||
| } catch (error) { | ||
| console.log(error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
The function encryptWithHatsKey is using a try-catch block to handle errors, but it's not following best practices for error handling. The catch block is simply logging the error and then rethrowing it. This could lead to unhandled promise rejections if the calling code doesn't also have error handling in place. It would be better to remove the rethrowing of the error and handle it appropriately within this function or let it propagate to the caller without catching it here.
Also, consider replacing console.log(error) with console.error(error). This will output the error message to the console in a more appropriate format for error messages.
export async function encryptWithHatsKey(dataToEncrypt: string): Promise<string> {
try {
const hatsPublicKeyString = await getHatsPublicKey();
if (!hatsPublicKeyString) throw new Error("Hats public key not found on server");
const hatsPublicKey = await readKey({ armoredKey: hatsPublicKeyString });
const encryptedData = await encrypt({
message: await createMessage({ text: dataToEncrypt }),
encryptionKeys: hatsPublicKey,
});
return encryptedData as string;
} catch (error) {
- console.log(error);
+ console.error(error);
- throw error;
}
}
| @@ -1,4 +1,4 @@ | |||
| import { IVulnerabilitySeverity } from "@hats-finance/shared"; | |||
| import { ISubmissionMessageObject, IVulnerabilitySeverity } from "@hats-finance/shared"; | |||
There was a problem hiding this comment.
The import of ISubmissionMessageObject is new in this hunk. Ensure that it's used appropriately in the rest of the code.
- import { IVulnerabilitySeverity } from "@hats-finance/shared";
+ import { ISubmissionMessageObject, IVulnerabilitySeverity } from "@hats-finance/shared";
| @@ -17,7 +17,7 @@ import { getCustomIsDirty, useEnhancedForm } from "hooks/form"; | |||
| import { useContext, useEffect, useState } from "react"; | |||
| import { Controller, useFieldArray, useWatch } from "react-hook-form"; | |||
| import { useTranslation } from "react-i18next"; | |||
| import { encryptWithKeys } from "../../encrypt"; | |||
| import { encryptWithHatsKey, encryptWithKeys } from "../../encrypt"; | |||
There was a problem hiding this comment.
The addition of encryptWithHatsKey function seems to be related to a new encryption method for private audits. Make sure that this function is implemented correctly and securely.
- import { encryptWithKeys } from "../../encrypt";
+ import { encryptWithHatsKey, encryptWithKeys } from "../../encrypt";
| @@ -31,6 +31,7 @@ export function SubmissionDescriptions() { | |||
| const [severitiesOptions, setSeveritiesOptions] = useState<FormSelectInputOption[] | undefined>(); | |||
|
|
|||
| const isAuditSubmission = vault?.description?.["project-metadata"].type === "audit"; | |||
| const isPrivateAudit = vault?.description?.["project-metadata"].isPrivateAudit; | |||
There was a problem hiding this comment.
The introduction of isPrivateAudit variable indicates a new condition based on whether the audit is private or not. Ensure that this condition is handled properly throughout the code.
-
+ const isPrivateAudit = vault?.description?.["project-metadata"].isPrivateAudit;
|
|
||
| let submissionInfo: ISubmissionMessageObject | undefined; | ||
|
|
||
| try { | ||
| submissionInfo = { | ||
| ref: submissionData.ref, | ||
| isEncryptedByHats: isPrivateAudit, | ||
| decrypted: isPrivateAudit ? await encryptWithHatsKey(decrypted ?? "--Nothing decrypted--") : decrypted, | ||
| encrypted: encryptedData as string, | ||
| }; | ||
| } catch (error) { | ||
| console.log(error); | ||
| return alert("There was a problem encrypting the submission with Hats key. Please contact HATS team."); | ||
| } | ||
|
|
||
| console.log(submissionInfo); |
There was a problem hiding this comment.
This block introduces a new way of handling submission information based on whether the audit is private or not. It also includes error handling for issues with Hats key encryption. However, using console.log for error logging and alert for user notification might not be the best approach. Consider using a more sophisticated error handling and logging mechanism. Also, sensitive data should not be logged to the console.
- const submissionInfo = {
- ref: submissionData.ref,
- decrypted,
- encrypted: encryptedData as string,
- };
+ let submissionInfo: ISubmissionMessageObject | undefined;
+
+ try {
+ submissionInfo = {
+ ref: submissionData.ref,
+ isEncryptedByHats: isPrivateAudit,
+ decrypted: isPrivateAudit ? await encryptWithHatsKey(decrypted ?? "--Nothing decrypted--") : decrypted,
+ encrypted: encryptedData as string,
+ };
+ } catch (error) {
+ console.log(error);
+ return alert("There was a problem encrypting the submission with Hats key. Please contact HATS team.");
+ }
+
+ console.log(submissionInfo);
| __html: t( | ||
| vaultType === "audit" | ||
| ? isPrivateAudit | ||
| ? "vaultRepoInformationExplanationPrivateAudit" | ||
| : "vaultRepoInformationExplanationAudit" | ||
| : "vaultRepoInformationExplanation" | ||
| ), |
There was a problem hiding this comment.
The use of dangerouslySetInnerHTML can expose your application to cross-site scripting (XSS) attacks. It's generally recommended to avoid using this property if possible. If the translated strings are not supposed to contain any HTML, consider just rendering them as children of the div.
- <div
- className="helper-text"
- dangerouslySetInnerHTML={{
- __html: t(
- vaultType === "audit"
- ? isPrivateAudit
- ? "vaultRepoInformationExplanationPrivateAudit"
- : "vaultRepoInformationExplanationAudit"
- : "vaultRepoInformationExplanation"
- ),
- }}
- />
+ <div className="helper-text">
+ {t(
+ vaultType === "audit"
+ ? isPrivateAudit
+ ? "vaultRepoInformationExplanationPrivateAudit"
+ : "vaultRepoInformationExplanationAudit"
+ : "vaultRepoInformationExplanation"
+ )}
+ </div>If the translated strings do need to contain HTML, you should sanitize the HTML content before setting it to prevent potential XSS attacks. You could use a library like DOMPurify for this purpose.
| const refreshEditSessionData = useCallback( | ||
| async (newEditSession: IEditedSessionResponse, withReset = true) => { | ||
| const wasSubmittedToCreation = (newEditSession.submittedToCreation ?? false) && !newEditSession.vaultAddress; | ||
|
|
||
| setEditSessionSubmittedCreation(wasSubmittedToCreation); | ||
| setDescriptionHash(newEditSession.descriptionHash); | ||
| setLastModifedOn(newEditSession.updatedAt); | ||
| setEditingExistingVaultStatus(newEditSession.vaultEditionStatus); | ||
| setIsSomeoneCreatingTheVault( | ||
| (!wasSubmittedToCreation && | ||
| newEditSession.lastCreationOnChainRequest && | ||
| moment().diff(moment(newEditSession.lastCreationOnChainRequest), "minute") < 5) ?? | ||
| false | ||
| ); | ||
|
|
||
| if (withReset) handleReset(newEditSession.editedDescription, { keepErrors: true }); | ||
| }; | ||
| if (withReset) handleReset(newEditSession.editedDescription, { keepErrors: true }); | ||
| }, | ||
| [handleReset] | ||
| ); |
There was a problem hiding this comment.
The refreshEditSessionData function has been moved inside a useCallback hook. This is a good practice as it ensures that the function will not be recreated every time the component re-renders, but only when one of its dependencies changes. However, I noticed that the dependency array only includes handleReset. If any other variables or functions from the component scope are used inside refreshEditSessionData, they should also be included in the dependency array to ensure correct behavior.
| const createOrSaveEditSession = useCallback( | ||
| async (isCreation = false, withIpfsHash = false) => { | ||
| try { | ||
| // If vault is already created or is isNonEditableStatus, edition is blocked | ||
| if (isNonEditableStatus) return; | ||
| if (allFormDisabled) return; | ||
| if (isSomeoneCreatingTheVault) return; | ||
| if (!isCreation && !formState.isDirty) return; | ||
| if (isCreation) setLoadingEditSession(true); | ||
| if (!isCreation) setSavingEditSession(true); | ||
|
|
||
| let sessionIdOrSessionResponse: string | IEditedSessionResponse; | ||
|
|
||
| if (isCreation) { | ||
| sessionIdOrSessionResponse = await VaultEditorService.upsertEditSession( | ||
| undefined, | ||
| undefined, | ||
| withIpfsHash ? editSessionId : undefined | ||
| ); | ||
| } else { | ||
| const data: IEditedVaultDescription = getValues(); | ||
| sessionIdOrSessionResponse = await VaultEditorService.upsertEditSession(data, editSessionId, undefined); | ||
| } | ||
|
|
||
| const createVaultOnChain = async () => { | ||
| if (typeof sessionIdOrSessionResponse === "string") { | ||
| navigate(`${RoutePaths.vault_editor}/${sessionIdOrSessionResponse}`, { replace: true }); | ||
| } else { | ||
| refreshEditSessionData(sessionIdOrSessionResponse); | ||
| } | ||
|
|
||
| setSavingEditSession(false); | ||
| setLoadingEditSession(false); | ||
| } catch (error) { | ||
| setSavingEditSession(false); | ||
| setLoadingEditSession(false); | ||
|
|
||
| if (!editSessionId) return; | ||
| const editSessionResponse = await VaultEditorService.getEditSessionData(editSessionId); | ||
| refreshEditSessionData(editSessionResponse); | ||
| } | ||
| }, | ||
| [ | ||
| allFormDisabled, | ||
| editSessionId, | ||
| formState.isDirty, | ||
| getValues, | ||
| isNonEditableStatus, | ||
| isSomeoneCreatingTheVault, | ||
| navigate, | ||
| refreshEditSessionData, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
The createOrSaveEditSession function has been moved inside a useCallback hook. This is a good practice as it ensures that the function will not be recreated every time the component re-renders, but only when one of its dependencies changes. The dependency array seems to include all necessary dependencies.
| @@ -3,6 +3,7 @@ import { | |||
| IEditedContractCovered, | |||
| IVault, | |||
| IVaultRepoInformation, | |||
| allowedElementsMarkdown, | |||
There was a problem hiding this comment.
The import allowedElementsMarkdown is added but it's not clear where it's defined. Please ensure that this module is correctly exported from the "@hats-finance/shared" package.
+ allowedElementsMarkdown,
| import { Alert, Button, CopyToClipboard, Loading, Pill, WithTooltip } from "components"; | ||
| import { defaultAnchorProps } from "constants/defaultAnchorProps"; | ||
| import useConfirm from "hooks/useConfirm"; |
There was a problem hiding this comment.
The import of useConfirm and useVaultRepoName hooks are added. Ensure these hooks are properly implemented and tested to avoid unexpected behavior in your component.
+ import useConfirm from "hooks/useConfirm";
+ import { useVaultRepoName } from "pages/Honeypots/VaultDetailsPage/hooks";
| const confirm = useConfirm(); | ||
|
|
||
| const { data: repoName } = useVaultRepoName(vault); |
There was a problem hiding this comment.
The confirm and repoName variables are introduced using the newly imported hooks. Make sure they are used correctly throughout the component.
+ const confirm = useConfirm();
+ const { data: repoName } = useVaultRepoName(vault);
| if (!vault.description) return <Loading fixed extraText={`${t("loading")}...`} />; | ||
|
|
||
| const isPrivateAudit = vault?.description?.["project-metadata"].isPrivateAudit; | ||
| const contractsCovered = severitiesToContractsCoveredForm(vault.description?.severities); | ||
| const docsLink = vault.description.scope?.docsLink; | ||
|
|
||
| const goToGithubIssuesPrivateAudit = async () => { | ||
| if (!repoName) return; | ||
|
|
||
| const githubLink = `https://github.com/hats-finance/${repoName}/issues`; | ||
|
|
||
| const wantToGo = await confirm({ | ||
| title: t("openGithub"), | ||
| titleIcon: <OpenIcon className="mr-2" fontSize="large" />, | ||
| description: t("doYouWantToSeeSubmissionsOnGithub"), | ||
| cancelText: t("no"), | ||
| confirmText: t("yesGo"), | ||
| }); | ||
|
|
||
| if (!wantToGo) return; | ||
| window.open(githubLink, "_blank"); | ||
| }; |
There was a problem hiding this comment.
The condition for rendering a loading state has been changed, and new logic for handling private audits has been introduced. This includes a new function goToGithubIssuesPrivateAudit. Ensure that this function is correctly implemented and that the user experience remains smooth during these transitions.
- if (!vault.description) return null;
+ if (!vault.description) return <Loading fixed extraText={`${t("loading")}...`} />;
+
+ const isPrivateAudit = vault?.description?.["project-metadata"].isPrivateAudit;
+ const contractsCovered = severitiesToContractsCoveredForm(vault.description?.severities);
+ const docsLink = vault.description.scope?.docsLink;
+
+ const goToGithubIssuesPrivateAudit = async () => {
+ if (!repoName) return;
+
+ const githubLink = `https://github.com/hats-finance/${repoName}/issues`;
+
+ const wantToGo = await confirm({
+ title: t("openGithub"),
+ titleIcon: <OpenIcon className="mr-2" fontSize="large" />,
+ description: t("doYouWantToSeeSubmissionsOnGithub"),
+ cancelText: t("no"),
+ confirmText: t("yesGo"),
+ });
+
+ if (!wantToGo) return;
+ window.open(githubLink, "_blank");
+ };
| @@ -161,7 +184,7 @@ export const InScopeSection = ({ vault }: InScopeSectionProps) => { | |||
|
|
|||
| <div className="section-content"> | |||
| <MDEditor.Markdown | |||
| disallowedElements={disallowedElementsMarkdown} | |||
| allowedElements={allowedElementsMarkdown} | |||
There was a problem hiding this comment.
The allowedElementsMarkdown variable is used in the MDEditor.Markdown component. Ensure that this variable contains the correct configuration for your Markdown editor.
+ allowedElements={allowedElementsMarkdown}
| {isPrivateAudit ? ( | ||
| <> | ||
| <h4 className="section-subtitle"> | ||
| <TerminalIcon className="icon" /> | ||
| <span>{t("repositories")}</span> | ||
| </h4> | ||
|
|
||
| <div className="section-content repos"> | ||
| {vault.description.scope?.reposInformation.map((repo, idx) => ( | ||
| <div className="repo" key={idx}> | ||
| <div className="info"> | ||
| <p className="repo-name">{repo.url}</p> | ||
| {repo.commitHash && ( | ||
| <p className="commit-hash"> | ||
| {t("commitHash")}: {repo.commitHash} | ||
| </p> | ||
| )} | ||
| </div> | ||
| <Button size="medium" onClick={() => goToRepo(repo)}> | ||
| {repo.commitHash ? t("goToRepoAndCommit") : t("goToRepo")} <ArrowForwardIcon className="ml-3" /> | ||
| </Button> | ||
| <div className="repo"> | ||
| <div className="info"> | ||
| <p className="repo-name">{`https://github.com/hats-finance/${repoName}`}</p> | ||
| </div> | ||
| ))} | ||
| <Button size="medium" onClick={goToGithubIssuesPrivateAudit}> | ||
| {t("goToRepo")} <ArrowForwardIcon className="ml-3" /> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| {(contractsCovered.length > 0 || repoContractsList.length > 0) && <div className="separator" />} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| {/* Repos information */} | ||
| {vault.description.scope?.reposInformation && vault.description.scope?.reposInformation.length > 0 && ( | ||
| <> | ||
| <h4 className="section-subtitle"> | ||
| <TerminalIcon className="icon" /> | ||
| <span>{t("repositories")}</span> | ||
| </h4> | ||
|
|
||
| <div className="section-content repos"> | ||
| {vault.description.scope?.reposInformation.map((repo, idx) => ( | ||
| <div className="repo" key={idx}> | ||
| <div className="info"> | ||
| <p className="repo-name">{repo.url}</p> | ||
| {repo.commitHash && ( | ||
| <p className="commit-hash"> | ||
| {t("commitHash")}: {repo.commitHash} | ||
| </p> | ||
| )} | ||
| </div> | ||
| <Button size="medium" onClick={() => goToRepo(repo)}> | ||
| {repo.commitHash ? t("goToRepoAndCommit") : t("goToRepo")} <ArrowForwardIcon className="ml-3" /> | ||
| </Button> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| {(contractsCovered.length > 0 || repoContractsList.length > 0) && <div className="separator" />} | ||
| </> | ||
| )} |
There was a problem hiding this comment.
A new condition has been added to render different content based on whether the audit is private or not. This includes a new button to go to GitHub issues for private audits. Make sure that this new functionality works as expected and doesn't break existing functionality.
+ {isPrivateAudit ? (
+ <>
+ <h4 className="section-subtitle">
+ <TerminalIcon className="icon" />
+ <span>{t("repositories")}</span>
+ </h4>
+ <div className="section-content repos">
+ <div className="repo">
+ <div className="info">
+ <p className="repo-name">{`https://github.com/hats-finance/${repoName}`}</p>
+ </div>
+ <Button size="medium" onClick={goToGithubIssuesPrivateAudit}>
+ {t("goToRepo")} <ArrowForwardIcon className="ml-3" />
+ </Button>
+ </div>
+ </div>
+ </>
+ ) : (
+ <>
+ {/* Repos information */}
+ {vault.description.scope?.reposInformation && vault.description.scope?.reposInformation.length > 0 && (
+ <>
+ <h4 className="section-subtitle">
+ <TerminalIcon className="icon" />
+ <span>{t("repositories")}</span>
+ </h4>
+
+ <div className="section-content repos">
+ {vault.description.scope?.reposInformation.map((repo, idx) => (
+ <div className="repo" key={idx}>
+ <div className="info">
+ <p className="repo-name">{repo.url}</p>
+ {repo.commitHash && (
+ <p className="commit-hash">
+ {t("commitHash")}: {repo.commitHash}
+ </p>
+ )}
+ </div>
+ <Button size="medium" onClick={() => goToRepo(repo)}>
+ {repo.commitHash ? t("goToRepoAndCommit") : t("goToRepo")} <ArrowForwardIcon className="ml-3" />
+ </Button>
+ </div>
+ ))}
+ </div>
+ {(contractsCovered.length > 0 || repoContractsList.length > 0) && <div className="separator" />}
+ </>
+ )}
+ </>
+ )}
| <Alert type="info" className="mb-4"> | ||
| {t("loadingContractsOnRepo")} | ||
| </Alert> |
There was a problem hiding this comment.
The loading alert has been updated with a new class and message. Ensure that this change is consistent with your styling and user experience guidelines.
- <Alert type="info">{t("loadingContractsOnRepo")}</Alert>
+ <Alert type="info" className="mb-4">
+ {t("loadingContractsOnRepo")}
+ </Alert>
| @@ -1,5 +1,5 @@ | |||
| import { CODE_LANGUAGES, DEFAULT_OUT_OF_SCOPE, DEFAULT_TOOLING_STEPS, IEditedVaultDescription } from "@hats-finance/shared"; | |||
| import { Button, FormInput, FormMDEditor, FormRadioInput, Pill } from "components"; | |||
| import { Alert, Button, FormInput, FormMDEditor, FormRadioInput, Pill } from "components"; | |||
There was a problem hiding this comment.
The Alert component is imported but it was not used in the previous version of the code. Make sure that this new import does not introduce unnecessary dependencies.
- import { Alert, Button, FormInput, FormMDEditor, FormRadioInput, Pill } from "components";
+ import { Button, FormInput, FormMDEditor, FormRadioInput, Pill } from "components";
| {/* Private audits alert */} | ||
| {isAdvancedMode && isPrivateAudit && ( | ||
| <Alert className="mt-2 mb-4" type="warning"> | ||
| {t("scopePrivateAuditsWarning")} | ||
| </Alert> | ||
| )} | ||
|
|
||
| {/* Project Outline */} | ||
| {isAdvancedMode && ( | ||
| {isAdvancedMode && !isPrivateAudit && ( |
There was a problem hiding this comment.
The condition for rendering the private audits alert and project outline has been changed to depend on both isAdvancedMode and isPrivateAudit. Ensure that this change doesn't affect other parts of the application that rely on these components being rendered when only isAdvancedMode is true.
- {isAdvancedMode && (
+ {isAdvancedMode && !isPrivateAudit && (
| @@ -90,7 +98,7 @@ export const ScopeDetailsForm = () => { | |||
| )} | |||
|
|
|||
| {/* Project Coding languages */} | |||
| {isAdvancedMode && ( | |||
| {isAdvancedMode && !isPrivateAudit && ( | |||
There was a problem hiding this comment.
The condition for rendering the project coding languages section has been changed to depend on both isAdvancedMode and isPrivateAudit. Ensure that this change doesn't affect other parts of the application that rely on these components being rendered when only isAdvancedMode is true.
- {isAdvancedMode && (
+ {isAdvancedMode && !isPrivateAudit && (
| {isAdvancedMode && !isPrivateAudit && ( | ||
| <> | ||
| <p className="section-title mt-5">{t("VaultEditor.reposAndDocumentation")}</p> | ||
| <ScopeReposInformation /> | ||
| </> | ||
| )} | ||
|
|
||
| {isAdvancedMode && ( | ||
| {isAdvancedMode && !isPrivateAudit && ( |
There was a problem hiding this comment.
The condition for rendering the repos and documentation section has been changed to depend on both isAdvancedMode and isPrivateAudit. Ensure that this change doesn't affect other parts of the application that rely on these components being rendered when only isAdvancedMode is true.
- {isAdvancedMode && (
+ {isAdvancedMode && !isPrivateAudit && (
| @@ -146,7 +158,7 @@ export const ScopeDetailsForm = () => { | |||
| ) : null} | |||
|
|
|||
| {/* Out of scope */} | |||
| {isAdvancedMode && ( | |||
| {isAdvancedMode && !isPrivateAudit && ( | |||
There was a problem hiding this comment.
The condition for rendering the out of scope section has been changed to depend on both isAdvancedMode and isPrivateAudit. Ensure that this change doesn't affect other parts of the application that rely on these components being rendered when only isAdvancedMode is true.
- {isAdvancedMode && (
+ {isAdvancedMode && !isPrivateAudit && (
| @@ -175,7 +187,7 @@ export const ScopeDetailsForm = () => { | |||
| )} | |||
|
|
|||
| {/* Steps to run project */} | |||
| {isAdvancedMode && ( | |||
| {isAdvancedMode && !isPrivateAudit && ( | |||
There was a problem hiding this comment.
The condition for rendering the steps to run project section has been changed to depend on both isAdvancedMode and isPrivateAudit. Ensure that this change doesn't affect other parts of the application that rely on these components being rendered when only isAdvancedMode is true.
- {isAdvancedMode && (
+ {isAdvancedMode && !isPrivateAudit && (
| isPrivateAudit: Yup.boolean(), | ||
| whitelist: Yup.array() | ||
| .of( | ||
| Yup.object({ | ||
| address: Yup.string().test(getTestWalletAddress(intl)).required(intl("required")), | ||
| }) | ||
| ) | ||
| .when("isPrivateAudit", (isPrivateAudit: boolean, schema: any) => { | ||
| if (!isPrivateAudit) return schema; | ||
| return schema.required(intl("required")).min(1, intl("required")); | ||
| }), |
There was a problem hiding this comment.
The validation schema for isPrivateAudit and whitelist seems fine. It checks if isPrivateAudit is a boolean and validates the whitelist array based on the value of isPrivateAudit. If isPrivateAudit is true, it requires at least one item in the whitelist array. Each item in the whitelist array should be an object with a valid wallet address.
+ isPrivateAudit: Yup.boolean(),
+ whitelist: Yup.array()
+ .of(
+ Yup.object({
+ address: Yup.string().test(getTestWalletAddress(intl)).required(intl("required")),
+ })
+ )
+ .when("isPrivateAudit", (isPrivateAudit: boolean, schema: any) => {
+ if (!isPrivateAudit) return schema;
+ return schema.required(intl("required")).min(1, intl("required"));
+ }),
| url: Yup.string() | ||
| .test(getTestGithubRepoUrl(intl)) | ||
| .test("required", intl("required"), (val, ctx: any) => !!ctx.from[2].value["project-metadata"]?.isPrivateAudit), | ||
| commitHash: Yup.string() | ||
| .test(getTestGitCommitHash(intl)) | ||
| .test("required", intl("required"), (val, ctx: any) => !!ctx.from[2].value["project-metadata"]?.isPrivateAudit), |
There was a problem hiding this comment.
The changes to the validation schema for url and commitHash are also correct. They add an additional test that makes these fields required only if isPrivateAudit is true. However, the path to access isPrivateAudit seems a bit complex and could potentially lead to undefined errors if the structure of the form data changes. Consider using optional chaining (?.) to safely access nested properties.
- url: Yup.string()
- .test(getTestGithubRepoUrl(intl))
- .test("required", intl("required"), (val, ctx: any) => !!ctx.from[2].value["project-metadata"]?.isPrivateAudit),
- commitHash: Yup.string()
- .test(getTestGitCommitHash(intl))
- .test("required", intl("required"), (val, ctx: any) => !!ctx.from[2].value["project-metadata"]?.isPrivateAudit),
+ url: Yup.string()
+ .test(getTestGithubRepoUrl(intl))
+ .test("required", intl("required"), (val, ctx: any) => !!ctx?.from?.[2]?.value?["project-metadata"]?.isPrivateAudit),
+ commitHash: Yup.string()
+ .test(getTestGitCommitHash(intl))
+ .test("required", intl("required"), (val, ctx: any) => !!ctx?.from?.[2]?.value?["project-metadata"]?.isPrivateAudit),
Summary by CodeRabbit
VaultCardcomponent andpopulateVaultsWithPricingfunction for better performance and maintainability.VaultEditorFormPageusinguseCallbackhook for better performance.getEditedDescriptionYupSchemafunction to ensure data integrity.