-
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
Parity for ION Tools #318
Parity for ION Tools #318
Conversation
Codecov Report
@@ Coverage Diff @@
## main #318 +/- ##
==========================================
- Coverage 57.46% 57.02% -0.44%
==========================================
Files 50 51 +1
Lines 5867 6110 +243
==========================================
+ Hits 3371 3484 +113
- Misses 1869 1970 +101
- Partials 627 656 +29
|
very nice |
did/ion/did.go
Outdated
} | ||
|
||
// GetShortFormDIDFromLongFormDID returns the short form DID from a long form DID | ||
func GetShortFormDIDFromLongFormDID(longFormDID string) (string, error) { |
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 GetShortFormDIDFromLongFormDID(longFormDID string) (string, error) { | |
func LongToShortFormDID(longFormDID string) (string, error) { |
did/ion/did.go
Outdated
return strings.Join([]string{"did", did.IONMethod.String(), hash}, ":"), nil | ||
} | ||
|
||
// GetShortFormDIDFromLongFormDID returns the short form DID from a long form 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.
A unit test for this would be nice.
did/ion/operations.go
Outdated
} | ||
|
||
type Resolver struct { | ||
baseURL string |
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.
Would suggest typing this, as you're already parsing it.
baseURL string | |
baseURL url.URL |
did/ion/operations.go
Outdated
// and similarly for submitting anchor operations to the ION node... | ||
// | ||
// https://ion.tbd.network/operations | ||
func NewIONResolver(baseURL string) (*Resolver, error) { |
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 think it's important to force HTTPS for the URL.
did/ion/operations.go
Outdated
if i.baseURL == "" { | ||
return nil, errors.New("resolver URL is empty") | ||
} | ||
resp, err := http.Get(strings.Join([]string{i.baseURL, "identifiers", id}, "/")) |
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 should expose a way for SDK users to provide their own http.Client
and the context.Context
they want to use for the request. The first allows for users to do their own telemetry and metric measurements. The second allows for termination signal and user configured deadlines.
See https://github.com/TBD54566975/ssi-service/blob/4936f10da0bae4b1606f5b2362c32bb6eb02e170/pkg/service/did/resolve/universal.go#L28 for how you could do both.
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 - done
did/ion/operations.go
Outdated
if err != nil { | ||
return errors.Wrapf(err, "marshaling anchor operation %+v", op) | ||
} | ||
resp, err := http.Post(strings.Join([]string{i.baseURL, "operations"}, "/"), "application/json", bytes.NewReader(jsonOpBytes)) |
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.
Same comments as previously noted.
did/ion/operations.go
Outdated
|
||
// Anchor submits an anchor operation to the ION node by appending the operations path to the base URL | ||
// and making a POST request | ||
func (i Resolver) Anchor(op AnchorOperation) error { |
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.
A top level package example on how to use this package would be awesome. Something super simple like: here's how you create an ION did:
NewIONDID()
Anchor()
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.
added
did/resolver.go
Outdated
// ParseDIDResolution attempts to parse a DID Resolution Result or a DID Document | ||
func ParseDIDResolution(resolvedDID []byte) (*DIDResolutionResult, error) { | ||
// first try to parse as a DID Resolution Result | ||
var result DIDResolutionResult |
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.
Note that DIDResolutionResult
does not have json
tags in the struct. I had to add them in https://github.com/TBD54566975/ssi-service/blob/4936f10da0bae4b1606f5b2362c32bb6eb02e170/pkg/service/did/resolve/universal.go#L55
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.
added
did/resolver.go
Outdated
@@ -64,3 +65,23 @@ func GetMethodForDID(did string) (Method, error) { | |||
} | |||
return Method(split[1]), nil | |||
} | |||
|
|||
// ParseDIDResolution attempts to parse a DID Resolution Result or a DID Document | |||
func ParseDIDResolution(resolvedDID []byte) (*DIDResolutionResult, error) { |
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.
Mind adding a test for this function?
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
did/web.go
Outdated
didWeb := DIDWeb(did) | ||
doc, err := didWeb.Resolve() | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "could not resolve did:web 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.
return nil, errors.Wrapf(err, "could not resolve did:web DID: %s", did) | |
return nil, errors.Wrapf(err, "resolving did:web DID: %s", did) |
thanks for the thorough review @andresuribe87 ; should have addressed all |
// TODO(gabe) enforce validation of DID syntax https://www.w3.org/TR/did-core/#did-syntax | ||
type DIDDocument struct { | ||
type Document struct { |
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 will break backwards compatibility on ssi-service
. Was this change intentional?
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.
yeah i think it's a simpler name given the package is did
I can do the uptake - that ok?
full parity with https://github.com/decentralized-identity/ion-tools