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 interface into the repository #903

Closed
wants to merge 4 commits into from

Conversation

fulldecent
Copy link
Contributor

I propose that ERCs that have an interface which is well documented should be merged into the repository.

This will be the canonical home for that file.

And clients can import the file using: http://remix.readthedocs.io/en/latest/tutorial_import.html

@nicksavers nicksavers added the ERC label Feb 25, 2018
@Arachnid
Copy link
Contributor

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

  • Frontmatter is now contained between lines with only a triple dash ('---')
  • Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

@Arachnid
Copy link
Contributor

Arachnid commented Apr 3, 2018

I personally don't think this is the place for it - I can't see anyone importing the EIPs repository for its list of interfaces, and interfaces can already be specified in the EIP text itself.

@fulldecent
Copy link
Contributor Author

Use case:

Currently you must copy/paste your interfaces from ERCs. Or in the case of ERC-20 you must think carefully about what an interface is and then write it. It is very likely that your interface for ERC-20 will be different than somebody else's. This is terrible for interoperability. For example, the ERC-721 interface published to OpenZeppelin is incorrect (it is missing ERC-165 support), and you would only know that if you read the entire standard.

Currently this is best practice:

pragma solidity ^0.4.20;

interface ERC165 {
    /// @notice Query if a contract implements an interface
    /// @param interfaceID The interface identifier, as specified in ERC-165
    /// @dev Interface identification is specified in ERC-165. This function
    ///  uses less than 30,000 gas.
    /// @return `true` if the contract implements `interfaceID` and
    ///  `interfaceID` is not 0xffffffff, `false` otherwise
    function supportsInterface(bytes4 interfaceID) external view returns (bool);
}

contract myContract is ERC165 {
    function supportsInterface(bytes4 interfaceID) external view returns (bool) {
        return interfaceID == 0x01ffc9a7;
    }
}

In the future, with this pull request accepted, the best practice would be updated to:

pragma solidity ^0.4.20;

import "https:/ethereum/EIPs/eip-165/erc-165.sol";

contract myContract is ERC165 {
    function supportsInterface(bytes4 interfaceID) external view returns (bool) {
        return interfaceID == 0x01ffc9a7;
    }
}

The new best practice uses a single source of truth for interfaces and promotes interoperability.

@Arachnid
Copy link
Contributor

Arachnid commented Apr 3, 2018

Finalised EIPs should never change, and well written EIPs should include a separate section with the interface in a cut-and-pasteable form. I think we can solve both of these issues without including them as separate files.

It's also worth noting that Solidity is not the only language that can compile to the EVM; specifying that everything must have a Solidity interface isn't a great approach in my mind.

@fulldecent
Copy link
Contributor Author

ERC-721 will change. This is noted in the text of 721, and it will be updated as bugs are fixed in Solidity. (And those changes will be backwards compatible.)

Currently there is no single source of truth. "Go read the standard and copy paste the correct code from inside" is a terrible way to validate standards compliance. Versus, go copy this interface file from OpenZeppelin. Guess which one of those people are actually doing today? Guess which one produces the wrong results.

Yes, Solidity is not the only language of EVM. If you do have a Solidity interface file ready then the EIP repository is a great canonical place to store that file.

@MicahZoltu
Copy link
Contributor

I agree with @Arachnid that EIPs repository is for standardization proposals, not a place for linking your code to. That being said, there may be value in having some other repository that contains a bunch of interfaces. I think OpenZeppelin is kind of trying to do this, and I don't see a problem with a third party maintaining such a repository.

@fulldecent
Copy link
Contributor Author

fulldecent commented Apr 4, 2018

My comment applies only to EIPs which are ERCs. For some reason, ERCs are in this repository, perhaps ERC-20 et. al. should have been put elsewhere, but that is besides the point.

If an ERC is a standardized interface, and if the interface can be defined by an interface file then that interface file should be stored right along with the text-based standard as an addendum.

OpenZeppelin demonstrates the problem when the interface.sol is housed separately from the standard. OZ has their own 721 which is different than the standard ERC-721. Many people are getting confused. This is a problem.

@MicahZoltu
Copy link
Contributor

There is an EIP floating around somewhere that recommends breaking the ERCs out into a separate location from EIPs. That feels like a good first step, before restructuring how ERCs are stored within this repository.

Most ERCs contain an interface as an embedded code block within the ERC. What is the advantage of storing the interface file separately from the ERC?

Re: OpenZeppelin issue, this feels like a trivial fix, just fix OpenZeppelin interface file. Is there something that makes that an untenable solution in this case and requires a broader solution? It seems like it is just as likely for a .sol file in the EIPs repository to have a bug as it is for the OpenZeppelin repository to have a bug.

Bugs happen, they are a fact of life, I want to make sure we aren't curve fitting for one instance of a bug that can be easily fixed without a change in process.

@fulldecent
Copy link
Contributor Author

Most ERCs do NOT have an interface or they have an interface that does not compile or has other serious problems.

When writing ERC-721, I was the first author to actually use an interface. Following are the issues I reported in the Solidity language related to interfaces directly as a result of writing 721:

ethereum/solidity#3379
ethereum/solidity#3391
ethereum/solidity#3392
ethereum/solidity#3393
ethereum/solidity#3411
ethereum/solidity#3412
https:/ethereum/remix/issues/665
ethereum/ethereum-org#784
ethereum/solidity#3418
ethereum/solidity#3419
ethereum/wiki#529
ethereum/solidity#3420
ethereum/solidity#3421
ethereum/solidity#3428
ethereum/solidity#3429
ethereum/solidity#3430
https:/ethereum/browser-solidity/issues/1012
ethereum/solidity#3431

The Solidity language usage for interfaces has changed significantly now. And now they are actually usable. So let's use them!


OZ makes their own interpretations of ERC standards. The OZ interfaces DO NOT MATCH standards. They have their own ERC-20-esque recommendation. They have an incomplete and proprietary version of ERC-721.

HERE IS THE PROBLEM. When MetaMask (a sponsor of the last ERC-721 event) releases their product will they copy and paste the correct standard from my lovingly tested and peer reviewed solidity code pasted inside my standard, or will they copy the file from OZ which is wrong? If OZ can change any standard they want and they are the only place that hosts interface files then why even bother discussing ERCs here?

@MicahZoltu
Copy link
Contributor

What you are describing is a problem with people not following standards. This is a problem all throughout software engineering and I'm dubious as to whether or not including a .sol file (separate from the ERC) will change that significantly. If people want to follow ERC-721, they can very easily copy the interface right out of the body of the ERC. Having a separate file won't likely make using ERC-721 any easier as users will still likely just copy ERC-721 out of GitHub.

OpenZeppelin I believe provides some tooling support for integrating their stuff with Gnache, a tool that many developers use. Having .sol files alone isn't likely to sway people away from OZ, instead tooling integration would be necessary.

Also, keep in mind that standards are really just suggestions. Ultimately what it comes down to is what people actually use. There are many standards throughout history that have failed to gain traction despite being superior due to marketing, tooling, sales, etc. Unfortunately just writing a great standard isn't enough to get usage because human behavior is not based on technical purity.

@fulldecent
Copy link
Contributor Author

It is not possible to "very easily copy" the interface from ERC-20 into your project. Please try. You will get compiler errors and warnings. Or worse, you and I both performing this exercise will get different answers. I can easily produce two conflicting interfaces which are both compatible with the ERC-20 specification.

This is what easy looks like:

import "https:/ethereum/EIPs/eip-721/erc-721.sol";

and it produces guaranteed consistent results.

If you want to use OZ, that's great. Here's how you do it:

import "https:/ethereum/EIPs/eip-721/erc-721.sol";
import "https:/OpenZeppelin/zeppelin-solidity/...";

@MicahZoltu
Copy link
Contributor

If copying the EIP contents into a .sol file in your project results in errors, then it feels like we should fix the contents of the EIP. Moving the current interface into a .sol file won't fix whatever bugs are present in the EIP.

I didn't know you could import a URL with Solidity, that is neat. It feels reckless (mutable), but useful for prototyping none the less.

@fulldecent
Copy link
Contributor Author

Since this discussion, the scope of the EIP process has changed from a place that evaluates whether EIPs should be accepted to a place that only checks EIPs for technical consistency.

Since this discussion has started, the situation has not improved. I continue to see that everybody is continuing to copy-paste code they found on OpenZeppelin /which is usually wrong/ and they believe it is authoritative. This harms everybody.

This EIP repository is an academic publishing platform where authors can publish technical specifications. It will be a big improvement if people publishing here can target human readers that are learning about standards as well as computer readers that want to verify an implementation is compliant.

@fulldecent
Copy link
Contributor Author

Now that the contents of this repository are being published to https://eips.ethereum.org I think this is PR is even more valuable.

@fulldecent
Copy link
Contributor Author

This is superseded by #2636

It allows ERC authors a specific place to put interfaces. And it allows a script to pick out official interfaces from ERCs. All without allowing new files inside the EIPs repository.

@fulldecent fulldecent closed this May 11, 2020
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.

6 participants