-
Notifications
You must be signed in to change notification settings - Fork 39
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
GrantsRound contract with tests #17
Conversation
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 great overall!
Left a lot of comments, but mostly best practices type of things. There's also been a lot of discussion in #12 so we should review that and make sure any changes/new requirements are accounted for here
The biggest change I mentioned is that I think we should switch from a Waffle mock to an actual mock token contract that behaves like an ERC20, since it seems there's a few issues/bugs that arise/aren't caught with the Waffle mock approach.
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! Left some additional feedback and responses to others comments. LMK if you have any follow up. Thanks @corydickson.
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.
Thanks for addressing the feedback @corydickson. Added a couple commits to address very minor issues, including minor formatting changes, and making the failing test skipped instead so it doesn't result in failing CI once #25 is merged.
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 to merge.
* Add needed '/' to storage var natspec comments * Add newlines between storage var declarations * Add header comments between different code sections
e56ba8a
to
1763f3d
Compare
Addresses #12