feat(contracts): forking mechanism TDD scaffold (skeletons + Foundry tests) - #2399
feat(contracts): forking mechanism TDD scaffold (skeletons + Foundry tests)#2399jaybuidl wants to merge 1 commit into
Conversation
…tests) Scaffolds the forking mechanism (docs/layer-1-core/07-forking.md) test-first: Foundry suites encode the spec's guarantees, invariants, and the two canonical worked examples, and compile-ready contract skeletons let them run red until the implementation pass turns them green one suite at a time. New contracts (skeletons; bodies revert NotImplemented or return defaults): - ForkMath: removal fixed point (one threshold-sorted list per losing option). - DisputeKitForking: reworked IDisputeKit at the Core boundary; inert views are real, commit/reveal/finalize are stubbed. createDispute's 3rd arg is numberOfChoices per IDisputeKit (the earlier draft misnamed it finalRuling). - ForkSettlement: paginated Capture/MainDistrib/Mint/Done phase machine. - ForkToken: fully implemented mintable ERC-20 (one per minority fork) — green. - PNKHolderEscrow: tier-2 participation path. - ISlashDestination: isolates the slash-destination decision (Q-006). Additive-only changes to existing contracts (new surface stubbed; existing appeal/execute/appealCost/initialize behavior untouched, to be filled in the implementation pass): - SortitionModule: stakingFrozen flag; freeze/unfreeze/captureUnstakeAllCourts. - KlerosCore: forkSettlement storage + setForkSettlement; captureStakeForForking and distributeForking hooks (onlyForkSettlement); ForkingRoundStarted event; AppealNotAllowed/NotForkingCourt errors. - ISortitionModule / KlerosProxies: matching declarations + DisputeKitForkingProxy. Tests (test/foundry/): ForkMath (canonical fixtures), ForkToken (green anchor), SortitionModule_Freeze, DisputeKitForking, ForkSettlement, KlerosCore_ForkingCourt, Forking_Integration, plus ForkingTestBase wiring the jump routing. Existing core suites still pass (no regressions). https://claude.ai/code/session_01W7bRbuy6gnrpKX5VctbC9K
❌ 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. |
WalkthroughThis PR introduces a TDD scaffolding for Kleros v2's forking mechanism. New contracts ChangesForking Mechanism TDD Scaffold
Sequence Diagram(s)sequenceDiagram
participant Juror
participant KlerosCore
participant SortitionModule
participant DisputeKitForking
participant ForkSettlement
rect rgba(173, 216, 230, 0.5)
note over Juror,KlerosCore: Appeal into forking court
Juror->>KlerosCore: appeal() — fund into FORKING_COURT
KlerosCore->>SortitionModule: freeze(disputeID)
SortitionModule-->>KlerosCore: stakingFrozen=true, emit StakingFreezeEngaged
KlerosCore->>DisputeKitForking: createDispute(coreDisputeID, numberOfChoices)
KlerosCore-->>Juror: emit ForkingRoundStarted
end
rect rgba(255, 200, 150, 0.5)
note over Juror,KlerosCore: Forking round voting (stubs)
Juror->>DisputeKitForking: commitVote(coreDisputeID, commit)
DisputeKitForking-->>Juror: revert NotImplemented
Juror->>DisputeKitForking: revealVote(coreDisputeID, choice, threshold, salt)
DisputeKitForking-->>Juror: revert NotImplemented
Juror->>KlerosCore: appeal() after forking
KlerosCore-->>Juror: revert AppealNotAllowed
end
rect rgba(180, 255, 180, 0.5)
note over ForkSettlement,SortitionModule: Settlement (stubs)
ForkSettlement->>KlerosCore: captureStakeForForking(joiner)
KlerosCore-->>ForkSettlement: revert NotImplemented
ForkSettlement->>SortitionModule: unfreeze()
SortitionModule-->>ForkSettlement: revert NotImplemented
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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)
Comment |
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
contracts/test/foundry/SortitionModule_Freeze.t.sol (1)
89-98: ⚡ Quick winAdd
unfreezeaccess-control assertion to match the test intent.The section states freeze/unfreeze/capture are Core-only, but this test only checks freeze and capture. Add a non-core
unfreezerevert assertion to fully lock the contract boundary.🤖 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/test/foundry/SortitionModule_Freeze.t.sol` around lines 89 - 98, The test_onlyCoreCanFreeze test in SortitionModule_Freeze.t.sol is incomplete. While the comment states it tests that freeze, unfreeze, and capture are Core-only operations, the test only verifies freeze and captureUnstakeAllCourts. Add another assertion block to the test that checks the unfreeze method also reverts with KlerosCoreOnly when called by a non-core account (other), using vm.expectRevert and vm.prank to match the pattern of the existing checks.contracts/test/foundry/ForkToken.t.sol (1)
43-49: ⚡ Quick winAssert per-holder balances in the multi-mint supply test.
Line 48 verifies total supply, but this test can still pass if minted amounts are misrouted between recipients. Add holder-level balance assertions to lock the behavior.
Proposed test hardening
function test_mint_tracksSupplyAcrossHolders() public { vm.startPrank(settlement); token.mint(alice, 700); token.mint(bob, 300); vm.stopPrank(); + assertEq(token.balanceOf(alice), 700, "alice genesis balance mismatch"); + assertEq(token.balanceOf(bob), 300, "bob genesis balance mismatch"); assertEq(token.totalSupply(), 1000, "supply must equal the sum of genesis balances"); }🤖 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/test/foundry/ForkToken.t.sol` around lines 43 - 49, In the test_mint_tracksSupplyAcrossHolders function, add holder-level balance assertions to verify that alice received exactly 700 tokens and bob received exactly 300 tokens after their respective mint calls. Currently the test only asserts the total supply equals 1000, which can pass even if tokens are misdirected between recipients. Use assertEq to check token.balanceOf(alice) and token.balanceOf(bob) against their expected amounts to ensure the minted amounts are routed to the correct recipients.contracts/test/foundry/ForkSettlement.t.sol (1)
41-45: ⚡ Quick winAdd a DK-authorized
slashplaceholder-revert test.This file verifies
slashaccess control, but it doesn’t assert current RED behavior for the authorized path. Add a test wheremsg.sender == address(forkingDK)andslashrevertsForkSettlement.NotImplemented.Suggested test addition
+ function test_slash_notImplemented() public { + vm.expectRevert(ForkSettlement.NotImplemented.selector); + vm.prank(address(forkingDK)); + forkSettlement.slash(DISPUTE_ID, staker1, 100); + }🤖 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/test/foundry/ForkSettlement.t.sol` around lines 41 - 45, Add a new test function in ForkSettlement.t.sol after test_slash_onlyByDK that verifies the authorized path behavior. This test should use vm.prank to set the caller to the forkingDK address, then expect the slash function to revert with ForkSettlement.NotImplemented selector before calling forkSettlement.slash with test parameters (DISPUTE_ID, staker1, and an amount). This validates that while the DisputeKit has permission to call slash, the function currently returns a NotImplemented revert rather than executing the logic.
🤖 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/ForkSettlement.sol`:
- Around line 105-111: The constructor in ForkSettlement.sol does not validate
that the _disputeKit parameter is not address(0), which could brick the contract
if an invalid address is passed. Add a require statement in the constructor to
validate that _disputeKit is not address(0), placing this check before the
assignment to the disputeKit state variable. If the validation fails, the
transaction should revert with a descriptive error message indicating that
_disputeKit cannot be the zero address.
In `@contracts/src/arbitration/dispute-kits/PNKHolderEscrow.sol`:
- Around line 77-79: The setForkSettlement function has two issues: it uses
NotImplemented() error which is inappropriate for an authorization check and
should be replaced with a proper auth-related error, and it accepts address(0)
for _forkSettlement which could cause routing misconfiguration. Add a require
statement to validate that _forkSettlement is not the zero address and revert
with an appropriate error if it is, and change the existing require statement to
use an explicit authorization error instead of NotImplemented().
- Around line 69-73: The constructor in PNKHolderEscrow does not validate that
the _owner parameter is non-zero, which allows owner to be set to address(0) and
makes the setForkSettlement function permanently unreachable and the escrow
unconfigurable. Add a require statement in the constructor to validate that
_owner is not address(0) before assigning it to the owner state variable.
In `@contracts/test/foundry/DisputeKitForking.t.sol`:
- Around line 27-35: The test function
`test_createDispute_storesNumberOfChoices` does not actually assert that the
numberOfChoices value (3) is stored correctly, despite its name suggesting this
is the primary test objective. After the first vm.prank and
forkingDK.createDispute call, add an assertion that verifies the stored
numberOfChoices for DISPUTE_ID equals 3 by using an existing getter method on
the forkingDK contract to retrieve the stored numberOfChoices value.
In `@contracts/test/foundry/Forking_Integration.t.sol`:
- Line 69: The test is using hardcoded appeal funding amounts (0.63 ether) in
multiple calls to disputeKit.fundAppeal, which creates brittle tests that fail
when fee configurations change. Instead of hardcoding this value, compute the
required appeal amount at runtime from the current protocol state (likely from
the dispute kit configuration or related fee parameters) and use that computed
value for all fundAppeal calls throughout the test. This ensures the test
remains resilient to changes in protocol fees.
---
Nitpick comments:
In `@contracts/test/foundry/ForkSettlement.t.sol`:
- Around line 41-45: Add a new test function in ForkSettlement.t.sol after
test_slash_onlyByDK that verifies the authorized path behavior. This test should
use vm.prank to set the caller to the forkingDK address, then expect the slash
function to revert with ForkSettlement.NotImplemented selector before calling
forkSettlement.slash with test parameters (DISPUTE_ID, staker1, and an amount).
This validates that while the DisputeKit has permission to call slash, the
function currently returns a NotImplemented revert rather than executing the
logic.
In `@contracts/test/foundry/ForkToken.t.sol`:
- Around line 43-49: In the test_mint_tracksSupplyAcrossHolders function, add
holder-level balance assertions to verify that alice received exactly 700 tokens
and bob received exactly 300 tokens after their respective mint calls. Currently
the test only asserts the total supply equals 1000, which can pass even if
tokens are misdirected between recipients. Use assertEq to check
token.balanceOf(alice) and token.balanceOf(bob) against their expected amounts
to ensure the minted amounts are routed to the correct recipients.
In `@contracts/test/foundry/SortitionModule_Freeze.t.sol`:
- Around line 89-98: The test_onlyCoreCanFreeze test in
SortitionModule_Freeze.t.sol is incomplete. While the comment states it tests
that freeze, unfreeze, and capture are Core-only operations, the test only
verifies freeze and captureUnstakeAllCourts. Add another assertion block to the
test that checks the unfreeze method also reverts with KlerosCoreOnly when
called by a non-core account (other), using vm.expectRevert and vm.prank to
match the pattern of the existing checks.
🪄 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: c076e58c-f9bb-40b4-95ab-45141e84c229
📒 Files selected for processing (19)
contracts/src/arbitration/KlerosCore.solcontracts/src/arbitration/SortitionModule.solcontracts/src/arbitration/dispute-kits/DisputeKitForking.solcontracts/src/arbitration/dispute-kits/ForkMath.solcontracts/src/arbitration/dispute-kits/ForkSettlement.solcontracts/src/arbitration/dispute-kits/PNKHolderEscrow.solcontracts/src/arbitration/interfaces/ISlashDestination.solcontracts/src/arbitration/interfaces/ISortitionModule.solcontracts/src/proxy/KlerosProxies.solcontracts/src/test/ForkMathHarness.solcontracts/src/token/ForkToken.solcontracts/test/foundry/DisputeKitForking.t.solcontracts/test/foundry/ForkMath.t.solcontracts/test/foundry/ForkSettlement.t.solcontracts/test/foundry/ForkToken.t.solcontracts/test/foundry/ForkingTestBase.solcontracts/test/foundry/Forking_Integration.t.solcontracts/test/foundry/KlerosCore_ForkingCourt.t.solcontracts/test/foundry/SortitionModule_Freeze.t.sol
| constructor(address _owner, KlerosCore _core, address _disputeKit) { | ||
| owner = _owner; | ||
| core = _core; | ||
| pinakion = _core.pinakion(); | ||
| disputeKit = _disputeKit; | ||
| slashDestination = ISlashDestination(address(this)); // Baseline Q-006: absorb into redistribution. | ||
| } |
There was a problem hiding this comment.
Validate _disputeKit in constructor to prevent a bricked settlement contract.
If _disputeKit is address(0), onlyByDK can never pass, so initSettle/slash become permanently unreachable.
Suggested fix
contract ForkSettlement {
@@
error NotImplemented();
error DisputeKitOnly();
+ error InvalidDisputeKit();
@@
constructor(address _owner, KlerosCore _core, address _disputeKit) {
+ require(_disputeKit != address(0), InvalidDisputeKit());
owner = _owner;
core = _core;
pinakion = _core.pinakion();
disputeKit = _disputeKit;
slashDestination = ISlashDestination(address(this)); // Baseline Q-006: absorb into redistribution.
}🤖 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/ForkSettlement.sol` around lines 105 -
111, The constructor in ForkSettlement.sol does not validate that the
_disputeKit parameter is not address(0), which could brick the contract if an
invalid address is passed. Add a require statement in the constructor to
validate that _disputeKit is not address(0), placing this check before the
assignment to the disputeKit state variable. If the validation fails, the
transaction should revert with a descriptive error message indicating that
_disputeKit cannot be the zero address.
| constructor(address _owner, KlerosCore _core) { | ||
| owner = _owner; | ||
| core = _core; | ||
| pinakion = _core.pinakion(); | ||
| } |
There was a problem hiding this comment.
Reject zero owner in constructor.
Deploying with owner = address(0) makes setForkSettlement permanently unreachable and leaves escrow unconfigurable.
Suggested fix
contract PNKHolderEscrow {
@@
error NotImplemented();
error KlerosCoreOnly();
error ForkSettlementOnly();
+ error InvalidOwner();
@@
constructor(address _owner, KlerosCore _core) {
+ require(_owner != address(0), InvalidOwner());
owner = _owner;
core = _core;
pinakion = _core.pinakion();
}🤖 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/PNKHolderEscrow.sol` around lines 69 -
73, The constructor in PNKHolderEscrow does not validate that the _owner
parameter is non-zero, which allows owner to be set to address(0) and makes the
setForkSettlement function permanently unreachable and the escrow
unconfigurable. Add a require statement in the constructor to validate that
_owner is not address(0) before assigning it to the owner state variable.
| function setForkSettlement(address _forkSettlement) external { | ||
| require(msg.sender == owner, NotImplemented()); | ||
| forkSettlement = _forkSettlement; |
There was a problem hiding this comment.
Use explicit auth/config errors in setForkSettlement and disallow zero address.
NotImplemented() here masks auth failures, and accepting address(0) allows avoidable routing misconfiguration for settlement handoff.
Suggested fix
contract PNKHolderEscrow {
@@
error NotImplemented();
error KlerosCoreOnly();
error ForkSettlementOnly();
+ error OwnerOnly();
+ error InvalidForkSettlement();
@@
function setForkSettlement(address _forkSettlement) external {
- require(msg.sender == owner, NotImplemented());
+ require(msg.sender == owner, OwnerOnly());
+ require(_forkSettlement != address(0), InvalidForkSettlement());
forkSettlement = _forkSettlement;
}🤖 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/PNKHolderEscrow.sol` around lines 77 -
79, The setForkSettlement function has two issues: it uses NotImplemented()
error which is inappropriate for an authorization check and should be replaced
with a proper auth-related error, and it accepts address(0) for _forkSettlement
which could cause routing misconfiguration. Add a require statement to validate
that _forkSettlement is not the zero address and revert with an appropriate
error if it is, and change the existing require statement to use an explicit
authorization error instead of NotImplemented().
| function test_createDispute_storesNumberOfChoices() public { | ||
| vm.prank(address(core)); | ||
| forkingDK.createDispute(DISPUTE_ID, 0, 3, "", 0); | ||
| assertTrue(forkingDK.initialized(DISPUTE_ID), "dispute must be marked initialized"); | ||
|
|
||
| vm.expectRevert(DisputeKitForking.ForkAlreadyInitiated.selector); | ||
| vm.prank(address(core)); | ||
| forkingDK.createDispute(DISPUTE_ID, 0, 3, "", 0); | ||
| } |
There was a problem hiding this comment.
test_createDispute_storesNumberOfChoices is missing the key assertion.
Line 27 names this as a number-of-choices storage test, but the body only checks initialization and duplicate-creation revert. Please assert that the stored numberOfChoices for the dispute is 3 (via existing getter or a small test helper).
🤖 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/test/foundry/DisputeKitForking.t.sol` around lines 27 - 35, The
test function `test_createDispute_storesNumberOfChoices` does not actually
assert that the numberOfChoices value (3) is stored correctly, despite its name
suggesting this is the primary test objective. After the first vm.prank and
forkingDK.createDispute call, add an assertion that verifies the stored
numberOfChoices for DISPUTE_ID equals 3 by using an existing getter method on
the forkingDK contract to retrieve the stored numberOfChoices value.
|
|
||
| // Funding the appeal must route the court jump to the forking court (DK is jumping to forking). | ||
| vm.prank(crowdfunder1); | ||
| disputeKit.fundAppeal{value: 0.63 ether}(DISPUTE_ID, 1); |
There was a problem hiding this comment.
Avoid hardcoded appeal funding amounts in integration tests.
Using a fixed 0.63 ether ties test outcomes to mutable fee config and can cause unrelated failures. Compute the required appeal amount from protocol state at runtime, then fund with that value.
Also applies to: 85-85, 97-97
🤖 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/test/foundry/Forking_Integration.t.sol` at line 69, The test is
using hardcoded appeal funding amounts (0.63 ether) in multiple calls to
disputeKit.fundAppeal, which creates brittle tests that fail when fee
configurations change. Instead of hardcoding this value, compute the required
appeal amount at runtime from the current protocol state (likely from the
dispute kit configuration or related fee parameters) and use that computed value
for all fundAppeal calls throughout the test. This ensures the test remains
resilient to changes in protocol fees.



What
Test-first scaffold for the forking mechanism (spec:
contracts/docs/layer-1-core/07-forking.md, onfeat/forking-kit-specs). Foundry suites encode the spec's guarantees / invariants / worked examples, and compile-ready contract skeletons let them run red until the implementation pass turns each suite green.This deliberately ships scaffolding, not finished logic — skeleton bodies revert
NotImplemented()or return inert defaults. The diff is small and the red/green line is clean.TDD state (current)
ForkToken.t.solForkTokenis fully implemented — the TDD sanity anchorDisputeKitForking.t.solIDisputeKitsurface; voting stubs assertNotImplementedForkSettlement.t.solForkMath.t.solSortitionModule_Freeze.t.solKlerosCore_ForkingCourt.t.solForking_Integration.t.solExisting core suites (
KlerosCore_Staking/Initialization/Appeals) still pass — changes to existing contracts are additive only.New contracts (skeletons)
ForkMath— removal fixed point, one threshold-sorted list per losing option (internal-only; exercised viaForkMathHarness).DisputeKitForking— reworkedIDisputeKitat the Core boundary.createDispute's 3rd arg isnumberOfChoicesper the interface (the earlier draft misnamed itfinalRuling).ForkSettlement— paginatedCapture → MainDistrib → Mint → Donephase machine (separate contract per Q-007).ForkToken— fully-working mintable ERC-20, one per minority fork.PNKHolderEscrow— tier-2 participation path.ISlashDestination— isolates the slash-destination decision (Q-006).Additive changes to existing contracts
New surface only;
appeal/execute/appealCost/initializebehavior is untouched (filled in the implementation pass):SortitionModule:stakingFrozenflag;freeze/unfreeze/captureUnstakeAllCourts(+ISortitionModule).KlerosCore:forkSettlementstorage +setForkSettlement;captureStakeForForking/distributeForkinghooks (onlyForkSettlement);ForkingRoundStartedevent;AppealNotAllowed/NotForkingCourterrors.KlerosProxies:DisputeKitForkingProxy.Implementation-pass map (what turns each red suite green)
ForkMathlogic →ForkMath.t.sol.SortitionModulefreeze guard +captureUnstakeAllCourts→SortitionModule_Freeze.t.sol.KlerosCoreforking guards (appeal/appealCost/_getCompatibleNextRoundSettings/execute) + forking-court config ininitialize→KlerosCore_ForkingCourt.t.sol+ the jump inForking_Integration.t.sol.DisputeKitForkingcommit/reveal/finalize +ForkSettlementsettle → integration lifecycle.https://claude.ai/code/session_01W7bRbuy6gnrpKX5VctbC9K
Generated by Claude Code
PR-Codex overview
This PR introduces the implementation of a forking mechanism in the Kleros arbitration system, including new contracts for dispute handling, token minting, and stake management during forking rounds.
Detailed summary
DisputeKitForkingProxyandDisputeTemplateRegistryProxycontracts.ISlashDestinationinterface for handling slashed PNK.ISortitionModulewithfreezeandunfreezefunctions.ForkTokencontract for minting tokens for minority forks.ForkMathlibrary for handling voting mechanics.ForkSettlementfor managing the settlement process.ForkMathHarnessfor testing theForkMathlibrary.SortitionModuleto manage stake freezing during forking.Forking_Integration_TestandSortitionModule_Freeze_Test.Summary by CodeRabbit
New Features
Bug Fixes