Skip to content
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

Add section on schema validity #1137

Merged
merged 5 commits into from May 31, 2023
Merged

Add section on schema validity #1137

merged 5 commits into from May 31, 2023

Conversation

decentralgabe
Copy link
Contributor

@decentralgabe decentralgabe commented May 23, 2023

Also updated reference to the FPWD of the Credential Schema 2023 spec


Preview | Diff

common.js Outdated Show resolved Hide resolved

<p>
If the <code>credentialSchema</code> property is available, the schema of a
<a>verifiable credential</a> is expected to be evaluated by the <a>verifier</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a>verifiable credential</a> is expected to be evaluated by the <a>verifier</a>
<a>verifiable credential</a> can be evaluated by the <a>verifier</a>

Copy link
Member

@msporny msporny May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Company representative hat on): Our software will probably not utilize credentialSchema in a significant number of use cases -- the reasoning being that either 1) the issuer wouldn't have issued a VC that goes against the schema they are specifying, or 2) they're doing bad job with their schema management and we don't want credentialSchema mismatches to get in the way of the VC being utilized. IOW, I'm not sure that we're positive or negative on this feature yet and therefore do not want any expectations set on verifier's using this feature. It's fine if it's there, it's fine if it's optional, so the request here is to not set any expectations that the verifier is expected to use the schema property (unless they want to).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my intention was to convey if the property was there it is expected to be evaluated. if the issuer puts it there, they intend for it to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the issuer puts it there and wants to say it's optional - I'm not sure how they convey that. I view the properties in the VCDM as a (loose) contract between an issuer and verifier

Copy link
Member

@msporny msporny May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I failed to realize that the section you added is in the Validation section of the spec. :)

If the intention is to make processing the credentialSchema field mandatory, then we need to discuss that as a WG.

With my company representative hat on, we would be a -1 to mandatory processing of credentialSchema. We believe it should be an option for the verifier to set because it is, ultimately, their decision. The feature should mirror how we do credentialStatus.

With my editor hat on, having language like "is expected to be", when what you really mean is "MUST", is dicey, because people will read it as a MAY instead of a MUST. Even SHOULD is dicey if you are suggesting that organizations will process the field by default.

To provide an analogy, the credentialStatus field processing is optional, the spec doesn't say you MUST process it. It just says what the field is used for and what the field should contain:

https://w3c.github.io/vc-data-model/#status

And then we use the "is expected to be" language in the section on Validation... which is what this PR does (IIUC).

In any case, I'm retracting my change request and merging the PR since I'm the only one that suggested changes. Thanks for the PR @decentralgabe ! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @msporny I'm open to a future change to keep the language more consistent with credentialStatus

index.html Outdated Show resolved Hide resolved
Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on this PR as long as one mandatory change is made: Do not set an expectation that Verifiers will use this property, it's optional. The other changes are minor editorial changes, but ones that need to be made in a consistent way.

Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
@msporny
Copy link
Member

msporny commented May 31, 2023

Editorial, multiple reviews, changes requested (but not made due to change request being retracted), no objections, merging.

@msporny msporny merged commit 2691c58 into main May 31, 2023
1 check passed
@msporny msporny deleted the credential-schema-validity branch May 31, 2023 06:29
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.

None yet

3 participants