-
Notifications
You must be signed in to change notification settings - Fork 16
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
Invariant test suite #35
Conversation
20797c5
to
7a6f026
Compare
c077aa4
to
8db1191
Compare
53e0558
to
33cedd0
Compare
@@ -0,0 +1,68 @@ | |||
# Invariant Suite |
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.
Is the detail here appropriate for public consumption, or do we want to cull it?
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 info is good, it may be clearer if the unimplemented actions are removed
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.
chatted a bit with @apbendi, going to keep them around for now
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.
Looks good, a bunch of nits
test/UniStaker.invariants.t.sol
Outdated
address rewardsNotifier; | ||
|
||
function setUp() public { | ||
// deploy UniStaker |
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.
Comment should probably be removed or moved to line 28
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.
removed
mapping(address => bool) saved; | ||
} | ||
|
||
library LibAddressSet { |
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.
Was this taken from the article? It may be good to add a comment and link to where this came from.
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 a comment, thanks
test/helpers/UniStaker.handler.sol
Outdated
address public admin; | ||
|
||
// actors, deposit state | ||
address internal currentActor; |
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.
Nit: should this have an underscore?
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.
good call, changed!
test/helpers/UniStaker.handler.sol
Outdated
ghost_rewardsNotified += _amount; | ||
} | ||
|
||
// TODO: distinguish between valid and invalid stake |
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.
Are this and the TODO below still todos?
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 -- invalid stake is not yet included in harness, nor is invalid withdraw
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.
Ah okay it may be good to move these to issues then
foundry.toml
Outdated
fail_on_revert = false | ||
include_push_bytes = true | ||
include_storage = true | ||
runs = 100 |
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.
It may be worth bumping some of these for CI runs.
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.
i'm happy to do that. what values would be good?
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 bump them back to the default values for runs and depth
ccc3fa3
to
7fe1451
Compare
Co-authored-by: Alexander Keating <[email protected]>
cf66d54
to
c6e46f4
Compare
Coverage after merging 01-25-Start_Invariant_Testing into main will be
Coverage Report
|
Resolves #11
This suite should not be considered complete. It's a solid base from which to expand, and tests the invariants listed in #11 as well as others.