Devnet Testnet Mainnet Beta upgrade to v0.11.0 #2061
Conversation
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram participant Upgrader as Upgrade Script participant Proxy as Contract Proxy participant Impl as New Implementation Upgrader->>Proxy: Upgrade to new implementation Upgrader->>Impl: Call reinitialize(address _wNative) Impl->>Impl: Set wNative = _wNative Note right of Impl: Contract is now initialized with new wNative address Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ 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). (15)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
contracts/deployments/arbitrumSepolia/DisputeKitGated.json (2)
982-996: Same initializer expansion as Shutter – apply previous checksAll observations from the Shutter artifact (guarding, call-site updates, etc.) apply here verbatim.
1058-1070:reinitializegovernance-guard requirementSee prior note – confirm proper access control.
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json (2)
952-966: Initializer parameterised with_wNative– ensure callers updatedSame action items as previous two artifacts.
1029-1041: Publicreinitialize– confirm access controlReplicates earlier concern.
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.json (1)
1058-1070: Duplicate access-control concern forreinitialize()The same governance / single-use guard discussed above applies here.
contracts/src/arbitration/KlerosCoreBase.sol (1)
671-673: See previous comment – same refactor applies.
🧹 Nitpick comments (9)
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
52-54: Consider adding validation for the _wNative parameter.The
reinitializefunction directly assigns the_wNativeparameter to the state variable without validation. Consider adding a zero-address check to prevent accidental misconfiguration.function reinitialize(address _wNative) external reinitializer(9) { + require(_wNative != address(0), "Invalid wNative address"); wNative = _wNative; }contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
33-35: Consider adding validation for the _wNative parameter.Similar to other dispute kits in this upgrade, the
reinitializefunction lacks validation for the_wNativeparameter. Adding a zero-address check would improve robustness.function reinitialize(address _wNative) external reinitializer(9) { + require(_wNative != address(0), "Invalid wNative address"); wNative = _wNative; }contracts/src/arbitration/KlerosCore.sol (1)
62-64: Consider adding validation for the _wNative parameter.The
reinitializefunction should validate the_wNativeparameter to prevent misconfiguration. The reinitializer(6) version is appropriate for KlerosCore's upgrade history.function reinitialize(address _wNative) external reinitializer(6) { + require(_wNative != address(0), "Invalid wNative address"); wNative = _wNative; }contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
68-70: Consider adding validation for the _wNative parameter.The
reinitializefunction follows the same pattern as other dispute kits but lacks validation. Given this contract's complexity with token gating, robust parameter validation is especially important.function reinitialize(address _wNative) external reinitializer(9) { + require(_wNative != address(0), "Invalid wNative address"); wNative = _wNative; }contracts/CHANGELOG.md (1)
7-13: Consider formatting consistency for the new bullet pointsThe new “Breaking” bullets add
_wNativebut the token name is not wrapped in back-ticks the way the contract names are, which breaks the previous markdown style and makes the parameter name less distinguishable.- **Breaking:** Add an extra wNative parameter to the initializer … + **Breaking:** Add an extra `wNative` parameter to the initializer …Same for the constructor bullet below.
Purely cosmetic, but keeps the changelog uniformly readable.contracts/deployments/arbitrumSepolia/DisputeKitShutter.json (1)
1189-1201: Minor: consider documentingwNative()in user-facing docs.A new public getter is exposed. Add it to README/API docs so integrators can discover it easily.
contracts/src/arbitration/KlerosCoreBase.sol (3)
808-809: Unified fee-transfer helper improves clarity; add zero-amount early return?
_transferFeeTokensimplifies the ETH/ERC20 branch, but it’s still called inside tight loops.
A micro-optimization:function _transferFeeToken(IERC20 _feeToken, address payable _recipient, uint256 _amount) internal { + if (_amount == 0) return; // save gas & skip needless external callTotally optional.
842-846: Gas hint: unchecked multiplication in_applyCoherence.The product
_amount * _degreeOfCoherenceis already bounded by 256-bit math-checked overflow; if you’re confident both operands are ‹ 1e77, wrapping the mul/div inunchecked {}cuts 3–5 gas per call in hot loops.
1048-1074: Helper trio looks solid; consider internal library extraction later.
_transferFeeToken,_applyCoherence, and_calculatePnkAtStakeare good first steps toward de-duplication.
If similar logic exists in DisputeKit contracts after this PR, factoring them into a small internal library would avoid future copy-paste.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
contracts/CHANGELOG.md(3 hunks)contracts/README.md(2 hunks)contracts/deploy/upgrade-all.ts(1 hunks)contracts/deployments/arbitrumSepolia/DisputeKitClassic.json(4 hunks)contracts/deployments/arbitrumSepolia/DisputeKitGated.json(4 hunks)contracts/deployments/arbitrumSepolia/DisputeKitGatedShutter.json(4 hunks)contracts/deployments/arbitrumSepolia/DisputeKitShutter.json(4 hunks)contracts/deployments/arbitrumSepolia/KlerosCore.json(4 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json(4 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.json(4 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.json(4 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json(4 hunks)contracts/deployments/arbitrumSepoliaDevnet/KlerosCore.json(4 hunks)contracts/scripts/storage-layout.ts(1 hunks)contracts/src/arbitration/KlerosCore.sol(2 hunks)contracts/src/arbitration/KlerosCoreBase.sol(7 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: the variables 'arbitrablechainid' and 'externaldisputeid' are required by the context to have upperc...
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/utils/getDispute.ts:38-40 Timestamp: 2024-10-21T10:32:16.970Z Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'. Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.solcontracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.solcontracts/src/arbitration/dispute-kits/DisputeKitGated.solcontracts/src/arbitration/dispute-kits/DisputeKitClassic.solcontracts/deployments/arbitrumSepolia/DisputeKitClassic.json
📚 Learning: in the `kleros-v2` codebase, the property `totalresolveddisputes` should remain and should not be re...
Learnt from: kemuru PR: kleros/kleros-v2#1702 File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11 Timestamp: 2024-10-08T16:23:56.291Z Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`. Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.solcontracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.solcontracts/src/arbitration/dispute-kits/DisputeKitGated.solcontracts/src/arbitration/dispute-kits/DisputeKitShutter.solcontracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
📚 Learning: in the kleros v2 codebase, using -1 as an initial value for choice tracking is preferred over undefi...
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1839 File: web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx:141-149 Timestamp: 2025-01-17T11:11:32.535Z Learning: In the Kleros v2 codebase, using -1 as an initial value for choice tracking is preferred over undefined on the client-side to explicitly indicate that no option has been selected. This value is only used internally and never reaches the contract. Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.solcontracts/src/arbitration/dispute-kits/DisputeKitClassic.solcontracts/src/arbitration/KlerosCore.sol
📚 Learning: in typescript code using ethers.js version 6, `contract.target` should be used instead of `contract....
Learnt from: jaybuidl PR: kleros/kleros-v2#1620 File: contracts/test/arbitration/draw.ts:84-84 Timestamp: 2024-11-05T11:32:11.238Z Learning: In TypeScript code using ethers.js version 6, `contract.target` should be used instead of `contract.address` to access a contract's address. Applied to files:
contracts/scripts/storage-layout.ts
📚 Learning: in `contracts/config/courts.v2.mainnet-neo.json`, the `minstake` parameter is denominated in pnk, no...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Applied to files:
contracts/src/arbitration/KlerosCoreNeo.solcontracts/README.mdcontracts/src/arbitration/KlerosCore.solcontracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: in `contracts/config/courts.v2.mainnet-neo.json`, the general court (id: 1) intentionally references...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. Applied to files:
contracts/src/arbitration/KlerosCoreNeo.solcontracts/deployments/arbitrumSepolia/DisputeKitGated.json
📚 Learning: in `usegenesisblock.ts`, within the `useeffect` hook, the conditions (`isklerosuniversity`, `isklero...
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/deploy/upgrade-all.ts
📚 Learning: the prettier-plugin-solidity plugin is installed in the kleros-v2 repository, even though it's not v...
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/README.md
📚 Learning: in solidity, a public state variable automatically implements a corresponding external view function...
Learnt from: jaybuidl PR: kleros/kleros-v2#1805 File: contracts/src/proxy/UUPSProxiable.sol:127-129 Timestamp: 2025-01-22T00:55:35.842Z Learning: In Solidity, a public state variable automatically implements a corresponding external view function with the same name. For example, `string public constant override version = "0.8.0"` implements the abstract `function version() external view returns (string memory)`. Applied to files:
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.jsoncontracts/deployments/arbitrumSepolia/DisputeKitGated.jsoncontracts/deployments/arbitrumSepolia/DisputeKitClassic.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.jsoncontracts/deployments/arbitrumSepolia/DisputeKitGatedShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.jsoncontracts/deployments/arbitrumSepolia/KlerosCore.jsoncontracts/deployments/arbitrumSepolia/DisputeKitShutter.jsoncontracts/deployments/arbitrumSepoliaDevnet/KlerosCore.json
📚 Learning: in the court hierarchy, child courts' `minstake` must be greater than or equal to their parent court...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. Applied to files:
contracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: in `contracts/config/courts.v2.mainnet-neo.json`, the general court (id: 1) can have its `parent` id...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. Applied to files:
contracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: in the `coherency` component (`web/src/pages/home/topjurors/jurorcard/coherency.tsx`), `totalresolve...
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:
contracts/src/arbitration/KlerosCoreBase.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). (11)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (42)
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
21-21: Version bump looks good.The version update from "0.10.0" to "0.12.0" aligns with the coordinated upgrade across the arbitration system. This contract already has the
_wNativeparameter in itsinitializefunction, so no additional reinitialize function is needed.contracts/scripts/storage-layout.ts (1)
7-8: LGTM! Script updated to focus on upgraded contract.The change to target
KlerosCoreNeoinstead ofDisputeKitClassicmakes sense given the significant updates to the core contract in this upgrade.contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
30-30: Version bump aligns with coordinated upgrade.The version update from "0.11.0" to "0.12.0" is consistent with the system-wide upgrade.
contracts/deploy/upgrade-all.ts (2)
95-95: LGTM! WETH deployment retrieval added.Getting the WETH deployment to pass to the reinitialize functions is the correct approach.
97-103: Upgrade calls updated correctly.The changes to use
reinitializewithwETH.addressfor the dispute kits and core contract align perfectly with the contract changes. The selective update (only for contracts with new reinitialize functions) is appropriate.contracts/src/arbitration/KlerosCoreNeo.sol (1)
12-12: Version bump is appropriate.The version update from "0.9.4" to "0.10.0" aligns with the coordinated upgrade across the arbitration system.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
15-15: LGTM - Version bump aligns with upgrade.The version increment from "0.11.1" to "0.12.0" appropriately reflects the addition of the new
reinitializefunction and the breaking change to the initialization interface.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
14-14: LGTM - Consistent version bump across dispute kits.The version increment to "0.12.0" maintains consistency with the other dispute kit contracts in this upgrade.
contracts/src/arbitration/KlerosCore.sol (1)
11-11: LGTM - Appropriate version bump for core contract.The version increment to "0.10.0" appropriately reflects the addition of wrapped native token support to the core arbitration contract.
contracts/README.md (1)
38-41: LGTM - Documentation accurately reflects contract upgrades.The implementation addresses have been properly updated for all contracts affected by the
wNativeupgrade across both testnet and devnet environments. Proxy addresses remain unchanged as expected for upgradeable contracts.Also applies to: 45-45, 83-87, 93-93
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
31-31: LGTM - Consistent version bump across all dispute kit variants.The version increment to "0.12.0" maintains consistency with the other dispute kit contracts, including the complex gated and shutter functionality variants.
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGatedShutter.json (4)
1059-1064: Validate_wNativeaddress insideinitialize.
initializenow accepts an extra_wNativeargument. Guard against zero-address and ensure it matches the wrapped native token for the target network to avoid bricking fee flows.
1132-1144: Restrictreinitialize()execution.
reinitialize(address _wNative)is externally callable and non-payable, yet the ABI shows no access-control hint. If the Solidity implementation lacks a governor-only or single-usereinitializermodifier, any actor could overwrite the token address and steal fees. Please confirm proper gating.
1189-1201: LGTM – newwNative()accessor is consistent.
1286-1289: Double-check migration stepnumDeployments= 3.Increment indicates an additional proxy upgrade. Make sure the upgrade script actually issued the delegate-call to
reinitialize(seeexecute.methodName) and that the emittedInitialized/Upgradedevents were observed.contracts/deployments/arbitrumSepolia/KlerosCore.json (4)
1672-1679: Confirm_wNativeordering ininitialize.Parameter list changed; any on-chain initializer replay would now revert if scripts still use the old 9-arg layout. Re-audit deployment scripts and tests.
1801-1813: Secure the newreinitialize()path.Same concern as for the dispute kit: ensure
reinitialize(address)is protected (governor-only &reinitializer(2)or similar) to prevent hostile updates.
1924-1936: LGTM –wNative()view exposed for integrations.
2030-2033: Verify 5th deployment correctness.
numDeploymentsbumped to 5 andexecute.methodNameisreinitialize. Confirm the specified WETH address (0xAEE953…) is the canonical wrapped native token on Arbitrum Sepolia and that state migration succeeded.contracts/deployments/arbitrumSepoliaDevnet/KlerosCore.json (4)
1801-1813:reinitialize()must be access-controlled orreinitializer-guardedAdding a public
reinitialize(address)entry point is fine for an upgrade step, but:
- Nothing in the ABI tells us it is restricted.
- If it lacks
reinitializer(…)/onlyGovernorguards, anyone could overwritewNative, bricking ETH→wETH fallback refunds.Double-check the Solidity implementation before pushing to prod.
1924-1936: 👍 NewwNative()view improves discoverabilityExposing the stored wrapped-native address is a welcome DX upgrade.
2030-2042: Deployment metadata: confirm WETH address & version bump
numDeploymentswas incremented andexecute.method = "reinitialize"now points to0x3829A2486d53ee984a0ca2D76552715726b77138. We weren’t able to query its symbol in this environment. Please manually verify that this address is the canonical WETH on Arbitrum-Sepolia-Devnet by running, for example:cast call 0x3829A2486d53ee984a0ca2D76552715726b77138 "symbol()(string)" --rpc-url <ARBITRUM_SEPOLIA_DEVNET_RPC>and confirming it returns
WETH.
1674-1680: Noinitializecall-sites detected – please manually verify updated signature
I ran the suggested search for the old 12-parameterinitialize(...)signature and found no matches, but absence of matches isn’t proof that all call-sites were updated. Please audit the following areas to ensure every invocation now includes the new_wNativeargument:• Deployment and upgrade scripts (e.g.,
scripts/ordeployments/folders)
• Proxy-constructor payloads (JSON files or hardcoded calldata)
• Tests that invokeinitializeonKlerosCore
• Front-end helpers or SDK methods encodinginitializeYou can broaden your search with, for example:
rg -n "initialize\(" -t js -t tscontracts/deployments/arbitrumSepolia/DisputeKitClassic.json (4)
956-962: Initializer signature changed – update factory/SDK helpers
initialize(address _governor, address _core, address _wNative)replaces the old two-param variant.
Ensure helper libs (e.g. kleros-sdk) encode the new ABI.
1029-1041: Samereinitialize()exposure risk as core contractVerify that the Solidity uses
reinitializer(2)and an appropriate auth modifier (Governor/Guardian).
Otherwise attackers could freely swap the wrapped token.
1086-1098: AccessorwNative()is a safe, useful additionNo concerns; adds clarity for integrators.
1183-1194: Please verify the devnet WETH address in your deploymentThe
DisputeKitClassic.jsonartifact now hasnumDeployments: 6and itsexecute.methodName = "reinitialize"uses the argument
0xAEE953CC26DbDeA52beBE3F97f281981f2B9d511.
If this isn’t the correct WETH token on your devnet, appeal‐fund refunds will fail.• File:
contracts/deployments/arbitrumSepolia/DisputeKitClassic.json
– Atexecute.args[0]Please confirm this address by querying its
symbol()(e.g., viacast,ethers.js, or your chain explorer) and ensure it matches your devnet WETH.contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json (3)
1189-1200: New public getterwNative()addedLooks good and keeps storage discoverable.
1292-1295: Upgrade payload switched toreinitializeLGTM, provided the
_wNativeaddress (0x3829…7138) is indeed the deployed WETH/Wrapped-Native for Arbitrum Sepolia Devnet.
1132-1144: Ensurereinitializeis restricted to governor onlyThe
reinitialize(address _wNative)function in all implementations is marked only with OpenZeppelin’sreinitializer(which prevents repeat calls) but has no governor–only guard. A malicious actor could front-run the proxy upgrade and callreinitializefirst, pointingwNativeto an attacker-controlled token.Please add the
onlyByGovernormodifier to eachreinitializeentrypoint:
- contracts/src/arbitration/KlerosCore.sol
- function reinitialize(address _wNative) external reinitializer(6) {
- function reinitialize(address _wNative) external onlyByGovernor reinitializer(6) {
wNative = _wNative;
}
- contracts/src/arbitration/dispute-kits/DisputeKitGated.sol
- function reinitialize(address _wNative) external reinitializer(9) {
- function reinitialize(address _wNative) external onlyByGovernor reinitializer(9) {
wNative = _wNative;
}
- contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
- function reinitialize(address _wNative) external reinitializer(9) {
- function reinitialize(address _wNative) external onlyByGovernor reinitializer(9) {
wNative = _wNative;
}This ensures only governance can update the wrapped-native token address during an upgrade.
Likely an incorrect or invalid review comment.
contracts/deployments/arbitrumSepolia/DisputeKitGated.json (2)
1115-1127:wNative()getter additionNo issues.
1218-1222: Execute section now callsreinitializewith0xAEE9…d511Verify this is the correct wrapped token for Arbitrum Sepolia network.
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitClassic.json (2)
1086-1098: GetterwNative()additionLooks correct.
1189-1193: Upgrade script usesreinitializewith0x3829…7138Cross-check that this token address is valid on Devnet.
contracts/deployments/arbitrumSepolia/DisputeKitGatedShutter.json (3)
1056-1069: ABI-level breaking change – double-check all deploy/upgrade scripts
initialize()now expects three parameters (_governor,_core,_wNative).
Old scripts that still pass two arguments will revert. Ensure:
- Hard-hat/Foundry upgrade tasks were updated for every environment (not just devnet).
- Front-end / SDK helpers that craft the calldata for proxy initialisation are bumped.
Otherwise new deployments will brick at step 1.
1189-1200: Public getter forwNativeis a welcome additionExposing the wrapped native token via an auto-generated getter will simplify off-chain
indexing and front-end configuration.
1132-1144: Add governor‐only access control toreinitialize()The
reinitializer(n)modifier already prevents multiple executions, but it does not restrict who can callreinitialize(). As a result, any external account can invoke it once and point the contract to an arbitrary_wNative, breaking token transfers. You should gate this function behind your governor (or owner) role in all dispute kits and core contracts:
- contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol:
reinitializeat line 68- contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol:
reinitializeat line 52- contracts/src/arbitration/dispute-kits/DisputeKitGated.sol:
reinitializeat line 49- contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol:
reinitializeat line 33- contracts/src/arbitration/KlerosCoreNeo.sol:
reinitializeat line 73- contracts/src/arbitration/KlerosCore.sol:
reinitializeat line 62Example change:
- function reinitialize(address _wNative) external reinitializer(9) { + function reinitialize(address _wNative) + external + onlyGovernor + reinitializer(9) { … }Likely an incorrect or invalid review comment.
contracts/deployments/arbitrumSepoliaDevnet/DisputeKitGated.json (2)
975-990: Same initialise signature change – ensure consistency across all kits
DisputeKitGatedmirrors the parameter extension seen in GatedShutter. Re-run any
fixtures/tests that rely on the previous two-arg version.
1115-1126: GetterwNative()exposedGood to see parity with the other dispute kit.
contracts/deployments/arbitrumSepolia/DisputeKitShutter.json (2)
1059-1064: Confirm constructor-time call sites were updated to pass_wNative.
initializenow takes 3 params with_wNativeappended.
Please double-check that:
- All deployment/upgrade scripts that still rely on
initializewere updated to include the extra argument (many scripts now switch toreinitialize, but some chains might still callinitializefor fresh deployments).- The argument order matches the Solidity signature (
_governor, _core, _wNative). A wrong ordering will silently deploy a mis-configured proxy.
1132-1144: Reinitializer annotation and upgrade script usage verified
- In DisputeKitShutter.sol,
initializeis markedreinitializer(1)and the newreinitialize(address)is correctly annotatedreinitializer(9), ensuring the version jump.- The
contracts/deploy/upgrade-all.tsscript invokes onlyreinitialize(notinitialize) fordisputeKitShutter.No further action needed.
contracts/src/arbitration/KlerosCoreBase.sol (1)
541-543: 👍 Refactored PNK stake formula into helper.Centralising the
minStake * alpha / ALPHA_DIVISORcomputation via_calculatePnkAtStakeremoves duplication and reduces risk of future drifting logic.
| "internalType": "contract KlerosCore", | ||
| "name": "_core", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "address", | ||
| "name": "_wNative", | ||
| "type": "address" | ||
| } | ||
| ], | ||
| "name": "initialize", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Initializer signature changed – audit all call-sites
initialize now expects _wNative.
Double-check every script/contract that still calls the 2-arg version; they will now revert.
🏁 Script executed:
#!/bin/bash # Find any stale 2-arg calls in TS/solidity. rg -n --json -e 'initialize\([^,)]*,[^,)]*\)' | jq -r '.data.path.text' | headLength of output: 783
Audit and update initialize call-sites for the new _wNative parameter
The initialize signature has changed to:
function initialize(address _core, address _wNative) …Any invocations still passing only the old arguments will now revert. Please update all call-sites — at minimum:
- contracts/src/arbitration/university/SortitionModuleUniversity.sol
- contracts/deployments/arbitrumSepolia/RandomizerRNG_Implementation.json
- contracts/deployments/arbitrum/DisputeKitClassicNeo_Implementation.json
Example diffs:
Solidity:
- sortitionModule.initialize(coreAddress, moduleParams); + sortitionModule.initialize(coreAddress, wNativeAddress);JSON deployment:
- "args": ["0xCoreAddress"] + "args": ["0xCoreAddress", "0xWNativeAddress"]Ensure you supply the correct wrapped-native token address for _wNative.
🤖 Prompt for AI Agents
In contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json around lines 1056 to 1069, the initialize function signature has been updated to include a new _wNative address parameter. You need to find all call-sites of this initialize function, especially in contracts/src/arbitration/university/SortitionModuleUniversity.sol, contracts/deployments/arbitrumSepolia/RandomizerRNG_Implementation.json, and contracts/deployments/arbitrum/DisputeKitClassicNeo_Implementation.json, and update them to pass the additional _wNative argument with the correct wrapped-native token address. This will prevent reverts caused by missing the new parameter. | function reinitialize(address _wNative) external reinitializer(9) { | ||
| wNative = _wNative; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation for the _wNative parameter.
The reinitialize function should validate that _wNative is not the zero address to prevent accidental misconfiguration.
function reinitialize(address _wNative) external reinitializer(9) { + require(_wNative != address(0), "wNative cannot be zero address"); wNative = _wNative; }Also, please verify that reinitializer version 9 is correct and consistent with the upgrade sequence for this contract.
🏁 Script executed:
#!/bin/bash # 1. Show all initializer/reinitializer usages in DisputeKitGated.sol echo "=== DisputeKitGated.sol initializer/reinitializer occurrences ===" rg -n "initializer\(|reinitializer\(" --color=never contracts/src/arbitration/dispute-kits/DisputeKitGated.sol # 2. Show all reinitializer versions across the entire repo echo echo "=== All reinitializer calls in the repository ===" rg -n "reinitializer\(" --color=neverLength of output: 266682
Require non-zero _wNative in reinitialize (DisputeKitGated.sol:49)
- Add an explicit check to prevent passing
address(0)toreinitialize. - Confirmed that
reinitializer(9)matches the upgrade sequence used by the other dispute kits (Shutter, GatedShutter).
--- a/contracts/src/arbitration/dispute-kits/DisputeKitGated.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitGated.sol @@ -46,6 +46,7 @@ contract DisputeKitGated is DisputeKitGatedBase { /// @param _wNative Wrapped native token address. function reinitialize(address _wNative) external reinitializer(9) { + require(_wNative != address(0), "wNative cannot be zero address"); wNative = _wNative; }📝 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 reinitialize(address _wNative) external reinitializer(9) { | |
| wNative = _wNative; | |
| } | |
| /// @param _wNative Wrapped native token address. | |
| function reinitialize(address _wNative) external reinitializer(9) { | |
| require(_wNative != address(0), "wNative cannot be zero address"); | |
| wNative = _wNative; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/dispute-kits/DisputeKitGated.sol at lines 49 to 51, add a require statement to ensure the _wNative address passed to the reinitialize function is not the zero address. This prevents invalid initialization by explicitly checking that _wNative is not address(0) before assigning it to the wNative state variable. | function reinitialize(address _wNative) external reinitializer(6) { | ||
| wNative = _wNative; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation for the _wNative parameter.
The reinitialize function should validate that _wNative is not the zero address to prevent accidental misconfiguration during upgrades.
function reinitialize(address _wNative) external reinitializer(6) { + require(_wNative != address(0), "wNative cannot be zero address"); wNative = _wNative; }Let me verify that the reinitializer version 6 is consistent with the contract's upgrade history:
🏁 Script executed:
#!/bin/bash # Description: Check for previous initializer functions in KlerosCoreNeo to verify reinitializer version sequence echo "Searching for previous initializer functions in KlerosCoreNeo..." rg -A 3 "reinitializer|initializer" --type sol | grep -A 3 -B 3 "KlerosCoreNeo\|KlerosCoreBase"Length of output: 264
🏁 Script executed:
#!/bin/bash # Search for all initializer and reinitializer usages across .sol files echo "Searching for initializer/reinitializer functions..." rg -n "initializer\s*\(|reinitializer\s*\(" --glob "*.sol"Length of output: 4743
Add non-zero address validation to reinitialize()
The reinitializer sequence in KlerosCoreNeo (v2 → v6) matches the one in KlerosCore.sol, so there’s no gap or version conflict. However, we should still guard against setting wNative to the zero address:
• File: contracts/src/arbitration/KlerosCoreNeo.sol
• Line: 73
function reinitialize(address _wNative) external reinitializer(6) { + require(_wNative != address(0), "wNative cannot be the zero address"); wNative = _wNative; }📝 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 reinitialize(address _wNative) external reinitializer(6) { | |
| wNative = _wNative; | |
| } | |
| function reinitialize(address _wNative) external reinitializer(6) { | |
| require(_wNative != address(0), "wNative cannot be the zero address"); | |
| wNative = _wNative; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreNeo.sol at lines 73 to 75, the reinitialize function sets wNative without validating the input address. Add a require statement to ensure _wNative is not the zero address before assigning it to wNative, preventing accidental or malicious setting of an invalid address. There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
contracts/deployments/arbitrum/SortitionModuleNeo.json (1)
128-141: _courtID made indexed – topic ordering has changed
StakeDelayednow indexes_courtIDas the second topic.
Existing log-parsers that assumed only the juror address was indexed will produce wrong court-level aggregations.Update the topic-decoder or storage schema accordingly.
♻️ Duplicate comments (1)
contracts/deployments/arbitrum/KlerosCoreNeo.json (1)
1749-1754: Initializer expanded – same breaking-change caveat
Like the dispute kit,_wNativeis appended here. Verify every proxydatablob built at deployment encodes the extra parameter or the transaction will revert.
🧹 Nitpick comments (3)
.gitignore (1)
81-85: Duplicate.envpatterns – clean up for clarityLines 81-85 already ignore specific
.env*files.
Lines 206-209 introduce a broader.env*rule plus selective re-inclusion. Keeping both sets is redundant and may confuse future maintainers.-# dotenv environment variables file -.env -.env.test -.env.production + +# dotenv environment variables files – ignore all except the project-wide ones +.env* +!.env.project +!.env.vaultThis single block covers every case while documenting the exceptions in one place.
Also applies to: 206-209
contracts/deployments/arbitrum/SortitionModuleNeo.json (2)
1042-1080: NewvalidateStakehelper added – consider usingviewinstead ofnonpayable
validateStakeperforms a pure calculation and returns three values; it doesn’t modify state.
Marking itviewwould make it clearer, cheaper, and callable from static-call contexts.
1094-1106: Leftover PNK withdrawal API – missing indexed_amountin event
withdrawLeftoverPNKexposes withdrawal amounts but the pairedLeftoverPNKWithdrawnevent does not index_amount.
Indexing the amount would enable efficient range queries on large datasets.Consider adding a third indexed field if topic space allows, or confirm the trade-off is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.dockerignore(1 hunks).gitignore(1 hunks)contracts/CHANGELOG.md(3 hunks)contracts/README.md(3 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(2 hunks)contracts/deployments/arbitrum/DisputeKitClassicNeo.json(7 hunks)contracts/deployments/arbitrum/DisputeKitGatedNeo.json(1 hunks)contracts/deployments/arbitrum/DisputeKitGatedNeo_Proxy.json(1 hunks)contracts/deployments/arbitrum/DisputeKitGatedShutterNeo.json(1 hunks)contracts/deployments/arbitrum/DisputeKitGatedShutterNeo_Proxy.json(1 hunks)contracts/deployments/arbitrum/DisputeKitShutterNeo.json(1 hunks)contracts/deployments/arbitrum/DisputeKitShutterNeo_Proxy.json(1 hunks)contracts/deployments/arbitrum/KlerosCoreNeo.json(6 hunks)contracts/deployments/arbitrum/SortitionModuleNeo.json(11 hunks)subgraph/core-neo/subgraph.template.yaml(2 hunks)subgraph/core-neo/subgraph.yaml(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- .dockerignore
- contracts/deployments/arbitrum/DisputeKitShutterNeo_Proxy.json
- contracts/deployments/arbitrum/DisputeKitGatedNeo_Proxy.json
- contracts/deployments/arbitrum/DisputeKitGatedNeo.json
- contracts/deployments/arbitrum/DisputeKitGatedShutterNeo_Proxy.json
- contracts/deployments/arbitrum/DisputeKitGatedShutterNeo.json
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/README.md
- contracts/CHANGELOG.md
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. 📚 Learning: in `usegenesisblock.ts`, within the `useeffect` hook, the conditions (`isklerosuniversity`, `isklero...
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/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: the variables 'arbitrablechainid' and 'externaldisputeid' are required by the context to have upperc...
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/utils/getDispute.ts:38-40 Timestamp: 2024-10-21T10:32:16.970Z Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'. Applied to files:
contracts/deploy/00-home-chain-arbitration-neo.tscontracts/deployments/arbitrum/DisputeKitClassicNeo.json
📚 Learning: in `contracts/config/courts.v2.mainnet-neo.json`, the general court (id: 1) intentionally references...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. Applied to files:
contracts/deploy/00-home-chain-arbitration-neo.tscontracts/deployments/arbitrum/KlerosCoreNeo.jsonsubgraph/core-neo/subgraph.template.yamlcontracts/deployments/arbitrum/SortitionModuleNeo.json
📚 Learning: in `web/src/hooks/queries/usepopulateddisputedata.ts`, the query and subsequent logic only execute w...
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/deploy/00-home-chain-arbitration-neo.tscontracts/deployments/arbitrum/DisputeKitClassicNeo.json
📚 Learning: in `kleros-sdk/src/sdk.ts`, the `publicclient` type is used and should not be flagged as unused....
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/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: in this typescript project, when a file (such as `kleros-sdk/src/requests/gqlclient.ts`) exports onl...
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/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: in the `contracts/src/rng/chainlinkrng.sol` contract, the `requestrandomness` function is restricted...
Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/ChainlinkRNG.sol:135-153 Timestamp: 2024-12-07T11:39:10.927Z Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context. Applied to files:
contracts/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: in the landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `use...
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:
contracts/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: next.js recommends using the `useeffect` hook to set `isclient` and using `suppresshydrationwarning`...
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:
contracts/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: in `contracts/config/courts.v2.mainnet-neo.json`, the `minstake` parameter is denominated in pnk, no...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Applied to files:
contracts/deployments/arbitrum/KlerosCoreNeo.jsoncontracts/deployments/arbitrum/SortitionModuleNeo.json
📚 Learning: in solidity, a public state variable automatically implements a corresponding external view function...
Learnt from: jaybuidl PR: kleros/kleros-v2#1805 File: contracts/src/proxy/UUPSProxiable.sol:127-129 Timestamp: 2025-01-22T00:55:35.842Z Learning: In Solidity, a public state variable automatically implements a corresponding external view function with the same name. For example, `string public constant override version = "0.8.0"` implements the abstract `function version() external view returns (string memory)`. Applied to files:
contracts/deployments/arbitrum/KlerosCoreNeo.jsoncontracts/deployments/arbitrum/DisputeKitClassicNeo.json
📚 Learning: in `kleros-sdk/src/datamappings/utils/actiontypes.ts`, the `abi` field in `abieventmapping` is inten...
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:29-29 Timestamp: 2024-10-22T09:36:10.478Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiEventMapping` is intentionally typed as `string` because it is parsed later in the action functions. Applied to files:
subgraph/core-neo/subgraph.template.yamlsubgraph/core-neo/subgraph.yaml
📚 Learning: in `contracts/config/courts.v2.mainnet-neo.json`, the general court (id: 1) can have its `parent` id...
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. Applied to files:
contracts/deployments/arbitrum/SortitionModuleNeo.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
🔇 Additional comments (18)
contracts/deploy/00-home-chain-arbitration-neo.ts (3)
9-9: LGTM on import cleanup.The removal of
RandomizerRNGimport aligns with the code cleanup as this import is no longer used in the deployment script.
13-13: LGTM on destructuring cleanup.Removing the unused
executemethod from the destructureddeploymentsobject simplifies the code and removes unnecessary imports.
126-147: Deployment of new dispute kits verifiedAll three contracts—DisputeKitShutter, DisputeKitGated, and DisputeKitGatedShutter—are present under
contracts/src/arbitration/dispute-kits/and define an initializer with signatureinitialize(address _governor, KlerosCore _core, address _wNative)which matches the deployment calls
args: [deployer, core.target, weth.target]and each is correctly registered via
core.addNewDisputeKit(...). No further changes are needed.contracts/deployments/arbitrum/DisputeKitShutterNeo.json (2)
1133-1144: LGTM on reinitialize function addition.The ABI correctly includes the new
reinitialize(address _wNative)function that accepts the wrapped native token address parameter, which aligns with the broader upgrade pattern described in the AI summary.
1282-1285: Constructor arguments verified – all addresses match expected deployment valuesIn contracts/deployments/arbitrum/DisputeKitShutterNeo.json, the constructor arguments have been confirmed against the deployment artifacts:
implementationArg:
• 0xF3103B46403A0bBd4551648BFb29BCC2b8783947
(matches DisputeKitShutterNeo_Implementation.json)initialize parameters:
• deployer: 0xf1C7c037891525E360C59f708739Ac09A7670c59
(matches the transaction’sfromaddress)
• core proxy: 0x991d2df165670b9cac3B022f4B68D65b664222ea
(matches KlerosCoreNeo_Proxy.json)
• wNative (WETH): 0x82aF49447D8a07e3bd95BD0d56f35241523fBab1
(matches WETH.json)All values align with the expected deployment script—no changes required.
subgraph/core-neo/subgraph.template.yaml (2)
109-110: LGTM on ABI migration reference.The update to point to a local ABI migration file (
./abi-migrations/KlerosCoreNeo.json) instead of the deployment path ensures the subgraph uses the correct ABI with event additions, as noted in the comment.
125-238: DisputeKitShutter, DisputeKitGated, and DisputeKitGatedShutter event handlers and ABI references are correctly configured
- Verified deployment JSONs exist under
contracts/deployments/arbitrum/:
- DisputeKitShutterNeo.json
- DisputeKitGatedNeo.json
- DisputeKitGatedShutterNeo.json
- Confirmed mapping file
subgraph/core-neo/src/DisputeKitClassic.tsexports all required handlers:
- handleDisputeCreation
- handleContributionEvent
- handleWithdrawal
- handleChoiceFunded
- handleVoteCast
- handleCommitCast
No further changes needed—everything lines up as expected.
subgraph/core-neo/subgraph.yaml (3)
3-4: LGTM on metadata additions.Adding description and repository fields improves the subgraph's documentation and makes it easier to identify and track.
110-125: LGTM on event handler additions for DisputeKitClassic.The explicit addition of event handlers for
DisputeCreation,Contribution,Withdrawal,ChoiceFunded,VoteCast, andCommitCastensures comprehensive indexing of dispute kit events.
126-239: Confirm startBlock values in subgraph mappingsWe’ve verified that all three contract addresses in subgraph/core-neo/subgraph.yaml
– DisputeKitShutter (0x9D3e3f1765744c2a1BC6F6088549770444BBC768),
– DisputeKitGated (0xaE1eed20C125B739b64c948820C61F809ad9a925), and
– DisputeKitGatedShutter (0x788330092B9704809C19858E39EB9Ac402c2E47b)
✓ match their deployment artifacts
✓ are valid Ethereum address formatsPlease double-check that the corresponding startBlock values in subgraph/core-neo/subgraph.yaml accurately reflect the on-chain block numbers when each contract went live:
- source.startBlock for DisputeKitShutter (364034407)
- source.startBlock for DisputeKitGated (364034429)
- source.startBlock for DisputeKitGatedShutter (364034451)
Verify these against your on-chain deployment records or block explorer to ensure the subgraph will index from the correct block.
contracts/deployments/arbitrum/DisputeKitClassicNeo.json (1)
1086-1098: 👍 Clear accessor forwNative
The new view accessor is straightforward and enables off-chain clients to fetch the token address.contracts/deployments/arbitrum/KlerosCoreNeo.json (3)
1956-1973: NewtransferBySortitionModule()requires interface sync
Sortition module contracts must implement the complementary call site; otherwise draws will revert. Confirm the module was updated in this PR and deployment JSONs reflected.
2012-2024: Accessor forwNative– good addition
This small view keeps external integrations simple.
1399-1407: Ignore draw() return-type change for SDK & mappings
We’ve searched the entire repo and found.draw(only in test and script files—no calls in the TS/JS SDK or in subgraph mappings. Because Ethers treats state-mutating functions as transactions (ignoring return values), existing tests and scripts will continue working without error. No updates to SDK code or subgraph mappings are needed.Likely an incorrect or invalid review comment.
contracts/deployments/arbitrum/SortitionModuleNeo.json (4)
68-85: ABI event rename – verify off-chain indexers are updated
StakeDelayedAlreadyTransferredDeposited/Withdrawnhave been consolidated into a singleLeftoverPNK/LeftoverPNKWithdrawnpair.
Any subgraph, backend, or analytics pipeline listening to the old event signatures will silently break.[action required] Update/upgrade every consumer that filters for the previous event names or topic hashes.
Failing to do so will result in missed leftover-PNK accounting and potentially stale UIs.
108-117: NewPhase parameter type changed to enum – confirm decoding libraries
_phaseis now declared asenum ISortitionModule.Phase(uint8 under the hood).
Some ABI-parsing libraries (ethers-v5 pre-5.7, web3-js 1.x) have trouble with enuminternalTypestrings that include an interface prefix.Double-check that your client stack decodes this field correctly; otherwise fall back to treating it as
uint8.
598-603: Proxy executesinitialize4()with zero arguments – sanity-check initialization
initialize4takes no params, yet the oldinitialize(with full governor/core/RNG config) is still present.
Ensure the proxy was previously initialized; otherwise critical state (governor, RNG, limits) will stay zeroed.If this is intended as a no-op version bump, add an explicit comment in deployment scripts to avoid confusion during future audits.
787-800:penalizeStakereturn signature changed – propagate to callersThe function now returns a tuple
(pnkBalance, availablePenalty).
Contracts/routers or off-chain services expecting the old zero-return version will mis-handle the call data size and revert.Search & upgrade every invocation site.
| }, | ||
| { | ||
| "internalType": "address", | ||
| "name": "_wNative", | ||
| "type": "address" | ||
| } | ||
| ], | ||
| "name": "initialize", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Initializer signature change propagates a breaking interface
initialize() now requires _wNative. Down-stream deployment scripts, tests, and factories that still pass two parameters will revert. Double-check every invocation (hard-hat scripts, upgrade tasks, sub-graph templates) is updated.
🏁 Script executed:
#!/bin/bash # Find every call site that still builds an "initialize(" with 2 params. rg --pcre2 -n $'initialize\\([^,]+,[^,]+\\)' | headLength of output: 299122
🏁 Script executed:
#!/bin/bash # Search for direct .initialize( invocations in JS/TS deployment or test scripts rg -n --glob '*.[jt]s' '\.initialize\(' | head -n 20 # Search for Hardhat deployProxy calls that specify initializer functions rg -n --glob '*.[jt]s' 'deployProxy' | head -n 20Length of output: 470
Update initialization calls to include the new _wNative parameter
The initialize(...) signature on your upgraded KlerosCore implementation now expects an extra address _wNative argument. Any script, factory, or test that still invokes initialize(...) with fewer parameters will break.
Callsites needing update:
contracts/test/proxy/index.ts- Line 50:
proxy.connect(deployer).initialize(deployer.address) - Line 55:
proxy.connect(user1).initialize(user1.address) - Line 60:
implementation.initialize(user1.address)
- Line 50:
Action items:
- Pass the wrapped native token address (
_wNative) as the last argument in eachinitialize(...)call. - Audit any other deployment scripts or factories for
initialize(...)invocations and update accordingly.
🤖 Prompt for AI Agents
In contracts/test/proxy/index.ts around lines 50, 55, and 60, the initialize(...) calls are missing the new _wNative parameter required by the updated KlerosCore implementation. Update these calls to include the wrapped native token address as the last argument. Additionally, review other deployment scripts and factories for initialize(...) calls and add the _wNative argument where needed to match the new function signature. | { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "address", | ||
| "name": "_wNative", | ||
| "type": "address" | ||
| } | ||
| ], | ||
| "name": "reinitialize", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Lock-down reinitialize() – potential privilege escalation
reinitialize(address _wNative) is non-payable & public. Unless it is gated by onlyGovernor/onlyProxy and an AlreadyInitialized guard, any account could swap the wrapped native token reference after deployment, breaking fee accounting.
Recommend enforcing both protections in Solidity:
- function reinitialize(address _wNative) external { + function reinitialize(address _wNative) external onlyGovernor whenNotInitialized { _setWNative(_wNative); }Verification script to confirm a modifier guard exists:
🏁 Script executed:
#!/bin/bash # Expect: at least one of "onlyGovernor", "onlyOwner" or "_checkGovernor" near reinitialize. rg -n --context 2 $'function reinitialize' | headLength of output: 684
Restrict reinitialize() to Governor
reinitializer(6) prevents multiple calls but does not restrict who can invoke it. Until version 6 is set, any account can call reinitialize() and swap out wNative, potentially breaking fee accounting. Enforce a governance check by adding the onlyGovernor modifier:
Files & locations to update:
- contracts/src/arbitration/KlerosCoreNeo.sol (around line 73)
- contracts/src/arbitration/KlerosCore.sol (around line 62)
Suggested diff:
- function reinitialize(address _wNative) external reinitializer(6) { + function reinitialize(address _wNative) external reinitializer(6) onlyGovernor { wNative = _wNative; }This ensures only the Governor role can perform the reinitialization.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreNeo.sol around line 73 and contracts/src/arbitration/KlerosCore.sol around line 62, the reinitialize() function lacks access control, allowing any account to call it before version 6 is set. To fix this, add the onlyGovernor modifier to the reinitialize() function declaration in both files to restrict its invocation exclusively to the Governor role, ensuring secure governance control over reinitialization. | { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "address", | ||
| "name": "_wNative", | ||
| "type": "address" | ||
| } | ||
| ], | ||
| "name": "reinitialize", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, |
There was a problem hiding this comment.
reinitialize() access-control audit needed
Same risk as in the dispute kit: an unprotected call lets an attacker redirect native-token wrapping and drain balances via fee reimbursements. Ensure the implementation checks msg.sender == governor (or guardian) and prevents multiple executions.
🤖 Prompt for AI Agents
In contracts/deployments/arbitrum/KlerosCoreNeo.json around lines 1889 to 1901, the reinitialize() function lacks access control, allowing unauthorized calls that could redirect native-token wrapping and cause fund loss. Add a check to ensure msg.sender is the governor or guardian before executing the function, and implement a mechanism to prevent multiple executions, such as a boolean flag that marks the function as already called. 
|
jaybuidl changed the title jaybuidl changed the title 
PR-Codex overview
This PR focuses on upgrading the Kleros smart contracts to version
0.11.0, introducing new dispute kits, and modifying existing functions to accommodate an additional parameter (_wNative). It also includes updates to various files and configurations for improved functionality.Detailed summary
0.10.0to0.11.0incontracts/package.json.DisputeKitShutter,DisputeKitGated, andDisputeKitGatedShutter.initialize7toreinitializeacross multiple contracts._wNativeparameter in various initializers and functions.KlerosCoreBasefor better handling of ETH and ERC20 tokens.Summary by CodeRabbit
New Features
wNative) across core and dispute kit contracts with new reinitialization methods.Improvements
reinitializemethod with wrapped native token parameter.Refactor
Documentation
Style