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

Added the Multikey class #93

Merged
merged 5 commits into from May 15, 2023
Merged

Added the Multikey class #93

merged 5 commits into from May 15, 2023

Conversation

iherman
Copy link
Member

@iherman iherman commented Apr 21, 2023

This is to fix #33

Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
- id: Ed25519Signature2020
label: ED2559 Signature Suite, 2020 version
upper_value: sec:Proof
upper_value: sec:VerificationMethod
deprecated: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why on which of the two statements?

For both cases I did not invent anything, just followed what is in the spec and/or in the other PR-s, that is all.

@msporny @dlongley ?

Copy link
Member

@msporny msporny Apr 25, 2023

Choose a reason for hiding this comment

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

I think what @OR13 is saying is that Ed25519Signature2020 is a type of Proof? That, or he's asking why we're deprecating the signature type? I can't tell.

But, to be clear, Ed25519Signature2020 is specifically not a type of VerificationMethod -- verification methods are mechanisms used to determine if a proof is valid.

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.

You be good to discuss PRs like this in the group.

@msporny
Copy link
Member

msporny commented Apr 25, 2023

@OR13 wrote:

You be good to discuss PRs like this in the group.

If you're going to block a PR, please provide a concrete change request in your review. I have requested a re-review from you.

- id: Ed25519Signature2020
label: ED2559 Signature Suite, 2020 version
upper_value: sec:Proof
upper_value: sec:VerificationMethod
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
upper_value: sec:VerificationMethod
upper_value: sec:Proof

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.

+1 to the PR in general. Two changes that will need to be made for it to be merged.

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.

I don't understand the PR, it seems the intention is to normatively define a key format for use with securing formats, but it is deprecating properties as well.

I prefer to discuss such changes on a call.

I do not think we should add key representations to a TR document without ever discussing them as a group.

@msporny
Copy link
Member

msporny commented Apr 25, 2023

I don't understand the PR, it seems the intention is to normatively define a key format for use with securing formats, but it is deprecating properties as well.

@iherman can you please move the Ed25519Signature2020 deprecation to another PR and just make this one about Multikey?

I prefer to discuss such changes on a call.
I do not think we should add key representations to a TR document without ever discussing them as a group.

The group is aware of Multikey's existence and the usage thereof; we have multiple implementers that have demonstrated interop on it (via did:key) and multiple implementers having written into the WG about implementing it and supporting it:

https://lists.w3.org/Archives/Public/public-vc-wg/2023Jan/0027.html

That said, feel free to mention it on the calls this week and ask for input from people on this PR.

iherman and others added 2 commits April 26, 2023 13:20
@iherman
Copy link
Member Author

iherman commented Apr 26, 2023

I don't understand the PR, it seems the intention is to normatively define a key format for use with securing formats, but it is deprecating properties as well.

@iherman can you please move the Ed25519Signature2020 deprecation to another PR and just make this one about Multikey?

I rolled back the Ed25519Signature2020 change.

@iherman
Copy link
Member Author

iherman commented Apr 26, 2023

I prefer to discuss such changes on a call.
I do not think we should add key representations to a TR document without ever discussing them as a group.

This PR (and any change on the vocabulary) just follows the standard. The current spec introduces a "Multikey" type in https://www.w3.org/TR/vc-di-eddsa/#multikey as a class which also says

type of the verification method MUST be Multikey

which means that Multikey must be a class (if it can appear as on object in a type statement), and the spec talks about a verification method be of that type, so the way I understand it it must be a subclass of VerificationMethod

And I have just translated what is in the text into the vocabulary. B.t.w., we should check all the various terms we use in any of the specs the same way.

@iherman
Copy link
Member Author

iherman commented Apr 26, 2023

I don't understand the PR, it seems the intention is to normatively define a key format for use with securing formats, but it is deprecating properties as well.

@iherman can you please move the Ed25519Signature2020 deprecation to another PR and just make this one about Multikey?

I rolled back the Ed25519Signature2020 change.

I have also created #95 for the deprecation.

@OR13
Copy link
Contributor

OR13 commented Apr 26, 2023

Based on the call today, I am approving this, it seems we are good to make these changes.

@OR13 OR13 self-requested a review April 26, 2023 19:51
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.

Approving, modulo @msporny feedback being applied.

@iherman
Copy link
Member Author

iherman commented Apr 27, 2023

The issue was discussed in a meeting on 2023-04-26

  • no resolutions were taken
View the transcript

2.3. Added the Multikey class (pr vc-data-integrity#93)

See github pull request vc-data-integrity#93.

Manu Sporny: Next up is data integrity, couple of stuck PRs here.
… orie, I think you wanted group to be aware of multikey class added to vocab (vc-data-integrity #93).

Orie Steele: my question was, for context, there are references from new vc-di-bbs work item to this, so I'm trying to understand from a normative standpoint, can I reference.
… multikey from vc-di-bbs?
… wasn't sure if group has decided to create normative key format.

Kristina Yasuda: would encourage the WG to review 1101 and 1100 before the next call.

Manu Sporny: I think the reference is fine, but agree we need to make a decision as a WG as to where the reference goes.
… multikey is the key format used in did:key, which has lots of implementations.

Orie Steele: Here is the todo, in vc-di-bbs that points to Multikey: https://w3c.github.io/vc-di-bbs/#multikey.

Manu Sporny: eddsa, ecdsa, maybe now bbs.
… we have been creating normative version of this in the cryptosuites themselves, e.g., here are the key formats this suite supports...
… I think the proposal right now is for each suite to keep defining sub-parts of multikey spec, and instead of spreading them around like that why don't we put them into the data integrity specification (normatively) so that all suites can point to one place?

Orie Steele: +1 to everything Manu is saying, it feels like we have made improvements for key formats that align with how we now have DataIntegrityProof once, and we don't define a new type for each new suite, multikey seems in that spirit.

Manu Sporny: Can a cryptosuite decide to define its own multikey identifier (probably have to do this in bbs).

Dave Longley: +1 to just put Multikey definition in the Data Integrity Proof spec as a key format that cryptosuites can use.

Orie Steele: +1 to (keeping) Multikey in data integrity, and citing it from vc-di-bbs (and others).

Orie Steele: +1 to allowing suites to not use Multikey.

Dave Longley: +1 to one place (data integrity spec).

Manu Sporny: any objections to putting multikey into data integrity spec?

Phillip Long: +1 to allow Multikey in the data integrity spec.

Chris Abernethy: (no objections voiced).

Manu Sporny: we will do that, and put def into data integrity spec.
… should cryptosuites be able to define key formats that they support? e.g., we support multi-key, but only "this subset". Would anyone object to this?

Phillip Long: +1 to allowing cryptosuites to define public keys they support.

Dave Longley: +1 to allow cryptosuites to define key types they support.

Orie Steele: +1 to allow cryptosuites to define key types they support.

Chris Abernethy: (no objections voiced).

Manu Sporny: Orie does that address your concerns.

Orie Steele: yes.
… talking about clarification of @context.

Kristina Yasuda: encourage folks to keep reviewing PRs, 1100 and 1101 appear to be crucial.


@msporny
Copy link
Member

msporny commented May 15, 2023

Substantive, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 6a52a22 into main May 15, 2023
2 checks passed
@msporny msporny deleted the vocab/eddsa-induced-changes branch May 15, 2023 08:23
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.

vc:proof scope
4 participants