feat: shutter support in dispute commiting & appeal #1994
Conversation
WalkthroughThe changes introduce support for a new "Shutter" dispute kit in the voting and appeal systems. This includes new React components for voting and appeal funding, utility functions for Shutter Network encryption/decryption, updated GraphQL queries, conditional rendering based on dispute kit type, and minor import path adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShutterComponent
participant ShutterUtils
participant Blockchain
participant ShutterAPI
User->>ShutterComponent: Initiate commit (selects vote)
ShutterComponent->>ShutterUtils: encrypt(message)
ShutterUtils->>ShutterAPI: fetch encryption params (POST /register_identity)
ShutterAPI-->>ShutterUtils: returns eon key, identity, timestamp
ShutterUtils->>ShutterUtils: encrypt message with eon key & sigma
ShutterUtils-->>ShutterComponent: returns encryptedCommitment, identity
ShutterComponent->>Blockchain: commit vote (includes commit hash, identity, encrypted commitment)
Blockchain-->>ShutterComponent: transaction receipt
ShutterComponent-->>User: UI updates (success/failure)
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🧹 Nitpick comments (8)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Vote.tsx (1)
3-3: Placeholder component should be either implemented or removed before merge
Votecurrently rendersnull, so the UI will silently fall back to the history list during the reveal phase.
If the reveal logic is not ready yet, consider:
- Adding a clear TODO with context / link to the follow-up ticket, or
- Removing the file from the PR to avoid a dead-code footprint.
This prevents future confusion and keeps the feature flag surface minimal.
web/src/pages/Cases/CaseDetails/Voting/index.tsx (2)
72-76: String-based detection of dispute-kit type is brittleRelying on a case-insensitive
includes("classic") / includes("shutter")can break once the wording changes (e.g. “Shutter DK v2”, localisation, etc.).
A safer approach is to classify through the numericdisputeKitIdwe already have:-const disputeKitName = disputeKitId ? getDisputeKitName(Number(disputeKitId)) : undefined; -const isClassicDisputeKit = disputeKitName?.toLowerCase().includes("classic") ?? false; -const isShutterDisputeKit = disputeKitName?.toLowerCase().includes("shutter") ?? false; +const isClassicDisputeKit = Number(disputeKitId) === 1; // dev-net mapping +const isShutterDisputeKit = Number(disputeKitId) === 2;…and optionally fall back to the name string only when the mapping is unknown.
This removes hidden coupling and guards against future typos.
118-120: Potential double rendering when both flags resolve to trueAlthough unlikely today, if
isClassicDisputeKitandisShutterDisputeKitever evaluate totruesimultaneously, both voting flows will mount, leading to mixed state updates.
Switching to a mutually-exclusiveelse ifmakes the intent explicit:- {isClassicDisputeKit ? <Classic ... /> : null} - {isShutterDisputeKit ? <Shutter ... /> : null} + {isClassicDisputeKit ? ( + <Classic ... /> + ) : isShutterDisputeKit ? ( + <Shutter ... /> + ) : null}web/src/utils/shutter.ts (5)
4-6: Make the decryption delay configurable instead of hard-coding5secondsA fixed 5-second window may be too short in production (network latency, block finality, Shutter service delays). Expose this value via an environment variable or function parameter so it can be tuned without a redeploy.
31-79: Minor: streamline JSON parsing & log handling
fetchShutterDatafirst reads the body as text and then performs a manualJSON.parse.
Usingresponse.json()is simpler, avoids a second buffer allocation, and automatically throws on malformed JSON:-const responseText = await response.text(); -… -jsonResponse = JSON.parse(responseText); +const jsonResponse: ShutterApiResponse = await response.json();You can still log the response status/body on error via
response.clone().text().
132-140: Validate the hex string beyond prefix check
ensureHexStringonly prepends0x. Adding a simple regex guards against accidental non-hex characters that could lead to cryptographic errors later:-const prefixedHex = hexString.startsWith("0x") ? hexString : `0x${hexString}`; +const prefixedHex = hexString.startsWith("0x") ? hexString : `0x${hexString}`; +if (!/^0x[0-9a-fA-F]+$/.test(prefixedHex)) { + throw new Error(`Invalid hex string supplied: ${hexString}`); +}
158-184: Consider returningdecryptionTimestamp(and maybesigma) fromencryptCallers currently have no visibility into when decryption becomes possible; they must either hard-code
DECRYPTION_DELAYagain or poll blindly. Returning the timestamp helps UI/UX (count-down timers) and avoids duplicated logic.-export async function encrypt(message: string): Promise<{ encryptedCommitment: string; identity: string }> { +export async function encrypt( + message: string +): Promise<{ encryptedCommitment: string; identity: string; decryptionTimestamp: number }> { … - return { encryptedCommitment, identity: identityHex }; + return { encryptedCommitment, identity: identityHex, decryptionTimestamp }; }
192-205: Surface “too early to decrypt” to the caller instead of throwing
fetchDecryptionKeyalready throws a descriptive error when the timestamp hasn’t been reached. Consider returning a custom error type or status enum fromdecryptso UI code can display a friendly “Please wait X seconds” message rather than a generic exception banner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
web/package.json(1 hunks)web/src/hooks/queries/useDisputeDetailsQuery.ts(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Vote.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/Vote.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/index.tsx(3 hunks)web/src/utils/shutter.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
🧬 Code Graph Analysis (2)
web/src/pages/Cases/CaseDetails/Voting/index.tsx (1)
web/src/consts/index.ts (1)
getDisputeKitName(46-56)
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (1)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
useDisputeDetailsQuery(46-62)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: SonarCloud
🔇 Additional comments (5)
web/package.json (1)
97-97: Verify Shutter SDK dependency version
Please confirm that@shutter-network/shutter-sdk@^0.0.1is published to npm and aligns with the SDK’s release schedule. If it lacks built-in TypeScript definitions, consider adding or adjusting the version range.web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
31-33: Include disputeKit ID in the DisputeDetails query
FetchingdisputeKit { id }enables downstream UI logic to branch based on the dispute kit type. Ensure you’ve rerun GraphQL codegen so the generatedDisputeDetailsQuerytypes include this new field.web/src/pages/Cases/CaseDetails/Voting/Classic/Vote.tsx (1)
12-12: Update import path for OptionsContainer
Import path changed to reference the centralizedOptionsContainerin the parent directory, matching the updated component structure.web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
22-22: Update import path for JustificationArea
Import path adjusted to pullJustificationAreafrom the shared directory above, keeping Classic and Shutter flows consistent.web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (1)
16-16: Update import path for OptionsContainer
Switched to the parent-directory import forOptionsContainer, consolidating component usage across voting flows.
| const { data: drawData, refetch } = useDrawQuery(address?.toLowerCase(), id, disputeData?.dispute?.currentRound.id); | ||
| const { isCommitPeriod, commited } = useVotingContext(); | ||
| const voteIDs = useMemo(() => drawData?.draws?.map((draw) => draw.voteIDNum) as string[], [drawData]); | ||
|
|
||
| return id && isCommitPeriod && !commited ? <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch }} /> : null; |
There was a problem hiding this comment.
voteIDs can be undefined, causing a runtime crash downstream
voteIDs is typed as string[], yet when drawData hasn’t loaded the expression evaluates to undefined.
Commit.tsx immediately calls voteIDs.map(...), so an undefined value will throw.
-const voteIDs = useMemo(() => drawData?.draws?.map((draw) => draw.voteIDNum) as string[], [drawData]);
+const voteIDs = useMemo(
+ () => drawData?.draws?.map((d) => d.voteIDNum) ?? [],
+ [drawData]
+);Additionally, guard the render:
-return id && isCommitPeriod && !commited ? <ShutterCommit ... /> : null;
+if (!id || !isCommitPeriod || commited || voteIDs.length === 0) return null;
+return <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch }} />;This prevents crashes and avoids dispatching empty commits.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data: drawData, refetch } = useDrawQuery(address?.toLowerCase(), id, disputeData?.dispute?.currentRound.id); | |
| const { isCommitPeriod, commited } = useVotingContext(); | |
| const voteIDs = useMemo(() => drawData?.draws?.map((draw) => draw.voteIDNum) as string[], [drawData]); | |
| return id && isCommitPeriod && !commited ? <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch }} /> : null; | |
| const { data: drawData, refetch } = useDrawQuery( | |
| address?.toLowerCase(), | |
| id, | |
| disputeData?.dispute?.currentRound.id | |
| ); | |
| const { isCommitPeriod, commited } = useVotingContext(); | |
| const voteIDs = useMemo( | |
| () => drawData?.draws?.map((d) => d.voteIDNum) ?? [], | |
| [drawData] | |
| ); | |
| if (!id || !isCommitPeriod || commited || voteIDs.length === 0) return null; | |
| return <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch }} />; |
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx around lines 20 to
24, the variable voteIDs is typed as string[] but can be undefined if drawData
is not loaded, causing runtime crashes when voteIDs.map is called downstream.
Fix this by defaulting voteIDs to an empty array when drawData or drawData.draws
is undefined. Also, add a guard in the return statement to ensure voteIDs is not
empty before rendering ShutterCommit to prevent crashes and avoid dispatching
empty commits.
| const handleCommit = useCallback( | ||
| async (choice: bigint) => { | ||
| const message = { message: saltKey }; | ||
| const rawSalt = !isUndefined(signingAccount) | ||
| ? await signingAccount.signMessage(message) | ||
| : await (async () => { | ||
| const account = await generateSigningAccount(); | ||
| return await account?.signMessage(message); | ||
| })(); | ||
| if (isUndefined(rawSalt)) return; | ||
|
|
||
| const salt = keccak256(rawSalt); | ||
| setSalt(JSON.stringify({ salt, choice: choice.toString(), justification })); | ||
|
|
||
| const encodedMessage = `${choice.toString()}${SEPARATOR}${salt}${SEPARATOR}${justification}`; | ||
| const { encryptedCommitment, identity } = await encrypt(encodedMessage); | ||
|
|
||
| const commitHash = keccak256( | ||
| encodePacked(["uint256", "uint256", "string"], [choice, BigInt(salt), justification]) | ||
| ); | ||
|
|
||
| const { request } = await simulateDisputeKitShutterCastCommit(wagmiConfig, { | ||
| args: [parsedDisputeID, parsedVoteIDs, commitHash, identity as `0x${string}`, encryptedCommitment], | ||
| }); | ||
| if (walletClient && publicClient) { | ||
| await wrapWithToast(async () => await walletClient.writeContract(request), publicClient).then(({ status }) => { | ||
| setIsOpen(status); | ||
| }); | ||
| } | ||
| refetch(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap cryptographic & chain calls in a try/catch and validate user input
handleCommit performs several async steps (keygen, encryption, simulation, tx write) but any rejected promise will bubble up unhandled, leaving the UI in an inconsistent state.
1. Wrap the body in try { ... } catch (err) { /* toast error */ }.
2. Reject justifications containing the separator ␟, otherwise the packed string will be ambiguously parsed during reveal.
const handleCommit = useCallback(
async (choice: bigint) => {
- const message = { message: saltKey };
+ try {
+ if (justification.includes(SEPARATOR))
+ throw new Error(`Justification cannot contain "${SEPARATOR}"`);
+
+ const message = { message: saltKey };
...
- if (walletClient && publicClient) {
- await wrapWithToast(async () => await walletClient.writeContract(request), publicClient).then(({ status }) => {
- setIsOpen(status);
- });
- }
- refetch();
+ if (walletClient && publicClient) {
+ await wrapWithToast(
+ async () => await walletClient.writeContract(request),
+ publicClient
+ ).then(({ status }) => setIsOpen(status));
+ }
+ refetch();
+ } catch (err) {
+ console.error(err);
+ // Replace with your centralized toast helper
+ wrapWithToast(Promise.reject(err), publicClient);
+ }
},This hardens the commit path and improves UX on failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCommit = useCallback( | |
| async (choice: bigint) => { | |
| const message = { message: saltKey }; | |
| const rawSalt = !isUndefined(signingAccount) | |
| ? await signingAccount.signMessage(message) | |
| : await (async () => { | |
| const account = await generateSigningAccount(); | |
| return await account?.signMessage(message); | |
| })(); | |
| if (isUndefined(rawSalt)) return; | |
| const salt = keccak256(rawSalt); | |
| setSalt(JSON.stringify({ salt, choice: choice.toString(), justification })); | |
| const encodedMessage = `${choice.toString()}${SEPARATOR}${salt}${SEPARATOR}${justification}`; | |
| const { encryptedCommitment, identity } = await encrypt(encodedMessage); | |
| const commitHash = keccak256( | |
| encodePacked(["uint256", "uint256", "string"], [choice, BigInt(salt), justification]) | |
| ); | |
| const { request } = await simulateDisputeKitShutterCastCommit(wagmiConfig, { | |
| args: [parsedDisputeID, parsedVoteIDs, commitHash, identity as `0x${string}`, encryptedCommitment], | |
| }); | |
| if (walletClient && publicClient) { | |
| await wrapWithToast(async () => await walletClient.writeContract(request), publicClient).then(({ status }) => { | |
| setIsOpen(status); | |
| }); | |
| } | |
| refetch(); | |
| const handleCommit = useCallback( | |
| async (choice: bigint) => { | |
| try { | |
| // 2. Validate that justification does not contain our separator | |
| if (justification.includes(SEPARATOR)) { | |
| throw new Error(`Justification cannot contain "${SEPARATOR}"`); | |
| } | |
| const message = { message: saltKey }; | |
| const rawSalt = !isUndefined(signingAccount) | |
| ? await signingAccount.signMessage(message) | |
| : await (async () => { | |
| const account = await generateSigningAccount(); | |
| return await account?.signMessage(message); | |
| })(); | |
| if (isUndefined(rawSalt)) return; | |
| const salt = keccak256(rawSalt); | |
| setSalt(JSON.stringify({ salt, choice: choice.toString(), justification })); | |
| const encodedMessage = `${choice.toString()}${SEPARATOR}${salt}${SEPARATOR}${justification}`; | |
| const { encryptedCommitment, identity } = await encrypt(encodedMessage); | |
| const commitHash = keccak256( | |
| encodePacked(["uint256", "uint256", "string"], [choice, BigInt(salt), justification]) | |
| ); | |
| const { request } = await simulateDisputeKitShutterCastCommit(wagmiConfig, { | |
| args: [parsedDisputeID, parsedVoteIDs, commitHash, identity as `0x${string}`, encryptedCommitment], | |
| }); | |
| if (walletClient && publicClient) { | |
| await wrapWithToast( | |
| async () => await walletClient.writeContract(request), | |
| publicClient | |
| ).then(({ status }) => setIsOpen(status)); | |
| } | |
| refetch(); | |
| } catch (err) { | |
| console.error(err); | |
| // TODO: replace with your centralized toast/error‐handling helper | |
| wrapWithToast(Promise.reject(err), publicClient); | |
| } | |
| }, | |
| [ | |
| saltKey, | |
| signingAccount, | |
| generateSigningAccount, | |
| justification, | |
| SEPARATOR, | |
| wagmiConfig, | |
| parsedDisputeID, | |
| parsedVoteIDs, | |
| walletClient, | |
| publicClient, | |
| ] | |
| ); |
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx lines 41 to 70, the
handleCommit function lacks error handling for its async cryptographic and
blockchain calls, risking unhandled promise rejections and inconsistent UI
state. Wrap the entire async function body in a try/catch block, and in the
catch block, display a toast error message to inform the user. Additionally,
before proceeding, validate the justification input to reject any strings
containing the separator character to prevent ambiguous parsing later.
| const responseText = await response.text(); | ||
|
|
||
| // Try to parse the error response even if the request failed | ||
| let jsonResponse; | ||
| try { | ||
| jsonResponse = JSON.parse(responseText); | ||
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } | ||
|
|
||
| // Handle the "too early" error case specifically | ||
| if (!response.ok) { | ||
| if (jsonResponse?.description?.includes("timestamp not reached yet")) { | ||
| throw new Error( | ||
| `Cannot decrypt yet: The decryption timestamp has not been reached.\n` + | ||
| `Please wait at least ${DECRYPTION_DELAY} seconds after encryption before attempting to decrypt.\n` + | ||
| `Error details: ${jsonResponse.description}` | ||
| ); | ||
| } | ||
| throw new Error(`API request failed with status ${response.status}: ${responseText}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a try…catch around fetchDecryptionKey network call
A network failure (e.g., offline client) will currently throw an unhandled promise rejection. Wrap the fetch in try…catch, mirror the structured error handling used in fetchShutterData, and re-throw with context so callers can surface user-friendly messages.
🤖 Prompt for AI Agents
In web/src/utils/shutter.ts around lines 97 to 117, the fetchDecryptionKey
network call lacks a try-catch block, causing unhandled promise rejections on
network failures. Wrap the fetchDecryptionKey call in a try-catch block, catch
any errors, and re-throw a new Error with additional context about the network
failure. This will align error handling with fetchShutterData and allow callers
to display user-friendly error messages.
| const response = await fetch(`https://shutter-api.shutter.network/api/get_decryption_key?identity=${identity}`, { | ||
| method: "GET", | ||
| headers: { | ||
| accept: "application/json", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL-encode the identity query parameter
identity contains the 0x prefix and hex data. Embedding it directly in the URL risks breaking the query string if additional special characters ever appear.
-const response = await fetch(`https://shutter-api.shutter.network/api/get_decryption_key?identity=${identity}`, {
+const query = new URLSearchParams({ identity }).toString();
+const response = await fetch(`https://shutter-api.shutter.network/api/get_decryption_key?${query}`, {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(`https://shutter-api.shutter.network/api/get_decryption_key?identity=${identity}`, { | |
| method: "GET", | |
| headers: { | |
| accept: "application/json", | |
| }, | |
| }); | |
| const query = new URLSearchParams({ identity }).toString(); | |
| const response = await fetch( | |
| `https://shutter-api.shutter.network/api/get_decryption_key?${query}`, | |
| { | |
| method: "GET", | |
| headers: { | |
| accept: "application/json", | |
| }, | |
| } | |
| ); |
🤖 Prompt for AI Agents
In web/src/utils/shutter.ts around lines 89 to 94, the identity query parameter
is directly embedded in the URL without encoding, which risks breaking the query
string if special characters appear. Fix this by URL-encoding the identity value
using encodeURIComponent before appending it to the URL in the fetch call.
| function generateRandomBytes32(): `0x${string}` { | ||
| return ("0x" + | ||
| window.crypto | ||
| .getRandomValues(new Uint8Array(32)) | ||
| .reduce((acc, byte) => acc + byte.toString(16).padStart(2, "0"), "")) as Hex; | ||
| } |
There was a problem hiding this comment.
generateRandomBytes32 breaks during SSR / Node – provide a fallback
window.crypto is undefined in server-side rendering or Node tests, throwing a ReferenceError. A cross-platform implementation keeps the function usable everywhere:
-function generateRandomBytes32(): `0x${string}` {
- return ("0x" +
- window.crypto
- .getRandomValues(new Uint8Array(32))
- .reduce((acc, byte) => acc + byte.toString(16).padStart(2, "0"), "")) as Hex;
+function generateRandomBytes32(): `0x${string}` {
+ let bytes: Uint8Array;
+ if (typeof window !== "undefined" && window.crypto?.getRandomValues) {
+ bytes = window.crypto.getRandomValues(new Uint8Array(32));
+ } else {
+ // Node.js / SSR fallback
+ // eslint-disable-next-line @typescript-eslint/no-var-requires
+ bytes = require("crypto").randomBytes(32);
+ }
+ return ("0x" +
+ Array.from(bytes)
+ .map((b) => b.toString(16).padStart(2, "0"))
+ .join("")) as Hex;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function generateRandomBytes32(): `0x${string}` { | |
| return ("0x" + | |
| window.crypto | |
| .getRandomValues(new Uint8Array(32)) | |
| .reduce((acc, byte) => acc + byte.toString(16).padStart(2, "0"), "")) as Hex; | |
| } | |
| function generateRandomBytes32(): `0x${string}` { | |
| let bytes: Uint8Array; | |
| if (typeof window !== "undefined" && window.crypto?.getRandomValues) { | |
| bytes = window.crypto.getRandomValues(new Uint8Array(32)); | |
| } else { | |
| // Node.js / SSR fallback | |
| // eslint-disable-next-line @typescript-eslint/no-var-requires | |
| bytes = require("crypto").randomBytes(32); | |
| } | |
| return ("0x" + | |
| Array.from(bytes) | |
| .map((b) => b.toString(16).padStart(2, "0")) | |
| .join("")) as Hex; | |
| } |
🤖 Prompt for AI Agents
In web/src/utils/shutter.ts around lines 146 to 151, the generateRandomBytes32
function uses window.crypto which is undefined in server-side rendering or Node
environments, causing a ReferenceError. Modify the function to check if
window.crypto is available; if not, use Node's crypto module as a fallback to
generate the random bytes. This ensures the function works correctly in both
browser and server contexts.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
web/src/pages/Cases/CaseDetails/Appeal/index.tsx (1)
54-55: Consider using an enum for dispute kit types instead of string checks.Using string inclusion checks (
includes()) for determining dispute kit types could be brittle if future dispute kit names contain these substrings.A more robust approach would be to use an enum or specific equality checks:
// In consts/index.ts export enum DisputeKitType { CLASSIC = "classic", SHUTTER = "shutter", // future types... } export const getDisputeKitType = (disputeKitName: string): DisputeKitType | undefined => { if (disputeKitName.includes("classic")) return DisputeKitType.CLASSIC; if (disputeKitName.includes("shutter")) return DisputeKitType.SHUTTER; return undefined; }; // Then in your component: const disputeKitType = disputeKitName ? getDisputeKitType(disputeKitName) : undefined;Then use a switch statement for rendering:
switch (disputeKitType) { case DisputeKitType.CLASSIC: return <Classic {...props} />; case DisputeKitType.SHUTTER: return <Shutter {...props} />; default: return <UnsupportedDisputeKit name={disputeKitName} />; }web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx (3)
44-44: Consider adding better type safety for the amount prop.The casting of
amountto`${number}`is a workaround that could lead to runtime issues if the string doesn't contain a valid number.Instead of casting, validate and convert the string to a proper number type:
- <Fund amount={amount as `${number}`} setAmount={setAmount} setIsOpen={setIsPopupOpen} /> + <Fund + amount={amount && !isNaN(Number(amount)) ? amount : "0"} + setAmount={setAmount} + setIsOpen={setIsPopupOpen} + />Alternatively, modify the Fund component to accept a string directly and handle the conversion internally.
23-33: Add error handling for the popup and context states.The component doesn't handle potential errors or loading states from the context hook or when opening/closing the popup.
Consider adding error boundaries or state handling:
const Shutter: React.FC<IShutter> = ({ isAppealMiniGuideOpen, toggleAppealMiniGuide }) => { const [isPopupOpen, setIsPopupOpen] = useState(false); const [amount, setAmount] = useState(""); - const { selectedOption } = useSelectedOptionContext(); + const { selectedOption, error: contextError } = useSelectedOptionContext(); + + if (contextError) return <ErrorMessage message="Failed to load appeal options" />;
42-44: Add better UI feedback and validation for the funding process.The component shows limited visual feedback about the funding constraints or validation requirements.
Consider enhancing the UI with more informative labels:
- <label>The jury decision is appealed when two options are fully funded.</label> + <label> + The jury decision is appealed when two options are fully funded. + {selectedOption && ( + <span> You are funding option: <strong>{selectedOption.title}</strong></span> + )} + </label>web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx (2)
52-62: Simplify complex conditional logic in useNeedFund hook.The
useNeedFundhook contains complex nested conditions that are difficult to read and maintain.Refactor for better readability:
const useNeedFund = () => { const { loserSideCountdown } = useCountdownContext(); const { fundedChoices, winningChoice } = useFundingContext(); // Check if countdown is still active if ((loserSideCountdown ?? 0) > 0) return true; // Check if there are funded choices but winning choice isn't funded if (isUndefined(fundedChoices) || isUndefined(winningChoice)) return false; if (fundedChoices.length === 0) return false; return !fundedChoices.includes(winningChoice); };
112-143: Add feedback for transaction confirmation.The component shows the transaction is in progress but doesn't provide feedback about transaction confirmation or mining status.
Add a transaction status indicator:
<StyledButton disabled={isFundDisabled} isLoading={(isSending || isLoading) && !insufficientBalance} - text={isDisconnected ? "Connect to Fund" : "Fund"} + text={isDisconnected ? "Connect to Fund" : isSending ? "Transaction pending..." : "Fund"} onClick={() => { // ...existing code }} />Consider also adding a transaction hash display and link to block explorer once the transaction is submitted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Appeal/index.tsx(2 hunks)web/src/pages/Resolver/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/src/pages/Resolver/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx (3)
web/src/pages/Home/TopJurors/index.tsx (1)
StyledLabel(39-41)web/src/hooks/useClassicAppealContext.tsx (3)
useCountdownContext(118-118)useFundingContext(120-120)useSelectedOptionContext(119-119)web/src/components/ErrorButtonMessage.tsx (1)
ErrorButtonMessage(3-11)
web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx (2)
web/src/hooks/useClassicAppealContext.tsx (1)
useSelectedOptionContext(119-119)web/src/pages/Cases/CaseDetails/Appeal/index.tsx (2)
AppealHeader(28-41)StyledTitle(43-46)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: SonarCloud
| const disputeKitId = disputeData?.dispute?.currentRound?.disputeKit?.id; | ||
| const disputeKitName = disputeKitId ? getDisputeKitName(Number(disputeKitId))?.toLowerCase() : ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing dispute kit data.
The code doesn't handle potential errors when fetching dispute data or if the disputeKitId is missing. This could lead to undefined behavior.
Add error handling and fallback:
- const disputeKitId = disputeData?.dispute?.currentRound?.disputeKit?.id;
+ const disputeKitId = disputeData?.dispute?.currentRound?.disputeKit?.id;
+ const isLoading = !disputeData;
+ const hasError = !disputeKitId && !isLoading;Then in your rendering logic:
if (isLoading) return <LoadingIndicator />;
if (hasError) return <ErrorMessage message="Failed to determine dispute kit type" />;🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Appeal/index.tsx around lines 52 to 53, add
error handling for cases where dispute data or disputeKitId is missing.
Introduce state variables or flags to track loading and error states when
fetching dispute data. Implement conditional rendering to show a loading
indicator while data is loading and an error message if fetching fails or
disputeKitId is undefined, ensuring the component handles these scenarios
gracefully.
| const { id } = useParams(); | ||
| const { data: disputeData } = useDisputeDetailsQuery(id); | ||
| const disputeKitId = disputeData?.dispute?.currentRound?.disputeKit?.id; | ||
| const disputeKitName = disputeKitId ? getDisputeKitName(Number(disputeKitId))?.toLowerCase() : ""; | ||
| const isClassicDisputeKit = disputeKitName?.includes("classic") ?? false; | ||
| const isShutterDisputeKit = disputeKitName?.includes("shutter") ?? false; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a fallback rendering for unsupported dispute kit types.
You've implemented conditional rendering for "classic" and "shutter" dispute kits, but there's no fallback if neither condition is met. This could result in nothing being rendered during the appeal period if a new dispute kit type is introduced.
Consider adding an else condition or default case:
{isClassicDisputeKit && (
<Classic isAppealMiniGuideOpen={isAppealMiniGuideOpen} toggleAppealMiniGuide={toggleAppealMiniGuide} />
)}
{isShutterDisputeKit && (
<Shutter isAppealMiniGuideOpen={isAppealMiniGuideOpen} toggleAppealMiniGuide={toggleAppealMiniGuide} />
)}
+{!isClassicDisputeKit && !isShutterDisputeKit && (
+ <div>Unsupported dispute kit type: {disputeKitName}</div>
+)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Appeal/index.tsx around lines 50 to 56, the
code checks for "classic" and "shutter" dispute kits but lacks a fallback for
unsupported or new dispute kit types. Add a default rendering case or an else
condition to handle situations where the dispute kit name does not include
"classic" or "shutter" to ensure something meaningful is rendered during the
appeal period.
| const Fund: React.FC<IFund> = ({ amount, setAmount, setIsOpen }) => { | ||
| const needFund = useNeedFund(); | ||
| const { address, isDisconnected } = useAccount(); | ||
| const { data: balance } = useBalance({ | ||
| query: { refetchInterval: REFETCH_INTERVAL }, | ||
| address, | ||
| }); | ||
| const publicClient = usePublicClient(); | ||
| const [isSending, setIsSending] = useState(false); | ||
| const [debouncedAmount, setDebouncedAmount] = useState<`${number}` | "">(""); | ||
| useDebounce(() => setDebouncedAmount(amount), 500, [amount]); | ||
| const parsedAmount = useParsedAmount(debouncedAmount as `${number}`); | ||
| const insufficientBalance = useMemo(() => balance && balance.value < parsedAmount, [balance, parsedAmount]); | ||
| const { fundAppealConfig, fundAppeal, isLoading, isError } = useFundAppeal(parsedAmount, insufficientBalance); | ||
| const isFundDisabled = useMemo( | ||
| () => | ||
| isDisconnected || | ||
| isSending || | ||
| !balance || | ||
| insufficientBalance || | ||
| Number(parsedAmount) <= 0 || | ||
| isError || | ||
| isLoading, | ||
| [isDisconnected, isSending, balance, insufficientBalance, parsedAmount, isError, isLoading] | ||
| ); | ||
|
|
||
| return needFund ? ( | ||
| <Container> | ||
| <StyledLabel>How much ETH do you want to contribute?</StyledLabel> | ||
| <StyledField | ||
| type="number" | ||
| value={amount} | ||
| onChange={(e) => setAmount(e.target.value)} | ||
| placeholder="Amount to fund" | ||
| /> | ||
| <EnsureChain> | ||
| <div> | ||
| <StyledButton | ||
| disabled={isFundDisabled} | ||
| isLoading={(isSending || isLoading) && !insufficientBalance} | ||
| text={isDisconnected ? "Connect to Fund" : "Fund"} | ||
| onClick={() => { | ||
| if (fundAppeal && fundAppealConfig && publicClient) { | ||
| setIsSending(true); | ||
| wrapWithToast(async () => await fundAppeal(fundAppealConfig.request), publicClient) | ||
| .then((res) => setIsOpen(res.status)) | ||
| .finally(() => setIsSending(false)); | ||
| } | ||
| }} | ||
| /> | ||
| {insufficientBalance && ( | ||
| <ErrorButtonMessage> | ||
| <ClosedCircleIcon /> Insufficient balance | ||
| </ErrorButtonMessage> | ||
| )} | ||
| </div> | ||
| </EnsureChain> | ||
| </Container> | ||
| ) : null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for the Fund component.
This component handles critical functionality (financial transactions) but doesn't appear to have accompanying test coverage.
Create unit tests that verify:
- The component renders correctly when funding is needed
- The component doesn't render when funding isn't needed
- The button is disabled under appropriate conditions
- Transaction execution is triggered correctly
- Error states are handled properly
Consider using mock providers for the web3 interactions to facilitate testing.
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx around lines 86 to
145, the Fund component lacks unit tests. Add tests to verify it renders
correctly when funding is needed and does not render otherwise. Test that the
fund button is disabled under the right conditions such as disconnection,
insufficient balance, or loading states. Also, verify that clicking the button
triggers the transaction execution logic properly and that error states display
the correct messages. Use mock providers to simulate web3 hooks like useAccount,
useBalance, and useFundAppeal for isolated testing.
| const parsedAmount = useParsedAmount(debouncedAmount as `${number}`); | ||
| const insufficientBalance = useMemo(() => balance && balance.value < parsedAmount, [balance, parsedAmount]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the amount field.
The code parses the amount without validating the input string format, which could lead to issues with invalid inputs.
Add validation before parsing:
- const parsedAmount = useParsedAmount(debouncedAmount as `${number}`);
+ const isValidAmount = debouncedAmount !== "" && !isNaN(Number(debouncedAmount));
+ const parsedAmount = isValidAmount ? useParsedAmount(debouncedAmount as `${number}`) : BigInt(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsedAmount = useParsedAmount(debouncedAmount as `${number}`); | |
| const insufficientBalance = useMemo(() => balance && balance.value < parsedAmount, [balance, parsedAmount]); | |
| const isValidAmount = debouncedAmount !== "" && !isNaN(Number(debouncedAmount)); | |
| const parsedAmount = isValidAmount | |
| ? useParsedAmount(debouncedAmount as `${number}`) | |
| : BigInt(0); | |
| const insufficientBalance = useMemo( | |
| () => balance && balance.value < parsedAmount, | |
| [balance, parsedAmount] | |
| ); |
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx around lines 97 to
98, the amount input is parsed without validating its format, which can cause
errors with invalid inputs. Add input validation to check that debouncedAmount
matches the expected numeric string pattern before calling useParsedAmount. If
the input is invalid, handle it appropriately (e.g., set parsedAmount to zero or
show an error) to prevent parsing errors and ensure robust behavior.
| if (fundAppeal && fundAppealConfig && publicClient) { | ||
| setIsSending(true); | ||
| wrapWithToast(async () => await fundAppeal(fundAppealConfig.request), publicClient) | ||
| .then((res) => setIsOpen(res.status)) | ||
| .finally(() => setIsSending(false)); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling during transaction execution.
The transaction execution has minimal error handling, and errors might not be properly communicated to users.
Enhance the error handling:
onClick={() => {
if (fundAppeal && fundAppealConfig && publicClient) {
setIsSending(true);
wrapWithToast(async () => await fundAppeal(fundAppealConfig.request), publicClient)
.then((res) => setIsOpen(res.status))
+ .catch((error) => {
+ console.error("Transaction failed:", error);
+ // Display user-friendly error message
+ })
.finally(() => setIsSending(false));
}
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fundAppeal && fundAppealConfig && publicClient) { | |
| setIsSending(true); | |
| wrapWithToast(async () => await fundAppeal(fundAppealConfig.request), publicClient) | |
| .then((res) => setIsOpen(res.status)) | |
| .finally(() => setIsSending(false)); | |
| } | |
| }} | |
| if (fundAppeal && fundAppealConfig && publicClient) { | |
| setIsSending(true); | |
| wrapWithToast(async () => await fundAppeal(fundAppealConfig.request), publicClient) | |
| .then((res) => setIsOpen(res.status)) | |
| .catch((error) => { | |
| console.error("Transaction failed:", error); | |
| // Display user-friendly error message | |
| }) | |
| .finally(() => setIsSending(false)); | |
| } | |
| }} |
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx around lines 128 to
134, the current transaction execution lacks robust error handling and may fail
to inform users of errors. Modify the promise chain to catch any errors from
wrapWithToast and handle them appropriately, such as displaying an error message
or notification to the user. Ensure setIsSending(false) is called in all cases
to reset the loading state, and update the UI to reflect error states clearly.
|
kemuru
changed the title
PR-Codex overview
This PR introduces new features and updates related to the voting system, including the addition of a
Shuttervoting mechanism and improvements to existing components. It also corrects a typo in the heading and updates package dependencies.Detailed summary
Shuttervoting component inweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx.CommitandFundcomponents forShuttervoting in respective files.Votingto conditionally renderClassicorShutterbased on dispute kit type.web/src/pages/Resolver/index.tsx.OptionsContainerandJustificationAreain several files.web/src/utils/shutter.ts.web/package.jsonandyarn.lock.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Enhancements