-
Notifications
You must be signed in to change notification settings - Fork 191
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
feat: nomismatokopio #8940
feat: nomismatokopio #8940
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @LHerskind and the rest of your teammates on Graphite |
d9bb3d2
to
5f14124
Compare
5f14124
to
525fefe
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.
Big +1 on this test layout. Huge improvements.
nom.mint(address(0xdead), 1); | ||
} | ||
|
||
modifier givenCallerIsOwner() { |
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, comment would be nice that this should be empty because the base contract test deployed it.
// it emits a {Transfer} event | ||
// it will return 0 for mintAvailable in same block | ||
uint256 amount = bound(_amount, 1, maxMint); | ||
assertGt(amount, 0); |
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 this assert needed?
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
525fefe
to
5d53711
Compare
Fixes #8135.
.solhint
as well do remove thegas-custom-errors
rule and turn offcustom-error-over-require
.The solhint version used in the docker is not the same as if using
yarn lint
so need to handle that in a separate PR as there is a bunch of warnings and errors.