KlerosCore to allow jumping to a non-classic DK #2114
Conversation
❌ Deploy Preview for kleros-v2-university 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-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a configurable "jump dispute kit" mechanism: dispute kits gain a stored jumpDisputeKitID (with getter/setter), initializers/reinitializers accept a fourth jump ID arg, KlerosCoreBase queries DK.getJumpDisputeKitID() during court jumps (falling back to Classic), and deployment scripts/tests updated to pass and assert jump IDs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Core as KlerosCoreBase
participant Parent as ParentCourt
participant DK as CurrentDK
participant Target as TargetDK
User->>Core: Trigger appeal / court jump
Core->>Parent: Check supports(CurrentDK)
alt CurrentDK unsupported
Core->>DK: getJumpDisputeKitID()
DK-->>Core: jumpDisputeKitID
Core->>Parent: Check supports(jumpDisputeKitID)
alt Supported
Note right of Core #e0f7fa: Use returned jump DK
Core->>Target: Route next round to TargetDK
else Not supported or 0
Note right of Core #fff3e0: Fallback to Classic DK
Core->>Target: Route next round to ClassicDK
end
else Supported
Note right of Core #e8f5e9: Keep current DK
end
Core-->>User: Proceed with next-round DK assignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (13)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🧹 Nitpick comments (5)
contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
130-133: Fix typo and clarify return semantics for jump DK IDTypo: “dispute kid ID” → “dispute kit ID”. Also, please document whether 0 means “use Classic DK (fallback)”, since Core may rely on that behavior.
- /// @dev Returns the dispute kid ID for Kleros Core, that will be used after court jump. - /// @return The ID of the dispute kit in Kleros Core disputeKits array. + /// @dev Returns the dispute kit ID (index in Kleros Core's `disputeKits`) to use after a court jump. + /// Implementations may return 0 to signal "no explicit target" — Core or the DK should then fall back to Classic. + /// @return The ID of the dispute kit in Kleros Core `disputeKits` array. function getJumpDisputeKitID() external view returns (uint256);contracts/deploy/00-home-chain-arbitration.ts (1)
37-41: Make jump DK configurable and assert post-deploy stateHardcoding jumpDisputeKitID=1 is fine for now; consider making it configurable (env/network param) to exercise non-classic paths in staging, and assert via getJumpDisputeKitID() after deploy.
Example (post-deploy checks):
+ // Sanity checks + console.log("Classic jump DK:", await disputeKitContract.getJumpDisputeKitID()); + const shutter = await ethers.getContract("DisputeKitShutter"); + console.log("Shutter jump DK:", await shutter.getJumpDisputeKitID());Note: deployUpgradable executes the initializer on upgrade; ensure DK initializers’ reinitializer versioning won’t revert on existing proxies (see comments in DK files).
Also applies to: 109-110, 117-118, 125-126
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
31-31: Avoid magic number for jump DK; centralize in a shared deploy constant.Hardcoding
1invites drift vs. other deploy scripts and SolidityConstants. Prefer a single TS constant.Apply:
-import { getContractOrDeploy, getContractOrDeployUpgradable } from "./utils/getContractOrDeploy"; +import { getContractOrDeploy, getContractOrDeployUpgradable } from "./utils/getContractOrDeploy"; +import { DISPUTE_KIT_CLASSIC_ID } from "./utils/constants"; @@ - const jumpDisputeKitID = 1; // Classic DK + const jumpDisputeKitID = DISPUTE_KIT_CLASSIC_ID; // Classic DKAdditionally add (new file):
// contracts/deploy/utils/constants.ts export const DISPUTE_KIT_CLASSIC_ID = 1;contracts/test/foundry/KlerosCore.t.sol (1)
239-241: Assert jump DK via getter and storage is valuable; add a setter flow test.Consider a small test exercising
changeJumpDisputeKitIDand verifying the core uses the updated target (and fallback when set to 0).I can add a focused test that: (1) calls
changeJumpDisputeKitID(newId), (2) triggers a jump path to observeDisputeKitJumpusesnewId, and (3) sets0to validate fallback to classic.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
183-188: Emit event and use namespaced storage in setter.
Add transparency and keep upgrade safety if adopting namespaced storage.- function changeJumpDisputeKitID(uint256 _jumpDisputeKitID) external onlyByOwner { - jumpDisputeKitID = _jumpDisputeKitID; - } + function changeJumpDisputeKitID(uint256 _jumpDisputeKitID) external onlyByOwner { + uint256 prev = DisputeKitClassicBaseStorage.layout().jumpDisputeKitID; + DisputeKitClassicBaseStorage.layout().jumpDisputeKitID = _jumpDisputeKitID; + emit JumpDisputeKitIDChanged(prev, _jumpDisputeKitID); + }Also add (outside this hunk, in the events section):
event JumpDisputeKitIDChanged(uint256 previous, uint256 current);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
contracts/deploy/00-home-chain-arbitration-neo.ts(2 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/src/arbitration/KlerosCoreBase.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(5 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
contracts/deploy/00-home-chain-arbitration-neo.ts (2)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/utils.ts (1)
deployments(3-16)
contracts/deploy/00-home-chain-arbitration.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
⏰ 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-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: Redirect rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
contracts/src/arbitration/KlerosCoreBase.sol (1)
1082-1086: LGTM: jump DK selection with safe fallbackThe switch to querying DK.getJumpDisputeKitID() and falling back to Classic if unsupported by parent court looks correct and defensive.
Please add tests for:
- Parent doesn’t support current DK, DK proposes non-classic jump DK supported by parent → switches to that DK.
- Parent doesn’t support current DK and proposed jump DK → falls back to Classic.
- DK returns 0 → Classic fallback still works.
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
45-53: Ignore reinitializer bump suggestion – signature mismatch detectedThe current
DisputeKitGated.solinitializer has only three parameters (_owner,_core,_wNative) and no_jumpDisputeKitIDargument, so the recommendation to bump the reinitializer version or migrate the new field isn’t applicable to this code. The review comment targets a method signature that doesn’t exist on this branch; you can safely disregard it.Likely an incorrect or invalid review comment.
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
64-72: Incorrect upgrade suggestion: initialize signature mismatchThe
initializefunction inDisputeKitGatedShutter.solonly takes three parameters—_owner,_core, and_wNative—and calls__DisputeKitClassicBase_initialize(_owner, _core, _wNative);. There is no_jumpDisputeKitIDargument in this contract, so the suggested diff (adding a fourth parameter and bumping the reinitializer version) and the related NatSpec update do not apply to this file.Please disregard this review comment.
Likely an incorrect or invalid review comment.
contracts/deploy/00-home-chain-arbitration-neo.ts (2)
35-35: Initializer arg extension looks correct.Fourth arg
jumpDisputeKitIDmatches DK initialize(owner, core, wNative, jumpDK).
128-129: Consistent propagation of jump DK to extra DKs.All DK deployments pass the fourth arg; good consistency.
Also applies to: 135-136, 142-143
contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
29-37: Initialize now carries jump DK; forwarding to base is correct.Signature and doc updated; call to
__DisputeKitClassicBase_initialize(..., _jumpDisputeKitID)is appropriate.contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
48-56: Shutter initializer aligned with base; OK.New param is documented and forwarded; no other logic affected.
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
43-44: SybilResistant initializer extended and forwarded correctly.Order
(owner, core, poh, wNative, jumpDK)is clear; base init receives(owner, core, wNative, jumpDK).Also applies to: 49-52
contracts/test/foundry/KlerosCore.t.sol (1)
105-110: Tests updated to 4-arg DK initializer; good coverage.All DK proxy instantiations now encode the extra
uint256correctly.Also applies to: 295-300, 2122-2127, 2947-2952
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
9-9: Good use of central constants.
Importing ONE_BASIS_POINT and DISPUTE_KIT_CLASSIC from Constants keeps consistency with the rest of the codebase.
71-72: Outdated storage‐layout concern: I searched theDisputeKitClassicBase.solfile—including the vicinity of lines 60–90—and found no declaration ofjumpDisputeKitID. The base contract does not introduce that state variable, so there’s no storage‐slot shift risk as described. Please disregard the suggestion to namespace or relocatejumpDisputeKitID, since it isn’t present in the current code.Likely an incorrect or invalid review comment.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
29-37: Initializer signature change OK; verify initializer versioning compatibility.Extending
initializewith_jumpDisputeKitIDis fine. However, usingreinitializer(1)at the top level can clash if the base initializer also usedinitializer/reinitializer(1). Confirm__DisputeKitClassicBase_initializeusesonlyInitializing(not anotherreinitializer(1)) to avoid version-collision reverts.If there’s no need to keep
reinitializer(1)here, prefer the clearerinitializer:- ) external reinitializer(1) { + ) external initializer {contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
64-72: Confirm base initializer signature & update NatSpec for _jumpDisputeKitIDVerified that
__DisputeKitClassicBase_initializeinDisputeKitClassicBase.solindeed declares its fourth parameter asuint256 _jumpDisputeKitIDin the same order (owner, core, wNative, jumpDisputeKitID). To improve clarity, please update the NatSpec on the derived kits to indicate that passing zero will fall back to the Classic kit.– File:
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(and likewise in any other kits)
Apply this diff:- /// @param _jumpDisputeKitID The ID of the dispute kit to switch to after the court jump. + /// @param _jumpDisputeKitID The ID of the dispute kit to switch to after a court jump (0 = fallback to Classic).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
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(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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). (8)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (6)
contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
14-14: Version bump acknowledged; keep artifacts in sync.Looks good. Ensure CHANGELOG, deployment scripts, and any tests asserting
versionare updated to "0.13.0".contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
31-31: Version bump confirmed across all Dispute KitsI ran a repo-wide search and verified that every Dispute Kit contract now uses
version = "0.13.0":
- DisputeKitSybilResistant.sol
- DisputeKitShutter.sol
- DisputeKitGatedShutter.sol
- DisputeKitGated.sol
- DisputeKitClassic.sol
All code is aligned with v0.13.0. Please remember to update any documentation or release notes to reflect v0.13.0 as well.
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (2)
30-30: Version bump acknowledged.0.13.0 constant looks consistent with the PR’s scope.
41-53: Review comment outdated – no 4-arg initializer presentThe current
DisputeKitGated.solimplementation still defines a three-parameterinitialize(governor, core, wNative) and calls the three-arg__DisputeKitClassicBase_initialize. There is no fourthjumpDisputeKitIDargument in either the initializer or the base implementation, so the proposed doc tweak and verification steps do not apply. Please disregard this comment.Likely an incorrect or invalid review comment.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
15-15: Version bump acknowledged (0.13.0).Matches the new DK jump feature scope; no issues spotted.
49-56: Ensure proxy initializers pass the new_jumpDisputeKitIDargument
- Confirmed that
__DisputeKitClassicBase_initializein
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
now accepts four parameters (including_jumpDisputeKitID) at line 152.- No direct
initialize(...)invocations were found in the TypeScript deployment scripts undercontracts/deployorcontracts/scripts.- Please double-check your UUPS proxy deployments (e.g.
upgrades.deployProxyor equivalent) to ensure they’re supplying all four arguments when initializingDisputeKitShutter(and any other dispute kits).- Review and update any test fixtures or migration scripts that call
initializeso they match the new four-parameter signature.
jaybuidl
added
Type: Enhancement ✨
Package: Contracts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/KlerosCoreBase.sol (1)
439-446: Compile-time bug:AcceptedFeeTokenevent is emitted but not declared.This will not compile. Add the missing event declaration.
event DisputeKitEnabled(uint96 indexed _courtID, uint256 indexed _disputeKitID, bool indexed _enable); + event AcceptedFeeToken(IERC20 indexed _feeToken, bool _accepted);
♻️ Duplicate comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
71-72: Governance safety: add event + bound check forjumpDisputeKitID; consider namespaced storage.
- Emit an event when changing this governance-critical parameter.
- Validate
_jumpDisputeKitID < core.getDisputeKitsLength()(allow 0 as sentinel).- Storage: using a namespaced layout avoids upgrade layout risks (reiterating previous suggestion).
@@ - uint256 public jumpDisputeKitID; // The ID of the dispute kit in Kleros Core disputeKits array that the dispute should switch to after the court jump, in case the new court doesn't support this dispute kit. + uint256 public jumpDisputeKitID; // The ID of the dispute kit in Kleros Core disputeKits array that the dispute should switch to after the court jump, in case the new court doesn't support this dispute kit (0 = fallback to Classic). @@ - /// @param _jumpDisputeKitID The ID of the dispute kit to switch to after the court jump. + /// @param _jumpDisputeKitID The ID of the dispute kit to switch to after the court jump (0 = fallback to Classic). function __DisputeKitClassicBase_initialize( address _owner, KlerosCore _core, address _wNative, uint256 _jumpDisputeKitID ) internal onlyInitializing { owner = _owner; core = _core; wNative = _wNative; - jumpDisputeKitID = _jumpDisputeKitID; + require(_jumpDisputeKitID < core.getDisputeKitsLength(), "InvalidJumpDK"); + jumpDisputeKitID = _jumpDisputeKitID; } @@ - /// @dev Changes the dispute kit ID used for the jump. + /// @dev Changes the dispute kit ID used for the jump. /// @param _jumpDisputeKitID The new value for the `jumpDisputeKitID` storage variable. - function changeJumpDisputeKitID(uint256 _jumpDisputeKitID) external onlyByOwner { - jumpDisputeKitID = _jumpDisputeKitID; - } + event JumpDisputeKitIDChanged(uint256 previous, uint256 current); + function changeJumpDisputeKitID(uint256 _jumpDisputeKitID) external onlyByOwner { + require(_jumpDisputeKitID < core.getDisputeKitsLength(), "InvalidJumpDK"); + emit JumpDisputeKitIDChanged(jumpDisputeKitID, _jumpDisputeKitID); + jumpDisputeKitID = _jumpDisputeKitID; + }Note: If you adopt namespaced storage, persist and read
jumpDisputeKitIDvia the namespaced layout in both initializer and setter.Also applies to: 151-162, 189-194
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
652-657: Typo and clarity in doc: “dispute kit ID” + mention 0 sentinel.- /// @dev Returns the dispute kid ID be used after court jump by Kleros Core. - /// @return The ID of the dispute kit in Kleros Core disputeKits array. + /// @dev Returns the dispute kit ID to be used after a court jump by Kleros Core. + /// @return The ID of the dispute kit in Kleros Core disputeKits array (0 = fallback to Classic).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
contracts/src/arbitration/KlerosCoreBase.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(5 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/arbitration/interfaces/IDisputeKit.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). (12)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
9-9: Import ofDISPUTE_KIT_CLASSICis correct for fallback usage.No issues.
jaybuidl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
contracts/deploy/00-home-chain-arbitration.ts (5)
110-110: Prefer consistent address accessors (target vs getAddress).Use a single style to reduce confusion between ethers v6 Contract.target and getAddress().
- args: [deployer, core.target, weth.target, classicDisputeKitID], + args: [deployer, await core.getAddress(), await weth.getAddress(), classicDisputeKitID],
119-119: Consistent address accessors (repeat).- args: [deployer, core.target, weth.target, classicDisputeKitID], + args: [deployer, await core.getAddress(), await weth.getAddress(), classicDisputeKitID],
128-128: Consistent address accessors (repeat).- args: [deployer, core.target, weth.target, disputeKitShutterID], // Does not jump to DKClassic + args: [deployer, await core.getAddress(), await weth.getAddress(), disputeKitShutterID], // Does not jump to DKClassic
108-113: Upgrade path: ensure reinitialize is used on upgrades.deployUpgradable() wires onUpgrade to the same method as init. For future upgrades of DKs (where initialize is an initializer), remember to pass initializer: "reinitialize" alongside newImplementation, otherwise onUpgrade will attempt initialize again and may revert.
If desired, we can extend deployUpgradable to accept a distinct onUpgradeMethodName to avoid per-call overrides. Do you want a follow-up patch?
Also applies to: 117-122, 126-131
37-41: Assert thatcore.disputeKits[classicDisputeKitID]equals the deployed DisputeKitClassic address
After deploying KlerosCore and DisputeKitClassic, add a runtime check instead of looking for a reverse‐lookup function:// After klerosCore and DisputeKitClassic are deployed const coreClassicDKAddress = await core.disputeKits(classicDisputeKitID) if (coreClassicDKAddress.toLowerCase() !== disputeKit.address.toLowerCase()) { throw new Error( `Classic DK registry mismatch: expected address ${disputeKit.address}, got ${coreClassicDKAddress}` ) }This guards against any drift in the core’s
disputeKitsmapping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/deploy/00-home-chain-arbitration-neo.ts(2 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/deploy/00-home-chain-arbitration-neo.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-21T10:32:16.970Z
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.ts
🧬 Code graph analysis (1)
contracts/deploy/00-home-chain-arbitration.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
⏰ 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-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - 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-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
contracts/deploy/00-home-chain-arbitration.ts (1)
40-41: DisputeKitClassic.initialize does not enforce a non-zero core
Theinitialize(address _governor, KlerosCore _core, address _wNative)implementation contains norequire(_core != address(0))check, so it will acceptcore = address(0)and simply store it. Any subsequent calls gated byonlyByCorewill revert untilchangeCore()is invoked with a valid KlerosCore address.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/src/arbitration/KlerosCoreBase.sol (1)
1082-1089: Defensive validation of jump DK and precise jump flag.Guard against out-of-range jump IDs and set disputeKitJump only on actual change. Prevents accidental event misreporting and future-proofing.
- if (!courts[newCourtID].supportedDisputeKits[newDisputeKitID]) { - // The current Dispute Kit is not compatible with the new court, jump to another Dispute Kit. - newDisputeKitID = disputeKits[_round.disputeKitID].getJumpDisputeKitID(); - if (newDisputeKitID == NULL_DISPUTE_KIT || !courts[newCourtID].supportedDisputeKits[newDisputeKitID]) { - // The new Dispute Kit is not defined or still not compatible, fall back to `DisputeKitClassic` which is always supported. - newDisputeKitID = DISPUTE_KIT_CLASSIC; - } - disputeKitJump = true; - } + if (!courts[newCourtID].supportedDisputeKits[newDisputeKitID]) { + // The current Dispute Kit is not compatible with the new court, try the DK-provided jump target. + uint256 candidate = disputeKits[_round.disputeKitID].getJumpDisputeKitID(); + if ( + candidate == NULL_DISPUTE_KIT || + candidate >= disputeKits.length || + !courts[newCourtID].supportedDisputeKits[candidate] + ) { + // Fall back to Classic which is always supported. + newDisputeKitID = DISPUTE_KIT_CLASSIC; + } else { + newDisputeKitID = candidate; + } + disputeKitJump = (newDisputeKitID != _round.disputeKitID); + }
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore_TestBase.sol (1)
118-123: Base DK init uses Classic as jump target — consider paramizing for broader coverage.Expose the jump DK ID as a test fixture param to easily exercise non-classic jump paths in specific tests.
- bytes memory initDataDk = abi.encodeWithSignature( - "initialize(address,address,address,uint256)", - owner, - address(proxyCore), - address(wNative), - DISPUTE_KIT_CLASSIC - ); + uint256 jumpDk = DISPUTE_KIT_CLASSIC; // allow overriding in inheriting tests + bytes memory initDataDk = abi.encodeWithSignature( + "initialize(address,address,address,uint256)", + owner, + address(proxyCore), + address(wNative), + jumpDk + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
contracts/src/arbitration/KlerosCoreBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(2 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (2)
contracts/test/foundry/KlerosCore_Voting.t.sol (1)
391-396: Initializer updated to 4 args — OK.Passing DISPUTE_KIT_CLASSIC as the jump DK matches the new initialize(address,address,address,uint256) signature.
contracts/test/foundry/KlerosCore_Initialization.t.sol (1)
59-61: Jump DK assertions — OK.Both getter and storage var reflect DISPUTE_KIT_CLASSIC as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
contracts/deploy/00-home-chain-arbitration.ts (1)
112-115: Derive new DK IDs from mapping/event, not from length-after-add (avoid off-by-one).Using getDisputeKitsLength() after addNewDisputeKit risks off-by-one if IDs are 0-based. Prefer the address→ID mapping (or parse the DisputeKitCreated event).
await core.addNewDisputeKit(disputeKitShutter.address); - const disputeKitShutterID = Number(await core.getDisputeKitsLength()); + const disputeKitShutterID = Number(await core.disputeKitAddressToID(disputeKitShutter.address)); await core.enableDisputeKits(Courts.GENERAL, [disputeKitShutterID], true); @@ await core.addNewDisputeKit(disputeKitGated.address); - const disputeKitGatedID = Number(await core.getDisputeKitsLength()); + const disputeKitGatedID = Number(await core.disputeKitAddressToID(disputeKitGated.address)); await core.enableDisputeKits(Courts.GENERAL, [disputeKitGatedID], true); @@ await core.addNewDisputeKit(disputeKitGatedShutter.address); - const disputeKitGatedShutterID = Number(await core.getDisputeKitsLength()); + const disputeKitGatedShutterID = Number(await core.disputeKitAddressToID(disputeKitGatedShutter.address)); await core.enableDisputeKits(Courts.GENERAL, [disputeKitGatedShutterID], true);Alternative (if no mapping exists): read the DisputeKitCreated event from the receipt and extract the emitted ID.
#!/bin/bash # Confirm ID semantics: indexing base and presence of mapping/event rg -nP -C3 'addNewDisputeKit\s*\(' contracts rg -nP -C3 'event\s+DisputeKitCreated' contracts rg -nP -C3 '\bgetDisputeKitsLength\b' contractsAlso applies to: 121-123, 130-132
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
126-133: IDs for extra DKs: use mapping/event instead of length-after-add.Same rationale as the main script. Replace getDisputeKitsLength() with disputeKitAddressToID(...) or parse the emitted event.
await core.addNewDisputeKit(disputeKitShutter.address); - const disputeKitShutterID = Number(await core.getDisputeKitsLength()); + const disputeKitShutterID = Number(await core.disputeKitAddressToID(disputeKitShutter.address)); @@ await core.addNewDisputeKit(disputeKitGated.address); - const disputeKitGatedID = Number(await core.getDisputeKitsLength()); + const disputeKitGatedID = Number(await core.disputeKitAddressToID(disputeKitGated.address)); @@ await core.addNewDisputeKit(disputeKitGatedShutter.address); - const disputeKitGatedShutterID = Number(await core.getDisputeKitsLength()); + const disputeKitGatedShutterID = Number(await core.disputeKitAddressToID(disputeKitGatedShutter.address));Also applies to: 134-141, 142-149
🧹 Nitpick comments (1)
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
125-149: Neo script registers extra DKs but does not enable them—confirm intent.The main script enables each DK for the General court; this Neo variant doesn’t. If unintentional, mirror the enabling (and import Courts).
-import { HomeChains, isSkipped, isDevnet, PNK, ETH } from "./utils"; +import { HomeChains, isSkipped, isDevnet, PNK, ETH, Courts } from "./utils"; @@ -const disputeKitShutterID = Number(await core.getDisputeKitsLength()); +const disputeKitShutterID = Number(await core.disputeKitAddressToID(disputeKitShutter.address)); +await core.enableDisputeKits(Courts.GENERAL, [disputeKitShutterID], true); @@ -const disputeKitGatedID = Number(await core.getDisputeKitsLength()); +const disputeKitGatedID = Number(await core.disputeKitAddressToID(disputeKitGated.address)); +await core.enableDisputeKits(Courts.GENERAL, [disputeKitGatedID], true); @@ -const disputeKitGatedShutterID = Number(await core.getDisputeKitsLength()); +const disputeKitGatedShutterID = Number(await core.disputeKitAddressToID(disputeKitGatedShutter.address)); +await core.enableDisputeKits(Courts.GENERAL, [disputeKitGatedShutterID], true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/deploy/00-home-chain-arbitration-neo.ts(2 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
contracts/deploy/00-home-chain-arbitration.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (1)
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
31-36: VerifydisputeKitAddressToIDandsetJumpDisputeKitIDexist on Neo
I couldn’t locate these functions in thecontracts/directory—confirm they’re exposed on your NeoKlerosCoreABI. If they are, add post-deploy logic (as in the main script) to fetch and compare the actual Classic DK ID and call the setter; if not, implement or import them before relying on this check.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
332-468: Great addition: non-classic DK jump path covered.This addresses the earlier request to test a non-classic jump target, including event and state verification.
🧹 Nitpick comments (5)
contracts/test/foundry/KlerosCore_Appeals.t.sol (5)
217-233: Avoid hard-coded DK IDs; derive from core to prevent brittleness.Hard-coding
newDkID = 2relies on implicit ordering. Derive the ID after registering the DK and buildnewExtraDatathen. This makes the test robust to future kit additions.Apply this diff:
- uint96 newCourtID = 2; - uint256 newDkID = 2; - uint256[] memory supportedDK = new uint256[](1); - supportedDK[0] = DISPUTE_KIT_CLASSIC; - bytes memory newExtraData = abi.encodePacked(uint256(newCourtID), DEFAULT_NB_OF_JURORS, newDkID); + uint96 newCourtID = 2; + uint256[] memory supportedDK = new uint256[](1); + supportedDK[0] = DISPUTE_KIT_CLASSIC; @@ vm.prank(owner); core.addNewDisputeKit(newDisputeKit); + uint256 newDkID = core.getDisputeKitsLength(); // Latest added DK + bytes memory newExtraData = abi.encodePacked(uint256(newCourtID), DEFAULT_NB_OF_JURORS, newDkID);Also applies to: 235-247, 249-256
288-299: Consider asserting emitter addresses for event checks.Optionally strengthen expectations by checking the emitting contract addresses (e.g., Core vs DK) when using
expectEmit, to catch misrouted emits in future refactors.
342-363: Remove magic DK IDs (2, 3); compute them to survive future insertions.Because
dkID3’s initializer depends ondkID2, deploy/adddisputeKit2, read its assigned ID, then deploy/initdisputeKit3with that value.High-level steps:
- Deploy DK2, add it, set
dkID2 = core.getDisputeKitsLength().- Build init data for DK3 with
dkID2.- Deploy DK3, add it, set
dkID3 = core.getDisputeKitsLength().This avoids tight coupling to assumed ordering.
Also applies to: 364-391
369-385: Add a fallback-to-Classic test when jump target is unsupported or NULL.Current tests cover Classic and non-classic jump targets that are supported. Add a case where
getJumpDisputeKitID()is 0 or points to an unsupported DK in the parent court, and assert fallback toDISPUTE_KIT_CLASSICand correspondingDisputeKitJump(..., DISPUTE_KIT_CLASSIC)emission.I can draft this test variant if desired.
Also applies to: 386-391
456-468: Minor assertion message mismatch.Message says “classic DK” but you assert against
disputeKit2(which is Classic-flavored but not the canonical Classic ID). Consider clarifying the message to “target DK” or include the ID for precision.- assertEq(account, staker1, "Wrong drawn account in the classic DK"); + assertEq(account, staker1, "Wrong drawn account in the target DK (dkID2)");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (3)
contracts/test/foundry/KlerosCore_Appeals.t.sol (3)
149-209: Rename to “NoJump” is clear; scenario remains valid.Good rename and assertions; flows still cover the full-funding/no-jump path.
211-330: Classic jump path test looks solid; covers CourtJump + DK jump to Classic.End-to-end assertions (jump flags, events, round DK, draw guard) read well.
291-295: Compute theDisputeCreationevent’s second argument instead of hardcoding2. The event’s second parameter is the dispute-kit round index—derive that value (e.g. from the dispute’s rounds array) rather than using a literal to avoid coupling to internal implementation.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
contracts/deploy/00-home-chain-arbitration.ts (1)
37-41: Avoid hardcoding Classic DK ID; verify and auto-correct jump ID post-deploy.Using
classicDisputeKitID = 1is brittle on non-fresh deployments. AfterKlerosCoreis deployed/registered, derive the actual Classic DK ID from core and, if it differs, update the DK via the exposed setter so jump routing remains correct.const disputeKit = await deployUpgradable(deployments, "DisputeKitClassic", { from: deployer, - args: [deployer, ZeroAddress, weth.target, classicDisputeKitID], + args: [deployer, ZeroAddress, weth.target, classicDisputeKitID], log: true, }); @@ if (currentCore !== klerosCore.address) { console.log(`disputeKit.changeCore(${klerosCore.address})`); await disputeKitContract.changeCore(klerosCore.address); } + // Ensure jump target matches the actual Classic ID in core. + let actualClassicID: bigint | number; + if ("disputeKitAddressToID" in core) { + // Prefer direct mapping if available + // @ts-ignore + actualClassicID = await (core as any).disputeKitAddressToID(disputeKit.address); + } else { + // Fallback: scan disputeKits array + const n = await core.getDisputeKitsLength(); + actualClassicID = -1n; + for (let i = 0n; i < n; i++) { + if ((await core.disputeKits(i)).toLowerCase() === disputeKit.address.toLowerCase()) { actualClassicID = i; break; } + } + if (actualClassicID === -1n) throw new Error("Classic DK not found in core"); + } + if (BigInt(actualClassicID) !== BigInt(classicDisputeKitID)) { + console.log(`disputeKit.changeJumpDisputeKitID(${actualClassicID})`); + // requires governor/owner; deployer should be set as owner in initialize + // @ts-ignore + await (disputeKitContract as any).changeJumpDisputeKitID(actualClassicID); + }contracts/deploy/00-home-chain-arbitration-neo.ts (1)
31-36: Same: don’t assume Classic DK ID is 1; verify and adjust via the setter.Mirror the Classic-ID verification and
changeJumpDisputeKitID(actualClassicID)logic here as well.const disputeKit = await deployUpgradable(deployments, "DisputeKitClassicNeo", { from: deployer, contract: "DisputeKitClassic", - args: [deployer, ZeroAddress, weth.target, classicDisputeKitID], + args: [deployer, ZeroAddress, weth.target, classicDisputeKitID], log: true, }); @@ if (currentCore !== klerosCore.address) { console.log(`disputeKit.changeCore(${klerosCore.address})`); await disputeKitContract.changeCore(klerosCore.address); } + // Verify Classic’s actual ID in core and fix jump ID if needed (same as non-neo path). + let actualClassicID: bigint | number; + if ("disputeKitAddressToID" in core) { + // @ts-ignore + actualClassicID = await (core as any).disputeKitAddressToID(disputeKit.address); + } else { + const n = await core.getDisputeKitsLength(); + actualClassicID = -1n; + for (let i = 0n; i < n; i++) { + if ((await core.disputeKits(i)).toLowerCase() === disputeKit.address.toLowerCase()) { actualClassicID = i; break; } + } + if (actualClassicID === -1n) throw new Error("Classic DK not found in core"); + } + if (BigInt(actualClassicID) !== BigInt(classicDisputeKitID)) { + // @ts-ignore + await (disputeKitContract as any).changeJumpDisputeKitID(actualClassicID); + }
🧹 Nitpick comments (3)
contracts/deploy/00-home-chain-arbitration.ts (3)
106-115: Dynamic ID derivation looks good; consider using event/simulate for robustness.
getDisputeKitsLength() - 1nis fine in sequential deploys. For extra safety, you could read the ID from theDisputeKitCreatedevent ofaddNewDisputeKitor simulate to get the return value (ethers v6:await core.addNewDisputeKit.staticCall(addr)) before sending the tx.
116-124: Same note for Gated DK.Optional: derive ID via event or staticCall to avoid reliance on array tail.
125-132: Good: DKGatedShutter jumps to Shutter. Add a quick runtime assert.After deployment, assert each DK’s
getJumpDisputeKitID()matches the intended target so miswiring is caught early.await core.enableDisputeKits(Courts.GENERAL, [disputeKitGatedShutterID], true); // enable disputeKitGatedShutter on the General Court + + // Sanity checks + const shutterDk = await ethers.getContractAt("DisputeKitShutter", disputeKitShutter.address); + const gatedDk = await ethers.getContractAt("DisputeKitGated", disputeKitGated.address); + const gatedShutterDk = await ethers.getContractAt("DisputeKitGatedShutter", disputeKitGatedShutter.address); + if ((await shutterDk.getJumpDisputeKitID()) !== BigInt(classicDisputeKitID)) throw new Error("Shutter jump ID mismatch"); + if ((await gatedDk.getJumpDisputeKitID()) !== BigInt(classicDisputeKitID)) throw new Error("Gated jump ID mismatch"); + if ((await gatedShutterDk.getJumpDisputeKitID()) !== BigInt(disputeKitShutterID)) throw new Error("GatedShutter jump ID mismatch");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/deploy/00-home-chain-arbitration-neo.ts(2 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-21T10:32:16.970Z
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.ts
🧬 Code graph analysis (1)
contracts/deploy/00-home-chain-arbitration.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
Follow-up on #1357 which went a bit too far in simplifying the DK logic.
jumpDisputeKitIDinDisputeKitBasecollides with the storage variables of the derived contracts.These changes require a full redeploy, not an upgrade.
PR-Codex overview
This PR focuses on enhancing the dispute kit functionality in the Kleros protocol by introducing a new parameter for handling court jumps, allowing dispute kits to specify which kit to use after a court transition.
Detailed summary
initializefunctions in various dispute kits to include auint256 _jumpDisputeKitIDparameter.getJumpDisputeKitIDfunction to retrieve the dispute kit ID for court jumps.KlerosCoreBase.0.13.0.Summary by CodeRabbit
New Features
Improvements
Tests