Address normative references issues #216

decentralgabe · 2023-09-13T10:41:45Z

Address remaining feedback in Pre-CR normative dependencies will trigger a 2nd CR #210 relating to media types, json schema version references, and $id language
Minor formatting updates

msporny

Minor nits, otherwise LGTM, take or leave the comments. There's some errant ref to [JSON-SCHEMA-2020-12] somewhere in the text that you'll want to make into a non-normative reference.

msporny · 2023-09-13T10:51:12Z

index.html

@@ -157,7 +157,7 @@ <h2>Data Model</h2>
            <a>verifiable credential</a>.
         </p>
         <p>
-            Implementers may find use in packaging a JSON Schema as a verifiable credential when they wish to
+            Implementers MAY find use in packaging a JSON Schema as a verifiable credential when they wish to


This normative language is strange, suggest changing it to "might".

Alternatively, you could say: "Implementers MAY package a JSON Schema as a verifiable credential when they..."

thanks, updating

msporny · 2023-09-13T10:52:01Z

index.html

@@ -525,7 +525,7 @@ <h3>Reserved Keywords</h3>
            </p>
            <p>
               In the upcoming sections we list <i>some</i> keywords that possess unique significance and
-               should not be used in conflicting ways.
+               SHOULD NOT be used in conflicting ways.


It's not clear what "conflicting ways" means. Providing an example would help

quite similar to your PR here will adjust the language w3c/vc-data-model#1271

msporny · 2023-09-13T10:53:52Z

index.html

+            The credential containing a <a>credential schema</a> MAY include a proof, either
            embedded according to <a data-cite="VC-DATA-MODEL-2.0/#securing-verifiable-credentials">
            Securing Verifiable Credentials</a>.


You say "either embedded" but only provide one option, I would expect there to be two options to pick from. You might be saying "MAY be secured by using either an embedded proof or an external proof as defined in..."?

thanks good find!

index.html

msporny · 2023-09-13T10:55:52Z

index.html

-                  representation of a [[JSON-SCHEMA]], defined by [[[OPENAPIS-3.1.0]]], the types <code>application/openapi+yaml</code>
-                  or <code>application/yaml</code> may be used.
+                  representation of a [[JSON-SCHEMA]], defined by [[[OPENAPIS-3.1.0]]], the types 
+                  <code>application/openapi+yaml</code> or <code>application/yaml</code> MAY be used.


I expect this to lead to interop challenges... this means that implementations would probably have to support both JSON and YAML schemas, and folks might complain. Just raising it as a concern -- I don't think it'll result in any issues in CR (just broader ecosystem issues when it's deployed).

I agree this is a risk. tagging @OR13 since he is the one that really cares about this 😄

@msporny I think its fine to use yaml serialization for JSON Schema... https://github.com/transmute-industries/vc-json-schema

#!/bin/bash JWT=$(cat ./example/vc.jwt) jq -R 'split(".") | .[1] | @base64d | fromjson' <<< "$JWT" > ./example/vc.json echo "🌴 Validation" ajv --spec=draft2020 compile -c ./customKeywords.js -s ./example/NewCredentialType.yaml ajv --spec=draft2020 validate -c ./customKeywords.js -s ./example/NewCredentialType.yaml -d ./example/vc.json ajv --spec=draft2020 test -c ./customKeywords.js -s ./example/NewCredentialType.yaml -d ./example/vc.json --valid curl -s https://transmute-industries.github.io/vc-json-schema/example/NewCredentialType.yaml > schema.yaml curl -s https://transmute-industries.github.io/vc-json-schema/example/vc.jwt > vc.jwt JWT=$(cat ./vc.jwt) jq -R 'split(".") | .[1] | @base64d | fromjson' <<< "$JWT" > ./vc.json ajv --spec=draft2020 test -c ./customKeywords.js -s ./schema.yaml -d ./vc.json --valid

I'm happy to write the tests for this, but they are not required because its not a MUST iirc.

index.html

Co-authored-by: Manu Sporny <[email protected]>

andresuribe87

Some nits that I believe make things more clear.

index.html

andresuribe87 · 2023-09-13T12:36:48Z

index.html

+                  When transmitting a [[JSON-SCHEMA]] represented as a <a>verifiable credential</a> with usage of
+                  the <a href="#jsonschema">JsonSchemaCredential</a> type, the media types <code>application/vc+ld+json</code>,
+                  <code>application/vc+ld+json+jwt</code>, and <code>application/vc+ld+json+sd-jwt</code> as defined
+                  in [[VC-DATA-MODEL-2.0]], [[VC-JOSE-COSE]], and [[SD-JWT]] specifications, respectively, SHOULD be used. 


Suggested change

When transmitting a [[JSON-SCHEMA]] represented as a <a>verifiable credential</a> with usage of

the <a href="#jsonschema">JsonSchemaCredential</a> type, the media types <code>application/vc+ld+json</code>,

<code>application/vc+ld+json+jwt</code>, and <code>application/vc+ld+json+sd-jwt</code> as defined

in [[VC-DATA-MODEL-2.0]], [[VC-JOSE-COSE]], and [[SD-JWT]] specifications, respectively, SHOULD be used.

The media types <code>application/vc+ld+json</code>,

<code>application/vc+ld+json+jwt</code>, or <code>application/vc+ld+json+sd-jwt</code> as defined

in [[VC-DATA-MODEL-2.0]], [[VC-JOSE-COSE]], or [[SD-JWT]] specifications, respectively, SHOULD be used when transmitting a [[JSON-SCHEMA]] represented as a <a>verifiable credential</a> with usage of

the <a href="#jsonschema">JsonSchemaCredential</a> type

index.html

decentralgabe · 2023-09-13T12:39:35Z

JSON-SCHEMA-2020-12

this is the one we want to normatively reference since it's required

Co-authored-by: Andres Uribe <[email protected]>

decentralgabe · 2023-09-14T13:57:53Z

Will merge tomorrow.

iherman · 2023-09-15T05:55:57Z

The issue was discussed in a meeting on 2023-09-14

no resolutions were taken

View the transcript

4.1. Address normative references issues (pr vc-json-schema#216)

See github pull request vc-json-schema#216.

Brent Zundel: PR 216 - can you walk us through this, Gabe?

Gabe Cohen: Manu had gone through normative references, suggest reducing some JSON Schema references... we only want to normatively refer to the most recent one. Make other refs non-normative, cleans up some other language.

Andres Uribe: manu: Went through the spec, looks good.

Andres Uribe: ... as to the changes suggested in the PR.

Andres Uribe: ... The goal here is to reduce the number of normative references to ease going into CR. Happy with it.

Gabe Cohen: Open to other questions on the spec, other than that, looking forward to proposal.

address issue comments

562154c

decentralgabe requested review from OR13 and andresuribe87 as code owners September 13, 2023 10:41

decentralgabe requested a review from msporny September 13, 2023 10:41

decentralgabe mentioned this pull request Sep 13, 2023

Pre-CR normative dependencies will trigger a 2nd CR #210

Closed

msporny approved these changes Sep 13, 2023

View reviewed changes

msporny requested review from davidlehn, dlongley, TallTed, brentzundel and Sakurann September 13, 2023 10:57

decentralgabe and others added 2 commits September 13, 2023 05:58

Apply suggestions from code review

c94fb2d

Co-authored-by: Manu Sporny <[email protected]>

pr comments

26d4548

andresuribe87 approved these changes Sep 13, 2023

View reviewed changes

decentralgabe and others added 2 commits September 13, 2023 07:40

Apply suggestions from code review

a5bc7fd

Co-authored-by: Andres Uribe <[email protected]>

formatting

91dad89

decentralgabe linked an issue Sep 14, 2023 that may be closed by this pull request

Pre-CR normative dependencies will trigger a 2nd CR #210

Closed

decentralgabe merged commit b1e8cac into main Sep 15, 2023

decentralgabe deleted the issue-210-take2 branch September 15, 2023 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address normative references issues #216

Address normative references issues #216

decentralgabe commented Sep 13, 2023 •

edited by pr-preview bot

Loading

msporny left a comment •

edited

Loading

msporny Sep 13, 2023

decentralgabe Sep 13, 2023

msporny Sep 13, 2023

decentralgabe Sep 13, 2023

msporny Sep 13, 2023

decentralgabe Sep 13, 2023

msporny Sep 13, 2023

decentralgabe Sep 13, 2023

OR13 Sep 13, 2023

OR13 Sep 13, 2023

andresuribe87 left a comment

andresuribe87 Sep 13, 2023

decentralgabe commented Sep 13, 2023

decentralgabe commented Sep 14, 2023

iherman commented Sep 15, 2023

4.1. Address normative references issues (pr vc-json-schema#216)

Address normative references issues #216

Address normative references issues #216

Conversation

decentralgabe commented Sep 13, 2023 • edited by pr-preview bot Loading

msporny left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresuribe87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentralgabe commented Sep 13, 2023

decentralgabe commented Sep 14, 2023

iherman commented Sep 15, 2023

4.1. Address normative references issues (pr vc-json-schema#216)

decentralgabe commented Sep 13, 2023 •

edited by pr-preview bot

Loading

msporny left a comment •

edited

Loading