vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.84k stars 789 forks source link

VIP: ERC165 support #2619

Open fubuloubu opened 2 years ago

fubuloubu commented 2 years ago

Simple Summary

ERC165 allows contracts to quickly determine if they are able to interact with a particular contract. Deeper support into Vyper will help improve contracts that would like to implement ERC165

Motivation

ERC721 introduced the concept of mandatory ERC165 interface checks in order to acheive compliance with a given ERC. This has been problematic for Vyper in the past because of the missing bytes4 type (which is also something we should add and support).

Until support is added for bytes4, it will not be possible to comply with ERC165 (and thus any ERCs which require ERC165 for full compliance). However, it seems like a potential solution to overload the implements: keyword, which currently like Vyper contracts specify what interfaces they comply to, with functionality that adds ERC165 compliance more implicitly to a contract

Specification

When the implements: keyword is used in a Vyper contract with an Interface type, Vyper will add the def supportsInterface(interface_id: bytes4) -> bool function directly to the contract's bytecode, which returns whether or not the contract supports a given interface_id.

ERC165 specifies interface_id as the logical XOR of all the mandatory method_ids located in the defined interface. Computing these manually requires adding lines to the constructor function in order to enable it correctly, or otherwise making large view function for supportsInterface with lots of manual typing.

A discussion point is whether these interface IDs only be limited to defined ERCs in Vyper's interfaces library, which would reduce clutter from using non-standardized interfaces with the implements: keyword

Lasty, any contracts which use this feature will also implicitly use the implements: ERC165 specifier. A discussion could be had that this support would only enabled by adding this specification to a contract (e.g. a contract will not produce the implementsInterface function unless implements: ERC165 is added), which would make this behavior more "opt in"

Backwards Compatibility

Any contracts that use the implements: keyword currently would raise an error if the supportsInterface function was also defined.

Dependencies

Not required for implementation, but the bytes4 type would make implementation of this VIP likely easier.

References

Copyright

Copyright and related rights waived via CC0

tserg commented 2 years ago

I think injecting the def supportsInterface(interface_id: bytes4) -> bool function based on the implements: keyword can be confusing given Vyper's goal of simplicity. I have in mind someone starting out in Vyper implementing a simple ERC20, deploying it to a testnet and then trying to verify the Vyper code on Etherscan but it fails because the compiled code includes this function but the local copy of the written contract does not. Furthermore, it also seems like a creeping in of class inheritance (of the ERC165 interface).

IMO, I think it is better to require the def supportsInterface(interface_id: bytes4) -> bool to be expressly included in the contract, but have a built-in attribute for interfaces that provides the interface ID (e.g. Solidity has an interfaceId for interfaces).

fubuloubu commented 2 years ago

I think injecting the def supportsInterface(interface_id: bytes4) -> bool function based on the implements: keyword can be confusing given Vyper's goal of simplicity. I have in mind someone starting out in Vyper implementing a simple ERC20, deploying it to a testnet and then trying to verify the Vyper code on Etherscan but it fails because the compiled code includes this function but the local copy of the written contract does not. Furthermore, it also seems like a creeping in of class inheritance (of the ERC165 interface).

IMO, I think it is better to require the def supportsInterface(interface_id: bytes4) -> bool to be expressly included in the contract, but have a built-in attribute for interfaces that provides the interface ID (e.g. Solidity has an interfaceId for interfaces).

Yes, got a lot of feedback from @charles-cooper about this too. I think adding the MyInterface.interface_id property to an interface object would be a better way to go. Then you can just do something like this:

@view
@external
def supportsInterface(interface_id: bytes4) -> bool:
    return interface_id in [
        ERC20.interface_id,
        ERC721.interface_id,
        ...  # as many as you want to support
    ]

This has the most optimal balance of flexibility and convenience

fubuloubu commented 2 years ago

Wonder if this should also add .method_id to method types e.g.:

@external
def marked(amount: uint256):
    ...

# self.marked.method_id == 0x92e37a93
charles-cooper commented 2 years ago

Wonder if this should also add .method_id to method types e.g.:

@external
def marked(amount: uint256):
    ...

# self.marked.method_id == 0x92e37a93

https://github.com/vyperlang/vyper/issues/2098

charles-cooper commented 2 years ago

meeting notes: implements: is too implicit, we will implement interface_id member (or maybe call it erc165_id)

liam-ot commented 2 years ago

Hello,

First thanks for the work being done on vyper

using vyper v0.3.3 brownie v1.19.0

I'm wondering what the state of bytes4 is at the moment in relation to its use in implementing the ERC165 interface?

I'd also like to know is if it is possible to use the standard erc-721 contract provided by vyper as it is provided? and if not is there a workaround/timeline for when it will be a usable contract?

I am using the standard vyper contract here and the reference to a bytes4 type on line 290 (assert returnValue == method_id("onERC721Received(address,address,uint256,bytes)", output_type=bytes4)) is causing this error on compile

Brownie v1.19.0 - Python development framework for Ethereum

Compiling contracts...
  Vyper version: 0.2.16
CompilerError: vyper returned the following errors:

No builtin or user-defined type named 'bytes4'
  contract "contracts/nft.vy", function "safeTransferFrom", line 290:103 
       289         # Throws if transfer destination is a contract which does not implement 'onERC721Received'
  ---> 290         assert returnValue == method_id("onERC721Received(address,address,uint256,bytes)", output_type=bytes4)
  ----------------------------------------------------------------------------------------------------------------^
       291

attempting to make the value a Bytes[4] or a bytes32 causes this:

vyper.exceptions.VyperException: Compilation failed with the following errors:vyper.exceptions.VyperException: Compilation failed with the following errors:

TypeMismatch: Cannot perform equality between dislike types
contract "nft/contracts/nft.vy", function "safeTransferFrom", line 290:15
289 # Throws if transfer destination is a contract which does not implement 'onERC721Received'
---> 290 assert returnValue == method_id("onERC721Received(address,address,uint256,bytes)", output_type=bytes32)
------------------------^
291

UndeclaredDefinition: 'uint2str' has not been declared.
contract "nft/contracts/nft.vy", function "tokenURL", line 378:32
377 def tokenURL(tokenId: uint256) -> String[132]:
---> 378 return concat(self.baseURL, uint2str(tokenId))
-----------------------------------------^

which seems wholly unrelated

charles-cooper commented 2 years ago

@liam-ot this seems to be a heisenbug wrt which version of vyper is being used, cf. https://github.com/eth-brownie/brownie/issues/1566

liam-ot commented 2 years ago

@charles-cooper Thanks for the response. So basically there is no way to fix it at this moment? I tried changing to another older version of Vyper with no luck. Is there any workaround for using the bytes4 type as of now?

charles-cooper commented 2 years ago

@liam-ot I'd love to help you debug this but I think this issue is not an appropriate place since it is the spec for a feature. Could we move this conversation to another issue, the discussions board, or discord?

liam-ot commented 2 years ago

yes @charles-cooper I will open a new issue, thanks a lot for being so helpful.