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

Update EIP-721: Update link #6012

Closed
wants to merge 2 commits into from
Closed

Conversation

fulldecent
Copy link
Contributor

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@fulldecent fulldecent requested a review from eth-bot as a code owner November 20, 2022 14:41
@github-actions github-actions bot added c-update Modifies an existing proposal s-final This EIP is Final t-erc labels Nov 20, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 20, 2022

All tests passed; auto-merging...

(pass) eip-721.md

classification
updateEIP
  • passed!

@Pandapip1 Pandapip1 changed the title Update link Update EIP-721: Update link Nov 20, 2022
@fulldecent
Copy link
Contributor Author

Sound good @xinbenlv I'm a fan of permalinks. Just updated it at 8508642 to use the version available at time of publication.

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

+0 to updating the link, +1 if it's removed altogether.

@eth-bot eth-bot enabled auto-merge (squash) November 21, 2022 01:20
@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 21, 2022

+0 to updating the link, +1 if it's removed altogether.

Respectfully, @Pandapip1 , I am afraid I can't agree with your suggestion of removing it

IMHO, such links were part of the technical argument demonstrating the adoption landscape back then and made the proposal argument more convincing. I think they play an important role in making ERC721 an model EIP.

@Pandapip1
Copy link
Member

Pandapip1 commented Nov 21, 2022

EIP-721 is not a model EIP, as:

  • Policies have changed since then
  • The template has changed significantly

I agree that the content of EIP-721 is excellent. As such, it would indeed make sense to update EIP-20, EIP-721, and EIP-1155 to reflect current EIP practices. However, current EIP practices include "not including external links to unapproved domains." Respectfully, arbitrary GitHub links and OpenZeppelin links are not approved domains, nor should they be.

@fulldecent
Copy link
Contributor Author

Comments here are discussing two things:

  1. Whether EIPs, especially new ones, should link to GitHub and other domains
  2. Updating ERC-721

I have updated many EIPs with and without the consent of the original authors. There are limits to the changes that we have pushed through in these situations. Because we are not planning to rewrite all existing published EIPs that means that there is a at least some difference on what is publishable for existing versus new EIPs.


I do not have any comments right now about whether linking to GitHub should be acceptable or not.

But I recognize one complaint is that most links to GitHub are rugpullable—a page at some URL can be changed and replaced with some entirely different content. There may be other complaints as well.

This PR changes an existing GitHub link on a named branch to link on a specific hash which cannot be modified. This corrects an error I originally made while publishing (sorry!), always use permalinks.

Whereas this PR addresses one material concern about an existing link to GitHub and it does not introduce any new link to GitHub, I request that this be merged as-is. Also the debate about whether linking to GitHub is good or bad should be closed without prejudice. Such broad discussion can be much better held elsewhere and this PR does not have any net impact on linking to GitHub.

@Pandapip1 Pandapip1 added this to the Manual Merge Queue milestone Nov 21, 2022
@Pandapip1
Copy link
Member

Pandapip1 commented Nov 21, 2022

I agree with @fulldescent that PRs that fix broken links are good. That's why I've approved this PR, and have added it to the manual merge queue.

I also think that PRs that remove links are also good. But that's, as you have rightly said, a separate discussion.

@SamWilsn
Copy link
Contributor

We've discussed this pull request on EIPIP today. Since this link is not part of the actual specification (aka non-normative), and it breaking doesn't affect the understanding of this EIP, we are going to close this pull request for the time being.

That said, @gcolvin would like to define a more formal process for correcting errata, and we should revisit this change at that time.

@SamWilsn SamWilsn closed this Dec 14, 2022
auto-merge was automatically disabled December 14, 2022 15:31

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal s-final This EIP is Final t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants