feat(DK): forking kit#2392
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-neo 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. |
Contributor
WalkthroughThis PR introduces ChangesForking Dispute Kit Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (2)
contracts/src/arbitration/KlerosCore.sol (1)
874-875: ⚡ Quick winClarify TODOs and verify placement for forking integration.
The TODO comments are vague and their placement may need reconsideration:
- "forking kit logic" doesn't specify what behavior is needed (e.g., should it prevent court jumps, create fork state, redirect to DisputeKitForking?).
- "handle arbitration fees in case of Forking" doesn't clarify whether fees should be refunded, held in escrow, or handled differently.
- These TODOs are placed after emitting the
CourtJumpevent but before updatingdispute.courtID. If forking logic needs to intercept or modify court jumps, this placement could emit misleading events or leave state partially updated.Consider:
- Making TODOs more specific (e.g., "TODO: If newCourtID is FORKING_COURT, initialize fork state in DisputeKitForking and skip standard appeal flow")
- Verifying whether the placement before
dispute.courtID = newCourtIDis intentional or whether forking checks should happen earlier in the function🤖 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 874 - 875, The two TODOs near the CourtJump emission are ambiguous and possibly misplaced; make them explicit and move or augment the checks so forking is handled before emitting CourtJump and before updating dispute.courtID: add a clear TODO like "TODO: if newCourtID == FORKING_COURT, initialize fork state via DisputeKitForking (create fork, assign fork ID) and bypass/alter standard appeal fee handling" and another like "TODO: define arbitration fee behavior on fork (refund to payer, hold in escrow, or transfer to fork bounty) and implement in the same pre-CourtJump branch"; ensure code paths that touch dispute.courtID and emit CourtJump only run after fork-specific initialization or redirection is resolved so events/state remain consistent.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
494-494: ⚡ Quick winProvide implementation guidance for forking court check.
The TODO lacks actionable details for the developer implementing this feature:
- Missing check: The code doesn't retrieve or check
dispute.courtIDfromcore.disputes(_coreDisputeID)to determine if the final court is the Forking court.- Unclear mechanism: "Force the tie distribution" could mean:
- Overriding
finalRulingto0before the loop (line 496)?- Modifying the conditional logic inside the loop to treat all funded choices equally?
- Applying tie distribution only to the last round while keeping earlier rounds' logic intact?
📝 Suggested implementation approach
Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; (uint256 finalRuling, , ) = core.currentRuling(_coreDisputeID); -// TODO: check if the final court is forking and if it is force the tie distribution for the last round. + +// Check if final court is FORKING_COURT and force tie distribution for last round +(uint96 finalCourtID, , , , ) = core.disputes(_coreDisputeID); +bool isForkingFinalCourt = (finalCourtID == FORKING_COURT); for (uint256 i = 0; i < dispute.rounds.length; i++) { Round storage round = dispute.rounds[i]; + + // Apply tie distribution in the last round if final court is Forking + bool applyTieDistribution = isForkingFinalCourt && (i == dispute.rounds.length - 1); + uint256 effectiveRuling = applyTieDistribution ? 0 : finalRuling; - if (!round.hasPaid[_choice]) { + if (!round.hasPaid[_choice]) { // Allow to reimburse if funding was unsuccessful for this ruling option. amount += round.contributions[_beneficiary][_choice]; } else { // Funding was successful for this ruling option. - if (_choice == finalRuling) { + if (_choice == effectiveRuling) { // This ruling option is the ultimate winner. amount += round.paidFees[_choice] > 0 ? (round.contributions[_beneficiary][_choice] * round.feeRewards) / round.paidFees[_choice] : 0; - } else if (!round.hasPaid[finalRuling]) { + } else if (applyTieDistribution || !round.hasPaid[effectiveRuling]) { // The ultimate winner was not funded in this round, or forking tie distribution applies amount += (round.contributions[_beneficiary][_choice] * round.feeRewards) / (round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]); } }🤖 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/DisputeKitClassic.sol` at line 494, Retrieve the dispute via core.disputes(_coreDisputeID) and read dispute.courtID, then query the court record to detect forking (e.g., core.courts(courtID).forking or a helper like core.isForkingCourt(courtID)); if the final court is a forking court, force tie handling for the last round by setting finalRuling = 0 before the loop that computes distributions and, when iterating the rounds (use rounds[rounds.length - 1] or the variable that denotes the last round), treat all funded choices as tied (i.e., don't prefer the current finalRuling, instead distribute tie shares equally among funded choices or use the same tie-distribution logic you apply elsewhere) while leaving earlier rounds’ logic unchanged; reference symbols: _coreDisputeID, dispute, dispute.courtID, core.courts / core.isForkingCourt, finalRuling, rounds and the last round index.
🤖 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/DisputeKitForking.sol`:
- Around line 184-197: In finalize(), avoid refunding the virtual TAIL sentinel
by checking localCutOffBidID (or bid.account == address(0)) and skipping the
refund/zeroing logic for that sentinel: if localCutOffBidID == TAIL then set
localCutOffBidID = fork.forkBids[localCutOffBidID].prev (advance to previous)
without calling fork.pinakion.safeTransfer or modifying value; otherwise proceed
as before but also guard safeTransfer calls by only calling when refund>0 and
bid.account != address(0) to prevent zero-address transfers and no-op transfers.
This change touches finalize(), the loop that reads
fork.cutOffBidID/localCutOffBidID, and the fork.forkBids[...].value/refund
handling referenced in the diff.
- Around line 281-287: DisputeKitForking.sol implements IDisputeKit but several
interface methods lack the override specifier; update the function signatures
for createDispute, draw, currentRuling, getDegreeOfCoherenceReward,
getDegreeOfCoherencePenalty, getCoherentCount, getRewards, areCommitsAllCast,
areVotesAllCast, isAppealFunded, getNextRoundSettings, isVoteActive, and
getVoteInfo to include the override keyword (e.g., function createDispute(...)
external onlyByCore override) so they properly override the IDisputeKit
interface; keep other modifiers and parameter names unchanged.
---
Nitpick comments:
In `@contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol`:
- Line 494: Retrieve the dispute via core.disputes(_coreDisputeID) and read
dispute.courtID, then query the court record to detect forking (e.g.,
core.courts(courtID).forking or a helper like core.isForkingCourt(courtID)); if
the final court is a forking court, force tie handling for the last round by
setting finalRuling = 0 before the loop that computes distributions and, when
iterating the rounds (use rounds[rounds.length - 1] or the variable that denotes
the last round), treat all funded choices as tied (i.e., don't prefer the
current finalRuling, instead distribute tie shares equally among funded choices
or use the same tie-distribution logic you apply elsewhere) while leaving
earlier rounds’ logic unchanged; reference symbols: _coreDisputeID, dispute,
dispute.courtID, core.courts / core.isForkingCourt, finalRuling, rounds and the
last round index.
In `@contracts/src/arbitration/KlerosCore.sol`:
- Around line 874-875: The two TODOs near the CourtJump emission are ambiguous
and possibly misplaced; make them explicit and move or augment the checks so
forking is handled before emitting CourtJump and before updating
dispute.courtID: add a clear TODO like "TODO: if newCourtID == FORKING_COURT,
initialize fork state via DisputeKitForking (create fork, assign fork ID) and
bypass/alter standard appeal fee handling" and another like "TODO: define
arbitration fee behavior on fork (refund to payer, hold in escrow, or transfer
to fork bounty) and implement in the same pre-CourtJump branch"; ensure code
paths that touch dispute.courtID and emit CourtJump only run after fork-specific
initialization or redirection is resolved so events/state remain consistent.
🪄 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: 0465cc9b-2036-4b38-a3fd-a52aca93986c
📒 Files selected for processing (3)
contracts/src/arbitration/KlerosCore.solcontracts/src/arbitration/dispute-kits/DisputeKitClassic.solcontracts/src/arbitration/dispute-kits/DisputeKitForking.sol
Comment on lines
+184
to
+197
| uint256 localCutOffBidID = fork.cutOffBidID; | ||
| uint256 localRemainingSupport = fork.remainingSupport; | ||
|
|
||
| // Search for the cut-off bid while removing bids whose minimum support is not met. | ||
| for (uint256 it = 0; it < _maxIt && !fork.finalized; it++) { | ||
| ForkBid storage bid = fork.forkBids[localCutOffBidID]; | ||
| if (bid.minSupport > localRemainingSupport && localCutOffBidID != HEAD) { | ||
| uint256 refund = bid.value; | ||
| bid.value = 0; | ||
| // Bid's condition is not satisfied, remove it. | ||
| localRemainingSupport -= refund; | ||
| localCutOffBidID = bid.prev; // Go to the previous bid. | ||
| // Refund the bid because its minimum support condition was not met. | ||
| fork.pinakion.safeTransfer(bid.account, refund); |
Contributor
There was a problem hiding this comment.
Skip the virtual TAIL bid before refunding in finalize().
createDispute() seeds cutOffBidID with TAIL, so the first finalize() pass always enters the prune branch on the sentinel node. That path zeroes/refunds forkBids[TAIL] and calls safeTransfer on address(0); if the token rejects zero-address transfers, finalization gets stuck, and this call site also ignores the transfer result for real refunds.
Suggested fix
- uint256 localCutOffBidID = fork.cutOffBidID;
+ uint256 localCutOffBidID = fork.cutOffBidID == TAIL ? fork.forkBids[TAIL].prev : fork.cutOffBidID;
uint256 localRemainingSupport = fork.remainingSupport;
...
- fork.pinakion.safeTransfer(bid.account, refund);
+ require(fork.pinakion.safeTransfer(bid.account, refund), TransferFailed());Also applies to: 309-309
🤖 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/DisputeKitForking.sol` around lines
184 - 197, In finalize(), avoid refunding the virtual TAIL sentinel by checking
localCutOffBidID (or bid.account == address(0)) and skipping the refund/zeroing
logic for that sentinel: if localCutOffBidID == TAIL then set localCutOffBidID =
fork.forkBids[localCutOffBidID].prev (advance to previous) without calling
fork.pinakion.safeTransfer or modifying value; otherwise proceed as before but
also guard safeTransfer calls by only calling when refund>0 and bid.account !=
address(0) to prevent zero-address transfers and no-op transfers. This change
touches finalize(), the loop that reads fork.cutOffBidID/localCutOffBidID, and
the fork.forkBids[...].value/refund handling referenced in the diff.
Comment on lines
+281
to
+287
| function createDispute( | ||
| uint256 _coreDisputeID, | ||
| uint256 /*_coreRoundID*/, | ||
| uint256 _finalRuling, | ||
| bytes calldata /*_extraData*/, | ||
| uint256 /*_nbVotes*/ | ||
| ) external onlyByCore { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'function (createDispute|draw|castCommit|castVote|fundAppeal|withdrawFeesAndRewards|currentRuling|getDegreeOfCoherenceReward|getDegreeOfCoherencePenalty|getCoherentCount|getRewards|areCommitsAllCast|areVotesAllCast|isAppealFunded|getNextRoundSettings|isVoteActive|getRoundInfo|getVoteInfo)\b' \
contracts/src/arbitration/interfaces/IDisputeKit.sol \
contracts/src/arbitration/dispute-kits/DisputeKitForking.solRepository: kleros/kleros-v2
Length of output: 3813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== IDisputeKit.sol =="
nl -ba contracts/src/arbitration/interfaces/IDisputeKit.sol | sed -n '1,260p'
echo
echo "== DisputeKitForking.sol (function signatures region) =="
nl -ba contracts/src/arbitration/dispute-kits/DisputeKitForking.sol | sed -n '250,570p'Repository: kleros/kleros-v2
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== IDisputeKit.sol =="
nl -ba contracts/src/arbitration/interfaces/IDisputeKit.sol | sed -n '1,260p'
echo
echo "== DisputeKitForking.sol (function signatures region) =="
nl -ba contracts/src/arbitration/dispute-kits/DisputeKitForking.sol | sed -n '250,570p'Repository: kleros/kleros-v2
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== IDisputeKit.sol (relevant functions) =="
cat -n contracts/src/arbitration/interfaces/IDisputeKit.sol | sed -n '1,260p'
echo
echo "== DisputeKitForking.sol (relevant functions) =="
cat -n contracts/src/arbitration/dispute-kits/DisputeKitForking.sol | sed -n '240,570p'Repository: kleros/kleros-v2
Length of output: 31305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DisputeKitForking.sol: contract declaration =="
rg -n "contract DisputeKitForking" -n contracts/src/arbitration/dispute-kits/DisputeKitForking.sol
sed -n '1,120p' contracts/src/arbitration/dispute-kits/DisputeKitForking.sol
echo
echo "== IDisputeKit.sol: check presence of extra methods =="
for fn in createDispute draw castCommit castVote fundAppeal withdrawFeesAndRewards currentRuling getDegreeOfCoherenceReward getDegreeOfCoherencePenalty getCoherentCount getRewards areCommitsAllCast areVotesAllCast isAppealFunded getNextRoundSettings isVoteActive getRoundInfo getVoteInfo; do
echo "--- $fn"
rg -n "\\bfunction[[:space:]]+$fn\\b" contracts/src/arbitration/interfaces/IDisputeKit.sol || true
doneRepository: kleros/kleros-v2
Length of output: 6215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "is IDisputeKit" contracts/src/arbitration/dispute-kits -S --glob '*.sol'
echo
echo "== Search for 'function getRoundInfo' override patterns =="
rg -n "function (createDispute|draw|currentRuling|getDegreeOfCoherenceReward|getDegreeOfCoherencePenalty|getRewards|getCoherentCount|areCommitsAllCast|areVotesAllCast|isAppealFunded|getNextRoundSettings|isVoteActive|getVoteInfo)\\b[^{;]*\\boverride\\b" contracts/src/arbitration/dispute-kits --glob '*.sol'Repository: kleros/kleros-v2
Length of output: 6168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol \
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol \
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol; do
echo "== $f (override on key IDisputeKit fns) =="
rg -n "function (createDispute|draw|castCommit|castVote|fundAppeal|withdrawFeesAndRewards|currentRuling|getDegreeOfCoherenceReward|getDegreeOfCoherencePenalty|getCoherentCount|getRewards|areCommitsAllCast|areVotesAllCast|isAppealFunded|getNextRoundSettings|isVoteActive|getRoundInfo|getVoteInfo)\\b" "$f"
echo
done
echo "== DisputeKitForking.sol (override presence) =="
rg -n "function (createDispute|draw|castCommit|castVote|fundAppeal|withdrawFeesAndRewards|currentRuling|getDegreeOfCoherenceReward|getDegreeOfCoherencePenalty|getCoherentCount|getRewards|areCommitsAllCast|areVotesAllCast|isAppealFunded|getNextRoundSettings|isVoteActive|getRoundInfo|getVoteInfo)\\b" contracts/src/arbitration/dispute-kits/DisputeKitForking.solRepository: kleros/kleros-v2
Length of output: 4080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DisputeKitClassic.sol pragma and createDispute/draw/currentRuling signatures =="
sed -n '1,40p' contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
sed -n '220,320p' contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
sed -n '540,620p' contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
echo
echo "== DisputeKitForking.sol pragma and createDispute/draw/currentRuling signatures =="
sed -n '1,50p' contracts/src/arbitration/dispute-kits/DisputeKitForking.sol
sed -n '260,420p' contracts/src/arbitration/dispute-kits/DisputeKitForking.solRepository: kleros/kleros-v2
Length of output: 20772
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "override[[:space:]]+virtual|virtual[[:space:]]+override" contracts/src/arbitration/dispute-kits --glob '*.sol' | head -n 50Repository: kleros/kleros-v2
Length of output: 391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DisputeKitClassic: getRewards/createDispute/draw/currentRuling signatures =="
sed -n '230,320p' contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
sed -n '520,860p' contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
echo
echo "== KlerosCore: call to DisputeKit.createDispute (search) =="
rg -n "createDispute\\(" contracts/src/arbitration/KlerosCore.sol contracts/src/arbitration -S --glob '*.sol' --glob '!**/test/**' --glob '!**/mocks/**'Repository: kleros/kleros-v2
Length of output: 26385
Add missing override specifiers for IDisputeKit methods in DisputeKitForking.sol.
DisputeKitForking implements IDisputeKit, but only getRoundInfo is marked override. Add override to: createDispute, draw, currentRuling, getDegreeOfCoherenceReward, getDegreeOfCoherencePenalty, getCoherentCount, getRewards, areCommitsAllCast, areVotesAllCast, isAppealFunded, getNextRoundSettings, isVoteActive, getVoteInfo. (castCommit, castVote, fundAppeal, withdrawFeesAndRewards are not in IDisputeKit.)
Suggested fix
function createDispute(
uint256 _coreDisputeID,
uint256 /*_coreRoundID*/,
uint256 _finalRuling,
bytes calldata /*_extraData*/,
uint256 /*_nbVotes*/
- ) external onlyByCore {
+ ) external override onlyByCore {
require(coreDisputeIDToForkID[_coreDisputeID] == 0, ForkAlreadyInitiated());
...
}
function draw(
uint256 /*_coreDisputeID*/,
uint256 /*_nonce*/,
uint256 /*_roundNbVotes*/
- ) external pure returns (address, uint96) {
+ ) external pure override returns (address, uint96) {
revert UnsupportedOperation();
}📝 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.
Suggested change
| function createDispute( | |
| uint256 _coreDisputeID, | |
| uint256 /*_coreRoundID*/, | |
| uint256 _finalRuling, | |
| bytes calldata /*_extraData*/, | |
| uint256 /*_nbVotes*/ | |
| ) external onlyByCore { | |
| function createDispute( | |
| uint256 _coreDisputeID, | |
| uint256 /*_coreRoundID*/, | |
| uint256 _finalRuling, | |
| bytes calldata /*_extraData*/, | |
| uint256 /*_nbVotes*/ | |
| ) external override onlyByCore { |
🤖 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/DisputeKitForking.sol` around lines
281 - 287, DisputeKitForking.sol implements IDisputeKit but several interface
methods lack the override specifier; update the function signatures for
createDispute, draw, currentRuling, getDegreeOfCoherenceReward,
getDegreeOfCoherencePenalty, getCoherentCount, getRewards, areCommitsAllCast,
areVotesAllCast, isAppealFunded, getNextRoundSettings, isVoteActive, and
getVoteInfo to include the override keyword (e.g., function createDispute(...)
external onlyByCore override) so they properly override the IDisputeKit
interface; keep other modifiers and parameter names unchanged.
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 introduces a new dispute kit,
DisputeKitForking, to handle forking logic in the Kleros arbitration system. It adds structures, functions, and events to manage bids and forks, while implementing necessary checks and modifiers for the forking process.Detailed summary
DisputeKitForkingcontract implementingIDisputeKit.ForkandForkBidstructures for managing bids.submitBid,searchAndBid,withdraw, andfinalizefunctions for bid management.ForkBidSubmittedfor bid submissions.onlyByCorefor access control.createDisputeandcurrentRuling.Summary by CodeRabbit
New Features
Chores