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

Address Jeffrey Yasskin's review comments that are Editorial #1464

Merged
merged 33 commits into from May 5, 2024

Conversation

msporny
Copy link
Member

@msporny msporny commented Mar 23, 2024

This PR is an attempt to address issue #1348 by fixing a large number of editorial concerns that @jyasskin raised.

Specifically:

1. Introduction

  • Use property terminology throughout entire specification. Fixed in c36dd64.

1.1 What is a Verifiable Credential?

  • Specify which properties exist on a VC. Fixed in 42d38f2.

1.3 Use Cases and Requirements

  • Use non-normative wording instead of "should". This section has been removed by the specification.

3.2 Credentials

  • Mention quoting and verifiable credential graph. Fixed in 6938250.

4.1 Getting Started

  • Make section non-normative. This was done in a previous PR.

4.2 Contexts

  • Use better language to specify ordered sets. Fixed to use INFRA in 1920037.
  • Remove redundant "This specification requires for a @context property to be present;" statement. Fixed in 52562ec.
  • Rewrite paragraph that contains "Verifiable credentials and verifiable presentations have many attributes and values that are identified by URLs" -- re-write paragraph to introduce contexts in a different way. Rewritten in 1920037.

4.3 Identifiers

  • Merge the id and the "If the id property is present:" block. Fixed in 7a96f9e.

4.4 Types

  • Clarify that you can use a full URL, or you can use a JSON-LD term that maps to a URL. That is, you can't use a number, string, object, etc. Fixed in a45bb7b.
  • Point to VC-SPECS for some examples of "A valid proof type". Discussion of proof was removed from this section in a previous PR.
  • Clean up and reword section containing "This enables implementers to rely on values associated with the type property". Attempted clean up in c07066f.

5.1 Lifecycle Details

5.2 Trust Model

5.3.1 Semantic Interoperability

  • "is expected to be published" diffuses responsibility. Use active voice. Fixed in 024994a.

5.4 Integrity of Related Resources

5.5 Refreshing

5.11 Ecosystem Compatibility

  • Clarify intent of "rules" in section to be guidance to specification authors that desire compatability. Fixed in df1a45e.

6.3 Proof Formats

  • Change "The sections detailing" to "The specifications detailing". The section has been rewritten in a previous PR and the problematic text was removed in the rewrite.

7. Privacy Considerations

  • "Implementers of software utilized by holders". This text no longer appears in the specification due to a previous PR.

7.3 Personally Identifiable Information

  • Update "Implementers" to "Implementers of software utilized by holders". Fixed in 63d1463.
  • Say that people should assume a VC will leak PII unless the credential and securing mechanism is designed specifically to avoid correlation (and there are credential types designed specifically for this purpose). Fixed in cd6116c.

7.4 Identifier-Based Correlation

  • Specify that proof specifications are responsible for avoiding identifier-based correlation. Fixed in 69af3d5.

7.5 Signature-Based Correlation

  • Specify who is responsible for signature-based correlation and update section to match modern practices. Fixed in

7.6 Long-Lived Identifier-Based Correlation

  • Fold this section into the "Identifier-based Correlation" section and provide a refresh on the text. Fixed in 1c9ea79.
  • Say credential repository software needs to provide protections. Fixed in 1c9ea79.

7.8 Favor Abstract Claims

  • Specify who should favor abstract claims -- "issuers should favor abstract claims". Fixed in bdc9c44.

7.9 The Principle of Data Minimization

  • Specify that this is advice for issuers and verifiers. Remind credential repositories to disclose what fields the verifier is asking for, so that users can push back. Fixed in aca01b0.

7.11 Validation

  • Possibly move section into larger Validation section? Renamed section in 4ee5f17.

7.12 Storage Providers and Data Mining

  • Say that civil society groups and regulators should play a part in this ecosystem (by doing evaluations and monitoring). Fixed in de2dec6.

7.14 Usage Patterns

  • Localize mitigations to particular actors by saying things like "Authors of specifications for credentialStatus types", and "Using a globally-unique identifier" could be guidance for credential repositories to warn uses when they're re-using an ID. Fixed in 4dc6700.

8.3 Content Integrity Protection

  • Update outdated text and speak to new relatedResource feature. Fixed in 8d9a315.

A.1 Credential Type

  • Specify that the documents at the type's URL are expected to describe the available properties and their usage. The language for this was put in the Types section in c07066f.

Preview | Diff

@msporny msporny changed the title Consistently use "property" instead of "attribute" terminology. Address Jeffrey Yasskin's review comments that are Editorial Mar 24, 2024
index.html Outdated
<dd>
Defined in Section [[[#credential-subject]]].
</dd>
<dt>proof</dt>
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to me to list proof as a property defined in this specification then point to data integrity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed from this list in ea0bfb8.

Copy link
Member

Choose a reason for hiding this comment

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

Sigh, the official preview was not regenerated, so this is still in the list. Just to be cautious for reviewers... ☹️

<p>
A [=verifiable credential=] can be extended to have addition properties
through the extension mechanism defined in Section [[[#extensibility]]].
</p>
Copy link
Member

Choose a reason for hiding this comment

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

maybe the proof property could be mentioned here instead, as an example of an additional property included via an extension mechanism?

Copy link
Member Author

Choose a reason for hiding this comment

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

We link to the VC-DATA-INTEGRITY spec in the Securing Mechanisms section, so we're probably fine to just not include it in this part of the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with not mentioning proof here.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Comment on lines +4111 to 3869
properties such as `initialRecipient` (a/k/a `issuee`), `presenter`,
`authorizedPresenter`, `holder`, etc. The associated vocabulary URL MUST be
Copy link
Member

Choose a reason for hiding this comment

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

Here we have `initialRecipient` (a/k/a `issuee`) as well as `authorizedPresenter`.

It seems like these should perhaps be added to the VCDMv2 reserved terms list.

I am also concerned that confidenceMethod is itself severely underspecified, and does not appear to discuss any of the above properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Editor hat on) The description of confidence method is problematic. Given that almost no work has gone into it since it was introduced, I'm hesitant to reserve properties that it contemplates (that aren't documented at all).

(Digital Bazaar hat on) We have found use cases where confidence method is useful, so expect it to be more fully formed in time. However, for the time being, I'd rather not do anything about this issue in this PR.

We might want to raise another issue about confidenceMethod... or just deal with it when we have to make a judgement call on reserving it or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @TallTed that issuee and authorisedPresenter should be reserved terms (even if their formal definitions are not included in the specification)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @msporny that this should be dealt with in a separate issue.

@decentralgabe decentralgabe linked an issue Apr 2, 2024 that may be closed by this pull request
37 tasks
@TallTed TallTed mentioned this pull request Apr 3, 2024
@msporny msporny added editorial Purely editorial changes to the specification. CR1 This item was processed during CR1 labels Apr 20, 2024
@msporny msporny force-pushed the msporny-jyasskin-editorial-fixes branch from 7a96f9e to 9d2560e Compare April 20, 2024 16:45
@msporny msporny marked this pull request as ready for review April 21, 2024 18:03
@msporny
Copy link
Member Author

msporny commented Apr 21, 2024

/cc @jyasskin Finally got around to applying all 39+ editorial changes you requested to VCDM v2.0.

index.html Outdated Show resolved Hide resolved
Comment on lines +4111 to 3869
properties such as `initialRecipient` (a/k/a `issuee`), `presenter`,
`authorizedPresenter`, `holder`, etc. The associated vocabulary URL MUST be
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @TallTed that issuee and authorisedPresenter should be reserved terms (even if their formal definitions are not included in the specification)

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
<dd>
Defined in Section [[[#credential-subject]]].
</dd>
<dt>proof</dt>
Copy link
Member

Choose a reason for hiding this comment

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

Sigh, the official preview was not regenerated, so this is still in the list. Just to be cautious for reviewers... ☹️

Comment on lines +1617 to +1485
<a data-cite="?VC-IMP-GUIDE#creating-new-credential-types">
Creating New Credential Types</a> in the [[[?VC-IMP-GUIDE]]].
Copy link
Member

Choose a reason for hiding this comment

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

This is a side issue, not closely related to the review. The latest official version of the impl guide is from September 19, which is ancient history in terms of the spec. I realize that there has been changes (the editor's draft dates from September '23), which is better. I believe we should re-publish that WG Note draft (and set up Echidna to handle the changes) roughly when this document goes to a new CR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, agreed. If we re-publish it as an updated NOTE, we should make a full Editorial pass... there is guidance in there now that is VERY OLD and should be updated.

index.html Outdated
`digestSRI` or `digestMultibase` value.
</dd>
</dl>

Copy link
Member

Choose a reason for hiding this comment

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

This new section "smells" like defining a new type, in RDF sense, for a related resource. At the moment, in the vocabulary, the range of a relatedResource property is IRI, i.e., the only restriction on the property is that it is a an object property (as opposed to a datatype property). This bullet list seems to indicate that we want a new type (RelatedResource), in the domain a number of properties.

Do we want to formalize that in the vocabulary? Note that the last statement ("MUST contain at least a digestSRI or digestMultibase value") would require some OWL wizardry, which we probably should not include in our (simplified) vocabulary. Note also that this would require some changes in the DI vocabulary, because that is where digestMultibase is defined.

(If we agree these changes should happen, I am happy to come up with new PR-s at some point.)

Copy link
Member Author

@msporny msporny Apr 22, 2024

Choose a reason for hiding this comment

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

If we formalize that, we should probably formalize both relatedResource and its type in the security vocabulary. It makes more sense for it to go over there than for it to be defined by the VCDM. If we make this decision, we'll need to make it quickly and affect the change quickly so that we can lock the tests down for CR.

Copy link
Member

Choose a reason for hiding this comment

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

I am happy to do either way... in any case, both vocabularies will have to be updated.

I would propose to (1) have this PR merged and (2) have DI PR-s, especially w3c/vc-data-integrity#258 handled and possibly merged before doing this. I do not want stupid merge conflicts...

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this addition is not editorial. As one example, it pulls the use of digestMultibase into the VCDM specification, which I object to, per previous discussions. Please remove these non-editorial additions.

Copy link
Member Author

@msporny msporny Apr 25, 2024

Choose a reason for hiding this comment

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

It was requested, by a rep from Google, that this section was cleaned up to align with the other sections.

In doing so, I consolidated all the existing normative requirements in a way that matches the other sections in the specification. The digestMultibase value was an at-risk feature, along with digestSRI, as specified here (see the at risk text starting with "Unification of cryptographic hash expression formats are under discussion" which was added when this section was added to the specification):

https://pr-preview.s3.amazonaws.com/w3c/vc-data-model/pull/1464.html#integrity-of-related-resources

Please concretely identify which normative assertions didn't exist in the specification before the change.

Comment on lines +4111 to 3869
properties such as `initialRecipient` (a/k/a `issuee`), `presenter`,
`authorizedPresenter`, `holder`, etc. The associated vocabulary URL MUST be
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @msporny that this should be dealt with in a separate issue.

index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@decentralgabe decentralgabe 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 work!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@iherman
Copy link
Member

iherman commented Apr 24, 2024

The issue was discussed in a meeting on 2024-04-24

  • no resolutions were taken
View the transcript

5.1. Address Jeffrey Yasskin's review comments that are Editorial (pr vc-data-model#1464)

See github pull request vc-data-model#1464.

Manu Sporny: the big ones for VCDM 2.0 are the editorial changes from J. Askin's (sp) proposed changes. Having more eyes on this would be helpful.

Brent Zundel: reviews of VCDM 2.0 has received positive reviews.

Manu Sporny: he will merge the VCDM 2.0 this weekend. Get your comments in!

by that identifier.
</li>
<li>
The `id` [=property=] MUST NOT have more than one value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the statement that the id must have only one value being deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't deleted, it was collapsed into a more succinct description:

https://pr-preview.s3.amazonaws.com/w3c/vc-data-model/pull/1464.html#dfn-id

index.html Outdated Show resolved Hide resolved
msporny and others added 20 commits April 28, 2024 10:05
Co-authored-by: David Chadwick <d.w.chadwick@verifiablecredentials.info>
Co-authored-by: Ivan Herman <ivan@w3.org>
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Michael B. Jones <michael_b_jones@hotmail.com>
@msporny msporny force-pushed the msporny-jyasskin-editorial-fixes branch from 92d28c1 to a3bdbb4 Compare April 28, 2024 14:07
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Small things...

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@iherman
Copy link
Member

iherman commented May 1, 2024

The issue was discussed in a meeting on 2024-05-01

  • no resolutions were taken
View the transcript

2.2. Address Jeffrey Yasskin's review comments that are Editorial (pr vc-data-model#1464)

See github pull request vc-data-model#1464.

Brent Zundel: This is Manu's healthy response to a very thorough review from Google's AC rep, Jeffrey Yasskin.
… The impetus for the review is that we wanted Google to say all the things they wanted fixed to avoid any formal objections from them. That was the gist of the request -- and he provided an acceptable thorough review, the normative changes have already been made, and this is the editorial requests.
… This is someone who is very versed in Web specs in general and the Web in particular, but not so versed in VCs and he requested clarifications where he thought clarifications would result in a better spec.
… Some other changes are in a PR from David Chadwick.
… This PR is quite long and has some requests for changes and some approvals.
… Timeboxing this one to 7 minutes.
… Please jump on the queue to talk about 1464.

Ted Thibodeau Jr.: I believe all of my stuff is editorial.

Brent Zundel: Mike you were perhaps saying "don't do this at all"?

Michael Jones: I am ok with the vast majority of this. I'm requesting that the text elevates multihash into normative usage be struck because that's not appropriate in an editorial PR.
… Manu's tried to say it's already in the context so it doesn't matter. The specification defines what people normatively do, not the context.

Brent Zundel: The one issue I'm running into -- with the additional commits and changes ... trying to find the comments. Down in line 3155, if folks want to read along. That's a conversation we want to have.

Gabe Cohen: I believe it would be more neutral to say that you can use a digest property defined by the securing mechanisms. digestSRI isn't defined by either of our securing mechanisms.
… Manu's suggestion was to move it into VC-JOSE-COSE, but I think on further reflection we shouldn't move it, I think it's in another spec already and neutral. I think a possible way forward is to say you can use either digestSRI or a method from another securing spec without saying which spec.

Dave Longley: my memory of consensus was that implementers could use either, and we'd see how that goes.
… in terms of how digestSRI being in a different spec, that's not quite right...there's lots of work in our spec to shape it as a property and value for VCs.
… this PR seems to aim at the previous consensus.

Brent Zundel: the digestMultibase in the Data Model does not point to an unspecified thing, but to something we have published.
… Before going back to the queue -- the digestMultibase as referenced, does not point to an unpublished spec somewhere at IETF, it points to the DI spec that our group has published as CR that defines the particular things it is concerned with. In my understanding, there is no normative pointing to something that isn't a spec, except that we're pointing at the DI spec which is underway in becoming a spec.
… so there is no normative pointing to something that cannot be normatively pointed to.

Dave Longley: +1 to brent.

Ted Thibodeau Jr.: Mike, you are objecting to a thing called multihash which isn't in this document.

Michael Jones: digestMultibase sorry.

Ted Thibodeau Jr.: That appears to not be as significant a change as you're making it be.
… My eyes are different than yours.

Michael Jones: Gabe, thank you, you are correct, I have no objection to digestSRI because that's in a W3C REC that defines or largely defines what we would need. Largely for the record, my objections to the whole multihash family is that they fail to make a choice.
… That's my primary objection to using that at all.

Dmitri Zagidulin: I think 'multihash family' may be somewhat confusing. I think Mike's main objection is to multiBASE.

Michael Jones: With respect to using that at all, with respect to DI standardizing digestMultibase, that's fine, but we went through an exercise six months ago or so of making sure that the data model didn't have any normative dependencies on either security spec -- it has informative references.
… Part of the reasons for doing the controller document work -- an alternative was expressed that maybe we can just reference the controller document section from the data model and call it good. Orie and I and others felt like that was turning things on their heads and the data model should stand on its own.
… I don't want to change it to have dependencies on either securing spec.

Dave Longley: just a clarification, there are pieces of the SRI spec did have to be added to our vocabulary earlier.
… I did not mean to imply that it was yet to be done, but to point out that we could not simply point to someone else's specification.

Gabe Cohen: +1 Brent.

Brent Zundel: Since the changes to section 5.3 of related resources is the piece under contention, and I will reach out to Manu here too, the suggestion is that the changes to this section be removed from this PR and made a separate PR that can be further discussed so it doesn't hold up all of the other myriad changes that do not have opposition.
… Mike, would that alleviate your concerns with the PR?

Michael Jones: Absolutely, once that text is gone, I'll approve.

Brent Zundel: I'll let Manu know we can put those changes in a separate PR.
… Any other comments on this PR before we move on?
… I think we covered the main big things we wanted to talk about and maybe we can move to issue processing now.

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@msporny
Copy link
Member Author

msporny commented May 5, 2024

Brent Zundel: Since the changes to section 5.3 of related resources is the piece under contention, and I will reach out to Manu here too, the suggestion is that the changes to this section be removed from this PR and made a separate PR that can be further discussed so it doesn't hold up all of the other myriad changes that do not have opposition.
… Mike, would that alleviate your concerns with the PR?
Michael Jones: Absolutely, once that text is gone, I'll approve.

I have reverted all changes to Section 5.3: Integrity of Related Resources (in commit 150bfbb) and raised those changes in PR #1484 for further discussion.

@msporny
Copy link
Member Author

msporny commented May 5, 2024

Editorial, multiple reviews, most changes requested and made, objections by @selfissued reverted and moved to PR #1484, no remaining objections, merging.

@msporny msporny merged commit af4ff9f into main May 5, 2024
1 check passed
@msporny msporny deleted the msporny-jyasskin-editorial-fixes branch May 5, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 This item was processed during CR1 editorial Purely editorial changes to the specification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-normative changes from Jeffrey Yasskin's review
8 participants