feat: add basic autocli config to every module #13786

aaronc · 2022-11-07T18:40:06Z

Description

This just adds the most basic necessary configuration to each module (essentially the name of the query and msg services). We can deal with more customization and help text in follow-ups.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2022-11-07T18:53:20Z

Codecov Report

Merging #13786 (6087514) into main (4f7d9ea) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13786      +/-   ##
==========================================
- Coverage   56.78%   56.70%   -0.08%     
==========================================
  Files         644      637       -7     
  Lines       54952    54585     -367     
==========================================
- Hits        31203    30951     -252     
+ Misses      21210    21130      -80     
+ Partials     2539     2504      -35

Impacted Files	Coverage Δ
x/auth/module.go	`56.17% <ø> (-0.97%)`	⬇️
x/auth/autocli.go	`100.00% <100.00%> (ø)`
x/gov/autocli.go	`100.00% <100.00%> (ø)`
x/group/keeper/keeper.go	`56.25% <0.00%> (-0.40%)`	⬇️
tx/textual/valuerenderer/string.go
tx/textual/valuerenderer/message.go
tx/textual/valuerenderer/dec.go
tx/textual/valuerenderer/bytes.go
tx/textual/valuerenderer/coins.go
tx/textual/valuerenderer/timestamp.go
... and 4 more

tac0turtle

left one comment, it looks good

tac0turtle · 2022-11-07T19:26:29Z

x/authz/autocli.go

+var AutoCLIOptions = &autocliv1.ModuleOptions{
+	Query: &autocliv1.ServiceCommandDescriptor{Service: _Query_serviceDesc.ServiceName},
+	Tx:    &autocliv1.ServiceCommandDescriptor{Service: _Msg_serviceDesc.ServiceName},
+}


could we add a quick few sentences on what each of these do in a doc. We can expand on it later.

julienrbrt · 2022-11-07T19:28:17Z

How should we describe that in the docs? Do you think it worth to add a page in https://docs.cosmos.network/main/building-modules/{...} specifically for AutoCLI?

pysel · 2022-11-08T07:54:04Z

hello @aaronc. Me and @faddat are currently trying to implement autocli for Osmosis here. We implemented autocli on valset-pref module, but it seems like it does not do anything now. If possible, we would be really really really grateful for any clarification on what we might be doing wrong. Thanks in advance

tac0turtle · 2022-11-08T08:12:12Z

hello @aaronc. Me and @faddat are currently trying to implement autocli for Osmosis here. We implemented autocli on valset-pref module, but it seems like it does not do anything now. If possible, we would be really really really grateful for any clarification on what we might be doing wrong. Thanks in advance

it doesn't seem like you have wired this up in the app.go, we will work on docs so it might help to wait for those.

aaronc · 2022-11-08T12:57:45Z

@RusAkh we don't have a full to end setup ready for this yet. I will be working on showing integration with the actual CLI in a follow-up PR. Also only queries are supported thus far. A bunch of pieces still need to be built

aaronc · 2022-11-08T13:07:30Z

One thing I'm considering is if we can auto-discover the config for modules with only one query and msg service. Maybe that should be the default rather than needing it to be explicit like I did in this PR. You should only need to add the config manually if there's customization needed and/or multiple services.

pysel · 2022-11-08T13:12:10Z

thank you for your response @aaronc! I will look into next pr then.

…li-all-modules

x/auth/autocli.go

I made substantial changes so it would be good to get it re-approved

aaronc · 2022-11-08T14:36:58Z

I changed it so that it auto-discovers options where it can. The PR is now substantially different and smaller.

julienrbrt

lgtm! few nits

julienrbrt · 2022-11-08T14:50:33Z

runtime/services/autocli.go

@@ -35,4 +62,25 @@ func (a AutoCLIQueryService) AppOptions(context.Context, *autocliv1.AppOptionsRe
 	}, nil
 }

+type autocliConfigurator struct {


Nit move that down next to its methods.
Can you explain the use case?

This is just so that we can call RegisterServices to discover the module's service names.

I guess this PR using the configurator approach is an intermediary solution, right? I'm in favor of removing the Configurator interface ultimately (since we're going the interfaces approach)

AppModule's RegisterServices should be simply RegisterServices(grpc.ServiceRegistrar), which would remove autocliConfigurator from this file.

x/auth/autocli.go

Co-authored-by: Julien Robert <julien@rbrt.fr>

aaronc · 2022-11-08T18:22:39Z

Could I get one more review here @tac0turtle or @AmauryM ?

tac0turtle

lgtm

amaury1093

LGTM

amaury1093 · 2022-11-09T10:23:54Z

x/gov/autocli.go

+			Service: govv1.Msg_ServiceDesc.ServiceName,
+			// map v1beta1 as a sub-command
+			SubCommands: map[string]*autocliv1.ServiceCommandDescriptor{
+				"v1beta1": {Service: govv1beta1.Msg_ServiceDesc.ServiceName},


autocli is not wired up to simd's root cmd, right? When I try simd q gov there's no v1beta1 subcommand. I guess there's only the remote info part of autocli for now

amaury1093 · 2022-11-09T10:27:20Z

runtime/services/autocli.go

@@ -22,6 +24,31 @@ func NewAutoCLIQueryService(appModules map[string]module.AppModule) *AutoCLIQuer
 			AutoCLIOptions() *autocliv1.ModuleOptions
 		}); ok {
 			moduleOptions[modName] = autoCliMod.AutoCLIOptions()
+		} else {
+			// try to auto-discover options based on the last msg and query


I was asking above because what happens when we wire up autocli with the root cobra command, and there are conflicts in command names between this auto-discovered one and custom ones?

AutoCLI prefers the custom commands

amaury1093 · 2022-11-09T10:41:00Z

runtime/services/autocli.go

@@ -35,4 +62,25 @@ func (a AutoCLIQueryService) AppOptions(context.Context, *autocliv1.AppOptionsRe
 	}, nil
 }

+type autocliConfigurator struct {


I guess this PR using the configurator approach is an intermediary solution, right? I'm in favor of removing the Configurator interface ultimately (since we're going the interfaces approach)

AppModule's RegisterServices should be simply RegisterServices(grpc.ServiceRegistrar), which would remove autocliConfigurator from this file.

feat: add autocli configs to every module

055c848

github-actions bot added C:x/auth C:x/authz C:x/bank C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/gov C:x/group C:x/mint C:x/nft C:x/params C:x/slashing C:x/staking C:x/upgrade labels Nov 7, 2022

aaronc marked this pull request as ready for review November 7, 2022 18:42

aaronc requested a review from a team as a code owner November 7, 2022 18:42

aaronc requested review from tac0turtle and amaury1093 November 7, 2022 18:42

tac0turtle reviewed Nov 7, 2022

View reviewed changes

julienrbrt previously approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/autoc…

e8fdf88

…li-all-modules

github-actions bot removed C:x/slashing C:x/upgrade C:x/nft labels Nov 8, 2022

tests + gov, auth config tweaks

a5c2b0e

github-actions bot added the C:x/auth label Nov 8, 2022

docs

2fc0943

aaronc marked this pull request as ready for review November 8, 2022 14:34

aaronc commented Nov 8, 2022

View reviewed changes

x/auth/autocli.go Show resolved Hide resolved

aaronc requested a review from julienrbrt November 8, 2022 14:36

julienrbrt approved these changes Nov 8, 2022

View reviewed changes

aaronc and others added 4 commits November 8, 2022 10:08

Update x/auth/autocli.go

c89c22b

Co-authored-by: Julien Robert <julien@rbrt.fr>

Update x/auth/autocli.go

8da86e4

Co-authored-by: Julien Robert <julien@rbrt.fr>

cleanup

fa33b5b

Merge branch 'main' into aaronc/autocli-all-modules

a4fadfb

tac0turtle assigned amaury1093, tac0turtle and julienrbrt Nov 8, 2022

Merge branch 'main' into aaronc/autocli-all-modules

c32e4cd

tac0turtle approved these changes Nov 9, 2022

View reviewed changes

amaury1093 approved these changes Nov 9, 2022

View reviewed changes

Merge branch 'main' into aaronc/autocli-all-modules

4d430c8

amaury1093 enabled auto-merge (squash) November 9, 2022 10:41

Merge branch 'main' into aaronc/autocli-all-modules

6087514

amaury1093 merged commit bcdf81c into main Nov 9, 2022

amaury1093 deleted the aaronc/autocli-all-modules branch November 9, 2022 11:44

julienrbrt mentioned this pull request Nov 9, 2022

docs: add autocli docs #13811

Closed

19 tasks

samricotta mentioned this pull request Mar 10, 2023

Create documentation for AutoCLI #15351

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add basic autocli config to every module #13786

feat: add basic autocli config to every module #13786

aaronc commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

tac0turtle left a comment

tac0turtle Nov 7, 2022

julienrbrt commented Nov 7, 2022 •

edited

Loading

pysel commented Nov 8, 2022

tac0turtle commented Nov 8, 2022

aaronc commented Nov 8, 2022

aaronc commented Nov 8, 2022

pysel commented Nov 8, 2022

aaronc commented Nov 8, 2022

julienrbrt left a comment

julienrbrt Nov 8, 2022

aaronc Nov 8, 2022

aaronc Nov 8, 2022

amaury1093 Nov 9, 2022

aaronc Nov 9, 2022

aaronc commented Nov 8, 2022

tac0turtle left a comment

amaury1093 left a comment

amaury1093 Nov 9, 2022

aaronc Nov 9, 2022

amaury1093 Nov 9, 2022

aaronc Nov 9, 2022

amaury1093 Nov 9, 2022

feat: add basic autocli config to every module #13786

feat: add basic autocli config to every module #13786

Conversation

aaronc commented Nov 7, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Nov 7, 2022 • edited Loading

Codecov Report

tac0turtle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt commented Nov 7, 2022 • edited Loading

pysel commented Nov 8, 2022

tac0turtle commented Nov 8, 2022

aaronc commented Nov 8, 2022

aaronc commented Nov 8, 2022

pysel commented Nov 8, 2022

aaronc commented Nov 8, 2022

julienrbrt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Nov 8, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

julienrbrt commented Nov 7, 2022 •

edited

Loading