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-725 to adhere to the last definition #3110

Merged
merged 9 commits into from
Nov 27, 2020

Conversation

frozeman
Copy link
Contributor

@frozeman frozeman commented Nov 9, 2020

This PR adjust 725 to the latest discussion version found here: #725

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.

This feels like a totally new EIP, in which case I would recommend a new EIP number. That being said, I am kind of annoyed by people treating draft EIPs as final so I'm not actually opposed to this since maybe this kind of drastic rewrite will help discourage that behavior.

The frontmatter stuff does need to be fixed prior to merge though.

EIPS/eip-725.md Outdated
title: Proxy Account
author: Fabian Vogelsteller (@frozeman), Tyler Yasaka (@tyleryasaka)
title: ERC-725 Smart Contract Based Account
author: Fabian Vogelsteller <fabian@lukso.network>, Tyler Yasaka (@tyleryasaka)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are the primary author, I recommend leaving your GitHub username in here as the bot struggles with email addresses sometimes. If you want both you can do:

Suggested change
author: Fabian Vogelsteller <fabian@lukso.network>, Tyler Yasaka (@tyleryasaka)
author: Fabian Vogelsteller (@frozeman), Fabian Vogelsteller <fabian@lukso.network>, Tyler Yasaka (@tyleryasaka)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

EIPS/eip-725.md Outdated
discussions-to: https://github.com/ethereum/EIPs/issues/725
status: Draft
type: Standards Track
category: ERC
requires: ERC165, ERC173, ERC1271 (optional)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
requires: ERC165, ERC173, ERC1271 (optional)
requires: 165, 173, 1271

Frontmatter is very strict on formatting.

Choose a reason for hiding this comment

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

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 think having the ERC is important as there will be other standards proposals from other chains, that are relevant to Ethereum smart contracts as well. In LUKSO we use LSP (LUKSO standard proposals), that will heavily extend 725

Copy link
Contributor

Choose a reason for hiding this comment

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

If you feel strongly about this, I recommend bringing it up to the EIP editors in Discord or via a pull request to EIP-1 and the validator bot. In the meantime, I recommend making this suggested change if you want to get this merged.

EIPS/eip-725.md Outdated
created: 2017-10-02
---
updated: 2020-07-02
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
updated: 2020-07-02

There is no updated field. git commit history is the source of truth for that information should someone want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

EIPS/eip-725.md Outdated Show resolved Hide resolved
EIPS/eip-725.md Outdated Show resolved Hide resolved
EIPS/eip-725.md Outdated
Comment on lines 13 to 15

**This is the new 725 v2 standard, that is radically different from [ERC 725 v1](https://github.com/ethereum/EIPs/blob/ede8c26a77eb1ac8fa2d01d8743a8cf259d8d45b/EIPS/eip-725.md). ERC 725 v1 is be moved to #734 as a new key manager standard.**
--------
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**This is the new 725 v2 standard, that is radically different from [ERC 725 v1](https://github.com/ethereum/EIPs/blob/ede8c26a77eb1ac8fa2d01d8743a8cf259d8d45b/EIPS/eip-725.md). ERC 725 v1 is be moved to #734 as a new key manager standard.**
--------

EIPs should be written for future readers who don't care about the history. If someone wants to see the progression of an EIP, they can go read through git history or the discussion link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

frozeman and others added 5 commits November 25, 2020 16:37
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Changed proposed key `ERC725Type` to `SupportedStandards` to allow more (sub)standards to be communicated by an ERC725 contract
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.

Once fixed, CI should pass and we can get this merged.

EIPS/eip-725.md Outdated Show resolved Hide resolved
frozeman and others added 2 commits November 27, 2020 10:09
@eip-automerger eip-automerger merged commit 4d1931c into ethereum:master Nov 27, 2020
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
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