Removing proofs from examples #817
Conversation
* Add PR review process for 2021. * Avoid GitHub id auto-linking. Co-authored-by: David I. Lehn <[email protected]> * Update README.md Co-authored-by: Manu Sporny <[email protected]> Co-authored-by: Manu Sporny <[email protected]> Co-authored-by: David I. Lehn <[email protected]> Co-authored-by: Brent Zundel <[email protected]>
* Update presentation-graph.svg * SVG coding changes - make text into actual text - use stylesheet - simplify source for legibility - Order similar to the flow, for accessibility - Make credential-graph.svg a subsection, and note with a comment. - Add slight background colouring to further delineate each subsection * label the claim As per comment from @TallTed * differentiate graph labels change font characteristics for graph-labels to distinguish them (and make the stylesheet more explicitly obvious) * Add desc (text alternative) in diagrams This makes credentialgraph.svg and presentation-graph.svg more accessible as standalone diagrams. * review comments
| use type information to determine whether or not a provided (verifiable) | ||
| <a>credential</a> or (verifiable) <a>presentation</a> is appropriate. |
There was a problem hiding this comment.
| use type information to determine whether or not a provided (verifiable) | |
| <a>credential</a> or (verifiable) <a>presentation</a> is appropriate. | |
| use type information to determine whether or not a provided <a>credential</a>, | |
| <a>verifiable credential</a>, <a>presentation</a>, or <a>verifiable | |
| presentation</a> is appropriate. |
| @@ -1499,7 +1496,7 @@ <h3>Credential Subject</h3> | |||
| <dd> | |||
| The value of the <code>credentialSubject</code> <a>property</a> is defined as | |||
| a set of objects that contain one or more properties that are each related to a | |||
| <a>subject</a> of the <a>verifiable credential</a>. Each object MAY contain an | |||
| <a>subject</a> of the (verifiable) <a>credential</a>. Each object MAY contain an | |||
There was a problem hiding this comment.
| <a>subject</a> of the (verifiable) <a>credential</a>. Each object MAY contain an | |
| <a>subject</a> of the <a>credential</a> or <a>verifiable credential</a>. Each | |
| object MAY contain an |
There was a problem hiding this comment.
Ok accepted. But please note that the current recommendation is inconsistent throughout the entire text in that sometimes it refers to credential and sometimes to verifiable credential, almost randomly it would seem.
There was a problem hiding this comment.
I guess we have some careful review to make, to ensure that all of these correctly refer to one or the other or both, possibly with additional text to explain why any given instance is one or the other or both.
There was a problem hiding this comment.
Agreed. But its not a simple quick task. Because the original text was crafted over several years by different authors, with multiple edits of different sections as well, then many inconsistencies have crept in. I used the shorthand (verifiable)credential to imply that both credential and verifiable credential are equally applicable in this sentence.
There was a problem hiding this comment.
I understand the shorthand, but many readers may not, and shorthand should generally be avoided in technical docs such as this, unless the intended meaning of that shorthand is clearly defined and every instance of the shorthand links back to that definition.
There was a problem hiding this comment.
Perhaps introducing this shorthand into the definitions section might be advantageous. It would make subsequent editing simpler, and it would resolve the "and" or "or" grammatical issue below :-)
There was a problem hiding this comment.
The immediate concern I have with that is making sure the definitions tools can handle the parenthetical <a>(verifiable) credential</a> differently than <a>verifiable credential</a>. I don't know whether they would recognize that difference today.
The definitely available way to handle this is the way I've already been aiming toward -- i.e., <a>verifiable credential</a> and/or <a>credential</a> or <a>credential</a> and/or <a>verifiable credential</a>.
| It is possible to express information related to multiple <a>subjects</a> in a (verifiable) | ||
| <a>credential</a>. The example below specifies two <a>subjects</a> |
There was a problem hiding this comment.
| It is possible to express information related to multiple <a>subjects</a> in a (verifiable) | |
| <a>credential</a>. The example below specifies two <a>subjects</a> | |
| It is possible to express information related to multiple <a>subjects</a> in a | |
| <a>credential</a> or <a>verifiable credential</a>. The example below specifies | |
| two <a>subjects</a> |
| @@ -1562,7 +1557,7 @@ <h3>Issuer</h3> | |||
| </p> | |||
|
|
|||
| <p> | |||
| A <a>verifiable credential</a> MUST have an <code>issuer</code> <a>property</a>. | |||
| A <a>credential</a> MUST have an <code>issuer</code> <a>property</a>. | |||
There was a problem hiding this comment.
| A <a>credential</a> MUST have an <code>issuer</code> <a>property</a>. | |
| A <a>credential</a> or <a>verifiable credential</a> MUST have an | |
| <code>issuer</code> <a>property</a>. |
There was a problem hiding this comment.
Rejected. A VC that is produced using JWT proofs does not have an issuer property as it has been replaced by the iss claim. This is the reason for specifically excluding verifiable credential in this text.
There was a problem hiding this comment.
I see. I think that this JWT-specific lack should be noted explicitly. Something like --
| A <a>credential</a> MUST have an <code>issuer</code> <a>property</a>. | |
| Each <a>credential</a> or <a>verifiable credential</a> MUST have an | |
| <code>issuer</code> <a>property</a> or equivalent, such as the | |
| `iss` claim in a JWT-based <a>verifiable credential</a>. |
There was a problem hiding this comment.
If you want to add verifiable credential then it should be "and" and not "or"
There was a problem hiding this comment.
No, it should be <a>credential</a> or <a>verifiable credential</a>, because any given instance will only be one or the other, because we've somehow tacitly agreed that <a>credential</a> means <a>unverifiable credential</a> (rather than <a>credentials</a> being the generic that includes all <a>verifiable credentials</a> and <a>unverifiable credentials</a>).
Even if that were not the case, A <a>credential</a> and <a>verifiable credential</a> MUST ... makes no sense.
There was a problem hiding this comment.
I think we will just have to agree to differ on this. Perhaps US and English grammar are different. But to me, "A or B must do something" means that only one of them must do it, whilst "Both A and B must do something" means that both must do it.
There was a problem hiding this comment.
I've edited my suggestion, changing the leading A to Each which I think makes the sentence mean the same in both UK and US English, even with the or, which I continue to think is appropriate here, because each document in question is one or the other, not one and the other.
There was a problem hiding this comment.
Yes this makes sense because each means we are talking about a singular object.
| } | ||
| </pre> | ||
|
|
||
| <p> | ||
| In the above <a>verifiable credential</a> the <a>issuer</a> is claiming that | ||
| In the above <a>credential</a> the <a>issuer</a> is claiming that |
There was a problem hiding this comment.
| In the above <a>credential</a> the <a>issuer</a> is claiming that | |
| In the above <a>credential</a>, the <a>issuer</a> is claiming that |
There was a problem hiding this comment.
Note that there are buttons on each suggested change, to "Commit suggestion" or "Add suggestion to batch", the latter of which leads to a subsequent "Commit batch", which avoid the need for "Ok accepted" comments and should make applying such suggestions faster and easier.
There was a problem hiding this comment.
Because I am a git novice, I found whilst updating my previous PR (808) that committing changes on the web site messed up my local GitHub copy and I could no longer push a revised version from my PC to the web, as they had become out of sync. So I prefer to make all the changes on my local GitHub copy then push the revised version up to web.
There was a problem hiding this comment.
I know there's a way around that issue. I can't tell you how to access it because I don't use local git at all; all my interactions are through GitHub's web interface. :-/
There was a problem hiding this comment.
If you like how everything reads on your local branch the simple answer is to force push to the remote branch. This can be done with something like the following:
git push --force origin Removing-Proof
This is effectively overwriting any changes that exists on github to match the git log on your local computer. This is a bit of a double edged sword in that it could overwrite things you thought were supposed to be there that only existed on github but not locally. If you get stuck you can ping me and I can help you figure out how to untangle it as well.
There was a problem hiding this comment.
If we could spend a few minutes at the next VCWG meeting to discuss how best to author PRs for novices I would appreciate it (web vs desktop vc command line etc.) Ease of use being the primary factor :-)
There was a problem hiding this comment.
[@David-Chadwick] If we could spend a few minutes at the next VCWG meeting to discuss how best to author PRs for novices I would appreciate it (web vs desktop vc command line etc.) Ease of use being the primary factor :-)
Best to reply to the meeting's AGENDA email, or email the chairs & editors directly, with this request.
(My personal answer is to use the GitHub interface as much as possible, as that seems to have the fewest foot-guns, and really is set up pretty well for non-coders and coders alike. I have only rarely had to appeal to someone else to use the command-line tools to achieve something I couldn't do easily though the website, and those times were mostly about global search-and-replace of a few typos that had permeated a repo.)
There was a problem hiding this comment.
If we're unable to get to this, I'm happy to find a time that we could cover some of the basics in a tutorial that we post in the CCG as a recording since I think it's useful for some of the initiatives they're doing to be more inclusive as well.
kdenhartog
added
editorial
|
I see that there's some (editorial) changes that are being suggested which further clarify some normative statements. In this case, I'm not expecting it to be a breaking change on the test suite or implementations so I'm leaning towards this being a non-substantive change and classified as "V1.1". If these changes do go in with the additional language that includes a normative statement I could actually see this being bumped to a "V1.2" change - which isn't a big deal still addressable in the purview of the maintenance WG. |
There was a problem hiding this comment.
Strong -1 to the concept of this PR. @David-Chadwick introduced a PR to differentiate between a credential and a verifiable credential, and is now removing the "verifiable" bit from all examples. This is highly problematic for a specification that is supposed to be about Verifiable things (that have proofs). The reason we included the proofs in the first place is because of the "verifiable" bit. Please put the spec back to the way it was... this switching between "credential" and "verifiable credential" is confusing if we go further than the small change @David-Chadwick made earlier. This PR will harm the understandability of the specification.
|
I strongly disagree with @msporny. When we wrote the spec originally we all believed that all VCs would have a proof statement, so it was perfectly sensible to include proof .... in all examples (since we did not know exactly what the proofs would contain, and they would be variable). Then, after the JWT proofing text was created, very late in the day if you remember, we had a discussion about what the proof statement should contain for JWT VCs. I argued that it should still be there indicating that the proof was an external JWT, but @msporny argued strongly that it should not be there and so it was removed. This now breaks all the examples. Consequently we should remove proofs from all the examples, since a) it is incorrect (since not all VCs contain proofs) and b) it biases the specification towards VCs that do contain embedded proofs as it wrongly implies that all VCs should have a proof. One solution would be to define (in v1.2) a proof for JWTs along the lines of "proof":{"type":"JWT:} and then all the examples would be correct again. But keeping proofs in all the current examples is fundamentally wrong. |
|
The issue was discussed in a meeting on 2021-09-15
View the transcript3.3. Removing proofs from examples (pr vc-data-model#817)See github pull request #817. Brent Zundel: 817. Removing proofs from examples. Manu Sporny: I would like David to intro - but I have to go in two minutes. Real quick... I think it's problematic to remove the proof property from all examples. David Chadwick: I've put my rationale in the PR.
David Chadwick: We didn't know what it would like, so we put
David Chadwick: That makes all the examples invalid if you are talking about verifiable credentials in general. Brent Zundel: To rephrase what Manu was saying, I don't think he was disagreeing with you in principle... just may need more deliberation rather than wholesale removal of all of them.
David Chadwick: I can go along with that if the majority...
Kyle Den Hartog: What i saw come from Michael Herman that I thought was reasonable was the ability to do multiple credentials - with a tab format David Chadwick: I think that would be great, but a lot of work... And the ZKP people may ask for a tab.
Kyle Den Hartog: Yeah, in my opinion, ZKPs would be reasonable since we have sections to describe it. mDLs may be out of scope, as no one has been able to align the two data models to show compliance at this point.
Brent Zundel: Conversation can continue in the PR. |
|
The issue was discussed in a meeting on 2021-09-29
View the transcript1.2. Removing proofs from examples (pr vc-data-model#817)See github pull request #817. David Chadwick: This is all in the issue request... basically, when we rolled the standard initially, every VC would have a proof property, very late in the day that JWT was specified, discussion in the group. Manu argued that proof should be removed for JWT, I argued that it should have some content. Manu's argument was accepted. Regardless of proof mechanism, you start off w/ a credential, you turn it into a VC, they should be identical irrespective of proof mechanism you use. Manu Sporny: My position hasn't changed, strong objection to removing all the proof stuff in the spec. I do agree with David and the basic framing of what we'd like to do. But mass-deleting every instance of proof I think would potentially send the wrong message. It's a big change for a v1.1 spec too. I'd like more people involved in what's the best way to do it.
David Chadwick: While the number of lines that are changed are significant, the changes are all absolutely identical, not a technically difficult change to comprehend, once you understand one of them, you understand all of them. Michael Herman: So, I'm going to link in issue 815, in addition to JSON-LD, having non-LD examples as well, this is going to result in broad changes, auxillary example for all JSON-LD examples.
Dave Longley: Something, IIRC, someone was going to look into using the tabs approach for examples, so this PR would become "add tabbed examples", adding JWT as additional examples. Manu Sporny: Yes, agree with Dave that that is probably the right approach. I'm objecting to the construction of the PR, not the idea behind it. Other specs have dealt with this issue by providing tabbed examples. The right approach is probably 1 tab with the credential itself, 1 with the LD proof VC, and 1 with the JWT VC. Then people can switch between the tabs and examples and see what everything looks like.
David Chadwick: I think that is an excellent suggestion, the only issue that i have is that I'm not technically competent enough with the software to do the tabbing
Wayne Chang: Ok, that can be done Charles Lehner: I like tabbed idea as well, have proof object that refers to have JWT, has that been discussed. Ivan Herman: On the practical side, the JSON-LD spec has such an example, so that may be a distilled mechanism there.
Manu Sporny: I'm happy to open a PR with the tabbed approach, but we should be aware that we may be reopening a can of worms and a whole discussion about the various types of proofs and so on.
Michael Herman: My suggestion, following on to what Manu said, we need a small number of canonical examples in the spec... JSON, JSON-LD, and some others. David Chadwick: I think it'll be less than 3-4, we really only have JSON-LD and JWT, I don't think we have ZKP, I think we'd only have two tabs now. Wayne Chang: Sounds like we're all agreed that way to go is tabbed approach. Michael Herman: How do we decide on the short list? Wayne Chang: The spec as is says you can use external proofs or embedded... Michael Herman: Hoping there would be a plain 'ol JSON example. Wayne Chang: As DavidC noted in the PR, there is such a credential definition ... perhaps that is an example that can be listed. Michael Herman: Can we make a proposal now that plain 'ol JSON is one, JSON-LD is another, third is JWT. Wayne Chang: I think that sounds like a good path, don't need to vote on it.
Manu Sporny: agree
|
I agree its confusing, the only thing that is worse, is using the same word for both. We clearly needed to define a word for the "input format" that is "signed or cryptographically protected with a proof" and the "output format" which contains that proof. In the code (in various places), I have seen this general pattern: credential -> issue -> verifiable credential presentation -> prove -> verifiable presentation issue and prove are problematic words, but in some ways they are better than 'create'.... perhaps we should have generalized them to the concept of |
|
I find it hard to review the PR as is, it seems to have a number of issues mixed, I don't feel comfortable approving as is, but I would like to review focused changes attempting the disambiguation. |
|
I am happy with keyword addproof to convert from a credential to verifiable credential. We could also define the reverse |
|
Resolved the merge conflicts here in line with the intent of this PR (there were two conflicts in examples where the proof property being removed and I removed them in line with this PR). We've got an issue filed (#811) as well to track this work, so I'll be removing the Errata label from the PR. Also, it should be noted that there's a discrepancy between the issue which has been labelled as a deferral to V2 during discussions at TPAC where as this issue has been labelled as a V1.1 editorial issue. More details on that can be found in the issue linked above. |
|
The issue was discussed in a meeting on 2021-11-10
View the transcript3.3. Removing proofs from examples (pr vc-data-model#817)See github pull request vc-data-model#817. Manu Sporny: I am addressing 817 by adding different options to the examples, to show the credential, JWT VC, and JSON_LD proofs. (Manu shows the alternative tabs on a shared screen). David Chadwick: This is brilliant Manu. Well done.
Ivan Herman: this is great. Please continue working on this. Manu Sporny: yes, this is a plugin that can be used in any spec.
David Chadwick: This is great, thank you -- once this PR is done, we can close the other PR.. |
|
This PR has been superceded by #834 so it can be closed |
Preview | Diff