-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
📣 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
`get`, `publicKeyToDidDoc`, and `generate` both take an options object with the following options: | ||
|
||
```js | ||
const options = { |
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.
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).
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.
Added more expansive documentation for the first 2 options: ab06c99
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); |
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.
Lift the static regex creation outside the function.
const pchar = '[a-zA-Z0-9\\-\\._~]|%[0-9a-fA-F]{2}|[!$&\'()*+,;=:@]'; | ||
const didKeyPattern = '^(?<scheme>did):(?<method>key)' + | ||
`(:(?<version>\\d+))?:(?<multibase>z(${pchar})+)`; |
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.
- 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)
- (aside: looks like that spec format section is missing the
- This doesn't end in
$
. Was the intent to use a base58-btc pattern formulitbase
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.
@@ -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", |
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.
Were these new flags temporary or did you mean them to stay?
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.
Let's leave them as they make the errors more informative.
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>
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.
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. |
Addresses: #46
Also Addresses: #48
Features:
did:key
spec to this libraryverificationSuite
(this is BREAKING)did:key
test suite such aspublicKeyFormat
verificationSuite
we now allow did:keys to resolve to thepublicKeyFormat
Ed25519VerificationKey2018
to the following:driver.get({did, options: {publicKeyFormat: 'Ed25519VerificationKey2018, enableExperimentalPublicKeyTypes: true'}})
To Do:
did:key
typespublicKeyType: JsonWebKey2020
with arepresentationNotSupported
error (this can be a minor feature in the near future)Ed25519VerificationKey2018
package.json
Sister PRs:
digitalbazaar/did-io#63
digitalbazaar/bedrock-did-io#14
digitalbazaar/bedrock-did-resolver-http#3