Court UI testing implementation#2212
Conversation
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds local Hardhat/localhost support, a mockable Web3 provider, hardhat-aware artifact/wagmi handling, typed action-builder framework for commit/reveal/vote flows, crypto/storage helpers, vote/commit/reveal builders/hooks, many frontend chain-id/type fixes, and extensive unit + e2e test infra and scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Hook as useCastCommit
participant Builder as CommitBuilder
participant Storage as localStorage
participant Wallet as WalletClient
participant Contract as On‑chain Contract
UI->>Hook: user triggers cast commit
Hook->>Builder: buildCommitTxn(params, context)
Builder->>Storage: storeCommitData(key, {choice, salt})
Builder-->>Hook: txPayload (account,address,abi,fn,args,chain)
Hook->>Wallet: walletClient.writeContract(txPayload)
Wallet->>Contract: send transaction
Contract-->>Wallet: tx hash / receipt
Wallet-->>Hook: tx result
Hook->>UI: notify success/failure (toast)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @contracts/deploy/utils/deployTokens.ts:
- Around line 24-25: Fix the typo in the inline comment inside deployTokens.ts:
change "ERC20contract" to "ERC20 contract" in the comment that explains swapping
the local ERC20 implementation to one with increaseAllowance (refer to the
comment containing "increaseAllowance" in deployTokens.ts).
In @contracts/src/token/mock/PinakionV2Local.sol:
- Around line 25-34: The recoverTokens function incorrectly wraps
SafeERC20.safeTransfer (which returns void) in a require and uses .send for ETH;
remove the require and call the safeTransfer library call directly (e.g.,
token.safeTransfer(owner(), balance) or SafeERC20.safeTransfer(token, owner(),
balance) depending on imports), and replace
payable(owner()).send(address(this).balance) with a call pattern that bubbles
errors (e.g., (bool success, ) = payable(owner()).call{value:
address(this).balance}(""); require(success, "ETH transfer failed");) so
failures revert correctly; keep references to recoverTokens, token,
safeTransfer, and owner() when making the edits.
In @README.md:
- Around line 223-239: Fix the malformed link fragment and heading level:
replace the fragment text "#####shell-1---local-rpc-with-contracts-deployed"
used in the [hardhat node] link with a proper fragment starting with a single
hash (e.g., "#shell-1---local-rpc-with-contracts-deployed"), and change the
"#### Step 2: Start the Docker compose stack" heading to "### Step 2: Start the
Docker compose stack" so the heading hierarchy flows from "##" to "###"; also
ensure the localhost URL is written as <http://localhost> for consistency.
In @web/src/consts/processEnvConsts.ts:
- Line 23: The file exports an unused legacy function isLocalDeployment that
reads process.env.REACT_APP_DEPLOYMENT and duplicates behavior already
implemented in index.ts; either delete web/src/consts/processEnvConsts.ts
entirely, or move/merge the isLocalDeployment export into
web/src/consts/index.ts replacing process.env.REACT_APP_DEPLOYMENT with
import.meta.env.REACT_APP_DEPLOYMENT to match Vite usage, and remove any
duplicate exports so only the index.ts implementation remains.
🧹 Nitpick comments (5)
contracts/src/token/mock/PinakionV2Local.sol (1)
36-51: Add a comment explaining whyincreaseAllowanceanddecreaseAllowanceare manually implemented.These functions were removed from OpenZeppelin ERC20 in v5 as non-standard helpers, but your contract reimplements them. Adding a comment clarifying the rationale (compatibility with existing code, backwards compatibility, etc.) will help future maintainers understand the intent.
contracts/deploy/utils/index.ts (1)
31-32: Consider clarifying the comment.The comment mentions the network name is "hardhat" when deployed while starting the node, but the code checks for
network.name === "localhost". While thechainIdcheck correctly handles both cases (covering "hardhat" name via the chainId), the comment could be clearer.🔎 Suggested comment improvement
-// when deployed while starting node, the network name is "hardhat", the common factor for determining local node is chainId +// The network name can be "localhost" or "hardhat" (when deployed while starting the node). +// ChainId 31337 is the common factor for reliably detecting local Hardhat nodes. export const isLocalhost = (network: Network) => network.name === "localhost" || network.config.chainId === 31337;contracts/deploy/utils/deployTokens.ts (1)
26-27: Consider extracting the repeated condition.The condition
ticker === "PNK" && isLocalhost(hre.network)is repeated on consecutive lines. While this works correctly, extracting it to a variable would improve maintainability.🔎 Suggested refactor
- const contractName = ticker === "PNK" && isLocalhost(hre.network) ? "PinakionV2Local" : "TestERC20"; - const args = ticker === "PNK" && isLocalhost(hre.network) ? [] : [ticker, ticker]; + const isPNKLocalDeployment = ticker === "PNK" && isLocalhost(hre.network); + const contractName = isPNKLocalDeployment ? "PinakionV2Local" : "TestERC20"; + const args = isPNKLocalDeployment ? [] : [ticker, ticker];web/src/context/Web3Provider.tsx (1)
53-56: Potential undefined access indefaultTransportwebsocket fallback.
chain.rpcUrls.default?.webSocket?.[0]may beundefinedfor chains that don't provide a default WebSocket URL. WhilewebSocket(undefined)is handled gracefully by viem (it becomes a no-op in the fallback), consider making the fallback more explicit or adding a comment explaining this behavior.🔎 Optional: Make undefined handling explicit
const defaultTransport = (chain: AppKitNetwork) => - fallback([http(chain.rpcUrls.default?.http?.[0]), webSocket(chain.rpcUrls.default?.webSocket?.[0])]); + fallback([ + http(chain.rpcUrls.default?.http?.[0]), + // WebSocket may not be available for all chains; fallback handles undefined gracefully + ...(chain.rpcUrls.default?.webSocket?.[0] ? [webSocket(chain.rpcUrls.default.webSocket[0])] : []), + ]);contracts/wagmi.config.hardhat.ts (1)
2-2: Same import assertion syntax note asweb/wagmi.config.ts.Biome flags the
assert { type: "json" }syntax. Consider updating towith { type: "json" }for consistency if your TypeScript/bundler versions support import attributes.🔎 Proposed update to import attributes syntax
-import IHomeGateway from "./artifacts/src/gateway/interfaces/IHomeGateway.sol/IHomeGateway.json" assert { type: "json" }; +import IHomeGateway from "./artifacts/src/gateway/interfaces/IHomeGateway.sol/IHomeGateway.json" with { type: "json" };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
README.mdcontracts/deploy/00-home-chain-arbitration.tscontracts/deploy/00-rng-chainlink.tscontracts/deploy/utils/deployTokens.tscontracts/deploy/utils/index.tscontracts/deployments/hardhat.viem.tscontracts/hardhat.config.tscontracts/package.jsoncontracts/src/token/mock/PinakionV2Local.solcontracts/wagmi.config.hardhat.tsservices/graph-node/docker-compose.ymlsubgraph/scripts/all.shweb/.env.local.publicweb/src/consts/chains.tsweb/src/consts/index.tsweb/src/consts/processEnvConsts.tsweb/src/context/Web3Provider.tsxweb/src/utils/getGraphqlUrl.tsweb/wagmi.config.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
contracts/deploy/00-home-chain-arbitration.tscontracts/deploy/utils/index.tsweb/src/consts/processEnvConsts.tsweb/src/consts/index.tscontracts/deploy/00-rng-chainlink.ts
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 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.
Applied to files:
web/src/utils/getGraphqlUrl.ts
📚 Learning: 2024-10-15T16:18:32.543Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1687
File: web/src/context/AtlasProvider.tsx:225-244
Timestamp: 2024-10-15T16:18:32.543Z
Learning: In `web/src/context/AtlasProvider.tsx`, the `atlasUri` variable comes from environment variables and does not change, so it does not need to be included in dependency arrays.
Applied to files:
web/.env.local.publicweb/src/consts/index.ts
📚 Learning: 2024-10-22T09:38:20.093Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:1-1
Timestamp: 2024-10-22T09:38:20.093Z
Learning: In the TypeScript file `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `Abi` type is parsed later in the action functions, so importing `Abi` from `viem` in this file is unnecessary.
Applied to files:
web/wagmi.config.ts
📚 Learning: 2025-09-03T22:48:32.972Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Applied to files:
contracts/src/token/mock/PinakionV2Local.sol
🧬 Code graph analysis (6)
contracts/deploy/00-home-chain-arbitration.ts (1)
contracts/deploy/utils/index.ts (2)
isDevnet(30-30)isLocalhost(32-32)
web/src/consts/processEnvConsts.ts (1)
web/src/consts/index.ts (1)
isLocalDeployment(33-33)
web/src/consts/index.ts (1)
web/src/consts/processEnvConsts.ts (1)
isLocalDeployment(23-23)
contracts/deploy/00-rng-chainlink.ts (1)
contracts/deploy/utils/index.ts (1)
isLocalhost(32-32)
web/src/context/Web3Provider.tsx (3)
contracts/deploy/utils/index.ts (1)
isLocalhost(32-32)web/src/consts/index.ts (2)
isLocalDeployment(33-33)HARDHAT_NODE_RPC(53-53)web/src/consts/processEnvConsts.ts (1)
isLocalDeployment(23-23)
web/src/consts/chains.ts (2)
web/src/consts/processEnvConsts.ts (3)
DEFAULT_CHAIN(32-32)isLocalDeployment(23-23)isProductionDeployment(22-22)web/src/consts/index.ts (2)
isLocalDeployment(33-33)isProductionDeployment(31-31)
🪛 Biome (2.1.2)
web/wagmi.config.ts
[error] 12-12: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 12-12: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
contracts/wagmi.config.hardhat.ts
[error] 2-2: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 2-2: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
🪛 dotenv-linter (4.0.0)
web/.env.local.public
[warning] 3-3: [UnorderedKey] The REACT_APP_CORE_SUBGRAPH key should go before the REACT_APP_DEPLOYMENT key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
README.md
113-113: Dollar signs used before commands without showing output
(MD014, commands-show-output)
225-225: Link fragments should be valid
(MD051, link-fragments)
227-227: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
239-239: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Mend Security Check
🔇 Additional comments (27)
contracts/hardhat.config.ts (1)
60-63: LGTM!The mining configuration with
auto: trueand a 5-second interval provides predictable block times for local development and testing. This is a standard pattern for simulating realistic block production while maintaining fast feedback loops.services/graph-node/docker-compose.yml (2)
26-26: Acknowledge local-only configuration for non-deterministic fulltext search.Enabling
GRAPH_ALLOW_NON_DETERMINISTIC_FULLTEXT_SEARCHis appropriate for local development but should never be used in production as it can cause inconsistent query results across Graph Node instances. This is acceptable since this docker-compose is for local development only.
38-38: LGTM!The postgres command reformatting is a non-functional change that improves readability.
contracts/package.json (2)
63-65: LGTM!Adding the
Resolvertag to bothstart-localanddeploy-localscripts ensures consistent local deployment of resolver contracts alongside arbitration components.
70-70: LGTM!The
populate:localscript provides a convenient way to seed local development with courts and policy registry data fromv2_devnet, streamlining the local setup workflow.web/.env.local.public (1)
2-5: LGTM!The environment variable updates correctly configure the web app for local development, pointing to local Graph Node endpoints and setting the deployment mode to
localhost. This aligns with the broader local development infrastructure changes in this PR.subgraph/scripts/all.sh (1)
13-13: The removal ofcore-universityfrom the batch loop is intentional and safe.The
core-universitysubgraph remains fully functional with dedicated npm scripts inpackage.json(lines 24-32), including update, codegen, build, test, deploy, and rebuild commands. The change toall.shis a deliberate scope reduction for the batch automation script, allowing developers to runcoreanddrtsubgraphs together while invokingcore-universitycommands individually when needed. This does not break any workflows.contracts/deploy/00-rng-chainlink.ts (2)
3-3: LGTM!The addition of
isLocalhostto the import aligns with the local development support introduced across the deployment scripts.
79-79: LGTM!The conditional timeout logic is well-designed: 10 seconds for local development (appropriate for the 5-second Hardhat mining interval) and 30 minutes for production networks.
web/src/utils/getGraphqlUrl.ts (2)
1-1: LGTM!The addition of
hardhatto the wagmi/chains import enables local development support.
13-14: LGTM!The hardhat chain mapping follows the same pattern as other chains, using an environment variable with an appropriate fallback message.
contracts/deploy/00-home-chain-arbitration.ts (2)
6-6: LGTM!The addition of
isLocalhostto the imports enables localhost detection for devnet-equivalent behavior.
50-52: LGTM!Treating localhost as devnet-equivalent for timing constants is appropriate. The shorter
minStakingTime(180s vs 1800s) andmaxFreezingTime(600s vs 1800s) enable faster testing cycles during local development.contracts/deploy/utils/deployTokens.ts (2)
4-4: LGTM!The addition of
isLocalhostto the imports follows the existing pattern.
28-33: LGTM!The conditional contract deployment correctly uses
PinakionV2Localfor PNK on localhost andTestERC20otherwise, with appropriate constructor arguments for each case.web/src/consts/chains.ts (2)
14-18: LGTM - Local deployment chain selection is correctly prioritized.The ternary chain correctly checks
isLocalDeployment()first, then falls back to the production/testnet logic. This ensures local development uses Hardhat while preserving existing behavior for other environments.
21-27: The code correctly handles the chain/transport mismatch for local deployments. WhileQUERY_CHAINSincludes gnosis/gnosisChiado and mainnet regardless of deployment type, thecreateAppKitconfiguration usesallowUnsupportedChain: true(line 110 of Web3Provider.tsx) which gracefully allows chains without corresponding transports. Additionally, mainnet is included in the localhost transport for ENS resolution. Since gnosis/gnosisChiado are read-only query chains not needed during local development, no changes are required.web/src/consts/index.ts (2)
52-53: LGTM - Standard Hardhat RPC endpoint.The
HARDHAT_NODE_RPCconstant uses the default Hardhat network address. This is the standard endpoint for local Hardhat nodes.
33-33: TheisLocalDeploymentfunction inindex.tsis the only active version—processEnvConsts.tsis not imported anywhere in the codebase.The concern about duplication is partially valid, but
processEnvConsts.ts(usingprocess.env) appears to be unused or legacy code. Only theimport.meta.envversion inindex.tsis actively imported and used bychains.tsandWeb3Provider.tsx. Consider clarifying whetherprocessEnvConsts.tsshould be maintained or removed.Likely an incorrect or invalid review comment.
web/src/context/Web3Provider.tsx (2)
67-71: LGTM - Local transport correctly includes mainnet for ENS resolution.The local transport configuration properly includes both the Hardhat node and mainnet (via Alchemy) to support ENS resolution during local development.
98-108: LGTM - SDK and AppKit configuration consistently updated for local deployment.Both
configureSDKandcreateAppKituse the sameisLocalhostcheck to select the appropriate chain and transport, maintaining consistency across the Web3 stack.README.md (1)
110-123: LGTM - Clear instructions for local data population and artifact generation.The new section clearly documents the required steps after local deployment, including the court population and wagmi/viem bindings regeneration.
web/wagmi.config.ts (3)
92-96: LGTM - Localhost deployment case correctly configured.The new
localhostcase properly setsviemNetworkto"hardhat",hardhatNetworkto"localhost", and useshardhatViemfor the arbitrator contracts, maintaining consistency with the artifact directory structure.
20-20: Good design: OptionalhardhatChainNameparameter enables flexible artifact resolution.The
readArtifactsfunction now accepts an optionalhardhatChainNameto decouple the viem chain name from the Hardhat deployment directory name. This elegantly handles cases likelocalhost/hardhatmapping.Also applies to: 51-51
12-16: Theassert { type: "json" }syntax is the standard ES2022 import assertions syntax and is fully supported by TypeScript 5.6.3 with module set to ESNext. This same syntax is used consistently across 7 files in the project (web/wagmi.config.ts, web-devtools/wagmi.config.ts, web/scripts/gitInfo.js, and multiple wagmi config files). While TypeScript 5.5+ nominally preferswith { type: "json" }for forward compatibility,assertremains valid and functional. The suggestion to update only this one file creates an inconsistency with the rest of the codebase and is not a functional requirement.contracts/wagmi.config.hardhat.ts (2)
5-27: LGTM - Async config with multi-network artifact discovery.The async
getConfigfunction properly:
- Reads artifacts from localhost, gnosisChiado, and sepolia networks
- Logs discovered artifacts for debugging visibility
- Merges artifacts across networks using the helper
- Includes
IHomeGatewayin the final outputThe comment on line 10 about network renaming breaking other scripts is a useful note for future maintainers.
3-3: No action required. ThewagmiHelpersmodule exists atcontracts/scripts/wagmiHelpers.tsand properly exports all three required functions:getAbi(line 11),readArtifacts(line 15), andmerge(line 45).
jaybuidl
added
Type: Toolchain ⚒️
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @web/src/actions/commit/helpers/index.ts:
- Around line 1-4: encodeShutterMessage currently uses choice.toString() but
relies on implicit conversion for salt; update the function
(encodeShutterMessage) to call salt.toString() as well when building the string
with SEPARATOR and justification to keep both bigint parameters handled
consistently.
In @web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx:
- Around line 37-44: The call to castCommit passes currentRoundIndex which can
be undefined (derived from disputeData?.dispute?.currentRoundIndex); guard it
before calling castCommit by returning early or showing an error/loading state
when currentRoundIndex is undefined, e.g., check if currentRoundIndex == null
and abort the commit flow (do not call castCommit) or provide a valid default,
then only call castCommit with the validated roundIndex and
setIsOpen(res.status) on success.
In @web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx:
- Around line 55-64: The call to castCommit uses currentRoundIndex which can be
undefined; before invoking castCommit in Commit.tsx check that currentRoundIndex
is a valid number (e.g., !== undefined/null) and bail out or show an error if
not set so you never pass undefined as roundIndex; update the code path that
currently calls castCommit({ ..., roundIndex: currentRoundIndex, ... }) to guard
on currentRoundIndex and only call castCommit when it is defined, referencing
the same symbols (castCommit, currentRoundIndex,
DisputeKits.GatedShutter/DisputeKits.Shutter, setIsOpen) so the UI handles the
missing round index safely.
🧹 Nitpick comments (9)
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
5-5: Unused import.
useLocalStorageis imported but no longer used after the refactor to directlocalStorage.getItem().-import { useLocalStorage } from "react-use";web/src/utils/crypto/generateSalt.ts (1)
15-16: Unnecessary optional chaining after validation.The optional chaining
signingAccount?.signMessageon line 15 is redundant sincesigningAccount.signMessageis already validated to exist on line 11. This is a minor inconsistency.♻️ Suggested cleanup
- const signature = await signingAccount?.signMessage({ message }); + const signature = await signingAccount.signMessage({ message });web/src/hooks/useCastCommit.tsx (1)
67-67: Hardcoded query key may become out of sync.The query key
["useDrawQuery"]is hardcoded. If the actual query definition uses a different key or includes additional parameters, this invalidation won't work as expected. Consider centralizing query keys to avoid mismatches.web/src/actions/commit/builders/index.ts (1)
14-23: Consider improving type safety in the builder registry.The
anytype in theRecordandas nevercast work correctly at runtime due to the discriminated union, but they bypass compile-time type checking. This is a common pattern for dispatch tables, but worth noting.If stricter typing is desired later, an overloaded function signature could enforce the relationship between
params.typeand the expected param type.web/src/actions/commit/builders/gated.builder.ts (1)
8-22: Consider validating chain support before address lookup.The address lookup
disputeKitGatedAddress[chain.id]will returnundefinedfor unsupported chains, which would cause a transaction failure. While the UI likely restricts to supported chains, a defensive check could provide a clearer error message.♻️ Optional defensive check
export const gatedCommitBuilder: CommitBuilder<GatedCommitParams, typeof disputeKitGatedAbi> = { build: async (params, context) => { const { disputeId, voteIds, choice, salt } = params; const { chain, account } = context; + const address = disputeKitGatedAddress[chain.id]; + if (!address) { + throw new Error(`DisputeKitGated not deployed on chain ${chain.id}`); + } + const commit = hashVote(choice, salt); return { account, - address: disputeKitGatedAddress[chain.id], + address, abi: disputeKitGatedAbi, functionName: "castCommit", args: [disputeId, voteIds, commit], chain, }; }, };web/src/actions/commit/builders/gatedShutter.builder.ts (1)
27-28: RedundantBigInt()conversion onsalt.According to
BaseCommitParamsinparams.ts,saltis already typed asbigint. TheBigInt(salt)calls are unnecessary and could be simplified.Suggested simplification
- const choiceCommit = hashVote(choice, BigInt(salt)); - const justificationCommit = hashJustification(BigInt(salt), justification); + const choiceCommit = hashVote(choice, salt); + const justificationCommit = hashJustification(salt, justification);web/src/actions/commit/builders/shutter.builder.ts (2)
13-38: Significant code duplication withgatedShutter.builder.ts.This builder shares nearly identical logic with
gatedShutter.builder.ts(env validation, message encoding, encryption, hashing). Consider extracting shared Shutter-specific logic into a helper function to reduce duplication.
27-28: RedundantBigInt()conversion onsalt.Same issue as
gatedShutter.builder.ts-saltis alreadybigintperBaseCommitParams.Suggested simplification
- const choiceCommit = hashVote(choice, BigInt(salt)); - const justificationCommit = hashJustification(BigInt(salt), justification); + const choiceCommit = hashVote(choice, salt); + const justificationCommit = hashJustification(salt, justification);web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
37-39: Duplicate data fetching for dispute details.The component receives
disputeas a prop (line 25) but also callsuseDisputeDetailsQuery(id)(line 37) just to getcurrentRoundIndex. Consider usingdispute?.currentRoundIndexfrom the prop instead to avoid redundant network requests.Suggested change
- const { data: disputeData } = useDisputeDetailsQuery(id); - - const currentRoundIndex = disputeData?.dispute?.currentRoundIndex; + const currentRoundIndex = dispute?.currentRoundIndex;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
web/package.jsonweb/src/actions/commit/builders/baseBuilder.tsweb/src/actions/commit/builders/classic.builder.tsweb/src/actions/commit/builders/gated.builder.tsweb/src/actions/commit/builders/gatedShutter.builder.tsweb/src/actions/commit/builders/index.tsweb/src/actions/commit/builders/shutter.builder.tsweb/src/actions/commit/context.tsweb/src/actions/commit/execute.tsweb/src/actions/commit/helpers/index.tsweb/src/actions/commit/params.tsweb/src/hooks/queries/useDrawQuery.tsweb/src/hooks/useCastCommit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/utils/crypto/generateSalt.tsweb/src/utils/crypto/hashJustification.tsweb/src/utils/crypto/hashVote.tsweb/src/utils/crypto/shutter.tsweb/tsconfig.json
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 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.
Applied to files:
web/src/hooks/queries/useDrawQuery.tsweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
Repo: kleros/kleros-v2 PR: 1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/hooks/queries/useDrawQuery.tsweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
web/src/actions/commit/execute.tsweb/src/actions/commit/builders/gated.builder.tsweb/src/hooks/useCastCommit.tsxweb/src/actions/commit/params.tsweb/src/actions/commit/context.tsweb/src/actions/commit/builders/classic.builder.tsweb/src/actions/commit/builders/shutter.builder.tsweb/src/actions/commit/builders/index.tsweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/actions/commit/params.tsweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2024-12-16T17:17:32.359Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1794
File: web/src/hooks/useStarredCases.tsx:13-18
Timestamp: 2024-12-16T17:17:32.359Z
Learning: In `useStarredCases.tsx`, when handling the `starredCases` Map from local storage, direct mutation is acceptable to prevent the overhead of copying, provided it doesn't adversely affect React's render cycle.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2024-10-09T10:22:41.474Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1582
File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90
Timestamp: 2024-10-09T10:22:41.474Z
Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2024-12-09T12:36:59.441Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx
📚 Learning: 2025-09-09T13:33:46.896Z
Learnt from: tractorss
Repo: kleros/kleros-v2 PR: 2117
File: web/src/components/DisputeFeatures/Features/GatedErc1155.tsx:51-66
Timestamp: 2025-09-09T13:33:46.896Z
Learning: The `setDisputeData` function in `NewDisputeContext` at `web/src/context/NewDisputeContext.tsx` has signature `(disputeData: IDisputeData) => void` and only accepts direct state values, not functional updates like standard React state setters. It cannot be used with the pattern `setDisputeData((prev) => ...)`.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2025-12-17T14:55:22.988Z
Learnt from: kemuru
Repo: kleros/kleros-v2 PR: 1871
File: web/src/pages/Profile/Votes/index.tsx:69-75
Timestamp: 2025-12-17T14:55:22.988Z
Learning: In `web/src/pages/Profile/Votes/index.tsx`, fetching up to 1000 draws client-side is a temporary solution until the subgraph supports fetching `totalVotes` directly. This is acknowledged technical debt that will be addressed when the subgraph capability becomes available.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
Repo: kleros/kleros-v2 PR: 1632
File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42
Timestamp: 2024-06-27T10:11:54.861Z
Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2025-05-09T13:39:15.086Z
Learnt from: tractorss
Repo: kleros/kleros-v2 PR: 1982
File: web/src/pages/Resolver/Parameters/NotablePersons/PersonFields.tsx:64-0
Timestamp: 2025-05-09T13:39:15.086Z
Learning: In PersonFields.tsx, the useEffect hook for address validation intentionally uses an empty dependency array to run only once on component mount. This is specifically designed for the dispute duplication flow when aliasesArray is already populated with addresses that need initial validation.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
🧬 Code graph analysis (11)
web/src/actions/commit/execute.ts (3)
web/src/actions/commit/params.ts (1)
CommitParams(32-32)web/src/actions/commit/context.ts (1)
CommitContext(3-8)web/src/actions/commit/builders/index.ts (1)
buildCommitTxn(21-23)
web/src/actions/commit/builders/gated.builder.ts (3)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
GatedCommitParams(22-24)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)
web/src/hooks/useCastCommit.tsx (3)
web/src/actions/commit/params.ts (5)
ClassicCommitParams(12-14)ShutterCommitParams(16-20)GatedCommitParams(22-24)GatedShutterCommitParams(26-30)CommitParams(32-32)web/src/utils/crypto/generateSalt.ts (1)
generateSalt(10-19)web/src/actions/commit/execute.ts (1)
executeCommit(9-13)
web/src/actions/commit/params.ts (1)
web/src/consts/disputeFeature.ts (1)
DisputeKits(32-32)
web/src/actions/commit/builders/classic.builder.ts (3)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
ClassicCommitParams(12-14)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)
web/src/actions/commit/builders/shutter.builder.ts (6)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
ShutterCommitParams(16-20)web/src/actions/commit/helpers/index.ts (1)
encodeShutterMessage(2-4)web/src/utils/crypto/shutter.ts (1)
encrypt(151-175)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)web/src/utils/crypto/hashJustification.ts (1)
hashJustification(3-15)
web/src/actions/commit/builders/gatedShutter.builder.ts (6)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
GatedShutterCommitParams(26-30)web/src/actions/commit/helpers/index.ts (1)
encodeShutterMessage(2-4)web/src/utils/crypto/shutter.ts (1)
encrypt(151-175)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)web/src/utils/crypto/hashJustification.ts (1)
hashJustification(3-15)
web/src/actions/commit/builders/baseBuilder.ts (2)
web/src/actions/commit/params.ts (1)
CommitParams(32-32)web/src/actions/commit/context.ts (1)
CommitContext(3-8)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (4)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
useDisputeDetailsQuery(46-62)web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
getDeadline(145-156)web/src/hooks/useCastCommit.tsx (1)
useCastCommit(25-73)web/src/consts/disputeFeature.ts (1)
DisputeKits(32-32)
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx (2)
web/src/hooks/queries/useDrawQuery.ts (1)
useDrawQuery(23-37)web/src/hooks/useVotingContext.tsx (1)
useVotingContext(134-134)
web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (3)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
useDisputeDetailsQuery(46-62)web/src/hooks/useCastCommit.tsx (1)
useCastCommit(25-73)web/src/consts/disputeFeature.ts (1)
DisputeKits(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
🔇 Additional comments (18)
web/package.json (1)
13-13: LGTM!The new
actionsalias is consistent with the existing alias pattern and aligns with the newly introducedsrc/actionsmodules.web/tsconfig.json (1)
15-17: LGTM!The
actions*path mapping correctly mirrors the package.json alias and follows the established pattern for other path aliases in this configuration.web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
70-76: LGTM on the localStorage change.Using
localStorage.getItem()directly during reveal is appropriate since you only need a one-time read of the stored salt/choice at execution time, rather than maintaining reactive state via the hook.web/src/hooks/queries/useDrawQuery.ts (1)
28-28: QueryKey is correctly structured for React Query cache management.The array format
["useDrawQuery", address, disputeID, roundID]is the proper approach, enabling React Query to properly compare and invalidate queries based on individual dependency parameters. The cache invalidation call inuseCastCommit.tsxcorrectly references the new key name, so no additional changes are needed.web/src/utils/crypto/hashVote.ts (1)
11-22: Consider edge case: empty string justification.The condition
justification === undefinedmeans an empty string""will be treated as a valid justification and hashed. If an empty justification should be treated the same as no justification, consider using a truthy check instead.If this is intentional (empty string is a valid justification distinct from none), then the implementation is correct.
web/src/utils/crypto/hashJustification.ts (1)
3-15: LGTM!The implementation correctly computes a deterministic justification hash using viem's cryptographic utilities. The ABI encoding structure is well-documented with inline comments.
web/src/actions/commit/context.ts (1)
3-8: LGTM!The
CommitContextinterface is well-structured and provides a clean abstraction for the commit workflow. The types from viem are appropriately used.web/src/hooks/useCastCommit.tsx (2)
25-72: Overall implementation looks good.The hook properly orchestrates the commit workflow with appropriate validation, salt generation, localStorage persistence, and query invalidation. The use of
useMutationfrom react-query is appropriate for this side-effect-heavy operation.
29-29: Missing validation forpublicClientbefore use.
publicClientis retrieved on line 29 but used with a non-null assertion on line 61 without prior validation. IfpublicClientis undefined (e.g., during initial render or network switching), this will cause a runtime error.🐛 Proposed fix
if (isUndefined(walletClient)) { throw new Error("WalletClient not defined. Is the wallet connected?"); } + + if (isUndefined(publicClient)) { + throw new Error("PublicClient not defined. Is the wallet connected?"); + }Then on line 61:
- const result = await wrapWithToast(executeTxn, publicClient!); + const result = await wrapWithToast(executeTxn, publicClient);Also applies to: 61-61
⛔ Skipped due to learnings
Learnt from: jaybuidl Repo: kleros/kleros-v2 PR: 2145 File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286 Timestamp: 2025-09-30T17:18:12.895Z Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.Learnt from: jaybuidl Repo: kleros/kleros-v2 PR: 1582 File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90 Timestamp: 2024-10-09T10:22:41.474Z Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1703 File: kleros-sdk/src/sdk.ts:1-3 Timestamp: 2024-10-22T10:23:15.789Z Learning: In `kleros-sdk/src/sdk.ts`, the `PublicClient` type is used and should not be flagged as unused.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1744 File: web/src/hooks/queries/useHomePageBlockQuery.ts:71-71 Timestamp: 2024-11-19T05:29:56.238Z Learning: In `web/src/hooks/queries/useHomePageBlockQuery.ts`, the non-null assertions on `blockNumber!` and `genesisBlock!` within `queryFn` are safe because `isEnabled` ensures that `queryFn` only runs when either `blockNumber` or `genesisBlock` is defined.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 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.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1703 File: kleros-sdk/src/sdk.ts:13-17 Timestamp: 2024-10-22T10:07:21.327Z Learning: In the SDK, `getPublicClient()` handles the scenario where `publicClient` is undefined by throwing `SdkNotConfiguredError`, so additional error handling in functions that call `getPublicClient()` is not necessary.web/src/actions/commit/execute.ts (1)
9-13: LGTM!Clean separation of transaction building and execution. The function correctly delegates to the centralized builder dispatch and executes via the wallet client. The return type (
Hash) is properly inferred fromwriteContract.web/src/actions/commit/builders/baseBuilder.ts (1)
1-16: LGTM - Well-designed builder interface.The generic interface cleanly constrains builder implementations while maintaining type safety with viem's
WriteContractParameters. This provides a solid foundation for the commit builder pattern.web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx (1)
25-31: LGTM - Simplified data flow with proper cache invalidation.The removal of
refetchfrom theuseDrawQuerydestructuring andCommitprops is a clean refactor. Query cache invalidation after commit is correctly handled in theuseCastCommitmutation'sonSuccesscallback viaqueryClient.invalidateQueries({ queryKey: ["useDrawQuery"] }), which is the idiomatic React Query pattern.web/src/actions/commit/builders/classic.builder.ts (1)
8-23: LGTM!Clean implementation following the builder pattern. The
hashVotecall correctly usessaltdirectly without redundant conversion.web/src/actions/commit/params.ts (2)
12-32: LGTM!Well-structured discriminated union types. The pattern enables type-safe dispatch to specific builders based on the
typefield.
1-10: No issues found.DisputeKitsis correctly defined as an enum insrc/consts/index.tswith the expected members (Classic, Shutter, Gated, GatedShutter), andparams.tsproperly imports and uses it as a type discriminator.web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (1)
26-47: LGTM on the refactored commit flow.The simplification using
useCastCommitis a good improvement over the previous multi-step wallet interaction. The dependency array is complete.web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
49-76: LGTM on callback structure.The decryption delay calculation with the 5-minute buffer is correctly implemented. The dependency array is complete.
web/src/actions/commit/builders/gatedShutter.builder.ts (1)
30-37: Add explicit chain support validation in all commit builders.The address lookups (
disputeKitGatedShutterAddress[chain.id], etc.) may returnundefinedif the chain is unsupported. While wagmi likely prevents unsupported chains from reaching this code, add an explicit guard for defensive programming, matching the pattern used inuseDisputeKitAddresses.ts:if (!(chain.id in disputeKitGatedShutterAddress)) { throw new Error(`Unsupported chain: ${chain.id}`); }This applies to all four builders:
classic.builder.ts,shutter.builder.ts,gated.builder.ts, andgatedShutter.builder.ts.⛔ Skipped due to learnings
Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1762 File: web/src/utils/parseWagmiError.ts:10-17 Timestamp: 2024-11-29T06:23:15.955Z Learning: In the `web/src/utils/parseWagmiError.ts` file and throughout the codebase, prefer using optional chaining to handle `undefined` or `null` values, including optional arrays, without adding explicit existence or length checks.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 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.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (1)
26-37: Ensurecommitis present before renderingReveal.The
commitvalue fromuseVotingContextis optional and derived fromdrawData?.draws?.[0]?.vote?.commit(useVotingContext.tsx:112), which can be undefined if data is still loading. SinceshouldShowRevealonly checksid && isVotingPeriodwithout validatingcommit, the component may render Reveal with an undefined value. While the type assertion masks this at compile time, Reveal will fail at runtime when the undefined commit is passed torevealVote.✅ Suggested fix
- const shouldShowReveal = id && isVotingPeriod; + const shouldShowReveal = id && isVotingPeriod && !!commit;
🤖 Fix all issues with AI agents
In `@web/src/actions/helpers/storage/index.ts`:
- Around line 3-11: Wrap all direct localStorage access in try/catch to prevent
exceptions from bubbling up: in storeCommitData (and the analogous functions
that read/remove commit data) perform JSON.stringify and localStorage.setItem
inside a try block, catch errors, optionally log the error (e.g., via
console.error) and fail gracefully (no throw) or return a safe fallback value;
ensure you preserve the StoredCommitData shape conversion (salt/choice
toString()) before attempting storage and do not change the function signatures.
In `@web/src/actions/reveal/builders/classic.builder.ts`:
- Around line 8-18: The build function in classic.builder.ts currently uses
disputeKitClassicAddress[chain.id] without guarding undefined; update the build
implementation to validate that disputeKitClassicAddress[chain.id] exists (e.g.,
const address = disputeKitClassicAddress[chain.id]) and if it is undefined throw
a clear error (e.g., "Unsupported chain id: {chain.id} for
disputeKitClassicAddress") before returning the transaction object so castVote
never gets built with an undefined address; ensure references to build,
ClassicRevealParams, disputeKitClassicAddress, and chain.id are used to locate
the fix.
In `@web/src/actions/reveal/resolveRevealInputs.ts`:
- Around line 20-23: The current logic always sets justification from params
then returns { ...params, ...stored, justification }, which discards
stored.justification when stored exists; change the justification resolution to
prefer stored.justification first, then params.justification, then empty string
(e.g., compute justification = stored?.justification ?? params.justification ??
""), and use that justification in the return path so resolveRevealInputs
preserves an existing stored justification.
In `@web/src/actions/vote/builders/index.ts`:
- Around line 9-18: The builders map and buildVoteTxn call use
builders[params.type] but VoteParams is currently only ClassicVoteParams (type:
DisputeKits.Classic), causing a type mismatch; fix by either (A) expanding
VoteParams to a union that includes GatedVoteParams, ShutterVoteParams, and
GatedShutterVoteParams (these can alias ClassicVoteParams if signatures are
identical) so params.type can be any DisputeKits and builders stays
Record<DisputeKits, VoteBuilder>, or (B) narrow builders to only include
[DisputeKits.Classic] and type it accordingly so buildVoteTxn uses a builders
keyed by Classic only; update the VoteParams/ClassicVoteParams and builders
declarations and ensure buildVoteTxn's params.type typing aligns with the chosen
approach (referencing VoteParams, ClassicVoteParams, builders, and
buildVoteTxn).
In `@web/src/hooks/useRevealVote.ts`:
- Around line 27-48: The code uses publicClient from usePublicClient() without
guarding it before calling wrapWithToast; add a check like the existing guards
for walletClient/account/chain in the mutationFn to throw a clear error if
publicClient is undefined, and ensure wrapWithToast is called only with a
validated publicClient (remove or avoid the non-null assertion publicClient!);
this touches the mutationFn in useRevealVote where executeTxn is wrapped with
wrapWithToast and should reference usePublicClient/publicClient.
In `@web/src/hooks/useVote.ts`:
- Around line 16-35: The code calls wrapWithToast(publicClient!) without
guarding publicClient; add a defensive check in useVote's mutationFn (same area
that checks account/chain/walletClient) to throw an Error like "Public client
not defined. Is the wallet initialized?" when usePublicClient() returns
undefined, and then pass publicClient (no non-null assertion) into
wrapWithToast; locate the logic around executeVote, wrapWithToast, and the
useMutation mutationFn to implement this change.
In `@web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx`:
- Around line 60-83: Guard against currentRoundIndex being undefined before
calling revealVote: in the handleReveal callback (Reveal.tsx) check that
currentRoundIndex is not undefined and return early (or show a validation) if it
is, so you do not call revealVote with an undefined round; reference the
revealVote call and the currentRoundIndex variable to locate the change. Also
disable the related Reveal button when currentRoundIndex is undefined (matching
the pattern used in Commit.tsx) so the user cannot trigger the action before
dispute data loads. Ensure the revealVote call still passes the same
params/context when currentRoundIndex is valid.
In `@web/src/pages/Cases/CaseDetails/Voting/Classic/Vote.tsx`:
- Around line 36-47: handleVote currently calls
BigInt(disputeData?.dispute?.currentRoundIndex) which can be BigInt(undefined)
while disputeData is still loading; update handleVote to guard the salt
computation by checking disputeData?.dispute?.currentRoundIndex is not
null/undefined before calling BigInt (or return early/disable voting when it's
missing), i.e. compute a local safeSalt only when currentRoundIndex is defined
and pass that to vote (or bail out if undefined) in the handleVote callback to
avoid calling BigInt on undefined.
In `@web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx`:
- Around line 31-63: Guard the reveal action until currentRoundIndex is
available: disable the Reveal button when currentRoundIndex is undefined (e.g.,
change Button disabled to disabled={isPending || currentRoundIndex ===
undefined}), and add a defensive check at the start of handleReveal to return
early if currentRoundIndex is undefined to avoid calling revealVote with an
invalid roundIndex; reference currentRoundIndex, handleReveal, revealVote,
parsedDisputeID, parsedVoteIDs, justification, isGated and the Button component
when making the changes.
♻️ Duplicate comments (2)
web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (1)
37-48: Guard againstundefinedcurrentRoundIndexbefore callingcastCommit.
currentRoundIndex(line 31) can beundefinedwhendisputeDatahasn't loaded. PassingundefinedtocastCommitwill propagate togetVoteKey()and salt generation, likely causing runtime issues.Suggested fix
const handleCommit = useCallback( async (choice: bigint) => { + if (currentRoundIndex === undefined) { + return; + } castCommit({ type: isGated ? DisputeKits.Gated : DisputeKits.Classic, disputeId: parsedDisputeID, choice, voteIds: parsedVoteIDs, roundIndex: currentRoundIndex, }); }, [castCommit, parsedDisputeID, currentRoundIndex, parsedVoteIDs, isGated] );web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
51-65: Guard commit when round index is unavailable.
currentRoundIndexcan be undefined before the dispute data loads, which makes the commit call invalid. Add a guard before invokingcastCommit.🐛 Suggested guard
const handleCommit = useCallback( async (choice: bigint) => { /* an extra 300 seconds (5 minutes) of decryptionDelay is enforced after Commit period is over to avoid premature decryption and voting attacks if no one passes the Commit period quickly */ const decryptionDelay = (countdownToVotingPeriod ?? 0) + 300; + if (currentRoundIndex === undefined) { + console.error("Round index not available"); + return; + } castCommit({ type: isGated ? DisputeKits.GatedShutter : DisputeKits.Shutter, disputeId: parsedDisputeID, choice, voteIds: parsedVoteIDs, roundIndex: currentRoundIndex, justification, decryptionDelay, }); }, [justification, parsedVoteIDs, parsedDisputeID, countdownToVotingPeriod, isGated, castCommit, currentRoundIndex] );
🧹 Nitpick comments (3)
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx (1)
33-42: Unsafe type assertion oncommit.Casting
commit as \0x${string}`bypasses TypeScript's type checking. Ifcommitisundefinedor an empty string fromuseVotingContext()`, this assertion masks the issue until runtime.Since
Reveal.tsxalready handlesisUndefined(commit)in rendering, consider passingcommitwith its actual type and letting the child component handle the typing:Safer approach
<Reveal {...{ arbitrable, setIsOpen, voteIDs, isRevealPeriod: !isCommitPeriod, isGated, - commit: commit as `0x${string}`, + commit, }} />Then update
IRevealinterface inReveal.tsxto acceptcommit?: \0x${string}`` and handle the undefined case explicitly.web/src/actions/helpers/storage/key.ts (1)
1-3: Make array-to-string conversion explicit for clarity.The
voteIdsarray is implicitly converted to a comma-separated string via template literal interpolation. While this works and produces consistent keys within a session (React Query caches the query results), the implicittoString()behavior is non-obvious to readers.Suggested improvement
export const getVoteKey = (disputeId: bigint, roundIndex: number, voteIds: bigint[]) => { - return `dispute-${disputeId}-round-${roundIndex}-voteids-${voteIds}`; + return `dispute-${disputeId}-round-${roundIndex}-voteids-${voteIds.join(",")}`; };web/src/actions/commit/builders/gatedShutter.builder.ts (1)
23-35: Normalizejustificationbefore hashing/encrypting.If
justificationis optional, passingundefinedintoencodeShutterMessage/hashJustificationcan produce inconsistent commits or runtime errors. Consider normalizing to an empty string and storing that.♻️ Suggested tweak
- const { disputeId, voteIds, choice, salt, decryptionDelay, justification, roundIndex } = params; + const { disputeId, voteIds, choice, salt, decryptionDelay, justification, roundIndex } = params; + const normalizedJustification = justification ?? ""; @@ - storeCommitData(key, { choice, salt, justification }); + storeCommitData(key, { choice, salt, justification: normalizedJustification }); @@ - const encodedMessage = encodeShutterMessage(choice, salt, justification); + const encodedMessage = encodeShutterMessage(choice, salt, normalizedJustification); @@ - const justificationCommit = hashJustification(BigInt(salt), justification); + const justificationCommit = hashJustification(BigInt(salt), normalizedJustification);
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@web/package.json`:
- Around line 28-30: The package.json currently pins an incorrect Vitest version
(a non-stable v4) referenced by the test scripts ("test", "test:watch",
"test:coverage") and the vitest dependency; update the vitest dependency to a
stable release (e.g., "^3.2.4") instead of any v4 beta, keep other test deps
(jsdom ^27.4.0) as-is, then run the package manager to update lockfiles
(npm/yarn/pnpm install) so the lockfile reflects the stable version.
In `@web/src/actions/commit/builders/shutter.builder.test.ts`:
- Around line 2-41: The test uses vi.stubEnv("REACT_APP_SHUTTER_API", ...) which
persists across suites; add an afterAll cleanup that calls vi.unstubAllEnvs() to
restore environment state after the tests in this file run (use the afterAll
hook in shutter.builder.test.ts alongside the existing mocks so vi.stubEnv is
undone); reference vi.stubEnv and call vi.unstubAllEnvs() inside afterAll to
prevent env leakage to other tests.
In `@web/src/actions/reveal/helpers/bruteForceChoice.test.ts`:
- Around line 85-107: Update the typo in the test case title: change the it
block whose description reads "iterates answers in the given orderh" to
"iterates answers in the given order" (the test around bruteForceChoice using
the answers array and createCommit should keep its logic unchanged; only rename
the test description string).
In `@web/src/actions/reveal/helpers/bruteForceChoice.ts`:
- Around line 11-23: The bruteForceChoice helper always calls hashVote without
the justification, so commits that included a justification cannot be recovered;
update bruteForceChoice to accept an optional justification parameter and pass
it through to hashVote when computing innerCommit, and then update callers (for
example resolveRevealInputs) to forward the justification (or undefined when
none) into bruteForceChoice so both justified and non-justified commits can be
matched.
In `@web/src/actions/reveal/resolveRevealInputs.test.ts`:
- Around line 325-355: The test fails because module-level vi.mock of
generateSalt returns undefined for this integration case; update the test to
restore the real implementation of generateSalt for this spec so BigInt(salt)
receives a real value: inside the "reconstructs choice and salt end-to-end"
test, replace the mocked generateSalt with the actual implementation (use
vi.importActual or retrieve the real generateSalt and set the mockGenerateSalt
to call-through) before calling generateSalt, then proceed to call
resolveRevealInputs and assert result.choice and result.salt as before.
In `@web/src/utils/crypto/generateSalt.test.ts`:
- Around line 37-51: The tests in generateSalt.test.ts use invalid hex
signatures (e.g., "0xconst123", "0xsig111") which cause viem's keccak256 to
throw; update the test vectors used in createMockAccount within the tests (both
the deterministic test using mockSignature and the differing-signature test
cases at the later lines) to use valid lowercase hex string literals (only 0-9
and a-f after 0x) of appropriate length so keccak256 accepts them; ensure all
other occurrences referenced (createMockAccount, mockAccount1, mockAccount2, and
any signatures around lines 79-81) are replaced consistently with valid hex
values.
🧹 Nitpick comments (9)
web/src/utils/crypto/hashVote.test.ts (1)
173-178: Consider adding a symmetric test for negative salt.Since
saltis also encoded asuint256, a negative salt should throwIntegerOutOfRangeErrorjust like negative choice. Adding this test would provide symmetric coverage.💡 Suggested addition
it("should throw on negative choice", () => { const choice = -1n; const salt = 123456789n; expect(() => hashVote(choice, salt)).toThrow(IntegerOutOfRangeError); }); + + it("should throw on negative salt", () => { + const choice = 1n; + const salt = -1n; + + expect(() => hashVote(choice, salt)).toThrow(IntegerOutOfRangeError); + }); }); });web/src/actions/commit/builders/classic.builder.test.ts (1)
88-104: PrefergetVoteKeyover a hardcoded key string.
This keeps the test aligned with the production key format if it changes.♻️ Proposed refactor
-import { storeCommitData } from "actions/helpers/storage"; +import { storeCommitData, getVoteKey } from "actions/helpers/storage"; @@ - expect(storeCommitData).toHaveBeenCalledWith("dispute-100-round-2-voteids-5,6", { + const expectedKey = getVoteKey(params.disputeId, params.roundIndex, params.voteIds); + expect(storeCommitData).toHaveBeenCalledWith(expectedKey, { choice: 1n, salt: 777n, });web/src/actions/vote/builders/classic.builder.test.ts (2)
13-18: Mock address is malformed.The mock address
0xVOTEADDRESS12345678901234567890123456is invalid:
- Contains non-hex characters (
O,T,D).- Only 37 characters after
0xinstead of the required 40.While this works because of the type assertion, using a valid address improves test robustness and avoids potential issues with stricter validation.
Suggested fix
vi.mock("hooks/contracts/generated", () => ({ disputeKitClassicAbi: [{ name: "castVote", type: "function" }], disputeKitClassicAddress: { - 421614: "0xVOTEADDRESS12345678901234567890123456" as const, + 421614: "0x1234567890123456789012345678901234567890" as const, }, }));
206-226: Test name doesn't match behavior.The test is named "comparison with reveal builder" but doesn't actually import or compare with the reveal builder's output. It only verifies the structure of the vote builder independently.
Consider either:
- Renaming the describe block to better reflect what it tests (e.g., "args structure validation")
- Actually importing and comparing with the reveal builder if cross-builder consistency is the intent
web/src/actions/commit/builders/gated.builder.test.ts (2)
18-23: Use a valid Ethereum address format in mock.The mock address
0xGATED1234567890123456789012345678901234is not a valid Ethereum address:
- Contains 'G' which is not valid hexadecimal (only 0-9 and a-f are valid)
- Has 39 hex characters instead of the required 40
While this won't break the current tests, it could cause issues if viem performs address validation, and it makes the tests less realistic.
Suggested fix
vi.mock("hooks/contracts/generated", () => ({ disputeKitGatedAbi: [{ name: "castCommit", type: "function" }], disputeKitGatedAddress: { - 421614: "0xGATED1234567890123456789012345678901234" as const, + 421614: "0x1234567890abcdef1234567890abcdef12345678" as const, }, }));Then update the expectation on line 56:
- address: "0xGATED1234567890123456789012345678901234", + address: "0x1234567890abcdef1234567890abcdef12345678",
77-95: Consider mockinggetVoteKeyfor better test isolation.This test hardcodes the expected key format
"dispute-100-round-2-voteids-5,6", creating an implicit dependency ongetVoteKey's implementation. IfgetVoteKeychanges its format, this test will fail even thoughgatedCommitBuilderitself remains correct.Options:
- Mock
getVoteKeyto return a predictable value (better isolation)- Import and use
getVoteKeyto compute the expected key (more maintainable)- Keep as-is if intentional integration testing (document the intent)
Option 2: Use getVoteKey for expected value
+import { getVoteKey } from "actions/helpers/storage/key"; + // In the test: + const expectedKey = getVoteKey(params.disputeId, params.roundIndex, params.voteIds); expect(storeCommitData).toHaveBeenCalledTimes(1); - expect(storeCommitData).toHaveBeenCalledWith("dispute-100-round-2-voteids-5,6", { + expect(storeCommitData).toHaveBeenCalledWith(expectedKey, { choice: 1n, salt: 777n, });web/src/actions/helpers/storage/key.test.ts (1)
5-65: Good test coverage, consider adding edge case for empty voteIds.The test suite comprehensively covers key formatting, large values, determinism, and uniqueness. Consider adding a test for an empty
voteIdsarray to document the expected behavior in that edge case.Optional: Add edge case test
+ it("should handle empty voteIds array", () => { + const result = getVoteKey(1n, 0, []); + + expect(result).toBe("dispute-1-round-0-voteids-"); + });web/src/test/fakes/shutter.ts (1)
18-27: Deterministic test fake is well-designed.The fake produces verifiable, deterministic outputs using keccak256 hashing. The TODO comment appropriately notes the need to validate against the real API.
Consider extracting the shared hash computation to reduce duplication between
fakeEncryptandverifyFakeEncryptOutput.Optional: Extract shared computation
+function computeShutterOutputs(message: string, decryptionDelay: number): ShutterEncryptResult { + const identity = keccak256(encodePacked(["string", "uint256"], [message, BigInt(decryptionDelay)])); + const encryptedCommitment = keccak256(encodePacked(["bytes32", "string"], [identity, "encrypted"])); + return { encryptedCommitment, identity }; +} + export function fakeEncrypt(message: string, decryptionDelay: number): ShutterEncryptResult { - const identity = keccak256(encodePacked(["string", "uint256"], [message, BigInt(decryptionDelay)])); - const encryptedCommitment = keccak256(encodePacked(["bytes32", "string"], [identity, "encrypted"])); - - return { - encryptedCommitment, - identity, - }; + return computeShutterOutputs(message, decryptionDelay); } export function verifyFakeEncryptOutput( message: string, decryptionDelay: number, actualIdentity: `0x${string}`, actualEncrypted: `0x${string}` ): boolean { - const expectedIdentity = keccak256(encodePacked(["string", "uint256"], [message, BigInt(decryptionDelay)])); - const expectedEncrypted = keccak256(encodePacked(["bytes32", "string"], [expectedIdentity, "encrypted"])); - - return actualIdentity === expectedIdentity && actualEncrypted === expectedEncrypted; + const expected = computeShutterOutputs(message, decryptionDelay); + return actualIdentity === expected.identity && actualEncrypted === expected.encryptedCommitment; }Also applies to: 33-43
web/src/actions/commit/builders/shutter.builder.test.ts (1)
130-135: UsegetVoteKeyto keep the test aligned with key formatting.This avoids duplicating the string format and protects the test if key formatting changes.
♻️ Suggested update
-import { storeCommitData } from "actions/helpers/storage"; +import { storeCommitData, getVoteKey } from "actions/helpers/storage"; @@ - expect(storeCommitData).toHaveBeenCalledWith("dispute-100-round-2-voteids-5,6", { + const expectedKey = getVoteKey(params.disputeId, params.roundIndex, params.voteIds); + expect(storeCommitData).toHaveBeenCalledWith(expectedKey, { choice: 1n, salt: 777n, justification: "Stored justification", });
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/src/actions/helpers/storage/getVoteKey.ts`:
- Around line 1-7: getVoteKey currently builds a storage key without chain
context which allows identical disputeId/roundIndex/voteIds to collide across
chains; update the function signature getVoteKey to accept a chainId parameter
and include it in the returned key (e.g., incorporate chainId into the template
string) and then update all call sites that use getVoteKey to pass the
appropriate chainId from their context (commit/reveal flows that already have
chain available) so stored commit data is namespaced per chain.
In `@web/src/hooks/useCastCommit.tsx`:
- Around line 43-52: The code always generates a new salt and signing account,
overwriting caller-provided params.salt; change the flow in useCastCommit so it
respects params.salt: if params.salt is present, skip generating a signing
account and skip generateSalt; otherwise create signingAccount via
generateSigningAccount(), throw only if you need to sign/generate salt, then
call generateSalt(signingAccount, getVoteKey(...)) and set executeParams.salt =
BigInt(generatedSalt); when using the caller salt set executeParams.salt =
BigInt(params.salt). Keep references to getVoteKey, generateSigningAccount,
generateSalt, CommitParams and executeCommit when updating the logic.
🧹 Nitpick comments (3)
web/src/actions/reveal/resolveRevealInputs.test.ts (1)
49-51: Prefervi.resetAllMocks()overvi.clearAllMocks()for test isolation.
vi.clearAllMocks()only clears call history while keeping the mock implementation intact, creating risk of test pollution if a future test forgets to reconfigure a mock.vi.resetAllMocks()both clears call history and resets implementations to their default state, ensuring clean isolation between tests.♻️ Proposed tweak
- beforeEach(() => { - vi.clearAllMocks(); - }); + beforeEach(() => { + vi.resetAllMocks(); + });web/src/test/fakes/shutter.ts (1)
1-1: Resolve or track the TODO for fake verification.
Please add the verification test or create a tracking issue so the fake doesn’t drift from the real API.web/src/actions/commit/builders/shutter.builder.ts (1)
36-37: RedundantBigInt()wrapping on already-bigint values.The
saltparameter is already typed asbigintinShutterCommitParams(which extendsBaseCommitParams). TheBigInt()calls are unnecessary and inconsistent with other builders likegated.builder.tswhich usehashVote(choice, salt)directly.♻️ Suggested fix
- const choiceCommit = hashVote(choice, BigInt(salt)); - const justificationCommit = hashJustification(BigInt(salt), justification); + const choiceCommit = hashVote(choice, salt); + const justificationCommit = hashJustification(salt, justification);
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx`:
- Around line 64-90: The revealVote mutation in handleReveal must be awaited so
the function doesn't return before the mutation completes; update handleReveal
to await the revealVote(...) call (same pattern used in Commit.tsx) so
OptionsContainer and related flows correctly reflect loading state — reference
the handleReveal function and the revealVote mutateAsync call (keep commit,
disputeDetails?.answers, parsedVoteIDs, parsedDisputeID and isGated usage
unchanged).
In `@web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx`:
- Around line 50-68: In handleReveal, the call to revealVote should be awaited;
update the async callback (handleReveal) to await revealVote({ params: {
disputeId: parsedDisputeID, voteIds: parsedVoteIDs, roundIndex:
Number(currentRoundIndex), justification, type: isGated ?
DisputeKits.GatedShutter : DisputeKits.Shutter }, context: { commit, answers:
disputeDetails?.answers } }) and return/propagate its result so the mutation
completes before the function returns, keeping the same dependency list
(parsedVoteIDs, justification, currentRoundIndex, revealVote, disputeDetails,
commit, isGated, parsedDisputeID).
In `@web/src/tests/e2e/fixtures/atlas.ts`:
- Around line 12-15: The GraphQL route handler using page.route async (route,
request) currently calls request.postDataJSON() directly which can throw on
empty/invalid bodies and crash tests; wrap the postDataJSON() call in a
try-catch inside the route handler (the function that reads const body =
request.postDataJSON() and const operation = body?.operationName) and if parsing
fails safely handle the request (e.g., route.continue() for non-JSON/preflight
or route.fulfill() with a 400/empty JSON response) instead of letting the
exception propagate; ensure the handler logs or no-ops on the parse error and
returns early so tests remain stable.
🧹 Nitpick comments (4)
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
103-109: Consider disabling button whencurrentRoundIndexis unavailable.For better UX, disable the reveal button until dispute data has loaded and
currentRoundIndexis available. This prevents users from clicking when the action would silently bail out.Suggested improvement
<StyledButton variant="secondary" text={t("buttons.justify_and_reveal")} - disabled={isPending || isUndefined(disputeDetails)} + disabled={isPending || isUndefined(disputeDetails) || isUndefined(currentRoundIndex)} isLoading={isPending} onClick={handleReveal} />web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (2)
39-39: UnusedsetJustificationsetter.
setJustificationis defined but never called. The TODO on line 46 suggests this is intentional for future functionality, but currentlyjustificationwill always be an empty string. Consider removing the unused state or implementing the TODO.
70-74: Consider disabling button whencurrentRoundIndexis unavailable.For better UX and consistency with the guard in
handleReveal, disable the button untilcurrentRoundIndexis available.Suggested improvement
- <Button text={t("buttons.reveal_your_vote")} onClick={handleReveal} disabled={isPending} isLoading={isPending} /> + <Button + text={t("buttons.reveal_your_vote")} + onClick={handleReveal} + disabled={isPending || isUndefined(currentRoundIndex)} + isLoading={isPending} + />web/src/tests/e2e/fixtures/atlas.ts (1)
100-105: Fail fast on unhandled GraphQL operations.Returning
{ data: {} }for unknown operations can hide missing mocks and let tests pass when they should fail. Consider returning a clear error with the operation name.🧭 Suggested change
- // Default fallback - return route.fulfill({ - status: 200, - contentType: "application/json", - body: JSON.stringify({ data: {} }), - }); + // Default fallback (fail fast on unhandled ops) + return route.fulfill({ + status: 400, + contentType: "application/json", + body: JSON.stringify({ + errors: [{ message: `Unhandled GraphQL operation: ${operation ?? "unknown"}` }], + }), + });
fcanela
left a comment
fcanela
left a comment
There was a problem hiding this comment.
LGTM. I replied the existing comments and noted a few remarks, but the PR is ready to be merged already.
|
|
❌ The last analysis has failed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/src/config/rpc.ts`:
- Around line 17-20: The current import-time throw using alchemyApiKey causes
local/test boot to fail; instead detect presence with a boolean (e.g.,
hasAlchemyKey from alchemyApiKey) and only throw when in production (use
isProductionDeployment() or similar) while wiring transports so
maybeAlchemyTransport(chain) uses alchemyTransport(chain) when hasAlchemyKey is
true and falls back to defaultTransport(chain) when false; update any references
to alchemyApiKey usage to use hasAlchemyKey and ensure the production-only guard
throws (e.g., if isProductionDeployment() && !hasAlchemyKey) rather than
throwing at module import.
- Around line 43-46: The defaultTransport function currently always calls
webSocket(...) which breaks when chain.rpcUrls.default?.webSocket?.[0] is
undefined; change defaultTransport to only include webSocket when a URL exists
(or call webSocket() with no arg if wagmi supports that) so fallback([...])
receives a valid transport list; update the defaultTransport definition (and
keep alchemyTransport unchanged) to conditionally construct the array using the
presence of chain.rpcUrls.default?.webSocket?.[0] and pass that array into
fallback.
4 participants
PR-Codex overview
This PR focuses on enhancing the functionality of the voting and committing systems in a blockchain application, introducing various new features and improving existing code for better performance and maintainability.
Detailed summary
VoteContext,CommitContext, andRevealContextinterfaces invote,commit, andrevealactions.Gated,Classic, andShutterdispute kits..idacross the application.Summary by CodeRabbit
New Features
Refactor
Tests
Chores / Docs