Skip to content
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 checkpointing in missing methods #88

Merged
merged 4 commits into from
Dec 13, 2024
Merged

Conversation

alexkeating
Copy link
Collaborator

@alexkeating alexkeating commented Dec 12, 2024

Description

  • Checkpoint global reward and deposit when altering a delegatee and claimer

Closes #83

@alexkeating alexkeating marked this pull request as ready for review December 13, 2024 14:10
@@ -2764,6 +2764,58 @@ contract NotifyRewardAmount is GovernanceStakerRewardsTest {
assertLe(govStaker.scaledRewardRate(), _expectedRewardRate);
}

function test_shareManipulationIfRewardsArentCeckpointed() public {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function test_shareManipulationIfRewardsArentCeckpointed() public {
function test_shareManipulationIfRewardsArentCheckpointed() public {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we rename this test to describe that there's no longer share manipulation? also, are there unit tests to write here (e.g. alterDelegatee checkpoints global reward, alterDelegatee checkpoints reward)? defer to you on that


address _fox = makeAddr("fox");

//!audit-ok Step 1, fox deposits with full earning power
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might revise these comments, or possibly attribute CergyK above the test

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@alexkeating alexkeating force-pushed the fix/earning-power-inflation branch from b63e68c to e3592d4 Compare December 13, 2024 19:55
Copy link

Coverage after merging fix/earning-power-inflation into main will be

98.08%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   BinaryEligibilityOracleEarningPowerCalculator.sol100%100%100%100%
   DelegationSurrogate.sol100%100%100%100%
   DelegationSurrogateVotes.sol100%100%100%100%
   GovernanceStaker.sol98.50%100%97.22%98.60%256–258
src/extensions
   GovernanceStakerDelegateSurrogateVotes.sol87.50%50%100%90.91%25–26
   GovernanceStakerOnBehalf.sol100%100%100%100%
   GovernanceStakerPermitAndStake.sol76.47%0%100%76.92%19–20, 48, 76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkpoint deposit and global reward in alterClaimer and alterDelegatee
2 participants