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

Add did key spec 7 validators #47

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

aljones15
Copy link

@aljones15 aljones15 commented Aug 5, 2022

Addresses: #46
Also Addresses: #48

Features:

  • Adds most of the validators from the did:key spec to this library
  • Removes the previous use of verificationSuite (this is BREAKING)
  • Adds the options from the did:key test suite such as publicKeyFormat
  • Instead of only allow did keys to resolve to the verificationSuite we now allow did:keys to resolve to the publicKeyFormat
  • This changes the way you get Ed25519VerificationKey2018 to the following: driver.get({did, options: {publicKeyFormat: 'Ed25519VerificationKey2018, enableExperimentalPublicKeyTypes: true'}})

To Do:

  • Expand test coverage for did:key types
  • Throw if publicKeyType: JsonWebKey2020 with a representationNotSupported error (this can be a minor feature in the near future)
  • Expand test coverage for driver methods that might be affected by this upgrade
  • Resolve any small FIXMES or other delays
  • Check the README
  • Add instructions to the README for Ed25519VerificationKey2018
  • Check the linter
  • Check the test project
  • Fix node 14 issues in CI
    • Looks like this is due to using a branch in the package.json
  • Check the package.json for deprecated/removed packages
  • Check the package.json for engine related updates
  • Check the package.json for description and other property related issues
  • Check for template code
  • Check for copy paste code (usually in tests)
  • Check for bad naming conventions
  • Check for outdated jsdoc comments
  • Checkout for outdated comments
  • Make issues about concerns not related to the PR
  • Link to issues the PR addresses or touches on
  • Link to sister PRs involved with this upgrade
  • Ensure PR description gives at least 2-3 lines of reason behind the PR
  • Spell check PR diff
  • Tag most recent editors of code in addition to task giver
  • Check CI actions for commits in PR see why they failed

Sister PRs:
digitalbazaar/did-io#63
digitalbazaar/bedrock-did-io#14
digitalbazaar/bedrock-did-resolver-http#3

@aljones15 aljones15 self-assigned this Aug 5, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2022

Codecov Report

Merging #47 (3c84f1d) into main (ca4b973) will decrease coverage by 1.37%.
The diff coverage is 93.47%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   96.38%   95.01%   -1.37%     
==========================================
  Files           2        4       +2     
  Lines         304      562     +258     
==========================================
+ Hits          293      534     +241     
- Misses         11       28      +17     
Impacted Files Coverage Δ
lib/validators.js 90.34% <90.34%> (ø)
lib/verificationMethods.js 92.30% <92.30%> (ø)
lib/DidKeyDriver.js 98.00% <100.00%> (+1.87%) ⬆️
lib/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aljones15 aljones15 marked this pull request as ready for review August 18, 2022 14:53
@aljones15 aljones15 requested a review from gannan08 March 20, 2023 17:24
CHANGELOG.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
`get`, `publicKeyToDidDoc`, and `generate` both take an options object with the following options:

```js
const options = {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have better docs here. Unclear what some of these do beyond what the names imply. Some of the comments are redundant ("defaults to false", then it's set to false).

Copy link
Author

Choose a reason for hiding this comment

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

Added more expansive documentation for the first 2 options: ab06c99

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +121 to +124
const pchar = '[a-zA-Z0-9\\-\\._~]|%[0-9a-fA-F]{2}|[!$&\'()*+,;=:@]';
const didKeyPattern = '^(?<scheme>did):(?<method>key)' +
`(:(?<version>\\d+))?:(?<multibase>z(${pchar})+)`;
const match = new RegExp(didKeyPattern).exec(did);
Copy link
Member

Choose a reason for hiding this comment

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

Lift the static regex creation outside the function.

Comment on lines +121 to +123
const pchar = '[a-zA-Z0-9\\-\\._~]|%[0-9a-fA-F]{2}|[!$&\'()*+,;=:@]';
const didKeyPattern = '^(?<scheme>did):(?<method>key)' +
`(:(?<version>\\d+))?:(?<multibase>z(${pchar})+)`;
Copy link
Member

Choose a reason for hiding this comment

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

  • Looks like the did:key part could just be static. No reason to complicate with patterns.
  • Is this trying to follow https://w3c-ccg.github.io/did-method-key/#format? That pchar pattern looks too complex if it's just looking for a base58-btc multibase string. It's going to allow all those non-base58 chars into the multibase group. I don't see negative tests to check this.
    • (aside: looks like that spec format section is missing the version part from elsewhere in that doc)
  • This doesn't end in $. Was the intent to use a base58-btc pattern for mulitbase and let other URL parts (like ...#foo) just not match? Or should it not match if trailing parts exist? I'm not sure the scope of how this is used, so this may not matter.

lib/verificationMethods.js Outdated Show resolved Hide resolved
@@ -66,7 +66,7 @@
],
"scripts": {
"test": "npm run test-node",
"test-node": "cross-env NODE_ENV=test mocha --preserve-symlinks -t 10000 test/*.spec.js",
"test-node": "cross-env NODE_ENV=test mocha --preserve-symlinks --full-trace --check-leaks -t 10000 test/*.spec.js",
Copy link
Member

Choose a reason for hiding this comment

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

Were these new flags temporary or did you mean them to stay?

Copy link
Author

Choose a reason for hiding this comment

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

Let's leave them as they make the errors more informative.

aljones15 and others added 6 commits March 24, 2023 10:25
Co-authored-by: David I. Lehn <dlehn@digitalbazaar.com>
Co-authored-by: David I. Lehn <dlehn@digitalbazaar.com>
Co-authored-by: David I. Lehn <dlehn@digitalbazaar.com>
Co-authored-by: David I. Lehn <dlehn@digitalbazaar.com>
…Derivation.

Co-authored-by: David I. Lehn <dlehn@digitalbazaar.com>
Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

See #53 -- it looks like this PR makes a few changes to something that needs to be fixed, but it heads a little in the opposite direction of where we want to go. I didn't review the rest of the PR, but we should instead move the API in the direction of #53 with the breaking changes here.

@dlongley
Copy link
Member

This PR is doing too many things at once -- and some of them no longer matter (like node 14 support, etc.) due to concurrent advances elsewhere. We should try and break off the validation pieces into their own separate PR and so on.

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.

None yet

4 participants