-
Notifications
You must be signed in to change notification settings - Fork 55
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
Implementation of did:jwk #363
Conversation
Codecov Report
@@ Coverage Diff @@
## main #363 +/- ##
==========================================
+ Coverage 57.99% 58.27% +0.28%
==========================================
Files 52 53 +1
Lines 6574 6686 +112
==========================================
+ Hits 3812 3896 +84
- Misses 2055 2075 +20
- Partials 707 715 +8
|
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.
Sweet! Some minor comments.
return false | ||
} | ||
|
||
func GetSupportedDIDJWKTypes() []crypto.KeyType { |
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.
Should this function call crypto.GetSupportedKeyTypes
?
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.
func isSupportedJWKType(kt crypto.KeyType) bool { | ||
jwkTypes := GetSupportedDIDJWKTypes() | ||
for _, t := range jwkTypes { | ||
if t == kt { | ||
return true | ||
} | ||
} | ||
return 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.
Seem like this is the same as crypto.IsSupportedKeyType
. Is it possible to DRY this up?
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.
it happens to overlap but it's a distinct method since there's no guarantee we enable all supported key types for did key and DID JWK. for example, did:jwk can support any JWK type. did:key only supports what's in the spec.
did/jwk.go
Outdated
if !strings.HasPrefix(did, JWKPrefix) { | ||
return nil, fmt.Errorf("not a did:jwk DID: %s", 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.
This bit of logic is also done in the Suffix
function, which is called inside Expand
. Consider removing it.
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.
removed
did/jwk_test.go
Outdated
assert.NotEmpty(t, pk) | ||
assert.NotEmpty(t, sk) | ||
|
||
gotJWK, err := jwk.FromRaw(sk) |
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 the secretKey
, no? Should you pass in the pk
, i.e. publicKey ?
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.
updated
did/jwk_test.go
Outdated
badDID := DIDJWK("bad") | ||
_, err := badDID.Expand() | ||
assert.Error(t, err) | ||
assert.Contains(t, err.Error(), "invalid did:jwk: bad") |
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.
Might be useful to have the error say that the prefix wasn't found. That way it's informative to devs how to fix.
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.
updated
did/jwk_test.go
Outdated
t.Run("DID but not a valid did:jwk", func(t *testing.T) { | ||
badDID := DIDKey("did:jwk:bad") | ||
_, err := badDID.Expand() | ||
assert.Error(t, err) | ||
assert.Contains(t, err.Error(), "could not parse did:key: invalid did:key: did:jwk:bad") | ||
}) |
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.
What's the intention behind testing methods from DIDKey
here? I'm not sure I follow.
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.
bad copy and paste, updating
Co-authored-by: Andres Uribe <[email protected]>
* origin/main: Implementation of did:jwk (#363)
Fixes #186