-
Notifications
You must be signed in to change notification settings - Fork 61
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
Make DataIntegrityProof created field optional #576
Conversation
@@ -13,8 +13,8 @@ pub struct ProofOptions<M, T> { | |||
pub context: Option<ssi_json_ld::syntax::Context>, | |||
|
|||
/// Date a creation of the proof. | |||
#[serde(default = "xsd_types::DateTime::now")] | |||
pub created: xsd_types::DateTime, | |||
#[serde(default = "created_default", skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the serde(default)
attribute. I know it was already there, but I don't think it makes much sense to put a default value if its not there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes definitely!
#[serde(default = "xsd_types::DateTime::now")] | ||
pub created: xsd_types::DateTime, | ||
#[serde(default = "created_default", skip_serializing_if = "Option::is_none")] | ||
pub created: Option<xsd_types::DateTime>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the spec also specifies that the type of created
should be an XSD dateTimeStamp
and not dateTime
(which has an optional timezone so it can be ambiguous). But maybe that's for another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't break anything so I'm doing it as part of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, VCDM 1.1 has issuanceDate
as dateTime
and we need to convert it to dateTimeStamp
in some tests but I don't see a straightforward way to do that (I don't think we can assume UTC when there's no offset).
According to https://www.w3.org/TR/vc-data-integrity/#proofs it should be optional (and some VC API tests fail because of it). I didn't change any of the functions that require
created
because it seems like we should enforce having acreated
field for some cryptosuites (like EdDSA where the language only defines the type ofcreated
but doesn't explicitely say that it should be present). But happy to change the API to be more tidy