Feature/erc20 erc165 #1073 #1629
Conversation
|
Thanks @utgarda! Please give us some time, we'll review it next week. |
|
@ElOpio is it any good? Can I help you somehow? Obviously, I can't add my own code review to this code :P |
|
Hello @utgarda. Sorry it's taking long to review it. Please give the maintainers some time while they catch up with the queue. |
CHANGELOG.md
Outdated
| * `ERC20`: added internal `_approve(address owner, address spender, uint256 value)`, allowing derived contracts to set the allowance of arbitrary accounts. | ||
| * `ERC20Metadata`: added internal `_setTokenURI(string memory tokenURI)`. | ||
|
|
||
| ### Improvements: | ||
| * `ERC20`: `name()`, `symbol()`, `decimals()` made external instead of public |
There was a problem hiding this comment.
We can't really make this change because it breaks backwards compatibility. What was the reason for it?
There was a problem hiding this comment.
@frangio you're right. I followed my reference PR too closely. I'll roll this particular change back.
This reverts commit c416a01
|
I'm not sure how (if) we should move forward with this feature: if someone is interacting with an ERC20 token, it seems obvious that it implements the ERC20 interface. What's more interesting is knowing about additional capabilities, e.g. If a user requires this, they can easily achieve it by inheriting from ERC165 and calling |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
After considering this for a while, I don't think we should move forward with adding ERC165 support to ERC20. We haven't received real requests for this, the original issue #1073 was simply a "it would seem to make sense to do this", and I don't think this justifies the added gas cost in the core ERC20 implementation. Really sorry about the unmerged work @utgarda! |
ERC20 made ERC165-compliant (Fixes #1073)
This PR mimics 259b9da and uses test helpers introduced there in a similar manner.
fix: ERC20Detailed's name(), symbol(), decimals() made external instead of public
feat: add ERC165 to ERC20Burnable
feat: add ERC165 to ERC20Detailed
feat: add ERC165 to ERC20
Added ERC165 implementation to ERC20, as well as Burnable and Detailed flavors,
didn't touch Capped, Mintable etc., because their functionality is mostly accessible to contract owners.