-
Notifications
You must be signed in to change notification settings - Fork 1
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
add AgentResolver #40
Conversation
…setter/getter to sender DID.
Refactor to new settings of user/issuer DIDs
} | ||
|
||
// AgentResolverConfig options for credential status verification | ||
type AgentResolverConfig 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.
Can we use an options pattern? Or if it required field pass the PackageManager directly to NewAgentResolver
constructor?
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.
no, it looks like you missed our discussions
resolvers/agent.go
Outdated
return out, errors.WithStack(err) | ||
} | ||
|
||
idUUID, err := uuid.NewV4() |
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.
use github.com/google/uuid to generate uuid
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.
+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.
done
resolvers/agent.go
Outdated
return out, errors.WithStack(err) | ||
} | ||
|
||
resp, err := http.DefaultClient.Post(status.ID, "application/json", bytes.NewBuffer(iden3commMsg)) |
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.
pass the http client as option
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
resolvers/agent_test.go
Outdated
|
||
agentResolver := NewAgentResolver(agentConfig) | ||
|
||
httpmock.Activate() |
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.
Consider to use https://pkg.go.dev/net/http/httptest@go1.21.6
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
resolvers/agent.go
Outdated
} | ||
defer func() { | ||
err2 := resp.Body.Close() | ||
if err != nil { |
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.
if err2 != nil?
In this case if err == nil
but err2 != nil
you retrun an nil error. But expected an 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.
just return revocationStatus.RevocationStatus, err in the end of the 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
resolvers/agent.go
Outdated
} | ||
}() | ||
|
||
if resp.StatusCode != http.StatusOK { |
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.
Accept range of 2xx codes.
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.
not sure why
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.
200 is expected on all other clients.
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
In general, try to add context for all errors. A good example of why need to do it. idUUID, err := uuid.NewV4()
if err != nil {
return out, err
}
threadUUID, err := uuid.NewV4()
if err != nil {
return out, 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.
some comments from Illiya are ok
resolvers/agent.go
Outdated
// AgentResolverConfig options for credential status verification | ||
type AgentResolverConfig struct { | ||
PackageManager *iden3comm.PackageManager | ||
customHTTPClient *http.Client |
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 is not possible to set customHTTPClient from the external package, since the field is private.
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.
made public
resolvers/agent.go
Outdated
return out, errors.WithStack(err) | ||
} | ||
|
||
var httpClient *http.Client |
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.
refactor to
httpClient := http.DefaultClient
if r.config.customHTTPClient != nil {
httpClient = r.config.customHTTPClient
}
go.mod
Outdated
github.com/gofrs/uuid/v5 v5.0.0 | ||
github.com/google/uuid v1.6.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.
why do we have to uuid packages? I know Illiya aksed to use goohle uuid, but in this case let's remove gofrs completely.
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
No description provided.