R4R: Standardize CLI Exports from Modules #2840
Conversation
jackzampolin
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #2840 +/- ##
===========================================
- Coverage 57% 56.99% -0.02%
===========================================
Files 131 120 -11
Lines 8581 8308 -273
===========================================
- Hits 4892 4735 -157
+ Misses 3359 3257 -102
+ Partials 330 316 -14 |
jackzampolin
changed the title
jackzampolin
changed the title
|
Tests are failing @jackzampolin |
|
The test failures are unrelated to this PR's changes - #2781 (comment). |
cmd/gaia/cmd/gaiacli/main.go
Outdated
| // TODO: Make the lcd command take a list of ModuleClient | ||
| mc := []sdk.ModuleClients{ | ||
| govClient.NewModuleClient(storeGov, cdc), | ||
| distClient.NewModuleClient("", cdc), |
There was a problem hiding this comment.
Why empty string? Do we never query the distribution store?
There was a problem hiding this comment.
There is currently no query functionality in the dist module. Rigel is working on that.
There was a problem hiding this comment.
We do already know the name of the store key though
| @@ -80,7 +80,7 @@ func main() { | |||
| stakecmd.GetCmdQueryRedelegation("stake", cdc), | |||
| stakecmd.GetCmdQueryRedelegations("stake", cdc), | |||
| slashingcmd.GetCmdQuerySigningInfo("slashing", cdc), | |||
| authcmd.GetAccountCmd("acc", cdc, types.GetAccountDecoder(cdc)), | |||
| authcmd.GetAccountCmd("acc", cdc), | |||
There was a problem hiding this comment.
Should "acc" be a constant somewhere (storeAcc maybe)?
There was a problem hiding this comment.
I don't know really. This is how I found the code. I've opened this issue to track. I'll update to make it a const.
| @@ -71,7 +70,7 @@ func main() { | |||
| // start with commands common to basecoin | |||
| rootCmd.AddCommand( | |||
| client.GetCommands( | |||
| authcmd.GetAccountCmd("acc", cdc, types.GetAccountDecoder(cdc)), | |||
| authcmd.GetAccountCmd("acc", cdc), | |||
There was a problem hiding this comment.
ditto (should be a constant)
|
|
||
| // GetQueryCmd returns the cli query commands for this module | ||
| func (mc ModuleClient) GetQueryCmd() *cobra.Command { | ||
| return &cobra.Command{Hidden: true} |
There was a problem hiding this comment.
As in, no query command? maybe we can return nil or return a boolean to indicate whether the module has any query commands.
There was a problem hiding this comment.
Well this also works just as well. Would you like me to change it?
There was a problem hiding this comment.
I don't know what Hidden: true does (I guess that it returns a nil-ish command that doesn't show up?) - I do think returning nil would be cleaner
There was a problem hiding this comment.
Keeping this in per discussion with @cwgoes
Addresses #2729
This PR refactors the CLI code to clean it up and provide modules with a standard interface for exposing client functionality. When reviewing, start with the interface in
types/moduleClients.go. Each module that exposes functionality under either thegaiacli queryorgaiacli txsub-commands has a new filex/{{ .Module.Name }}/client/moduleClient.gothat implements the interface and stitches together functionality from/x/{{ .Module.Name }}/client/cli(and once #2836 is merged another PR will include/x/{{ .Module.Name }}/client/restroutes).PENDING.mdwith issue #Files changedin the github PR explorerFor Admin Use: