Skip to content
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

Remove default parameters in built-in interfaces #3296

Open
pcaversaccio opened this issue Feb 20, 2023 · 6 comments
Open

Remove default parameters in built-in interfaces #3296

pcaversaccio opened this issue Feb 20, 2023 · 6 comments

Comments

@pcaversaccio
Copy link
Collaborator

pcaversaccio commented Feb 20, 2023

Currently, some built-in interfaces (see e.g. ERC4626) use default parameters (can be used to autogenerate overloaded function selectors) but is not required by the interface definitions itself, and other interfaces (see e.g. ERC721) don't define default parameters, for which it could make sense however (see e.g. safeTransferFrom). I would like to propose removing any default parameters from the built-in interfaces due to the following reasons:

  • consistency
  • it's not very well documented how overloading functions in Vyper work and thus very few people understand how to import built-in interfaces and use one of the overload versions

Let me know what you think. In any case, we need to have consistent behavior.

@pcaversaccio pcaversaccio changed the title Remove default parameters in interfaces Remove default parameters in built-in interfaces Feb 20, 2023
@fubuloubu
Copy link
Member

One of the problems is that Vyper needs overloading in order to work with overloaded functions defined in ERC specs, like 721

I would argue a shift to using definitions defined in ABI JSON files (or importing via a single package manifest containing all of them) would be beneficial to better support this since it natively supports overloading in a better way

@pcaversaccio
Copy link
Collaborator Author

I'm all in for anything that supports natively overloading.

@pcaversaccio
Copy link
Collaborator Author

One of the problems is that Vyper needs overloading in order to work with overloaded functions defined in ERC specs, like 721

Sorry, I don't get that point - could you explain why in the ERC721 we don't use that approach?

@fubuloubu
Copy link
Member

One of the problems is that Vyper needs overloading in order to work with overloaded functions defined in ERC specs, like 721

Sorry, I don't get that point - could you explain why in the ERC721 we don't use that approach?

In vyper, you can't have multiple functions with the same name

@pcaversaccio
Copy link
Collaborator Author

pcaversaccio commented Feb 20, 2023

In vyper, you can't have multiple functions with the same name

Yeah that one I know - but I just realised how you meant it in the context of ERC721. But actually, something that I observed is that the ERC721 interface doesn't work actually as intended in combination with implements. If you do something like the following (without data: Bytes[1024]=b""), the compiler will not throw even though the overloaded function of safeTransferFrom (without data payload) is not implemented and therefore it doesn't implement the ERC721 interface fully:

from vyper.interfaces import ERC721
implements: ERC721

@external
@payable
def safeTransferFrom(owner: address, to: address, token_id: uint256, data: Bytes[1024]):
    ...

@trocher
Copy link
Contributor

trocher commented Feb 20, 2023

In vyper, you can't have multiple functions with the same name

Yeah that one I know - but I just realised how you meant it in the context of ERC721. But actually, something that I observed is that the ERC721 interface doesn't work actually as intended in combination with implements. If you do something like the following (without data: Bytes[1024]=b""), the compiler will not throw even though the overloaded function of safeTransferFrom (without data payload) is not implemented and therefore it doesn't implement the ERC721 interface fully:

from vyper.interfaces import ERC721
implements: ERC721

@external
@payable
def safeTransferFrom(owner: address, to: address, token_id: uint256, data: Bytes[1024]):
    ...

I believe the implements not intended behavior you describe should be documented here #3170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants