feat: shutter support in dispute commiting & appeal #1994
Conversation
WalkthroughThis update adds support for the Shutter dispute kit, including new React components for commit and reveal phases, utility functions for Shutter encryption/decryption, and updates to dispute and voting logic to handle multiple dispute kit types. It also modifies build configuration to include Shutter SDK files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShutterCommit (UI)
participant ShutterUtils
participant Blockchain
participant ShutterAPI
User->>ShutterCommit: Selects vote and submits
ShutterCommit->>ShutterUtils: encrypt(vote data, delay)
ShutterUtils->>ShutterAPI: fetchShutterData(decryptionTimestamp)
ShutterAPI-->>ShutterUtils: eonKey, identity
ShutterUtils->>ShutterUtils: Encrypt vote data
ShutterUtils-->>ShutterCommit: encryptedCommitment, identity
ShutterCommit->>Blockchain: commitVote(disputeId, voteId, hash, identity, encryptedCommitment)
Blockchain-->>ShutterCommit: Transaction receipt
ShutterCommit-->>User: Confirmation/toast
Note over User,ShutterCommit: Later, during reveal period
User->>ShutterReveal: Clicks "Reveal"
ShutterReveal->>ShutterUtils: decrypt(encryptedCommitment, identity)
ShutterUtils->>ShutterAPI: fetchDecryptionKey(identity)
ShutterAPI-->>ShutterUtils: decryptionKey
ShutterUtils->>ShutterUtils: Decrypt vote data
ShutterUtils-->>ShutterReveal: vote data
ShutterReveal->>Blockchain: revealVote(disputeId, voteId, vote data)
Blockchain-->>ShutterReveal: Transaction receipt
ShutterReveal-->>User: Confirmation/toast
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
🪧 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.
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
kemuru
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
web/src/utils/shutter.ts (3)
89-94: URL-encode the identity query parameterThe identity parameter is directly embedded in the URL without proper encoding, which could cause issues if it contains special characters.
97-117: Add atry…catcharoundfetchDecryptionKeynetwork callThe
fetchDecryptionKeyfunction should have error handling for network failures, similar to what's implemented infetchShutterData.
146-151:generateRandomBytes32breaks during SSR / Node – provide a fallbackThe current implementation uses
crypto.getRandomValuesdirectly, which is only available in browser environments and will cause errors during server-side rendering or in Node.js environments.
🧹 Nitpick comments (3)
web/src/utils/shutter.ts (3)
32-77: Consider conditionally logging based on environmentThe function contains multiple logging statements which are useful for debugging but may be excessive in production environments.
Consider adding a debug flag and conditionally logging only when debugging:
+ const DEBUG = process.env.NODE_ENV !== 'production'; ... async function fetchShutterData(decryptionTimestamp: number): Promise<ShutterApiMessageData> { try { - console.log(`Sending request to Shutter API with decryption timestamp: ${decryptionTimestamp}`); + if (DEBUG) console.log(`Sending request to Shutter API with decryption timestamp: ${decryptionTimestamp}`); // Generate a random identity prefix const identityPrefix = generateRandomBytes32(); - console.log(`Generated identity prefix: ${identityPrefix}`); + if (DEBUG) console.log(`Generated identity prefix: ${identityPrefix}`); // ...other code... // Log the response status - console.log(`API response status: ${response.status}`); + if (DEBUG) console.log(`API response status: ${response.status}`);Apply similar changes to other logging statements throughout the file.
158-184: Validate and limit the message size before encryptionThe function doesn't validate or limit the size of the input message. Very large messages could potentially cause performance issues or even failures during encryption.
Consider adding message size validation:
export async function encrypt(message: string): Promise<{ encryptedCommitment: string; identity: string }> { + // Check message size to avoid performance issues with very large messages + const MAX_MESSAGE_SIZE = 10 * 1024; // 10KB example limit + if (message.length > MAX_MESSAGE_SIZE) { + throw new Error(`Message too large: ${message.length} bytes exceeds the limit of ${MAX_MESSAGE_SIZE} bytes`); + } // Set decryption timestamp const decryptionTimestamp = Math.floor(Date.now() / 1000) + DECRYPTION_DELAY; // Rest of the function...
86-125: Consider implementing retry logic for API callsThe function makes a single attempt to fetch the decryption key. Network calls can fail intermittently, so implementing retry logic could improve reliability.
Consider implementing a simple retry mechanism with exponential backoff:
async function fetchWithRetry(url: string, options: RequestInit, maxRetries = 3): Promise<Response> { let retries = 0; while (true) { try { const response = await fetch(url, options); return response; } catch (error) { if (retries >= maxRetries) throw error; retries++; const delay = Math.pow(2, retries) * 1000; // Exponential backoff console.log(`Retrying in ${delay}ms... (${retries}/${maxRetries})`); await new Promise(resolve => setTimeout(resolve, delay)); } } }Then use this in both API calling functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
web/package.json(2 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(1 hunks)web/src/utils/shutter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/package.json
- web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: SonarCloud
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
62-105: Wrap cryptographic & chain calls in a try/catch and validate user input
handleCommitperforms several async steps (keygen, encryption, simulation, tx write) but any rejected promise will bubble up unhandled, leaving the UI in an inconsistent state.
- Wrap the body in
try { ... } catch (err) { /* toast error */ }.- 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.
🧹 Nitpick comments (5)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (5)
1-16: Import organization suggestionThe imports are functionally correct but could benefit from more structured organization. Consider grouping imports by type (React, third-party libraries, internal hooks, utilities, components).
import React, { useCallback, useMemo, useState } from "react"; import styled from "styled-components"; import { useParams } from "react-router-dom"; import { useLocalStorage } from "react-use"; import { keccak256, stringToHex, encodeAbiParameters } from "viem"; import { useWalletClient, usePublicClient, useConfig } from "wagmi"; import { simulateDisputeKitShutterCastCommitShutter } from "hooks/contracts/generated"; import useSigningAccount from "hooks/useSigningAccount"; import { isUndefined } from "utils/index"; import { encrypt } from "utils/shutter"; import { wrapWithToast } from "utils/wrapWithToast"; import OptionsContainer from "../OptionsContainer";
29-29: Define SEPARATOR as a constant value with self-documenting nameThe SEPARATOR constant is used to delimit values in the encoded message, but its purpose isn't immediately clear.
-const SEPARATOR = "-"; +const VOTE_DATA_SEPARATOR = "-"; // Separates choice, salt, and justification in encoded messages
60-60: Unused variable in destructuring assignmentThe underscore variable is used to ignore the first return value from useLocalStorage, but this pattern can be confusing.
-const [_, setSalt] = useLocalStorage(saltKey); +const [, setSalt] = useLocalStorage(saltKey);
76-77: Consider encrypting object data instead of a concatenated stringUsing string concatenation with separators is error-prone and fragile. Consider encrypting a structured object instead.
- const encodedMessage = `${choice.toString()}${SEPARATOR}${salt}${SEPARATOR}${justification}`; - const { encryptedCommitment, identity } = await encrypt(encodedMessage); + const voteData = JSON.stringify({ + choice: choice.toString(), + salt, + justification + }); + const { encryptedCommitment, identity } = await encrypt(voteData);This would make the data structure more explicit and easier to extend in the future without breaking backward compatibility.
107-119: Consider handling loading and error states in UIThe component returns either the voting UI or null based solely on the existence of an ID param, but doesn't account for loading states or error handling in the UI.
return id ? ( <Container> + {/* Consider adding loading indicator when transaction is in progress */} <OptionsContainer {...{ arbitrable, justification, setJustification, handleSelection: handleCommit, + disabled: false, // Add disabled state when transaction is in progress }} /> </Container> ) : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (2)
web-devtools/src/hooks/useLocalStorage.ts (1)
useLocalStorage(3-22)web/src/utils/shutter.ts (1)
encrypt(158-184)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: SonarCloud
🔇 Additional comments (3)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (3)
34-48: LGTM for the hashVote implementationThe hashing function correctly implements the logic required to match the
DisputeKitClassic.hashVote()Solidity function. The comment and implementation are clear and well-structured.
81-83:❓ Verification inconclusive
Add typecasting for encryptedCommitment parameter
The
encryptedCommitmentparameter is passed directly to the contract simulation, but there's no explicit type assertion to ensure it matches the expected format.
🏁 Script executed:
#!/bin/bash # Check the contract interface to determine the expected type of encryptedCommitment ast-grep --pattern 'function castCommitShutter(uint256 $_, uint256[] calldata $_, bytes32 $_, address $_, $_encryptedCommitment)'Length of output: 131
🏁 Script executed:
#!/bin/bash # Search solidity contracts for the castCommitShutter signature to confirm the expected type of encryptedCommitment rg -n "function castCommitShutter" -g '*.sol' -A2 rg -n "castCommitShutter(" -g '*.sol'Length of output: 471
#!/bin/bash # Retrieve the full castCommitShutter function signature from the DisputeKitShutter contract rg -n "function castCommitShutter" -g 'contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol' -A10
#!/bin/bash # Retrieve the full castCommitShutter function signature (including parameter types) from the DisputeKitShutter contract rg -n -A10 "function castCommitShutter" contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
Ensure explicit type casting for encryptedCommitment
- Location:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(lines 81–83)- Issue: The
encryptedCommitmentvalue is passed directly into the simulated contract call without an explicit cast. If the DisputeKitShutter contract expects abytes32(or other fixed‐length byte array), passing a rawstringrisks runtime mismatches.- Recommendation: Convert or pad
encryptedCommitmentto the exact type expected by the contract. For example, using ethers.js:import { ethers } from 'ethers' const paddedEncrypted = ethers.utils.hexZeroPad(encryptedCommitment, 32) const { request } = await simulateDisputeKitShutterCastCommitShutter(wagmiConfig, { args: [ parsedDisputeID, parsedVoteIDs, commitHash, identity as `0x${string}`, paddedEncrypted, ], })- Next step: Manually verify the
castCommitShuttersignature inDisputeKitShutter.solto confirm whether it requiresbytes32, a genericbytes, or another type, and adjust the cast accordingly.
74-77:❌ Incorrect review comment
Missing salt format standardization
The salt is passed as a hex string to both localStorage and the encoded message, but it's unclear if both consumers expect the same format.
🏁 Script executed:
#!/bin/bash # Check how the salt is used during reveal phase to ensure format consistency rg -A 5 "const salt = " --glob="*.tsx" --glob="*.ts" | grep -v "const salt = keccak256"Length of output: 1788
We still need to confirm how the Shutter “reveal” step consumes the stored salt to ensure it expects the same hex‐string format. Let’s pull up the Reveal component in
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsxto see whether it’s parsing the salt back into a number or treating it as a hex string.
🏁 Script executed:
#!/bin/bash # Locate and show how the salt is read/used in the Shutter reveal phase rg -n "Reveal" --glob="web/src/pages/Cases/CaseDetails/Voting/Shutter/*.tsx" rg -A5 "localStorage.getItem" --glob="web/src/pages/Cases/CaseDetails/Voting/Shutter/*.tsx"Length of output: 168
We still need to confirm how the Shutter reveal phase reads and expects the salt format (and whether it ever touches localStorage). Please list the files under the Shutter folder and show how
setSaltis defined/used, plus anylocalStoragecalls:
🏁 Script executed:
#!/bin/bash # 1) List all files in the Shutter voting folder find web/src/pages/Cases/CaseDetails/Voting/Shutter -maxdepth 1 -type f # 2) Show the definition and usage of setSalt in Commit.tsx rg -n "setSalt" -C3 web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx # 3) Search for any localStorage usage in the Shutter folder rg -n "localStorage\." -C3 web/src/pages/Cases/CaseDetails/Voting/ShutterLength of output: 1286
Let’s inspect the Shutter “Vote” and entrypoint files to see how the stored salt is retrieved and consumed:
🏁 Script executed:
#!/bin/bash # Show first 200 lines of Vote.tsx and index.tsx for any salt parsing or useLocalStorage reads echo "---- Vote.tsx ----" sed -n '1,200p' web/src/pages/Cases/CaseDetails/Voting/Shutter/Vote.tsx echo "---- index.tsx ----" sed -n '1,200p' web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsxLength of output: 1396
No Shutter reveal step consumes the stored salt—ignore format‐mismatch warning
I reviewed the Shutter voting components and found:
- Files in
web/src/pages/Cases/CaseDetails/Voting/Shutter/:
Commit.tsx(only writes salt viasetSalt)Vote.tsx(stub, returnsnull)index.tsx(mounts<ShutterCommit />during commit period)- There is no Reveal component or any
localStorage.getItem(…)usage in this folder to read back the salt.Since the salt is only used once in the commit stage and never re-read by another consumer, there’s no mismatch to address.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
144-155: Good job exporting the getDeadline function for reuse.Making this utility function exportable promotes code reuse across the new Shutter components, which aligns well with the PR's objectives. This is a good practice for shared functionality.
A few minor suggestions to consider:
- Line 153: Consider returning
undefinedinstead of0when conditions aren't met since the return type includesundefinedand this would better indicate an invalid input case- Lines 150-151: Although it works correctly, it's a best practice to specify the radix in
parseInt()calls (e.g.,parseInt(lastPeriodChange, 10))export const getDeadline = ( currentPeriodIndex: number, lastPeriodChange?: string, timesPerPeriod?: string[] ): number | undefined => { if (lastPeriodChange && timesPerPeriod && currentPeriodIndex < timesPerPeriod.length) { - const parsedLastPeriodChange = parseInt(lastPeriodChange); - const parsedTimeCurrentPeriod = parseInt(timesPerPeriod[currentPeriodIndex]); + const parsedLastPeriodChange = parseInt(lastPeriodChange, 10); + const parsedTimeCurrentPeriod = parseInt(timesPerPeriod[currentPeriodIndex], 10); return parsedLastPeriodChange + parsedTimeCurrentPeriod; } - return 0; + return undefined; };web/src/pages/Cases/CaseDetails/Voting/index.tsx (1)
61-65: Consider optimizing duplicate dispute data fetching.The component accepts
disputeas a prop but also fetches dispute data again usinguseDisputeDetailsQuery. This could lead to duplicate network requests for the same data.Consider refactoring to use only one source of dispute data. If the complete dispute data is already available in props, you might be able to skip the additional query or use a more targeted query for just the missing pieces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
web/src/pages/Cases/CaseDetails/Timeline.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/index.tsx(5 hunks)web/src/pages/Cases/CaseDetails/index.tsx(1 hunks)web/src/utils/shutter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
- web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx
- web/src/utils/shutter.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/src/pages/Cases/CaseDetails/Voting/index.tsx (2)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
DisputeDetailsQuery(8-8)web/src/consts/index.ts (1)
getDisputeKitName(46-56)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: SonarCloud
🔇 Additional comments (6)
web/src/pages/Cases/CaseDetails/index.tsx (1)
82-82: LGTM! Correctly passing the dispute prop to the Voting component.This change properly passes the
disputeobject to theVotingcomponent, enabling it to determine the dispute kit type for conditional rendering of the appropriate voting interface (Classic or Shutter).web/src/pages/Cases/CaseDetails/Voting/index.tsx (5)
18-18: LGTM! Proper type import.The import of the
DisputeDetailsQuerytype is correctly added to provide typing for the new dispute prop.
28-31: LGTM! Necessary imports for the Shutter implementation.The Shutter component import and getDisputeKitName utility function are correctly added to support the new dispute kit functionality.
58-58: LGTM! Proper interface update.The IVoting interface is correctly extended to include the dispute prop with appropriate typing.
73-77: LGTM! Clear dispute kit detection logic.The implementation to determine the dispute kit type is clean and handles potential undefined values properly with null coalescing operators.
119-122: LGTM! Appropriate conditional rendering of voting components.The conditional rendering logic correctly switches between Classic and Shutter components based on the dispute kit type. The Shutter component receives all necessary props including dispute and currentPeriodIndex.
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/vite.config.js (1)
26-33: Consider adding explanatory comment for Shutter SDK file copyingThe implementation correctly uses the viteStaticCopy plugin to ensure Shutter SDK files are available in the build. However, adding a brief comment explaining why this copying is necessary would improve maintainability.
viteStaticCopy({ + // Copy Shutter SDK files to prevent 404 errors for blst.js and blst.wasm targets: [ { src: resolve(__dirname, "../node_modules/@shutter-network/shutter-sdk/dist/*"), dest: "node_modules/@shutter-network/shutter-sdk/dist", }, ], }),
📜 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 (2)
web/package.json(2 hunks)web/vite.config.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/package.json
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: SonarCloud
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Mend Security Check
🔇 Additional comments (2)
web/vite.config.js (2)
5-7: Good additions for SDK asset handlingThese imports are correctly added to support the Shutter SDK integration, allowing for proper file path resolution in the Vite configuration.
9-9: Proper implementation of __dirname in ES modulesThis correctly implements the CommonJS-style
__dirnamevariable in an ES module environment, which is necessary for reliable path resolution regardless of the working directory.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (2)
25-25:⚠️ Potential issueCritical: Fix undefined
voteIDscausing runtime crashes.This is the same critical issue flagged in previous reviews.
voteIDscan beundefinedwhendrawDatahasn't loaded, causing crashes whenvoteIDs.map(...)is called downstream inCommit.tsx.Apply the previously suggested fix:
-const voteIDs = useMemo(() => drawData?.draws?.map((draw) => draw.voteIDNum) as string[], [drawData]); +const voteIDs = useMemo( + () => drawData?.draws?.map((d) => d.voteIDNum) ?? [], + [drawData] +);
27-31:⚠️ Potential issueAdd guard condition to prevent rendering with invalid data.
The conditional rendering should guard against empty
voteIDsto prevent dispatching empty commits and potential crashes in child components.Update the conditional rendering logic:
- return id && isCommitPeriod && !commited ? ( - <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch, dispute, currentPeriodIndex }} /> - ) : id && isVotingPeriod ? ( - <Reveal {...{ setIsOpen, voteIDs }} /> - ) : null; + if (!id || voteIDs.length === 0) return null; + + if (isCommitPeriod && !commited) { + return <ShutterCommit arbitrable={arbitrable} setIsOpen={setIsOpen} voteIDs={voteIDs} refetch={refetch} dispute={dispute} currentPeriodIndex={currentPeriodIndex} />; + } + + if (isVotingPeriod) { + return <Reveal setIsOpen={setIsOpen} voteIDs={voteIDs} />; + } + + return null;
🧹 Nitpick comments (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (1)
95-105: Accessibility & UX nit – button hidden without contextWhen
parsedStoredDatais undefined nothing is rendered, leaving the user clueless.
Consider displaying a disabled button or an informational message (“No committed vote found on this device”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Mend Security Check
🔇 Additional comments (3)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (3)
71-88:wrapWithToastpromise shape is assumed – verify returned object
wrapWithToastis chained with.then(({ status }) => …).
If the util returns a plain transaction hash or void, this will throw and the UI stays in the loading state forever.Please double-check the helper’s contract or defensively handle the return type:
-).then(({ status }) => { - if (status) { +).then((res) => { + const succeeded = typeof res === "object" ? res.status : Boolean(res); + if (succeeded) { removeStoredData(); } - setIsOpen(status); + setIsOpen(succeeded); });
38-39: Minor:isRevealingstate could be derivedBecause the only transition to
trueis right beforeawait walletClient.writeContract, you can eliminate the explicit state and useisPendingfrom the toast / transaction hook, reducing state complexity.
31-33: 🛠️ Refactor suggestionLocal-storage key can silently collide for multiple vote sets
voteIDsis stringified by implicit array → string conversion, resulting in a comma-separated list (e.g."1,2,3").
Two different permutations (["1","2"]vs["2","1"]) map to the same key, so storing both will overwrite each other.-const saltKey = useMemo(() => `shutter-dispute-${id}-voteids-${voteIDs}`, [id, voteIDs]); +const saltKey = useMemo( + () => `shutter-dispute-${id}-voteids-${[...voteIDs].sort().join("-")}`, // or JSON.stringify(voteIDs) + [id, voteIDs] +);Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (2)
24-24: The voteIDs type assertion is still unsafe and can cause runtime crashes.The memoized
voteIDscomputation still suffers from the same issue mentioned in past reviews. WhendrawData?.drawsis undefined, the expression evaluates toundefined, but the type assertionas string[]masks this, creating a type safety violation.Apply this fix to handle undefined cases properly:
-const voteIDs = useMemo(() => drawData?.draws?.map((draw) => draw.voteIDNum) as string[], [drawData]); +const voteIDs = useMemo( + () => drawData?.draws?.map((draw) => draw.voteIDNum) ?? [], + [drawData] +);
26-30: Add guard conditions to prevent rendering with invalid data.The conditional rendering still doesn't handle the case where
voteIDscould be empty or undefined, which could lead to downstream components receiving invalid data.Add proper guards to prevent rendering with invalid state:
-return id && isCommitPeriod && !commited ? ( - <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch, dispute, currentPeriodIndex }} /> -) : id && isVotingPeriod ? ( - <Reveal {...{ setIsOpen, voteIDs }} /> -) : null; +if (!id || voteIDs.length === 0) return null; + +if (isCommitPeriod && !commited) { + return <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch, dispute, currentPeriodIndex }} />; +} + +if (isVotingPeriod) { + return <Reveal {...{ setIsOpen, voteIDs }} />; +} + +return null;web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (1)
58-69: Simulation hook args still need conditional guarding.While the
enabledcondition prevents the query from running, wagmi hooks can still evaluate theargsarray before checkingenabled, potentially causing BigInt conversion errors whenparsedStoredDatais undefined.Conditionally set the args to prevent evaluation errors:
const { data: simulateData, isLoading: isSimulating, error: simulateError, } = useSimulateDisputeKitShutterCastVoteShutter({ query: { enabled: !isUndefined(parsedStoredData), }, - args: [ - parsedDisputeID, - parsedVoteIDs, - BigInt(parsedStoredData?.choice ?? 0), - BigInt(parsedStoredData?.salt ?? 0), - parsedStoredData?.justification ?? "", - ], + args: !isUndefined(parsedStoredData) ? [ + parsedDisputeID, + parsedVoteIDs, + BigInt(parsedStoredData.choice), + BigInt(parsedStoredData.salt), + parsedStoredData.justification, + ] : undefined, });
🧹 Nitpick comments (2)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (2)
31-31: Potential issue with localStorage key generation.The
saltKeyincludes thevoteIDsarray directly in string interpolation, which could result in inconsistent keys if the array order changes or contains complex objects.Use a more stable key generation approach:
-const saltKey = useMemo(() => `shutter-dispute-${id}-voteids-${voteIDs}`, [id, voteIDs]); +const saltKey = useMemo(() => `shutter-dispute-${id}-voteids-${voteIDs.sort().join(',')}`, [id, voteIDs]);
81-86: Enhance error handling in transaction execution.The transaction execution could benefit from more specific error handling and user feedback. The current implementation only logs errors to console without providing meaningful feedback to the user.
Consider adding more comprehensive error handling:
await wrapWithToast(async () => await walletClient.writeContract(request), publicClient).then(({ status }) => { if (status) { removeStoredData(); + } else { + console.error("Transaction failed"); } setIsOpen(status); +}).catch((error) => { + console.error("Transaction error:", error); + // Consider showing user-friendly error message });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx(1 hunks)web/src/utils/shutter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/utils/shutter.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
🔇 Additional comments (3)
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (1)
22-22: Good improvement using dispute prop directly.The component now correctly uses the
disputeprop instead of fetching dispute data redundantly, which addresses the past review comment about unnecessary network requests.web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (2)
44-44: Good fix for zero value handling.The component now correctly uses
isUndefined()checks instead of falsy checks, which addresses the past review comment about legitimate zero values being treated as invalid. This ensures that votes with choice=0 or salt=0 can be properly revealed.
95-106: Good conditional rendering and user experience.The component correctly shows the reveal button only when stored data is available and handles loading states appropriately. The disabled state logic properly prevents user interaction during simulation or when errors occur.
|
PR-Codex overview
This PR enhances the voting functionality in a dispute resolution system by integrating a new
Shuttercomponent, improving dispute handling, and adding relevant hooks and utilities for better interaction with the Shutter API.Detailed summary
Shuttercomponent for handling voting and commitment.getDeadlineto be exported.disputein theVotingcomponent.ClassicandShutterdispute kits.Fundcomponent for appeal funding with appropriate hooks and error handling.Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores