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

ERC Proxy Identity draft #1036

Merged
merged 8 commits into from
May 2, 2018
Merged

ERC Proxy Identity draft #1036

merged 8 commits into from
May 2, 2018

Conversation

frozeman
Copy link
Contributor

This PR adds the ERC identity draft with slight modifications to the discussed #725.
Once this PR is merged we can discuss specific changes by making separate PRs to the draft itself.

Following modifications are present:

I removed the getKeyPurposes in favour of keyHasPurpose, which returns a bool and will work as a check for checking keys at an identity.

I also changed the key structure to keep an array of purpose IDs in the struct of the key, but we should discuss the idea of @nick to use a bit mask instead for storing purposes later.

@frozeman frozeman mentioned this pull request Apr 26, 2018
@@ -0,0 +1,302 @@
---
eip: <to be assigned>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename to eip-725.md and number 725.

@@ -0,0 +1,302 @@
---
eip: <to be assigned>
title: ERC-725 Identity
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to include the EIP/ERC number in the title.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also argue that the title is a bit broad.

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 renamed it to Proxy Identity to be more specific

---
eip: <to be assigned>
title: ERC-725 Identity
author: Fabian Vogelsteller (@frozeman) <fabian@ethereum.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the build script will know how to parse this into a single link.
Either GitHub username or e-mail address should be sufficient.
See: https://github.com/ethereum/EIPs/blob/master/_includes/authorlist.html

@Arachnid
Copy link
Contributor

Arachnid commented May 2, 2018

Please rename to eip-725.md.

@Arachnid
Copy link
Contributor

Arachnid commented May 2, 2018

Also, see the build failure- you have an invalid internal link in your document.

@frozeman frozeman changed the title ERC Identity draft ERC Proxy Identity draft May 2, 2018
@frozeman
Copy link
Contributor Author

frozeman commented May 2, 2018

Why not adding the title in the md document name, like we have for ERC20: eip-20-token-standard.md?
Also your drafts have a title added?

@Arachnid
Copy link
Contributor

Arachnid commented May 2, 2018

@frozeman Because that's not the naming convention we use for merged drafts.
eip-20-token-standard is legacy, and provided as a redirect, because people already link to it.

@Arachnid
Copy link
Contributor

Arachnid commented May 2, 2018

@frozeman Those two drafts should never have been merged as-is. Thanks for pointing it out, I've fixed them.

@Arachnid Arachnid merged commit ede8c26 into ethereum:master May 2, 2018
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.

3 participants