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

Amplify/broaden def of 'verification method' #284

Merged
merged 3 commits into from
May 16, 2020

Conversation

dhh1128
Copy link
Contributor

@dhh1128 dhh1128 commented May 13, 2020

Signed-off-by: Daniel Hardman [email protected]

See #276

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Looks good. I believe this also addresses #57.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

This is awesome. Only improvement I can see is to the bottom paragraph.

We have a couple scenarios bundled here... threshold signatures and biometrics...

I would like to see concrete example of both somewhere, if we are going to comment on them here... but I am also not aware that there are any, so this might not be possible.

@peacekeeper
Copy link
Contributor

The only aspect of this PR I find problematic is that it adds a lot of text to the Terminology section. All other terms in that section consist of a single paragraph, whereas this one has three. Perhaps most of this new text should be moved elsewhere (especially if a new verificationMethod core property is introduced - as suggested in #283).

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

LGTM other than minor editorial nits to not use normative looking language (someone on the Internet inevitably complains about it while we're in the final push to REC and the Editors have to go through and remove all lowercase must, may, should, etc. from the spec).

I also note @peacekeeper's concern, and that we'll probably create a new section on verification methods and move some of the text to that section, with a pointer from the definition on those that want to learn more. I think we should preserve @dhh1128's text through to the end because it does the best job of describing what a verification method is to date.

Thank you, @dhh1128! :)

terms.html Outdated Show resolved Hide resolved
terms.html Outdated Show resolved Hide resolved
terms.html Outdated Show resolved Hide resolved
terms.html Outdated Show resolved Hide resolved
Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

Editorial, multiple positive reviews, changes made, no objections, merging.

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.

5 participants