-
Notifications
You must be signed in to change notification settings - Fork 51
fix(SortitionModule): total stake update #2199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ import {DisputeKitClassicBase} from "../../src/arbitration/dispute-kits/DisputeK | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {IArbitratorV2, IArbitrableV2} from "../../src/arbitration/KlerosCore.sol"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {IERC20} from "../../src/libraries/SafeERC20.sol"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import "../../src/libraries/Constants.sol"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {console} from "forge-std/console.sol"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// @title KlerosCore_ExecutionTest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// @dev Tests for KlerosCore execution, rewards, and ruling finalization | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -749,6 +750,52 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(address(disputeKit).balance, 0, "Wrong balance of the DK"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function test_inflatedTotalStaked_whenDelayedStakeExecute_whenJurorHasNoFunds() public { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // pre conditions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 1. there is a dispute in drawing phase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 2. juror call setStake with an amount greater than his PNK balance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 3. draw jurors, move to voting phase and execute voting | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 4. move sortition to staking phase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 disputeID = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 amountToStake = 20000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _stakePnk_createDispute_moveToDrawingPhase(disputeID, staker1, amountToStake); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KlerosCore.Round memory round = core.getRoundInfo(disputeID, 0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 pnkAtStakePerJuror = round.pnkAtStakePerJuror; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _stakeBalanceForJuror(staker1, type(uint256).max); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _drawJurors_advancePeriodToVoting(disputeID); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _vote_execute(disputeID, staker1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sortitionModule.passPhase(); // set it to staking phase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _assertJurorBalance( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disputeID, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| staker1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| amountToStake, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| amountToStake, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("totalStaked before: %e", sortitionModule.totalStaked()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // execution: execute delayed stake | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sortitionModule.executeDelayedStakes(1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // post condition: inflated totalStaked | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("totalStaked after: %e", sortitionModule.totalStaked()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _assertJurorBalance( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disputeID, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| staker1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| amountToStake, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| amountToStake, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // new juror tries to stake but totalStaked already reached type(uint256).max | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // it reverts with "arithmetic underflow or overflow (0x11)" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _stakeBalanceForJuror(staker2, 20000); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function testFuzz_executeIterations(uint256 iterations) public { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 disputeID = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 roundID = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -847,4 +894,61 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(totalLocked, (pnkAtStake * nbJurors) - unlockedTokens, "Wrong amount locked"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(stakedInCourt, 2000, "Wrong amount staked in court"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ///////// Internal ////////// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function _assertJurorBalance( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 disputeID, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| address juror, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 totalStakedPnk, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 totalLocked, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 stakedInCourt, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 nbCourts | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) internal { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (uint256 totalStakedPnk, uint256 totalLocked, uint256 stakedInCourt, uint256 nbCourts) = sortitionModule | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .getJurorBalance(juror, GENERAL_COURT); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(totalStakedPnk, totalStakedPnk, "Wrong totalStakedPnk"); // jurors total staked a.k.a juror.stakedPnk | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(totalLocked, totalLocked, "Wrong totalLocked"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(stakedInCourt, stakedInCourt, "Wrong stakedInCourt"); // juror staked in court a.k.a _stakeOf | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(nbCourts, nbCourts, "Wrong nbCourts"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+900
to
+914
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Variable shadowing makes all assertions useless. The tuple destructuring on lines 908-909 redeclares variables with the same names as the function parameters, causing variable shadowing. This means all assertions on lines 910-913 compare values against themselves and will always pass, regardless of the actual vs. expected values. The expected values passed as parameters are never used. Apply this diff to fix the variable shadowing: function _assertJurorBalance(
uint256 disputeID,
address juror,
- uint256 totalStakedPnk,
- uint256 totalLocked,
- uint256 stakedInCourt,
- uint256 nbCourts
+ uint256 expectedTotalStakedPnk,
+ uint256 expectedTotalLocked,
+ uint256 expectedStakedInCourt,
+ uint256 expectedNbCourts
) internal {
(uint256 totalStakedPnk, uint256 totalLocked, uint256 stakedInCourt, uint256 nbCourts) = sortitionModule
.getJurorBalance(juror, GENERAL_COURT);
- assertEq(totalStakedPnk, totalStakedPnk, "Wrong totalStakedPnk"); // jurors total staked a.k.a juror.stakedPnk
- assertEq(totalLocked, totalLocked, "Wrong totalLocked");
- assertEq(stakedInCourt, stakedInCourt, "Wrong stakedInCourt"); // juror staked in court a.k.a _stakeOf
- assertEq(nbCourts, nbCourts, "Wrong nbCourts");
+ assertEq(totalStakedPnk, expectedTotalStakedPnk, "Wrong totalStakedPnk");
+ assertEq(totalLocked, expectedTotalLocked, "Wrong totalLocked");
+ assertEq(stakedInCourt, expectedStakedInCourt, "Wrong stakedInCourt");
+ assertEq(nbCourts, expectedNbCourts, "Wrong nbCourts");
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function _stakeBalanceForJuror(address juror, uint256 amount) internal { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("actual juror PNK balance before staking: %e", pinakion.balanceOf(juror)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.prank(juror); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| core.setStake(GENERAL_COURT, amount); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function _stakePnk_createDispute_moveToDrawingPhase(uint256 disputeID, address juror, uint256 amount) internal { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.prank(juror); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| core.setStake(GENERAL_COURT, amount); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.prank(disputer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.warp(block.timestamp + minStakingTime); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sortitionModule.passPhase(); // Generating | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.warp(block.timestamp + rngLookahead); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sortitionModule.passPhase(); // Drawing phase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertEq(sortitionModule.totalStaked(), amount, "!totalStaked"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function _drawJurors_advancePeriodToVoting(uint256 disputeID) internal { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| core.draw(disputeID, DEFAULT_NB_OF_JURORS); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.warp(block.timestamp + timesPerPeriod[0]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| core.passPeriod(disputeID); // Vote | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function _vote_execute(uint256 disputeID, address juror) internal { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256[] memory voteIDs = new uint256[](3); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| voteIDs[0] = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| voteIDs[1] = 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| voteIDs[2] = 2; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.prank(juror); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| core.passPeriod(disputeID); // Appeal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vm.warp(block.timestamp + timesPerPeriod[3]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| core.passPeriod(disputeID); // Execution | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,6 +141,32 @@ contract KlerosCore_StakingTest is KlerosCore_TestBase { | |
| ); | ||
| } | ||
|
|
||
| function test_setStake_totalStaked() public { | ||
| // Increase | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 4000); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 5001); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1000); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1500); | ||
|
|
||
| assertEq(sortitionModule.totalStaked(), 6501, "Wrong totalStaked"); | ||
|
|
||
| // Decrease | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 3000); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 2500); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1400); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1200); | ||
|
|
||
| assertEq(sortitionModule.totalStaked(), 3700, "Wrong totalStaked"); | ||
| } | ||
|
Comment on lines
+144
to
+168
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# First, search for totalStaked references in the test file
rg -n "totalStaked" contracts/test/foundry/KlerosCore_Staking.t.sol -B2 -A2Repository: kleros/kleros-v2 Length of output: 7231 🏁 Script executed: #!/bin/bash
# Search for tests that combine delayed stakes with totalStaked checks
rg -n "delayedStake.*totalStaked|totalStaked.*delayedStake" contracts/test/foundry/ -iRepository: kleros/kleros-v2 Length of output: 42 🏁 Script executed: #!/bin/bash
# Search for all test functions that mention delayed stake
rg -n "function test.*[Dd]elayed" contracts/test/foundry/KlerosCore_Staking.t.solRepository: kleros/kleros-v2 Length of output: 182 🏁 Script executed: #!/bin/bash
# Check the entire context around delayed stake tests to see if totalStaked is tested
rg -n "test_delayedStake" contracts/test/foundry/KlerosCore_Staking.t.sol -A 50 | head -100Repository: kleros/kleros-v2 Length of output: 42 🏁 Script executed: #!/bin/bash
# Read the test_executeDelayedStakes function
sed -n '324,450p' contracts/test/foundry/KlerosCore_Staking.t.solRepository: kleros/kleros-v2 Length of output: 6960 🏁 Script executed: #!/bin/bash
# Find the SortitionModule contract file
fd -t f "SortitionModule\.sol" contracts/Repository: kleros/kleros-v2 Length of output: 163 🏁 Script executed: #!/bin/bash
# Search for the totalStaked function/variable definition
rg -n "totalStaked" contracts/src/ -B2 -A5 | head -100Repository: kleros/kleros-v2 Length of output: 7695 🏁 Script executed: #!/bin/bash
# Search for executeDelayedStakes implementation
rg -n "function executeDelayedStakes" contracts/src/ -A 30Repository: kleros/kleros-v2 Length of output: 9796 🏁 Script executed: #!/bin/bash
# Search for setStakeBySortitionModule in KlerosCore
rg -n "function setStakeBySortitionModule" contracts/src/ -A 15Repository: kleros/kleros-v2 Length of output: 3466 🏁 Script executed: #!/bin/bash
# Confirm _setStake calls the totalStaked update
rg -n "function _setStake" contracts/src/arbitration/KlerosCore.sol -A 40 | head -60Repository: kleros/kleros-v2 Length of output: 2059 Test coverage for delayed stakes affecting The verification confirms the review comment's concern is valid. While the current
A test case should verify that executing delayed stakes properly increments the global 🤖 Prompt for AI Agents |
||
|
|
||
| function test_setStake_maxStakePathCheck() public { | ||
| uint256[] memory supportedDK = new uint256[](1); | ||
| supportedDK[0] = DISPUTE_KIT_CLASSIC; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete test: Missing assertions or revert expectation.
The test comment on line 795 states that staker2's stake attempt "reverts with arithmetic underflow or overflow," but line 796 calls
_stakeBalanceForJuror(staker2, 20000)without a precedingvm.expectRevert()call and without any subsequent assertions. This leaves the test's behavior and purpose unclear.Additionally, the test relies on the
_assertJurorBalancehelper which currently has a critical bug (see separate comment), so the existing assertions at lines 769-776 and 785-792 are not functioning correctly.Please clarify the test's intent and add appropriate assertions:
If the stake should revert:
console.log("totalStaked after: %e", sortitionModule.totalStaked()); _assertJurorBalance( disputeID, staker1, amountToStake, pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, amountToStake, 1 ); // new juror tries to stake but totalStaked already reached type(uint256).max - // it reverts with "arithmetic underflow or overflow (0x11)" + vm.expectRevert(); // or specify the exact error _stakeBalanceForJuror(staker2, 20000);If the stake should succeed:
📝 Committable suggestion