Extraction of CourtRegistry from KlerosCore #2137
Conversation
|
Warning Rate limit exceeded@jaybuidl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
WalkthroughAdds an upgradeable CourtRegistry contract and refactors KlerosCore and related modules to use it for all court data and operations. Adjusts initialization to accept a CourtRegistry, migrates court creation/queries and dispute-kit checks to the registry, and restores a documented toTreeKey helper in SortitionTrees. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Deployer
participant Core as KlerosCore
participant Reg as CourtRegistry
participant DK as DisputeKit(s)
Deployer->>Reg: initialize(owner, core=Core)
Deployer->>Core: initialize(..., courts=Reg)
Note over Core,Reg: Runtime interactions
Core->>Reg: createCourt(parent, params, kits)
Reg-->>Core: courtID\nemit CourtCreated
Core->>Reg: get(courtID)
Core->>Reg: getTimesPerPeriod(courtID)
Core->>Reg: isSupported(courtID, disputeKitID)
alt dispute kit supported
Core->>DK: proceed with arbitration/appeal flow
DK-->>Core: results
else not supported
Reg-->>Core: returns false -> Core reverts/handles
end
Core->>Reg: enableDisputeKit(courtID, kitID, enable) [onlyByCore]
Reg-->>Core: emit DisputeKitEnabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches🧪 Generate unit tests
Comment |
❌ Deploy Preview for kleros-v2-university 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 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
contracts/src/arbitration/KlerosCore.sol (1)
351-371: Add validation for empty_supportedDisputeKitsarray.The function validates individual dispute kit IDs but doesn't check if the
_supportedDisputeKitsarray is empty before the loop. While the CourtRegistry'screateCourtwill validate this, it's good practice to fail fast with a clear error.function createCourt( uint96 _parent, bool _hiddenVotes, uint256 _minStake, uint256 _alpha, uint256 _feeForJuror, uint256 _jurorsForCourtJump, uint256[4] memory _timesPerPeriod, bytes memory _sortitionExtraData, uint256[] memory _supportedDisputeKits ) external onlyByOwner { + if (_supportedDisputeKits.length == 0) revert WrongDisputeKitIndex(); uint96 courtID = courts.createCourt(contracts/src/arbitration/CourtRegistry.sol (2)
93-95: Address the TODO for FORKING_COURT initialization.The FORKING_COURT is created as an empty court without proper initialization. This TODO should be addressed before production deployment.
Would you like me to help implement the proper initialization for the FORKING_COURT with appropriate properties and emit the CourtCreated event?
173-180: Inconsistent validation message when checking child courts.When a child court's
minStakeis less than the new parent'sminStake, the error messageMinStakeLowerThanParentCourtis misleading since it's actually the child's stake that's problematic.Consider adding a more specific error for this case:
+ error ChildCourtMinStakeTooLow(); for (uint256 i = 0; i < court.children.length; i++) { if (courts[court.children[i]].minStake < _minStake) { - revert MinStakeLowerThanParentCourt(); + revert ChildCourtMinStakeTooLow(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/CourtRegistry.sol(1 hunks)contracts/src/arbitration/KlerosCore.sol(21 hunks)contracts/src/libraries/SortitionTrees.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
📚 Learning: 2024-11-19T16:09:41.467Z
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/KlerosCore.solcontracts/src/arbitration/CourtRegistry.sol
📚 Learning: 2024-11-19T17:18:39.007Z
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/CourtRegistry.sol
🔇 Additional comments (7)
contracts/src/libraries/SortitionTrees.sol (1)
185-190: LGTM! Well-documented helper function.The reintroduced
toTreeKeyfunction is properly placed in the Public Views section with clear NatSpec documentation. The implementation correctly wraps aCourtIDinto aTreeKeyfor use with sortition trees.contracts/src/arbitration/KlerosCore.sol (5)
10-10: LGTM! Clean import statement for the new CourtRegistry.The import statement properly introduces the
CourtRegistrycontract and theCourtstruct that will be used throughout the refactored KlerosCore.
1106-1110: Court validation logic looks correct.The validation properly checks against FORKING_COURT and the total number of courts from the registry. The minimum stake validation also correctly uses the registry's getter.
207-259: ```shell
#!/bin/bash
set -euo pipefailecho "== Searching for FORKING_COURT and GENERAL_COURT occurrences =="
rg -n -S "FORKING_COURT|GENERAL_COURT" || trueecho "== Searching for createTree, createCourt, enableDisputeKit calls =="
rg -n -S "createTree(|createCourt(|enableDisputeKit(" || trueecho "== Locating CourtRegistry contract files =="
rg -n -S "contract CourtRegistry" || trueREG_FILES=$(rg -l -S "contract CourtRegistry" || true)
for f in $REG_FILES; do
echo "---- $f ----"
rg -n -C5 "function createCourt(" "$f" || true
rg -n -C20 "createCourt(" "$f" || true
doneecho "== Showing KlerosCore.initialize() context =="
KC_FILES=$(rg -l -S "contract KlerosCore" || true)
for f in $KC_FILES; do
echo "---- $f ----"
rg -n -C10 "function initialize(" "$f" || true
doneecho "Done."
--- `1025-1034`: **All _isCourtJumping callers updated to accept _jurorsForCourtJump** Both calls to _isCourtJumping in contracts/src/arbitration/KlerosCore.sol (lines 1003 and 1045) pass court.jurorsForCourtJump; no other callers were found. --- `773-773`: **Resolved — penalizedInCourtID comes from validated sources.** drawnJurorFromCourtIDs is set in draw as `fromSubcourtID != 0 ? fromSubcourtID : dispute.courtID` (contracts/src/arbitration/KlerosCore.sol:591); dispute.courtID is obtained via `courts.get(courtID)` during dispute creation (contracts/src/arbitration/KlerosCore.sol:502), so `courts.get(penalizedInCourtID).minStake` at contracts/src/arbitration/KlerosCore.sol:773 is accessing a validated court. </blockquote></details> <details> <summary>contracts/src/arbitration/CourtRegistry.sol (1)</summary><blockquote> `214-244`: **Public view functions are well-implemented.** The getter functions provide clean interfaces for accessing court data, children, times per period, dispute kit support, and the total number of courts. Good separation of concerns. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if (courts[_parent].minStake > _minStake) revert MinStakeLowerThanParentCourt(); | ||
| if (_supportedDisputeKits.length == 0) revert UnsupportedDisputeKit(); | ||
| if (_parent == FORKING_COURT) revert InvalidForkingCourtAsParent(); | ||
|
|
||
| courtID = uint96(courts.length); | ||
| Court storage court = courts.push(); | ||
|
|
||
| // Check that Classic DK support was added. | ||
| if (!supportedDisputeKits[courtID][DISPUTE_KIT_CLASSIC]) revert MustSupportDisputeKitClassic(); | ||
|
|
||
| court.parent = _parent; | ||
| court.children = new uint256[](0); | ||
| court.hiddenVotes = _hiddenVotes; | ||
| court.minStake = _minStake; | ||
| court.alpha = _alpha; | ||
| court.feeForJuror = _feeForJuror; | ||
| court.jurorsForCourtJump = _jurorsForCourtJump; | ||
| court.timesPerPeriod = _timesPerPeriod; | ||
|
|
||
| // Update the parent. | ||
| courts[_parent].children.push(courtID); | ||
| emit CourtCreated( | ||
| uint96(courtID), | ||
| _parent, | ||
| _hiddenVotes, | ||
| _minStake, | ||
| _alpha, | ||
| _feeForJuror, | ||
| _jurorsForCourtJump, | ||
| _timesPerPeriod, | ||
| _supportedDisputeKits | ||
| ); | ||
| } |
There was a problem hiding this comment.
Critical issue: Classic dispute kit support check happens after court creation.
The check for Classic DK support (line 137) occurs after the court has already been pushed to the array (line 134). This means if the check fails, the court is already created but in an invalid state. The check should validate the _supportedDisputeKits array before creating the court.
function createCourt(
uint96 _parent,
bool _hiddenVotes,
uint256 _minStake,
uint256 _alpha,
uint256 _feeForJuror,
uint256 _jurorsForCourtJump,
uint256[4] memory _timesPerPeriod,
uint256[] memory _supportedDisputeKits
) external onlyByCore returns (uint96 courtID) {
if (courts[_parent].minStake > _minStake) revert MinStakeLowerThanParentCourt();
if (_supportedDisputeKits.length == 0) revert UnsupportedDisputeKit();
if (_parent == FORKING_COURT) revert InvalidForkingCourtAsParent();
+
+ // Check that Classic DK will be supported
+ bool hasClassicDK = false;
+ for (uint256 i = 0; i < _supportedDisputeKits.length; i++) {
+ if (_supportedDisputeKits[i] == DISPUTE_KIT_CLASSIC) {
+ hasClassicDK = true;
+ break;
+ }
+ }
+ if (!hasClassicDK) revert MustSupportDisputeKitClassic();
courtID = uint96(courts.length);
Court storage court = courts.push();
- // Check that Classic DK support was added.
- if (!supportedDisputeKits[courtID][DISPUTE_KIT_CLASSIC]) revert MustSupportDisputeKitClassic();
-
court.parent = _parent;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (courts[_parent].minStake > _minStake) revert MinStakeLowerThanParentCourt(); | |
| if (_supportedDisputeKits.length == 0) revert UnsupportedDisputeKit(); | |
| if (_parent == FORKING_COURT) revert InvalidForkingCourtAsParent(); | |
| courtID = uint96(courts.length); | |
| Court storage court = courts.push(); | |
| // Check that Classic DK support was added. | |
| if (!supportedDisputeKits[courtID][DISPUTE_KIT_CLASSIC]) revert MustSupportDisputeKitClassic(); | |
| court.parent = _parent; | |
| court.children = new uint256[](0); | |
| court.hiddenVotes = _hiddenVotes; | |
| court.minStake = _minStake; | |
| court.alpha = _alpha; | |
| court.feeForJuror = _feeForJuror; | |
| court.jurorsForCourtJump = _jurorsForCourtJump; | |
| court.timesPerPeriod = _timesPerPeriod; | |
| // Update the parent. | |
| courts[_parent].children.push(courtID); | |
| emit CourtCreated( | |
| uint96(courtID), | |
| _parent, | |
| _hiddenVotes, | |
| _minStake, | |
| _alpha, | |
| _feeForJuror, | |
| _jurorsForCourtJump, | |
| _timesPerPeriod, | |
| _supportedDisputeKits | |
| ); | |
| } | |
| if (courts[_parent].minStake > _minStake) revert MinStakeLowerThanParentCourt(); | |
| if (_supportedDisputeKits.length == 0) revert UnsupportedDisputeKit(); | |
| if (_parent == FORKING_COURT) revert InvalidForkingCourtAsParent(); | |
| // Check that Classic DK will be supported | |
| bool hasClassicDK = false; | |
| for (uint256 i = 0; i < _supportedDisputeKits.length; i++) { | |
| if (_supportedDisputeKits[i] == DISPUTE_KIT_CLASSIC) { | |
| hasClassicDK = true; | |
| break; | |
| } | |
| } | |
| if (!hasClassicDK) revert MustSupportDisputeKitClassic(); | |
| courtID = uint96(courts.length); | |
| Court storage court = courts.push(); | |
| court.parent = _parent; | |
| court.children = new uint256[](0); | |
| court.hiddenVotes = _hiddenVotes; | |
| court.minStake = _minStake; | |
| court.alpha = _alpha; | |
| court.feeForJuror = _feeForJuror; | |
| court.jurorsForCourtJump = _jurorsForCourtJump; | |
| court.timesPerPeriod = _timesPerPeriod; | |
| // Update the parent. | |
| courts[_parent].children.push(courtID); | |
| emit CourtCreated( | |
| uint96(courtID), | |
| _parent, | |
| _hiddenVotes, | |
| _minStake, | |
| _alpha, | |
| _feeForJuror, | |
| _jurorsForCourtJump, | |
| _timesPerPeriod, | |
| _supportedDisputeKits | |
| ); | |
| } |
| function enableDisputeKit(uint96 _courtID, uint256 _disputeKitID, bool _enable) external onlyByCore { | ||
| supportedDisputeKits[_courtID][_disputeKitID] = _enable; | ||
| emit DisputeKitEnabled(_courtID, _disputeKitID, _enable); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider preventing disabling of Classic dispute kit.
The enableDisputeKit function allows disabling any dispute kit, including the Classic DK. Based on the requirement that all courts must support Classic DK, consider adding validation.
function enableDisputeKit(uint96 _courtID, uint256 _disputeKitID, bool _enable) external onlyByCore {
+ // Classic dispute kit must always be enabled
+ if (_disputeKitID == DISPUTE_KIT_CLASSIC && !_enable) {
+ revert MustSupportDisputeKitClassic();
+ }
supportedDisputeKits[_courtID][_disputeKitID] = _enable;
emit DisputeKitEnabled(_courtID, _disputeKitID, _enable);
}📝 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 enableDisputeKit(uint96 _courtID, uint256 _disputeKitID, bool _enable) external onlyByCore { | |
| supportedDisputeKits[_courtID][_disputeKitID] = _enable; | |
| emit DisputeKitEnabled(_courtID, _disputeKitID, _enable); | |
| } | |
| function enableDisputeKit(uint96 _courtID, uint256 _disputeKitID, bool _enable) external onlyByCore { | |
| // Classic dispute kit must always be enabled | |
| if (_disputeKitID == DISPUTE_KIT_CLASSIC && !_enable) { | |
| revert MustSupportDisputeKitClassic(); | |
| } | |
| supportedDisputeKits[_courtID][_disputeKitID] = _enable; | |
| emit DisputeKitEnabled(_courtID, _disputeKitID, _enable); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/CourtRegistry.sol around lines 202 to 205, the
enableDisputeKit function currently allows disabling any dispute kit including
the Classic DK; add validation so that when _enable is false the call reverts if
_disputeKitID equals the Classic dispute kit id (use the contract constant/enum
for the Classic DK or the known id value), e.g., require(!(!_enable &&
_disputeKitID == CLASSIC_DISPUTE_KIT_ID), "Cannot disable Classic dispute kit");
keep the existing state update and event emission for other kits.
jaybuidl
force-pushed
the
refactor/court-registry
branch
from
7a90bbc to
6c40c01
Compare
|
|
Abandoning as it makes KlerosCore even bigger |
The goal is to offload Courts related functions to a dedicated contract to reduce the size of
KlerosCorewhich is above 24KB with the current optimizer settingsPR-Codex overview
This PR focuses on refactoring and enhancing the
KlerosCoreand related contracts by introducing a newCourtRegistrycontract, which centralizes court management and modifies how courts and their properties are accessed.Detailed summary
CourtRegistrycontract to manage court data.courts.get()method.KlerosCoreto utilizeCourtRegistryfor court data.CourtRegistryfor creating and modifying courts.KlerosCore.CourtRegistry.Summary by CodeRabbit