-
Notifications
You must be signed in to change notification settings - Fork 112
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
Improve the explanation of using graphs in VCDM #1326
Conversation
* main: Remove unused references and vendor-specific protocols.
I marked it as post-CR, insofar as the PR does not introduce any normative change, just tightens up the specification (e.g., for the usage of the I leave the decision to the chairs. @brentzundel @Sakurann |
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.
A few suggestions, then I'm supportive. Thanks!
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
I am concerned that neither of the things now called "the default graph" in the new diagrams actually is "the default graph" in any scenario. If anything, I would expect each of these instead to be another "named graph". (In at least one RDF quadstore, the At minimum, I think some discussion of how and why these come to be "the default graph" here is necessary, certainly in the text neighboring, and possibly within, each diagram. Possibly the |
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'll have more, but the direction I go in from here will depend on the outcome of the comments already made, so I'll wait for the more...
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Thanks for your comments, @TallTed. As for:
I use the term "default graph" in the way it is defined in the RDF 1.1 concepts spec. This is in line with the way the term is used in JSON-LD 1.1 or N-Quads (both are relevant to us). The succinct way N-Quads describes the quads in a default graph is (when defining a simple quad term): I'm afraid the usage of the term "default graph" in that quadstore may not be aligned with the RDF specs, but I cannot judge it.
I've not brought in anything new. I just describe, in more precise terms, what is already implicitly defined by the spec. E.g., let us look at Example 5 in the spec, which is the case of a "simple" credential (i.e., no presentation). The same dataset in N-Quads can be seen on the JSON-LD playground example. The terms that are part of the "core" of the credentials are all quads without a graph label, i.e., part of the default graph of the dataset per the N-Quads specification.
I don't think so. As the aforementioned playground example shows, those quads are different; there is also a real named graph in that example (the proof graph), and the quads all have an explicit graph label B.t.w., to make things complete and coherent I did make some changes in other places of the spec making use of the default graph/named graph terminology. See, for example, the section on verifiable credential graphs or, primarily, the definition of the I believe the real goal of all this PR is the issue around the |
Perhaps there is a way to qualify "the default graph" so it doesn't appear to be "the only default graph", but rather is "the default graph of this dataset"? I will think further on this. I raise the question here so others who might have good ideas might voice them. I realized that I mis-typed above, where I said '(In at least one RDF quadstore, the |
I take your point, and I am happy to change this. The problem is that "the default graph of this dataset" is a bit of a mouthful. "Dataset default graph"? "Local default graph"? (I am not even sure the VCDM spec uses the term "dataset", so we may not want to go there. Hence the "local".) |
Verifiable Credentials Specifications Directory [[?VC-SPECS]]. | ||
<p> | ||
The <a>verifiable presentation</a> MAY include a <code>proof</code> | ||
<a>property</a>, which refers to a separate <a>named graph</a> containing a single proof. |
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.
my understanding is that multiple proofs are allowed here, and that in the case of BBS, the secure these information graphs in very different ways.
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.
@OR13 I have no opinion on this, and I would prefer if this was raised and discussed as a separate issue. The text in this version simply reused what is currently in the official draft.
Could you raise a separate issue and mark this conversation as resolved?
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.
Just a comment - not blocking - it would be great to have a similar set of graphics/text that highlight a graph VC with a vc-jose-cose based proof
mechanism, such as those listed in the | ||
Verifiable Credentials Specifications Directory [[?VC-SPECS]]. | ||
<p> | ||
The <a>verifiable presentation</a> MAY include a <code>proof</code> |
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 feel that proof remains a property of data integrity, and would prefer to see normative guidance related to data integrity done in the data integrity specification.
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.
@OR13 similar comment as above: I've just reused the current draft, and I do not have a strong opinion either way. Would it be possible to move this into a separate issue and not hold up this one on this?
@decentralgabe I agree with that, and I am also happy looking into this, but I would prefer to do that once this PR is merged. Truth must be said, I am not really deeply familiar with the JWT world, so I do not know whether there is an accepted "style" of diagrams. Do you know? |
@iherman no problem if you want to take a shot at it after this PR is merged that would be greatly appreciated! Perhaps @selfissued knows if there's prior art on such diagrams, if not, I think a similar form to what you've already done would work nicely. |
The issue was discussed in a meeting on 2023-11-01
View the transcript1.2. Improve the explanation of using graphs in VCDM (pr vc-data-model#1326)See github pull request vc-data-model#1326. Ivan Herman: There is one PR 1326 last week that is labeled as Post-CR, but I wonder whether this is not a Pre-CR PR. It does include a more precise and normative definition on which part of a credential representation the proof property applies to. Brent Zundel: I am happy to make that change. Ivan Herman: Maybe Dave Manu and Ted have already looked at it. Manu Sporny: The PR needs to be re-based. There are a whole bunch of changes which makes it difficult to review. Ivan Herman: I put a separate preview into the instance. Manu Sporny: I can try to rebase to see which normative language is modified. Ivan Herman: Extremely grateful if you did that. Manu Sporny: Do you which normative text? Ivan Herman: There is a terminology change and the definition of the Manu Sporny: we need to deal with this pre-CR. Ivan Herman: As an answer to Henry raised issue. So that issue can be closed if this PR gets accepted. Brent Zundel: I will change from Post-CR to Pre-CR. Only outstanding requests for changes are from Ted. Welcome to speak on changes. Ted Thibodeau Jr.: My concern is the reference to the default graph. Its used for the first time in this document. I don't think it has the meaning intended. Ivan Herman: I think we do disagree on that. Brent Zundel: This conversation can continue in the PR. Manu has agreed to rebase and cleanup. |
Normative, multiple reviews, changes requested and made, no objections, merging. |
This PR is a replacement for #1321, following up on #1321 (comment). Globally, it takes a more thorough approach and also attempts to provide a more complete solution to #1248 and #1318.
What it does is:
proof
property has now a proper definition of what is being proven, i.e., which claims; this is done by using the graph terminology.All this required some more, relatively minor changes on the text for a better coherence. The most visible change is that what used to be §4.11.1 Verifiable Credential Graphs has been moved to the top level of the Basic Concepts section, and is now known as §4.9 (with some minor changes).
(As an aside, the PR also puts the sources of the new versions of Figures 6 and 8 to the repository, using the drawio format. Better have all these at the same place.)
For reviewers: the automatic preview below does not work properly because it does not process included files (e.g., the terminology section or the diagrams). You can use these pointers instead:
Preview | Diff