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.

Sign up for GitHub

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

Jump to bottom

Vc 2.0 time props #158

Closed
wants to merge 0 commits into from
Closed

Vc 2.0 time props #158

wants to merge 0 commits into from

Conversation

aljones15
Copy link
Contributor

@aljones15 aljones15 commented Nov 28, 2023 •
edited
Loading

Implements VC 2.0 validUntil and validFrom feature and tests.
CI is failing in node 14.

Addresses: #102

@aljones15 aljones15 self-assigned this Nov 28, 2023
JSAssassin
JSAssassin reviewed Nov 28, 2023
View reviewed changes
lib/helpers.js Outdated
@@ -10,7 +10,16 @@ export const {
CONTEXT_URL: CREDENTIALS_CONTEXT_V2_URL
}
} = credentialsContextV2;
// Z and T can be lowercase
// RFC3339 regex
export const dateRegex = new RegExp('^(\\d{4})-(0[1-9]|1[0-2])-' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this match XML Schema 1.1 dateTimeStamp? https://w3c.github.io/vc-data-model/#example-regular-expression-to-detect-a-valid-xml-schema-1-1-part-2-datetimestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The year validation in that RegExp is more strict than mine and it also handles years larger than 4 numbers so that seems like an advantage. it also has a different validator for T or Z so yeah I'll adapt it to this lib today.

Copy link
Contributor

Choose a reason for hiding this comment

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

The VCDM spec says:

To avoid doubt, the regular expression in [XMLSCHEMA11-2] is the normative definition.

Sadly, the XML Schema 1.1 spec lacks a handy copy/paste-able regular expression. I will note that that spec does point out that year is simply an integer--and doesn't denote any limit on number of characters.

At the very least, we should use the regular expression called out in the VCDM spec as example 14. However, even the VCDM calls that out as "minimal":

The regular expression shown below (minus the whitespace included here for readability), is often adequate when processing library-generated dates and times on modern systems.

So...if we can find something more accurate/complete, we should...but lets at least start from what's in the VCDM spec rather than something custom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here: 80d30d7

JSAssassin
JSAssassin reviewed Nov 29, 2023
View reviewed changes
test/10-verify.spec.js Outdated Show resolved Hide resolved
JSAssassin
JSAssassin reviewed Nov 29, 2023
View reviewed changes
test/10-verify.spec.js Outdated Show resolved Hide resolved
JSAssassin
JSAssassin reviewed Nov 29, 2023
View reviewed changes
test/10-verify.spec.js Outdated Show resolved Hide resolved
BigBlueHat
BigBlueHat requested changes Nov 30, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
lib/helpers.js Outdated
@@ -10,7 +10,16 @@ export const {
CONTEXT_URL: CREDENTIALS_CONTEXT_V2_URL
}
} = credentialsContextV2;
// Z and T can be lowercase
// RFC3339 regex
export const dateRegex = new RegExp('^(\\d{4})-(0[1-9]|1[0-2])-' +
Copy link
Contributor

Choose a reason for hiding this comment

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

The VCDM spec says:

To avoid doubt, the regular expression in [XMLSCHEMA11-2] is the normative definition.

Sadly, the XML Schema 1.1 spec lacks a handy copy/paste-able regular expression. I will note that that spec does point out that year is simply an integer--and doesn't denote any limit on number of characters.

At the very least, we should use the regular expression called out in the VCDM spec as example 14. However, even the VCDM calls that out as "minimal":

The regular expression shown below (minus the whitespace included here for readability), is often adequate when processing library-generated dates and times on modern systems.

So...if we can find something more accurate/complete, we should...but lets at least start from what's in the VCDM spec rather than something custom.

lib/helpers.js Outdated Show resolved Hide resolved
test/10-verify.spec.js Outdated Show resolved Hide resolved
aljones15
aljones15 commented Dec 1, 2023
View reviewed changes
lib/helpers.js Outdated Show resolved Hide resolved
@aljones15 aljones15 marked this pull request as ready for review December 1, 2023 20:05
JSAssassin
JSAssassin suggested changes Dec 4, 2023
View reviewed changes
lib/helpers.js Outdated Show resolved Hide resolved
test/10-verify.spec.js Outdated Show resolved Hide resolved
test/20-dateRegex.spec.js Outdated Show resolved Hide resolved
JSAssassin
JSAssassin approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@JSAssassin JSAssassin left a comment

Choose a reason for hiding this comment

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

Thanks!

BigBlueHat
BigBlueHat requested changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@BigBlueHat BigBlueHat left a comment

Choose a reason for hiding this comment

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

Small change to hopefully reduce confusion down the road.

test/10-verify.spec.js Outdated Show resolved Hide resolved
BigBlueHat
BigBlueHat reviewed Dec 4, 2023
View reviewed changes
test/helpers.js Outdated Show resolved Hide resolved
BigBlueHat
BigBlueHat requested changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@BigBlueHat BigBlueHat left a comment

Choose a reason for hiding this comment

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

Not sure about removing the milliseconds. Other folks using JS to generate these date strings won't bother.

BigBlueHat
BigBlueHat approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@BigBlueHat BigBlueHat left a comment

Choose a reason for hiding this comment

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

Works for me.

@JSAssassin
Copy link
Contributor

@aljones15 Is the PR this one is merging into ready for review? I suggest reviewing the other PR before merging this one into it. If it's ready for review, please mark it as such. Thanks.

@aljones15 aljones15 force-pushed the update-vc-2.0 branch 2 times, most recently from fc2a605 to 02c9701 Compare December 7, 2023 14:35
@aljones15 aljones15 closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JSAssassin JSAssassin JSAssassin approved these changes

@BigBlueHat BigBlueHat BigBlueHat approved these changes

Assignees

@aljones15 aljones15

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aljones15 @JSAssassin @BigBlueHat