-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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 ERC20 and ERC777 fixed supply presets #2377 #2399
Add ERC20 and ERC777 fixed supply presets #2377 #2399
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.
Thanks @ashwinYardi!
…C777PresetFixedSupply constructors
c76503e
to
8e40673
Compare
Hi all! |
@frangio @Skyge Some info about the testcases included: Since the base contracts used in above presets, already have a full fledged test cases to cover the entire behavior, Only bare minimum test cases which help in establishing the high level behavior of preset contracts have been added. Again, I referred to test cases written for already added presets and it made sense to proceed this way. Thanks and please let me know if there is anything missing or needs to be corrected, would be happy to do it immediately so that we can get this meged! ( PS. Sorry for not mentioning this info about testcases on PR's description ). |
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.
@ashwinYardi Sorry for taking so long to come back to this!
Thank you for explaining the rationale for what to test. I didn't remember the other presets followed the same approach to testing.
I'd like to see a few more changes in the tests before we merge this:
- Remove the
context
anddescribe
"wrappers", since they don't serve any purpose here. - Remove tests that we can assume correct from the base ERC777 contract:
- returns a granularity of 1
- returns 18 when decimals is called
- ERC20: add a test for the total supply
I also think the documentation needs a little bit of work. But this is something we can do if you prefer. If you'd like to take a stab at it you can take a look at the docs comments for the existing presets and follow a similar format. The docs for the constructor are missing too.
In order for the presets to show in the site, you need to add their names in contracts/presets/README.adoc
.
Hi @frangio ! Thanks a lot for the feedback.
|
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 work @ashwinYardi! I made a few small changes to the docs, added a changelog entry, and I realized I prefer to keep the defaultOperators
argument in the same place as the ERC777
constructor so I changed that as well.
Open issue: #2377
Additions #