-
Notifications
You must be signed in to change notification settings - Fork 116
Vocabulary improvement Phase 1 #1209
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
Conversation
- id: odrl | ||
value: http://www.w3.org/ns/odrl/2/ | ||
# prefix: | ||
# - id: odrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer removing rather than commenting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see any resolution on that so far, so I preferred to put it into a comment to be sure there is a consensus around this. Personally, I am in favor of removing it...
|
||
- id: serviceEndpoint | ||
label: service endpoint | ||
label: Service endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like DID Core Terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe... but how did this land in this vocabulary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether this was added during the expansion of the issuer-holder-verifier trinity to include coordinator, service, etc., for each?
vocab/credentials/v2/vocabulary.yml
Outdated
domain: cred:VerifiablePresentation | ||
range: cred:VerifiableCredentialGraph | ||
comment: The value of the `verifiableCredential` property MUST identify a <a href="#VerifiableCredentialGraph">Verifiable credential graph</a> (informally, it indirectly identifies a <a href="#VerifiableCredential">Verifiable credential</a> contained in a separate graph). | ||
comment: The value of the property must identify a <a href="#VerifiableCredentialGraph">Verifiable credential graph</a> (informally, it indirectly identifies a <a href="#VerifiableCredential">Verifiable credential</a> contained in a separate graph). |
There was a problem hiding this 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 what this is saying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part? The main sentence or the parenthetical one?
The value of the property is not a verifiable credential. It is a separate (rdf) graph that contains a (single) verifiable credential. This is what the JSON-LD "@container": "@graph"
statement in the @context
file leads to in terms of RDF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I further wonder about the several terms that are left without comment
descriptions.
comment: | | ||
A Presentation is data derived from one or more Credentials, issued by one or more `issuers`, that is shared with a specific `verifier`. A Verifiable Presentation is a tamper-evident Presentation encoded in such a way that authorship of the data can be trusted after a process of cryptographic verification. Certain types of verifiable presentations might contain data that is synthesized from, but do not contain, the original verifiable credentials (for example, zero-knowledge proofs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment should not be deleted, though some edit does seem worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can snag updates from the presentation related PRs for this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you propose to keep it for now and then handle it in a separate issue/PR? That is fine with me. I do not think the discussion should block the merge of this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TallTed I have reinstated the text in the latest commit.
What I would prefer is to wait until this PR is merged and then I would welcome a separate issue (or PR) proposing a replacement text.
(There are already several PRs that might add new entries to the vocabulary, and I would prefer to handle those on the improved version of the vocabulary.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iherman -- A fine plan. Might be worth memorializing whatever we plan to do (or review) later, after merging this PR, in new issue(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TallTed could I rely on you raising a new issue when the time comes regarding this specific question? This is your question after all... I will take care of the other issues (essentially, missing term definitions in the spec).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guideline is that comments should not appear if there is a "defined by" field: go to the official spec for the definition. This is not an absolute rule, there may be cases when an extra local explanation is necessary, like in the case of the usage of RDF Graphs. Or when the class is really some sort of local, RDF term that is more or less necessary to provide a consistent structure to the vocabulary (e.g., common supertypes for classes that are used in practice as objects of properties). But those are (and should be) rare. Does this answer your concern, @TallTed ? |
OK. That was not clear from reading the PR. It might be worth adding such a statement to the description of the Credentials Vocabulary itself; inline and near the start, if possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed several issues with the generated Turtle and JSON-LD files.
One of them is due to an error in the Yaml file (for which I requested a change).
Others are due to issues in yml2vocab itself, for which I made two PRs w3c/yml2vocab#13 and w3c/yml2vocab#14
For the records: the ymlvocab errors have been taken care of, and a new version of the npm library published. |
@TallTed I have added a new, general section into the template (see also in the preview). I would welcome your review to finalize it (please, review, and possibly propose replacement texts in the template.html file of this repo). Note that, if we agree on the final text, I will also do the same change on the security vocabulary. Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor tweaks.
I believe all outstanding questions/suggestions have been addressed. From my point of view, this is ready to be merged... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo the small changes added today, I'm OK with this being merged. I think it represents an overall improvement.
a129740
to
a60bb4a
Compare
2d2d8c3
to
a60bb4a
Compare
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Pierre-Antoine Champin <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
a60bb4a
to
ee9a7ff
Compare
Normative, multiple reviews, changes requested and made, no objections, merging. |
This is the first of several steps to clean up the Credentials Vocabulary. The main changes are:
defined by
tag has been added in the (YAML) vocabulary definition. This is translated into therdfs:isDefinedBy
statement in the formal vocabularies, and a textual translation thereof in the HTML text. Any other text in the property definitions had been removed. Bottom line: the official term definition is the in VCWG specifications, the vocabulary just "refers" to that. Some additional comments:serviceEndpoint
that has no formal definition. The vocabulary has a comment on this, and this must be solved asapJsonSchema2023
class is defined by the schema spec.For reviewers
The PR does not include the visible files, i.e., the vocabulary in HTML/JSON-LD/Turtle. This is because the generation of these files happen automatically by a GitHub action. To make the reviews possible:
vocab/credentials/v2/vocabulary.yml
and, for the text framework in the generated HTML, on thevocab/credentials/v2/template.html
files. (These are the vocabulary generation input files.)Preview | Diff