Release #2076
Release #2076
Conversation
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThis PR refactors the arbitration stack to v2.0.0: replaces governor with owner across contracts, introduces IRNG and RNGWithFallback, extracts sortition logic into SortitionTrees, consolidates SortitionModule APIs, overhauls KlerosCore (including dispute/court models, events, flows), updates dispute kits (classic/gated/shutter/sybil), adjusts gateways/evidence/template registries, redeploys artifacts, revises scripts/tests/configs, and removes Neo variants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Arbitrable
participant Core as KlerosCore
participant DK as DisputeKit (Classic/Gated/Shutter)
participant SM as SortitionModule
participant RNG as RNGWithFallback
participant PRNG as Primary RNG (Chainlink/Randomizer)
participant BRNG as BlockhashRNG
User->>Arbitrable: createDispute(...)
Arbitrable->>Core: createDispute(..., feeToken/amount)
Core->>SM: createDisputeHook(disputeID, roundID)
Core->>DK: createDispute(coreDisputeID, coreRoundID, ...)
Core->>SM: passPhase() to Generating
SM->>RNG: requestRandomness()
Note right of RNG: Records requestTimestamp
Core->>SM: passPhase() to Drawing
SM->>RNG: receiveRandomness()
alt Primary RNG ready
RNG->>PRNG: receiveRandomness()
PRNG-->>RNG: randomNumber
RNG-->>SM: randomNumber
else Timeout elapsed
RNG-->>SM: 0 (not ready)
RNG->>BRNG: fallback (previous blockhash)
RNG-->>SM: fallbackRandomNumber
RNG-->>Core: RNGFallback event
end
loop draw nbVotes
SM->>SM: draw(courtID, disputeID, nonce)
SM-->>Core: drawn juror + fromSubcourtID
Core->>DK: draw(..., roundNbVotes) -> (juror, fromSubcourtID)
end
Core-->>User: NewPeriod(Vote/Commit)
sequenceDiagram
autonumber
actor FunderA
actor FunderB
participant Core as KlerosCore
participant DK as DisputeKit
participant SM as SortitionModule
FunderA->>Core: fundAppeal(disputeID, choice, amount)
Core->>DK: fundAppeal(...)
DK-->>Core: track funded choices
FunderB->>Core: fundAppeal(disputeID, otherChoice, amount)
DK-->>Core: both sides funded?
alt Both sides funded
Core-->>Core: Court/DisputeKit compatibility check
Core->>DK: getNextRoundSettings(...)
DK-->>Core: (newCourtID, newDisputeKitID, newRoundNbVotes)
Core-->>Core: emit CourtJump/DisputeKitJump
Core->>SM: postDrawHook(disputeID, newRoundID)
Core-->>FunderA: AppealDecision/NewPeriod
else Partial funding
Core-->>FunderA: AppealPossible
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo-devtools failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
contracts/hardhat.config.ts (1)
29-35: Verify compile time and bytecode size with viaIR and 10000 runsThe size‐contracts check failed (missing node_modules), and the compile step reported only 1.26 s (likely no real work). Please run the following under
contracts/and share the results to ensure there are no regressions:
- Install dependencies:
yarn install- Measure bytecode sizes without recompiling:
yarn hardhat size-contracts --no-compile- Clean compile and time it:
rm -rf cache artifacts TIMEFORMAT='compile took %R sec' time yarn hardhat compileIf you observe significant increases in compile times or bytecode sizes (EIP-170 limit concerns), consider lowering optimizer runs (e.g., 200–2000).
contracts/package.json (1)
160-161: Question: is isomorphic-fetch needed on Node 20?Node 20 has stable global fetch. Adding isomorphic-fetch may be redundant and can introduce polyfill conflicts in some runtimes.
If not strictly needed for browser bundles, consider removing:
"dependencies": { "@chainlink/contracts": "^1.4.0", "@kleros/vea-contracts": "^0.7.0", "@openzeppelin/contracts": "^5.4.0", "@shutter-network/shutter-sdk": "0.0.2", - "isomorphic-fetch": "^3.0.0" },If needed in specific scripts, prefer local import over global polyfill.
contracts/CHANGELOG.md (1)
7-15: Mark unreleased changes as “Unreleased” to avoid confusion with published versions.Labeling 0.13.0 as “Not published yet” while giving it a version can confuse consumers and tools.
Consider:
-## [0.13.0] - 2025-08-07 (Not published yet) +## [Unreleased]Move these entries under Unreleased until the tag is published and the package version is bumped.
web/src/public/llms.txt (1)
6-8: Improve sentence structure to avoid repetitive beginnings.The descriptions are informative, but three consecutive sentences start with "Kleros" which affects readability.
Consider rewording for better flow:
- [Kleros Dispute Cases](https://v2.kleros.builders/#/cases/display/1/desc/all): Provide a platform for viewing and managing decentralized dispute resolution cases. - [Kleros Decentralized Courts](https://v2.kleros.builders/#/courts): Facilitate decentralized dispute resolution by allowing users to stake tokens, participate as jurors, and view court cases. - [Kleros Jurors Leaderboard](https://v2.kleros.builders/#/jurors/1/desc/all): Display ranking and statistics of jurors based on coherent voting and rewards in the Kleros decentralized arbitration system. + [Kleros Dispute Cases](https://v2.kleros.builders/#/cases/display/1/desc/all): Comprehensive platform for viewing and managing decentralized dispute resolution cases. + [Decentralized Courts](https://v2.kleros.builders/#/courts): Enable decentralized dispute resolution by allowing users to stake tokens, participate as jurors, and view court cases. + [Jurors Leaderboard](https://v2.kleros.builders/#/jurors/1/desc/all): Rankings and statistics of jurors based on coherent voting and rewards in the decentralized arbitration system.
📜 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)
contracts/CHANGELOG.md(2 hunks)contracts/deployments/disputeKitsViem.ts(1 hunks)contracts/deployments/index.ts(1 hunks)contracts/foundry.toml(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(4 hunks)contracts/scripts/getDisputeKits.ts(1 hunks)package.json(1 hunks)web/netlify.toml(1 hunks)web/src/public/llms.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-05-23T17:47:39.947Z
Learnt from: kemuru
PR: kleros/kleros-v2#1994
File: web/vite.config.js:26-33
Timestamp: 2025-05-23T17:47:39.947Z
Learning: In Yarn workspace setups, dependencies defined in individual workspace package.json files (like web/package.json) are typically hoisted to the root node_modules directory. This means vite config paths should point to "../node_modules" from workspace directories to access hoisted dependencies, not to local workspace node_modules.
Applied to files:
package.json
📚 Learning: 2024-11-21T23:16:14.816Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1757
File: prettier-config/package.json:7-7
Timestamp: 2024-11-21T23:16:14.816Z
Learning: As of November 2024, ESLint v9.15.0 has been released and is acceptable to use in the project.
Applied to files:
contracts/hardhat.config.ts
📚 Learning: 2024-11-21T23:20:28.163Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1757
File: web-devtools/package.json:36-40
Timestamp: 2024-11-21T23:20:28.163Z
Learning: TypeScript-ESLint v8.15.0 is compatible with both ESLint v8 and v9.
Applied to files:
contracts/hardhat.config.ts
📚 Learning: 2024-10-14T13:58:25.708Z
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.
Applied to files:
contracts/scripts/getDisputeKits.tscontracts/deployments/disputeKitsViem.ts
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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/scripts/getDisputeKits.ts
📚 Learning: 2024-10-24T08:16:02.749Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as `kleros-sdk/src/requests/gqlClient.ts`) exports only a single entity, it's acceptable to use default exports instead of named exports.
Applied to files:
contracts/scripts/getDisputeKits.tscontracts/deployments/index.tscontracts/deployments/disputeKitsViem.ts
📚 Learning: 2024-10-22T09:38:20.093Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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:
contracts/deployments/index.tscontracts/deployments/disputeKitsViem.ts
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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:
contracts/deployments/index.tscontracts/deployments/disputeKitsViem.ts
📚 Learning: 2025-01-23T08:14:47.397Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1846
File: prettier-config/index.js:26-26
Timestamp: 2025-01-23T08:14:47.397Z
Learning: The prettier-plugin-solidity plugin is installed in the kleros-v2 repository, even though it's not visible in the sandbox environment's node_modules (which is expected as node_modules is not committed to the repository).
Applied to files:
contracts/package.json
📚 Learning: 2025-05-15T06:50:45.650Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:0-0
Timestamp: 2025-05-15T06:50:45.650Z
Learning: In the Kleros V2 codebase, it's acceptable to use ESLint disable comments for dependency arrays in useEffect hooks when including certain dependencies (like state that is being updated within the effect) would cause infinite loops.
Applied to files:
contracts/package.json
📚 Learning: 2025-05-23T17:47:39.947Z
Learnt from: kemuru
PR: kleros/kleros-v2#1994
File: web/vite.config.js:26-33
Timestamp: 2025-05-23T17:47:39.947Z
Learning: The viteStaticCopy plugin configuration in web/vite.config.js correctly copies Shutter SDK files from the installed node_modules location. The path resolve(__dirname, "../node_modules/shutter-network/shutter-sdk/dist/*") works when dependencies are properly installed, despite initial analysis suggesting otherwise due to sandbox environment limitations.
Applied to files:
contracts/package.json
📚 Learning: 2024-10-22T10:23:15.789Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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.
Applied to files:
contracts/deployments/disputeKitsViem.ts
🪛 LanguageTool
web/src/public/llms.txt
[style] ~7-~7: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...entralized dispute resolution cases. - [Kleros Decentralized Courts](https://v2.kleros...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ate as jurors, and view court cases. - [Kleros Jurors Leaderboard](https://v2.kleros.b...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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). (11)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- 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
🔇 Additional comments (11)
contracts/package.json (3)
3-3: Version bump looks good.0.12.0 aligns with the new peer requirement and helper additions.
157-157: OK to bump @kleros/vea-contracts to ^0.7.0.Matches the Hardhat/Foundry upgrades and related changes.
124-124: Verify Hardhat Plugin Compatibility with v2.26.2
Ensure all Hardhat plugins in contracts/package.json (lines 135–142) are compatible with Hardhat v2.26.2:
- hardhat-contract-sizer (l.135)
- hardhat-deploy (l.136)
- hardhat-deploy-ethers (l.137)
- hardhat-deploy-tenderly (l.138)
- hardhat-docgen (l.139)
- hardhat-gas-reporter (l.140)
- hardhat-tracer (l.141)
- hardhat-watcher (l.142)
If your CI build has passed, you can safely ignore this check.
contracts/foundry.toml (2)
3-7: LGTM! Solidity 0.8.30 upgrade with IR pipeline is well-configured.The compiler upgrade to 0.8.30 with IR compilation enabled aligns with the coordinated changes across build tools. The optimizer settings with 500 runs and Yul stack allocation are appropriate for production builds.
8-13: Good approach to handle IR compilation compatibility issues.The test profile and compilation restrictions provide flexibility for files that may have issues with IR compilation. This is a pragmatic solution for the transition period.
contracts/deployments/index.ts (1)
21-22: LGTM! Clean integration of dispute kits functionality.The export structure follows established patterns and provides a clear API for the new Viem-based dispute kits functionality. The type exports and function aliasing are consistent with the module's existing approach.
contracts/scripts/getDisputeKits.ts (3)
5-13: LGTM! Good environment variable validation and client setup.The environment variable check and Viem client configuration are properly implemented with appropriate error handling.
26-33: LGTM! Clean script execution pattern.The main function execution pattern with proper process exit handling follows Node.js best practices.
15-24: No action needed: “devnet” deployment exists for Arbitrum Sepolia
Verified thatcontracts/deployments/arbitrumSepoliaDevnet.tsdefines the “devnet” deployment on Arbitrum Sepolia, matching the RPC endpoint used ingetDisputeKits(client, "devnet").contracts/deployments/disputeKitsViem.ts (2)
5-18: LGTM! Well-structured type definitions.The type definitions properly model dispute kit contracts and their metadata. The union type for
DisputeKitcorrectly handles the nullable case, and the exported types provide a clean API surface.
66-85: LGTM! Robust contract matching logic.The address comparison with case normalization and the use of
satisfiesfor type checking demonstrate good defensive programming practices. The logic correctly identifies gated and shutter variants.
| [0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 | ||
| [0.12.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.12.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid adding a release link for an unpublished tag.
If 0.13.0 isn’t published, this link will 404. Add it only when the release is live.
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0
+<!-- [0.13.0]: (add after publishing) -->Optionally confirm release existence before merging:
🏁 Script executed:
#!/bin/bash
# Prints 200 if the tag exists, otherwise likely 404
curl -s -o /dev/null -w "%{http_code}\n" \
"https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0"Length of output: 132
Remove unpublished release link for v0.13.0 in CHANGELOG.md
The 0.13.0 release tag currently returns 404, so we should not include its link until the release is live.
• File: contracts/CHANGELOG.md
Lines: 129–130
Recommended change:
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0
+<!-- [0.13.0]: (add after publishing) -->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 | |
| [0.12.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.12.0 | |
| <!-- [0.13.0]: (add after publishing) --> | |
| [0.12.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.12.0 |
🤖 Prompt for AI Agents
In contracts/CHANGELOG.md at lines 129 to 130, the release link for version
0.13.0 leads to a 404 error because the release is not yet published. Remove or
comment out the line containing the 0.13.0 release link to prevent broken links
in the changelog until the release is officially live.
| const fetchDisputeKits = async (client: PublicClient, klerosCoreAddress: `0x${string}`, klerosCoreAbi: Abi) => { | ||
| const DisputeKitCreated = getAbiItem({ | ||
| abi: klerosCoreAbi, | ||
| name: "DisputeKitCreated", | ||
| }) as AbiEvent; | ||
| const logs = await client.getLogs({ | ||
| address: klerosCoreAddress, | ||
| event: DisputeKitCreated, | ||
| fromBlock: 0n, | ||
| toBlock: "latest", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance implications of unbounded block range.
Fetching events from block 0 to latest will become increasingly expensive as the chain grows. Consider adding block range parameters or caching mechanisms for production use.
-const fetchDisputeKits = async (client: PublicClient, klerosCoreAddress: `0x${string}`, klerosCoreAbi: Abi) => {
+const fetchDisputeKits = async (
+ client: PublicClient,
+ klerosCoreAddress: `0x${string}`,
+ klerosCoreAbi: Abi,
+ fromBlock: bigint = 0n
+) => {
const DisputeKitCreated = getAbiItem({
abi: klerosCoreAbi,
name: "DisputeKitCreated",
}) as AbiEvent;
const logs = await client.getLogs({
address: klerosCoreAddress,
event: DisputeKitCreated,
- fromBlock: 0n,
+ fromBlock,
toBlock: "latest",
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchDisputeKits = async (client: PublicClient, klerosCoreAddress: `0x${string}`, klerosCoreAbi: Abi) => { | |
| const DisputeKitCreated = getAbiItem({ | |
| abi: klerosCoreAbi, | |
| name: "DisputeKitCreated", | |
| }) as AbiEvent; | |
| const logs = await client.getLogs({ | |
| address: klerosCoreAddress, | |
| event: DisputeKitCreated, | |
| fromBlock: 0n, | |
| toBlock: "latest", | |
| }); | |
| const fetchDisputeKits = async ( | |
| client: PublicClient, | |
| klerosCoreAddress: `0x${string}`, | |
| klerosCoreAbi: Abi, | |
| fromBlock: bigint = 0n | |
| ) => { | |
| const DisputeKitCreated = getAbiItem({ | |
| abi: klerosCoreAbi, | |
| name: "DisputeKitCreated", | |
| }) as AbiEvent; | |
| const logs = await client.getLogs({ | |
| address: klerosCoreAddress, | |
| event: DisputeKitCreated, | |
| fromBlock, | |
| toBlock: "latest", | |
| }); | |
| // ...rest of the implementation | |
| }; |
🤖 Prompt for AI Agents
In contracts/deployments/disputeKitsViem.ts around lines 20 to 30, the event log
fetching uses an unbounded block range from 0 to latest, which can cause
performance issues as the blockchain grows. Modify the function to accept
optional fromBlock and toBlock parameters to limit the block range for fetching
logs, or implement caching to avoid repeated full-range queries. Update the
client.getLogs call to use these parameters instead of fixed values.
| const DisputeKitCreated = getAbiItem({ | ||
| abi: klerosCoreAbi, | ||
| name: "DisputeKitCreated", | ||
| }) as AbiEvent; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe type assertion - add runtime validation.
The type assertion as AbiEvent on line 24 could fail if the ABI doesn't contain the expected event structure.
- const DisputeKitCreated = getAbiItem({
- abi: klerosCoreAbi,
- name: "DisputeKitCreated",
- }) as AbiEvent;
+ const DisputeKitCreated = getAbiItem({
+ abi: klerosCoreAbi,
+ name: "DisputeKitCreated",
+ });
+
+ if (!DisputeKitCreated || DisputeKitCreated.type !== 'event') {
+ throw new Error('DisputeKitCreated event not found in ABI');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DisputeKitCreated = getAbiItem({ | |
| abi: klerosCoreAbi, | |
| name: "DisputeKitCreated", | |
| }) as AbiEvent; | |
| const DisputeKitCreated = getAbiItem({ | |
| abi: klerosCoreAbi, | |
| name: "DisputeKitCreated", | |
| }); | |
| if (!DisputeKitCreated || DisputeKitCreated.type !== 'event') { | |
| throw new Error('DisputeKitCreated event not found in ABI'); | |
| } |
🤖 Prompt for AI Agents
In contracts/deployments/disputeKitsViem.ts around lines 21 to 24, the code uses
a direct type assertion 'as AbiEvent' which can be unsafe if the ABI item does
not match the expected event structure. To fix this, add runtime validation to
check that the returned object from getAbiItem has the required properties of an
AbiEvent before asserting its type. If the validation fails, handle the error
appropriately instead of blindly asserting the type.
| return Object.fromEntries( | ||
| logs | ||
| .filter((log) => { | ||
| const args = log.args as Record<string, unknown>; | ||
| return "_disputeKitID" in args && "_disputeKitAddress" in args; | ||
| }) | ||
| .map((log) => { | ||
| const { _disputeKitID, _disputeKitAddress } = log.args as { | ||
| _disputeKitID: bigint; | ||
| _disputeKitAddress: string; | ||
| }; | ||
| return { | ||
| disputeKitID: _disputeKitID, | ||
| disputeKitAddress: _disputeKitAddress, | ||
| }; | ||
| }) | ||
| .map(({ disputeKitID, disputeKitAddress }) => [disputeKitID!.toString(), disputeKitAddress as `0x${string}`]) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for blockchain interactions.
The function lacks error handling for potential RPC failures or malformed event data.
Wrap the blockchain interaction in try-catch:
+ try {
const logs = await client.getLogs({
address: klerosCoreAddress,
event: DisputeKitCreated,
fromBlock: 0n,
toBlock: "latest",
});
+ } catch (error) {
+ throw new Error(`Failed to fetch DisputeKitCreated events: ${error}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/deployments/disputeKitsViem.ts around lines 31 to 49, the code
processes blockchain event logs without any error handling, which risks
unhandled exceptions from RPC failures or malformed data. Wrap the entire log
processing logic inside a try-catch block to catch and handle any errors
gracefully. In the catch block, log or rethrow the error with a clear message to
aid debugging and prevent the application from crashing unexpectedly.
contracts/package.json
Outdated
| "peerDependencies": { | ||
| "viem": "^2.24.1" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "viem": { | ||
| "optional": false | ||
| } | ||
| } |
There was a problem hiding this comment.
Peer dependency viem ^2.24.1 conflicts with root resolution (^2.23.2).
This will produce peer warnings/errors and potentially divergent behavior across workspaces.
Either relax the peer to match the resolution, or better, update the root resolution to satisfy the peer (recommended):
Preferred (root package.json):
- "viem@npm:2.x": "npm:^2.23.2"
+ "viem@npm:2.x": "npm:^2.24.1"If you must stay on 2.23.x, change this package’s peer to:
- "viem": "^2.24.1"
+ "viem": "^2.23.0"…but this may block newer features you rely on.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "peerDependencies": { | |
| "viem": "^2.24.1" | |
| }, | |
| "peerDependenciesMeta": { | |
| "viem": { | |
| "optional": false | |
| } | |
| } | |
| "peerDependencies": { | |
| - "viem": "^2.24.1" | |
| + "viem": "^2.23.0" | |
| }, | |
| "peerDependenciesMeta": { | |
| "viem": { | |
| "optional": false | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/package.json around lines 162 to 169, the peer dependency version
for "viem" (^2.24.1) conflicts with the root workspace resolution (^2.23.2),
causing warnings and potential inconsistent behavior. To fix this, update the
root package.json to use "viem" version ^2.24.1 to match the peer dependency, or
alternatively, relax the peer dependency in this package.json to ^2.23.2 if you
must stay on that version, understanding this may limit access to newer
features.
| "undici@npm:7.3.0": "npm:7.5.0", | ||
| "viem@npm:2.x": "npm:^2.23.2" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align viem resolution with contracts’ peer dependency to avoid peer conflicts.
Root resolution pins viem 2.x to ^2.23.2, while contracts require viem ^2.24.1 as a peer. This will trigger peer resolution warnings/errors in Yarn 4 and could break consumers.
Update the resolution to satisfy the peer:
"resolutions": {
...
- "viem@npm:2.x": "npm:^2.23.2"
+ "viem@npm:2.x": "npm:^2.24.1"
},To check for other mismatches:
🏁 Script executed:
#!/bin/bash
rg -n --json '"viem"\s*:\s*"' | jq -r '.data.path.text + ":" + .data.line_number|tostring + " " + .data.lines.text'Length of output: 1417
🏁 Script executed:
#!/bin/bash
# Search for all references to “viem” in any JSON file to spot mismatched pins
rg -n '"viem"' -g '*.json'Length of output: 440
Update viem resolution to match peerDependency (^2.24.1)
All of your sub-packages (contracts, web, kleros-sdk, web-devtools, kleros-app) declare viem at ^2.24.1, but the root resolutions entry still pins viem@npm:2.x to ^2.23.2. This mismatch will trigger peer warnings/errors in Yarn 4 and may break consumers.
• File: package.json (root)
• Change the resolutions.viem@npm:2.x entry
"resolutions": {
…
- "viem@npm:2.x": "npm:^2.23.2"
+ "viem@npm:2.x": "npm:^2.24.1"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "undici@npm:7.3.0": "npm:7.5.0", | |
| "viem@npm:2.x": "npm:^2.23.2" | |
| "resolutions": { | |
| "undici@npm:7.3.0": "npm:7.5.0", | |
| "viem@npm:2.x": "npm:^2.24.1" | |
| }, |
🤖 Prompt for AI Agents
In package.json around lines 80 to 81, the resolutions entry for viem is set to
^2.23.2, which conflicts with the peerDependency version ^2.24.1 declared in
sub-packages. Update the resolutions.viem@npm:2.x entry to ^2.24.1 to align with
the peerDependency version and prevent peer warnings or errors in Yarn 4.
| [[headers]] | ||
| for = "/*" | ||
| [headers.values] | ||
| X-Robots-Tag = "llms-txt" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Header value likely incorrect; consider advertising llms.txt via Link header instead of X-Robots-Tag.
X-Robots-Tag expects crawler directives (e.g., noindex, noai). Using "llms-txt" here is non-standard and probably ignored. To advertise a policy file, prefer a Link header (and ensure the file is served at the expected path).
Apply one of these:
Option A — advertise a policy file:
[[headers]]
for = "/*"
[headers.values]
- X-Robots-Tag = "llms-txt"
+ Link = "</llms.txt>; rel=\"ai-policy\""Option B — if you meant to control AI crawlers:
[[headers]]
for = "/*"
[headers.values]
- X-Robots-Tag = "llms-txt"
+ X-Robots-Tag = "noai, noimageai"Also verify the file is published at the root (e.g., web/public/llms.txt → https://site/llms.txt), not under src.
Run to locate the file and confirm its publish path:
🏁 Script executed:
#!/bin/bash
fd -HI --glob "llms.txt"Length of output: 48
Misuse of X-Robots-Tag; advertise AI policy via Link header
- The current
X-Robots-Tag = "llms-txt"is non-standard and will be ignored by crawlers. - To expose llms.txt as your AI policy, use a Link header with
rel="ai-policy". - If you actually intend to block AI crawlers or image-AI, use valid
X-Robots-Tagdirectives (e.g.,noai, noimageai). - We located your policy file at web/src/public/llms.txt. Make sure it’s published at the site root (https:///llms.txt)—either move it to web/public/llms.txt or adjust your Netlify publish directory.
Option A — advertise your policy file via Link header:
[[headers]]
for = "/*"
[headers.values]
- X-Robots-Tag = "llms-txt"
+ Link = "</llms.txt>; rel=\"ai-policy\""Option B — apply valid X-Robots-Tag directives:
[[headers]]
for = "/*"
[headers.values]
- X-Robots-Tag = "llms-txt"
+ X-Robots-Tag = "noai, noimageai"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[headers]] | |
| for = "/*" | |
| [headers.values] | |
| X-Robots-Tag = "llms-txt" | |
| [[headers]] | |
| for = "/*" | |
| [headers.values] | |
| Link = "</llms.txt>; rel=\"ai-policy\"" |
🤖 Prompt for AI Agents
In web/netlify.toml around lines 13 to 16, the X-Robots-Tag header is set to a
non-standard value "llms-txt" which will be ignored by crawlers. Replace this
header with a Link header that advertises your AI policy file using
rel="ai-policy" pointing to /llms.txt, or alternatively, use valid X-Robots-Tag
directives like "noai, noimageai" if you want to block AI crawlers. Also ensure
the llms.txt file is published at the site root by moving it to the web/public
directory or adjusting the Netlify publish directory accordingly.
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
web/src/hooks/useVotingContext.tsx (3)
45-47: Guard usage of disputeKitAddress in address-to-name resolutionIf
disputeKitAddressis temporarily undefined whiledisputeDataloads, ensureuseDisputeKitAddressestoleratesundefined. If not, gate the hook param.Possible defensive tweak:
- const { disputeKitName } = useDisputeKitAddresses({ disputeKitAddress }); + const { disputeKitName } = useDisputeKitAddresses({ disputeKitAddress: disputeKitAddress as `0x${string}` | undefined });Or add an internal guard in the hook to early-return when the address is undefined.
51-83: Pattern is sound; keep hooks stable and gate viaenabledCalling all four hooks with
enabledflags preserves hook order and avoids conditional-hook pitfalls. Refetch cadence is centralized viaREFETCH_INTERVAL. LGTM.If you want to remove repetition, you can extract the common
queryoptions:+ const baseQuery = { refetchInterval: REFETCH_INTERVAL as const }; const classicVoteResult = useReadDisputeKitClassicIsVoteActive({ - query: { - enabled: isEnabled && disputeKitName === DisputeKits.Classic, - refetchInterval: REFETCH_INTERVAL, - }, + query: { ...baseQuery, enabled: isEnabled && disputeKitName === DisputeKits.Classic }, args: hookArgs, });
84-105: Memo switch is correct; consider exposing a loading flag for the active kitThe selector is correct and dependencies are complete. Optional: expose
voteStatusLoadingfrom the active hook to improve UX around thehasVotedread.Example:
const activeVoteStatusLoading = useMemo(() => { switch (disputeKitName) { case DisputeKits.Classic: return classicVoteResult.isLoading; case DisputeKits.Shutter: return shutterVoteResult.isLoading; case DisputeKits.Gated: return gatedVoteResult.isLoading; case DisputeKits.GatedShutter: return gatedShutterVoteResult.isLoading; default: return false; } }, [disputeKitName, classicVoteResult.isLoading, shutterVoteResult.isLoading, gatedVoteResult.isLoading, gatedShutterVoteResult.isLoading]); // Optionally export it in the context valueweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (1)
32-39: Behavioral change: commit and reveal can render together—confirm this is intentionalThe fragment now allows both
<ShutterCommit />and<Reveal />to render if both flags are true. If the domain guaranteescommitandvoteperiods are mutually exclusive (as expected), this is fine and equivalent in practice. If not guaranteed, consider restoring exclusivity or adding a runtime assertion.Option to restore exclusivity:
- return ( - <> - {shouldShowCommit && ( - <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch, dispute, currentPeriodIndex, isGated }} /> - )} - {shouldShowReveal && <Reveal {...{ setIsOpen, voteIDs, isGated }} />} - </> - ); + return shouldShowCommit ? ( + <ShutterCommit {...{ arbitrable, setIsOpen, voteIDs, refetch, dispute, currentPeriodIndex, isGated }} /> + ) : shouldShowReveal ? ( + <Reveal {...{ setIsOpen, voteIDs, isGated }} /> + ) : null;Additionally, ensure
voteIDscan’t be undefined while the component renders. Consider defaulting to an empty array (outside this hunk):// Above: current line 27 const voteIDs = useMemo(() => drawData?.draws?.map((d) => d.voteIDNum) ?? [], [drawData]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/hooks/useVotingContext.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2024-10-14T13:58:25.708Z
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.
Applied to files:
web/src/hooks/useVotingContext.tsx
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
PR: kleros/kleros-v2#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/useVotingContext.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-12-09T12:36:59.441Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-10-09T10:22:41.474Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2025-05-09T13:39:15.086Z
Learnt from: tractorss
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsx
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsx
📚 Learning: 2024-12-16T17:17:32.359Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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/hooks/useVotingContext.tsx
📚 Learning: 2024-12-06T13:04:50.495Z
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Applied to files:
web/src/hooks/useVotingContext.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-11-07T10:48:16.774Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
Applied to files:
web/src/hooks/useVotingContext.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-11-19T05:29:56.238Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#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.
Applied to files:
web/src/hooks/useVotingContext.tsx
📚 Learning: 2024-10-29T10:13:04.524Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:69-69
Timestamp: 2024-10-29T10:13:04.524Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when the button is disabled, the associated logic won't be reached, and certain code paths may exist for TypeScript purposes.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-10-29T10:14:52.512Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:125-127
Timestamp: 2024-10-29T10:14:52.512Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when `isEmailUpdateable` is false, `user?.emailUpdateableAt` is always defined. Therefore, using the non-null assertion `!` with `user?.emailUpdateableAt!` is acceptable because TypeScript may not infer its definiteness.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-10-09T10:23:39.325Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1582
File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:98-103
Timestamp: 2024-10-09T10:23:39.325Z
Learning: In `SelectArbitrable.tsx` of the web-devtools project (React/TypeScript), direct DOM manipulation using `child.click()` is acceptable when considered reasonably safe.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
📚 Learning: 2024-10-09T10:17:37.935Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1582
File: web-devtools/src/app/(main)/dispute-template/page.tsx:324-360
Timestamp: 2024-10-09T10:17:37.935Z
Learning: In this codebase, prefer using the `??` (nullish coalescing operator) over `||` when handling default values for optional properties in React components.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx
⏰ 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). (4)
- GitHub Check: contracts-testing
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (2)
web/src/hooks/useVotingContext.tsx (2)
107-107: Good defaulting for hidden votesDefaulting
isHiddenVoteswith?? falseavoids transient undefineds from GraphQL. Matches the codebase’s preference for nullish coalescing over||.
48-50: Normalize all DisputeKit call arguments tobigintand includeidin the enabled guardViem/Wagmi read hooks for
IsVoteActiveexpect a tuple ofuint256values asbigint, but hereid,roundId, andvoteIdmay bestring | number. Also, the currentisEnabledguard skips checkingid, so the hook could fire with[0n, …].Proposed diff:
- const hookArgs = [BigInt(id ?? 0), roundId, voteId] as const; - const isEnabled = !isUndefined(roundId) && !isUndefined(voteId); + // Helper to coerce string|number to bigint + const toBigInt = (v: string | number | bigint) => + typeof v === "bigint" ? v : BigInt(v); + + // Only enable when all three IDs are defined + const disputeId = id !== undefined ? toBigInt(id) : undefined; + const roundIdBn = roundId !== undefined ? toBigInt(roundId) : undefined; + const voteIdBn = voteId !== undefined ? toBigInt(voteId) : undefined; + const isEnabled = + disputeId !== undefined && + roundIdBn !== undefined && + voteIdBn !== undefined; + + // Pass the normalized bigint tuple + const hookArgs = [disputeId!, roundIdBn!, voteIdBn!] as const;Please verify that the generated
useReadDisputeKit…IsVoteActivehook signatures indeed expect[bigint, bigint, bigint]and adjust accordingly.
| const shouldShowCommit = id && isCommitPeriod && !commited; | ||
| const shouldShowReveal = id && isVotingPeriod; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Coerce id to boolean to avoid string leakage into predicates
id && isCommitPeriod && !commited and id && isVotingPeriod may infer string | boolean. Use Boolean(id) (or !!id) to keep these strictly boolean.
Apply:
- const shouldShowCommit = id && isCommitPeriod && !commited;
- const shouldShowReveal = id && isVotingPeriod;
+ const hasId = Boolean(id);
+ const shouldShowCommit = hasId && isCommitPeriod && !commited;
+ const shouldShowReveal = hasId && isVotingPeriod;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldShowCommit = id && isCommitPeriod && !commited; | |
| const shouldShowReveal = id && isVotingPeriod; | |
| const hasId = Boolean(id); | |
| const shouldShowCommit = hasId && isCommitPeriod && !commited; | |
| const shouldShowReveal = hasId && isVotingPeriod; |
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx around lines 29 to
31, the variables shouldShowCommit and shouldShowReveal use `id` directly in
logical expressions, which can cause the result to be a string or boolean. To
fix this, explicitly coerce `id` to a boolean by replacing `id` with
`Boolean(id)` or `!!id` in these expressions to ensure the predicates are
strictly boolean values.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
88-91: Tiny readability tweak: trim onceAvoid calling trim on the env value more than necessary.
- if (!import.meta.env.REACT_APP_SHUTTER_API || import.meta.env.REACT_APP_SHUTTER_API.trim() === "") { + const shutterApi = import.meta.env.REACT_APP_SHUTTER_API?.trim(); + if (!shutterApi) { console.error("REACT_APP_SHUTTER_API environment variable is not set or is empty"); - throw new Error("Cannot commit vote: REACT_APP_SHUTTER_API environment variable is required but not set"); + // see earlier comment re: return vs throw + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(1 hunks)
⏰ 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). (5)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
88-91: Env prefix correctly configured in vite.config.jsThe project’s Vite configuration (web/vite.config.js at line 24) already includes
"REACT_APP"in itsenvPrefix, soimport.meta.env.REACT_APP_SHUTTER_APIwill be loaded as intended. No changes are required.
| if (!import.meta.env.REACT_APP_SHUTTER_API || import.meta.env.REACT_APP_SHUTTER_API.trim() === "") { | ||
| console.error("REACT_APP_SHUTTER_API environment variable is not set or is empty"); | ||
| throw new Error("Cannot commit vote: REACT_APP_SHUTTER_API environment variable is required but not set"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard is good, but avoid throwing inside the async handler; surface a user-facing error and return early
Throwing here can lead to an unhandled rejection unless the caller traps it. Prefer a toast/notification (or disable the commit UI upfront) and return early.
Apply minimal safe change (replace throw with a graceful return); wire to your toast system if available:
- if (!import.meta.env.REACT_APP_SHUTTER_API || import.meta.env.REACT_APP_SHUTTER_API.trim() === "") {
- console.error("REACT_APP_SHUTTER_API environment variable is not set or is empty");
- throw new Error("Cannot commit vote: REACT_APP_SHUTTER_API environment variable is required but not set");
- }
+ if (!import.meta.env.REACT_APP_SHUTTER_API || import.meta.env.REACT_APP_SHUTTER_API.trim() === "") {
+ console.error("REACT_APP_SHUTTER_API environment variable is not set or is empty");
+ // TODO: replace with your app's toast/notification system
+ // e.g., toast.error("Shutter API is not configured. Please contact support.");
+ return;
+ }Optional but recommended: also disable the commit action in the UI when the API is missing, so users never reach this branch. I can propose a patch for OptionsContainer usage if you want.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!import.meta.env.REACT_APP_SHUTTER_API || import.meta.env.REACT_APP_SHUTTER_API.trim() === "") { | |
| console.error("REACT_APP_SHUTTER_API environment variable is not set or is empty"); | |
| throw new Error("Cannot commit vote: REACT_APP_SHUTTER_API environment variable is required but not set"); | |
| } | |
| if (!import.meta.env.REACT_APP_SHUTTER_API || import.meta.env.REACT_APP_SHUTTER_API.trim() === "") { | |
| console.error("REACT_APP_SHUTTER_API environment variable is not set or is empty"); | |
| // TODO: replace with your app's toast/notification system | |
| // e.g., toast.error("Shutter API is not configured. Please contact support."); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx around lines 88 to
91, do not throw inside the async handler when REACT_APP_SHUTTER_API is missing;
instead log the error, show a user-facing notification (call the app's
toast/notification helper if available) and return early from the handler to
avoid an unhandled rejection. Replace the throw with a graceful return after
emitting the notification (or set an error state used by the UI), and
additionally ensure the commit action/button is disabled when the API env var is
not set so users cannot reach this branch.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/scripts/coverage.sh (1)
24-24: Scope VIA_IR to specific commands to avoid environment leakageExport works, but scoping the variable per-invocation makes the script more robust if additional steps are added.
Minimal change within this context:
-export VIA_IR=falseThen run the commands with scoped env:
VIA_IR=false yarn build VIA_IR=false yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --show-stack-traces --testfiles "test/**/*.ts"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/.solcover.js(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/scripts/coverage.sh(1 hunks)
⏰ 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). (20)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- 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-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
| module.exports = { | ||
| istanbulReporter: ["lcov"], | ||
| configureYulOptimizer: true, | ||
| irMinimum: true, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unrecognized solcover option "irMinimum" — likely a no-op; remove to avoid confusion
I don’t see "irMinimum" in solidity-coverage’s documented options. With coverage.sh already forcing VIA_IR=false for Hardhat, this flag is unnecessary and potentially misleading.
Apply this diff to remove it:
- irMinimum: true,If you want an inline hint about IR for coverage runs, consider a comment:
configureYulOptimizer: true,
+ // IR is disabled for coverage via scripts/coverage.sh (VIA_IR=false)Optionally verify support via docs:
🌐 Web query:
Is there a documented `irMinimum` option supported by solidity-coverage's .solcover.js config?
💡 Result:
Yes. The .solcover.js config includes a documented irMinimum option: a boolean (default false) that “speeds up test execution times when solc is run in viaIR mode,” and it may not work for all projects.[1]
Details:
- Location: Listed under Config Options in the solidity-coverage package docs.[1]
- Purpose: Enable a faster path when compiling with Solidity’s viaIR; useful if your project compiles successfully under coverage with it enabled.[1]
- Type/Default: Boolean, default false.[1]
You can set it in .solcover.js like:
module.exports = {
irMinimum: true
}
Mirrors of the same table appear in downstream copies of the docs as well.[2]
Citations:
- 1: https://classic.yarnpkg.com/en/package/solidity-coverage
- 2: https://gitlab.emse.fr/ext.r.fernandes/voting-system/-/tree/26-implement-proof-verification-system/backend/node_modules/solidity-coverage
- 3: https://github.com/sc-forks/solidity-coverage/blob/master/docs/advanced.md
- 4: https://github.com/sc-forks/solidity-coverage/blob/master/docs/api.md
- 5: https://www.npmjs.com/package/solidity-coverage/v/0.6.3
- 6: https://www.npmjs.com/package/solidity-coverage/v/0.7.17-rc.0
- 7: https://www.npmjs.com/package/solidity-coverage/v/0.7.20
- 8: https://learnblockchain.cn/docs/hardhat/plugins/solidity-coverage.html
- 9: https://forum.openzeppelin.com/t/test-smart-contracts-like-a-rockstar/1001
- 10: https://github.com/sc-forks/solidity-coverage
Documented option “irMinimum” is redundant under VIA_IR=false
solidity-coverage does support irMinimum (boolean, default false) to speed up test runs when compiling via IR, but your coverage.sh script forces VIA_IR=false, so this setting has no effect and may confuse future maintainers.
• Location: contracts/.solcover.js (line 10)
• Suggested change:
- irMinimum: true,
+ // IR is disabled for coverage via scripts/coverage.sh (VIA_IR=false), so `irMinimum` is redundant🤖 Prompt for AI Agents
In contracts/.solcover.js around line 10, the irMinimum: true option is
redundant because coverage.sh forces VIA_IR=false; remove the irMinimum property
or conditionally set it only when IR is enabled (e.g., check process.env.VIA_IR
=== 'true' and only include irMinimum when true) so the config reflects the
actual compilation mode and avoids confusing future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
contracts/src/gateway/interfaces/IHomeGateway.sol (1)
5-9: Missing import for IArbitratorV2 breaks compilationThis interface references IArbitratorV2 in the CrossChainDisputeIncoming event (Line 21) but does not import it. Indirect imports via IArbitrableV2 do not bring IArbitratorV2 into scope. This will fail to compile.
Apply this diff to add the missing import:
import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@kleros/vea-contracts/src/interfaces/gateways/ISenderGateway.sol"; import "../../arbitration/interfaces/IArbitrableV2.sol"; +import "../../arbitration/interfaces/IArbitratorV2.sol";Also applies to: 20-21
🧹 Nitpick comments (2)
contracts/src/gateway/interfaces/IHomeGateway.sol (1)
66-66: NatSpec tag typoUse @return for NatSpec consistency.
- /// return The fee token. + /// @return The fee token.contracts/src/arbitration/interfaces/IEvidence.sol (1)
8-10: Fix typos in NatSpec for clarityMinor spelling corrections improve readability in docs.
- /// @param _externalDisputeID Unique identifier for this dispute outside Kleros. It's the submitter responsability to submit the right external dispute ID. - /// @param _party The address of the party submiting the evidence. Note that 0x0 refers to evidence not submitted by any party. + /// @param _externalDisputeID Unique identifier for this dispute outside Kleros. It's the submitter's responsibility to submit the right external dispute ID. + /// @param _party The address of the party submitting the evidence. Note that 0x0 refers to evidence not submitted by any party.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/src/arbitration/interfaces/IArbitrableV2.sol(1 hunks)contracts/src/arbitration/interfaces/IArbitratorV2.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeTemplateRegistry.sol(1 hunks)contracts/src/arbitration/interfaces/IEvidence.sol(1 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/gateway/interfaces/IForeignGateway.sol(1 hunks)contracts/src/gateway/interfaces/IHomeGateway.sol(1 hunks)contracts/src/rng/IRandomizer.sol(1 hunks)contracts/src/rng/RNG.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- contracts/src/rng/RNG.sol
- contracts/src/arbitration/interfaces/ISortitionModule.sol
⏰ 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). (9)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (8)
contracts/src/rng/IRandomizer.sol (1)
3-3: Pragma range broadened — LGTMBroadening to >=0.8.0 <0.9.0 is fine for an interface and aligns with the repo-wide compiler upgrades. No API changes.
contracts/src/arbitration/interfaces/IArbitratorV2.sol (1)
3-3: Pragma range broadened — LGTMCompatible with the toolchain update to 0.8.30; no interface changes.
contracts/src/gateway/interfaces/IHomeGateway.sol (1)
3-3: Pragma range broadened — LGTMConsistent with the rest of the interfaces and the toolchain update.
contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
3-3: Pragma range broadened — LGTMInterface remains unchanged; the wider pragma is appropriate for external consumers.
contracts/src/arbitration/interfaces/IDisputeTemplateRegistry.sol (1)
3-3: Pragma range broadened — LGTMNo API changes; aligns with the repo-wide pragma update.
contracts/src/arbitration/interfaces/IArbitrableV2.sol (1)
3-3: Pragma range broadened — LGTMConsistent with other interfaces; no surface changes.
contracts/src/arbitration/interfaces/IEvidence.sol (1)
3-3: Pragma range broadened — LGTMInterface unchanged; safe pragma widening.
contracts/src/gateway/interfaces/IForeignGateway.sol (1)
3-3: Pragma range broadened — LGTMMatches the ecosystem-wide update; no interface changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
contracts/scripts/generateMetrics.sh (3)
7-7: Pin the CLI package for reproducibility.Unpinned dlx introduces non-deterministic outputs across environments. Pin solidity-code-metrics to a known-good version.
Example:
- Define: SCM_PKG='solidity-code-metrics@'
- Invoke: yarn dlx "$SCM_PKG" …
If you prefer, I can propose a specific version based on what you use in CI.
7-23: Future-proof file selection; avoid missing new contracts.The curated list risks drifting as new dispute kits/components are added. Consider generating the list dynamically and excluding mocks/tests.
Alternative snippet:
# Build a deterministic, filtered list of .sol files mapfile -t FILES < <(find "$SOURCE_DIR" -type f -name '*.sol' \ ! -path '*/mocks/*' ! -path '*/test/*' | sort) TMP_OUT="$(mktemp)" yarn dlx solidity-code-metrics "${FILES[@]}" >"$TMP_OUT" mv "$TMP_OUT" "$OUTPUT_FILE"This keeps metrics current as the codebase evolves (e.g., with multiple dispute kits).
1-1: Optional: Add a usage header and a Yarn availability check.Helps DX and avoids cryptic failures if yarn dlx isn’t available (Yarn Classic).
Example preflight:
command -v yarn >/dev/null || { echo "yarn is required"; exit 127; }And a brief header comment on output location and how to override OUTPUT_FILE.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/scripts/generateMetrics.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
contracts/scripts/generateMetrics.sh
[warning] 3-3: SCRIPT_DIR appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (5)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
contracts/scripts/generateMetrics.sh
Outdated
| SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" | ||
|
|
||
| SOURCE_DIR="src" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix SC2034 (unused var), make paths robust, and enable strict mode.
SCRIPT_DIR is computed but never used (ShellCheck SC2034). Also, relying on a relative "src" assumes the caller’s CWD. Use SCRIPT_DIR to anchor paths and enable strict mode for safer execution.
Apply this diff:
#!/usr/bin/env bash
-SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+set -Eeuo pipefail
+# Fail if any glob doesn’t match; helps catch missing files/typos.
+shopt -s failglob
+
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
-SOURCE_DIR="src"
+# Source directory relative to this script (contracts/scripts -> contracts/src).
+SOURCE_DIR="$SCRIPT_DIR/../src"
+# Allow overriding output path via env; defaults to CWD/METRICS.md.
+OUTPUT_FILE="${OUTPUT_FILE:-METRICS.md}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" | |
| SOURCE_DIR="src" | |
| #!/usr/bin/env bash | |
| set -Eeuo pipefail | |
| # Fail if any glob doesn’t match; helps catch missing files/typos. | |
| shopt -s failglob | |
| SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" | |
| # Source directory relative to this script (contracts/scripts -> contracts/src). | |
| SOURCE_DIR="$SCRIPT_DIR/../src" | |
| # Allow overriding output path via env; defaults to CWD/METRICS.md. | |
| OUTPUT_FILE="${OUTPUT_FILE:-METRICS.md}" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 3-3: SCRIPT_DIR appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In contracts/scripts/generateMetrics.sh around lines 3 to 6, SCRIPT_DIR is
computed but unused and SOURCE_DIR is a relative path; enable strict mode and
anchor SOURCE_DIR to the script location: add a strict-mode header (set -euo
pipefail and a safe IFS) at the top of the script, keep or keep-and-use the
existing SCRIPT_DIR variable, and change SOURCE_DIR to reference SCRIPT_DIR
(e.g., SOURCE_DIR="${SCRIPT_DIR}/src") so paths are robust regardless of the
caller's CWD.
contracts/scripts/generateMetrics.sh
Outdated
| yarn dlx solidity-code-metrics \ | ||
| "$SOURCE_DIR"/arbitration/KlerosCore* \ | ||
| "$SOURCE_DIR"/arbitration/PolicyRegistry.sol \ | ||
| "$SOURCE_DIR"/arbitration/SortitionModule* \ | ||
| "$SOURCE_DIR"/arbitration/arbitrables/DisputeResolver.sol \ | ||
| "$SOURCE_DIR"/arbitration/DisputeTemplateRegistry.sol \ | ||
| "$SOURCE_DIR"/arbitration/dispute-kits/* \ | ||
| "$SOURCE_DIR"/arbitration/evidence/EvidenceModule.sol \ | ||
| "$SOURCE_DIR"/arbitration/interfaces/* \ | ||
| "$SOURCE_DIR"/libraries/Constants.sol \ | ||
| "$SOURCE_DIR"/libraries/Safe* \ | ||
| "$SOURCE_DIR"/rng/RNGWithFallback.sol \ | ||
| "$SOURCE_DIR"/rng/ChainlinkRNG.sol \ | ||
| "$SOURCE_DIR"/rng/IRNG.sol \ | ||
| "$SOURCE_DIR"/proxy/UUPSProx* \ | ||
| "$SOURCE_DIR"/proxy/Initializable.sol \ | ||
| >METRICS.md |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid truncating METRICS.md on failure; write atomically via a temp file.
As written, shell truncates METRICS.md before yarn runs; a failure produces an empty report. Write to a temp file and move on success.
Apply this diff:
-yarn dlx solidity-code-metrics \
- "$SOURCE_DIR"/arbitration/KlerosCore* \
- "$SOURCE_DIR"/arbitration/PolicyRegistry.sol \
- "$SOURCE_DIR"/arbitration/SortitionModule* \
- "$SOURCE_DIR"/arbitration/arbitrables/DisputeResolver.sol \
- "$SOURCE_DIR"/arbitration/DisputeTemplateRegistry.sol \
- "$SOURCE_DIR"/arbitration/dispute-kits/* \
- "$SOURCE_DIR"/arbitration/evidence/EvidenceModule.sol \
- "$SOURCE_DIR"/arbitration/interfaces/* \
- "$SOURCE_DIR"/libraries/Constants.sol \
- "$SOURCE_DIR"/libraries/Safe* \
- "$SOURCE_DIR"/rng/RNGWithFallback.sol \
- "$SOURCE_DIR"/rng/ChainlinkRNG.sol \
- "$SOURCE_DIR"/rng/IRNG.sol \
- "$SOURCE_DIR"/proxy/UUPSProx* \
- "$SOURCE_DIR"/proxy/Initializable.sol \
->METRICS.md
+TMP_OUT="$(mktemp)"
+yarn dlx solidity-code-metrics \
+ "$SOURCE_DIR"/arbitration/KlerosCore* \
+ "$SOURCE_DIR"/arbitration/PolicyRegistry.sol \
+ "$SOURCE_DIR"/arbitration/SortitionModule* \
+ "$SOURCE_DIR"/arbitration/arbitrables/DisputeResolver.sol \
+ "$SOURCE_DIR"/arbitration/DisputeTemplateRegistry.sol \
+ "$SOURCE_DIR"/arbitration/dispute-kits/* \
+ "$SOURCE_DIR"/arbitration/evidence/EvidenceModule.sol \
+ "$SOURCE_DIR"/arbitration/interfaces/* \
+ "$SOURCE_DIR"/libraries/Constants.sol \
+ "$SOURCE_DIR"/libraries/Safe* \
+ "$SOURCE_DIR"/rng/RNGWithFallback.sol \
+ "$SOURCE_DIR"/rng/ChainlinkRNG.sol \
+ "$SOURCE_DIR"/rng/IRNG.sol \
+ "$SOURCE_DIR"/proxy/UUPSProx* \
+ "$SOURCE_DIR"/proxy/Initializable.sol \
+>"$TMP_OUT"
+mv "$TMP_OUT" "$OUTPUT_FILE"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/scripts/generateMetrics.sh around lines 7 to 23, the current
redirection (>) truncates METRICS.md before yarn runs so a failure leaves an
empty file; change the script to write output to a temporary file (use mktemp or
similar), run the yarn dlx command redirecting stdout/stderr into that temp
file, check the command exit status, on success mv the temp file to METRICS.md
(atomic replacement), and on failure remove the temp file and exit with a
non-zero status; also ensure the temp file is created securely and cleaned up in
all error paths.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (11)
contracts/src/arbitration/arbitrables/DisputeResolver.sol (2)
110-120: Critical: Unknown arbitrator dispute IDs can silently target dispute 0If
_arbitratorDisputeIDis not mapped,arbitratorDisputeIDToLocalID[_arbitratorDisputeID]returns 0 and will indexdisputes[0]. This can wrongly mark and emit a ruling for dispute 0. Guard against unknown IDs using a sentinel offset mapping.Apply this diff within rule():
- uint256 localDisputeID = arbitratorDisputeIDToLocalID[_arbitratorDisputeID]; - DisputeStruct storage dispute = disputes[localDisputeID]; + uint256 localDisputeIDPlusOne = arbitratorDisputeIDToLocalID[_arbitratorDisputeID]; + if (localDisputeIDPlusOne == 0) revert UnknownDisputeID(); + uint256 localDisputeID = localDisputeIDPlusOne - 1; + DisputeStruct storage dispute = disputes[localDisputeID];And in _createDispute():
- arbitratorDisputeIDToLocalID[arbitratorDisputeID] = localDisputeID; + arbitratorDisputeIDToLocalID[arbitratorDisputeID] = localDisputeID + 1;Add the error to the errors section:
+ error UnknownDisputeID();Note: This “+1 sentinel” pattern avoids ambiguous zero values and prevents misrouting rules to dispute 0.
145-148: Align mapping write with sentinel approachIf you adopt the sentinel mapping to prevent unknown IDs, update the assignment accordingly.
- arbitratorDisputeIDToLocalID[arbitratorDisputeID] = localDisputeID; + arbitratorDisputeIDToLocalID[arbitratorDisputeID] = localDisputeID + 1;contracts/src/arbitration/devtools/DisputeResolverRuler.sol (1)
39-53: Keep sentinel mapping consistent with base fixTo stay consistent with the base contract’s safe lookup, store
localID + 1here too.- arbitratorDisputeID = IKlerosCoreRulerFragment(address(arbitrator)).getNextDisputeID(); - arbitratorDisputeIDToLocalID[arbitratorDisputeID] = localDisputeID; + arbitratorDisputeID = IKlerosCoreRulerFragment(address(arbitrator)).getNextDisputeID(); + arbitratorDisputeIDToLocalID[arbitratorDisputeID] = localDisputeID + 1;contracts/src/gateway/HomeGateway.sol (2)
210-222: Guard against unknown dispute IDs in rule()If
_disputeIDhas no mapping,disputeHashis zero and you’ll send a cross-chain rule with a zero hash and relayer=0x0. Add a guard.function rule(uint256 _disputeID, uint256 _ruling) external override { if (msg.sender != address(arbitrator)) revert ArbitratorOnly(); - bytes32 disputeHash = disputeIDtoHash[_disputeID]; + bytes32 disputeHash = disputeIDtoHash[_disputeID]; + if (disputeHash == bytes32(0)) revert UnknownDisputeID(); RelayedData memory relayedData = disputeHashtoRelayedData[disputeHash];Add the error:
+ error UnknownDisputeID();
130-172: Consider nonReentrant guards around relayCreateDispute pathsBoth relay functions call into an external
arbitrator. A malicious arbitrator could reenterrule()before mappings are populated. Add a minimal nonReentrant guard to both relay functions (and optionally to rule()).I can draft a minimal custom nonReentrant modifier using a
lockedflag and aReentrancy()error, matching the repo’s custom-error style. Want me to open a PR patch?Also applies to: 168-208
contracts/src/gateway/ForeignGateway.sol (2)
176-183: Replace string reverts with a typedNotSupportederrorThese overloads still revert with a string. For consistency and cheaper bytecode, introduce and use a custom error.
- ) external pure override returns (uint256) { - revert("Not supported"); + ) external pure override returns (uint256) { + revert NotSupported(); } @@ - ) public pure override returns (uint256 /*cost*/) { - revert("Not supported"); + ) public pure override returns (uint256 /*cost*/) { + revert NotSupported(); } @@ error GovernorOnly(); error HomeGatewayMessageSenderOnly(); error VeaOutboxOnly(); error ArbitrationFeesNotEnough(); error DisputeDoesNotExist(); error CannotRuleTwice(); error NotRuledYet(); + error NotSupported();Also applies to: 192-197, 271-282
219-227: Avoid usingtransfer; usecalland handle failures
transfercan break for relayers that are contracts or under EVM gas cost changes. Usecalland revert on failure. Consider a specific error for clarity.uint256 amount = dispute.paid; dispute.paid = 0; - payable(dispute.relayer).transfer(amount); + (bool ok, ) = payable(dispute.relayer).call{value: amount}(""); + if (!ok) revert TransferToRelayerFailed();And add an error:
error GovernorOnly(); @@ error NotRuledYet(); + error TransferToRelayerFailed();Also applies to: 271-282
contracts/src/arbitration/KlerosGovernor.sol (1)
260-270: Use the existing SafeSend pattern instead oftransferEarlier in this contract you already use
safeSendwithwNative. Mirror that here to avoidtransferbrittleness.- payable(msg.sender).transfer(submission.deposit); + payable(msg.sender).safeSend(submission.deposit, wNative);contracts/src/arbitration/SortitionModuleBase.sol (2)
209-223: Make delayed-stake iteration calculation safer and simplerThe current actualIterations formula risks overflow on (delayedStakeReadIndex + _iterations) and is harder to reason about. Consider a saturating computation based on the number of available items.
Apply this diff:
- if (phase != Phase.staking) revert NotStakingPhase(); - if (delayedStakeWriteIndex < delayedStakeReadIndex) revert NoDelayedStakeToExecute(); - - uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex - ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1 - : _iterations; - uint256 newDelayedStakeReadIndex = delayedStakeReadIndex + actualIterations; + if (phase != Phase.staking) revert NotStakingPhase(); + if (delayedStakeWriteIndex < delayedStakeReadIndex) revert NoDelayedStakeToExecute(); + + // Items available to process: [delayedStakeReadIndex, delayedStakeWriteIndex], inclusive. + uint256 available = delayedStakeWriteIndex + 1 - delayedStakeReadIndex; + uint256 actualIterations = _iterations > available ? available : _iterations; + if (actualIterations == 0) revert NoDelayedStakeToExecute(); + uint256 newDelayedStakeReadIndex = delayedStakeReadIndex + actualIterations;
443-472: Review Draw Gating Impact on Dispute Kit
The newNotDrawingPhase()revert inSortitionModuleBase.draw()affects one off-core consumer:
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
• Line 241:drawnAddress = sortitionModule.draw(key, _coreDisputeID, _nonce);Because
draw()now reverts wheneverphase != drawing, ensure this call is only executed during the drawing phase. If it can run earlier (e.g., in view-only simulations), adjust the logic to:
- Guard the call with a phase check
- Or catch/handle the revert in this dispute kit
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
271-284: Fix commit counting to prevent duplicate overcount and enforce non-empty commit setCurrently, totalCommitted is incremented by the length of _voteIDs regardless of whether any were already committed or duplicated, which can prematurely signal “all commits cast.” Count only newly committed votes and reject empty arrays.
Apply this diff:
- (, , KlerosCoreBase.Period period, , ) = core.disputes(_coreDisputeID); - if (period != KlerosCoreBase.Period.commit) revert NotCommitPeriod(); - if (_commit == bytes32(0)) revert EmptyCommit(); - if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + (, , KlerosCoreBase.Period period, , ) = core.disputes(_coreDisputeID); + if (period != KlerosCoreBase.Period.commit) revert NotCommitPeriod(); + if (_commit == bytes32(0)) revert EmptyCommit(); + if (_voteIDs.length == 0) revert EmptyVoteIDs(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); @@ - for (uint256 i = 0; i < _voteIDs.length; i++) { - if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote(); - round.votes[_voteIDs[i]].commit = _commit; - } - round.totalCommitted += _voteIDs.length; + uint256 newCommits = 0; + for (uint256 i = 0; i < _voteIDs.length; i++) { + uint256 id = _voteIDs[i]; + if (round.votes[id].account != msg.sender) revert JurorHasToOwnTheVote(); + if (round.votes[id].commit == bytes32(0)) newCommits++; + round.votes[id].commit = _commit; + } + round.totalCommitted += newCommits;Note: reusing EmptyVoteIDs for commit is consistent with vote path.
♻️ Duplicate comments (1)
contracts/CHANGELOG.md (1)
131-132: Remove unpublished release link for 0.13.0 to avoid 404The header states “Not published yet,” but Lines 131–132 add the live link. This will likely 404 until the release is published. Comment it out and add after publishing.
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 +<!-- [0.13.0]: (add after publishing) -->
🧹 Nitpick comments (17)
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
71-75: Avoid duplicating common errors across contractsGovernorOnly is being declared in multiple modules in this PR. Consider centralizing common access-control errors (e.g., in a small Errors.sol or a Governable base) to reduce duplication and keep selectors uniform across tests.
Example change within this file:
- error GovernorOnly(); + // Prefer importing a shared error: + // import {GovernorOnly} from "../errors/AccessErrors.sol"; + // error declaration removedAnd add once (outside this hunk) in a shared file:
// contracts/src/errors/AccessErrors.sol // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; error GovernorOnly();contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (1)
73-77: Consolidate shared errors to reduce duplicationAs with other modules, consider importing a shared GovernorOnly error instead of re-declaring it in each contract.
- error GovernorOnly(); + // import {GovernorOnly} from "../../errors/AccessErrors.sol";contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
84-88: DRY up error declarationsSame suggestion as elsewhere: centralize GovernorOnly in a shared errors file or a lightweight base to avoid duplication and make selector imports in tests consistent.
- error GovernorOnly(); + // import {GovernorOnly} from "../errors/AccessErrors.sol";contracts/src/arbitration/PolicyRegistry.sol (1)
89-93: Centralize common error typesRecommend importing a shared GovernorOnly error rather than re-declaring.
- error GovernorOnly(); + // import {GovernorOnly} from "../errors/AccessErrors.sol";contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
368-375: Consider reusing shared error declarationsGovernorOnly, KlerosCoreOnly, and NotEligibleForWithdrawal are common across modules. Centralizing them (e.g., Errors.sol or a base contract) would reduce duplication and keep ABI selectors consistent.
- error GovernorOnly(); - error KlerosCoreOnly(); - error NotEligibleForWithdrawal(); + // import {GovernorOnly, KlerosCoreOnly, NotEligibleForWithdrawal} from "../../errors/AccessErrors.sol";contracts/src/arbitration/arbitrables/DisputeResolver.sol (1)
51-63: Access control migrated to custom errors looks goodThe governor checks via custom errors are consistent with the repo-wide pattern. Consider emitting events for governor/arbitrator/template changes for better off-chain observability (optional).
contracts/src/gateway/HomeGateway.sol (3)
130-148: ERC20/native path guards look correct; set relayed metadataThe fee medium and foreignChainID checks are sound. Populate
arbitrationCostto avoid stale/unused field.RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash]; if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed(); uint256 disputeID = arbitrator.createDispute{value: msg.value}(_params.choices, _params.extraData); disputeIDtoHash[disputeID] = disputeHash; disputeHashtoID[disputeHash] = disputeID; relayedData.relayer = msg.sender; + relayedData.arbitrationCost = msg.value;
168-189: ERC20 path: set relayed metadata and consider allowance strategyPopulate
arbitrationCost. Optional: consider approving exact amount from zero to avoid cumulative allowances, unless arbitrator expects accumulating allowances.RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash]; if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed(); if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed(); if (!feeToken.increaseAllowance(address(arbitrator), _feeAmount)) revert AllowanceIncreaseFailed(); uint256 disputeID = arbitrator.createDispute(_params.choices, _params.extraData, feeToken, _feeAmount); disputeIDtoHash[disputeID] = disputeHash; disputeHashtoID[disputeHash] = disputeID; relayedData.relayer = msg.sender; + relayedData.arbitrationCost = _feeAmount;If you opt for exact allowance from zero, change to:
- approve(0) then approve(_feeAmount), or use a safeApprove helper that enforces zero-first, depending on your SafeERC20 semantics.
96-124: Optional: emit events on parameter changesEmitting events for arbitrator/vea/foreignGateway/feeToken changes will help indexers and off-chain services track configuration updates.
contracts/src/arbitration/arbitrables/ArbitrableExample.sol (1)
95-116: Optional: emit change events for governance updatesEmitting events on arbitrator/template/extraData changes helps off-chain consumers.
Also applies to: 117-138
contracts/src/gateway/ForeignGateway.sol (2)
74-79: Fix incorrect constructor docstringThe docstring says “Constructs the PolicyRegistry contract,” which is misleading in this gateway contract context.
Apply:
- /// @dev Constructs the `PolicyRegistry` contract. + /// @dev Initializes the ForeignGateway contract.
106-110: Use theonlyByGovernormodifier for consistencyBoth functions manually check
governor != msg.senderand revert withGovernorOnly(). ReusingonlyByGovernorreduces duplication and keeps access control consistent with other governance functions.- function changeGovernor(address _governor) external { - if (governor != msg.sender) revert GovernorOnly(); + function changeGovernor(address _governor) external onlyByGovernor { governor = _governor; } @@ - function changeHomeGateway(address _homeGateway) external { - if (governor != msg.sender) revert GovernorOnly(); + function changeHomeGateway(address _homeGateway) external onlyByGovernor { homeGateway = _homeGateway; }Also applies to: 124-126
contracts/src/arbitration/KlerosGovernor.sol (1)
84-86: Clarify the meaning of GovernorOnly in this contextHere “governor” is
address(this). Consider renaming the error to improve clarity (e.g.,SelfOnly) or documenting this in a NatSpec note on the modifier to avoid confusion.contracts/src/arbitration/SortitionModuleBase.sol (1)
692-708: Error declarations are clear and cohesiveGood coverage and naming consistency. Consider centralizing common errors in a shared interface/library to avoid duplication across modules.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
169-175: Consider surfacing revert data for failed governor callsReverting with UnsuccessfulCall() is fine; optionally include the return data for debugging (e.g., error UnsuccessfulCall(bytes returndata)) and bubble it.
Example:
- (bool success, ) = _destination.call{value: _amount}(_data); - if (!success) revert UnsuccessfulCall(); + (bool success, bytes memory returndata) = _destination.call{value: _amount}(_data); + if (!success) revert UnsuccessfulCall(returndata);And update the error:
-error UnsuccessfulCall(); +error UnsuccessfulCall(bytes returndata);
320-335: Micro-optimization: compute voteHash only when hiddenVotes is trueAvoids unnecessary hashing when votes aren’t hidden. Keeps semantics identical.
Apply this diff:
- (, bool hiddenVotes, , , , , ) = core.courts(courtID); - bytes32 voteHash = hashVote(_choice, _salt, _justification); + (, bool hiddenVotes, , , , , ) = core.courts(courtID); + bytes32 voteHash; + if (hiddenVotes) { + voteHash = hashVote(_choice, _salt, _justification); + } @@ - if (round.votes[_voteIDs[i]].account != _juror) revert JurorHasToOwnTheVote(); - if (hiddenVotes && round.votes[_voteIDs[i]].commit != voteHash) - revert HashDoesNotMatchHiddenVoteCommitment(); + if (round.votes[_voteIDs[i]].account != _juror) revert JurorHasToOwnTheVote(); + if (hiddenVotes && round.votes[_voteIDs[i]].commit != voteHash) + revert HashDoesNotMatchHiddenVoteCommitment();
717-735: Error set is comprehensive and consistentWell-structured errors. Consider extracting common errors (GovernorOnly, KlerosCoreOnly, NotActiveForCoreDisputeID, etc.) into a shared library/interface to reduce duplication across contracts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
contracts/CHANGELOG.md(2 hunks)contracts/src/arbitration/DisputeTemplateRegistry.sol(2 hunks)contracts/src/arbitration/KlerosGovernor.sol(8 hunks)contracts/src/arbitration/PolicyRegistry.sol(2 hunks)contracts/src/arbitration/SortitionModuleBase.sol(6 hunks)contracts/src/arbitration/arbitrables/ArbitrableExample.sol(3 hunks)contracts/src/arbitration/arbitrables/DisputeResolver.sol(4 hunks)contracts/src/arbitration/devtools/DisputeResolverRuler.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(8 hunks)contracts/src/arbitration/evidence/EvidenceModule.sol(2 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(3 hunks)contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol(2 hunks)contracts/src/gateway/ForeignGateway.sol(8 hunks)contracts/src/gateway/HomeGateway.sol(7 hunks)contracts/test/arbitration/index.ts(1 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/foundry/KlerosCore.t.sol(16 hunks)
⏰ 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). (12)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (28)
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
25-25: Typed custom error guard in onlyByGovernor — LGTMSwitching to a typed error improves gas usage and standardizes revert handling. Behavior is preserved.
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (1)
29-29: Typed custom error in onlyByGovernor — LGTMConsistent with the repo-wide shift to typed errors; keeps gas lower than string reverts and simplifies selector-based testing.
contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
28-28: Custom error-based access control — LGTMThe modifier semantics remain unchanged and are now gas-optimized with a typed error.
contracts/src/arbitration/PolicyRegistry.sol (1)
35-35: Typed error in onlyByGovernor — LGTMConsistent with the broader migration; keeps revert reasons concise and gas-efficient.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (3)
70-72: Governor guard migrated to typed error — LGTMAccess control behavior preserved with lower-gas revert path.
75-77: Core-only guard migrated to typed error — LGTMClearer intent and selector-friendly for tests. No functional change.
283-287: Use of NotEligibleForWithdrawal() improves clarity — LGTMSemantics preserved; revert is now an explicit typed error. Ensure tests assert on the selector.
contracts/src/arbitration/arbitrables/DisputeResolver.sol (1)
133-134: Good: Replace require with custom error for ruling options boundThe new guard is correct and cheaper in gas.
contracts/src/gateway/HomeGateway.sol (2)
46-50: LGTM: onlyByGovernor with custom errorConsistent with repo-wide error style.
17-17: Confirm version string policy
versionis "0.8.0" while the toolchain and errors changed. If this field is a protocol/ABI version visible to other components, consider bumping. If it’s unrelated to Solidity compiler version, ignore.Would you like me to scan usages of
HomeGateway.versionacross the repo to determine coupling and propose a safe bump?contracts/src/arbitration/arbitrables/ArbitrableExample.sol (2)
39-42: LGTM: onlyByGovernor migrated to custom errorConsistent with the rest of the codebase.
129-131: SafeERC20 usage pattern checkAssuming your SafeERC20 returns boolean, this is fine. If it reverts on failure (typical OZ semantics), the boolean checks are dead code. Confirm library semantics.
I can grep the SafeERC20 implementation in this repo to confirm return types and usages if you’d like.
contracts/src/gateway/ForeignGateway.sol (3)
52-57: Access control guard looks correct and readableThe consolidated checks correctly allow either the current veaOutbox or, during grace period, the deprecated outbox; and enforce the message sender to be the homeGateway. No functional concerns.
61-63: Governor-only modifier migration LGTMReplacing the require with a custom error simplifies bytecode and aligns with the PR-wide pattern.
145-146: Typed error for insufficient arbitration fees is correctSwitching to
ArbitrationFeesNotEnough()is in line with the repository-wide shift to custom errors.contracts/src/arbitration/KlerosGovernor.sol (2)
73-75: Period guards converted to typed errors — LGTMThe logic remains equivalent and the custom errors improve introspection.
Also applies to: 79-81
415-433: Error taxonomy reads well and matches call sitesThe custom errors map one-to-one to the guards above. Good consistency.
contracts/test/arbitration/index.ts (1)
76-79: Test updated to custom error — LGTMSwitching to
revertedWithCustomError(disputeKit, "KlerosCoreOnly")matches the new typed error.contracts/test/foundry/KlerosCore.t.sol (1)
1140-1141: Migration to error selectors across Foundry tests — LGTMThe assertions now use
.selectorfor custom errors, aligning tests with the new revert style across the contracts. The updated imports cover the error types referenced.Also applies to: 1153-1155, 1273-1274, 1281-1282, 1569-1570, 1592-1593, 1630-1631, 1634-1635, 1702-1703, 1720-1721, 1726-1727, 1730-1731, 1739-1740, 1974-1999, 2028-2029, 2036-2037, 2043-2044, 2214-2215, 2598-2599
contracts/src/arbitration/SortitionModuleBase.sol (4)
124-132: Good switch to custom errors in access-control modifiersReplacing string reverts with typed errors reduces gas and standardizes error handling.
200-204: Tree creation guards LGTMThe checks for existing tree and K > 1 are correct and now use typed errors.
419-423: Withdrawal guard LGTMReverting with NotEligibleForWithdrawal when there is no leftover PNK is correct and avoids silent no-ops.
172-192: Sentinel 0 is the documented “not ready” value and all production RNGs enforce non-zero once readyThe RNG interface (RNG.sol) explicitly specifies that
receiveRandomnessreturns 0 if the value is not yet available.
– BlockhashRNG: once_block < block.number, it always writes a non-zero blockhash (or fallback) before returning.
– ChainlinkRNG: maps to VRF output, which is effectively uniform over the full 256-bit space and treated as “ready” when non-zero.
– RandomizerRNG: only returns 0 until its callback populates a new value.In practice none of the on-chain generators will legitimately return 0 once randomness is ready, so testing
randomNumber == 0remains a correct readiness check. No changes are required.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
127-135: Good adoption of typed errors in access-control modifiersThe shift to custom errors reduces gas and aligns error semantics.
313-316: Vote-period and active-dispute checks LGTMAccurate use of custom errors for period and activity validation.
318-318: Bounds check on choice LGTMChoiceOutOfBounds prevents invalid options early.
360-386: Appeal funding guards LGTMChoice bounds, active-dispute, appeal period window (including loser’s shortened window), and duplicate funding prevention all look correct.
442-445: Withdrawal guards LGTMExplicit checks for dispute resolution, core pause, and activity improve safety of reimbursements.
| uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID]; | ||
| DisputeStruct storage dispute = disputes[localDisputeID]; | ||
| require(msg.sender == address(arbitrator), "Only the arbitrator can execute this."); | ||
| require(_ruling <= dispute.numberOfRulingOptions, "Invalid ruling."); | ||
| require(dispute.isRuled == false, "This dispute has been ruled already."); | ||
| if (msg.sender != address(arbitrator)) revert ArbitratorOnly(); | ||
| if (_ruling > dispute.numberOfRulingOptions) revert RulingOutOfBounds(); | ||
| if (dispute.isRuled) revert DisputeAlreadyRuled(); | ||
|
|
||
| dispute.isRuled = true; | ||
| dispute.ruling = _ruling; | ||
|
|
||
| emit Ruling(IArbitratorV2(msg.sender), _arbitratorDisputeID, dispute.ruling); | ||
| } |
There was a problem hiding this comment.
Critical: Unknown arbitrator dispute IDs can silently target dispute 0
Same issue as DisputeResolver: missing sentinel mapping allows unknown _arbitratorDisputeID to operate on disputes[0].
Apply these diffs:
In rule():
- uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];
- DisputeStruct storage dispute = disputes[localDisputeID];
+ uint256 localDisputeIDPlusOne = externalIDtoLocalID[_arbitratorDisputeID];
+ if (localDisputeIDPlusOne == 0) revert UnknownDisputeID();
+ uint256 localDisputeID = localDisputeIDPlusOne - 1;
+ DisputeStruct storage dispute = disputes[localDisputeID];Where the mapping is written:
- externalIDtoLocalID[disputeID] = localDisputeID;
+ externalIDtoLocalID[disputeID] = localDisputeID + 1;Add the error:
+ error UnknownDisputeID();🤖 Prompt for AI Agents
In contracts/src/arbitration/arbitrables/ArbitrableExample.sol around lines
143-153, the function currently looks up localDisputeID =
externalIDtoLocalID[_arbitratorDisputeID] and proceeds without verifying the
mapping, allowing unknown external IDs to silently resolve to dispute 0; add a
guard that reverts when localDisputeID == 0 (e.g., revert
UnknownArbitratorDisputeID()) before accessing disputes array/struct, declare
the new error at the top of the contract, and ensure elsewhere when
creating/writing the mapping you use a 1-based sentinel (never map a valid
external ID to 0) by setting externalIDtoLocalID[_arbitratorDisputeID] = localID
(where localID is non-zero) so the new check reliably detects unknown IDs.
| error GovernorOnly(); | ||
| error ArbitratorOnly(); | ||
| error RulingOutOfBounds(); | ||
| error DisputeAlreadyRuled(); | ||
| error ShouldBeAtLeastTwoRulingOptions(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add UnknownDisputeID custom error
Needed for the safe lookup in rule().
error GovernorOnly();
error ArbitratorOnly();
error RulingOutOfBounds();
error DisputeAlreadyRuled();
error ShouldBeAtLeastTwoRulingOptions();
+ error UnknownDisputeID();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error GovernorOnly(); | |
| error ArbitratorOnly(); | |
| error RulingOutOfBounds(); | |
| error DisputeAlreadyRuled(); | |
| error ShouldBeAtLeastTwoRulingOptions(); | |
| } | |
| error GovernorOnly(); | |
| error ArbitratorOnly(); | |
| error RulingOutOfBounds(); | |
| error DisputeAlreadyRuled(); | |
| error ShouldBeAtLeastTwoRulingOptions(); | |
| error UnknownDisputeID(); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/arbitrables/DisputeResolver.sol around lines
154-159, add a new custom error declaration named UnknownDisputeID() to the
existing error list and update rule() to revert with UnknownDisputeID() when a
dispute lookup fails (safe lookup) instead of allowing an out-of-bounds access
or a generic revert; place the error alongside the other error definitions and
use it at the start of rule() to guard against invalid dispute IDs.
| modifier notJumped(uint256 _coreDisputeID) { | ||
| require(!disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped, "Dispute jumped to a parent DK!"); | ||
| if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); | ||
| _; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against non-active disputes in notJumped modifier
Accessing disputes[coreDisputeIDToLocal[_coreDisputeID]] without verifying active status relies on later checks to catch inactive IDs. Add an active-dispute guard here to prevent accidental reads of an uninitialized local dispute (id 0).
Apply this diff:
modifier notJumped(uint256 _coreDisputeID) {
- if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK();
+ if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();
+ if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK();
_;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modifier notJumped(uint256 _coreDisputeID) { | |
| require(!disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped, "Dispute jumped to a parent DK!"); | |
| if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); | |
| _; | |
| } | |
| modifier notJumped(uint256 _coreDisputeID) { | |
| if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); | |
| if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); | |
| _; | |
| } |
| if (_target.length != _value.length) revert WrongInputTargetAndValue(); | ||
| if (_target.length != _dataSize.length) revert WrongInputTargetAndDatasize(); | ||
| Session storage session = sessions[sessions.length - 1]; | ||
| Submission storage submission = submissions.push(); | ||
| submission.submitter = payable(msg.sender); | ||
| // Do the assignment first to avoid creating a new variable and bypass a 'stack too deep' error. | ||
| submission.deposit = submissionBaseDeposit + arbitrator.arbitrationCost(arbitratorExtraData); | ||
| require(msg.value >= submission.deposit, "Not enough ETH to cover deposit"); | ||
| if (msg.value < submission.deposit) revert InsufficientDeposit(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate _data bounds while splitting calldata
Currently the loop copies _dataSize[i] bytes without guarding against readingPosition + _dataSize[i] > _data.length. Out-of-bounds will revert with a generic panic. Fail fast with a typed error for better dev UX and clearer failures.
uint256 readingPosition;
for (uint256 i = 0; i < _target.length; i++) {
- bytes memory readData = new bytes(_dataSize[i]);
+ if (readingPosition + _dataSize[i] > _data.length) revert WrongInputTargetAndDatasize();
+ bytes memory readData = new bytes(_dataSize[i]);
Transaction storage transaction = submission.txs.push();
transaction.target = _target[i];
transaction.value = _value[i];
for (uint256 j = 0; j < _dataSize[i]; j++) {
readData[j] = _data[readingPosition + j];
}
transaction.data = readData;
readingPosition += _dataSize[i];Also applies to: 223-235
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosGovernor.sol around lines 211-219 (also apply
same change to lines 223-235), the code copies _dataSize[i] bytes from calldata
without checking bounds which causes an uncaught panic; add an explicit bounds
check before each copy: if readingPosition + _dataSize[i] > _data.length then
revert with a typed error (e.g., InvalidInputDataSize or DataOutOfBounds) so the
function fails fast with a clear message, and ensure readingPosition is updated
only after the validated copy; apply this same guard to the second loop
mentioned at lines 223-235.
| if (!submission.approved) revert SubmissionNotApproved(); | ||
| if (block.timestamp - submission.approvalTime > executionTimeout) revert TimeToExecuteHasPassed(); | ||
| for (uint256 i = _cursor; i < submission.txs.length && (_count == 0 || i < _cursor + _count); i++) { | ||
| Transaction storage transaction = submission.txs[i]; | ||
| uint256 expendableFunds = getExpendableFunds(); | ||
| if (!transaction.executed && transaction.value <= expendableFunds) { | ||
| (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data); | ||
| // An extra check to prevent re-entrancy through target call. | ||
| if (callResult == true) { | ||
| require(!transaction.executed, "Already executed"); | ||
| if (transaction.executed) revert AlreadyExecuted(); | ||
| transaction.executed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate reentrancy window around external call to transaction.target
The state flag transaction.executed is set after the external call. A malicious target can re-enter executeTransactionList before this flag flips, triggering the same call path. You then detect that and revert, but this still leaves a reentrancy window and reverts the whole batch.
Prefer checks-effects-interactions or add a simple reentrancy guard for this function. If preserving “continue on failure” semantics is required, set executed = true before the call and revert on failure (so state reverts too), or introduce a lightweight nonReentrant modifier.
Minimal option while keeping current semantics (no revert on target failure):
- if (!transaction.executed && transaction.value <= expendableFunds) {
- (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data);
- // An extra check to prevent re-entrancy through target call.
- if (callResult == true) {
- if (transaction.executed) revert AlreadyExecuted();
- transaction.executed = true;
- }
- }
+ if (!transaction.executed && transaction.value <= expendableFunds) {
+ // Effects
+ transaction.executed = true;
+ // Interaction
+ (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data);
+ if (!callResult) {
+ // Restore effect if call failed to preserve current non-reverting behavior.
+ transaction.executed = false;
+ }
+ }Alternatively, add a nonReentrant guard to the function.
🤖 Prompt for AI Agents
contracts/src/arbitration/KlerosGovernor.sol lines 341-353: the code sets
transaction.executed after the external call, leaving a reentrancy window; fix
by applying checks-effects-interactions or a reentrancy guard—specifically, mark
transaction.executed = true before performing the external call to
transaction.target to prevent re-entrancy, then perform the call and keep the
executed flag set even if the call fails (this preserves the current "continue
on failure" semantics), or alternatively add a lightweight nonReentrant modifier
to the executeTransactionList function and keep the executed assignment after
the call.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol (2)
186-206: Align modifiers with PR’s custom-error pattern (gas + consistency)The PR migrates to custom errors; these modifiers still use require(). Switch to custom errors for clearer semantics and reduced bytecode.
Apply within this block:
modifier onlyDuringPhase(Phase _phase) { - require(phase == _phase); + if (phase != _phase) revert InvalidPhase(_phase, phase); _; } modifier onlyDuringPeriod(uint256 _disputeID, Period _period) { - require(disputes[_disputeID].period == _period); + if (disputes[_disputeID].period != _period) revert InvalidPeriod(_disputeID, _period, disputes[_disputeID].period); _; } modifier onlyByGovernor() { - require(governor == msg.sender); + if (governor != msg.sender) revert GovernorOnly(); _; }Add these error definitions near the top of the contract (after the enums to allow typed parameters):
error GovernorOnly(); error InvalidPhase(Phase expected, Phase actual); error InvalidPeriod(uint256 disputeID, Period expected, Period actual);If useful, I can prepare a follow-up patch converting other requires in this contract to typed errors.
484-485: Replace string reverts with a custom NotSupported() errorThese paths use revert("Not supported"); switch to a custom error to align with the migration and save gas.
Define the error once:
error NotSupported();Then update both call sites:
) external override returns (uint256) { - revert("Not supported"); + revert NotSupported(); }) public pure override returns (uint256 /*cost*/) { - revert("Not supported"); + revert NotSupported(); }Also applies to: 629-631
🧹 Nitpick comments (2)
contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol (2)
500-505: Avoid underflow in onTransfer to return false instead of revertingSubtracting before checking can revert underflow in 0.8.x. If the intention is to gracefully block transfers, pre-check the balance.
- uint256 newBalance = pinakion.balanceOf(_from) - _amount; + uint256 bal = pinakion.balanceOf(_from); + if (bal < _amount) return false; // Gracefully block insufficient balance. + uint256 newBalance = bal - _amount; if (newBalance < jurors[_from].stakedTokens || newBalance < jurors[_from].lockedTokens) return false;
140-145: Minor: simplify NON_PAYABLE_AMOUNT for readabilityEquivalent value, clearer intent, avoids exponentiation in constant expression.
- uint256 public constant NON_PAYABLE_AMOUNT = (2 ** 256 - 2) / 2; // An amount higher than the supply of ETH. + // 2**255 - 1; An amount higher than the supply of ETH. + uint256 public constant NON_PAYABLE_AMOUNT = type(uint256).max / 2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol(3 hunks)contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol(1 hunks)contracts/src/libraries/CappedMath.sol(0 hunks)
💤 Files with no reviewable changes (1)
- contracts/src/libraries/CappedMath.sol
⏰ 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). (3)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (2)
contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol (1)
12-12: Import path verified and correct
SortitionSumTreeFactory.solexists atcontracts/src/kleros-v1/libraries/SortitionSumTreeFactory.sol.- The only import in the
contracts/directory is incontracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.soland uses the updated relative path:- import {SortitionSumTreeFactory} from "../kleros-liquid/libraries/SortitionSumTreeFactory.sol"; + import {SortitionSumTreeFactory} from "../libraries/SortitionSumTreeFactory.sol";- No other occurrences of the old import path remain.
All good to merge.
contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol (1)
317-319: No remaining CappedMath references – subtraction logic is safeAll Solidity contracts and tests were searched for
CappedMath,capSub, and “saturating” references; none remain. The new ternary-based subtraction:uint256 requiredAmount = _moderation.paidFees[uint256(_side)] >= _totalRequired ? 0 : _totalRequired - _moderation.paidFees[uint256(_side)];correctly mirrors saturating behavior (returns zero instead of underflow). No further changes needed.
| uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | ||
| uint256 depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR; |
There was a problem hiding this comment.
Replace unchecked arithmetic with safe calculations or add overflow protection.
The direct arithmetic operations could potentially overflow, especially when arbitrationCost and multipliers are large values. The removal of CappedMath library eliminates the built-in overflow protection that was previously in place.
Consider adding explicit overflow checks or using SafeMath for these calculations:
- uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR;
- uint256 depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR;
+ uint256 totalCost;
+ uint256 depositRequired;
+
+ // Check for overflow in multiplication
+ require(arbitrationCost == 0 || totalCostMultiplier <= type(uint256).max / arbitrationCost, "Overflow in totalCost calculation");
+ totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR;
+
+ require(totalCost == 0 || initialDepositMultiplier <= type(uint256).max / totalCost, "Overflow in depositRequired calculation");
+ depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | |
| uint256 depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR; | |
| uint256 totalCost; | |
| uint256 depositRequired; | |
| // Check for overflow in multiplication | |
| require( | |
| arbitrationCost == 0 || | |
| totalCostMultiplier <= type(uint256).max / arbitrationCost, | |
| "Overflow in totalCost calculation" | |
| ); | |
| totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | |
| require( | |
| totalCost == 0 || | |
| initialDepositMultiplier <= type(uint256).max / totalCost, | |
| "Overflow in depositRequired calculation" | |
| ); | |
| depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR; |
| uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | ||
|
|
||
| uint256 opposition = 3 - uint256(_side); | ||
| uint256 depositRequired = moderation.paidFees[opposition] * 2; | ||
| if (depositRequired == 0) { | ||
| depositRequired = totalCost.mulCap(initialDepositMultiplier) / MULTIPLIER_DIVISOR; | ||
| depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR; |
There was a problem hiding this comment.
Apply consistent overflow protection for arithmetic operations.
Similar to the previous concern, these calculations need overflow protection since the CappedMath library was removed.
Add overflow checks for these calculations:
- uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR;
+ // Check for overflow in multiplication
+ require(arbitrationCost == 0 || totalCostMultiplier <= type(uint256).max / arbitrationCost, "Overflow in totalCost calculation");
+ uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR;
uint256 opposition = 3 - uint256(_side);
uint256 depositRequired = moderation.paidFees[opposition] * 2;
if (depositRequired == 0) {
- depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR;
+ require(totalCost == 0 || initialDepositMultiplier <= type(uint256).max / totalCost, "Overflow in depositRequired calculation");
+ depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | |
| uint256 opposition = 3 - uint256(_side); | |
| uint256 depositRequired = moderation.paidFees[opposition] * 2; | |
| if (depositRequired == 0) { | |
| depositRequired = totalCost.mulCap(initialDepositMultiplier) / MULTIPLIER_DIVISOR; | |
| depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR; | |
| // Check for overflow in multiplication | |
| require(arbitrationCost == 0 || totalCostMultiplier <= type(uint256).max / arbitrationCost, "Overflow in totalCost calculation"); | |
| uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | |
| uint256 opposition = 3 - uint256(_side); | |
| uint256 depositRequired = moderation.paidFees[opposition] * 2; | |
| if (depositRequired == 0) { | |
| require(totalCost == 0 || initialDepositMultiplier <= type(uint256).max / totalCost, "Overflow in depositRequired calculation"); | |
| depositRequired = (totalCost * initialDepositMultiplier) / MULTIPLIER_DIVISOR; |
🤖 Prompt for AI Agents
In contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol around lines
245 to 250, the arithmetic that used to rely on CappedMath
(multiplications/divisions and a "* 2") needs explicit overflow-safe operations
now that CappedMath was removed; add OpenZeppelin SafeMath usage (add "using
SafeMath for uint256" at the top of the contract and replace raw * and /
operations here with SafeMath calls) so compute totalCost, the doubled paidFees
and the initial deposit calculation via .mul(...).div(...) to ensure
overflow/underflow safety.
| uint256 totalCost = (arbitrationCost * totalCostMultiplier) / MULTIPLIER_DIVISOR; | ||
|
|
||
| uint256 opposition = 3 - uint256(_side); | ||
| uint256 depositRequired = moderation.paidFees[opposition] * 2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential overflow in opposition calculation and depositRequired multiplication.
The multiplication moderation.paidFees[opposition] * 2 could overflow if paidFees[opposition] is extremely large.
Add overflow protection for the doubling calculation:
uint256 opposition = 3 - uint256(_side);
- uint256 depositRequired = moderation.paidFees[opposition] * 2;
+ uint256 depositRequired;
+ require(moderation.paidFees[opposition] <= type(uint256).max / 2, "Overflow in depositRequired calculation");
+ depositRequired = moderation.paidFees[opposition] * 2;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 depositRequired = moderation.paidFees[opposition] * 2; | |
| uint256 opposition = 3 - uint256(_side); | |
| - uint256 depositRequired = moderation.paidFees[opposition] * 2; | |
| + uint256 depositRequired; | |
| + require( | |
| + moderation.paidFees[opposition] <= type(uint256).max / 2, | |
| + "Overflow in depositRequired calculation" | |
| + ); | |
| + depositRequired = moderation.paidFees[opposition] * 2; |
🤖 Prompt for AI Agents
contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol around line 248:
the multiplication `moderation.paidFees[opposition] * 2` can overflow; add an
explicit overflow guard before computing depositRequired by either using
SafeMath.mul or a require check such as require(moderation.paidFees[opposition]
<= type(uint256).max / 2, "deposit overflow") and then perform the
multiplication, or if the project uses Solidity >=0.8 rely on built-in checks
but still add a clear require/error message to document the constraint.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/CHANGELOG.md (1)
135-136: Remove unpublished 0.13.0 release link to avoid 404The 0.13.0 tag is marked “Not published yet,” and this link will 404 until it’s live. Comment it out and re-enable upon publishing.
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 +<!-- [0.13.0]: (add after publishing) -->If you’d like, I can provide a small script to check link availability before merging.
🧹 Nitpick comments (4)
contracts/CHANGELOG.md (2)
20-20: Tighten wording for clarity (Voting period rule)Improve readability and remove ambiguity.
-- Do not pass to Voting period if all the commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085)) +- Do not advance to the Voting period when all commits are already cast, as this breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085))
30-30: Use “info” instead of “infos”Minor grammar fix.
-- Add helper function `getDisputeKitsViem` to retrieve a deployment's available dispute kit infos including their capabilities (`isShutter`, `isGated`) ([`5a81f9e`](https://github.com/kleros/kleros-v2/commit/5a81f9ec)) +- Add helper function `getDisputeKitsViem` to retrieve a deployment's available dispute kit info, including their capabilities (`isShutter`, `isGated`) ([`5a81f9e`](https://github.com/kleros/kleros-v2/commit/5a81f9ec))contracts/src/arbitration/KlerosCoreBase.sol (1)
1179-1181:⚠️ RenamingInvalidDisputKitParentIs a Breaking Change
The misspelled error selector appears throughout the codebase’s ABIs and deployment artifacts. Renaming it will require updating all of the following (and likely more) to avoid runtime and tooling breakages:• contracts/src/arbitration/KlerosCoreBase.sol (line 1179)
• contracts/src/arbitration/university/KlerosCoreUniversity.sol (line 1156)
• All ABI-migration JSONs under subgraph/core-neo/abi-migrations and subgraph/core/abi-migrations
• contracts/deployments/.{viem.ts,.json} (Testnet, Mainnet, Devnet, ArbitrumSepoliaDevnet, etc.)
• Any subgraphs, front-end code, or integrations relying on the current selector nameIf you still want to correct the typo, update every reference accordingly and regenerate all ABIs/deployments. Otherwise, it’s safer to leave the existing name until the next major version bump.
contracts/test/foundry/KlerosCore.t.sol (1)
1568-1569: Commit-period tests updated to typed errors and explicit time warps — correct with new policy
- Line 1568: NotCommitPeriod is asserted before entering commit — correct.
- Lines 1571–1578: Evidence period must elapse; test now warps accordingly — matches KlerosCoreBase change.
- Lines 1585, 1591: EmptyCommit and ownership checks verified via selectors — good coverage.
- Lines 1626, 1631, 1634: Vote-phase transition and reveal mismatch checks are now asserted by selector — correct.
These updates align with the new time-gated commit policy and custom-error migration.
You can reduce repetition by extracting small helpers, e.g., warpToPeriod(Period) and createAndDraw(disputeID, nbJurors), to streamline many tests that follow identical staging patterns.
Also applies to: 1571-1573, 1585-1587, 1591-1593, 1626-1627, 1631-1632, 1634-1636
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
contracts/CHANGELOG.md(2 hunks)contracts/src/arbitration/KlerosCoreBase.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(16 hunks)
⏰ 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). (17)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- 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: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
🔇 Additional comments (12)
contracts/src/arbitration/KlerosCoreBase.sol (1)
569-573: Commit→Vote is now purely time-gated — good call to avoid Shutter auto-reveal breakageRemoving the areCommitsAllCast fast-path and relying only on the elapsed commit period clarifies behavior and avoids early transitions that could interfere with auto-reveal flows. Tests appear adapted to time progression.
- Side-effect to acknowledge: early progression to vote is no longer possible even if all commits are in; off-chain tooling/UI must respect the full commit period.
If any scripts or dapps previously triggered passPeriod early upon full commit, ensure they now wait out the commit period. Do you want me to scan the repo for any helper scripts that call passPeriod during commit to suggest updates?
contracts/test/foundry/KlerosCore.t.sol (11)
1140-1141: Typed errors for delayed-stake execution checks — solid migrationAsserting SortitionModuleBase.NoDelayedStakeToExecute and NotStakingPhase by selector improves brittleness and bytecode size over string reason checks. The scenarios covered are correct.
Also applies to: 1153-1155
1273-1274: Snapshot proxy GovernorOnly checks switched to error selectors — LGTMThe negative access tests now validate KlerosCoreSnapshotProxy.GovernorOnly via selector. Good parity with the production change to custom errors.
Also applies to: 1280-1281
1676-1683: Commit timeout gate now enforced with CommitPeriodNotPassed — test reflects the new behaviorGood verification that early pass from commit to vote is disallowed until the full commit period elapses.
1703-1704: Vote-period negative paths switched to custom errors — comprehensive coverage
- NotVotePeriod, EmptyVoteIDs, ChoiceOutOfBounds, JurorHasToOwnTheVote, and VoteAlreadyCast are now asserted via selectors.
- Scenarios remain accurate and thorough.
Also applies to: 1721-1722, 1727-1728, 1731-1732, 1739-1741
1905-1909: Quick pass to appeal when all reveals (for committed votes) are done — test matches passPeriod logicThis validates the early Vote→Appeal transition path using areVotesAllCast while commit remains time-gated. Good separation of concerns in tests.
1975-1976: Appeal funding error-paths moved to typed errors — correct
- ChoiceOutOfBounds for invalid side.
- AppealFeeIsAlreadyPaid once a side is funded.
Selectors are used consistently with contract changes.Also applies to: 1999-2001
2028-2030: Appeal-period timing checks updated to typed errors — robust
- AppealPeriodIsOver and AppealPeriodIsOverForLoser paths covered at mid and end intervals.
- Matches the refined revert policy.
Also applies to: 2035-2037, 2043-2045
2214-2216: Prevent operations on the original DK after a parent-DK jump — correct guardAsserting DisputeKitClassicBase.DisputeJumpedToParentDK on draw protects against stale DK usage post-jump. Good regression coverage.
2598-2600: Leftover PNK withdrawal eligibility uses typed error — accurateAsserting NotEligibleForWithdrawal before execute finishes unlocks is the right invariant.
2868-2869: Withdrawal gating by dispute resolution and pause state — selectors verify both constraints
- DisputeNotResolved before ruling execution.
- CoreIsPaused prevents withdrawal while paused.
Good enforcement of lifecycle and pause semantics.Also applies to: 2875-2876
2973-2975: Casting on the wrong DK is blocked — selector-based assertion is correctNotActiveForCoreDisputeID ensures calls are routed to the DK that owns the dispute after DK changes. Strong safety check.
chore: label text improvements
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
web/src/components/DisputeView/CardLabels/index.tsx (3)
73-85: Monetary math bug: converting Number → BigInt loses precision; sum in wei using BigInt
Number(val.amount)andNumber(val.rewardAmount)can overflow and lose precision before converting back to BigInt, leading to incorrect ETH values shown to users. Do the accumulation entirely in wei (BigInt), then format.Apply:
-const getFundingRewards = (contributions: ClassicContribution[], closed: boolean) => { - if (isUndefined(contributions) || contributions.length === 0) return 0; - const contribution = contributions.reduce((acc, val) => { - if (isUndefined(val?.rewardAmount) && isUndefined(val?.amount)) return acc; - if (closed) { - acc += val.rewardAmount === null ? -1 * Number(val.amount) : Number(val.rewardAmount) - Number(val.amount); - } else { - acc += Number(val.amount); - } - return acc; - }, 0); - return Number(formatUnits(BigInt(contribution), 18)); -}; +const getFundingRewards = (contributions: ClassicContribution[] | undefined, closed: boolean) => { + if (isUndefined(contributions) || contributions.length === 0) return 0; + const totalWei = contributions.reduce<bigint>((acc, val) => { + const amountWei = isUndefined(val?.amount) ? 0n : BigInt(val.amount); + const rewardWei = + isUndefined(val?.rewardAmount) || val.rewardAmount === null ? null : BigInt(val.rewardAmount); + if (closed) { + return acc + (rewardWei === null ? -amountWei : rewardWei - amountWei); + } + return acc + amountWei; + }, 0n); + return Number(formatUnits(totalWei, 18)); +};
103-111: Guard against undefined and simplify flattening of contributionsIf
localRoundsis undefined, the current expression returns undefined instead of[]. Also,val.contributionsmay be undefined. UseflatMapwith nullish coalescing.Apply:
- const contributions = useMemo( - () => - localRounds?.reduce((acc, val) => { - acc.push(...val.contributions); - return acc; - }, []), - [localRounds] - ); + const contributions = useMemo( + () => localRounds?.flatMap((val) => val?.contributions ?? []) ?? [], + [localRounds] + );
95-100: Ensure safe optional chaining for array lengths
Directly accessing.lengthafter optional chaining can throw at runtime when the value isundefined. Replace all unsafe length checks with a null-coalescing fallback.Locations needing fixes:
• web/src/components/DisputeView/CardLabels/index.tsx
– Line 96:currentRound?.drawnJurors.length
– Line 98:item?.drawnJurors.length
– Line 99:localRounds?.[round]?.contributions.length
• web/src/pages/Cases/CaseDetails/Evidence/index.tsx
– Line 133:evidences?.spamEvidences.lengthProposed refactor:
--- a/web/src/components/DisputeView/CardLabels/index.tsx +++ b/web/src/components/DisputeView/CardLabels/index.tsx @@ -95,7 +95,7 @@ - const isDrawnCurrentRound = currentRound?.drawnJurors.length !== 0; + const isDrawnCurrentRound = (currentRound?.drawnJurors?.length ?? 0) !== 0; @@ -97,7 +97,7 @@ - const isDrawnInDispute = rounds?.some((item) => item?.drawnJurors.length); + const isDrawnInDispute = (rounds?.some((item) => (item?.drawnJurors?.length ?? 0) > 0) ?? false); @@ -98,7 +98,7 @@ - const hasFundedCurrentRound = localRounds?.[round]?.contributions.length !== 0; + const hasFundedCurrentRound = (localRounds?.[round]?.contributions?.length ?? 0) !== 0;--- a/web/src/pages/Cases/CaseDetails/Evidence/index.tsx +++ b/web/src/pages/Cases/CaseDetails/Evidence/index.tsx @@ -131,7 +131,7 @@ - {spamEvidences && evidences?.spamEvidences.length !== 0 ? ( + {spamEvidences && (evidences?.spamEvidences?.length ?? 0) !== 0 ? ( <> <Divider /> {/* ... */}
🧹 Nitpick comments (1)
web/src/components/DisputeView/CardLabels/index.tsx (1)
159-162: Nit: remove stray whitespace nodeThe standalone {" "} adds an extra text node and is unnecessary.
Apply:
- <RewardsContainer> - {" "} + <RewardsContainer>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
web/src/components/DisputeView/CardLabels/index.tsx(1 hunks)
⏰ 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). (6)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: Scorecards analysis
🔇 Additional comments (1)
web/src/components/DisputeView/CardLabels/index.tsx (1)
64-71: Copy updates improve clarity and user-centric tone — LGTMThe new phrasing (“You were not drawn”, “You can vote now”, etc.) is clearer and more actionable. Icons and colors remain consistent.
fix(web): timeline-bug-fix
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
106-142: Fix subitems countdown misalignment when Commit is hiddenWhen hiddenVotes is false and the on-chain period is Commit (Periods.commit), the Commit step is omitted from the UI but
getSubitemsstill compares againstcurrentPeriodIndexfrom the raw timeline. That makes the highlighted “Voting” step show a static per-period duration instead of the current countdown (or “Time’s up!”). The root cause is using raw indices fromtitlesafter filtering out Commit.Refactor to compute a visible titles array and compare against a display index aligned with the filtered list. Also make “Time’s up!” robust for negative values and parse
timesPerPeriodstrings before formatting.Apply this diff:
-const useTimeline = (dispute: DisputeDetailsQuery["dispute"], currentPeriodIndex: number) => { - const isDesktop = useIsDesktop(); - const titles = ["Evidence", "Commit", "Voting", "Appeal", "Executed"]; - - const deadlineCurrentPeriod = getDeadline( - currentPeriodIndex, - dispute?.lastPeriodChange, - dispute?.court.timesPerPeriod - ); - - const countdown = useCountdown(deadlineCurrentPeriod); - const getSubitems = (index: number): string[] | React.ReactNode[] => { - if (typeof countdown !== "undefined" && dispute) { - if (index === titles.length - 1) { - return []; - } else if (index === currentPeriodIndex && countdown === 0) { - return ["Time's up!"]; - } else if (index < currentPeriodIndex) { - return []; - } else if (index === currentPeriodIndex) { - return [secondsToDayHourMinute(countdown)]; - } else { - return [secondsToDayHourMinute(dispute?.court.timesPerPeriod[index])]; - } - } - return [<StyledSkeleton key={index} width={60} />]; - }; - return titles.flatMap((title, i) => { - // if not hidden votes, skip commit index - if (!dispute?.court.hiddenVotes && i === Periods.commit) return []; - return [ - { - title: i + 1 < titles.length && isDesktop ? `${title} Period` : title, - subitems: getSubitems(i), - }, - ]; - }); -}; +const useTimeline = (dispute: DisputeDetailsQuery["dispute"], currentPeriodIndex: number) => { + const isDesktop = useIsDesktop(); + const allTitles = ["Evidence", "Commit", "Voting", "Appeal", "Executed"]; + const hiddenVotes = Boolean(dispute?.court.hiddenVotes); + // Filter Commit out of the visible timeline when hidden votes are disabled. + const visibleTitles = hiddenVotes ? allTitles : allTitles.filter((_, i) => i !== Periods.commit); + // Index of the current item in the visible timeline (aligned with visibleTitles). + const displayCurrentIndex = currentPeriodToCurrentItem(currentPeriodIndex, hiddenVotes); + + const deadlineCurrentPeriod = getDeadline( + currentPeriodIndex, + dispute?.lastPeriodChange, + dispute?.court.timesPerPeriod + ); + + const countdown = useCountdown(deadlineCurrentPeriod); + + // Map a visible index back to the raw period index (for timesPerPeriod). + const toRawIndex = (i: number) => (hiddenVotes ? i : i >= Periods.commit ? i + 1 : i); + + const getSubitems = (visibleIndex: number): string[] | React.ReactNode[] => { + if (typeof countdown !== "undefined" && dispute) { + // Last item ("Executed") shows no subitems. + if (visibleIndex === visibleTitles.length - 1) { + return []; + } + if (visibleIndex === displayCurrentIndex && countdown <= 0) { + return ["Time's up!"]; + } + if (visibleIndex < displayCurrentIndex) { + return []; + } + if (visibleIndex === displayCurrentIndex) { + return [secondsToDayHourMinute(Math.max(0, countdown))]; + } + // Future periods: show configured duration for that period. + const rawIndex = toRawIndex(visibleIndex); + const perPeriod = dispute?.court.timesPerPeriod?.[rawIndex]; + return [secondsToDayHourMinute(perPeriod ? parseInt(perPeriod, 10) : 0)]; + } + return [<StyledSkeleton key={visibleIndex} width={60} />]; + }; + + return visibleTitles.map((title, visibleIndex) => ({ + title: visibleIndex + 1 < visibleTitles.length && isDesktop ? `${title} Period` : title, + subitems: getSubitems(visibleIndex), + })); +};Why this helps:
- The visible list and the display index stay aligned when Commit is omitted.
- Subitems correctly show countdown (or “Time’s up!”) for the highlighted step.
- Per-period durations are parsed to numbers before formatting (avoids type issues).
- Time’s up uses
<= 0and clamps negative countdowns for a cleaner UX.If you prefer a smaller change, I can generate a minimal patch that preserves
flatMapbut corrects the index comparisons only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
web/src/pages/Cases/CaseDetails/Timeline.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/src/pages/Cases/CaseDetails/Timeline.tsx (2)
web/src/utils/date.ts (1)
secondsToDayHourMinute(10-16)web/src/components/StyledSkeleton.tsx (1)
StyledSkeleton(8-10)
⏰ 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). (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: Scorecards analysis
🔇 Additional comments (1)
web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
71-71: AlluseTimelinecalls updated to the new signatureI ran a repo-wide search for any three-argument invocations of
useTimelineand found none. The only call site atweb/src/pages/Cases/CaseDetails/Timeline.tsx:71already uses the updated(dispute, currentPeriodIndex)signature—no legacy usages remain.
Contracts: multi-dimensional coherence
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
742-744: Consider a non-reentrancy guard around execute()execute() performs multiple external interactions (token transfers, ETH sends, SortitionModule calls) before final storage writes; the comment accurately notes CEI is not strictly followed. Add a nonReentrant guard to the function or refactor to isolate effects before interactions to reduce reentrancy surface. Given the number of external calls, a guard is the simplest robust mitigation.
♻️ Duplicate comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
138-140: Add active-dispute guard in notJumped modifier (prevents accidental reads of dispute 0)Accessing disputes[coreDisputeIDToLocal[_coreDisputeID]] without first verifying coreDisputeIDToActive can read local dispute 0 for unknown IDs and rely on later checks. Guard early to fail fast.
Apply this diff:
modifier notJumped(uint256 _coreDisputeID) { - if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }
🧹 Nitpick comments (4)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
783-783: Event coherence parameter: confirm intended meaningYou now emit the computed coherence value to TokenAndETHShift. Confirm downstream indexers/analytics expect this to represent “coherence (basis points)” rather than a legacy name (degreeOfCoherency). Names aren’t part of ABI, but aligning docs/specs avoids confusion.
Also applies to: 860-860
1159-1159: Typo in error name: InvalidDisputKitParentMinor naming nit: consider renaming to InvalidDisputeKitParent for clarity and consistency.
Apply this diff:
- error InvalidDisputKitParent(); + error InvalidDisputeKitParent();contracts/test/foundry/KlerosCore.t.sol (1)
1551-1561: Reduce duplication with small test helpersYou repeatedly advance periods (warp + passPhase/passPeriod) and set up disputes. Consider helper functions for:
- createAndDrawDispute(...)
- advanceToCommit/vote/appeal/execution(...)
This will shrink boilerplate and make scenarios easier to compose.Also applies to: 1677-1683, 1895-1900, 2052-2058, 2238-2244
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
480-491: NatSpec param ordering is misleading (nit)hashVote signature is (choice, salt, justification), but the docs list justification before salt. Swap the param docs to match the signature.
Apply this diff:
- * @param _justification The justification for the vote - * @param _salt A random salt for commitment + * @param _salt A random salt for commitment + * @param _justification The justification for the vote
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
contracts/src/arbitration/KlerosCoreBase.sol(5 hunks)contracts/src/arbitration/SortitionModuleBase.sol(7 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(10 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(2 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(7 hunks)contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol(1 hunks)contracts/src/libraries/Constants.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(18 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.solcontracts/src/arbitration/university/KlerosCoreUniversity.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/SortitionModuleBase.sol
⏰ 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). (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (32)
contracts/src/libraries/Constants.sol (1)
23-24: LGTM! Clean introduction of a standardized basis point constant.The addition of
ONE_BASIS_POINTconstant with value 10000 provides a clear, reusable denominator for basis point calculations across the codebase. This improves consistency and readability compared to scattered magic numbers or individualALPHA_DIVISORconstants.contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol (1)
12-12: Import path verified:SortitionSumTreeFactory.solexists incontracts/src/kleros-v1/libraries/and has been removed from the old location. No further changes needed.contracts/src/arbitration/interfaces/IDisputeKit.sol (3)
3-3: Pragma range broadened appropriately for library compatibility.The pragma was relaxed from
^0.8.24to>=0.8.0 <0.9.0, providing better compatibility across different Solidity versions while maintaining the v0.8.x feature set.
64-78: Well-structured API separation for reward vs penalty coherence calculations.The split of the original
getDegreeOfCoherencefunction into separate reward and penalty functions improves API clarity:
getDegreeOfCoherenceRewardreturns both PNK and fee coherence valuesgetDegreeOfCoherencePenaltyreturns only PNK coherence for penalty calculationsThe documentation clearly explains the different return values and use cases.
80-93: Good separation of penalty-specific coherence logic.The dedicated
getDegreeOfCoherencePenaltyfunction provides a clean interface for penalty calculations, returning only the PNK coherence value needed for that specific use case.contracts/src/arbitration/SortitionModuleBase.sol (8)
124-131: Improved error handling with custom errors.The replacement of string-based
requirestatements with custom errors (GovernorOnly(),KlerosCoreOnly()) provides better gas efficiency and clearer error identification. The logic remains identical while improving the developer experience.
174-186: Enhanced phase transition validation with descriptive errors.The phase transition logic now uses custom errors that clearly describe each failure condition:
MinStakingTimeNotPassed()- Time-based guardNoDisputesThatNeedJurors()- State-based guardRandomNumberNotReady()- External dependency guardDisputesWithoutJurorsAndMaxDrawingTimeNotPassed()- Combined condition guardThis improves debuggability while maintaining the same validation logic.
200-201: Clear validation with descriptive custom errors.The tree creation validation uses appropriate custom errors:
TreeAlreadyExists()for duplicate creation attemptsKMustBeGreaterThanOne()for invalid tree parameters
209-210: Proper phase and state validation for delayed stake execution.The function correctly validates both the current phase (
NotStakingPhase()) and the availability of work (NoDelayedStakeToExecute()) before proceeding with delayed stake execution.
419-419: Consistent error handling for withdrawal eligibility.The
NotEligibleForWithdrawal()error provides clear feedback when a juror attempts to withdraw PNK but doesn't meet the eligibility criteria.
443-443: Appropriate phase validation for drawing operations.The
NotDrawingPhase()error ensures that drawing operations can only occur during the correct phase, maintaining the state machine integrity.
692-708: Well-organized custom error definitions.The comprehensive set of custom errors at the end of the contract provides:
- Clear naming conventions
- Appropriate granularity for different failure scenarios
- Complete coverage of all validation conditions used throughout the contract
This follows Solidity best practices for error handling.
347-355: No mismatch incurrentCourtIDwhen callingcore.courtsThe auto-generated getter for the
courtsmapping uses auint256key. In particular:
- contracts/src/test/KlerosCoreMock.sol:
function getCourtChildren(uint256 _courtId) { children = courts[_courtId].children; }This shows the mapping key is
uint256, and Solidity will implicitly upcast auint96touint256. You can safely remove the type-consistency check.contracts/src/arbitration/KlerosCoreBase.sol (7)
568-572: Clarification comment improves understanding of commit period logic.The added comment explains that the commit period advancement is intentionally time-gated rather than completion-gated to maintain compatibility with Shutter auto-reveal functionality. This is a valuable clarification for future developers.
768-779: Robust penalty calculation with coherence capping.The penalty calculation logic correctly:
- Uses the new
getDegreeOfCoherencePenaltyAPI- Implements a safety guard against coherence values exceeding 100% (
ONE_BASIS_POINT)- Applies the penalty using the standardized basis point calculation
The coherence capping is a defensive programming practice that prevents unexpected behavior from dispute kit implementations.
782-782: Correct penalty calculation using standardized constant.The penalty calculation
(round.pnkAtStakePerJuror * (ONE_BASIS_POINT - coherence)) / ONE_BASIS_POINTproperly uses the new standardized constant instead of the oldALPHA_DIVISOR, ensuring consistency across the codebase.
827-841: Well-structured reward calculation with dual coherence values.The reward calculation logic correctly handles the new API that returns separate coherence values for PNK and fee rewards:
- Destructures the tuple from
getDegreeOfCoherenceReward- Applies safety guards to both coherence values
- Uses appropriate coherence values for different reward types
This separation allows for more nuanced reward calculations while maintaining safety bounds.
844-853: Consistent application of coherence to different reward types.The code correctly applies:
pnkCoherencefor PNK-related calculations (locked amount and PNK rewards)feeCoherencefor fee reward calculationsThis distinction allows for different coherence models for different reward types, providing flexibility in the dispute resolution mechanism.
1063-1067: Clean helper function with updated documentation.The
_applyCoherencefunction is correctly updated to useONE_BASIS_POINTwith appropriate parameter documentation. The function signature and implementation are clear and consistent with the new coherence calculation standard.
1074-1075: Standardized PNK calculation using unified constant.The
_calculatePnkAtStakefunction correctly usesONE_BASIS_POINTinstead of the oldALPHA_DIVISOR, maintaining consistency with the codebase-wide standardization effort.contracts/src/arbitration/university/KlerosCoreUniversity.sol (6)
11-11: Constants import aligns with new basis-point semanticsImporting Constants.sol is appropriate given the use of ONE_BASIS_POINT throughout the file.
528-528: Basis-point staking math: LGTM (double-check alpha scale across callers)Switching pnkAtStakePerJuror to (minStake * alpha) / ONE_BASIS_POINT correctly aligns with basis-point semantics. As a sanity check, ensure that all alpha inputs across court governance are expressed in the same ONE_BASIS_POINT scale (e.g., 10_000 == 100%).
Also applies to: 657-657
756-767: Defensive clamp on coherence from DK is fineClamping coherence to ONE_BASIS_POINT guards the core from DK misbehavior. It's a minor gas cost and acceptable for safety.
770-770: Penalty formula matches intended semanticsUsing penalty = pnkAtStakePerJuror * (1 - coherence) / ONE_BASIS_POINT correctly yields 0 penalty for fully coherent jurors and full penalty when coherence is 0.
821-835: Reward coherence clamping mirrors penalty side — goodClamping both pnkCoherence and feeCoherence mirrors the penalty path and prevents accidental over-rewarding.
837-837: Reward distribution math is sound
- Unlocking pnkLocked = pnkAtStakePerJuror * pnkCoherence / ONE_BASIS_POINT releases the coherent portion of locked PNK.
- pnkReward and feeReward prorate by coherence, which allows partial coherence without breaking ties.
Net unlock across penalty+reward passes sums to the originally locked stake (as expected).
Also applies to: 843-847
contracts/test/foundry/KlerosCore.t.sol (2)
1140-1141: Migration to custom error selectors: LGTMReplacing string-based revert reasons with error selectors improves gas and consistency across the suite. The covered assertions map cleanly to the new error surface.
Also applies to: 1153-1155, 1273-1274, 1280-1281, 1568-1569, 1626-1627, 1631-1632, 1634-1635, 1703-1704, 1721-1722, 1727-1728, 1731-1732, 1975-1976, 2028-2029, 2036-2037, 2043-2044
2316-2341: New coherence tests validate reward/penalty paths wellGood additions validating:
- getDegreeOfCoherenceReward returns (pnk, fee) coherences in basis points.
- getDegreeOfCoherencePenalty aligns with the reward side and zero-coherence scenarios.
These tests exercise the new DK API and the basis-point arithmetic effectively.Also applies to: 2426-2444
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
9-9: ONE_BASIS_POINT import centralizes basis-point arithmeticGood move to reuse the shared constant across DK logic.
272-275: Commit/vote validation flow: LGTM
- Period checks moved to custom errors.
- Empty commit/voteIDs and DK activity checks are explicit.
- Juror ownership and hidden-vote hash validation are correct.
- Choice bounds check permits 0 as “refuse to arbitrate”, matching the contract’s stated model.
Also applies to: 314-316, 319-319, 328-332
362-367: Appeal funding window and paid-state checks: correct and explicit
- AppealPeriodIsOver vs. AppealPeriodIsOverForLoser timing split is consistent with LOSER_APPEAL_PERIOD_MULTIPLIER.
- AppealFeeIsAlreadyPaid prevents double-funding for a choice.
Also applies to: 374-378, 385-385
537-541: Coherence API split (reward/penalty) and internal computation: LGTM
- getDegreeOfCoherenceReward returns identical pnk/fee coherences for classic; extensible for DK variants.
- Penalty coherence exposed separately; internal _getDegreeOfCoherence returns 0/ONE_BASIS_POINT as expected based on voted/tie/winner.
Also applies to: 547-556, 557-573
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/CHANGELOG.md (1)
137-137: Remove unpublished release link for 0.13.0 to avoid 404.You note “Not published yet” in the header; the link currently points to a tag that likely doesn’t exist.
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 +<!-- [0.13.0]: (add after publishing) -->
🧹 Nitpick comments (1)
contracts/CHANGELOG.md (1)
22-22: Minor grammar: drop “the”.Smoother phrasing in changelog prose.
- Do not pass to Voting period if all the commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085)) + Do not pass to the Voting period if all commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contracts/CHANGELOG.md(2 hunks)
⏰ 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). (5)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: contracts-testing
- GitHub Check: contracts-testing
🔇 Additional comments (1)
contracts/CHANGELOG.md (1)
14-14: Clarify CHANGELOG entry for widened pragma scopeTo accurately reflect the files updated, please update the entry in contracts/CHANGELOG.md to:
- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + Widen the allowed pragma to `>=0.8.0 <0.9.0` for interfaces and RNG contracts (`RNG.sol`, `IRandomizer.sol`) ([#2083](https://github.com/kleros/kleros-v2/issues/2083))optional_refactors_recommended
contracts/CHANGELOG.md
Outdated
| - Set the Foundry Solidity version to v0.8.30 and enable the IR pipeline ([#2073](https://github.com/kleros/kleros-v2/issues/2073)) | ||
| - Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) | ||
| - Make `IDisputeKit.getDegreeOfCoherenceReward()` multi-dimensional so different calculations may be applied to PNK rewards, fee rewards and PNK penalties (future-proofing) ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) | ||
| - Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) |
There was a problem hiding this comment.
Fix constant name and phrasing (typo).
The constant is ONE_BASIS_POINT (singular), not ONE_BASIS_POINTS. Also “consolidate with” is imprecise here.
Apply:
- Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))
+ Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) | |
| - Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) |
🤖 Prompt for AI Agents
In contracts/CHANGELOG.md around line 16, the changelog entry uses the wrong
constant name and imprecise phrasing; change "ONE_BASIS_POINTS" to the correct
constant name "ONE_BASIS_POINT" (singular) and replace "Consolidate the constant
... with" with clearer wording such as "Consolidate the constant ALPHA_DIVISOR
into ONE_BASIS_POINT" or "Replace ALPHA_DIVISOR with ONE_BASIS_POINT" to
accurately reflect the change.
Fix decode to allow '-' characters in justification otherwise justification with '-' will spit an error
Update keeperBotShutter.ts: Allow - characters in justification field when decoding messages
fix: react-vuln-55183-and-55184
Devnet 2.0-rc Deployment
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry.json (1)
2-66: Potential duplication withDisputeTemplateRegistry_Proxy.json(same address/tx).
In the provided content, this file andcontracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry_Proxy.jsonappear to carry the same deployed proxyaddressandtransactionHash, plus proxy-style ABI/constructor. If both are meant to exist, consider clarifying via naming (or removing one) to avoid tooling/users picking the wrong “canonical” artifact.Also applies to: 258-303
contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry_Proxy.json (1)
2-66: Confirm whether this is intentionally identical toDisputeTemplateRegistry.json.
As shown, this looks like the same proxy deployment metadata. If it’s an intentional alias, consider keeping only one (or generating one from the other) to reduce drift risk.contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverUniversity.json (1)
20-69: Event shape change requires critical consumer updates (DisputeRequest).
DisputeRequestevent now emits only(_arbitrator, _arbitratorDisputeID, _templateId). Test code and bot scripts still expect the removed fields_externalDisputeIDand_templateUri:
contracts/test/evidence/index.ts:264— test destructures 4 parameters from event (will fail)contracts/scripts/disputeRelayerBot.ts:84-86— references_externalDisputeIDand_templateUri(will be undefined)Update consumers to match the new event signature before deployment.
🧹 Nitpick comments (2)
contracts/README.md (1)
85-86: Document the University contract variants.New University-specific contract variants have been introduced (DisputeKitClassicUniversity, DisputeTemplateRegistryUniversity, KlerosCoreUniversity, SortitionModuleUniversity). These represent a significant addition to the deployment architecture. Ensure that:
- The purpose and use case of these University variants is documented (either in this README or in a linked document)
- Deployment scripts and configuration files properly distinguish between standard and University variants
- Related documentation is updated to clarify when users should deploy standard vs. University versions
Consider adding a brief explanatory section or link explaining the difference between standard and University contract variants to improve clarity for developers using this README.
Also applies to: 92-93, 96-97, 103-103
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity_Proxy.json (1)
2-66: Keep “notice” metadata consistent across DK proxy artifacts.
This artifact includesuserdoc.notice(Line 75) while other DK proxy artifacts shown don’t. If that’s intentional, fine; otherwise consider regenerating artifacts consistently (same compiler/settings) so diffs don’t include avoidable doc churn across similar proxy wrappers.Also applies to: 75-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.yarn/releases/yarn-4.10.3.cjsis excluded by!**/.yarn/**.yarn/versions/7fe4657f.ymlis excluded by!**/.yarn/**contracts/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.yarnrc.yml(1 hunks)contracts/README.md(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/ArbitrableExample.json(0 hunks)contracts/deployments/arbitrumSepoliaDevnet/BlockHashRNG.json(0 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json(20 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.json(22 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity_Proxy.json(3 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic_Proxy.json(2 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.json(22 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.json(23 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter_Proxy.json(2 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated_Proxy.json(2 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json(23 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter_Proxy.json(2 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeResolver.json(10 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverRuler.json(0 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverUniversity.json(10 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry.json(5 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistryUniversity.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistryUniversity_Implementation.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistryUniversity_Proxy.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry_Implementation.json(7 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry_Proxy.json(2 hunks)contracts/deployments/arbitrumSepoliaDevnet/EvidenceModule.json(7 hunks)
💤 Files with no reviewable changes (3)
- contracts/deployments/arbitrumSepoliaDevnet/ArbitrableExample.json
- contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverRuler.json
- contracts/deployments/arbitrumSepoliaDevnet/BlockHashRNG.json
✅ Files skipped from review due to trivial changes (1)
- contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistryUniversity_Proxy.json
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistryUniversity.json
- contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistryUniversity_Implementation.json
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-23T17:47:39.947Z
Learnt from: kemuru
Repo: kleros/kleros-v2 PR: 1994
File: web/vite.config.js:26-33
Timestamp: 2025-05-23T17:47:39.947Z
Learning: In Yarn workspace setups, dependencies defined in individual workspace package.json files (like web/package.json) are typically hoisted to the root node_modules directory. This means vite config paths should point to "../node_modules" from workspace directories to access hoisted dependencies, not to local workspace node_modules.
Applied to files:
.yarnrc.yml
📚 Learning: 2024-10-21T10:32:16.970Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Applied to files:
contracts/deployments/arbitrumSepoliaDevnet/EvidenceModule.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
Applied to files:
contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverUniversity.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeResolver.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json
📚 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:
contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverUniversity.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeResolver.json
📚 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:
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.json
📚 Learning: 2024-11-19T17:18:39.007Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Applied to files:
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.json
📚 Learning: 2025-01-22T00:55:35.842Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1805
File: contracts/src/proxy/UUPSProxiable.sol:127-129
Timestamp: 2025-01-22T00:55:35.842Z
Learning: In Solidity, a public state variable automatically implements a corresponding external view function with the same name. For example, `string public constant override version = "0.8.0"` implements the abstract `function version() external view returns (string memory)`.
Applied to files:
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json
⏰ 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). (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (21)
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter_Proxy.json (1)
2-2: Deployment artifact updated with new compiler configuration and redeployment data.This JSON artifact reflects a redeployment of the DisputeKitGatedShutter proxy with updated compiler settings (solc 0.8.30, Cancun EVM, viaIR enabled, optimizer runs increased to 850). The structure and formatting are correct. Note that this artifact is specific to arbitrumSepoliaDevnet; equivalent updates should be applied to other network deployments as part of the release process.
Also applies to: 29-29, 33-66
.yarnrc.yml (1)
15-15: Yarn release file is present and properly configured. No further action needed for this change.The new Yarn 4.10.3 release file exists at
.yarn/releases/yarn-4.10.3.cjsand is correctly referenced in.yarnrc.yml. No stale references to the previous version remain in the repository.contracts/deployments/arbitrumSepoliaDevnet/EvidenceModule.json (4)
38-42: ✓ OwnerOnly error addition aligns with governor→owner refactoring.The new error definition reflects the intended shift to owner-based access control patterns across the codebase.
114-114: ✓ Governor-to-owner refactoring correctly reflected in ABI.The
initializeparameter renamed from_governorto_owner(line 114) and the newowner()view function (lines 125–134) are consistent with the contract's shift to owner-based governance. These changes are standard for ERC-1967 upgradeable proxy patterns.Also applies to: 125-134
215-259: ✓ Deployment artifacts correctly updated for new contract build.The transaction hash, block metadata, receipt details, bytecode, compiler hash, and implementation address have all been updated to reflect a fresh deployment. These changes are expected and correct when contract source code is modified.
65-65: Parameter rename is consistent with source code and requires no further action.The source file (EvidenceModule.sol) and interface (IEvidence.sol) already use
_arbitratorDisputeIDconsistently in both thesubmitEvidencefunction (line 63) and theEvidenceevent (line 11 in IEvidence.sol). The arbitrumSepoliaDevnet deployment file correctly reflects this naming. Downstream code references were not found; the parameter name is internal to the event ABI and does not affect external callers.contracts/README.md (3)
77-77: Clarify the stability/status of V2 Devnet label.The new heading "V2 Devnet (unstable)" is clearer than the previous "Devnet", but verify this aligns with the project's communication strategy for dev deployments. If the devnet is expected to reset or be subject to breaking changes, this label is appropriate.
101-101: RNGWithFallback is documented consistently with other RNG contracts in the codebase. BlockHashRNG and ChainlinkRNG follow the same pattern in README.md (address link only), and RNGWithFallback's contract includes comprehensive NatSpec documentation describing its role as a fallback mechanism and its relationship to other RNG implementations.Likely an incorrect or invalid review comment.
81-106: Verify all deployment addresses match the actual deployed contracts.The Arbitrum Sepolia devnet section lists 39 contract addresses. Ensure that each address is consistent with the actual on-chain deployments by comparing against deployment artifacts or scripts. Pay particular attention to verifying proxy and implementation addresses for contracts that use proxy patterns (DisputeKitClassic, DisputeTemplateRegistry, etc.) to ensure both are correctly specified.
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated_Proxy.json (1)
2-66: Sanity-check proxy “args” and on-chain verification for the new address/tx.
This artifact is effectively a full redeploy (newaddress,transactionHash,receipt.*, andargs). Please verify the_implementation(Line 59) is the intended deployed implementation for this environment, and that the init calldata (Line 60) matches the expected initializer for the new owner-based flow.contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter_Proxy.json (1)
2-66: Sanity-check proxy “args” and on-chain verification for the new address/tx.
Same as above: confirm the_implementation(Line 59) and init calldata (Line 60) are correct for the DisputeKitShutter wiring on arbitrumSepoliaDevnet after the owner refactor.contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic_Proxy.json (1)
2-66: Sanity-check proxy “args” and on-chain verification for the new address/tx.
Given the broad refactor + compiler pipeline shift (viaIR/optimizer), it’s worth double-checking the_implementation(Line 59) and init calldata (Line 60) correspond to the exact Classic DK implementation and initializer you expect.contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry.json (1)
38-154: ABI-breaking rename: ensure all callers migrated (governor*→owner*).
This artifact exposesOwnerOnly(Line 38-42),changeOwner(Line 117-128), andowner()(Line 142-154). Please ensure any scripts/UI/subgraph mappings that previously invokedchangeGovernor/governor()are updated for arbitrumSepoliaDevnet specifically.contracts/deployments/arbitrumSepoliaDevnet/DisputeResolverUniversity.json (1)
279-294: Receipt logs are empty: verify deployment tool didn’t drop logs unexpectedly.
This artifact’s receipt haslogs: [](Line 289) despite successful status. That can be normal (constructor didn’t emit), but if your pipeline expects specific deployment-time events, confirm this is acceptable.contracts/deployments/arbitrumSepoliaDevnet/DisputeTemplateRegistry_Implementation.json (1)
35-151: Implementation ABI addsOwnerOnly+owner(); validate upgrade authorization matches owner model.
The implementation surface now includesOwnerOnly(Line 35-39) andowner()(Line 139-151), andinitialize(address _owner)(Line 126-137). Please ensure the deployed proxy initialization (in the proxy artifact) actually setsowneras intended, otherwise upgrades could be locked toaddress(0)or an unexpected deployer.contracts/deployments/arbitrumSepoliaDevnet/DisputeResolver.json (1)
20-69: ABI-breaking changes:DisputeRequestsignature +governor→ownerrename—double-check downstream consumers (indexers/SDK/UI).
This artifact now exposesowner()/changeOwner(address)and theDisputeRequestevent is the 3-parameter variant; any consumer decoding the previous event/function names will break.Also applies to: 141-152, 235-246
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.json (2)
286-334: Large ABI surface expansion (NextRoundSettings + token gating + owner governance): ensure all callers decode the new shapes.
In particular:draw(..., _roundNbVotes)now returns(drawnAddress, fromSubcourtID)and token gating addssupportedTokens()/changeSupportedTokens()+TokenNotSupported.Also applies to: 559-634, 856-872, 1356-1374, 1503-1512
1503-1510: The initialize args are correct for arbitrumSepoliaDevnet. All three values match the expected addresses: owner (0xf1C7c037891525E360C59f708739Ac09A7670c59), core/KlerosCore (0x9EfCaeF787d0b53d7a24fdeAB067A4BAFCDb892F), and wNative/WETH (0x3829A2486d53ee984a0ca2D76552715726b77138). These match consistently across other deployment files in the same devnet suite.contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json (1)
211-252: Shutter ABI-breaking changes: commit event payload split + newhashJustification/justificationCommitments.
Anything decodingCommitCastShutter(or producing/verifying commits offchain) must be updated to the two-commit model.Also applies to: 555-592, 1312-1335, 1436-1464
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.json (1)
128-138: Gated+Shutter combines two sets of breaking ABI changes—ensure clients handle both token gating and split commitments.
Pay attention toTokenNotSupported/supportedTokens()plus the_choiceCommit/_justificationCommitfields.Also applies to: 221-263, 741-758, 1533-1551
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.json (1)
1455-1461: Theaddress(0)core initialization in devnet is intentional and safe.This pattern is documented in the deployment scripts (
contracts/deploy/00-home-chain-arbitration-university.ts), which callchangeCore(klerosCore.address)immediately after initialization. ThechangeCore()function exists specifically to support this two-step initialization flow, allowing the core address to be set once available. The contract will be unusable untilchangeCore()is called, but this is expected behavior during the devnet deployment process and does not indicate a security concern.
| "execute": { | ||
| "methodName": "reinitialize", | ||
| "methodName": "initialize", | ||
| "args": [ | ||
| "0xf1C7c037891525E360C59f708739Ac09A7670c59", | ||
| "0x0000000000000000000000000000000000000000", | ||
| "0x3829A2486d53ee984a0ca2D76552715726b77138" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find DisputeKitClassic contract implementation
find . -type f -name "*.sol" | xargs rg -l "DisputeKitClassic" | head -20Repository: kleros/kleros-v2
Length of output: 176
🏁 Script executed:
# Also search for the initialize method pattern in contracts
find . -type f -name "*.sol" | xargs rg -l "contract DisputeKitClassic" | head -10Repository: kleros/kleros-v2
Length of output: 302
🏁 Script executed:
# Look at the deployment file to understand context
cat contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json | head -100Repository: kleros/kleros-v2
Length of output: 1940
🏁 Script executed:
# Read DisputeKitClassic.sol to examine initialize method
wc -l contracts/src/arbitration/dispute-kits/DisputeKitClassic.solRepository: kleros/kleros-v2
Length of output: 123
🏁 Script executed:
# Search for initialize method in DisputeKitClassic
rg -A 20 "function initialize" contracts/src/arbitration/dispute-kits/DisputeKitClassic.solRepository: kleros/kleros-v2
Length of output: 585
🏁 Script executed:
# Check the base class for initialize method
rg -A 20 "function initialize" contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.solRepository: kleros/kleros-v2
Length of output: 42
🏁 Script executed:
# Search for core variable declaration and usage
rg "core" contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol | head -30Repository: kleros/kleros-v2
Length of output: 275
🏁 Script executed:
# Read DisputeKitClassicBase.sol completely since it's the base contract
cat -n contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.solRepository: kleros/kleros-v2
Length of output: 44238
🏁 Script executed:
# Search for core variable declaration in base
rg "core\s*=" contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.solRepository: kleros/kleros-v2
Length of output: 115
🏁 Script executed:
# Search for changeCore calls to see if core is typically set after initialization
rg -A 3 "changeCore" contracts/src/arbitration/dispute-kits/ contracts/deployments/Repository: kleros/kleros-v2
Length of output: 50374
🏁 Script executed:
# Check for other DisputeKitClassic deployments
find . -name "*DisputeKitClassic*.json" -type fRepository: kleros/kleros-v2
Length of output: 944
🏁 Script executed:
# Look at other deployment files to see if this pattern is common
fd "\.json$" contracts/deployments/ | head -10 | xargs grep -l "initialize" | head -5Repository: kleros/kleros-v2
Length of output: 342
🏁 Script executed:
# Search test files for DisputeKitClassic initialization patterns
find . -name "*.sol" -path "*/test/*" | xargs rg -l "DisputeKitClassic" | head -5Repository: kleros/kleros-v2
Length of output: 319
🏁 Script executed:
# Check arbitrum mainnet DisputeKitClassic initialization
grep -A 10 '"execute"' contracts/deployments/arbitrum/DisputeKitClassic.json | grep -A 8 "initialize"Repository: kleros/kleros-v2
Length of output: 287
🏁 Script executed:
# Check arbitrumSepolia (non-devnet) for comparison
grep -A 10 '"execute"' contracts/deployments/arbitrumSepolia/DisputeKitClassic.json | grep -A 8 "initialize"Repository: kleros/kleros-v2
Length of output: 287
🏁 Script executed:
# Check DisputeKitClassicUniversity in devnet to see if it also has zero core
grep -A 10 '"execute"' contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassicUniversity.json | grep -A 8 "initialize"Repository: kleros/kleros-v2
Length of output: 351
🏁 Script executed:
# Search test file for initialize patterns
grep -B 3 -A 10 "initialize" contracts/test/foundry/KlerosCore_Initialization.t.sol | head -50Repository: kleros/kleros-v2
Length of output: 2305
🏁 Script executed:
# Search for deployment scripts that might handle post-initialization
find . -type f \( -name "*deploy*" -o -name "*setup*" \) | grep -E "\.(ts|js)$" | head -10Repository: kleros/kleros-v2
Length of output: 145
🏁 Script executed:
# Check if there are any comments or documentation about zero core being intentional
rg -i "zero.*core|core.*zero|placeholder|temporary" contracts/deployments/ contracts/src/arbitration/dispute-kits/Repository: kleros/kleros-v2
Length of output: 50374
🏁 Script executed:
# Check test base setup to understand the complete initialization flow
grep -B 5 -A 15 "DisputeKitClassic" contracts/test/foundry/KlerosCore_TestBase.sol | head -50Repository: kleros/kleros-v2
Length of output: 2442
🏁 Script executed:
# Check if there's any explicit zero address validation or documentation
rg -i "zero address|address.*zero|core.*require|require.*core" contracts/src/arbitration/dispute-kits/DisputeKitClassic*.solRepository: kleros/kleros-v2
Length of output: 42
🏁 Script executed:
# Check if initialize method has ANY validation
grep -A 5 "function initialize" contracts/src/arbitration/dispute-kits/DisputeKitClassic.solRepository: kleros/kleros-v2
Length of output: 304
Add zero-address validation for the core parameter in initialize() and changeCore().
The deployment initializes with core = 0x0000...0000, which will cause all dispute-kit operations to fail since core is accessed throughout the contract (in onlyByCore modifier checks and 30+ function calls). While this may be a placeholder for incomplete devnet setup, the contract should validate that core is not the zero address, as it's a critical dependency with no safe fallback behavior. Either complete the devnet deployment configuration by setting a valid core address, or add explicit zero-address rejection in both initialization functions.
🤖 Prompt for AI Agents
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json around lines
1455-1462: the deployment uses the zero address for the `core` arg and the
contract lacks validation for a zero `core`, so add a non-zero address check to
both the `initialize()` and `changeCore()` entry points (e.g., require(core !=
address(0), "Core cannot be zero")) to revert on zero-address input, and
alternatively fix the devnet deployment by replacing the zero address with the
correct core address in the JSON args if available; ensure any tests or
deployment scripts are updated accordingly.
| "rule(uint256,uint256)": { | ||
| "details": "To be called by the arbitrator of the dispute, to declare the winning ruling.", | ||
| "details": "This is a callback function for the arbitrator to provide the ruling to this contract. Only the arbitrator must be allowed to call this function. Ruling 0 is reserved for \"Not able/wanting to make a decision\".", | ||
| "params": { | ||
| "_arbitratorDisputeID": "ID of the dispute in arbitrator contract.", | ||
| "_ruling": "The ruling choice of the arbitration." | ||
| "_disputeID": "The identifier of the dispute in the Arbitrator contract.", | ||
| "_ruling": "Ruling given by the arbitrator." | ||
| } | ||
| } |
There was a problem hiding this comment.
Devdoc param name mismatch for rule is confusing (_disputeID vs _arbitratorDisputeID).
Not a runtime issue, but it’s easy for integrators to misread; consider regenerating docs so names match the actual ABI if your tooling supports it.
🤖 Prompt for AI Agents
In contracts/deployments/arbitrumSepoliaDevnet/DisputeResolver.json around lines
347 to 353, the devdoc parameter name for the rule callback is `_disputeID` but
the actual ABI/signature uses `_arbitratorDisputeID`, causing a mismatch; update
the devdoc param name to exactly match the function parameter in the ABI (or
vice versa if the ABI is wrong) and then regenerate or re-export the contract
JSON so the documentation and ABI are consistent across the file.
fix(web): juror-number-input-crash
…ainance-button fix(web): withdraw-appeal-fees-maintainance-button
Feat/leaderboard offset
fix: wildcard MIME types validation
|
PR-Codex overview
This PR focuses on significant refactoring and updates to various smart contracts and components in the Kleros project, including renaming governance roles, enhancing dispute kits, and improving code structure for better maintainability.
Detailed summary
governortoownerin multiple contracts.IRNGinterface and implemented inIncrementalNG.MarkdownRendererinstead ofReactMarkdown.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.