Skip to content

Modernize examples in specification #1129

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

Merged
merged 9 commits into from
May 22, 2023
Merged

Modernize examples in specification #1129

merged 9 commits into from
May 22, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented May 14, 2023

This PR modernizes the examples in the specification by:

  • Updating the old RsaSignature2018 examples to use DataIntegrityProof.
  • Updating the Alumni and Degree examples to use the examples/v2 context (and changing the type names to be more explicit about being examples).
  • Removing the complex @lang/@value examples and using simpler text instead.

This PR addresses #967 and #1121.


Preview | Diff

@iherman
Copy link
Member

iherman commented May 15, 2023

A comment on the "Getting Started" example, (Example 3 in §4.1). An attentive reader's reaction may be that the example is incorrect JSON-LD, because there is no definition for, e.g., MyPrototypeCredential. We know this isn't true by virtue of the @vocab in the VCDM core @context, but the presence of that is not mentioned anywhere in the document. I realize we do not want to give a high level of publicity for it, because it may drive implementers to a wrong path, ie, misusing what is essentially a security measure, but maybe it is worth adding an editorial note in this paragraph mentioning this.

@iherman
Copy link
Member

iherman commented May 15, 2023

Looking at the examples, there is still room for confusion for the user. The term definitions come from (at least) four different places:

  • terms defined in this specification (e.g., issuer)
  • terms defined in another specification in the "family" of specs published by this WG (e.g., verificationMethod, or eddsa-2022). We also know that, somewhat deceptively, the proof property is not formally defined by this specification either, although it looks that way.
  • terms taken over from external vocabularies (e.g., name coming from schema.org)
  • example terms (e.g., alumniOf).

I know from my own experience that all this is extremely confusing, and makes it very difficult to understand the spec.

One way of handling this may be:

  • outline what I just wrote in the spec
  • (this is where it is relevant for the PR) systematically use some ways of differentiating among these categories (different color, usage of italics/bold, different fonts; I am not sure what is the best from an accessibility point of view).

I am fine if the comment is "Ivan, let us do this in a different PR". But I consider it imperative to do it at some point.

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.

The changes look OK, as far as they go. I share @iherman's concerns, and think there should be an issue opened for them if they are not addressed in this PR, so we don't forget to address them.

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.

These diagrams don't match the consensus regarding "proof" and "credential".

We had some really long threads about how "proof" is part of both "credential" and "verifiable credential"

These diagrams show proof as separate graph object.

This does correspond to what the JSON-LD context does when it is applied, but the context is not normative, so the diagrams are not reflective of normative requirements... and they seem to be contradicting consensus on "anything can have a proof".

These diagrams might be better in vc-data-integrity.

Technically "proof" is defined inside of "VerifiableCredential" and "VerifiablePresentation":

https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L51

I am requesting the following changes:

  1. Use "VerifiableCredential" not "Credential".
  2. Add examples for JWT (or file an issue and link to tools so I can provide equivalent diagrams).

@OR13
Copy link
Contributor

OR13 commented May 16, 2023

Related issue regarding aligning informative examples, with normative requirements... #1103 (comment)

@OR13
Copy link
Contributor

OR13 commented May 16, 2023

Related to the "Credential" vs "VerifiableCredential" part... #1126 (comment)

@OR13
Copy link
Contributor

OR13 commented May 17, 2023

Related issue: #881

Discussed on the call today.

@msporny
Copy link
Member Author

msporny commented May 20, 2023

@OR13 wrote:

I am requesting the following changes:

  1. Use "VerifiableCredential" not "Credential".

This was done in 6156f48 and 96920dd.

  1. Add examples for JWT (or file an issue and link to tools so I can provide equivalent diagrams).

The Editors of the VC-JWT specification can provide those. I've raised issue#1135 for them to process.

I believe this addresses both of your change requests @OR13, please re-review.

@msporny msporny requested a review from OR13 May 20, 2023 18:57
@OR13
Copy link
Contributor

OR13 commented May 21, 2023

See this PR which attempts to update the examples, and the related tooling currently being deployed... w3c/respec-vc#8

I think it would be better to simply switch to mermaid at this point... but I can do that in a subsequent PR.

@OR13
Copy link
Contributor

OR13 commented May 21, 2023

I agree with @iherman 's comments about the proof values, we expect to see cryptosuite, we don't expect to see nice human readable names for verification method.

It would be good to make the diagrams consistent, with the actual nquads.

@longpd
Copy link
Contributor

longpd commented May 22, 2023

+1 to Ivan's suggestion for future attention, and the resolution to the naming issues Manu has responded to from Orie. If I were an approver I'd agree, too!

@msporny
Copy link
Member Author

msporny commented May 22, 2023

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

@msporny msporny merged commit f26b43e into main May 22, 2023
@msporny msporny deleted the msporny-update-examples branch May 22, 2023 16:56
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.

9 participants