-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Enable supportsInterface(...)
from ERC165 on ERC20 contract implementation
#3575
Comments
supportsInterface(...)
from ERC165 on ERC20 contract implementation
Hello @CJ42 As you point out:
ERC721's interfaceId is widely known, and expected in all ERC721 implementation. You can therefore rely on it to check it an unknown contract is ERC721 ERC20's interfaceId is not widelly accepted, ERC20 is not required to implement ERC165, and (almost) no instance of ERC20 include it. This means that you either:
Going back in time, it would have been nice to have ERC165 before ERC20 (among a LOT of other things we would change about ERC20 is we were designing it today). However we have to work with the existing ecosystem, and given that ERC20 doesn't include 165, it makes very little sens to suddenly add it. Note that if you want to add it, it should be very easy for you. We are just not going to make that choice for the user. |
@Amxx This would be a good start to make it a "best practice" to include supportsInterface into ERC20 if OZ does it. I honestly would also include erc223 too |
UPDATE 08/27/2023: ERC-223 now exists and is in Last Call status. I would approve of OZ adding an ERC-223 implementation, although developers should not be using it in production until ERC-223 reaches Final, which is scheduled to happen on 09/03. |
@Pandapip1 thanks for the heads-up! so far none of those are implemented, right? and it would be really nice to have one. |
Yea, I would like to see an EIP-1363 implementation, but a new issue should be created for that. |
@Amxx going back on this topic, I do still think it would make sense to add ERC165 by default in ERC20, so to advocate this as a best practice. The most popular standard currently in effect are ERC20, ERC721 and ERC115, and the last two (721 & 1155) implement interface detection. It sounds non-standard and not conventional to not implement it on one of the three most used. Especially as ERC20 is cited in the motivation section of the ERC165 standard itself. Althought "supportsInterface" can always be faked (a dummy/malicious function implementation that can return "true" for certain interface IDs), it makes it more explicit. Otherwise it feels it's more "assumed". I would be interested to hear @frangio opinion on this too. |
there is no downside of adding erc165 to erc20 however this may be superseded with upcoming merge of erc1363 |
btw @Amxx do you have some kind of discord where I can contact you directly? |
I would like to notify everyone that ERC-20 standard violates common secure software design principles (source 1 and source 2) that state:
What @Pandapip1 calls "safe to use" resulted in a loss of $201,690,000 worth of tokens. Here is a description thread: https://ethresear.ch/t/security-concerns-regarding-token-standards-and-130m-worth-of-erc20-tokens-loss-on-ethereum-mainnet/16387 Here is my old article on subject: https://dexaran820.medium.com/known-problems-of-erc20-token-standard-e98887b9532c#:~:text=ERC%2D20%20token%20standard%20contains,during%20the%20past%206%20years. Here is another article that describes how As a security auditor I would not call something that resulted in a loss of $201M "safe to use". |
Edit: I did say that. The statement in question is now retracted. Thanks for the heads-up, and I'm sorry for using such harsh language. |
|
As you can see ERC-1363 inherits all the problems of ERC-20 that resulted in a loss of $200M. |
Fair enough. I will edit that comment, as you are right, it is misleading. Consider that original statement retracted. It's also horribly out-of-date. Added an update section and crossed out the outdated stuff. |
I would add that an ERC being Final shouldn't be interpreted as it being safe, in general for any ERC. Closing this issue for the reasons explained in #3575 (comment). |
🧐 Motivation
In the current token implementations of OpenZeppelin contracts, only the ERC721 implementation contains the
supportsInterface(...)
function.ERC20 does not contain
supportsInterface(...)
by default.I am not sure why this is the case, but my first guess would be the order that each of these ERC20 came up?
📝 Details
Would it make sense to add
supportsInterface(...)
in ERC20, and make the contract inherit fromERC165
as well? So that by default, both fungible and non-fungible tokens implementation from OZ library can be detected easily via this function?Let me know your thoughts @frangio @Amxx and I would be happy to make a brief PR if this feature request is accepted.
The text was updated successfully, but these errors were encountered: