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

VC 2.0 Release Canidate #148

Draft
wants to merge 103 commits into
base: main
Choose a base branch
from
Draft

VC 2.0 Release Canidate #148

wants to merge 103 commits into from

Conversation

aljones15
Copy link
Contributor

@aljones15 aljones15 commented Jul 24, 2023

Features:

  1. Issues and Verifies VCs using the 1.0 and 2.0 VC Data Models
  2. Has integration tests for VCs using both models
  3. Has validators for validUntil and validFrom
  4. Updates the timestamp validator to use the XML standard from the spec
  5. includes an expanded suite with timestamp tests

NOTE:
new features in the VC 2.0 release should branch from this PR unless they build on another feature in a sister PR.

TODO:

  • add or borrow logic for validUntil (PR approved for this)
  • add or borrow logic for validFrom (PR approved for this)
  • Merge validUntil and validFrom feature into this branch.
  • Solve mixed context issues in Verifiable Presentations Mixing VC Contexts in VPs causes VPs to throw #169
  • check against spec revision history
  • use release of VC 2.0 context
  • Do we check credentialSchemas?
    • no
  • Do we check evidence?
    • no
  • Do we need to support node 14?
    • drop support for node 14 and 16 (separate PR for this)
  • Add MAJOR Changelog entry as vc2.0 is the new default context / data model.
  • @OR13 When this get's merged we will want to update the various plugins that build examples.

@JSAssassin
Copy link
Contributor

@aljones15 IMO we should drop support for Node 14 and 16 and that can be addressed in a separate PR.

@aljones15
Copy link
Contributor Author

@aljones15 IMO we should drop support for Node 14 and 16 and that can be addressed in a separate PR.

awesome thanks for letting me, node 14 tests are failing probably because of the branches.

@OR13
Copy link
Contributor

OR13 commented Sep 17, 2023

When this get's merged we will want to update the various plugins that build examples.

@aljones15
Copy link
Contributor Author

When this get's merged we will want to update the various plugins that build examples.

@OR13 this branch was used for the test suite for VC 2.0, but as that spec is still in progress, we might not release this build of this library. This PR also still lacks a few VC 2.0 features such as validUntil and validFrom. So this PR is not currently prioritized to my knowledge for merge or release.

@aljones15 aljones15 force-pushed the update-vc-2.0 branch 2 times, most recently from d9d7ba4 to dcecf7b Compare November 28, 2023 14:54
lib/helpers.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/helpers.js Outdated Show resolved Hide resolved
lib/helpers.js Outdated Show resolved Hide resolved
test/10-verify.spec.js Outdated Show resolved Hide resolved
test/10-verify.spec.js Outdated Show resolved Hide resolved
Comment on lines +472 to +830
if(result.error) {
const firstError = [].concat(result.error)[0];
throw firstError;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed here and in other places?

Copy link
Contributor Author

@aljones15 aljones15 Dec 6, 2023

Choose a reason for hiding this comment

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

This is actually not my code or part of this PR, but I do know that the VC library doesn't throw in all cases, verification can return an error or an array of errors. it looks like this code takes that error which could be an array, ensures it is an array and then throws the first error. We probably should be asserting on that error. Maybe an issue can be made about this to assert on these errors rather than throw? EDIT: in most cases it looks like we don't want to assert on the error, and that this code ensures that positive tests throw if an error occurs in verification as this library often doesn't throw when verifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitrizagidulin got any insight here? it looks like you just wanted to throw if there is an error so you could check it and figure out what was going wrong with this lib?

test/10-verify.spec.js Outdated Show resolved Hide resolved
test/10-verify.spec.js Outdated Show resolved Hide resolved
@aljones15 aljones15 force-pushed the update-vc-2.0 branch 2 times, most recently from ec59695 to 8d114c3 Compare December 6, 2023 14:39
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2023

Codecov Report

Attention: Patch coverage is 90.85366% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 90.46%. Comparing base (be05032) to head (c22021f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   88.66%   90.46%   +1.79%     
==========================================
  Files           4        5       +1     
  Lines         812     1059     +247     
==========================================
+ Hits          720      958     +238     
- Misses         92      101       +9     
Files Coverage Δ
lib/contexts/index.js 100.00% <100.00%> (ø)
lib/helpers.js 92.39% <92.39%> (ø)
lib/index.js 90.70% <89.68%> (+1.64%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be05032...c22021f. Read the comment docs.

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! LGTM.

aljones15 and others added 28 commits May 17, 2024 22:56
…test data.

Co-authored-by: Tashi D. Gyeltshen <38434620+JSAssassin@users.noreply.github.com>
Co-authored-by: Tashi D. Gyeltshen <38434620+JSAssassin@users.noreply.github.com>
Co-authored-by: Tashi D. Gyeltshen <38434620+JSAssassin@users.noreply.github.com>
Co-authored-by: Tashi D. Gyeltshen <38434620+JSAssassin@users.noreply.github.com>
Co-authored-by: Tashi D. Gyeltshen <38434620+JSAssassin@users.noreply.github.com>
…stamp.

Co-authored-by: Tashi D. Gyeltshen <38434620+JSAssassin@users.noreply.github.com>
- Make "id" optional.
- If "id" present, check it's a URL.
- Add tests.
Co-authored-by: BigBlueHat <byoung@bigbluehat.com>
Co-authored-by: BigBlueHat <byoung@bigbluehat.com>
@@ -1,5 +1,6 @@
export default {
CREDENTIALS_CONTEXT_URL: 'https://www.w3.org/2018/credentials/v1',
CREDENTIALS_CONTEXT_URL: 'https://www.w3.org/ns/credentials/v2',
CREDENTIALS_CONTEXT_V1_URL: 'https://www.w3.org/2018/credentials/v1',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aljones15 maybe we need a CREDENTIALS_CONTEXT_V2_URL here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants