-
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
Refactor DID package #371
Refactor DID package #371
Conversation
Codecov Report
@@ Coverage Diff @@
## main #371 +/- ##
==========================================
- Coverage 59.25% 56.83% -2.43%
==========================================
Files 55 65 +10
Lines 6840 6900 +60
==========================================
- Hits 4053 3921 -132
- Misses 2076 2283 +207
+ Partials 711 696 -15
|
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.
Thanks!
did/resolver/resolver.go
Outdated
@@ -1,12 +1,15 @@ | |||
package did | |||
package resolver |
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.
To avoid stuttering, and seems like a better name for the package.
package resolver | |
package resolution |
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 idea
did/resolver/resolver_test.go
Outdated
func TestDIDDocumentMetadata(t *testing.T) { | ||
// good | ||
var metadata DocumentMetadata | ||
assert.True(t, metadata.IsValid()) | ||
|
||
// bad | ||
badMetadata := DocumentMetadata{ | ||
Created: "bad", | ||
Updated: time.Now().UTC().Format(time.RFC3339), | ||
} | ||
assert.False(t, badMetadata.IsValid()) | ||
} |
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't tell if this was existing, but the tests could be separated
func TestDIDDocumentMetadata(t *testing.T) { | |
// good | |
var metadata DocumentMetadata | |
assert.True(t, metadata.IsValid()) | |
// bad | |
badMetadata := DocumentMetadata{ | |
Created: "bad", | |
Updated: time.Now().UTC().Format(time.RFC3339), | |
} | |
assert.False(t, badMetadata.IsValid()) | |
} | |
func TestDIDDocumentMetadata_IsValid(t *testing.T) { | |
t.Run("returns true with empty", func(t *testing.T) { | |
var metadata DocumentMetadata | |
assert.True(t, metadata.IsValid()) | |
}) | |
t.Run("returns false when created field is not a timestamp", func(t *testing.T) { | |
badMetadata := DocumentMetadata{ | |
Created: "bad", | |
Updated: time.Now().UTC().Format(time.RFC3339), | |
} | |
assert.False(t, badMetadata.IsValid()) | |
}) | |
} |
// The '=' at the end is an artifact of the encoding, and will mess up the decoding | ||
// over the partials, so is removed. | ||
// https://identity.foundation/peer-did-method-spec/index.html#generation-method | ||
// ds := string(d) | ||
// if ds[len(ds)-1] == '=' { | ||
// d = DIDPeer(ds[:len(ds)-1]) | ||
// } |
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 this be 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.
I'm not familiar with the code so would prefer leaving it
did/peer/peer_test.go
Outdated
// unsupported type | ||
_, err := encodePublicKeyWithKeyMultiCodecType(crypto.KeyType("unsupported"), nil) | ||
assert.Error(t, err) | ||
assert.Contains(t, err.Error(), "not a supported key type") | ||
|
||
// bad public key | ||
_, err = encodePublicKeyWithKeyMultiCodecType(crypto.Ed25519, nil) | ||
assert.Error(t, err) | ||
assert.Contains(t, err.Error(), "unknown public key type; could not convert to bytes") |
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.
Similar comment to the above. Though I see this was only moved. Up to you.
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.
done
no new code - just refactoring