Add Unit Tests for Staker-Calculator Interactions #75
Conversation
test/GovernanceStaker.t.sol
Outdated
| @@ -4164,6 +4166,142 @@ contract BumpEarningPower is GovernanceStakerRewardsTest { | |||
| } | |||
| } | |||
|
|
|||
| contract EarningPowerCalculation is GovernanceStakerRewardsTest { | |||
There was a problem hiding this comment.
Test contract names should match the specific method that is being called so scopelint spec can render things correctly
There was a problem hiding this comment.
So, all of these stake tests should go under that test contract
| contract MockConfigurableEarningPowerCalculator is IEarningPowerCalculator { | ||
| // Multiplier applied to all earning power calculations (in basis points) | ||
| // 10000 = 100% (no change), 5000 = 50%, 20000 = 200% etc | ||
| uint256 public multiplier = 10_000; |
There was a problem hiding this comment.
nit: might be good to name this something like multiplierBips or scaleBips i.e. for Basis Points, which should help make it clearer it's a 10K denominator scale.
mikeghen
force-pushed
the
calculator-tests
branch
3 times, most recently
from
415b390 to
33a0a89
Compare
test/GovernanceStaker.t.sol
Outdated
| @@ -789,6 +795,95 @@ contract Stake is GovernanceStakerTest { | |||
| vm.expectRevert(GovernanceStaker.GovernanceStaker__InvalidAddress.selector); | |||
| govStaker.stake(_amount, _delegatee, address(0)); | |||
| } | |||
|
|
|||
| function testFuzz_StakeWithReducedEarningPower( | |||
There was a problem hiding this comment.
The test names need to be improved to provide a better explanation of what part of the spec is being defined & tested here. See how these read compared to other Stake tests in the output of scopelint spec.
So for this test, I would say something more like, "Sets the appropriate earning power when the calculator scales down the amount staked"
test/GovernanceStaker.t.sol
Outdated
| ) public { | ||
| vm.assume(_depositor != address(0)); | ||
| _stakeAmount = _boundMintAmount(_stakeAmount); | ||
| _multiplierBips = bound(_multiplierBips, 1, 20_000); |
There was a problem hiding this comment.
The test name says "reduced earning power" but the multiplier bound could be > 100%, no? Similarly, we could include 0 here, couldn't we?
test/GovernanceStaker.t.sol
Outdated
| ) public { | ||
| vm.assume(_depositor != address(0)); | ||
| _stakeAmount = _boundMintAmount(_stakeAmount); | ||
| _multiplierBips = bound(_multiplierBips, 1, 20_000); |
There was a problem hiding this comment.
Similarly, the test says "boosted" but the multiplier could be < 100%
|
|
||
| import {IEarningPowerCalculator} from "src/GovernanceStaker.sol"; | ||
|
|
||
| contract MockConfigurableEarningPowerCalculator is IEarningPowerCalculator { |
There was a problem hiding this comment.
I'm not sure we really needed a new mock here. I think we could add the multiplier bips override to the existing mock (and rename it). Curious if you agree @alexkeating
There was a problem hiding this comment.
I opted to add this into the existing mock in favor of having less code overall.
test/GovernanceStaker.t.sol
Outdated
| @@ -15,12 +15,15 @@ import {ERC20VotesMock, ERC20Permit} from "test/mocks/MockERC20Votes.sol"; | |||
| import {IERC20Errors} from "openzeppelin/interfaces/draft-IERC6093.sol"; | |||
There was a problem hiding this comment.
arbitrary comment location
I think there a lot more tests we could write for this, including ones that show the earning power is updated when the user takes various other actions, such as stakeMore, withdraw, alter*, claimReward, etc....
We don't necessarily have to do them in this PR, but let's make sure we don't close out the issue if we don't, and add some notes to the issue of additional tests we should handle.
There was a problem hiding this comment.
@apbendi, I went and added simple tests to the contracts that test those methods. I figured I could just do it so we could close this issue out. Lmk if there were other contracts to be added for the etc., it wasn't obvious to me where else to add these scaled earning calculator tests to.
mikeghen
force-pushed
the
calculator-tests
branch
2 times, most recently
from
eacf025 to
93473a2
Compare
This commit introduces a mock calculator with adjustable earning power to expand test coverage, validating staker responses to varied earning scenarios such as scaled and boosted staking amounts.
mikeghen
force-pushed
the
calculator-tests
branch
from
93473a2 to
24f9563
Compare
|
Coverage after merging calculator-tests into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Resolves: #27