decentralized-identity / presentation-exchange

Specification that codifies an inter-related pair of data formats for defining proof presentations (Presentation Definition) and subsequent proof submissions (Presentation Submission)
https://identity.foundation/presentation-exchange
Apache License 2.0
86 stars 37 forks source link

Fix spec references & two examples #311

Closed decentralgabe closed 2 years ago

decentralgabe commented 2 years ago

examples were using formatMinimum instead of the specified minimum

decentralgabe commented 2 years ago

it appears the tests fail because minimum expects a number. I believe this is a problem with the test...

JaceHensley commented 2 years ago

minimum/maximum is only for numbers, https://github.com/ajv-validator/ajv-formats#keywords-to-compare-values-formatmaximum--formatminimum-and-formatexclusivemaximum--formatexclusiveminimum

decentralgabe commented 2 years ago

Ok...upon further investigation:

formatMaximum and formatMinimum are not defined in the Draft 7 specs:

There is a specific JS library that supports these additional fields...but it's not in the spec. So neither my change, nor the spec are correct and we probably need non-normative language to support non-integer min/max validation.

Edit: opened https://github.com/decentralized-identity/presentation-exchange/issues/312

decentralgabe commented 2 years ago

@JaceHensley that library uses a non-normative json schema extension to define these properties...which isn't widely supported across json schema implementations

JaceHensley commented 2 years ago

yeah I noticed that this week, I could've sworn I saw formatMinimum on https://json-schema.org/ previously but when I went back to look at it again I couldn't find it.

csuwildcat commented 2 years ago

What is formatMinimum anyway?

JaceHensley commented 2 years ago

It's a minimum, but the the comparison is based on the JSON schema format (date, date-time, uri, uuid, ip, etc.)

bumblefudge commented 2 years ago

@decentralgabe plz re-work, examples fixed in separate PR merged just now on call!

decentralgabe commented 2 years ago

thanks @bumblefudge minor fixes - updated!