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

Replace "schema" with more explicit "type" approach #258

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

JaceHensley
Copy link
Collaborator

@JaceHensley JaceHensley commented Oct 4, 2021

Closes #256
Closes #213
Closes #250
Closes #217

These two constraints will be the most used of all the constraints so they should be first
"const": "https://yourwatchful.gov/drivers-license-schema.json"
}
}
],
"fields": [
{
"path": ["$.expirationDate"],
"filter": {
"type": "string",
"format": "date-time",
Copy link
Member

Choose a reason for hiding this comment

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

notably, the newest version of json schema removes the format property. this may be considered in PEv2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting could you send me a link to that? I hadn't heard that format was going away, what is replacing it?

"path": ["$.credentialSchema.id", "$.vc.credentialSchema.id"],
"filter": {
"type": "string",
"const": "hub://did:foo:123/Collections/schema.us.gov/passport.json"
Copy link
Member

Choose a reason for hiding this comment

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

is there an example that shows OR logic?

{
"path": ["$.credentialSchema", "$.vc.credentialSchema"],
"filter": {
"allOf": [
Copy link
Member

Choose a reason for hiding this comment

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

I see it here

decentralgabe
decentralgabe previously approved these changes Oct 5, 2021
Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

thanks for doing this!

spec/spec.md Outdated
- The _constraints objext_ ****MAY**** contain a `types` property. If present it's value ****MUST**** be an array of objects as composed as follows:
- The _types object_ ****MUST**** contains a `path` property. The value of this property has the same requirements as the _fields object_'s `path` property
- The _types object_ ****MUST**** contain a `filter` property. The value of this property has the same requirements as the _fields object_'s `filter` property
- The _types object_ ****MAY**** contain an `id` property. The value of this property has the same requirements as the _fields object_'s `id` property
Copy link
Member

Choose a reason for hiding this comment

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

The reason the fields object has an id property is because it is referred to elsewhere in the spec. Is there a similar requirement here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah that makes sense why id is on fields, I don't see a similar constraint for the types. Will remove

- The _types object_ ****MUST**** contains a `path` property. The value of this property has the same requirements as the _fields object_'s `path` property
- The _types object_ ****MUST**** contain a `filter` property. The value of this property has the same requirements as the _fields object_'s `filter` property
- The _types object_ ****MAY**** contain an `id` property. The value of this property has the same requirements as the _fields object_'s `id` property
- The _types object_ ****MAY**** contain an `purpose` property. The value of this property has the same requirements as the _fields object_'s `purpose` property
Copy link
Member

Choose a reason for hiding this comment

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

I also wonder if the purpose property is necessary here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do think the purpose field is important, to allow wallets to have a human readable sentence for why the requester is specifying this type

"constraints": {
"limit_disclosure": "required",
"types": [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could also be done with multiple "type objects"in the types array

@kimdhamilton kimdhamilton self-requested a review October 7, 2021 17:19
kimdhamilton
kimdhamilton previously approved these changes Oct 7, 2021
brentzundel
brentzundel previously approved these changes Oct 7, 2021
@kimdhamilton kimdhamilton self-requested a review October 7, 2021 17:22
@JaceHensley JaceHensley merged commit 0015702 into master Oct 7, 2021
@JaceHensley JaceHensley deleted the feat/replace-schema-with-type/dev branch October 7, 2021 17:22
@JaceHensley JaceHensley mentioned this pull request Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants