Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update did key v7 #3
base: initial
Are you sure you want to change the base?
Update did key v7 #3
Changes from 23 commits
74f3911
e2b3a32
c28c251
38fde21
1299da9
34fc90d
a17e0ea
e26070b
d0b1f22
29b57ec
992b590
6dc9f64
510f909
963ab56
6248cb6
bea2e99
0a2e89f
38bc2cf
98b2212
cb0c52e
ed6989f
e2299ea
ad39610
7e69c10
c40562c
629b79a
ef6af7a
177f622
bc44d0b
02c977c
ae77629
a47725f
6aea527
23e0f63
3364e4a
2c1c0d5
99ca721
a05ff0e
d9d4794
db32c3a
40eed63
16d594f
8b06c20
d88e2de
81a8d70
337f2ff
4f16b24
b25d609
297950b
06560a3
e83a8ce
5e918ab
c4ff86b
cae41cc
809aaa3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
We definitely don't want the
experimental
DID Method in any of our resolution code -- this is going to go into production and could become an attack vector. Why do we need this here?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.
We need a method that is not key for testing the
did:key
statement about the method always beingkey
. We could use v1, but this actually shows a larger issue, right now passing in adid:v1
will result in this implementation throwing as it only allowsdid:key
.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.
Abstraction here feels wrong... getting method-specific things should probably be placed in method-specific libraries.
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.
Totally agree here, this should probably be in
did-method-key-js
. I made an issue to start discussion on this: digitalbazaar/did-method-key#46There are a lot of validation stuff included here that should be in
did-method-key-js
ed25519-verification-key-2020
and possiblyx25519
.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.
Marking this as resolved as
getDidKeyComponents
is no longer in this library and the way we parse did keys has changed. See digitalbazaar/did-method-key#47 for how did keys are parsed now.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 seems like it's wrong, you can have version AND multibase exist at the same time. If the code block is right, it feels confusing. Didn't pass the code smell test... something seems off here.
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.
that is exactly what this accounts for
did:key:3:zf00
will result in multibase and version being defined at the same time. Ifmultibase
is not defined then the string 1 is used as the version. p.s. this results from using the algorithm from the spec that states to separate the components of the identifier using:
.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.
Another problem that needs accounting for here is if someone uses
did:key:1:...
(version 1 the wrong way) ... that needs to be an error, I'd think.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.
that actually should not error as the version number is a positive integer unless I'm not understanding this correctly.
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.
Can the description be more clear? To me the description for
didOptions
sounds the same as description forresolverOptions
. How are the two different?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 is an excellent point and I made changes here: e83a8ce
This file was deleted.