feat: centralized DK#2457
Draft
unknownunknown1 wants to merge 1 commit into
Draft
Conversation
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
|
Contributor
WalkthroughAdds a new ChangesCentralized dispute kit and KlerosCore forking fix
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KlerosCore
participant CentralizedKit
participant Ruler
KlerosCore->>CentralizedKit: createDispute(coreDisputeID, numberOfChoices, extraData)
CentralizedKit-->>CentralizedKit: store Dispute, emit DisputeCreation
Ruler->>CentralizedKit: giveRuling(coreDisputeID, ruling)
CentralizedKit-->>CentralizedKit: validate ruling, set ruled/ruling, emit RulingGiven
KlerosCore->>CentralizedKit: currentRuling(coreDisputeID)
CentralizedKit-->>KlerosCore: return ruling
sequenceDiagram
participant KlerosCore
participant SortitionModule
KlerosCore->>KlerosCore: appeal() -> compute newCourtID
alt newCourtID == FORKING_COURT
KlerosCore->>KlerosCore: set extra round nbVotes = 0
else
KlerosCore->>SortitionModule: createDisputeHook()
end
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/KlerosCore.sol (1)
879-892: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftWire the forking jump through the full appeal path.
newCourtID == FORKING_COURTis currently unreachable because_getCompatibleNextRoundSettings()falls back before returning it. If that guard is relaxed, Line 880 still divides by the Forking Court’sfeeForJurorbefore Lines 886-888 setnbVotesto0, andappealCost()still blocks the jump withNON_PAYABLE_AMOUNT. Update the helper, cost path, and vote/fee calculation together before relying on this branch.Also applies to: 1189-1207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/arbitration/KlerosCore.sol` around lines 879 - 892, The forking-court appeal branch is not wired consistently across the round setup and cost calculation paths. Update `_getCompatibleNextRoundSettings()` so `FORKING_COURT` can actually be returned, then adjust `appealCost()` to allow the fork jump instead of reverting with `NON_PAYABLE_AMOUNT`. In the round-creation logic around `extraRound.nbVotes`, make the fee/vote computation safe for `newCourtID == FORKING_COURT` by avoiding the `court.feeForJuror` division before the special-case zeroing, and keep the `sortitionModule.createDisputeHook()` bypass tied to the fork branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/src/arbitration/dispute-kits/CentralizedKit.sol`:
- Around line 115-121: `CentralizedKit.createDispute()` currently accepts
positive `_nbVotes` even though `draw()` always reverts, which can leave
disputes stuck when `KlerosCore` creates an initial round with votes. Update
`createDispute()` in `CentralizedKit` to reject any non-zero `_nbVotes` (or
otherwise enforce zero-vote rounds for this kit) so only disputes that cannot
require juror drawing are created; also make sure the same guard is applied
wherever this create path is duplicated in the kit logic.
- Around line 75-100: Reject zero addresses for the critical role assignments in
CentralizedKit. Update `initialize()`, `changeOwner()`, `changeCore()`, and
`changeRuler()` to validate `_owner`, `_core`, and `_ruler` are not `address(0)`
before writing to storage. Use the existing `owner`, `core`, and `ruler` state
variables and the governance setters to locate the affected logic, and revert on
invalid inputs so these roles cannot be set to zero.
In `@contracts/src/arbitration/KlerosCore.sol`:
- Around line 1002-1004: The zero-juror note is attached to the wrong statement
in `execute()`/`_executePenalties()`: the revert in a zero-draw round happens
earlier when dividing by `numberOfVotesInRound`, not at
`round.drawnJurors[_params.repartition]`. Move the comment to the actual
division site or add an explicit zero-vote guard in `execute()` so the failure
mode is documented where it occurs, and keep the `drawnJurors` access comment
only if it still applies separately.
---
Outside diff comments:
In `@contracts/src/arbitration/KlerosCore.sol`:
- Around line 879-892: The forking-court appeal branch is not wired consistently
across the round setup and cost calculation paths. Update
`_getCompatibleNextRoundSettings()` so `FORKING_COURT` can actually be returned,
then adjust `appealCost()` to allow the fork jump instead of reverting with
`NON_PAYABLE_AMOUNT`. In the round-creation logic around `extraRound.nbVotes`,
make the fee/vote computation safe for `newCourtID == FORKING_COURT` by avoiding
the `court.feeForJuror` division before the special-case zeroing, and keep the
`sortitionModule.createDisputeHook()` bypass tied to the fork branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ec0b562-007d-4632-a0c5-3076e822dd77
📒 Files selected for processing (2)
contracts/src/arbitration/KlerosCore.solcontracts/src/arbitration/dispute-kits/CentralizedKit.sol
Comment on lines
+75
to
+100
| function initialize(address _owner, KlerosCore _core, address _ruler) external initializer { | ||
| owner = _owner; | ||
| core = _core; | ||
| ruler = _ruler; | ||
| } | ||
|
|
||
| // ************************ // | ||
| // * Governance * // | ||
| // ************************ // | ||
|
|
||
| /// @notice Changes the `owner` storage variable. | ||
| /// @param _owner The new value for the `owner` storage variable. | ||
| function changeOwner(address payable _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } | ||
|
|
||
| /// @notice Changes the `core` storage variable. | ||
| /// @param _core The new value for the `core` storage variable. | ||
| function changeCore(address _core) external onlyByOwner { | ||
| core = KlerosCore(_core); | ||
| } | ||
|
|
||
| /// @notice Changes the `ruler` storage variable. | ||
| /// @param _ruler The new value for the `ruler` storage variable. | ||
| function changeRuler(address _ruler) external onlyByOwner { | ||
| ruler = _ruler; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject zero addresses for critical roles.
Setting owner, core, or ruler to address(0) can permanently block governance, Core-only dispute creation, or centralized rulings. Add zero-address checks in initialize() and the corresponding setters.
Suggested fix
+ error ZeroAddress();
+
function initialize(address _owner, KlerosCore _core, address _ruler) external initializer {
+ require(_owner != address(0) && address(_core) != address(0) && _ruler != address(0), ZeroAddress());
owner = _owner;
core = _core;
ruler = _ruler;
}
@@
function changeOwner(address payable _owner) external onlyByOwner {
+ require(_owner != address(0), ZeroAddress());
owner = _owner;
}
@@
function changeCore(address _core) external onlyByOwner {
+ require(_core != address(0), ZeroAddress());
core = KlerosCore(_core);
}
@@
function changeRuler(address _ruler) external onlyByOwner {
+ require(_ruler != address(0), ZeroAddress());
ruler = _ruler;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/arbitration/dispute-kits/CentralizedKit.sol` around lines 75 -
100, Reject zero addresses for the critical role assignments in CentralizedKit.
Update `initialize()`, `changeOwner()`, `changeCore()`, and `changeRuler()` to
validate `_owner`, `_core`, and `_ruler` are not `address(0)` before writing to
storage. Use the existing `owner`, `core`, and `ruler` state variables and the
governance setters to locate the affected logic, and revert on invalid inputs so
these roles cannot be set to zero.
Comment on lines
+115
to
+121
| function createDispute( | ||
| uint256 _coreDisputeID, | ||
| uint256 _coreRoundID, | ||
| uint256 _numberOfChoices, | ||
| bytes calldata _extraData, | ||
| uint256 /*_nbVotes*/ | ||
| ) public override onlyByCore { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Prevent positive-vote centralized disputes from getting stuck.
createDispute() accepts any _nbVotes, but draw() always reverts. For an initial Core dispute, KlerosCore can pass a positive round.nbVotes, and then evidence cannot finish because no jurors can ever be drawn. Reject non-zero _nbVotes here or make Core create zero-vote rounds for this kit.
Minimal safety guard
- uint256 /*_nbVotes*/
+ uint256 _nbVotes
) public override onlyByCore {
+ require(_nbVotes == 0, NonZeroVotesUnsupported());
coreDisputeIDToLocal[_coreDisputeID] = disputes.length;
@@
error UnsupportedOperation();
+ error NonZeroVotesUnsupported();Also applies to: 155-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/arbitration/dispute-kits/CentralizedKit.sol` around lines 115 -
121, `CentralizedKit.createDispute()` currently accepts positive `_nbVotes` even
though `draw()` always reverts, which can leave disputes stuck when `KlerosCore`
creates an initial round with votes. Update `createDispute()` in
`CentralizedKit` to reject any non-zero `_nbVotes` (or otherwise enforce
zero-vote rounds for this kit) so only disputes that cannot require juror
drawing are created; also make sure the same guard is applied wherever this
create path is duplicated in the kit logic.
Comment on lines
1002
to
1004
| // Unlock all the PNKs for this draw. | ||
| // Note: if drawnJurors is 0 it will revert (eg. in centralized kit). | ||
| address account = round.drawnJurors[_params.repartition]; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move this zero-juror note to the actual failing point.
For a zero-draw round, execute() divides by numberOfVotesInRound before _executePenalties() is reached, so this comment points to the wrong failure mode. Either handle zero-vote execution explicitly or document it at the division.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/arbitration/KlerosCore.sol` around lines 1002 - 1004, The
zero-juror note is attached to the wrong statement in
`execute()`/`_executePenalties()`: the revert in a zero-draw round happens
earlier when dividing by `numberOfVotesInRound`, not at
`round.drawnJurors[_params.repartition]`. Move the comment to the actual
division site or add an explicit zero-vote guard in `execute()` so the failure
mode is documented where it occurs, and keep the `drawnJurors` access comment
only if it still applies separately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR focuses on implementing the
CentralizedKitcontract for managing disputes in the Kleros arbitration system, including modifications to theKlerosCorecontract to handle new dispute types and conditions, particularly for theForkingCourt.Detailed summary
CentralizedKitcontract with structures and storage for disputes.onlyByOwner,onlyByCore).KlerosCoreto handle new conditions for disputes and jurors.nbVotesin thecreateDisputeHooklogic.Summary by CodeRabbit
New Features
Bug Fixes