-
Notifications
You must be signed in to change notification settings - Fork 54
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
Singyiu/feat did web 01 #116
Singyiu/feat did web 01 #116
Conversation
did/web_test.go
Outdated
assert.NoError(t, err) | ||
assert.Equal(t, string(didWebToBeResolved), doc.ID) | ||
_, err = didWebCannotBeResolved.Resolve() | ||
assert.NotNil(t, err) |
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.
nit: please use assert.Error(...)
followed by an assert.Contains(t, err.Error(), ...)
to validate the error
did/web_test.go
Outdated
assert.NoError(t, err) | ||
docBytes, err := didWebBasic.CreateDocBytes(crypto.Ed25519, pk) | ||
assert.NoError(t, err) | ||
assert.NotNil(t, docBytes) |
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.
nit: assert.NotEmpty
makes sure that the value is neither nil nor an empty value
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.
please see changes. thanks for the work
did/web.go
Outdated
didWebStr := string(did) | ||
didDoc.ID = didWebStr | ||
verMethodID := didWebStr + "#owner" | ||
didDoc.VerificationMethod[0].ID = verMethodID |
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.
I'm not certain how I feel about relying so heavily on the did key methods. They have a separate API, and could change. Changes in the did:key API should not break the did:web
methods.
What do you think about re-implementing some of the DID creation logic, specific to this DID method?
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.
Good call. Will do.
did/web.go
Outdated
|
||
//with well-known path | ||
if numSubStrs == 3 { | ||
urlStr := "https://" + decodedDomain + "/" + DID_WEB_WELL_KNOWN_URL_PATH + DID_WEB_DID_DOC_NAME |
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 step 4
If no path has been specified in the URL, append /.well-known.
however, we need to check whether a path has been specified in the URL before appending the known path
did/web.go
Outdated
// IsValidDoc performs a minimal check on the DIDDocument | ||
// and make sure the Document's ID is the same as the DIDWeb | ||
func (did DIDWeb) IsValidDoc(doc DIDDocument) bool { | ||
if len(doc.ID) == 0 || len(doc.VerificationMethod) == 0 || len(doc.Authentication) == 0 || len(doc.AssertionMethod) == 0 { |
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.
I am not sure these are all required properties of a did:web document. Where did you source them from?
It may be useful to define an IsEmpty
method on DIDWeb
and put the checks there.
fixing typo Co-authored-by: Gabe <gabe.l.cohen@gmail.com>
nit changes Co-authored-by: Gabe <gabe.l.cohen@gmail.com>
@decentralgabe |
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.
nice work! thanks for making the changes.
For issue #49
Implementation for did:web
according to method specification https://w3c-ccg.github.io/did-method-web
(create and resolve methods implemented with optional path supported, but not update and deactivate)