fix(DisputeKit): check that dispute belongs to DK #2039
Conversation
✅ 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? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughA new public mapping was introduced in the dispute kit contract to record the length of the disputes array when a core dispute ID is mapped, ensuring accurate validation of dispute existence. Several functions now require this mapping to be set before proceeding. The test suite was expanded to verify these behaviors across multiple dispute kits. Changes
Suggested labels
Suggested reviewers
Poem
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. 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 (
|
|
Code Climate has analyzed commit 5ac93ef and detected 0 issues on this pull request. View more on Code Climate. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
256-256: Consider the order of validation checks.The require statement correctly prevents operations on unmapped disputes. However, the
notJumpedmodifier executes before this check and could accessdisputes[0]for unmapped disputes, potentially causing misleading error messages.Consider moving this check before the modifier execution or adding it to the modifier itself:
function castCommit( uint256 _coreDisputeID, uint256[] calldata _voteIDs, bytes32 _commit -) external notJumped(_coreDisputeID) { +) external { (, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID); require(period == KlerosCoreBase.Period.commit, "The dispute should be in Commit period."); require(_commit != bytes32(0), "Empty commit."); require(coreDisputeIDToDisputeLength[_coreDisputeID] != 0, "No local dispute for core ID"); + require(!disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped, "Dispute jumped to a parent DK!");
286-286: Same modifier order issue as castCommit.The require statement is correctly placed for validation, but the
notJumpedmodifier could execute on unmapped disputes before this check.Apply the same fix as suggested for
castCommitto ensure proper error handling order.contracts/test/foundry/KlerosCore.t.sol (1)
2832-2924: Comprehensive test validates dispute kit isolationThis test function effectively validates the new dispute kit validation functionality by:
- Creating and registering a new dispute kit
- Testing that different dispute kits maintain separate mappings for core dispute IDs
- Verifying that the
coreDisputeIDToDisputeLengthmapping correctly tracks dispute counts- Ensuring that cross-dispute kit operations fail with appropriate error messages
- Confirming that voting works correctly within the proper dispute kit
The test logic is sound and follows good testing practices with proper setup, execution, and assertion phases.
A few observations for consideration:
- The test creates 3 disputes but primarily focuses on dispute ID 2 - consider adding assertions for the other dispute IDs for more comprehensive coverage
- The test assumes specific dispute IDs (0, 1, 2) which could be made more robust by using variables or dynamic lookups
- Consider adding a test case for the boundary condition where
coreDisputeIDToDisputeLengthreturns 0 for non-existent disputesThese are minor suggestions and don't affect the core functionality being tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(6 hunks)contracts/test/foundry/KlerosCore.t.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
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`.
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'.
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (6)
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'.
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.
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
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`.
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.
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.
contracts/test/foundry/KlerosCore.t.sol (5)
undefined
<retrieved_learning>
Learnt from: kemuru
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: kemuru
PR: #1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the kleros-v2 codebase, the property totalResolvedDisputes should remain and should not be renamed to totalResolvedVotes.
</retrieved_learning>
<retrieved_learning>
Learnt from: Harman-singh-waraich
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: tractorss
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: Harman-singh-waraich
PR: #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.
</retrieved_learning>
⏰ 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-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
67-67: Well-designed mapping to track dispute ownership.The new mapping effectively distinguishes between mapped and unmapped disputes by storing
localDisputeID + 1, ensuring the value is never 0 for valid disputes.
209-209: Correct initialization of dispute length mapping.Setting the mapping to
localDisputeID + 1ensures it's never 0 for valid disputes, allowing the require checks to properly distinguish between mapped and unmapped disputes.
333-333: Correct placement of dispute validation.The require statement is properly placed and this function doesn't have the modifier order issue since the check happens early in the function body.
413-413: Proper validation for withdrawal function.The require statement correctly ensures only disputes belonging to this dispute kit can have their fees withdrawn.
contracts/test/foundry/KlerosCore.t.sol (2)
1396-1396: LGTM: Assertion verifies dispute length mappingThe assertion correctly checks that the
coreDisputeIDToDisputeLengthmapping returns 1 for dispute ID 0, which aligns with the new functionality to track dispute lengths by core dispute ID.
1787-1787: LGTM: Assertion message correctionThe assertion message correctly expects choice value "2" instead of "empty", which aligns with the test logic where the choice is set to 2 on line 1764.
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
PR-Codex overview
This PR introduces a new mapping in the
DisputeKitClassicBasecontract to track the length of disputes associated with core dispute IDs. It enhances dispute handling by ensuring that local disputes are correctly linked to core disputes, adding necessary checks and updating related functions.Detailed summary
coreDisputeIDToDisputeLengthmapping to track dispute lengths.DisputeKitClassicBasecontract to set dispute lengths after local disputes are created.KlerosCoreTestto verify the new dispute length functionality.Summary by CodeRabbit
New Features
Tests