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

DID Core editorial review (2) #403

Closed
17 of 30 tasks
iherman opened this issue Sep 18, 2020 · 13 comments
Closed
17 of 30 tasks

DID Core editorial review (2) #403

iherman opened this issue Sep 18, 2020 · 13 comments
Assignees
Labels
editorial Editors should update the spec then close pr exists There is an open PR to address this issue

Comments

@iherman
Copy link
Member

iherman commented Sep 18, 2020

Here is the second (and last) part of my editorial review.

  • (Generic comment) maybe we should remove the references to closed issues from the document (e.g., Issue 204, referred to from §6.1.1)

5. Core properties

  • If, as a reader, I have to find the normative definition of the properties, I would look at the table of content to find that. But there is a bunch of inconsistencies:

    • Some section titles are simply the name of the property (e.g., "authentication") others are not (e.g., "DID Subject", "Service Endpoint")
    • Some properties are not in their own subsection like id, type (also alsoKnownAs but there is a separate PR to take care of that).
    • Some properties are actually shared among others, like id and type but are not separately defined
    • The only place where there is a reference to all core properties is in the introductory paragraph of §5, but that is not clear from the TOC

    I think what is missing is a clear, formal specification of the full DID Core vocabulary, with all the properties and classes (types), their shape constraints, domain and range definitions, etc. That specification should be at one place in the document with links to the section where these terms are defined in English prose. That vocabulary would also be defined, for the LD world, as a formal, RDFS/OWL ontology in its own namespace. The Web Annotation Vocabulary might be a good example for what I am referring to, although that vocabulary is many time larger and more complex than DID Core.

    (Note that the definition should not be made using rdf:domain and rdf:range to define constraints. Those statements are licenses to infer and not a constraint. If we want to define constraints we either must use English prose, or formalism like SHACL. Or both, actually.)

  • This is related to the previous comment, but is a comment in itself. For a clean ontological definition of the verification methods and verification relationships, I wonder whether it is not worth defining, formally, something like a VerificationMethod type, which would be a supertype for the types like Ed25519VerificationKey2018, and which would also act as a clean range constraint for methods like authentication as well as part of the domain constraints for properties like controller or id. All this should be part of the aforementioned vocabulary specification. (Clearly, this is not really an editorial comment only. Sorry about that…)

5.3.1 Key types and formats

  • It is not 100% clear from the text whether the terms in the table (RSA, ed25519, etc) are specified in this specification, i.e., whether they are defined as standard terms just like the core properties. I suspect they are because it is all part of a normative section. If it so, then the terms like Ed25519VerificationKey2018 must be formally defined as a type, as part of the vocabulary defined by this specification. These terms must be added to the aforementioned DID Core vocabulary definition. Furthermore, and as a consequence, terms like publicKeyPem, publicKeyBase58 are also terms formally defined by this specification and should be presented as such.

5.4 Verification Relationships

  • I must admit the practical differences among the different verification relationships confuse me. The definition of each of those terms is essentially identical (I have the impression that the sections are copy pasted first and then the terms have been adapted in the text and the examples) and I found no examples, definition, etc, on what the real-life usage difference is among these different terms (the only reference I saw much later in the document is where authentication and authorization entries are used by services). Why is it necessary to have a different assertion method than an authentication, for example? Why isn't enough for a DID document to "simply" convey a set of verification methods?

    I am sure there are good reasons to have those, but these are not made clear in the text. One or more very short use cases put into a Note would go a long way to make text clearer (like the aforementioned reference to service endpoints).

5.5 Service endpoints

(I know that this section may radically change but the comment might still be relevant)

  • I have already commented on the fact that the examples should make it absolutely clear which properties are defined in this document and which are here for the purpose of an example only (see DID Core editorial review (1) #401). Example 19 is a very clear case where this is absolutely essential. It is a long JSON fragment, which is full of ad-hoc terms (instances, spamCost, currency) and types that are not defined by this specification. A cursory reader should be aware of that!
  • One of the examples uses the verificationMethod property. The intention is clear but this shows that the verificationMethod property is not properly defined: what is the allowed domain of this property? Everything I read so far in the text suggests that it is a DID Document (although, again in a vocabulary, no such type has been defined) but the example shows that it is not the case, because it can also be used on an object of type "service". On the other hand, I presume it could not be used on any other object type. These details MUST be defined formally somehow, somewhere.

6.1 Core representation - JSON

  • The reference to [JSON-LD] should be exchanged against [JSON-LD11]. The latter is now a Rec that will (soon) officially supersede 1.0.
  • Both the JSON production and consumption subsection say, e.g., "Implementers consuming JSON are advised to ensure that their algorithms are aligned with the JSON consumption rules in the [INFRA] specification." (emphasis is mine). Any reason why we do not rely on the INFRA rules normatively and we "just" define the specificities (e.g., that @context must be ignored)?

6.2 Core representation - JSON - LD

  • Just flagging a minor point: if we adopt my comment (expressed in DID Core editorial review (1) #401) of introducing a separate subject property, that must be aliased to @id, too.
  • In the section on consumption it says "To enable interoperability with other representations, URLs registered in the DID Specification Registries [DID-SPEC-REGISTRIES] referring to JSON-LD Contexts SHOULD be associated with a cryptographic hash of the content of the JSON-LD Context." While the intention is clear, this specification cannot dictate how the registry operates. The registry should make it clear that it requires a hash to be added to a context, and this specification should say something like if the hash is present in the registry, the consumption step MUST (SHOULD?) include a check of the hash value of the relevant context file.

7. Methods

  • "The method name SHOULD be five characters or less.": Why this restriction? Actually, the registry already includes methods that violate this: did:vaultie, did:emtrust, did:selfkey. Even if the statement is a SHOULD and not a MUST, it looks funny if by the time we go to Rec we will have registered methods that do not follow this…
  • There are several MUST, SHOULD, and MAY statements in this section ("MUST specify", "MUST be able"…). How would we check this from a conformance point of view? Maybe these statements should not be BCP47 terms in this document (ie, should be spelled as, e.g., "must"), and these requirements must be made explicit (either by repeating the text or by reference) in the registry document (at present this is not the case). This text should also make clear that the method registry mechanism makes it sure that a particular did method abides to these rules.

8.1 DID Resolution

  • It is not clear from the text what a "be a DID document conforming to the abstract data model" means as opposed to "DID document in one of the conformant representations.". Indeed, an abstract data model is abstract and it is not clear how a resolver could return it without representing it in a conformant representation. Which ends up saying that I am not sure what the difference is between resolve and resolveStream. An explanatory note on this would be useful.

  • The accept input metadata property: the way the text reads the value can be a single mime type. Is it worth considering a comma separated link of mime types like in the case of the HTTP Accept header?

    Note if the answer is 'yes' and changes are made, they should also be made on the corresponding DID URL dereferencing input metadata in §8.2.1. In fact, for the case of DID URL Dereferencing this option may be even more useful than for DID dereferencing as this value could be translated, by a method, directly into an HTTP Accept header in case the resource to be fetched is, in fact, an HTTP resource.

8.3 Metadata structure

  • I am not sure it is a good idea to show the metadata in JSON. It suggests (although not true) that the metadata structure is supposed to be conveyed in JSON; in a specification where all the examples are in JSON, this is an easy mistake to be made. The INFRA syntax should suffice imho.

9.8 The role of human-friendly identifier

  • "Solutions to this problem (and there are many)": I think we should remove the parenthetical phrase. It does not belong here.

9.9. Immutability

  • Flagging again: if we adopt my comment (expressed in DID Core editorial review (1) #401) of introducing a separate subject property, all references to id must be changed in this section.

10. Privacy Considerations

11. Examples

  • I am not sure the examples in §11.2 and §11.3 really bring anything to table in understanding this spec. They are relatively complex VC examples where DID-s are used as identifiers. That is all. I would propose to remove them.

A. Closed issues:

  • Remove the references to the closed issues.

B.2 application/did+ld+json

B.3 and B.4

C1. Normative references

  • BASE58 (https://tools.ietf.org/html/draft-msporny-base58-01) is a draft, and cannot be a normative reference. Must be moved to the C2.
  • DID-CORE: there should not be any self-reference in the references' list (this is related to a comment I had in DID Core editorial review (1) #401)
  • The DID Spec Registries document is not a Rec-track document, it must not be listed as a normative reference. Move it to C.2
  • Based on my comment above, the JSON-LD reference should be removed. The JSON-LD 11 reference should be updated to the Rec (isn't the document using spec-ref? How come it still refers to a Working Draft?)

C2. Infomative references

  • The DID Use Case document reference should use the W3C TR URL https://www.w3.org/TR/did-use-cases/ as a reference
  • The most recent reference to hashlinks should be https://tools.ietf.org/html/draft-sporny-hashlink-04
  • VC Data model reference should refer to the Rec and not to the Proposed Rec.
@iherman
Copy link
Member Author

iherman commented Sep 18, 2020

I realize that the first comment on §5, though does not change the core specification and, in this respect, is not a substantial change, is way more than "just" an editorial issue. I let @msporny, @peacekeeper, or @talltree decide whether this should be spun into a separate issue and/or a PR.

At this point, I am not sure whether I can find the time to contribute such a PR (ie, to create a spec-text for the vocabulary) but we can discuss that later. And it is of course contingent on whether the WG agrees with its creation.

@iherman
Copy link
Member Author

iherman commented Sep 19, 2020

I decided to move the comment on §5 into a separate issue (#404). I have not removed that comment from the current issue, but I have checked the relevant checkbox, as well some other that all belong to the same problem area of vocabulary definition.

My apologies for the confusion, I should have put that one into a separate issue in the first place.

@rhiaro
Copy link
Member

rhiaro commented Sep 22, 2020

[Methods] How would we check this from a conformance point of view?

The section should be an easy to read list so that a method implementer can look down it and say "yes I do this", "no I don't do this", and then self-assert (and ideally point to the sections in their spec) for the purposes of say, passing the 'test suite' or being listed somewhere as a conformant implementation of a DID method spec. Similarly the list is there for the people who are checking additions to the DID Method Registry. Personally I think the normative language is fine - it's how you can say if something is a conformant DID method spec or not. Agreed it wouldn't hurt for the DID Method Registry to point to this.

@rhiaro rhiaro added the editorial Editors should update the spec then close label Sep 22, 2020
@rhiaro rhiaro self-assigned this Sep 22, 2020
rhiaro added a commit that referenced this issue Sep 22, 2020
@rhiaro
Copy link
Member

rhiaro commented Sep 22, 2020

terms like Ed25519VerificationKey2018 must be formally defined as a type, [..] terms like publicKeyPem, publicKeyBase58 are also terms formally defined by this specification and should be presented as such.

Agreed... #281 (comment) says some of the terms are defined, but I don't see where. Will look into this in a separate PR from the editorial stuff.

@iherman
Copy link
Member Author

iherman commented Sep 23, 2020

Agreed it wouldn't hurt for the DID Method Registry to point to this. (#403 (comment))

Yes, in any case. Something should be there reinforce this.

@rhiaro
Copy link
Member

rhiaro commented Oct 25, 2020

PR in the Registries for comment about context hashing: w3c/did-extensions#146. Agreed we could have language about the JSON-LD consumer checking the hash, but only after it's a registration requirement.

@rhiaro
Copy link
Member

rhiaro commented Oct 25, 2020

The accept input metadata property: the way the text reads the value can be a single mime type. Is it worth considering a comma separated link of mime types like in the case of the HTTP Accept header?

@peacekeeper Are multiple mime types valid? Will PR if so.

@rhiaro rhiaro added the pr exists There is an open PR to address this issue label Oct 25, 2020
msporny pushed a commit that referenced this issue Oct 26, 2020
rhiaro added a commit that referenced this issue Oct 27, 2020
rhiaro added a commit that referenced this issue Oct 27, 2020
rhiaro added a commit that referenced this issue Oct 27, 2020
We already have registered methods that violate this requirement;
and methods should be free to make their own choices about this.
For #403
@rhiaro
Copy link
Member

rhiaro commented Oct 27, 2020

Re: key types and formats - I believe this is covered by #423 (and in general is definitely more than an editorial issue at the moment).

One of the examples uses the verificationMethod property. The intention is clear but this shows that the verificationMethod property is not properly defined: what is the allowed domain of this property? Everything I read so far in the text suggests that it is a DID Document (although, again in a vocabulary, no such type has been defined) but the example shows that it is not the case, because it can also be used on an object of type "service". On the other hand, I presume it could not be used on any other object type. These details MUST be defined formally somehow, somewhere.

As the spec is currently written, the domain would not be the DID doc, but the DID subject, which (as written today) can be anything. So my understanding is there is no constraint with regard to type on the domain of the verificationMethod property.

the examples should make it absolutely clear which properties are defined in this document and which are here for the purpose of an example only

I've spun this out into #447 as a reminder to run through everything later when things are more stable.

@rhiaro
Copy link
Member

rhiaro commented Oct 27, 2020

Which PR should I check (among the many) to check #401? I have already checked off a number of those...

I just meant, the things that are duplicated in or covered by another issue already, or that I commented I don't think will be addressed.. ticking them off doesn't seem right if they're not addressed, but some way of marking things as no longer outstanding so I don't have to keep reading the list over and over.. maybe I should make my own copy of the list :)

rhiaro added a commit that referenced this issue Nov 2, 2020
msporny pushed a commit that referenced this issue Nov 4, 2020
msporny pushed a commit that referenced this issue Nov 4, 2020
We already have registered methods that violate this requirement;
and methods should be free to make their own choices about this.
For #403
@msporny
Copy link
Member

msporny commented Dec 1, 2020

Closing based on request by @iherman since many changes have been made based on this issue and another re-read will be required at this point to see if the original unresolved issues remain.

@msporny msporny closed this as completed Dec 1, 2020
msporny pushed a commit that referenced this issue Dec 6, 2020
msporny pushed a commit that referenced this issue Dec 6, 2020
msporny pushed a commit that referenced this issue Dec 6, 2020
We already have registered methods that violate this requirement;
and methods should be free to make their own choices about this.
For #403
rhiaro added a commit to w3c/did-extensions that referenced this issue Jan 10, 2021
msporny pushed a commit to w3c/cid that referenced this issue Jun 2, 2024
We already have registered methods that violate this requirement;
and methods should be free to make their own choices about this.
For w3c/did-core#403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Editors should update the spec then close pr exists There is an open PR to address this issue
Projects
None yet
Development

No branches or pull requests

3 participants