-
Notifications
You must be signed in to change notification settings - Fork 529
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
Add ERC: Decentralized Identity Verification (DID) #517
Conversation
✅ All reviewers have approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the eipw
errors, this seems fine. My only concern is that it doesn't provide enough detail on who would want to implement it, and how it's supposed to be used.
How is verifyIdentity
supposed to be used? How does the Identity
struct get stored in the contract?
Some of that might be covered in your reference implementation, but you need to make sure your standard is implementable with only the Specification section. The Reference Implementation is just an aide to help with understanding.
ERCS/erc-7734.md
Outdated
--- | ||
eip: 7734 | ||
title: Decentralized Identity Verification (DID) | ||
description: A standard for decentralized identity verification on the Ethereum blockchain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to mention Ethereum in the title or description. We assume all ERCs are Ethereum related.
ERCS/erc-7734.md
Outdated
|
||
## Motivation | ||
|
||
Centralized identity verification methods are often cumbersome, prone to data breaches, and do not give users control over their identity data. A decentralized identity verification standard will allow users to maintain control over their identity information while ensuring security and privacy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should include a bit more detail that differentiates this proposal from others that may be similar. There are a few that are DID-related, if I remember correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SamWilsn Updated the code, please check.
The commit 5999900 (as a parent of 4499ff4) contains errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
Head branch was pushed to by a user without write access
ac1e8fb
@SamWilsn please review it, by mistake I changed draft to review, may be because of that the merging has got stopped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
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: