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

Adding eip-2535 #2795

Merged
merged 8 commits into from
Jul 18, 2020
Merged

Adding eip-2535 #2795

merged 8 commits into from
Jul 18, 2020

Conversation

mudgen
Copy link
Contributor

@mudgen mudgen commented Jul 17, 2020

EIP-2535 is ready to be added.

@mudgen
Copy link
Contributor Author

mudgen commented Jul 17, 2020

@MicahZoltu Can you please merge this pull request?

EIPS/eip-2535.md Outdated
type: Standards Track
category: ERC
created: 2020-02-22
replaces: 1538
Copy link
Member

Choose a reason for hiding this comment

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

One can only replace/supersede a final EIP. I suggest to:

  • remove this line
  • mention this somewhere in the motivation or another appropriate section
  • since 1538 is also authored by you, independently of this PR mark it as "Abandoned"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@axic Makes sense and good suggestions.

I removed the "replaces" line and mentioned the replacement in the text of the EIP. I updated the pull request with the changes.

I'll go to EIP 1538 and mark it as abandoned and replaced with eip 2535.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this pull request EIP-1538 is now marked as abandoned and replaced by EIP-2535.

status: Draft
status: Abandoned
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just delete this EIP instead? This repository has a huge volume of DRAFT EIPs that are not adding value by sitting idle. If nick@mokens.io agrees, I would prefer to delete rather than mark as abandoned. We have git history, so its not like the work will be lost forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No because EIP-1538 is in use by contracts in production and is being linked to by various places on the Internet. In addition the final EIP-1155 suggests using EIP-1538 for upgrading contracts.

Copy link
Contributor Author

@mudgen mudgen Jul 18, 2020

Choose a reason for hiding this comment

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

EIP-1538 is part of the development and history of EIP-2535.

Copy link
Contributor Author

@mudgen mudgen Jul 18, 2020

Choose a reason for hiding this comment

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

Editor Feedback:

  • address the "who is Nick Mudge" question I had (different email addresses)
  • make section headings match template section headings

Personal Feedback:

  • I'm not a fan of the themed naming convention. I think standards should name things in a way that makes them immediately understandable to the widest audience possible while conveying what they do in a pithy manner. Following a naming theme in the contract like loupe, facets and cut is cute but not useful for understanding.

I made the fixes. Thanks very much for your feedback and help with this.

You are right about the terminology and it has been debated. Terminology has been in use for 6 months and we are stuck with it now.

EIPS/eip-2535.md Outdated
Comment on lines 20 to 24
New terminology from the diamond industry.

Improved design over [ERC1538](https://eips.ethereum.org/EIPS/eip-1538) using ABIEncoderV2 and function selectors.

Replaces ERC1538.
Copy link
Contributor

Choose a reason for hiding this comment

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

Imagine you are reading this standard 10 years in the future. These lines would not add value or improve your understanding of the standard, so IMO they should not be included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point and you are right. I'll move the mentions of ERC1538 to the bottom of the EIP under inspiration & development.

EIPS/eip-1538.md Outdated
@@ -3,12 +3,13 @@ eip: 1538
title: Transparent Contract Standard
author: Nick Mudge <nick@mokens.io>
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these EIPs are authored by Nick Mudge, but the email address is different. Did you just change email addresses? If so, can you send a quick email to me (micah at zoltu dot net) from nick@mokens.io letting me know that this is you or can one of the other editors verify?

I generally prefer GitHub accounts as authors for this reason, though I can appreciate that email is more censorship resistant so I generally support people using email as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same person. I will send you an email. What is your email address?

I will change nick@mokens.io to nick@perfectabstractions.com, so the email address is consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the email address.

EIPS/eip-2535.md Outdated

The diamond address is the address that users interact with. The diamond address does not change. Only facet addresses can change by using the `diamondCut` function.

## Diamond Example
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't follow the EIP template section headings. Recommend putting this as a sub-heading in one of the template sections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps move this to the implementations section, or link to it from the implementations section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I moved the example under the Implementation section.

EIPS/eip-2535.md Outdated
Comment on lines 449 to 455
* Andrew Redden
* Patrick Gallagher
* Leo Alt
* Santiago Palladino
* William Entriken
* Gonçalo Sá
* Brian Burns
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't add value to a standard. If you want these people to have an ownership role in EIP editing/maintenance, then add them to the Authors line. If not, then recommend removing this section. In 10 years when someone is looking at this standard, they aren't going to care who helped edit it. If they want, they can look at the GitHub history and/or discussion-to history for supplementary information like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but I don't feel it is right to take it out now. I want to keep this as-is.

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Editor Feedback:

  • address the "who is Nick Mudge" question I had (different email addresses)
  • make section headings match template section headings

Personal Feedback:

  • I'm not a fan of the themed naming convention. I think standards should name things in a way that makes them immediately understandable to the widest audience possible while conveying what they do in a pithy manner. Following a naming theme in the contract like loupe, facets and cut is cute but not useful for understanding.

EIPS/eip-2535.md Outdated
* Leo Alt
* Santiago Palladino
* William Entriken
* Gonçalo Sá
Copy link
Contributor

Choose a reason for hiding this comment

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

The CI spellchecker is complaining about the spelling of the word alo here, which suggests to me that it isn't unicode friendly. I have pinged some people more knowledgeable than I asking how to bypass that particular check, but I don't know how long until they get back to me. If you don't want to wait, other options are to use ASCII characters for the name to work around the issue or remove this section entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. I removed the section. Ready for merge.

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • Trying to change EIP 1538 state from Draft to Abandoned
  • Contains new file EIPS/eip-2535.md
  • File assets/eip-2535/diamond.svg is not an EIP
  • EIP 1538 requires approval from one of (nick@mokens.io)

@MicahZoltu MicahZoltu merged commit 13f2286 into ethereum:master Jul 18, 2020
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
A diamond is a set of contracts that can access the same storage variables and share the same Ethereum address.

A contract architecture that makes upgradeable contracts flexible, unlimited in size, and transparent.
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
A diamond is a set of contracts that can access the same storage variables and share the same Ethereum address.

A contract architecture that makes upgradeable contracts flexible, unlimited in size, and transparent.
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.

4 participants