fix(DK): allow recurring DK #2159
Conversation
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
WalkthroughReworks DisputeKitClassicBase dispute creation and round indexing: reuse active local disputes for a coreDisputeID (resetting Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Core as KlerosCore
participant DK as DisputeKitClassicBase
participant Map as Mappings
Core->>DK: createDispute(coreDisputeID, numberOfChoices, extraData)
DK->>Map: check coreDisputeIDToActive[coreDisputeID]
alt Active → reuse local dispute
DK->>Map: localID = coreDisputeIDToLocal[coreDisputeID]
DK->>DK: load dispute(localID)
DK->>DK: dispute.jumped = false
DK->>Map: set coreRoundIDToLocal[nextCoreRound] = dispute.rounds.length (length-based)
DK-->>Core: return localID
else Not active → create new local dispute
DK->>DK: create dispute (init choices, extraData)
DK->>Map: coreDisputeIDToLocal[coreDisputeID] = newLocalID
DK->>Map: coreDisputeIDToActive[coreDisputeID] = true
DK->>Map: coreRoundIDToLocal[nextCoreRound] = dispute.rounds.length
DK-->>Core: return newLocalID
end
Note right of DK: Reads now index as rounds[coreRoundIDToLocal[coreRoundID] - 1]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
✅ 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. |
![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 (4)
contracts/test/foundry/KlerosCore_Appeals.t.sol (4)
609-619: Pin event emitters to avoid false positivesTie expected logs to their emitters to ensure they come from the intended contracts.
Apply this diff:
- vm.expectEmit(true, true, true, true); + vm.expectEmit(address(core)); emit KlerosCore.CourtJump(disputeID, 1, courtID3, courtID2); - vm.expectEmit(true, true, true, true); + vm.expectEmit(address(core)); emit KlerosCore.DisputeKitJump(disputeID, 1, dkID3, dkID2); - vm.expectEmit(true, true, true, true); + vm.expectEmit(address(disputeKit2)); emit DisputeKitClassicBase.DisputeCreation(disputeID, 2, newExtraData); - vm.expectEmit(true, true, true, true); + vm.expectEmit(address(core)); emit KlerosCore.AppealDecision(disputeID, arbitrable); - vm.expectEmit(true, true, true, true); + vm.expectEmit(address(core)); emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.evidence);
685-691: Also pin emitters on the second jump (and consider asserting NewPeriod)Use specific emitters here as well; optionally assert NewPeriod(evidence) like in the first jump.
- vm.expectEmit(true, true, true, true); + vm.expectEmit(address(core)); emit KlerosCore.CourtJump(disputeID, 2, courtID2, GENERAL_COURT); - vm.expectEmit(true, true, true, true); + vm.expectEmit(address(core)); emit KlerosCore.DisputeKitJump(disputeID, 2, dkID2, dkID3); + vm.expectEmit(address(core)); + emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.evidence);
605-606: Avoid magic funding amounts; compute from appealCost and multipliersHard-coded values (0.63, 0.42, 1.35, 0.9 ether) will drift if fees/multipliers change. Derive from core.appealCost(disputeID).
Example:
- disputeKit3.fundAppeal{value: 0.63 ether}(disputeID, 1); + uint256 ac1 = core.appealCost(disputeID); + uint256 lose1 = ac1 + (ac1 * 20000) / Constants.MULTIPLIER_DIVISOR; + disputeKit3.fundAppeal{value: lose1}(disputeID, 1); ... - disputeKit3.fundAppeal{value: 0.42 ether}(disputeID, 2); + uint256 win1 = ac1 + (ac1 * 10000) / Constants.MULTIPLIER_DIVISOR; + disputeKit3.fundAppeal{value: win1}(disputeID, 2); ... - disputeKit2.fundAppeal{value: 1.35 ether}(disputeID, 1); + uint256 ac2 = core.appealCost(disputeID); + uint256 lose2 = ac2 + (ac2 * 20000) / Constants.MULTIPLIER_DIVISOR; + disputeKit2.fundAppeal{value: lose2}(disputeID, 1); ... - disputeKit2.fundAppeal{value: 0.9 ether}(disputeID, 2); + uint256 win2 = ac2 + (ac2 * 10000) / Constants.MULTIPLIER_DIVISOR; + disputeKit2.fundAppeal{value: win2}(disputeID, 2);Note: If multipliers are accessible via getters, prefer them over literals.
Also applies to: 620-621, 683-684, 690-691
758-766: Optional: assert total rounds after the second jumpQuick sanity on core-side tracking.
core.executeRuling(disputeID); // winning choice is 2 + assertEq(core.getNumberOfRounds(disputeID), 3, "Should have 3 rounds after two appeals");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/test/foundry/KlerosCore_Appeals.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). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- 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: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
470-793: Thorough recurring DK/court jump scenario — looks solidCovers re-entry semantics, round mapping, jump gating, events, and withdrawals end-to-end. No blockers.
unknownunknown1
force-pushed
the
feat/recurring-dk-test
branch
from
3d197d6 to
1e7d185
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/dispute-kits/DisputeKitClassicBase.sol (1)
141-144: notJumped can revert with storage OOB for unknown disputesIf called with a
_coreDisputeIDthis DK never created,coreDisputeIDToLocal[_coreDisputeID]is 0 anddisputes[0]may be OOB (length==0). Prefer checking activity first to provide a deterministic custom error instead of a panic.- modifier notJumped(uint256 _coreDisputeID) { - if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); + modifier notJumped(uint256 _coreDisputeID) { + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + uint256 local = coreDisputeIDToLocal[_coreDisputeID]; + if (disputes[local].jumped) revert DisputeJumpedToParentDK(); _; }
🧹 Nitpick comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
209-221: Recurring DK reuse path looks good; consider idempotent active-flag setReusing the existing localDisputeID and resetting jumped=false is correct for returning disputes. Optionally reassert
coreDisputeIDToActive[_coreDisputeID] = true;in both branches to be explicit/idempotent.if (coreDisputeIDToActive[_coreDisputeID]) { localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; dispute = disputes[localDisputeID]; dispute.jumped = false; + coreDisputeIDToActive[_coreDisputeID] = true; // idempotent } else { localDisputeID = disputes.length; dispute = disputes.push(); coreDisputeIDToLocal[_coreDisputeID] = localDisputeID; coreDisputeIDToActive[_coreDisputeID] = true; }
229-231: Guard against accidental underflow and cache the round IDMinor: cache the core round ID and assert it exists before subtracting. Improves readability and prevents a panic if call ordering ever changes.
- // New round in the Core should be created before the dispute creation in DK. - dispute.coreRoundIDToLocal[core.getNumberOfRounds(_coreDisputeID) - 1] = dispute.rounds.length; + // New round in the Core should be created before the dispute creation in DK. + uint256 coreRoundID = core.getNumberOfRounds(_coreDisputeID); + require(coreRoundID > 0, "Core round not initialized"); + dispute.coreRoundIDToLocal[coreRoundID - 1] = dispute.rounds.length; // 1-based storage
462-463: Avoid -1 underflow panics; centralize local round lookupAll these sites assume
coreRoundIDToLocal[_coreRoundID] != 0and do- 1. If unset, you get a panic (arithmetic or OOB). Tests currently rely on panics to detect “wrong DK/round”, but a targeted error improves debuggability and future flexibility.Suggestion:
- Add an internal helper returning the 0-based localRound index with a descriptive revert.
- Use it across read paths to remove duplication and accidental off-by-one errors.
// Add once: error RoundNotHandledByThisDK(); function _localRoundIndexOrRevert(Dispute storage d, uint256 _coreRoundID) internal view returns (uint256) { uint256 idx1 = d.coreRoundIDToLocal[_coreRoundID]; if (idx1 == 0) revert RoundNotHandledByThisDK(); // not mapped in this DK return idx1 - 1; // convert to 0-based }Then replace patterns like:
Round storage round = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID] - 1]; Vote storage vote = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID] - 1].votes[_voteID]; localRoundID = disputes[localDisputeID].coreRoundIDToLocal[_coreRoundID] - 1;with:
uint256 lr = _localRoundIndexOrRevert(dispute, _coreRoundID); Round storage round = dispute.rounds[lr]; Vote storage vote = dispute.rounds[lr].votes[_voteID]; localRoundID = _localRoundIndexOrRevert(disputes[localDisputeID], _coreRoundID);Note: Keeping current tests is possible by also testing for
RoundNotHandledByThisDK()in addition to panics.Also applies to: 566-567, 579-580, 647-648, 670-671, 698-699, 708-709
contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
471-800: Strong E2E coverage for recurring DK; consider reducing brittleness and duplication
- Reliance on
stdError.arithmeticErroris brittle; a future switch to explicit custom errors would break tests. Consider accepting both via try/catch or dedicated helpers.- Extract small helpers for repeated steps (stake/draw/vote/fund/expectJump) to cut boilerplate and improve readability.
If you want, I can propose a small set of reusable helpers for these flows.
Please confirm if we should keep asserting for panics vs. migrate to explicit error types in DK.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(10 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(2 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). (14)
- 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: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
28-28: Clarify 1-based index semantics in mappingStoring the length (1-based) is fine and matches the new read-path offsets. Please keep this invariant documented prominently, as misuse will cause subtle -1 underflows elsewhere.
432-436: Indexing for the next round on no-jump path is correctSetting
coreRoundIDToLocal[coreRoundID + 1] = dispute.rounds.length + 1before pushing the new round matches the 1-based scheme. Good.
jaybuidl
force-pushed
the
feat/recurring-dk-test
branch
from
1e7d185 to
536e61c
Compare
|
2 participants
PR-Codex overview
This PR focuses on enhancing the dispute handling mechanism in the
DisputeKitClassicBasecontract by allowing disputes to jump between different dispute kits (DKs). It also adds a comprehensive test to validate the behavior of disputes when transitioning between DKs.Detailed summary
DisputeKitClassicBaseto check if the dispute was already active.test_appeal_recurringDKto validate DK transitions and dispute behaviors.Summary by CodeRabbit
Bug Fixes
Tests