fix(KC): remove null dk #1893
fix(KC): remove null dk #1893
Conversation
WalkthroughThis pull request adjusts the dispute kit identification across the contracts and tests. The changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Task as Simulation Task
participant Core as KlerosCore Contract
participant Validator as Dispute Kit Validator
Task->>Core: createCourt(..., [0])
Core->>Validator: Validate dispute kit (index 0)
Validator-->>Core: Return validated kit
Core-->>Task: Court created with dispute kit 0
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (12)
🔇 Additional comments (14)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site 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: 1
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore.t.sol (1)
1-2713: Consider adding tests for additional edge cases.While the test coverage is comprehensive, consider adding tests for:
- Concurrent dispute resolution in multiple courts
- Mass juror unstaking scenarios
- Complex appeal funding patterns with multiple funders
- Edge cases around delayed stake execution
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/scripts/simulations/tasks.ts(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(3 hunks)contracts/src/arbitration/devtools/KlerosCoreRuler.sol(0 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(3 hunks)contracts/src/libraries/Constants.sol(1 hunks)contracts/test/arbitration/draw.ts(1 hunks)contracts/test/arbitration/index.ts(2 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/arbitration/staking.ts(2 hunks)contracts/test/foundry/KlerosCore.t.sol(10 hunks)
💤 Files with no reviewable changes (1)
- contracts/src/arbitration/devtools/KlerosCoreRuler.sol
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- 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: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
contracts/src/libraries/Constants.sol (1)
12-12: LGTM! Simplified dispute kit indexing.The change from
1to0forDISPUTE_KIT_CLASSICand removal ofNULL_DISPUTE_KITsimplifies the indexing scheme by eliminating the concept of a null dispute kit.contracts/test/arbitration/index.ts (1)
19-19: LGTM! Test expectations updated for new dispute kit indexing.Test assertions have been correctly updated to match the new dispute kit indexing scheme where
DISPUTE_KIT_CLASSICis now0.Also applies to: 33-33, 38-38
contracts/test/arbitration/draw.ts (1)
100-100: LGTM! Test setup updated for new dispute kit indexing.The
createCourtcall has been correctly updated to use the new dispute kit index.contracts/test/arbitration/staking.ts (1)
43-43: LGTM! Test setups updated for new dispute kit indexing.Both
createCourtcalls have been correctly updated to use the new dispute kit index.Also applies to: 389-389
contracts/scripts/simulations/tasks.ts (1)
88-88: LGTM!The update to
supportedDisputeKitsarray aligns with the broader refactoring to remove null dispute kit checks.contracts/test/arbitration/staking-neo.ts (1)
402-402: LGTM!The updates to
createCourtcalls maintain test coverage for the refactored dispute kit functionality.Also applies to: 737-737
contracts/src/arbitration/KlerosCoreBase.sol (2)
346-347: LGTM!The removal of the zero index check simplifies the dispute kit validation logic while maintaining the core validation that the index is within bounds.
1126-1128: LGTM!The simplified logic for handling invalid dispute kit IDs maintains the fallback to classic dispute kit while improving code clarity.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (4)
217-219: LGTM! Simplified dispute kit initialization.The initialization has been streamlined by directly adding the classic dispute kit without a NULL_DISPUTE_KIT placeholder, which aligns with the PR objective.
340-345: LGTM! Simplified dispute kit validation.The validation has been simplified while maintaining security by:
- Preventing out-of-bounds access to dispute kits
- Ensuring classic dispute kit support (line 347)
414-418: LGTM! Consistent dispute kit validation.The validation logic is consistent with createCourt changes and maintains security by:
- Preventing out-of-bounds access
- Preserving the requirement for classic dispute kit support
1118-1120: LGTM! Consistent extradata validation.The validation logic is consistent with other changes and maintains safety by defaulting to DISPUTE_KIT_CLASSIC for invalid indices.
contracts/test/foundry/KlerosCore.t.sol (2)
19-20: LGTM! Import path update reflects better organization.The import path change from
../../src/snapshot-proxy/KlerosCoreSnapshotProxy.solto../../src/arbitration/view/KlerosCoreSnapshotProxy.solreflects a better organization of the codebase, placing view-related contracts in a dedicated directory.
60-151: LGTM! Comprehensive test setup.The
setUpfunction provides thorough initialization of all required contracts and test variables, including:
- Core contracts (KlerosCore, SortitionModule, DisputeKit)
- Supporting contracts (RNG, PNK, TestERC20)
- Test accounts with appropriate balances
- Configuration parameters
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
unknownunknown1
force-pushed
the
fix/dk-null-remove
branch
from
43dd2bf to
a14f146
Compare
jaybuidl
force-pushed
the
fix/dk-null-remove
branch
from
a14f146 to
e7f0d52
Compare
|
Code Climate has analyzed commit e7f0d52 and detected 0 issues on this pull request. View more on Code Climate. |
|
|
Putting this PR on hold because it would break the current production deployment after upgrade. It would require some dispute migration logic in the initializer and a fair amount of testing/simulation. In addition the subgraph refers to the dispute kit classic index at 1 in a number of places.
Overall it's a high risk/low value change. |
PR-Codex overview
This PR focuses on modifying the handling of dispute kits in the Kleros arbitration system, particularly changing default values and references from
1to0, indicating a shift in the index of the classic dispute kit and related functionality.Detailed summary
disputeKitIDdefault from1to0in multiple contracts.supportedDisputeKitsin tests from[1]to[0].NULL_DISPUTE_KITto remove its usage.Summary by CodeRabbit
These improvements enhance the consistency and reliability of the arbitration and court creation features without impacting the overall user experience.