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 @jyasskin's pre-Candidate Recommendation review comments #1357

Merged
merged 13 commits into from
Nov 30, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented Nov 18, 2023

This PR attempts to address issue #1347 by making modifications to the specification based on @jyasskin's review:

2. Terminology

  • Make section normative. [Fixed in e8a7c9f]

4.1 Getting Started

  • Make section non-normative. [Fixed in cb3696e]

4.2 Contexts

  • Remove ""each URL in the @context be one which, if dereferenced, results in a document containing machine-readable information about the @context." statement since it just describes how JSON-LD works (the data model for the spec). [Fixed in f62f40c]
  • Clarify what "express context information" means. [Fixed in 52dfb8c]

4.4 Types

  • Downgrade "All credentials, presentations, and encapsulated objects MUST specify, or be associated with, additional more narrow types" from MUST to SHOULD. [Fixed in 9b21005]

4.7 Issuer

  • Update "the URL …, if dereferenced, results in a document … that can be used to verify the information expressed in the credential." to refer to controller documents. [Fixed in 6aa599c]

5.4 Integrity of Related Resources

  • Update normative statements to say that the Accept header would be used for mediaType and the returned document would need to have that media type listed in the Content-Type. [Fixed in 9ebfe55]
  • Specify that any relatedResource that fails content integrity protection results in a verification error. [Fixed in 8187708]

5.8 Zero-Knowledge Proofs

  • Remove "The verifiable credential MUST contain a Proof" normative statement. [Fixed in 3a346e9]
  • Delegate "not leaking information" to securing specifications. [Fixed in 3fdc1f7]
  • Delegate "The verifiable presentation SHOULD contain a proof property to enable …" to securing specifications. [Fixed in 3a346e9, by not mentioning how to implement the securing mechanism -- which is up to the securing mechanism]

7.10 Validity Checks

  • Add normative statements on CredentialStatus specifications to not enable individual tracking. [Fixed in fcfd9e5]

Preview | Diff

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.

Relatively small. I think all are editorial. Some further tweaking may be necessary.

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
@msporny msporny requested a review from TallTed November 20, 2023 16:57
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Just 1 substantive comment for this document; looks good once you've considered that.

Comment on lines +3083 to +3116
<li>
be used when retrieving the content, such as via the `Accept` HTTP Header
</li>
<li>
match the retrieved content media type, such as via the `Content-Type` HTTP
Header.
Copy link
Member

Choose a reason for hiding this comment

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

Note that these restrictions will need to be enforced in the fix to w3c/vc-data-integrity#222. No need to block this PR on that.

index.html Outdated
@@ -3069,6 +3100,11 @@ <h2>Integrity of Related Resources</h2>
an object in the <code>relatedResource</code> array.
</p>
<p>
Any failure to verify content integrity information that is vital to the
validity of a <a>conforming document</a>, such as the integrity of content
identified by related `@context` URLs, SHOULD result in a validation error.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should be a MUST, but that MUST should appear in the place that uses the related resource, rather than here. The wording here could be something like

The authors of any specification that fetches a resource based on the `id`
of an object inside a <a>conforming document</a> need to consider whether
that resource's content is vital to the validity of that document. If it is,
the specification MUST produce a validation error unless the resource has
the expected media type and its bytes hash to the expected digest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied a variation of your text in: 12f98c1

@msporny
Copy link
Member Author

msporny commented Nov 30, 2023

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

@msporny msporny merged commit 1d7b554 into main Nov 30, 2023
1 check passed
@msporny msporny deleted the msporny-jyasskin-pre-cr branch November 30, 2023 14:41
@iherman
Copy link
Member

iherman commented Nov 30, 2023

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

@msporny just flagging this: the VC Controller document MUST be published as a WD for the VCDM to go to CR. It is all right if it is published on the same day, but certainly not later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants