Skip to content
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

feat: add autocli multi-chain command #14696

Merged
merged 16 commits into from
Jan 23, 2023
Merged

feat: add autocli multi-chain command #14696

merged 16 commits into from
Jan 23, 2023

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jan 19, 2023

Description

Closes: #13285


I suggest testing with some real chains. Not everything will work! But hopefully, this is a good start.

To get started, you'll need to instead cosmcli:

# from within the cosmos-sdk folder
cd v2/client
go install ./cmd/cosmcli

A few commands to try are:

cosmcli # for basic help
cosmcli --init <some-chain>
cosmcli <some-chain> query
# and then any query command, for example:
cosmcli <some-chain> query bank total-supply

This WILL NOT work with the cosmos hub yet because https://github.com/tendermint/liquidity/tree/develop/third_party/proto/cosmos_proto makes it impossible to correctly build a protobuf FileDescriptorSet. I think we will need to work with chains to make sure they manager their proto files correctly. So far, it does work in my testing with regen, osmosis, juno, and evmos. Not all gRPC endpoints work (Notional seems to pretty consistently work thanks @faddat).

With existing chains this uses some fallback ways to generate the auto CLI config. For chains that upgrade to v0.47, there are gRPC services that will directly be used and provide a better UX (cosmos.reflection.v1 and cosmos.autocli.v1).

I want to get it working with https://github.com/julienrbrt/simapp so that we can test the official support in v0.47 (one would run cosmcli --init simapp and then specify one of these endpoints but there's some networking issue currently).

I'm not attached to the cosmcli name. Open to other suggestions.


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@github-actions github-actions bot added the C:CLI label Jan 19, 2023
client/v2/autocli/internal/remote/compat.go Fixed Show fixed Hide fixed
client/v2/autocli/internal/remote/compat.go Fixed Show fixed Hide fixed
client/v2/autocli/remote.go Fixed Show fixed Hide fixed
client/v2/autocli/internal/remote/compat.go Fixed Show fixed Hide fixed
client/v2/autocli/internal/remote/load.go Fixed Show fixed Hide fixed
client/v2/autocli/internal/remote/config.go Fixed Show fixed Hide fixed
@aaronc aaronc marked this pull request as ready for review January 20, 2023 01:53
@aaronc aaronc requested a review from a team as a code owner January 20, 2023 01:53
@github-prbot github-prbot requested review from a team, kocubinski and julienrbrt and removed request for a team January 20, 2023 01:53
client/v2/autocli/remote.go Fixed Show fixed Hide fixed
client/v2/autocli/internal/remote/load.go Fixed Show fixed Hide fixed
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance we can have the tool under tools? kind of confusing client v2 is a lib and binary, right ?

@aaronc
Copy link
Member Author

aaronc commented Jan 20, 2023

any chance we can have the tool under tools? kind of confusing client v2 is a lib and binary, right ?

Sure tools/cosmcli?

What about the rest of client/v2? The only issue is that I don't think we need a separate go.mod's for cosmcli

@tac0turtle
Copy link
Member

any chance we can have the tool under tools? kind of confusing client v2 is a lib and binary, right ?

Sure tools/cosmcli?

What about the rest of client/v2? The only issue is that I don't think we need a separate go.mod's for cosmcli

I could be wrong, will read the pr more in depth to understand the overlap between the two.

@julienrbrt
Copy link
Member

julienrbrt commented Jan 20, 2023

any chance we can have the tool under tools? kind of confusing client v2 is a lib and binary, right ?

I agree, all the remote stuff and cosmcli makes more sense to be extracted as specific tool. I don't think it is supposed to be re-used anyway.

Sure tools/cosmcli?

lgtm

What about the rest of client/v2? The only issue is that I don't think we need a separate go.mod's for cosmcli

I think we should still have a go.mod so it is not tied to the SDK. All tools are standalone. Dependabot will bump the lib in the tool anytime needed. This allows to iterate on the tool without needed to bump client/v2 all the time as well.

@aaronc
Copy link
Member Author

aaronc commented Jan 20, 2023

There is currently no code in cosmcli besides client/v2, it is a thin wrapper. Maybe we can extract the remote code, but to me it feels like that just adds versioning complexity... But I guess there could be benefits in separating the two

@aaronc
Copy link
Member Author

aaronc commented Jan 20, 2023

Actually maybe it's fine to pull out the remote code. It's not meant to be reusable really

@aaronc
Copy link
Member Author

aaronc commented Jan 20, 2023

Do people like the name cosmcli? Or maybe just cosmoscli which is longer but doesn't try to be cute?

@tac0turtle
Copy link
Member

tac0turtle commented Jan 20, 2023

what about hubl, its a play on spelling for the Hubble telescope

(random thought)

@aaronc
Copy link
Member Author

aaronc commented Jan 20, 2023

I refactored the remote code to tools/cosmcli as a standalone go.mod. I kind of like hubl as an alternative... curious what others think

tools/cosmcli/internal/compat.go Fixed Show fixed Hide fixed
tools/cosmcli/internal/compat.go Fixed Show fixed Hide fixed
tools/cosmcli/internal/remote.go Fixed Show fixed Hide fixed
tools/cosmcli/internal/compat.go Fixed Show fixed Hide fixed
tools/cosmcli/internal/load.go Fixed Show fixed Hide fixed
tools/cosmcli/internal/config.go Fixed Show fixed Hide fixed
@julienrbrt
Copy link
Member

I refactored the remote code to tools/cosmcli as a standalone go.mod. I kind of like hubl as an alternative... curious what others think

Hubl is kinda fancy, I like it too personally.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK with Evmos.

It takes a bit of times to get use to it, as everything is a flag and not an argument (compared to the classic cli). But it is super cool!! 👍🏾

Occasionally, I as well get a not found error but it makes sense as it is not a normal account. It works well with Umee, and the rest.

cosmcli evmos query auth account --address evmos1fooo
Error: proto: google.protobuf.Any: unable to resolve "/ethermint.types.v1.EthAccount": not found

One improvement I can think for a follow-up of is not to have default value, which is a bit confusing: e.g.

$ cosmcli evmos query auth account-address-by-id 
{
  "account_address":  "evmos1rtj2r4eaz0v68mxjt5jleynm85yjfu2uxm7pxx"
}
cosmcli evmos query auth account-address-by-id --id 0
{
  "account_address":  "evmos1rtj2r4eaz0v68mxjt5jleynm85yjfu2uxm7pxx"
}

@aaronc
Copy link
Member Author

aaronc commented Jan 21, 2023

tACK with Evmos.

It takes a bit of times to get use to it, as everything is a flag and not an argument (compared to the classic cli). But it is super cool!! 👍🏾

We can customize this in the future by adding autocli options to modules

Occasionally, I as well get a not found error but it makes sense as it is not a normal account. It works well with Umee, and the rest.

cosmcli evmos query auth account --address evmos1fooo
Error: proto: google.protobuf.Any: unable to resolve "/ethermint.types.v1.EthAccount": not found

This is because we're using gRPC reflection which as far as I can tell provides no way to pull the full FileDescriptorSet in one go. This will be improved in 0.47 with cosmos.reflection.v1 (once we resolve #14713).

One improvement I can think for a follow-up of is not to have default value, which is a bit confusing: e.g.

$ cosmcli evmos query auth account-address-by-id 
{
  "account_address":  "evmos1rtj2r4eaz0v68mxjt5jleynm85yjfu2uxm7pxx"
}
cosmcli evmos query auth account-address-by-id --id 0
{
  "account_address":  "evmos1rtj2r4eaz0v68mxjt5jleynm85yjfu2uxm7pxx"
}

Yeah, that's an interesting edge case we should addres. Can you open an issue and add to the autocli epic @julienrbrt ?

Comment on lines +155 to +161
for _, descriptorProto := range fdMap {
for _, dep := range descriptorProto.Dependency {
if fdMap[dep] == nil && !cantFind[dep] /* skip deps we've marked as can't find */ {
missing = append(missing, dep)
}
}
}

Check failure

Code scanning / gosec

the value in the range statement should be _ unless copying a map: want: for key := range m

the value in the range statement should be _ unless copying a map: want: for key := range m
Comment on lines +134 to +136
for _, descriptorProto := range fdMap {
fdSet.File = append(fdSet.File, descriptorProto)
}

Check failure

Code scanning / gosec

the value in the range statement should be _ unless copying a map: want: for key := range m

the value in the range statement should be _ unless copying a map: want: for key := range m
Comment on lines +49 to +107
for chain, chainConfig := range config.Chains {
chainInfo := NewChainInfo(configDir, chain, chainConfig)
err = chainInfo.Load(false)
if err != nil {
cmd.AddCommand(&cobra.Command{
Use: chain,
Short: "Unable to load data",
Long: "Unable to load data, reconfiguration needed.",
RunE: func(cmd *cobra.Command, args []string) error {
fmt.Printf("Error loading chain data for %s: %+v\n", chain, err)
return reconfigure(configDir, chain, config)
},
})
continue
}

appOpts := autocli.AppOptions{
ModuleOptions: chainInfo.ModuleOptions,
}

builder := &autocli.Builder{
Builder: flag.Builder{
TypeResolver: &dynamicTypeResolver{chainInfo},
FileResolver: chainInfo.ProtoFiles,
},
GetClientConn: func(command *cobra.Command) (grpc.ClientConnInterface, error) {
return chainInfo.OpenClient()
},
AddQueryConnFlags: func(command *cobra.Command) {},
}

var update bool
var reconfig bool
chainCmd := &cobra.Command{
Use: chain,
Short: fmt.Sprintf("Commands for the %s chain", chain),
RunE: func(cmd *cobra.Command, args []string) error {
if reconfig {
return reconfigure(configDir, chain, config)
} else if update {
fmt.Printf("Updating autocli data for %s\n", chain)
chainInfo := NewChainInfo(configDir, chain, chainConfig)
err := chainInfo.Load(true)
return err
} else {
return cmd.Help()
}
},
}
chainCmd.Flags().BoolVar(&update, "update", false, "update the CLI commands for the selected chain (should be used after every chain upgrade)")
chainCmd.Flags().BoolVar(&reconfig, "config", false, "re-configure the selected chain (allows choosing a new gRPC endpoint and refreshes data)")

err = appOpts.EnhanceRootCommandWithBuilder(chainCmd, builder)
if err != nil {
return nil, err
}

cmd.AddCommand(chainCmd)
}

Check failure

Code scanning / gosec

the value in the range statement should be _ unless copying a map: want: for key := range m

expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 13
Comment on lines +129 to +131
for dep := range cantFind {
fmt.Printf("Warning: can't find %s.\n", dep)
}

Check failure

Code scanning / gosec

the value in the range statement should be _ unless copying a map: want: for key := range m

expected either an append, delete, or copy to another map in a range with a map, got: ""

func (c *ChainInfo) getCacheDir() (string, error) {
cacheDir := path.Join(c.ConfigDir, "cache")
return cacheDir, os.MkdirAll(cacheDir, 0755)

Check failure

Code scanning / gosec

Expect directory permissions to be 0750 or less

Expect directory permissions to be 0750 or less
return err
}

err = os.MkdirAll(configDir, 0755)

Check failure

Code scanning / gosec

Expect directory permissions to be 0750 or less

Expect directory permissions to be 0750 or less
@aaronc
Copy link
Member Author

aaronc commented Jan 23, 2023

Does this need another ACK?

@aaronc aaronc enabled auto-merge (squash) January 23, 2023 19:11
@aaronc aaronc merged commit d86ba04 into main Jan 23, 2023
@aaronc aaronc deleted the aaronc/autocli-remote-cmd branch January 23, 2023 19:23
@julienrbrt
Copy link
Member

julienrbrt commented Jan 23, 2023

Creating a PR for adding the tooling (as we added a new go mod) and some docs to show up in docs.cosmos.network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutoCLI multi-chain command
3 participants