Implementation of did:jwk #363
Conversation
decentralgabe
requested review from
nitro-neal and
andresuribe87
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
+ Coverage 57.99% 58.29% +0.31%
==========================================
Files 52 53 +1
Lines 6574 6685 +111
==========================================
+ Hits 3812 3897 +85
- Misses 2055 2074 +19
- Partials 707 714 +7
|
| return false | ||
| } | ||
|
|
||
| func GetSupportedDIDJWKTypes() []crypto.KeyType { |
There was a problem hiding this comment.
Should this function call crypto.GetSupportedKeyTypes?
There was a problem hiding this comment.
| 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.
Seem like this is the same as crypto.IsSupportedKeyType. Is it possible to DRY this up?
There was a problem hiding this comment.
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.
This bit of logic is also done in the Suffix function, which is called inside Expand. Consider removing it.
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.
this is the secretKey, no? Should you pass in the pk, i.e. publicKey ?
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.
Might be useful to have the error say that the prefix wasn't found. That way it's informative to devs how to fix.
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.
What's the intention behind testing methods from DIDKey here? I'm not sure I follow.
There was a problem hiding this comment.
bad copy and paste, updating
Co-authored-by: Andres Uribe <[email protected]>
* origin/main: Implementation of did:jwk (#363)
Fixes #186