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

Add an interface folder that lists common interfaces #2517

Merged
merged 29 commits into from
Aug 6, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 8, 2021

Includes:

  • ERC20: ERC-20 Token Standard (final)
  • ERC165: ERC-165 Standard Interface Detection (final)
  • ERC721: ERC-721 Non-Fungible Token Standard (final) → includes IERC721Enumerable and IERC721Metadata
  • ERC777: ERC777 Token Standard (final)
  • ERC1155: ERC-1155 Multi Token Standard (final) → includes IERC1155MetadataURI, IERC1155Receiver
  • ERC1271: Standard Signature Validation Method for Contracts (last call)
  • ERC1363: ERC-1363 Payable Token (final) → should be implemented as an ERC20 extension
  • ERC1820: Pseudo-introspection Registry Contract (final)
  • ERC2612: permit – 712-signed approvals (draft) → base of ERC20Permit
  • ERC2981: NFT Royalty Standard (final)
  • ERC3156: Flash Loans (final)

Eventually, all of https://eips.ethereum.org/erc should be considered

PR Checklist

  • Documentation
  • Changelog entry

@frangio frangio added the idea label Feb 9, 2021
@frangio
Copy link
Contributor

frangio commented Feb 9, 2021

Since this is not a breaking change we will discuss this after 4.0 or at least after we've finished work on the breaking changes.

@Amxx Amxx force-pushed the feature/interfaces branch 2 times, most recently from 3430cf2 to 9a225ab Compare February 22, 2021 23:21
@Amxx Amxx force-pushed the feature/interfaces branch from 9a225ab to 1dc0141 Compare March 12, 2021 08:44
@Amxx Amxx marked this pull request as ready for review March 12, 2021 08:58
@frangio
Copy link
Contributor

frangio commented Apr 19, 2021

What should we do with the interface we have elsewhere? Should we deduplicate them?

@Amxx
Copy link
Collaborator Author

Amxx commented Apr 20, 2021

What should we do with the interface we have elsewhere? Should we deduplicate them?

We just cannot move them for obvious backward compatibility reasons. Yet, I think they should be in here. So my approach has been to link them using import statements. The other option I see, is to move the interface file here, and have the links where they originally were ... but my guess is you won't like it.

Any preference/opinion ?

@frangio
Copy link
Contributor

frangio commented Apr 20, 2021

Oh sorry I missed that there were import statements to the existing interfaces.

Well what are we going to do for future ERCs we implement? Are we going to add two different files for each new ERC interface? It seems like we should instead add all new interfaces in the interfaces directory, and import them from there.

@Amxx
Copy link
Collaborator Author

Amxx commented Apr 20, 2021

It seems like we should instead add all new interfaces in the interfaces directory

Yes. This is what I did for ERC1271 in signaturechecker

@frangio
Copy link
Contributor

frangio commented Apr 22, 2021

So given that any future interfaces are going to be added in the interfaces directory, I would move the current interfaces over there for consistency, and place imports in the current locations for backwards compatibility.

@frangio frangio removed the idea label Apr 23, 2021
@frangio
Copy link
Contributor

frangio commented May 19, 2021

@Amxx Please see my comment above.

@Amxx Amxx requested a review from frangio August 5, 2021 14:02
@Amxx Amxx mentioned this pull request Aug 5, 2021
2 tasks
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@frangio frangio merged commit 1866887 into OpenZeppelin:master Aug 6, 2021
@Amxx Amxx deleted the feature/interfaces branch August 6, 2021 13:44
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.

2 participants