-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add incentivized earning power update #30
Conversation
9572a04
to
ee1f4d5
Compare
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.
Hey @alexkeating, some mostly minor comments, but I did hit one failing test
test/GovernanceStaker.t.sol
Outdated
govStaker.bumpEarningPower(_depositId, _tipReceiver, _requestedTip); | ||
} | ||
|
||
function testFuzz_RevertIf_NewEarningPowerIsGreaterThanTheCurrentEarningPower( |
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.
The name of this test makes it sound like the method should revert if the earning power is going up. It should have a name that's something like "Revert If: Unclaimed Rewards Are Less Than The Requested Tip When Earning Power Is Being Bumped Up"
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.
test/GovernanceStaker.t.sol
Outdated
govStaker.bumpEarningPower(_depositId, _tipReceiver, _requestedTip); | ||
} | ||
|
||
function testFuzz_RevertIf_NewEarningPowerIsLessThanTheCurrentEarningPower( |
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.
Similar to the prior test, this one needs a better name. "Revert If: Unclaimed Rewards Are Insufficient To Leave An Additional Bump Tip After Current Tip Is Paid When Earning Power Is Being Bumped Down"
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.
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.
Small spelling error, it says "Maxc" instead of "Max"
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.
Yes, forgot to link the fix here 77e7a86
Coverage after merging add-incentivized-earning-power-update into main will be
Coverage Report
|
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.
One small spelling error but otherwise this is good to merge.
Description