search

ethcatherders / EIPIP

EIP Improvement Process
77 stars 38 forks source link

Call for Input: Remove External Implementations from ERC-20 #287

Closed SamWilsn closed 7 months ago

SamWilsn commented 9 months ago

Call for Input

Decision Do we merge https://github.com/ethereum/EIPs/pull/7886?
If Affirmed https://github.com/ethereum/EIPs/pull/7886 is merged, removing links to external implementations.
If Rejected ERC-20 is not changed.
Method Rough Consensus
Deadline November 19, 2023

Background

See https://github.com/ethereum/EIPs/pull/7886.

SamWilsn commented 9 months ago

Responding to the points @dexaran raised on their pull request:

The links are outdated significantly and the code is written in Solidity 0.4.x versions. [...]

The standard was written against solidity 0.4, and is specified in terms of that version. If solidity released a version 1.0 with a different calling convention, we wouldn't update ERC-20 to use that calling convention because that would invalidate all existing ERC-20 tokens.

By that logic, I don't believe that the age of the reference implementation is a good argument for removing it.

The content in the links is licensed under MIT while the EIP process requires that the content would be licensed under CC0.

EIP-1 only requires that the EIP is CC0, and doesn't really specify whether assets are part of the EIP or are separate works. I usually operate under the policy that assets can be any license that doesn't impose too much on implementers. We have a discussion thread open to come up with an official policy.

All that to say that if we wanted to move an MIT-licensed reference implementation into the EIPs repository, I wouldn't object.

Further, since we do allow linking to non-CC0 resources (eg. IETF RFCs, WHATWG specs, etc.), I don't believe the license is a sufficient argument for removing these links.

The developers of the initial reference implementation benefit from this commercially.

This is, at least somewhat, convincing. We should maintain credible neutrality as much as possible, and having a link to a particular external implementation directly in an EIP somewhat violates that principle, especially since we cannot update EIPs as new implementations are created.

That said, I think we need to balance this credible neutrality against our promise of immutability. ERC-20 is Final, and shouldn't be changed. Our policy on external links didn't exist when ERC-20 was written. Obviously if it were written today, ERC-20 wouldn't have those links.

I am going to have to oppose this decision, though I'm open to being convinced otherwise.

abcoathup commented 9 months ago

I vote no (but still don't have a vote).

Final ERCs should be final.

Other ERCs have links/references to specific implementations e.g. https://eips.ethereum.org/EIPS/eip-721#implementations If the approach is to remove implementations (or links to implementations) for the reasons given then this should be done for every ERC which has them. Alternatively the implementations section could link to a wiki where modern, supported implementations can be listed and maintained.

If this PR is merged then the same should be done for every ERC rather than singling one out.

SamWilsn commented 9 months ago

this should be done for every ERC which has them

I believe that's the intent, and this call for input is the first of many (if it passes.)

abcoathup commented 9 months ago

I believe that's the intent, and this call for input is the first of many (if it passes.)

My understanding is that this was just proposed for ERC-20 which Dexaran has a very public gripe about.

SamWilsn commented 9 months ago

@dexaran had suggested:

Open a PR to remove old links from old EIPs and there will be a discussion among EIP editors.

To which I replied:

One PR per EIP, and maybe just start with one so we can gauge how the Editors feel about it. I can't imagine it'll go smoothly...

Dexaran commented 9 months ago

@SamWilsn

The standard was written against solidity 0.4, and is specified in terms of that version.

"The standard" is just a set of abstract rules described in text. Any particular implementation is not standard. So I see no reason to mislead potential implementers by providing them with an insanely outdated code. The EIP repo is supposed to help people to build useful things. And the presence of this links doesn't help in any sensible way and is potentially misleading from my point of view.

I would also argue that solidity version bumps must be allowed even for "final" EIPs but this is not the topic of this PR.

having a link to a particular external implementation directly in an EIP somewhat violates that principle, especially since we cannot update EIPs as new implementations are created.

You also don't allow new EIPs to have links.

It looks like a serious endorsement (which again goes directly against EIP-5069) for any person who is not deeply involved in the EIP process. Other users certainly interpret it as an endorsement.

EIP-decide-winners

Dexaran commented 9 months ago

@abcoathup

My understanding is that this was just proposed for ERC-20 which Dexaran has a very public gripe about.

I do have a lot of concerns about EIP-20, most importantly the fact that it caused a loss of $200M for the end users. As the result I work quite often with the proposal text so I spotted a detail that must be improved in my opinion.

Pandapip1 commented 9 months ago

I vote +1, for a variety of reasons:

  1. The implementations change regularly. This is not consistent with EIPs remaining Final.
  2. EIPs are CC0-licensed. I believe that users should have the reasonble expectation that anything that an EIP references is also CC0-licensed. The implementations are not.
  3. The implementations are non-trivial and designed to be primarily production-ready. This is not consistent with the aims of the Reference Implementation section.
  4. The OZ implementation can be found with a simple search for "ERC-20" or "EIP-20." Keeping the link serves only to solidify OZ as 'the only place you should be getting your contracts from.'
  5. The Reference Implementation section is non-normative. Removing the links is absolutely fine.

FWIW I do agree that all external reference implementations should be removed or inlined.

I dismiss the following counterarguments:

  1. 'Final means no more changes' - In Final, normative changes should be disallowed, and non-normative changes that result in a different interpretation are discouraged. Admittedly, the links to these implementations does result in a different interpretation - that those implementations are NOT somehow canonical.
  2. '@Dexaran has a conflict of interest' - Everyone knows he's the author of ERC-223, a competing standard. The governance system we've made makes decisions on the objective evidence provided and the subjective consensus of the editor group. @Dexaran is not a part of the editor group, so his bias has limited impact here. We shouldn't consider that he is biased against ERC-20, only what he has to say.

Note: @Dexaran's gripe with ERC-20 is very much justified. Nobody should be arguing that the problem doesn't exist. If money is being lost, then there's a problem and the responsibility lies somewhere. I would argue ERC-223 is not the best way to solve the problem, because the responsibility lies with wallets not providing a good UX when submitting transactions, not with the token contracts themselves.

g11tech commented 9 months ago

+1 , the implementation references and details can happen in discussions-to link, don't need to be part of the standard

SamWilsn commented 9 months ago
  1. The implementations change regularly. This is not consistent with EIPs remaining Final.

Removing the links is also not consistent with EIPs remaining final. I'd argue that removing the links is more disruptive than keeping them. Both links are qualified with a commit hash.

  1. EIPs are CC0-licensed. I believe that users should have the reasonble expectation that anything that an EIP references is also CC0-licensed. The implementations are not.

Both the linked OpenZeppelin and Consensys implementations are MIT. As everyone should be well aware by now (:wink:), I fully support MIT licensed reference implementations. We can take this particular argument over here.

Hell, we could move both of those implementations into the ERCs repository, and be done with this.

  1. The implementations are non-trivial and designed to be primarily production-ready. This is not consistent with the aims of the Reference Implementation section.

The OZ implementation clocks in at 205 lines (plus 65 for SafeMath) and the Consensys implementation is 72 lines. These are both much smaller than other reference implementations. Yes, they probably have some unnecessary bells and whistles, but they aren't exactly huge bits of code.

  1. The OZ implementation can be found with a simple search for "ERC-20" or "EIP-20." Keeping the link serves only to solidify OZ as 'the only place you should be getting your contracts from.'

If we don't just move the implementations into the assets directory (which right now is my preferred solution), I'd be okay adding a rel="nofollow" to those links to adjust their SEO.

  1. The Reference Implementation section is non-normative. Removing the links is absolutely fine.

The whole concept of "normative" is mentioned twice in EIP-1: here and here. The former resets the last call period, and the latter explicitly states:

A Final EIP [...] should only be updated to correct errata and add non-normative clarifications.

Those links were not against the rules at the time, which is a justification we've used recently, and so are not an errata. I'd hardly call removing example code "add[ing] non-normative clarifications".

Pandapip1 commented 9 months ago

Those links were not against the rules at the time, which is a justification we've used recently, and so are not an errata. I'd hardly call removing example code "add[ing] non-normative clarifications".

This is your most convincing argument IMO. However, if I get permission from the authors, would that change your mind?

SamWilsn commented 9 months ago

This is your most convincing argument IMO.

:smile_cat:

However, if I get permission from the authors, would that change your mind?

Should authors have extra authority over anyone else once an EIP goes to final? I don't really think so.

If we are going to do something with these links, would everyone be happy with moving these files into the assets/... directory and linking from there? Would remove the appearance of favouring OZ/ConsenSys, without changing the content of the EIPs at all.

Pandapip1 commented 9 months ago

Should authors have extra authority over anyone else once an EIP goes to final? I don't really think so.

The precedent was that if the editors and the people responsible for an action disagree with whether the action was 'wrong' then EIP-1 takes precedence.

If the editors and the people responsible for an action agree the action should be reverted, and the community seems to be in favor of removing the links... why wouldn't we?

SamWilsn commented 8 months ago

From @Pandapip1:

[moving in the reference implementations] will be vastly superior to what we have today

SamWilsn commented 8 months ago

Here's my attempt at a solution that satisfies everyone: https://github.com/ethereum/ERCs/pull/112

The reference implementations are moved into the ERCs repository, under the MIT license.

g11tech commented 8 months ago

i think once something becomes "commodity" reference implementations can be dropped, imo (although not a strongly held one) reference implementations should/could exist in discussions-to or we can have reference implementations or spec links section

If reference implementations have uncovered vulnerabilities, a final EIP should be allowed to mark them deprecated and refer to a fixed one. If the EIP itself is flawed, it should be marked deprecated/superceeded by

SamWilsn commented 8 months ago

@g11tech:

i think once something becomes "commodity" reference implementations can be dropped, imo (although not a strongly held one) reference implementations should/could exist in discussions-to or we can have reference implementations or spec links section

Having a permissively licensed reference implementation that the proposal itself can refer to is pretty nice, IMO. For example, when the specification section describes an algorithm in English, having the code to refer to is useful as a developer (eg. EIP-2315). What benefit do we get by removing them?

If reference implementations have uncovered vulnerabilities, a final EIP should be allowed to mark them deprecated and refer to a fixed one. If the EIP itself is flawed, it should be marked deprecated/superceeded by

I am not against a supersedes: 1234 header field.

SamWilsn commented 8 months ago

Would everyone mind restating their opinion on this? I fear I've made it more confusing by adding my pull request.

The options (as I see them) are:

I think we should move the ERC-20 reference implementations into the ERCs repository (see https://github.com/ethereum/ERCs/pull/112.)

abcoathup commented 8 months ago

As an associate I don't have a vote.

I would rather option 4: that implementations and security sections for every ERC link to a wiki page on Eth Magicians, so they can be maintained by the community.

If this PR is merged then the same should be done for every ERC rather than singling one out.

SamWilsn commented 7 months ago

@xinbenlv:

Fine that they are cited and wants to give authors more freedom. ERC-20 is widely adopted, and as a historical document doesn't follow our latest formats, so we need a much bigger justification for changing the document. I would like to leave it as is.

@gcolvin:

Generally in favour of moving into our repository, and fixing the broken links in ERC-20.

@lightclient

I have a slight preference for removing the links, but am overall indifferent.

SamWilsn commented 7 months ago

So to summarize my Keeper ruling, we have:

This is a difficult one to judge, because there is no clear prevailing opinion.

Deciding between doing something and doing nothing is a pretty easy call, however. Only @xinbenlv was of the opinion that we should leave the links unchanged. The rough consensus is pretty clear: do something with the links.

I am not choosing "remove the links" because only @g11tech has expressed clear support in removing the links, while I am extremely opposed to changing the meaning of a Final proposal. If @lightclient was more strongly in favour of removing the links, I would've gone with this option.

So that leaves moving the reference implementations into our repository.