R4R: Expose LCD router, allowing devs to register custom routes from their modules #2836
Conversation
jackzampolin
requested review from
cwgoes,
ebuchman,
rigelrozanski and
zramsay
as code owners
client/keys/utils.go
Outdated
| @@ -13,9 +14,10 @@ import ( | |||
|
|
|||
| "github.com/cosmos/cosmos-sdk/client" | |||
|
|
|||
| "net/http" | |||
There was a problem hiding this comment.
I think this should go above the SDK import two lines above (minor).
There was a problem hiding this comment.
@jackzampolin just run goimports 🙏
client/lcd/root.go
Outdated
| rs.log.Error("error closing listener", "err", err) | ||
| }) | ||
|
|
||
| var cleanupFunc func() |
There was a problem hiding this comment.
Why do we define this here?
There was a problem hiding this comment.
🤷♂️ Thats the way it looked when I found it. I'll remove it!
client/lcd/root.go
Outdated
| return err | ||
| } | ||
|
|
||
| cleanupFunc = func() { |
There was a problem hiding this comment.
... seems like we could just define it here
There was a problem hiding this comment.
Thanks @jackzampolin! Glad you got this working. Left a few remarks in addition to the points @cwgoes made ⚡️
client/keys/utils.go
Outdated
| @@ -13,9 +14,10 @@ import ( | |||
|
|
|||
| "github.com/cosmos/cosmos-sdk/client" | |||
|
|
|||
| "net/http" | |||
There was a problem hiding this comment.
@jackzampolin just run goimports 🙏
client/lcd/root.go
Outdated
|
|
||
| cliCtx := context.NewCLIContext().WithCodec(cdc) | ||
| // Start starts the rest server | ||
| func (rs RestServer) Start(listenAddr string, sslHosts string, certFile string, keyFile string, maxOpen int, insecure bool) (err error) { |
There was a problem hiding this comment.
Long function signature. Let's break it up on multi-line.
client/lcd/root.go
Outdated
| return err | ||
| } | ||
|
|
||
| cleanupFunc = func() { |
client/lcd/root.go
Outdated
| // (aka Light Client Daemon) that exposes functionality similar | ||
| // to the cli, but over rest | ||
| func (rs RestServer) ServeCommand() *cobra.Command { | ||
|
|
There was a problem hiding this comment.
Can we remove the extra line (I've noticed people doing this and I'm not sure why (it makes sense for multi-line function signatures though)?
|
@jackzampolin Looks like the build fails now. |
|
Changes LGTM |
|
|
|
Issue was that the right keybase wasn't getting picked up in the LCD tests. I've fixed the tests so that the right keybase is getting passed in at the right time. Tests are passing! |
|
Going to merge this once it's passing tests. |
jackzampolin
dismissed
alexanderbez’s stale review
Addressed concerns and tests are passing. The last comment is an ACK
Fixes #1081