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

fix[ux]: IERC20Detailed inherits from IERC20 #4349

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AlbertoCentonze
Copy link
Contributor

@AlbertoCentonze AlbertoCentonze commented Nov 5, 2024

What I did

ERC20Detailed should inherit from ERC20.

How I did it

Given Vyper does not support inheritance I just added implements: ERC20 and copy pasted the methods from ERC20.vyi

How to verify it

Make a mock contract that implements the builtin

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@AlbertoCentonze AlbertoCentonze changed the title chore[builtin]: merge ERC20 and ERC20Detailed fix[builtin]: merge ERC20 and ERC20Detailed Nov 5, 2024
@AlbertoCentonze AlbertoCentonze changed the title fix[builtin]: merge ERC20 and ERC20Detailed fix[ux]: merge ERC20 and ERC20Detailed Nov 5, 2024
@charles-cooper charles-cooper changed the title fix[ux]: merge ERC20 and ERC20Detailed fix[ux]: expand IERC20Detailed Nov 5, 2024
@charles-cooper
Copy link
Member

this looks good. can you add/update tests as appropriate?

@AlbertoCentonze AlbertoCentonze marked this pull request as ready for review November 5, 2024 18:44
@AlbertoCentonze AlbertoCentonze changed the title fix[ux]: expand IERC20Detailed fix[ux]: IERC20Detailed inherits from ERC20 Nov 5, 2024
@AlbertoCentonze AlbertoCentonze changed the title fix[ux]: IERC20Detailed inherits from ERC20 fix[ux]: IERC20Detailed inherits from IERC20 Nov 5, 2024
@charles-cooper
Copy link
Member

@pcaversaccio thoughts here?

i rather like the idea of renaming IERC20Detailed to IERC20, and changing the existing IERC20 to something like IERC20Basic

@pcaversaccio
Copy link
Collaborator

@pcaversaccio thoughts here?

i rather like the idea of renaming IERC20Detailed to IERC20, and changing the existing IERC20 to something like IERC20Basic

No - I think IERC20 should be the base one (i.e. without metadata) and we can rename IERC20Detailed to IERC20Metadata instead. This would be also in line with e.g. OpenZeppelin here or snekmate interfaces (I also use Metadata for naming) so we can prevent further confusion like this.

I would like to note, that if we do this we might also want to do it for ERC721 as well. I provide my own interface here already but could remove it in that case :)

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

Successfully merging this pull request may close these issues.

3 participants