Release #2076
Release #2076
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Enabled Hardhat viaIR compilation with solc v0.8.30
bumped @kleros/vea-contracts to v0.7.0
chore: viaIR compilation enabled for Foundry with explicit solc v0.8.30
… deprecated for later removal
feat: court llms.txt
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughMassive refactor across contracts, tests, deployments and web: owner/gov rename, IRNG and RNGWithFallback, SortitionTrees extraction and SortitionModule rewrite (phases/delayed stakes/penalties), expanded dispute-kit APIs (draw/commit/vote/coherence rewards/penalties), many ABI/initializer/upgrade changes, tooling and frontend token-gating additions. Changes
Sequence Diagram(s)%%{init: {"theme":"base","themeVariables":{"actorBorder":"#2b2b2b","actorBg":"#f7fafc","noteBg":"#f0f4f8"}}}%%
sequenceDiagram
autonumber
participant SM as SortitionModule
participant RF as RNGWithFallback
participant PR as PrimaryRNG
participant BH as BlockhashRNG
SM->>RF: requestRandomness()
RF->>PR: requestRandomness() (forward to primary RNG)
RF->>RF: record requestTimestamp
Note over SM,RF: When drawing / receiving randomness
SM->>RF: receiveRandomness()
alt Primary RNG returned (non-zero)
RF-->>SM: primary random
else Primary RNG timed out or returned 0
RF->>BH: compute fallback from recent blockhash
RF-->>SM: fallback random (emit RNGFallback)
end
SM->>SM: draw(uint96 courtID, disputeID, nonce)
SM-->>Caller: (drawnAddress, fromSubcourtID)
%%{init: {"theme":"base","themeVariables":{"actorBorder":"#2b2b2b","actorBg":"#f7fafc","noteBg":"#f0f4f8"}}}%%
sequenceDiagram
autonumber
participant User
participant Core as KlerosCore
participant DK as IDisputeKit
participant SM as SortitionModule
User->>Core: createDispute(... + fee)
Core->>DK: getNbVotesAfterAppeal(...currentNbVotes)
DK-->>Core: nbVotesNext
Core->>DK: earlyCourtJump(coreDisputeID)?
alt DisputeKit requests jump
Core->>Core: compute newCourtID & newDK (jump settings)
Core-->>User: emit CourtJump / DisputeKitJump
else normal path
Core->>SM: schedule draw hooks & lock stakes
end
Note over Core,DK: Subsequent commit/reveal/vote/appeal flows use updated signatures and coherence reward/penalty APIs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150+ minutes Areas to pay extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (13)📚 Learning: 2025-09-04T23:36:16.415ZApplied to files:
📚 Learning: 2025-09-03T22:48:32.972ZApplied to files:
📚 Learning: 2025-09-11T01:00:13.355ZApplied to files:
📚 Learning: 2024-10-22T09:38:20.093ZApplied to files:
📚 Learning: 2024-10-21T10:32:16.970ZApplied to files:
📚 Learning: 2025-09-09T13:33:46.896ZApplied to files:
📚 Learning: 2024-11-07T10:48:16.774ZApplied to files:
📚 Learning: 2025-09-30T17:18:12.895ZApplied to files:
📚 Learning: 2024-11-19T17:18:39.007ZApplied to files:
📚 Learning: 2024-11-19T16:09:41.467ZApplied to files:
📚 Learning: 2025-05-15T06:50:40.859ZApplied to files:
📚 Learning: 2024-10-14T13:58:25.708ZApplied to files:
📚 Learning: 2024-10-22T10:23:15.789ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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.
| "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 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (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.
…mpDisputeKitIDOnCourtJump
sanity check in Core._getCompatibleNextRoundSettings(), bug fix in appealCost(), naming of the NextRoundSettingsChanged parameters, better comments describing the NextRoundSettings parameters.
…pDisputeKitIDOnCourtJump
Increased Court and DK jump logic flexibility
For accredited Argentinian consumer protection lawyers
feat(KC): allow to pass commit period quicker
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/deploy/00-home-chain-arbitration.ts (1)
44-49: Fragile address prediction using "latest" nonce.The code predicts KlerosCore's address via
nonce + 3but uses the default "latest" nonce. Any pending transaction will break this calculation, causing SortitionModule to initialize with an incorrect core address.Apply these safeguards:
- const nonce = await ethers.provider.getTransactionCount(deployer); + const nonce = await ethers.provider.getTransactionCount(deployer, "pending"); klerosCoreAddress = getContractAddress(deployer, nonce + 3); console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress);Add an assertion after Line 89 (after KlerosCore deployment):
if (klerosCoreAddress.toLowerCase() !== klerosCore.address.toLowerCase()) { throw new Error( `Predicted KlerosCore ${klerosCoreAddress} ≠ actual ${klerosCore.address}. SortitionModule was initialized with the wrong core address.` ); }contracts/src/libraries/SafeERC20.sol (2)
32-48: SafeERC20 must revert on failed transfers.Lines 33-47 now just return a bool. Every caller that previously relied on
safeTransfer/safeTransferFromreverting (the whole point of the wrapper) will silently ignore failures and continue executing, leaving funds locked. Restore the revert-on-failure semantics.function safeTransfer(IERC20 _token, address _to, uint256 _value) internal returns (bool) { (bool success, bytes memory data) = address(_token).call(abi.encodeCall(IERC20.transfer, (_to, _value))); - return (success && (data.length == 0 || abi.decode(data, (bool)))); + if (!success || (data.length != 0 && !abi.decode(data, (bool)))) { + revert("SafeERC20: transfer failed"); + } + return true; } @@ function safeTransferFrom(IERC20 _token, address _from, address _to, uint256 _value) internal returns (bool) { (bool success, bytes memory data) = address(_token).call( abi.encodeCall(IERC20.transferFrom, (_from, _to, _value)) ); - return (success && (data.length == 0 || abi.decode(data, (bool)))); + if (!success || (data.length != 0 && !abi.decode(data, (bool)))) { + revert("SafeERC20: transferFrom failed"); + } + return true; }
22-26:increaseAllowanceignores failure.Line 23 calls
approvebut never inspects its return value. Non-compliant tokens (e.g., those that returnfalsewhen the allowance must be reset) will silently fail, yet the helper reports success. Perform the call via low-level encode and revert when it doesn’t signal success.function increaseAllowance(IERC20 _token, address _spender, uint256 _addedValue) internal returns (bool) { - _token.approve(_spender, _token.allowance(address(this), _spender) + _addedValue); - return true; + uint256 newAllowance = _token.allowance(address(this), _spender) + _addedValue; + (bool success, bytes memory data) = + address(_token).call(abi.encodeCall(IERC20.approve, (_spender, newAllowance))); + if (!success || (data.length != 0 && !abi.decode(data, (bool)))) { + revert("SafeERC20: approve failed"); + } + return true; }
♻️ Duplicate comments (42)
contracts/scripts/generateMetrics.sh (1)
1-3: Enable strict mode and anchor paths to script location.The script lacks error handling and uses a relative path for SOURCE_DIR, which breaks if invoked from a different working directory. Enable strict mode (
set -euo pipefail) at the top and anchor SOURCE_DIR to the script's location using SCRIPT_DIR.Apply this diff to add strict mode and fix path resolution:
#!/usr/bin/env bash +set -euo 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_DIR="$SCRIPT_DIR/../src" +OUTPUT_FILE="${OUTPUT_FILE:-METRICS.html}"contracts/deploy/00-home-chain-arbitration-mainnet.ts (1)
38-43: The nonce prediction issue remains unaddressed.The past review comment highlighting the fragility of predicting KlerosCore's address via nonce+3 still applies. The code continues to use "latest" nonce instead of "pending", and there's no assertion after deployment to catch prediction mismatches early.
Apply the previously suggested fixes:
- const nonce = await ethers.provider.getTransactionCount(deployer); + const nonce = await ethers.provider.getTransactionCount(deployer, "pending"); klerosCoreAddress = getContractAddress(deployer, nonce + 3); console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress);Add this assertion after Line 85 (after KlerosCore deployment):
if (klerosCoreAddress.toLowerCase() !== klerosCore.address.toLowerCase()) { throw new Error( `Predicted KlerosCore ${klerosCoreAddress} ≠ actual ${klerosCore.address}. SortitionModule was initialized with the wrong core address.` ); }contracts/src/rng/RandomizerRNG.sol (4)
53-62: Validate constructor parameters to prevent initialization with zero addresses.Initializing with zero addresses would brick critical functionality: zero owner disables governance, zero consumer blocks RNG requests, and zero randomizer breaks callbacks.
Apply this diff:
constructor(address _owner, address _consumer, IRandomizer _randomizer) { + require(_owner != address(0), "OwnerZeroAddress"); + require(_consumer != address(0), "ConsumerZeroAddress"); + require(address(_randomizer) != address(0), "RandomizerZeroAddress"); owner = _owner; consumer = _consumer; randomizer = _randomizer; callbackGasLimit = 50000; }Based on learnings
70-72: Add zero-address check and event emission.The function should prevent setting owner to zero address and emit an event for off-chain monitoring.
function changeOwner(address _owner) external onlyByOwner { + require(_owner != address(0), "OwnerZeroAddress"); + address previousOwner = owner; owner = _owner; + emit OwnerChanged(previousOwner, _owner); }Declare the event near line 28:
event OwnerChanged(address indexed previousOwner, address indexed newOwner);
76-78: Add zero-address check and event emission.The function should prevent setting consumer to zero address and emit an event for transparency.
function changeConsumer(address _consumer) external onlyByOwner { + require(_consumer != address(0), "ConsumerZeroAddress"); + address previousConsumer = consumer; consumer = _consumer; + emit ConsumerChanged(previousConsumer, _consumer); }Declare the event near line 29:
event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer);
88-90: Add validation and event emission for Randomizer changes.Setting randomizer to zero address or an EOA would break callback functionality. An event is needed for transparency.
+event RandomizerChanged(address indexed previousRandomizer, address indexed newRandomizer); function setRandomizer(address _randomizer) external onlyByOwner { + require(_randomizer != address(0), "RandomizerZeroAddress"); + require(_randomizer.code.length > 0, "RandomizerNotContract"); + address prev = address(randomizer); randomizer = IRandomizer(_randomizer); + emit RandomizerChanged(prev, _randomizer); }contracts/src/rng/BlockhashRNG.sol (5)
13-13: Filename casing mismatch: already flagged in previous reviewThe contract name
BlockHashRNG(uppercase "H") doesn't match the filenameBlockhashRNG.sol(lowercase "h"). This causes issues on case-sensitive filesystems and breaks imports/tooling. This was already comprehensively documented in a previous review comment.
18-23: Unbounded storage growth: old randomness slots are never cleaned upThe
randomNumbersmapping is keyed byexpectedTimestamp(calculated asrequestTimestamp + lookaheadTime). After each newrequestRandomness(), the previous timestamp's entry becomes unreachable (no public API queries by old timestamps) but is never deleted, causing permanent storage bloat.Option 1: Clean up the old entry on each new request:
function requestRandomness() external override onlyByConsumer { + // Clean up old randomness if it exists + if (requestTimestamp != 0) { + uint256 oldExpectedTimestamp = requestTimestamp + lookaheadTime; + delete randomNumbers[oldExpectedTimestamp]; + } requestTimestamp = block.timestamp; }Option 2 (simpler): Refactor to single-slot cache:
- uint256 public requestTimestamp; // Timestamp of the current request - mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp, 0 otherwise. + uint256 public requestTimestamp; // Timestamp of the current request + uint256 public lastReadyTimestamp; // The timestamp window for the cached random number. + uint256 public lastRandomNumber; // The cached random number.Then update
receiveRandomness()to uselastReadyTimestampandlastRandomNumberinstead of the mapping.
13-36: Compilation blocker: custom errorsOwnerOnly()andConsumerOnly()must be declaredThe modifiers
onlyByOwnerandonlyByConsumerrevert with custom errors that are not declared anywhere in this contract, causing a compilation failure.Apply this diff to declare the errors:
contract BlockHashRNG is IRNG { + // ************************************* // + // * Errors * // + // ************************************* // + + error OwnerOnly(); + error ConsumerOnly(); + // ************************************* // // * Storage * //
42-66: Add zero-address validation and events for governance role changesSetting
ownerorconsumertoaddress(0)would brick the contract's access control. Additionally, role changes should emit events for off-chain observability and governance transparency.Apply this diff to add validation and events:
contract BlockHashRNG is IRNG { // ************************************* // // * Errors * // // ************************************* // error OwnerOnly(); error ConsumerOnly(); + error InvalidOwner(); + error InvalidConsumer(); + + // ************************************* // + // * Events * // + // ************************************* // + + event OwnerChanged(address indexed previousOwner, address indexed newOwner); + event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer);constructor(address _owner, address _consumer, uint256 _lookaheadTime) { + if (_owner == address(0)) revert InvalidOwner(); + if (_consumer == address(0)) revert InvalidConsumer(); owner = _owner; consumer = _consumer; lookaheadTime = _lookaheadTime; }function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert InvalidOwner(); + emit OwnerChanged(owner, _owner); owner = _owner; }function changeConsumer(address _consumer) external onlyByOwner { + if (_consumer == address(0)) revert InvalidConsumer(); + emit ConsumerChanged(consumer, _consumer); consumer = _consumer; }
72-102: Critical timing-manipulation vulnerability remains unaddressedDespite a previous comment indicating this was fixed (commits 59ca8e1 to 7c30463), the code still derives randomness from
blockhash(block.number - 1)at the timereceiveRandomness()is called. Since anyone can callreceiveRandomness()after readiness (via SortitionModule'spassPhase), the first caller effectively chooses which block's hash is used, enabling timing-based bias.Mitigation: Pre-commit the target block at request time to eliminate caller timing selection:
address public owner; // The address that can withdraw funds. address public consumer; // The address that can request random numbers. uint256 public immutable lookaheadTime; // Minimal time in seconds between requesting and obtaining a random number. uint256 public requestTimestamp; // Timestamp of the current request + uint256 public requestBlock; // Block number at request time (anchor to prevent timing bias). mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp, 0 otherwise. + mapping(uint256 timestamp => uint256 blockNumber) public randomBlocks; // The block number used to derive randomness.function requestRandomness() external override onlyByConsumer { requestTimestamp = block.timestamp; + requestBlock = block.number; }function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) { if (requestTimestamp == 0) return 0; // No requests were made yet. uint256 expectedTimestamp = requestTimestamp + lookaheadTime; // Check if enough time has passed if (block.timestamp < expectedTimestamp) { return 0; // Not ready yet } // Check if we already have a saved random number for this timestamp window randomNumber = randomNumbers[expectedTimestamp]; if (randomNumber != 0) { return randomNumber; } - // Use last block hash for randomness - randomNumber = uint256(blockhash(block.number - 1)); - if (randomNumber != 0) { - randomNumbers[expectedTimestamp] = randomNumber; - } - return randomNumber; + // Derive from pre-committed block to prevent timing attacks + uint256 targetBlock = requestBlock + 1; + randomNumber = uint256(blockhash(targetBlock)); + if (randomNumber == 0) { + // Target block hash unavailable (>256 blocks old or not yet mined). + return 0; + } + randomNumbers[expectedTimestamp] = randomNumber; + randomBlocks[expectedTimestamp] = targetBlock; + return randomNumber; }contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
809-904: Stop swallowing failed ETH transfersAll three
.sendcalls drop failures silently, so juror fees/penalties can get stuck when recipients have expensive fallbacks. Use a checkedcalland revert on failure (you already haveUnsuccessfulCall).- payable(owner).send(round.totalFeesForJurors); + (bool okFee, ) = payable(owner).call{value: round.totalFeesForJurors}(""); + if (!okFee) revert UnsuccessfulCall(); @@ - payable(account).send(feeReward); + (bool okReward, ) = payable(account).call{value: feeReward}(""); + if (!okReward) revert UnsuccessfulCall(); @@ - payable(owner).send(leftoverFeeReward); + (bool okLeftover, ) = payable(owner).call{value: leftoverFeeReward}(""); + if (!okLeftover) revert UnsuccessfulCall();
280-308: Block zero-address configuration for critical rolesSetting
owner,pinakion,jurorProsecutionModule, orsortitionModuleto the zero address bricks upgrades, staking, or prosecutions. Please guard each setter (and reuse a shared error) so the contract cannot enter a dead state.function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; } @@ function changePinakion(IERC20 _pinakion) external onlyByOwner { - pinakion = _pinakion; + if (address(_pinakion) == address(0)) revert ZeroAddress(); + pinakion = _pinakion; } @@ function changeJurorProsecutionModule(address _jurorProsecutionModule) external onlyByOwner { - jurorProsecutionModule = _jurorProsecutionModule; + if (_jurorProsecutionModule == address(0)) revert ZeroAddress(); + jurorProsecutionModule = _jurorProsecutionModule; } @@ function changeSortitionModule(ISortitionModuleUniversity _sortitionModule) external onlyByOwner { - sortitionModule = _sortitionModule; + if (address(_sortitionModule) == address(0)) revert ZeroAddress(); + sortitionModule = _sortitionModule; } @@ - error OwnerOnly(); + error OwnerOnly(); + error ZeroAddress();contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
192-214: Clamp penalties to the stake actually held in this courtWhen
_penaltyexceeds_stakeOf(_courtID), the current code still subtracts_penaltyfromjuror.stakedPnkwhilestakesByCourtIDonly drops by the local court stake. That breaks the accounting invariants and leavesstakedPnkinconsistent with per-court totals, which later logic (e.g., withdrawals) relies on. Clamp the withdrawal to the lesser of the juror’s total balance and the court stake, and return the slashed amount.- availablePenalty = _penalty; - newCourtStake = _stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = _stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() + uint256 maxApplicable = _penalty; + if (juror.stakedPnk < maxApplicable) { + maxApplicable = juror.stakedPnk; + } + + uint256 currentStake = _stakeOf(_account, _courtID); + if (maxApplicable == 0 || currentStake == 0) { + return (juror.stakedPnk, currentStake, 0); + } + + uint256 applied = currentStake < maxApplicable ? currentStake : maxApplicable; + uint256 newStake = currentStake - applied; + _setStake(_account, _courtID, 0, applied, newStake); + pnkBalance = juror.stakedPnk; // updated by _setStake() + newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() + availablePenalty = applied;contracts/src/arbitration/SortitionModule.sol (1)
124-133: Block zero-address initialization before bricking ownership.Line 124 assigns
_owner,_core, and_rngwithout validating them. If deployment data or an upgrade script ever passesaddress(0),onlyByOwnercan never succeed again or the module will laterdelegatecallinto a zero RNG/core. Please guard these inputs and reuse explicit errors as previously requested.function initialize( address _owner, KlerosCore _core, uint256 _minStakingTime, uint256 _maxDrawingTime, IRNG _rng, uint256 _maxStakePerJuror, uint256 _maxTotalStaked ) external initializer { + if (_owner == address(0)) revert OwnerZeroAddress(); + if (address(_core) == address(0)) revert CoreZeroAddress(); + if (address(_rng) == address(0)) revert RNGZeroAddress(); owner = _owner; core = _core; minStakingTime = _minStakingTime; @@ - error OwnerOnly(); + error OwnerOnly(); + error OwnerZeroAddress(); + error CoreZeroAddress(); + error RNGZeroAddress();contracts/src/arbitration/devtools/KlerosCoreRuler.sol (2)
181-210: Block zero owner in initialize() before assignmentsLeaving
_ownerunchecked allows initializing with the zero address, permanently disabling everyonlyByOwnerpath in this contract. Please revert when_owner == address(0)(and add a matching custom error to the errors section) before assigningowner = _owner.function initialize(address _owner, IERC20 _pinakion, uint256[4] memory _courtParameters) external initializer { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; pinakion = _pinakion;Don’t forget to declare
error ZeroAddress();alongside the other custom errors.
234-236: Guard changeOwner() against zero address
changeOwnerstill permits setting the owner to the zero address, which bricks every futureonlyByOwneraction. Add an explicit zero-address check (and keep the guard reusable by using the sameZeroAddress()error you add forinitialize). Emitting anOwnerChangedevent would also restore observability for off-chain listeners.function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + address oldOwner = owner; + owner = _owner; + emit OwnerChanged(oldOwner, _owner);Declare
event OwnerChanged(address indexed _oldOwner, address indexed _newOwner);near the other events.contracts/src/arbitration/PolicyRegistry.sol (1)
50-70: Prevent zero owner from bricking the registryAllowing
_owner == address(0)ininitializeorchangeOwnerleaves the registry without a callable owner forever, so upgrades and policy updates stop working. Please reject zero owner inputs in both functions and emit a clear error.function initialize(address _owner) external initializer { - owner = _owner; + if (_owner == address(0)) revert ZeroOwner(); + owner = _owner; } ... function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroOwner(); + owner = _owner; } ... - error OwnerOnly(); + error OwnerOnly(); + error ZeroOwner();Also applies to: 89-89
contracts/src/gateway/HomeGateway.sol (5)
74-105: Disallow zero owner to avoid locking the gateway
initializeandchangeOwnerlet_ownerbeaddress(0), which permanently disables everyonlyByOwnercontrol point. Reject zero before assignment and add a custom error.function initialize( address _owner, ... - owner = _owner; + if (_owner == address(0)) revert ZeroOwner(); + owner = _owner; ... function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroOwner(); + owner = _owner; } ... - error OwnerOnly(); + error OwnerOnly(); + error ZeroOwner();Also applies to: 243-244
66-75: Fix the initializer docstringThe NatSpec still claims this initializes “PolicyRegistry”. Update it to describe HomeGateway and its actual parameters to avoid confusing downstream tooling/docs.
- /// @notice Constructs the `PolicyRegistry` contract. + /// @notice Initializes the HomeGateway contract.
151-159: Close the reentrancy window before calling the arbitratorWe still assign
relayedData.relayerafterarbitrator.createDispute{value: …}, so a malicious/buggy arbitrator can re-enter viarelayCreateDisputeand bypass the guard to double-relay. Pre-mark the relayer before any external call.RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash]; if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed(); + relayedData.relayer = msg.sender; - uint256 disputeID = arbitrator.createDispute{value: msg.value}(_params.choices, _params.extraData); + uint256 disputeID = arbitrator.createDispute{value: msg.value}(_params.choices, _params.extraData); ... - relayedData.relayer = msg.sender;
187-200: Same reentrancy window on the ERC20 pathThe ERC20 overload still writes
relayedData.relayeronly after token transfers andarbitrator.createDispute. Re-entering during those calls bypasses theDisputeAlreadyRelayedguard. Move the assignment directly after the guard and drop the trailing write.RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash]; if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed(); + relayedData.relayer = msg.sender; - if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed(); + if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed(); ... - relayedData.relayer = msg.sender;
172-172: Add the missing override modifierThe ERC20 overload of
relayCreateDisputemustoverridethe interface declaration (otherwise the contract doesn’t compile).-function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external { +function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external override {contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
41-61: Block zero-owner assignments before deployment/ownership transfers
initializeandchangeOwnerboth acceptaddress(0), which irreversibly disables everyonlyByOwneraction (no one can ever call them again). This bricks upgrades and template writes, so it’s a release blocker. Add a zero-address guard to both entry points and introduce a dedicated error.function initialize(address _owner) external initializer { - owner = _owner; + if (_owner == address(0)) revert ZeroOwner(); + owner = _owner; } ... function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroOwner(); + owner = _owner; } ... - error OwnerOnly(); + error OwnerOnly(); + error ZeroOwner();Also applies to: 81-82
contracts/src/gateway/interfaces/IHomeGateway.sol (1)
58-64: Fix NatSpec to referencefeeToken()The ERC20 overload comment still cites
acceptedFeeToken(), but the interface now exposesfeeToken(). Please update the docstring to the correct accessor so downstream generators don’t emit stale names.- /// This function accepts the fees payment in the ERC20 `acceptedFeeToken()`. + /// This function accepts the fees payment in the ERC20 `feeToken()`.contracts/src/arbitration/arbitrables/DisputeResolver.sol (1)
60-72: Add event emissions for critical governance changes.The governance functions
changeOwner,changeArbitrator, andchangeTemplateRegistrymodify critical contract parameters but don't emit events, impacting off-chain monitoring and auditability.Consider adding events:
+ event OwnerChanged(address indexed previousOwner, address indexed newOwner); + event ArbitratorChanged(IArbitratorV2 indexed previousArbitrator, IArbitratorV2 indexed newArbitrator); + event TemplateRegistryChanged(IDisputeTemplateRegistry indexed previousRegistry, IDisputeTemplateRegistry indexed newRegistry);And emitting them in each function (capture previous value before assignment).
contracts/src/arbitration/arbitrables/ArbitrableExample.sol (2)
88-109: Add event emissions for governance changes.The governance functions lack event emissions for off-chain monitoring and auditability. Consider adding events for
changeArbitrator,changeArbitratorExtraData,changeTemplateRegistry,changeDisputeTemplate, andchangeNumberOfRulingOptions.
152-163: Critical: Unknown arbitrator dispute IDs can silently target dispute 0.The mapping
externalIDtoLocalIDdefaults to 0 for unknown keys. An attacker or mistaken call with an unregistered_arbitratorDisputeIDwill operate ondisputes[0]without reverting.Apply a sentinel pattern (store localID + 1 in the mapping):
function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override { - uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID]; + uint256 localDisputeIDPlusOne = externalIDtoLocalID[_arbitratorDisputeID]; + if (localDisputeIDPlusOne == 0) revert UnknownDisputeID(); + uint256 localDisputeID = localDisputeIDPlusOne - 1; DisputeStruct storage dispute = disputes[localDisputeID];And when writing:
- externalIDtoLocalID[disputeID] = localDisputeID; + externalIDtoLocalID[disputeID] = localDisputeID + 1;Add the error:
+ error UnknownDisputeID();contracts/src/gateway/ForeignGateway.sol (1)
108-113: Harmonize access control and validate owner address.The
changeOwnerfunction uses an inline owner check instead of theonlyByOwnermodifier, and doesn't validate against zero-address.-function changeOwner(address _owner) external { - if (owner != msg.sender) revert OwnerOnly(); +function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert OwnerOnly(); owner = _owner; }contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (2)
83-86: Validate owner address in initialize.The initializer doesn't validate that
_owneris non-zero, which could brick the contract's ownership.function initialize(address _owner, KlerosCore _core, address _wNative) external initializer { + if (_owner == address(0)) revert ZeroAddress(); __DisputeKitClassicBase_initialize(_owner, _core, _wNative); supportedTokens[NO_TOKEN_GATE] = true; }Add error:
+ error ZeroAddress();
173-189: Scope transient callerIsJuror to avoid cross-call leakage.The transient
callerIsJurorflag is set before calling_castVote. If_castVoteor any of its callees performs external calls, reentrancy could observe unintended juror context. At minimum, save and restore the prior value:+ bool _prev = callerIsJuror; callerIsJuror = juror == msg.sender; _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); - callerIsJuror = false; + callerIsJuror = _prev;contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
64-66: Validate owner address in initialize.The initializer doesn't validate that
_owneris non-zero.function initialize(address _owner, KlerosCore _core, address _wNative) external initializer { + if (_owner == address(0)) revert ZeroAddress(); __DisputeKitClassicBase_initialize(_owner, _core, _wNative); }Add error:
+ error ZeroAddress();
129-145: Scope transient callerIsJuror via save/restore.Save and restore the previous value of
callerIsJurorto avoid nested-context bugs:+ bool _prev = callerIsJuror; callerIsJuror = juror == msg.sender; _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); - callerIsJuror = false; + callerIsJuror = _prev;contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
90-104: Fix NatSpec: "reward" → "penalty" in getDegreeOfCoherencePenalty.Line 97 incorrectly states "reward" in the return documentation for
getDegreeOfCoherencePenalty. This should say "penalty" to match the function's purpose.- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. + /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK penalty.contracts/src/arbitration/KlerosGovernor.sol (1)
85-88: Critical: onlyByOwner checks address(this), not an owner variable.The
onlyByOwnermodifier comparesmsg.sendertoaddress(this), which means only the contract itself can call these functions (presumably viaexecuteOwnerProposal). However, there's noownerstorage variable initialized. If this pattern is intentional (self-governance only), it should be documented. If not, you need to add anownervariable and update the modifier.contracts/src/arbitration/KlerosCore.sol (5)
317-365: Add zero-address validation in initialize for critical dependencies.The initialize function doesn't validate that critical addresses are non-zero, which could brick the contract.
) external initializer { + if (_owner == address(0)) revert ZeroAddress(); + if (address(_pinakion) == address(0)) revert ZeroAddress(); + if (address(_disputeKit) == address(0)) revert ZeroAddress(); + if (address(_sortitionModuleAddress) == address(0)) revert ZeroAddress(); + if (_wNative == address(0)) revert ZeroAddress(); owner = _owner;Add error if missing:
+ error ZeroAddress();
400-427: Add zero-address validation for governance setters.The governance setter functions lack zero-address guards, which could break critical functionality.
Apply validation to
changeOwner,changeGuardian,changePinakion,changeJurorProsecutionModule, andchangeSortitionModule:function changeOwner(address payable _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); owner = _owner; }(Apply similar pattern to other setters)
432-436: Validate dispute kit address when adding.Prevent adding a zero-address dispute kit:
function addNewDisputeKit(IDisputeKit _disputeKitAddress) external onlyByOwner { + if (address(_disputeKitAddress) == address(0)) revert ZeroAddress(); uint256 disputeKitID = disputeKits.length;
577-581: Validate rateInEth > 0 when changing currency rates.Setting a token's rate to zero will cause division-by-zero at line 1231 in
convertEthToTokenAmount.function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { + if (_rateInEth == 0) revert TokenRateZero(); currencyRates[_feeToken].rateInEth = _rateInEth;Add error:
+ error TokenRateZero();
1230-1232: Division-by-zero risk in convertEthToTokenAmount.If
rateInEthis zero (token not configured), this will revert with a raw division error.function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { + if (currencyRates[_toToken].rateInEth == 0) revert TokenNotAccepted(); return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; }contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
167-175: Add zero-address guard for owner parameter.The initializer allows setting
ownertoaddress(0), which would make the contract unusable since all governance functions requireonlyByOwner.Apply this diff:
function __DisputeKitClassicBase_initialize( address _owner, KlerosCore _core, address _wNative ) internal onlyInitializing { + if (_owner == address(0)) revert OwnerOnly(); owner = _owner; core = _core; wNative = _wNative; }
485-503: Fix potential revert when only one choice was funded.Lines 501-502 access
fundedChoices[0]andfundedChoices[1]unconditionally, assuming exactly two funded choices. However,fundedChoicescan have length 1 if only one side funded their appeal, causing an out-of-bounds revert that blocks all withdrawals for that round.Apply this safer aggregation:
} else if (!round.hasPaid[finalRuling]) { // The ultimate winner was not funded in this round. In this case funded ruling option(s) are reimbursed. - amount += - (round.contributions[_beneficiary][_choice] * round.feeRewards) / - (round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]); + uint256 denom; + for (uint256 j = 0; j < round.fundedChoices.length; j++) { + denom += round.paidFees[round.fundedChoices[j]]; + } + if (denom > 0) { + amount += (round.contributions[_beneficiary][_choice] * round.feeRewards) / denom; + } }
🧹 Nitpick comments (3)
contracts/scripts/generateMetrics.sh (1)
5-23: Prevent data loss on command failure by writing atomically via a temp file.If the
yarn dlx solidity-code-metricscommand fails, the output file (METRICS.html) is left empty or corrupted because the shell truncates it before the command runs. Write to a temporary file first, then move it to the final location only on success.Apply this diff to add atomic writes and proper error handling:
+TMP_OUT="$(mktemp)" +trap "rm -f '$TMP_OUT'" EXIT + 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/SortitionTrees.sol \ "$SOURCE_DIR"/libraries/Safe* \ "$SOURCE_DIR"/rng/RNGWithFallback.sol \ "$SOURCE_DIR"/rng/ChainlinkRNG.sol \ "$SOURCE_DIR"/rng/ChainlinkConsumerBaseV2Plus.sol \ "$SOURCE_DIR"/rng/IRNG.sol \ "$SOURCE_DIR"/proxy/UUPSProx* \ "$SOURCE_DIR"/proxy/Initializable.sol \ - --html >METRICS.html + --html >"$TMP_OUT" +mv "$TMP_OUT" "$OUTPUT_FILE"contracts/src/rng/RandomizerRNG.sol (1)
82-84: Validate callback gas limit bounds.Setting an unreasonably low (e.g., 0) or excessively high gas limit could cause callback failures or waste funds.
function setCallbackGasLimit(uint256 _callbackGasLimit) external onlyByOwner { + require(_callbackGasLimit >= 21000, "GasLimitTooLow"); + require(_callbackGasLimit <= 5000000, "GasLimitTooHigh"); callbackGasLimit = _callbackGasLimit; }contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
192-194: Consider zero-address validation inchangeOwner.While less critical than the initializer,
changeOwnershould validate that_owner != address(0)to prevent accidentally locking the contract.function changeOwner(address payable _owner) external onlyByOwner { + if (_owner == address(0)) revert OwnerOnly(); owner = _owner; }
| function initialize( | ||
| address _owner, | ||
| KlerosCore _core, | ||
| address _wNative, | ||
| address _accreditedProfessionalToken, | ||
| address _accreditedConsumerProtectionLawyerToken | ||
| ) external initializer { | ||
| __DisputeKitClassicBase_initialize(_owner, _core, _wNative); | ||
| accreditedProfessionalToken = _accreditedProfessionalToken; | ||
| accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; | ||
| } | ||
|
|
||
| // ************************ // | ||
| // * Governance * // | ||
| // ************************ // | ||
|
|
||
| /// @dev Access Control to perform implementation upgrades (UUPS Proxiable) | ||
| /// Only the owner can perform upgrades (`onlyByOwner`) | ||
| function _authorizeUpgrade(address) internal view override onlyByOwner { | ||
| // NOP | ||
| } | ||
|
|
||
| /// @notice Changes the accredited professional token. | ||
| /// @param _accreditedProfessionalToken The address of the accredited lawyer token. | ||
| function changeAccreditedProfessionalToken(address _accreditedProfessionalToken) external onlyByOwner { | ||
| accreditedProfessionalToken = _accreditedProfessionalToken; | ||
| } | ||
|
|
||
| /// @notice Changes the accredited consumer protection lawyer token. | ||
| /// @param _accreditedConsumerProtectionLawyerToken The address of the accredited consumer protection lawyer token. | ||
| function changeAccreditedConsumerProtectionLawyerToken( | ||
| address _accreditedConsumerProtectionLawyerToken | ||
| ) external onlyByOwner { | ||
| accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; | ||
| } |
There was a problem hiding this comment.
Guard token gate setters against zero addresses
If either token address is set to address(0), every future draw hits IBalanceHolder(0).balanceOf(...), which returns empty data and reverts during ABI decoding. That bricks the dispute kit and cannot be recovered without an upgrade. Please reject zero addresses during initialize and in both setters, reusing TokenNotSupported for the revert.
function initialize(
address _owner,
KlerosCore _core,
address _wNative,
address _accreditedProfessionalToken,
address _accreditedConsumerProtectionLawyerToken
) external initializer {
+ if (_accreditedProfessionalToken == address(0)) revert TokenNotSupported(_accreditedProfessionalToken);
+ if (_accreditedConsumerProtectionLawyerToken == address(0)) {
+ revert TokenNotSupported(_accreditedConsumerProtectionLawyerToken);
+ }
__DisputeKitClassicBase_initialize(_owner, _core, _wNative);
accreditedProfessionalToken = _accreditedProfessionalToken;
accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken;
}
@@
function changeAccreditedProfessionalToken(address _accreditedProfessionalToken) external onlyByOwner {
+ if (_accreditedProfessionalToken == address(0)) revert TokenNotSupported(_accreditedProfessionalToken);
accreditedProfessionalToken = _accreditedProfessionalToken;
}
@@
function changeAccreditedConsumerProtectionLawyerToken(
address _accreditedConsumerProtectionLawyerToken
) external onlyByOwner {
+ if (_accreditedConsumerProtectionLawyerToken == address(0)) {
+ revert TokenNotSupported(_accreditedConsumerProtectionLawyerToken);
+ }
accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize( | |
| address _owner, | |
| KlerosCore _core, | |
| address _wNative, | |
| address _accreditedProfessionalToken, | |
| address _accreditedConsumerProtectionLawyerToken | |
| ) external initializer { | |
| __DisputeKitClassicBase_initialize(_owner, _core, _wNative); | |
| accreditedProfessionalToken = _accreditedProfessionalToken; | |
| accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; | |
| } | |
| // ************************ // | |
| // * Governance * // | |
| // ************************ // | |
| /// @dev Access Control to perform implementation upgrades (UUPS Proxiable) | |
| /// Only the owner can perform upgrades (`onlyByOwner`) | |
| function _authorizeUpgrade(address) internal view override onlyByOwner { | |
| // NOP | |
| } | |
| /// @notice Changes the accredited professional token. | |
| /// @param _accreditedProfessionalToken The address of the accredited lawyer token. | |
| function changeAccreditedProfessionalToken(address _accreditedProfessionalToken) external onlyByOwner { | |
| accreditedProfessionalToken = _accreditedProfessionalToken; | |
| } | |
| /// @notice Changes the accredited consumer protection lawyer token. | |
| /// @param _accreditedConsumerProtectionLawyerToken The address of the accredited consumer protection lawyer token. | |
| function changeAccreditedConsumerProtectionLawyerToken( | |
| address _accreditedConsumerProtectionLawyerToken | |
| ) external onlyByOwner { | |
| accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; | |
| } | |
| function initialize( | |
| address _owner, | |
| KlerosCore _core, | |
| address _wNative, | |
| address _accreditedProfessionalToken, | |
| address _accreditedConsumerProtectionLawyerToken | |
| ) external initializer { | |
| if (_accreditedProfessionalToken == address(0)) revert TokenNotSupported(_accreditedProfessionalToken); | |
| if (_accreditedConsumerProtectionLawyerToken == address(0)) { | |
| revert TokenNotSupported(_accreditedConsumerProtectionLawyerToken); | |
| } | |
| __DisputeKitClassicBase_initialize(_owner, _core, _wNative); | |
| accreditedProfessionalToken = _accreditedProfessionalToken; | |
| accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; | |
| } | |
| // ************************ // | |
| // * Governance * // | |
| // ************************ // | |
| /// @dev Access Control to perform implementation upgrades (UUPS Proxiable) | |
| /// Only the owner can perform upgrades (`onlyByOwner`) | |
| function _authorizeUpgrade(address) internal view override onlyByOwner { | |
| // NOP | |
| } | |
| /// @notice Changes the accredited professional token. | |
| /// @param _accreditedProfessionalToken The address of the accredited lawyer token. | |
| function changeAccreditedProfessionalToken(address _accreditedProfessionalToken) external onlyByOwner { | |
| if (_accreditedProfessionalToken == address(0)) revert TokenNotSupported(_accreditedProfessionalToken); | |
| accreditedProfessionalToken = _accreditedProfessionalToken; | |
| } | |
| /// @notice Changes the accredited consumer protection lawyer token. | |
| /// @param _accreditedConsumerProtectionLawyerToken The address of the accredited consumer protection lawyer token. | |
| function changeAccreditedConsumerProtectionLawyerToken( | |
| address _accreditedConsumerProtectionLawyerToken | |
| ) external onlyByOwner { | |
| if (_accreditedConsumerProtectionLawyerToken == address(0)) { | |
| revert TokenNotSupported(_accreditedConsumerProtectionLawyerToken); | |
| } | |
| accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; | |
| } |
🤖 Prompt for AI Agents
In
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol
around lines 49-83, add validation to reject zero addresses for both token
fields: during initialize and in changeAccreditedProfessionalToken and
changeAccreditedConsumerProtectionLawyerToken. Specifically, before assigning
each token address, check that the provided address is not address(0) and revert
using the existing TokenNotSupported error (e.g., revert TokenNotSupported();)
if it is zero; leave all other logic and onlyByOwner guards unchanged.
| /// @param _rng The new random number generator. | ||
| function changeRandomNumberGenerator(IRNG _rng) external onlyByOwner { | ||
| rng = _rng; | ||
| if (phase == Phase.generating) { | ||
| rng.requestRandomness(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject zero RNG updates.
Line 179 lets the owner set rng to address(0). The next passPhase() in the generating phase will then call rng.requestRandomness() and revert forever. Guard the setter with the same zero-address error introduced above.
function changeRandomNumberGenerator(IRNG _rng) external onlyByOwner {
+ if (address(_rng) == address(0)) revert RNGZeroAddress();
rng = _rng;
if (phase == Phase.generating) {
rng.requestRandomness();🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModule.sol around lines 178 to 184, the
changeRandomNumberGenerator function currently allows the owner to set rng to
the zero address which will cause future calls (e.g., passPhase during
generating) to call rng.requestRandomness() and permanently revert; add a guard
that reverts if address(_rng) == address(0) using the same zero-address error
message used elsewhere in the contract so the owner cannot set a zero RNG, then
proceed to assign rng and call requestRandomness() when in Phase.generating.
| Juror storage juror = jurors[_account]; | ||
| availablePenalty = _penalty; | ||
| newCourtStake = _stakeOf(_account, _courtID); | ||
| if (juror.stakedPnk < _penalty) { | ||
| availablePenalty = juror.stakedPnk; | ||
| } | ||
|
|
||
| if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. | ||
|
|
||
| uint256 currentStake = newCourtStake; | ||
| uint256 newStake = 0; | ||
| if (currentStake >= availablePenalty) { | ||
| newStake = currentStake - availablePenalty; | ||
| } | ||
| _setStake(_account, _courtID, 0, availablePenalty, newStake); | ||
| pnkBalance = juror.stakedPnk; // updated by _setStake() | ||
| newCourtStake = newStake; | ||
| } |
There was a problem hiding this comment.
Keep totalStaked in sync when penalizing.
Line 354 removes stake via availablePenalty, but totalStaked is never decremented. After a few penalties the global cap check totalStaked + stakeChange > maxTotalStaked becomes meaningless (it stays high while actual stake shrinks), blocking fresh stakes and desynchronising aggregates. Subtract the penalty before calling _setStake.
if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply.
uint256 currentStake = newCourtStake;
uint256 newStake = 0;
if (currentStake >= availablePenalty) {
newStake = currentStake - availablePenalty;
}
+ totalStaked -= availablePenalty;
_setStake(_account, _courtID, 0, availablePenalty, newStake);🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModule.sol around lines 344 to 361, the
penalty branch updates juror and court stakes but does not decrement the global
totalStaked, causing the global aggregate to become desynchronized and blocking
future stakes; before calling _setStake subtract availablePenalty from
totalStaked (ensuring totalStaked >= availablePenalty to avoid underflow) so the
global total reflects the applied penalty, then proceed to call _setStake and
update local variables as before.
| address _account, | ||
| uint96 _courtID, | ||
| uint256 _reward | ||
| ) external override onlyByCore returns (bool success) { | ||
| if (_reward == 0) return true; // No reward to add. | ||
|
|
||
| uint256 currentStake = _stakeOf(_account, _courtID); | ||
| if (currentStake == 0) return false; // Juror has been unstaked, don't increase their stake. | ||
|
|
||
| uint256 newStake = currentStake + _reward; | ||
| _setStake(_account, _courtID, _reward, 0, newStake); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Update totalStaked when rewarding.
Line 375 increases a juror’s stake, yet totalStaked is untouched. That lets us exceed maxTotalStaked by minting rewards and then staking additional PNK because the cap check undercounts reality. Please bump the aggregate before delegating to _setStake.
uint256 currentStake = _stakeOf(_account, _courtID);
if (currentStake == 0) return false; // Juror has been unstaked, don't increase their stake.
uint256 newStake = currentStake + _reward;
+ totalStaked += _reward;
_setStake(_account, _courtID, _reward, 0, newStake);
return true;🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModule.sol around lines 365 to 377, the
function that rewards a juror increases the juror's stake but never updates the
contract-wide totalStaked, which allows totalStaked to be undercounted; update
the aggregate before calling _setStake by adding the _reward to totalStaked (and
perform the same maxTotalStaked check/require used elsewhere to prevent
exceeding the cap), then call _setStake with the existing arguments; ensure
state changes occur in the correct order (update totalStaked first, revert on
cap breach, then update the individual stake).
| /// @inheritdoc ISortitionModule | ||
| function withdrawLeftoverPNK(address _account) external override { | ||
| // Can withdraw the leftover PNK if fully unstaked, has no tokens locked and has positive balance. | ||
| // This withdrawal can't be triggered by calling setStake() in KlerosCore because current stake is technically 0, thus it is done via separate function. | ||
| uint256 amount = getJurorLeftoverPNK(_account); | ||
| if (amount == 0) revert NotEligibleForWithdrawal(); | ||
| jurors[_account].stakedPnk = 0; | ||
| core.transferBySortitionModule(_account, amount); | ||
| emit LeftoverPNKWithdrawn(_account, amount); | ||
| } |
There was a problem hiding this comment.
Reduce totalStaked when leftover PNK exits.
Line 463 zeroes jurors[_account].stakedPnk but the global counter still includes that balance. After a few withdrawals totalStaked may exceed the actual sum of stakes, breaking the cap logic. Subtract the withdrawn amount before zeroing the juror.
uint256 amount = getJurorLeftoverPNK(_account);
if (amount == 0) revert NotEligibleForWithdrawal();
+ totalStaked -= amount;
jurors[_account].stakedPnk = 0;
core.transferBySortitionModule(_account, amount);
emit LeftoverPNKWithdrawn(_account, amount);🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModule.sol around lines 456 to 465, the
function withdrawLeftoverPNK zeroes jurors[_account].stakedPnk but does not
update the global totalStaked, causing totalStaked to become larger than the
true sum after withdrawals; subtract the withdrawn amount from totalStaked
(totalStaked -= amount) before zeroing jurors[_account].stakedPnk to keep the
global counter consistent and avoid under/overflow issues.
| /// @notice Get the "random number" (which is always the same). | ||
| /// @return randomNumber The random number or 0 if it is not ready or has not been requested. |
There was a problem hiding this comment.
Fix misleading documentation.
The documentation is inaccurate:
- Line 22 states the number is "always the same," but the implementation returns an incrementing value (
number++). - Line 23 claims it may return
0if not ready, but this implementation always returns the next incremented number.
Apply this diff to correct the documentation:
- /// @notice Get the "random number" (which is always the same).
- /// @return randomNumber The random number or 0 if it is not ready or has not been requested.
+ /// @notice Get the next incremented number.
+ /// @return randomNumber The next number in the sequence (increments on each call).📝 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.
| /// @notice Get the "random number" (which is always the same). | |
| /// @return randomNumber The random number or 0 if it is not ready or has not been requested. | |
| /// @notice Get the next incremented number. | |
| /// @return randomNumber The next number in the sequence (increments on each call). |
🤖 Prompt for AI Agents
In contracts/src/rng/IncrementalNG.sol around lines 22 to 23, the NatSpec is
incorrect: it says the random number is "always the same" and "or 0 if it is not
ready", but the function actually returns an incrementing value (number++) and
never returns 0 for readiness; update the comments to state that each call
returns the next incremented value (deterministic incrementer) and remove the
"or 0 if it is not ready or has not been requested" clause, and adjust the
@notice and @return text to clearly describe that behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
45-54: Validate_pohparameter and verify initializer modifier change.Two concerns:
Unvalidated
_pohparameter (lines 52): If_pohis zero address or not a contract,_postDrawCheckwill revert when callingpoh.isHuman(_juror), halting draws at runtime. Add validation at initialization to fail fast.Initializer modifier change (line 50): Changed from
reinitializer(1)toinitializer. This prevents re-initialization of existing proxy instances. Verify this is intentional—if upgrading existing deployments, you'll needreinitializer(n)where n > previous version.Apply this diff to add
_pohvalidation:function initialize( address _owner, KlerosCore _core, IProofOfHumanity _poh, address _wNative ) external initializer { + require(address(_poh) != address(0), "Invalid PoH address"); + require(address(_poh).code.length > 0, "PoH must be a contract"); __DisputeKitClassicBase_initialize(_owner, _core, _wNative); poh = _poh; singleDrawPerJuror = true; }Run this script to check if this contract has existing proxy deployments that would need re-initialization:
#!/bin/bash # Search for deployment artifacts or references to DisputeKitSybilResistant proxy deployments echo "=== Searching for DisputeKitSybilResistant deployment artifacts ===" fd -e json -e js -e ts . contracts/deployments web/src --exec grep -l "DisputeKitSybilResistant" {} echo -e "\n=== Searching for proxy upgrade patterns ===" rg -n "DisputeKitSybilResistant" --type-add 'deploy:*.{js,ts}' -t deploy -A 5 -B 5
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
8-14: Consider importing the interface from a shared location.Defining the
IProofOfHumanityinterface inline can lead to maintenance issues if it diverges from the actual ProofOfHumanity contract implementation. Since the documentation already references the external contract, consider importing this interface from a dedicated interface file (e.g.,contracts/src/arbitration/interfaces/IProofOfHumanity.sol) to ensure consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/KlerosGovernor.sol(0 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(4 hunks)
💤 Files with no reviewable changes (1)
- contracts/src/arbitration/KlerosGovernor.sol
🧰 Additional context used
🧠 Learnings (1)
📚 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/src/arbitration/dispute-kits/DisputeKitSybilResistant.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: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (4)
5-6: LGTM!The import paths have been correctly updated to reflect the refactored structure.
23-23: LGTM!The version bump to 2.0.0 is appropriate for the breaking changes in this release (ownership model change, interface method rename, initializer signature update).
60-64: LGTM!The upgrade authorization correctly transitions from
onlyByGovernortoonlyByOwner, aligning with the ownership model refactor across the codebase.
70-78: LGTM!The
_postDrawCheckfunction correctly:
- Extends the signature with the new
_roundNbVotesparameter- Updates the call to use
poh.isHuman(_juror)instead of the deprecatedisRegistered- Maintains proper short-circuit evaluation by checking parent conditions first
|
PR-Codex overview
This PR focuses on significant codebase updates, including the removal of deprecated contracts, refactoring of function names and parameters, and enhancements to the random number generation interface. It also introduces new components for better markdown rendering and improved dispute handling.
Detailed summary
governorreferences toownerin multiple contracts and scripts.IRNG).Summary by CodeRabbit
New Features
Changes
Bug Fixes
Documentation
Tests
Chores