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

Make vc gen suite configurable #39

Merged
merged 26 commits into from
Jun 11, 2024

Conversation

aljones15
Copy link
Contributor

@aljones15 aljones15 commented Jun 4, 2024

Removes all cryptosuites from data integrity pushing them out to their respective test projects
the only exception is eddsa-2022 which is maintained for backwards compatibility and should be removed
in a future major release.

Features:

  1. changes the verifier test suite to take in a cryptosuite library or instance for test data generation
  2. moves deps for cryptosuites to devDeps ensuring that test suites that use this lib don't end up with multiple versions of cryptosuites installed
  3. adds VC 2.0 context to documentLoader
  4. turns issuedVc and validVc into pure JSON

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.

Just a wee whitespace tweak request at this point.

vc-generator/index.js Outdated Show resolved Hide resolved
@aljones15 aljones15 marked this pull request as ready for review June 7, 2024 16:04
@aljones15 aljones15 requested a review from BigBlueHat June 7, 2024 18:04
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.

Some tweak requests and some questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we store this as a .json file, since that's all it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it a parameter to the functionL 09184ae

I am hoping to remove a default key entirely in a MAJOR release as this library really should just throw if you don't pass in a key to this suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this reply was to something else, maybe? I don't see sdDoc.js included anywhere (in this code at least), but would still like to see it be stored as JSON if possible.

Copy link
Contributor Author

@aljones15 aljones15 Jun 10, 2024

Choose a reason for hiding this comment

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

sdDoc is now json: ca1a065

There is some pain involved with this. Even node 22's support using module/ es6 imports for json is still experimental: https://nodejs.org/api/esm.html#json-modules

So I have to use createRequire or use node's fs to import json. Using the experimental feature with {type: 'json'} results in eslint breaking as eslint does not support features in the experimental stage. Weirdly, the parser they use esprima does not throw on esprima.tokenize("import foo from './bar.json' with {type: 'json;}");

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we do this already to import the other JSON in the test suites. My main goal is to make sure we use "the simplest thing that could possibly work"--i.e. don't use JavaScript when JSON is all that's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also of note, there is probably a small army of developers working on making es6 import statements work with json and other file types with out having to use with anyways, I moved the key, issuedVc, and validVc all to json here: f772bf8

I will need to check and make sure no test suite was somehow calling on validVc outside of this library (I just began exporting it in this PR so we should be safe). anyways, that should be it.

index.js Outdated Show resolved Hide resolved
vc-generator/index.js Outdated Show resolved Hide resolved
vc-generator/secret.js Outdated Show resolved Hide resolved
@@ -6,7 +6,7 @@
"type": "module",
"scripts": {
"lint": "eslint .",
"test": "mocha tests/"
"test": "mocha tests/ --timeout 25000"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the timeout increase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bbs takes forever.

"@digitalbazaar/bbs-2023-cryptosuite": "^1.2.0",
"@digitalbazaar/ecdsa-rdfc-2019-cryptosuite": "^1.0.1",
"@digitalbazaar/ecdsa-sd-2023-cryptosuite": "^3.2.1",
"@digitalbazaar/eddsa-rdfc-2022-cryptosuite": "^1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason these were moved to devDependencies?

Copy link
Contributor Author

@aljones15 aljones15 Jun 10, 2024

Choose a reason for hiding this comment

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

these are no longer required by this library, but are supplied instead by each individual test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why are they still in the dependency list here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#devdependencies

If someone is planning on downloading and using your module in their program, then they probably don't want or need to download and build the external test or documentation framework that you use.

In this case, it's best to map these additional items in a devDependencies object.

The devDeps in this case are for the test project.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the tests that test this test project? 😜

"@digitalbazaar/bbs-2023-cryptosuite": "^1.2.0",
"@digitalbazaar/ecdsa-rdfc-2019-cryptosuite": "^1.0.1",
"@digitalbazaar/ecdsa-sd-2023-cryptosuite": "^3.2.1",
"@digitalbazaar/eddsa-rdfc-2022-cryptosuite": "^1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Then why are they still in the dependency list here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this reply was to something else, maybe? I don't see sdDoc.js included anywhere (in this code at least), but would still like to see it be stored as JSON if possible.

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.

This looks good to go. I'd love those long lived FIXME comments taken out, but we can do that later/elsewhere if you need to get this merged sooner than later.

Thanks!

index.js Outdated Show resolved Hide resolved
tests/fixtures/keys/index.js Outdated Show resolved Hide resolved
vc-generator/secret.js Outdated Show resolved Hide resolved
aljones15 and others added 3 commits June 11, 2024 11:24
Co-authored-by: BigBlueHat <byoung@bigbluehat.com>
Co-authored-by: BigBlueHat <byoung@bigbluehat.com>
Co-authored-by: BigBlueHat <byoung@bigbluehat.com>
@aljones15 aljones15 merged commit 5750066 into add-suite-verifier-tests Jun 11, 2024
2 checks passed
@aljones15 aljones15 deleted the make-vc-gen-suite-configurable branch June 13, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants