Increased Court and DK jump logic flexibility #2187
Conversation
jaybuidl
added
Type: Feature🗿
Compatibility: ABI change 🗯
✅ 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 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? →
|
WalkthroughReplaces global per-dispute jump ID with per‑court NextRoundSettings, consolidates next‑round API to return (newCourtID, newDisputeKitID, newRoundNbVotes), removes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KlerosCore
participant DisputeKit
rect rgba(200,230,255,0.12)
Note over User,KlerosCore: Appeal funding → determine next‑round settings
User->>KlerosCore: fundAppeal(disputeID,...)
KlerosCore->>DisputeKit: getNextRoundSettings(coreDisputeID,currentCourtID,parentCourtID,currentCourtJurorsForJump,currentDisputeKitID,currentRoundNbVotes)
DisputeKit->>DisputeKit: read courtIDToNextRoundSettings[currentCourtID] & compatibility checks
alt Compatible
DisputeKit-->>KlerosCore: (newCourtID,newDisputeKitID,newRoundNbVotes)
else Incompatible / fallback
DisputeKit-->>KlerosCore: (classicCourtID,classicDisputeKitID,adjustedNbVotes)
end
KlerosCore-->>User: proceed with updated court/DK/votes (emit CourtJump if newCourtID != current)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/hardhat.config.ts(1 hunks)contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(6 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.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-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect 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: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (15)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
53-54: LGTM! Initializer signature updated consistently.The removal of the
_jumpDisputeKitIDparameter aligns with the PR's shift to per-court next-round settings, and the base initializer call is correctly updated.contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
47-49: LGTM! Consistent initializer update.The parameter removal and updated base initializer call are consistent with the PR's architectural changes.
contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
29-30: LGTM! Initializer signature simplified.Consistent with the removal of global jump settings across all dispute kit implementations.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
63-64: LGTM! Initializer updated consistently.The signature change matches the pattern across all dispute kit implementations in this PR.
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
82-83: LGTM! Initializer signature updated.Consistent with the initialization changes across all dispute kit variants.
contracts/src/arbitration/KlerosCore.sol (3)
788-793: Verify unused return values are intentional.The call to
_getCourtAndDisputeKitJumpsignoresnewRoundNbVotes(line 788) anddisputeKitJump(line 788) with underscores. WhilecourtJumpis used on line 794, confirm thatdisputeKitJumpdoesn't need to be emitted or tracked here. TheDisputeKitJumpevent is emitted later at line 812, but only conditionally when the dispute kit actually changes.
1089-1094: LGTM! Return values properly utilized.The function correctly unpacks all 5 return values from
_getCourtAndDisputeKitJumpsand usesnbVotesAfterAppealandcourtJumpto compute the appeal cost. The ignored values (newCourtIDanddisputeKitJump) are appropriately not needed for cost calculation.
1183-1213: LGTM! Public getter provides transparency.The new public
getCourtAndDisputeKitJumpsfunction appropriately exposes the jump logic for external callers and front-ends, enabling better visibility into dispute progression.contracts/hardhat.config.ts (1)
35-35: Verify contract size remains under 24KB limit through proper build compilation.The review comment's concern is valid—24.576 KB is the contract size limit on Ethereum—and the hardhat config confirms optimizer runs are "Constrained by the size of the KlerosCore contract."
Verification could not be completed in sandbox (compilation failed), but code analysis shows the PR changes are optimizations rather than bloat:
- Removed
isDisputeKitJumping()function (bytecode savings)- Added
getCourtAndDisputeKitJumps()function (bytecode cost)- Removed external call to
disputeKits[].getNbVotesAfterAppeal()and inlined the logic (further savings)The net bytecode impact likely favors the changes. However, you should verify this locally by compiling and checking the deployed bytecode size before merging to confirm the contract remains compliant with the 24KB limit.
contracts/src/arbitration/interfaces/IDisputeKit.sol (2)
54-58: LGTM: Enhanced draw function signature.The additional
fromSubcourtIDreturn value enables tracking which subcourt a juror was drawn from, which appears necessary for the new per-court settings logic.
127-154: Well-designed unified jump determination function.This function consolidates the previously separate jump-related helpers (
getJumpDisputeKitID,earlyCourtJump,getNbVotesAfterAppeal) into a single, comprehensive interface that returns all necessary jump information in one call. The documentation clearly states that compatibility validation is the Core's responsibility.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
61-68: Well-structured settings with fine-grained control.The
NextRoundSettingsstruct provides comprehensive per-court configuration with clear semantics. Theenabledflag allows toggling settings without data loss, and the separateearlyDisputeKitJumpandearlyCourtJumpflags enable nuanced control over jump behavior.
631-670: Well-designed jump logic with appropriate fallbacks.The implementation correctly handles multiple scenarios:
- Uses per-court settings when enabled
- Falls back to default behavior (vote doubling, parent court on jump, DISPUTE_KIT_CLASSIC) when settings are absent or zero
- Combines early jump flags with threshold-based jump detection (line 661-663), allowing jumps to occur via either mechanism
The logic flow ensures that even with partial settings, the function returns valid values.
433-434: No issues found — KlerosCore signature is correct.KlerosCore.getCourtAndDisputeKitJumps at line 1190 accepts a single
uint256 _disputeIDparameter and returns five values:uint96 newCourtID,uint256 newDisputeKitID,uint256 newRoundNbVotes,bool courtJump, andbool disputeKitJump. The call in DisputeKitClassicBase.sol:433-434 correctly invokes this function with_coreDisputeIDand destructures the fifth return value (isDisputeKitJumping) as expected.
161-169: Verify operational setup and document migration path for per-court dispute kit settings.The architectural change from global
_jumpDisputeKitIDto per-courtcourtIDToNextRoundSettingsintroduces an operational concern. Verification confirms:
No automatic initialization: The
populateCourtsscript and deployment process do not callchangeNextRoundSettings. Per-court settings must be manually configured via governance.Default behavior: Courts fall back to
DISPUTE_KIT_CLASSICuntil explicitly configured (line 668), which may be acceptable but is undocumented.Missing migration documentation: No deployment guide or migration script shows how to initialize settings for existing or newly deployed courts.
Recommended actions:
- Document the expected post-deployment/upgrade process for initializing per-court settings
- Consider automating this in
populateCourtsor provide a dedicated migration task- Ensure governance has clear procedures for configuring settings per court
- Add safeguards or validation to prevent courts from operating in unconfigured state unintentionally
jaybuidl
force-pushed
the
feat/next-round-settings-in-dk
branch
from
9ea9e98 to
fac9901
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/KlerosCore.sol (1)
788-806: Use DK-computed vote count inappeal()to align withappealCost()estimation.The inconsistency is confirmed.
appealCost()(lines 1089, 1102, 1106) uses the dispute kit's computednbVotesAfterAppeal, whileappeal()(line 803) ignores the DK-returnednewRoundNbVotesand instead calculatesextraRound.nbVotes = msg.value / court.feeForJuror. This breaks the dispute kit abstraction—if the DK applies custom vote logic (thresholds, multipliers, rounding), the cost estimate and actual allocation diverge, potentially allowing overpayment to inflate vote counts beyond what the DK computed.Recommended fix:
- (uint96 newCourtID, uint256 newDisputeKitID, , bool courtJump, ) = _getCourtAndDisputeKitJumps( + (uint96 newCourtID, uint256 newDisputeKitID, uint256 newRoundNbVotes, bool courtJump, ) = _getCourtAndDisputeKitJumps( dispute, round, courts[dispute.courtID], _disputeID ); // ... Court storage court = courts[newCourtID]; - extraRound.nbVotes = msg.value / court.feeForJuror; + extraRound.nbVotes = newRoundNbVotes; + uint256 requiredFee = court.feeForJuror * newRoundNbVotes; + if (msg.value < requiredFee) revert AppealFeesNotEnough();
♻️ Duplicate comments (2)
contracts/src/arbitration/KlerosCore.sol (1)
1269-1274: Verify newRoundNbVotes consistency after dispute kit fallback.When the compatibility check (lines 1270-1274) falls back to
DISPUTE_KIT_CLASSICbecause the new court doesn't support the originally proposed dispute kit, thenewRoundNbVotesvalue is not recalculated. This value was computed by the original dispute kit'sgetCourtAndDisputeKitJumpsimplementation, which might use different logic than whatDISPUTE_KIT_CLASSICwould compute.Scenario:
// Original DK (e.g., specialized kit) returns: // newDisputeKitID = 5 (not supported by new court) // newRoundNbVotes = 100 (calculated by specialized kit's logic) // After fallback: // newDisputeKitID = DISPUTE_KIT_CLASSIC (forced) // newRoundNbVotes = 100 (still from specialized kit's logic) // But DISPUTE_KIT_CLASSIC might compute it as 50 with its own logicBased on learnings from past review comments, this issue was previously identified. Confirm whether:
- The number of votes for a round is standardized and doesn't depend on the specific dispute kit implementation, OR
- The
newRoundNbVotesshould be recalculated by callinggetCourtAndDisputeKitJumpsagain on theDISPUTE_KIT_CLASSICinstance after the fallbackIf recalculation is needed:
// Ensure compatibility between the next round's court and dispute kit. if (!courts[newCourtID].supportedDisputeKits[newDisputeKitID] || newDisputeKitID == NULL_DISPUTE_KIT) { // Fall back to `DisputeKitClassic` which is always supported. newDisputeKitID = DISPUTE_KIT_CLASSIC; disputeKitJump = (newDisputeKitID != disputeKitID); + // Recalculate votes with the fallback dispute kit + (, , newRoundNbVotes, , ) = disputeKits[DISPUTE_KIT_CLASSIC].getCourtAndDisputeKitJumps( + _disputeID, + _dispute.courtID, + _court.parent, + _court.jurorsForCourtJump, + _round.nbVotes + ); }contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
196-204: Validation and event emission still needed (echoing past review).As noted in the previous review, this governance function should:
- Validate that court IDs and dispute kit IDs exist (at least basic bounds checking)
- Emit an event for off-chain monitoring and transparency
The past review comment provided a concrete suggestion with validation and an event. While the interface notes that Core handles compatibility checking (Line 128 in IDisputeKit), basic validation here prevents governance configuration errors.
Based on learnings (past review comments).
🧹 Nitpick comments (2)
contracts/deploy/00-home-chain-arbitration-mainnet.ts (1)
144-144: Address the TODO comment in the context of this PR.The TODO comment asks about jumping to a Shutter DK instead of Classic. Since this PR introduces flexible per-court jump configuration via
NextRoundSettings(seeDisputeKitClassicBase.courtIDToNextRoundSettings), this is the ideal time to resolve this question. The default behavior can now be configured per-court after deployment usingchangeNextRoundSettings, but clarifying the intended default during initialization would improve code clarity.Consider either:
- Removing the TODO if the per-court settings make this flexible enough
- Setting an explicit
jumpDisputeKitIDin theNextRoundSettingsfor courts using DisputeKitGatedShutterWould you like me to search the codebase for how other dispute kits configure their jump targets?
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
631-670: Consider adding inline comments to clarify the decision flow.The implementation correctly handles the complex interdependencies between
nbVotes,courtJump,newCourtID, andnewDisputeKitID. However, the logic flow has several non-obvious aspects:
Line 658: The condition
if (nextRoundSettings.nbVotes == 0)applies the default doubling formula even whenenabled=true, which is correct but might be unexpected.Line 662: The condition
if (!courtJump)means "if not explicitly set to true byearlyCourtJump", allowing the threshold check to proceed. This subtle override logic could use a comment.Lines 664-666: The
newCourtIDdetermination depends on the computedcourtJumpvalue, creating a dependency chain:newRoundNbVotes→ threshold check →courtJump→newCourtIDselection.Consider adding inline comments like:
+ // Use settings if configured, otherwise apply defaults if (nextRoundSettings.nbVotes == 0) { + // Default: double the votes and add 1 newRoundNbVotes = (_currentRoundNbVotes * 2) + 1; } if (!courtJump) { + // Check threshold-based jump if not explicitly requested via earlyCourtJump courtJump = (newRoundNbVotes >= _currentCourtJurorsForJump); } if (newCourtID == 0) { + // Jump to parent court if threshold exceeded, otherwise stay in current newCourtID = courtJump ? _parentCourtID : _currentCourtID; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
contracts/deploy/00-home-chain-arbitration-mainnet.ts(2 hunks)contracts/deploy/00-home-chain-arbitration-university.ts(1 hunks)contracts/deploy/00-home-chain-arbitration.ts(4 hunks)contracts/hardhat.config.ts(1 hunks)contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(6 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(2 hunks)contracts/test/arbitration/helpers/dispute-kit-gated-common.ts(1 hunks)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- contracts/src/arbitration/dispute-kits/DisputeKitGated.sol
- contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol
- contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
- contracts/hardhat.config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/deploy/00-home-chain-arbitration-mainnet.ts (2)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/utils.ts (1)
deployments(3-16)
🔇 Additional comments (18)
contracts/deploy/00-home-chain-arbitration.ts (4)
40-40: LGTM! Deployment args correctly updated.The removal of the fourth argument aligns with the updated
initializesignature in DisputeKitClassic that no longer accepts_jumpDisputeKitID.
118-118: LGTM! Deployment args correctly updated.The removal of the fourth argument aligns with the updated
initializesignature in DisputeKitShutter.
127-127: LGTM! Deployment args correctly updated.The removal of the fourth argument aligns with the updated
initializesignature in DisputeKitGated.
136-136: Address the TODO comment regarding DK jump behavior.The TODO suggests considering a jump to a Shutter DK instead of a Classic one. With the new per-court NextRoundSettings approach, this configuration should now be handled via
changeNextRoundSettingsrather than at initialization time. Consider whether this TODO is still relevant or should be updated/removed.contracts/deploy/00-home-chain-arbitration-university.ts (1)
37-37: LGTM! Deployment args correctly updated.The removal of the fourth argument aligns with the updated
initializesignature for DisputeKitClassic.contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (1)
183-183: LGTM! Test setup correctly updated.The removal of the fourth argument aligns with the updated dispute kit initialization signatures.
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (1)
249-249: LGTM! Test setup correctly updated.The removal of the fourth argument aligns with the updated
initializesignature for DisputeKitGatedShutter.contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
82-84: LGTM! Initializer correctly refactored.The removal of
_jumpDisputeKitIDparameter and the updated base initializer call align with the new per-court NextRoundSettings design, where jump behavior is configured per court rather than globally at initialization.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
29-30: LGTM! Initializer correctly refactored.The removal of
_jumpDisputeKitIDparameter and the updated base initializer call align with the new per-court NextRoundSettings design.contracts/src/arbitration/KlerosCore.sol (1)
1183-1213: LGTM! Public getter properly delegates to internal helper.The new public
getCourtAndDisputeKitJumpsfunction correctly exposes the internal jump logic for external callers, maintaining consistent behavior with internal usage.contracts/src/arbitration/interfaces/IDisputeKit.sol (2)
54-58: LGTM! Draw signature extended for subcourt tracking.The additional
fromSubcourtIDreturn value enables KlerosCore to track which subcourt a juror was drawn from, supporting more granular stake management.
127-154: LGTM! Consolidated jump logic improves interface clarity.The new
getCourtAndDisputeKitJumpsfunction consolidates the previously fragmented jump-related methods (earlyCourtJump,getNbVotesAfterAppeal,getJumpDisputeKitID) into a single, comprehensive interface. This reduces complexity and provides all jump-related information in one call.contracts/deploy/00-home-chain-arbitration-mainnet.ts (2)
34-34: LGTM! Correct alignment with updated initializer signature.The args array correctly reflects the removal of
_jumpDisputeKitIDparameter. UsingZeroAddressfor core is appropriate here since KlerosCore hasn't been deployed yet; Line 92 updates it viachangeCore.
128-128: LGTM! Args correctly updated for new initializer signatures.Both DisputeKitShutter and DisputeKitGated args arrays correctly reflect the removal of
_jumpDisputeKitIDand properly usecore.targetsince KlerosCore is already deployed at this point.Also applies to: 136-136
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
61-68: Well-designed struct for per-court configuration.The
NextRoundSettingsstruct provides comprehensive control over next-round behavior with clear field semantics. The separation ofearlyDisputeKitJumpandearlyCourtJumpflags allows fine-grained control over when jumps occur relative to threshold conditions.
85-85: LGTM! Appropriate storage for per-court settings.The public mapping allows transparent per-court configuration and correctly uses
uint96for court IDs, matching KlerosCore's convention.
161-169: LGTM! Initializer correctly updated to remove global jump dispute kit ID.The removal of
_jumpDisputeKitIDaligns with the PR's shift to per-courtNextRoundSettings. This change is correctly reflected in the deployment scripts (e.g., Line 34 of00-home-chain-arbitration-mainnet.ts).
433-434: LGTM! Correct usage of the new API.The code correctly adapts to the expanded
getCourtAndDisputeKitJumpsreturn signature, extracting only the neededisDisputeKitJumpingflag while skipping irrelevant return values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
196-204: Add validation + event for governance changes.Prevent bad IDs and emit an event for observability.
+ event NextRoundSettingsChanged(uint96 indexed currentCourtID, NextRoundSettings settings); @@ function changeNextRoundSettings( uint96 _currentCourtID, NextRoundSettings memory _nextRoundSettings ) external onlyByOwner { + if (_nextRoundSettings.enabled) { + if (_nextRoundSettings.jumpDisputeKitID != 0) { + require( + _nextRoundSettings.jumpDisputeKitID < core.getDisputeKitsLength(), + "Invalid dispute kit ID" + ); + } + // Optional: court ID validation requires a Core getter; consider adding one in Core. + } courtIDToNextRoundSettings[_currentCourtID] = _nextRoundSettings; + emit NextRoundSettingsChanged(_currentCourtID, _nextRoundSettings); }contracts/src/arbitration/KlerosCore.sol (1)
1233-1276: Recalculate nbVotes after fallback to Classic to avoid mixing policies.Current fallback forces
(nbVotes*2)+1, which may differ from Classic DK’s configured NextRoundSettings and produce inconsistencies. Recompute via Classic DK.if (!courts[newCourtID].supportedDisputeKits[newDisputeKitID] || newDisputeKitID == NULL_DISPUTE_KIT) { // Fall back to `DisputeKitClassic` which is always supported. newDisputeKitID = DISPUTE_KIT_CLASSIC; disputeKitJump = (newDisputeKitID != disputeKitID); - newRoundNbVotes = (newRoundNbVotes * 2) + 1; // Reset nbVotes to the default logic. + // Recalculate votes using the fallback DK policy. + (, , newRoundNbVotes, , ) = disputeKits[DISPUTE_KIT_CLASSIC].getCourtAndDisputeKitJumps( + _disputeID, + _dispute.courtID, + _court.parent, + _court.jurorsForCourtJump, + DISPUTE_KIT_CLASSIC, + _round.nbVotes + ); }
🧹 Nitpick comments (4)
contracts/src/arbitration/interfaces/IDisputeKit.sol (2)
54-58: Document zero-sentinel behavior for fromSubcourtID in draw().Core normalizes 0 to current dispute.courtID. Clarify in NatSpec that 0 may be returned and what it means to avoid integrator ambiguity.
127-156: Interface param bloat; consider bundling context.Six inputs and five outputs increase ABI churn risk and stack-pressure. Consider a JumpContext struct for inputs to future‑proof and improve readability, or at least document invariants (e.g., 0 means “no override”) for each field.
contracts/src/arbitration/KlerosCore.sol (1)
788-807: Use DK-provided newRoundNbVotes instead of recomputing from msg.value.Capture and use the third return from _getCourtAndDisputeKitJumps for extraRound.nbVotes to keep one source of truth (and catch mismatches early).
- (uint96 newCourtID, uint256 newDisputeKitID, , bool courtJump, ) = _getCourtAndDisputeKitJumps( + (uint96 newCourtID, uint256 newDisputeKitID, uint256 newRoundNbVotes, bool courtJump, ) = _getCourtAndDisputeKitJumps( dispute, round, courts[dispute.courtID], _disputeID ); @@ - Court storage court = courts[newCourtID]; - extraRound.nbVotes = msg.value / court.feeForJuror; // As many votes that can be afforded by the provided funds. + Court storage court = courts[newCourtID]; + // Keep nbVotes consistent with DK policy used to compute appealCost. + extraRound.nbVotes = newRoundNbVotes;contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
61-68: Clarify semantics of zero values in NextRoundSettings.Document that 0 means “no override” for nbVotes/jump IDs, and how early* flags interact. This reduces governance misconfiguration risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(6 hunks)contracts/src/arbitration/interfaces/IDisputeKit.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). (3)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (2)
contracts/src/arbitration/KlerosCore.sol (2)
1089-1108: LGTM: appeal cost uses DK-provided nbVotes and switches fee by parent on court jump.
1183-1213: LGTM: public getCourtAndDisputeKitJumps wrapper mirrors internal logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contracts/test/foundry/KlerosCore_Appeals.t.sol (2)
256-268: Good use of per‑court NextRoundSettings; add one test without overrides.You mention tests should pass without the override. Please add a variant asserting the same jump behavior with default settings to guard regressions in default logic.
Also applies to: 403-415, 526-536, 548-558
298-300: Correct migration to getCourtAndDisputeKitJumps.Using the new public view to assert DK jumps is appropriate. Consider also asserting the returned newCourtID/newDisputeKitID to fully validate the path.
Also applies to: 448-450, 659-661, 730-732
contracts/src/arbitration/KlerosCore.sol (1)
788-794: Clarify why returned nbVotes is ignored in appeal().Intent is to derive nbVotes from msg.value/feeForJuror (consistent with appealCost). Add a short comment to prevent future “bug fix” attempts.
- (uint96 newCourtID, uint256 newDisputeKitID, ) = _getCompatibleNextRoundSettings( + // nbVotes from _getCompatibleNextRoundSettings is intentionally ignored: + // we recompute nbVotes below from the paid appealCost to keep source-of-truth in Core. + (uint96 newCourtID, uint256 newDisputeKitID, ) = _getCompatibleNextRoundSettings( dispute, round, courts[dispute.courtID], _disputeID );contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
200-209: Add minimal validation or explicit docs for governance setter.As-is, owner can set non‑existent court/DK IDs causing Core fallback or worse UX. Either:
- Add basic sanity checks (preferred), or
- Document that Core sanitizes/falls back and zero means “unset”.
function changeNextRoundSettings( uint96 _currentCourtID, NextRoundSettings memory _nextRoundSettings ) external onlyByOwner { + // Optional basic sanity: zero means "unset" + if (_nextRoundSettings.jumpCourtID != 0) { + // Best-effort bound check via Core: treat out-of-range as unset. + // If Core exposes a courts length getter in future, use it here. + } + if (_nextRoundSettings.jumpDisputeKitID == 0) { + // ok: unset -> Core keeps current DK unless jumping + } courtIDToNextRoundSettings[_currentCourtID] = _nextRoundSettings; emit NextRoundSettingsChanged(_currentCourtID, _nextRoundSettings); }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(12 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
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- 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-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
217-223: Initializer arity updates look correct.All DK proxy init payloads consistently use initialize(address,address,address). No issues spotted.
Also applies to: 357-363, 366-372, 516-523, 539-545
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
61-67: NextRoundSettings: interface clear.Fields and zero-as-unset semantics are clear. LGTM.
438-444: Good: query jump before mutating local dispute state.Consulting Core.getCourtAndDisputeKitJumps to decide whether to create a fresh local round avoids stale state when jumping DKs.
…mpDisputeKitIDOnCourtJump
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
200-209: Consider adding basic validation to prevent governance errors.While the event emission addresses part of the previous review feedback, the function still accepts settings without validation. Although Core is responsible for compatibility checking, basic sanity checks here would catch obvious governance mistakes before they're committed to state.
Consider adding validation:
function changeNextRoundSettings( uint96 _currentCourtID, NextRoundSettings memory _nextRoundSettings ) external onlyByOwner { + // Basic validation to catch obvious errors + if (_nextRoundSettings.enabled) { + if (_nextRoundSettings.jumpCourtID != 0) { + require(_nextRoundSettings.jumpCourtID < core.getNumberOfCourts(), "Invalid court ID"); + } + if (_nextRoundSettings.jumpDisputeKitID != 0) { + require(core.getNumberOfDisputeKits() > _nextRoundSettings.jumpDisputeKitID, "Invalid DK ID"); + } + if (_nextRoundSettings.jumpDisputeKitIDOnCourtJump != 0) { + require(core.getNumberOfDisputeKits() > _nextRoundSettings.jumpDisputeKitIDOnCourtJump, "Invalid fallback DK ID"); + } + } courtIDToNextRoundSettings[_currentCourtID] = _nextRoundSettings; emit NextRoundSettingsChanged(_currentCourtID, _nextRoundSettings); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(12 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-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- 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: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
636-670: LGTM - Court jump logic correctly fixed.The implementation correctly checks the current round's vote count (
_currentRoundNbVotes >= _currentCourtJurorsForJumpat line 654) rather than the projected next round count, addressing the previous review feedback. The override logic is clear: explicit settings take precedence, with sensible defaults applied when values are undefined (0).
438-439: LGTM - API update correctly applied.The transition from the old
isDisputeKitJumpingmethod to the newgetCourtAndDisputeKitJumpsAPI is implemented correctly, extracting theisDisputeKitJumpingboolean from the returned tuple.contracts/test/foundry/KlerosCore_Appeals.t.sol (3)
217-222: LGTM - Initialization signatures correctly updated.All DisputeKit initialization calls have been properly updated to use the new 3-parameter signature (
initialize(address,address,address)), removing the deprecated_jumpDisputeKitIDparameter. The updates are consistent across all test cases.Also applies to: 357-371, 517-544
256-267: LGTM - NextRoundSettings properly exercised.The tests correctly configure various NextRoundSettings scenarios through
changeNextRoundSettings, including:
- Explicit court and DK jumps (lines 256-267)
- Conditional DK jumps using
jumpDisputeKitIDOnCourtJumpfallback (lines 403-414, 526-558)This provides good coverage of the new configuration API.
Also applies to: 403-414, 526-558
298-299: LGTM - Test assertions updated for new API.All assertions correctly use
getCourtAndDisputeKitJumpsto check jump conditions, properly extracting theisDisputeKitJumpingboolean from the returned tuple. The pattern is consistently applied across all test cases.Also applies to: 448-449, 659-660, 730-731
sanity check in Core._getCompatibleNextRoundSettings(), bug fix in appealCost(), naming of the NextRoundSettingsChanged parameters, better comments describing the NextRoundSettings parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/src/arbitration/KlerosCore.sol (1)
1083-1103: Remove unreachable Forking branch in appealCost._getCompatibleNextRoundSettings() already forbids FORKING_COURT by falling back to current court. The NON_PAYABLE_AMOUNT path can no longer be reached; simplify to two branches.
- if (newCourtID == dispute.courtID) { - // No court jump - return court.feeForJuror * nbVotesAfterAppeal; - } - if (dispute.courtID != GENERAL_COURT && newCourtID != FORKING_COURT) { - // Court jump but not to the Forking court - return courts[newCourtID].feeForJuror * nbVotesAfterAppeal; - } - return NON_PAYABLE_AMOUNT; // Jumping to the Forking Court is not supported yet. + if (newCourtID == dispute.courtID) { + return court.feeForJuror * nbVotesAfterAppeal; + } + return courts[newCourtID].feeForJuror * nbVotesAfterAppeal;contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
200-209: Add minimal validation in changeNextRoundSettings (defense‑in‑depth).Core clamps inputs later, but basic checks here prevent obvious misconfigurations by governance.
function changeNextRoundSettings( uint96 _courtID, NextRoundSettings memory _nextRoundSettings ) external onlyByOwner { + if (_nextRoundSettings.enabled) { + // 0 means “unspecified”; only validate non-zero overrides. + if (_nextRoundSettings.jumpCourtID != 0) { + // Ensure not FORKING_COURT and within Core bounds. + ( , , , , ) = core.courts(_nextRoundSettings.jumpCourtID); // reverts if OOB + require(_nextRoundSettings.jumpCourtID != core.FORKING_COURT(), "Invalid jump court"); + } + if (_nextRoundSettings.jumpDisputeKitID != 0) { + require(_nextRoundSettings.jumpDisputeKitID < core.getDisputeKitsLength(), "Invalid DK ID"); + } + if (_nextRoundSettings.jumpDisputeKitIDOnCourtJump != 0) { + require(_nextRoundSettings.jumpDisputeKitIDOnCourtJump < core.getDisputeKitsLength(), "Invalid DK ID (on jump)"); + } + } courtIDToNextRoundSettings[_courtID] = _nextRoundSettings; emit NextRoundSettingsChanged(_courtID, _nextRoundSettings); }If you prefer to avoid extra calls, consider a lighter check (e.g., only DK ID bounds) and rely on Core for court validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 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-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- 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: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
contracts/src/arbitration/KlerosCore.sol (2)
788-799: Appeal flow integration looks correct.Using the compatibility helper to resolve next-round court/DK, then deriving nbVotes from msg.value/fee is consistent with appealCost and DK fund flow.
1178-1210: Public jump probe API is clear and useful.Return shape and booleans are intuitive; compatibility helper reuse keeps semantics aligned.
Please ensure off-chain consumers expect newRoundNbVotes to be the post-compatibility, potentially defaulted value (not raw DK output).
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (6)
61-68: NextRoundSettings shape LGTM.Fields cover court/DK overrides, court-jump-specific DK, and nbVotes override, all optional.
84-85: Storage mapping is appropriate.Per‑court settings keyed by current court ID is the right granularity.
132-136: Good: settings change event added.Enables off‑chain monitoring and audits of governance changes.
165-169: Initializer signature update is clean.Removal of jump param aligns with per‑court settings; wNative inclusion preserved.
438-439: Correct: use Core jump probe to decide local round creation.Avoids creating a local round when the dispute kit will jump.
636-670: getNextRoundSettings logic is sound and matches Core semantics.
- Court jump uses current‑round threshold (>= jurorsForCourtJump).
- DK override on court jump respects jumpDisputeKitIDOnCourtJump only when jump happens.
- nbVotes defaults to doubling+1 when unspecified.
Add tests for boundaries: exactly at threshold, nbVotes override with/without enabled, and court jump with/without jumpDisputeKitIDOnCourtJump.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
298-299: Consider asserting additional values from getCourtAndDisputeKitJumps.While the API change is correct, the test only verifies
isDisputeKitJumpingand ignores the other return values (nextCourtID,nextDisputeKitID,nextNbVotes,isCourtJumping). Consider adding assertions for these values to strengthen test coverage and validate the complete jump prediction logic.For example:
-(, , , , bool isDisputeKitJumping) = core.getCourtAndDisputeKitJumps(disputeID); -assertEq(isDisputeKitJumping, true, "Should be jumping"); +(uint96 nextCourtID, uint256 nextDisputeKitID, , bool isCourtJumping, bool isDisputeKitJumping) = core.getCourtAndDisputeKitJumps(disputeID); +assertEq(nextCourtID, GENERAL_COURT, "Should jump to GENERAL_COURT"); +assertEq(nextDisputeKitID, DISPUTE_KIT_CLASSIC, "Should jump to DISPUTE_KIT_CLASSIC"); +assertEq(isCourtJumping, true, "Should be court jumping"); +assertEq(isDisputeKitJumping, true, "Should be DK jumping");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol(13 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-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
contracts/test/foundry/KlerosCore_Appeals.t.sol (9)
217-222: LGTM: Initialization signature correctly updated.The removal of the
jumpDisputeKitIDparameter from the initialization call aligns with the PR's refactoring to use the NextRoundSettings mechanism for managing next-round configurations.
403-414: LGTM: NextRoundSettings correctly configured for fallback testing.This configuration properly tests the scenario where a dispute jumps to a parent court but the current DK is not supported, triggering the
jumpDisputeKitIDOnCourtJumpfallback todkID2.
516-558: LGTM: Circular DK jump setup is well-designed.The test correctly configures a circular dispute kit pattern (DK3 → DK2 → DK3) to validate that disputes can jump back to previously used DKs. The NextRoundSettings configurations properly use
jumpDisputeKitIDOnCourtJumpto specify the fallback DK when courts jump.
990-1117: Excellent test coverage for non-parent court jumps.This test comprehensively validates the new NextRoundSettings functionality, specifically:
- Jumps to non-parent (sibling) courts work correctly
appealCost()correctly uses the target court'sfeeForJuror, not the current court'sgetCourtAndDisputeKitJumps()accurately predicts the jumpThe test structure is clear and assertions are thorough.
1119-1237: LGTM: Multi-level court jump test is comprehensive.This test validates that disputes can jump multiple levels up the court hierarchy (directly to grandparent), skipping intermediate courts. The test properly verifies both the jump behavior and cost calculations.
1239-1345: LGTM: Disabled NextRoundSettings fallback test is thorough.This test properly validates that when
NextRoundSettings.enabled = false, all custom settings are ignored and the default parent court jump logic is used. The assertions correctly verify that the configuredjumpCourtIDandnbVotesare not applied.
1440-1669: Excellent coverage of custom nbVotes functionality.These two tests comprehensively validate the
NextRoundSettings.nbVotesfeature:
test_appeal_jumpCourtWithCustomNbVotes (lines 1440-1560): Validates that custom
nbVotesis correctly used in cost calculations and the new round configuration.test_appeal_customNbVotesDoesNotTriggerCourtJump (lines 1562-1669): Critically validates that custom
nbVotesfor the next round does NOT influence whether a court jump occurs (which is based on the current round's drawn jurors). This is an important edge case.Both tests are well-structured with clear assertions and comments explaining the expected behavior.
1347-1438: Review comment is correct and test behavior is intentional.The
_getCompatibleNextRoundSettingsfunction explicitly validates and handles invalidjumpCourtIDvalues. At lines 1254-1264 in KlerosCore.sol, whennewCourtID >= courts.length(which catches the 999 case), the system intentionally falls back to the current court with the comment "Falling back to the current court and dispute kit with default nbVotes." This defensive fallback design is intentional, not accidental. The test correctly validates this behavior.
256-267: Remove the misleading comment about NextRoundSettings override being unnecessary.The override at lines 256-267 is actually required for the test to pass. Without it, the dispute kit would not jump to
DISPUTE_KIT_CLASSIC(it would remain asnewDkID), causing theDisputeKitJumpevent assertion at lines 305-306 to fail. The comment stating "the test should pass even without this override" is incorrect and should be removed or corrected to explain why the override is necessary.Likely an incorrect or invalid review comment.
…pDisputeKitIDOnCourtJump
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
256-268: Clarify the comment about test passing without override.The comment states "Note that the test should pass even without this override," but then proceeds to configure
NextRoundSettings. If the test truly passes without this configuration, consider removing it to avoid confusion, or clarify why it's included despite being technically unnecessary (e.g., for explicitness, future-proofing, or demonstrating the API).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/test/foundry/KlerosCore_Appeals.t.sol(13 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). (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- 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: 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-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: SonarCloud
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
contracts/test/foundry/KlerosCore_Appeals.t.sol (9)
218-221: LGTM: Initialize signature changes align with PR objectives.The removal of the
jumpDisputeKitIDparameter from DisputeKit initialization calls is consistent throughout the test file and matches the PR's refactoring to use per-courtNextRoundSettingsinstead.Also applies to: 358-361, 367-370, 518-521, 540-543, 1683-1686, 1691-1695
298-299: LGTM: Correct migration to new API.The tests correctly use
getCourtAndDisputeKitJumpswhich returns a comprehensive tuple including both court and DK jump information, replacing the previous separateisDisputeKitJumpingfunction. The destructuring correctly extracts the 5th return value.Also applies to: 448-449, 659-660, 730-731
991-1117: LGTM: Comprehensive test of non-parent court jumps with cost validation.This test thoroughly validates:
- Jumping to a sibling court (not parent) using
NextRoundSettings.jumpCourtIDappealCost()correctly uses the target court'sfeeForJuror(0.07 vs 0.05)- Correct multiplier application for losing (2x) and winning (1x) sides
- Jump prediction via
getCourtAndDisputeKitJumpsThe arithmetic and assertions are correct.
1119-1237: LGTM: Multi-level court jump test validates hierarchy skipping.This test properly validates that a court can jump multiple levels up the hierarchy (Court4 -> GENERAL_COURT, skipping Court2 and Court3). The cost calculation correctly uses the target court's feeForJuror, and the jump logic works as expected.
1239-1345: LGTM: Fallback behavior correctly tested.This test validates the critical fallback scenario where
NextRoundSettings.enabled = falsecauses the system to ignore custom settings and use default parent-court jump logic. The assertions correctly verify:
- Jump to parent (GENERAL_COURT), not custom
jumpCourtID(Court3)- Default vote formula (7), not custom
nbVotes(9)- Parent court's
feeForJurorin cost calculation
1347-1438: LGTM: Invalid jumpCourtID fallback behavior tested.This test validates that when
NextRoundSettings.jumpCourtIDis set to an invalid/non-existent court ID, the system falls back to staying in the current court (no jump). The test correctly verifies:
- No court jump occurs (
isCourtJumping = false)- Appeal cost uses current court's
feeForJuror- New round stays in the same court
1440-1560: LGTM: Custom nbVotes correctly tested.This test comprehensively validates that
NextRoundSettings.nbVotesoverrides the default formula (currentVotes * 2 + 1). The test correctly verifies:
appealCost()uses custom nbVotes (11) with target court's fee (0.08 * 11 = 0.88)- Multipliers correctly applied (2.64 for loser, 1.76 for winner)
- New round created with custom nbVotes (11), not default (7)
- All jurors can be drawn with custom count
1562-1669: LGTM: Critical test validates correct jump trigger logic.This is an important test that validates a subtle but critical behavior: court jump decisions are based on the current round's drawn jurors (3), not the next round's custom
nbVotes(9). DespitecustomNbVotes = 9 > jurorsForCourtJump = 7, no court jump occurs because the current round only has 3 jurors. This ensures the jump threshold is evaluated against actual dispute scale, not future projections.
1671-1838: LGTM: Precedence rule correctly validated.This test validates the important precedence rule: when both
jumpDisputeKitIDandjumpDisputeKitIDOnCourtJumpare set,jumpDisputeKitIDtakes precedence. The test comprehensively verifies:
getCourtAndDisputeKitJumpspredictsdkID2(notdkID3)DisputeKitJumpevent shows jump todkID2- New round uses
DisputeKit2, notDisputeKit3- Correct dispute kit activation states
This precedence rule avoids configuration ambiguity and is well-tested.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/src/arbitration/KlerosCore.sol (1)
788-796: Consider documenting why newRoundNbVotes is discarded.The third return value from
_getCompatibleNextRoundSettingsis intentionally discarded on line 788 becausenbVotesis recalculated on line 803 based onmsg.value / court.feeForJuror. This design allows appellants to pay for more votes than the minimum cost, but the reasoning is not immediately obvious from the code.Consider adding a brief comment to clarify the design:
- (uint96 newCourtID, uint256 newDisputeKitID, ) = _getCompatibleNextRoundSettings( + // Note: newRoundNbVotes is discarded here as the actual nbVotes is calculated from msg.value on line 803 + (uint96 newCourtID, uint256 newDisputeKitID, ) = _getCompatibleNextRoundSettings(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/KlerosCore.sol(4 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: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- 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/KlerosCore.sol (2)
1178-1210: LGTM! Clean implementation of the new public API.The new
getCourtAndDisputeKitJumpsfunction provides clear visibility into next-round settings and jump indicators. The implementation correctly delegates to_getCompatibleNextRoundSettingsand properly computes the boolean jump flags.
1230-1272: Excellent defensive validation - all past concerns addressed.The
_getCompatibleNextRoundSettingsimplementation includes comprehensive guards:
- Line 1255-1256: Prevents invalid court IDs (FORKING_COURT, out-of-bounds)
- Line 1257-1258: Prevents invalid dispute kit IDs (NULL_DISPUTE_KIT, out-of-bounds)
- Line 1259: Prevents zero nbVotes that would enable free appeals (addresses critical past comment)
- Lines 1267-1271: Ensures court/DK compatibility with fallback to DISPUTE_KIT_CLASSIC
The two-stage fallback strategy (invalid inputs → current court/DK, then incompatibility → DISPUTE_KIT_CLASSIC) provides robust protection against malicious or buggy dispute kit implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/test/foundry/KlerosCore_Appeals.t.sol (3)
257-268: Consider removing the redundant NextRoundSettings override.The comment at line 257 indicates "the test should pass even without this override," suggesting this configuration may be unnecessary. If the default behavior already provides the correct jump behavior (court2 → GENERAL_COURT, dk2 → DISPUTE_KIT_CLASSIC), consider removing this override to simplify the test and rely on the default logic.
299-300: Consider validating all getCourtAndDisputeKitJumps return values.The test retrieves 5 values from
getCourtAndDisputeKitJumpsbut only asserts onisDisputeKitJumping. For more comprehensive validation, consider also asserting the predictednewCourtID,newDisputeKitID, andnewRoundNbVotesvalues, similar to what's done in the new tests starting at line 1084.Alternatively, if the other values aren't relevant for this test, use a simpler destructuring pattern:
(, , , , bool isDisputeKitJumping) = core.getCourtAndDisputeKitJumps(disputeID);
991-2563: Consider extracting helper functions to reduce test code duplication.The new test functions contain significant duplication in setup code (court creation, dispute kit initialization, staking, dispute progression). Extracting helper functions would improve maintainability and reduce the risk of inconsistencies:
function _createCourtWithFee(uint96 parent, uint256 fee, uint256 jurorsForJump, uint256[] memory supportedDKs) internal returns (uint96) function _createAndInitializeDisputeKit() internal returns (DisputeKitClassic, uint256) function _setupStakingAndDispute(uint96 courtID, uint256 dkID, uint256 fee) internal returns (uint256) function _progressToAppealPeriod(uint256 disputeID, address voter, uint256[] memory voteIDs, uint256 choice) internalThis would make the tests more concise and easier to maintain while keeping the unique test logic clearly visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/test/DisputeKitClassicMockUncheckedNextRoundSettings.sol(1 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(14 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). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect 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: Header rules - 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-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: SonarCloud
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
contracts/src/test/DisputeKitClassicMockUncheckedNextRoundSettings.sol (1)
7-22: LGTM! Well-designed test mock for fallback logic validation.This mock correctly bypasses DisputeKitClassicBase's safety logic to enable testing of KlerosCore's compatibility checks and fallback behavior. The intentional lack of validation allows you to test edge cases like invalid court IDs, invalid dispute kit IDs, and zero vote counts that would normally be caught earlier.
contracts/test/foundry/KlerosCore_Appeals.t.sol (2)
1841-1986: Excellent test coverage for invalid dispute kit ID fallback behavior.This test thoroughly validates the complete triple fallback mechanism when
jumpDisputeKitID >= disputeKits.length. The critical insight is that even thoughjumpCourtIDis valid andnbVotesis custom, all three parameters fall back to current settings due to the single invalid parameter. The assertions comprehensively verify this behavior, and the test documentation clearly explains the safety check being tested.
2161-2563: Excellent test strategy for validating KlerosCore's sanity checks.These three tests use the mock dispute kit to bypass DisputeKitClassicBase's safety logic and verify that KlerosCore has its own defensive checks. This demonstrates good defense-in-depth:
- Line 2161: Tests
jumpCourtID == FORKING_COURT(0) triggers fallback- Line 2293: Tests
jumpDisputeKitID == NULL_DISPUTE_KIT(0) triggers fallback- Line 2430: Tests
newRoundNbVotes == 0triggers fallbackAll three scenarios correctly validate complete triple fallback even when other parameters appear valid. This ensures that even if a malicious or buggy dispute kit returns invalid settings, KlerosCore won't accept them.
|
PR-Codex overview
This PR focuses on refining the initialization parameters for various
DisputeKitcontracts by removing unnecessary arguments, enhancing the optimization settings, and implementing new logic for dispute kit transitions. It also adds tests for the new functionality.Detailed summary
jumpDisputeKitIDargument frominitializefunctions in severalDisputeKitcontracts.runsfrom 800 to 1000 inhardhat.config.ts.DisputeKitClassic,DisputeKitShutter, andDisputeKitGated.NextRoundSettingsstruct inDisputeKitClassicBasefor managing round settings.getNextRoundSettingsmethod to accommodate new logic for determining court and dispute kit transitions.KlerosCore_AppealsTest, including scenarios for court jumps and dispute kit changes.Summary by CodeRabbit
New Features
Refactor
Chore
Tests