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

Add publicKeyMultibase definition. #103

Merged
merged 4 commits into from
May 15, 2021
Merged

Conversation

msporny
Copy link
Collaborator

@msporny msporny commented May 8, 2021

This PR partially addresses an issue in w3c/did#707 by registering publicKeyMultibase, which is needed before registering publicKeyMultibase in https://github.com/w3c/did-spec-registries.

@msporny msporny requested review from OR13 and tplooker as code owners May 8, 2021 14:59
@msporny msporny force-pushed the msporny-did-core-issue-707 branch from 1fda444 to 8c5ef90 Compare May 8, 2021 15:00
@msporny msporny changed the base branch from master to dependabot/npm_and_yarn/elliptic-6.5.4 May 8, 2021 15:01
@msporny msporny changed the base branch from dependabot/npm_and_yarn/elliptic-6.5.4 to master May 8, 2021 15:01
Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Do we want to have a line in here indicating that key definitions may restrict which bases are supported?

@msporny
Copy link
Collaborator Author

msporny commented May 8, 2021

Do we want to have a line in here indicating that key definitions may restrict which bases are supported?

Yeah, we do. I'm deep in something else right now, so change request would be good, if not, I may get back around to it.

@peacekeeper
Copy link
Member

Do we want to have a line in here indicating that key definitions may restrict which bases are supported?

+1, this would address questions raised e.g. in w3c/did#731 (comment) and w3c/did#707 (comment).

version of a public key. Each key type definition is expected to specify
one or more required encoding bases. Specifying only a single encoding
base per key type is preferable as it reduces the burden to reach broad
interoperability.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit worried that there's quite a bit of tribal knowledge in this. For example, how do I encode a nist p256 key here? How do I specify the encoding. If this is meant to be documentation to be "hey this thing exists" then this is fine, but if we're actually expecting people to build interoperable software against this definition they'll undoubtedly get stuck.

For specific action items I'd like to see, I'd like to see us reference how encoding works by directly referencing multibase and additionally reference how multicodec table is used and finally include how serialization of the bytes should work as well.

I'm all for this additional change, but I think this is a bit too light to actually be something that someone without prior knowledge could implement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, probably also a good idea to always recommend compressed form keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have applied suggestions from @kdenhartog, @peacekeeper, and @OR13 in 924eea4.

If you want further changes, please suggest concrete spec text -- it's hard to read minds and then write spec text. :P

@msporny msporny requested a review from kdenhartog May 14, 2021 22:03
Copy link
Contributor

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Thanks for the additional details. This is good enough now to help people define properties and as we come back to defining suites with this properties we can further align it.

@msporny msporny merged commit 2f24ac0 into master May 15, 2021
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