-
Notifications
You must be signed in to change notification settings - Fork 196
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
test: vaults fuzzing #918
base: feat/vaults
Are you sure you want to change the base?
test: vaults fuzzing #918
Conversation
Hardhat Unit Tests Coverage Summary
Diff against master
Results for commit: e9a302d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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.
Great start!
Some suggestions and comments left.
Also, looks like it need to me merged with vaults branch once again, because some tests are failing.
1d41029
to
7d05ee2
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.
No major issues, at least for now, need to validate boundaries and maybe add more invariants.
* forge-config: default.invariant.depth = 256 | ||
* forge-config: default.invariant.fail-on-revert = true | ||
*/ | ||
function invariant_totalShares() public view { |
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.
💡 Maybe add an invariant that lido.getExternalShares = startExternalBalance + mintedExternal - burnedExternal? So we'll know it something is odd inside a math for external shares?
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.
added like TODO
// if (_report.timestamp >= block.timestamp) revert IncorrectReportTimestamp(_report.timestamp, block.timestamp); | ||
vm.warp(_timestamp + 1); | ||
|
||
fuzz._lidoExecutionLayerRewardVaultWei = bound(fuzz._lidoExecutionLayerRewardVaultWei, 0, 1_000) * 1 ether; |
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.
Providing comments on why the bounding appears as it does would be helpful during reviews and test updates.
} | ||
|
||
contract AccountingHandler is CommonBase, StdCheats, StdUtils { | ||
struct Ghost { |
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.
💡 Maybe add a boundary holder to track minimum and maximum values used in all the runs, so we can review them after the test and ensure we're not missing any potential values chunks?
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.
Added in e9a302d. Shows the max <> min range.. Like this:
Boundary Values:
PreClValidators min: 250000
PreClValidators max: 100000000000
ClValidators min: 259381
ClValidators max: 100000043199
ClBalanceWei min: 8300705805470639196490834
ClBalanceWei max: 3200271341074368560000000000000
DepositedValidators min: 293200
DepositedValidators max: 100000086397
ElRewardsVaultBalanceWei min: 0
ElRewardsVaultBalanceWei max: 977277467356288757093
…zzing-share-rate # Conflicts: # package.json # test/0.4.24/contracts/StakingRouter__MockForLidoAccounting.sol
Test shareRate with fuzzing