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

fix(client/v2): resolve keyring flags properly #19060

Merged
merged 8 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions client/cmd.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client

import (
"context"
"crypto/tls"
"fmt"
"strings"
Expand Down Expand Up @@ -278,7 +279,7 @@ func readTxCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, err
from, _ := flagSet.GetString(flags.FlagFrom)
fromAddr, fromName, keyType, err := GetFromFields(clientCtx, clientCtx.Keyring, from)
if err != nil {
return clientCtx, err
return clientCtx, fmt.Errorf("failed to convert address field to address: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message has been improved to provide more context, but the error wrapping with %w is only useful if the error is going to be unwrapped later. If not, %v should be used instead.

- return clientCtx, fmt.Errorf("failed to convert address field to address: %w", err)
+ return clientCtx, fmt.Errorf("failed to convert address field to address: %v", err)

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
return clientCtx, fmt.Errorf("failed to convert address field to address: %w", err)
return clientCtx, fmt.Errorf("failed to convert address field to address: %v", err)

}

clientCtx = clientCtx.WithFrom(from).WithFromAddress(fromAddr).WithFromName(fromName)
Expand Down Expand Up @@ -358,13 +359,6 @@ func GetClientContextFromCmd(cmd *cobra.Command) Context {
// SetCmdClientContext sets a command's Context value to the provided argument.
// If the context has not been set, set the given context as the default.
func SetCmdClientContext(cmd *cobra.Command, clientCtx Context) error {
v := cmd.Context().Value(ClientContextKey)
if v == nil {
v = &clientCtx
}

clientCtxPtr := v.(*Context)
*clientCtxPtr = clientCtx

cmd.SetContext(context.WithValue(cmd.Context(), ClientContextKey, &clientCtx))
return nil
}
11 changes: 10 additions & 1 deletion client/v2/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,16 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Features

* [#18461](https://github.com/cosmos/cosmos-sdk/pull/18461) Support governance proposals.
* [#19039](https://github.com/cosmos/cosmos-sdk/pull/19039) add support for pubkey in autocli.
* [#19039](https://github.com/cosmos/cosmos-sdk/pull/19039) Add support for pubkey in autocli.

### Improvements

* [#19060](https://github.com/cosmos/cosmos-sdk/pull/19060) Use client context from root (or enhanced) command in autocli commands.
* Note, the given command must have a `client.Context` in its context.

### Bug Fixes

* [#19060](https://github.com/cosmos/cosmos-sdk/pull/19060) Simplify key flag parsing logic in flag handler.

### API Breaking Changes

Expand Down
5 changes: 2 additions & 3 deletions client/v2/autocli/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ func (appOptions AppOptions) EnhanceRootCommand(rootCmd *cobra.Command) error {
ValidatorAddressCodec: appOptions.ClientCtx.ValidatorAddressCodec,
ConsensusAddressCodec: appOptions.ClientCtx.ConsensusAddressCodec,
},
ClientCtx: appOptions.ClientCtx,
TxConfigOpts: appOptions.TxConfigOpts,
GetClientConn: func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
return client.GetClientQueryContext(cmd)
Expand Down Expand Up @@ -112,7 +111,7 @@ func (appOptions AppOptions) EnhanceRootCommandWithBuilder(rootCmd *cobra.Comman
return err
}
} else {
queryCmd, err := builder.BuildQueryCommand(appOptions, customQueryCmds)
queryCmd, err := builder.BuildQueryCommand(rootCmd.Context(), appOptions, customQueryCmds)
if err != nil {
return err
}
Expand All @@ -125,7 +124,7 @@ func (appOptions AppOptions) EnhanceRootCommandWithBuilder(rootCmd *cobra.Comman
return err
}
} else {
subCmd, err := builder.BuildMsgCommand(appOptions, customMsgCmds)
subCmd, err := builder.BuildMsgCommand(rootCmd.Context(), appOptions, customMsgCmds)
if err != nil {
return err
}
Expand Down
5 changes: 0 additions & 5 deletions client/v2/autocli/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import (

"cosmossdk.io/client/v2/autocli/flag"
authtx "cosmossdk.io/x/auth/tx"

"github.com/cosmos/cosmos-sdk/client"
)

// Builder manages options for building CLI commands.
Expand All @@ -19,9 +17,6 @@ type Builder struct {
// from a given context.
GetClientConn func(*cobra.Command) (grpc.ClientConnInterface, error)

// ClientCtx contains the necessary information needed to execute the commands.
ClientCtx client.Context

// TxConfigOptions is required to support sign mode textual
TxConfigOpts authtx.ConfigOptions

Expand Down
6 changes: 2 additions & 4 deletions client/v2/autocli/common.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package autocli

import (
"context"
"fmt"
"strings"

Expand Down Expand Up @@ -57,7 +56,6 @@ func (b *Builder) buildMethodCommandCommon(descriptor protoreflect.MethodDescrip
Version: options.Version,
}

cmd.SetContext(context.Background())
binder, err := b.AddMessageFlags(cmd.Context(), cmd.Flags(), inputType, options)
if err != nil {
return nil, err
Expand Down Expand Up @@ -179,7 +177,7 @@ func (b *Builder) enhanceCommandCommon(
// enhanceQuery enhances the provided query command with the autocli commands for a module.
func enhanceQuery(builder *Builder, moduleName string, cmd *cobra.Command, modOpts *autocliv1.ModuleOptions) error {
if queryCmdDesc := modOpts.Query; queryCmdDesc != nil {
subCmd := topLevelCmd(moduleName, fmt.Sprintf("Querying commands for the %s module", moduleName))
subCmd := topLevelCmd(cmd.Context(), moduleName, fmt.Sprintf("Querying commands for the %s module", moduleName))
if err := builder.AddQueryServiceCommands(subCmd, queryCmdDesc); err != nil {
return err
}
Expand All @@ -193,7 +191,7 @@ func enhanceQuery(builder *Builder, moduleName string, cmd *cobra.Command, modOp
// enhanceMsg enhances the provided msg command with the autocli commands for a module.
func enhanceMsg(builder *Builder, moduleName string, cmd *cobra.Command, modOpts *autocliv1.ModuleOptions) error {
if txCmdDesc := modOpts.Tx; txCmdDesc != nil {
subCmd := topLevelCmd(moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
subCmd := topLevelCmd(cmd.Context(), moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
if err := builder.AddMsgServiceCommands(subCmd, txCmdDesc); err != nil {
return err
}
Expand Down
29 changes: 14 additions & 15 deletions client/v2/autocli/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ import (
)

type fixture struct {
conn *testClientConn
b *Builder
conn *testClientConn
b *Builder
clientCtx client.Context
}

func initFixture(t *testing.T) *fixture {
Expand Down Expand Up @@ -60,8 +61,7 @@ func initFixture(t *testing.T) *fixture {
interfaceRegistry := encodingConfig.Codec.InterfaceRegistry()
banktypes.RegisterInterfaces(interfaceRegistry)

var initClientCtx client.Context
initClientCtx = initClientCtx.
clientCtx := client.Context{}.
WithAddressCodec(addresscodec.NewBech32Codec("cosmos")).
WithValidatorAddressCodec(addresscodec.NewBech32Codec("cosmosvaloper")).
WithConsensusAddressCodec(addresscodec.NewBech32Codec("cosmosvalcons")).
Expand All @@ -79,29 +79,29 @@ func initFixture(t *testing.T) *fixture {
Builder: flag.Builder{
TypeResolver: protoregistry.GlobalTypes,
FileResolver: protoregistry.GlobalFiles,
AddressCodec: initClientCtx.AddressCodec,
ValidatorAddressCodec: initClientCtx.ValidatorAddressCodec,
ConsensusAddressCodec: initClientCtx.ConsensusAddressCodec,
AddressCodec: clientCtx.AddressCodec,
ValidatorAddressCodec: clientCtx.ValidatorAddressCodec,
ConsensusAddressCodec: clientCtx.ConsensusAddressCodec,
Keyring: akr,
},
GetClientConn: func(*cobra.Command) (grpc.ClientConnInterface, error) {
return conn, nil
},
AddQueryConnFlags: flags.AddQueryFlagsToCmd,
AddTxConnFlags: flags.AddTxFlagsToCmd,
ClientCtx: initClientCtx,
}
assert.NilError(t, b.ValidateAndComplete())

return &fixture{
conn: conn,
b: b,
conn: conn,
b: b,
clientCtx: clientCtx,
}
}

func runCmd(conn *testClientConn, b *Builder, command func(moduleName string, b *Builder) (*cobra.Command, error), args ...string) (*bytes.Buffer, error) {
func runCmd(fixture *fixture, command func(moduleName string, f *fixture) (*cobra.Command, error), args ...string) (*bytes.Buffer, error) {
out := &bytes.Buffer{}
cmd, err := command("test", b)
cmd, err := command("test", fixture)
if err != nil {
return out, err
}
Expand Down Expand Up @@ -215,14 +215,13 @@ func TestErrorBuildCommand(t *testing.T) {
Tx: commandDescriptor,
},
},
ClientCtx: b.ClientCtx,
}

_, err := b.BuildMsgCommand(appOptions, nil)
_, err := b.BuildMsgCommand(context.Background(), appOptions, nil)
assert.ErrorContains(t, err, "can't find field un-existent-proto-field")

appOptions.ModuleOptions["test"].Tx = &autocliv1.ServiceCommandDescriptor{Service: "un-existent-service"}
appOptions.ModuleOptions["test"].Query = &autocliv1.ServiceCommandDescriptor{Service: "un-existent-service"}
_, err = b.BuildMsgCommand(appOptions, nil)
_, err = b.BuildMsgCommand(context.Background(), appOptions, nil)
assert.ErrorContains(t, err, "can't find service un-existent-service")
}
14 changes: 8 additions & 6 deletions client/v2/autocli/flag/address.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,9 @@ func (a *addressValue) Set(s string) error {
return nil
}

_, err = a.addressCodec.StringToBytes(s)
if err != nil {
return fmt.Errorf("invalid account address or key name: %w", err)
}

// failed all validation, just accept the input.
// TODO(@julienrbrt), for final client/v2 2.0.0 revert the logic and
// do a better keyring instantiation.
a.value = s

return nil
Expand Down Expand Up @@ -129,7 +127,11 @@ func (a *consensusAddressValue) Set(s string) error {
var pk cryptotypes.PubKey
err2 := cdc.UnmarshalInterfaceJSON([]byte(s), &pk)
if err2 != nil {
return fmt.Errorf("input isn't a pubkey %w or is an invalid account address: %w", err, err2)
// failed all validation, just accept the input.
// TODO(@julienrbrt), for final client/v2 2.0.0 revert the logic and
// do a better keyring instantiation.
a.value = s
return nil
}

a.value, err = a.addressCodec.BytesToString(pk.Address())
Expand Down
9 changes: 4 additions & 5 deletions client/v2/autocli/msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ import (
// BuildMsgCommand builds the msg commands for all the provided modules. If a custom command is provided for a
// module, this is used instead of any automatically generated CLI commands. This allows apps to a fully dynamic client
// with a more customized experience if a binary with custom commands is downloaded.
func (b *Builder) BuildMsgCommand(appOptions AppOptions, customCmds map[string]*cobra.Command) (*cobra.Command, error) {
msgCmd := topLevelCmd("tx", "Transaction subcommands")
func (b *Builder) BuildMsgCommand(ctx context.Context, appOptions AppOptions, customCmds map[string]*cobra.Command) (*cobra.Command, error) {
msgCmd := topLevelCmd(ctx, "tx", "Transaction subcommands")

if err := b.enhanceCommandCommon(msgCmd, msgCmdType, appOptions, customCmds); err != nil {
return nil, err
}
Expand All @@ -50,7 +51,7 @@ func (b *Builder) AddMsgServiceCommands(cmd *cobra.Command, cmdDescriptor *autoc
for cmdName, subCmdDescriptor := range cmdDescriptor.SubCommands {
subCmd := findSubCommand(cmd, cmdName)
if subCmd == nil {
subCmd = topLevelCmd(cmdName, fmt.Sprintf("Tx commands for the %s service", subCmdDescriptor.Service))
subCmd = topLevelCmd(cmd.Context(), cmdName, fmt.Sprintf("Tx commands for the %s service", subCmdDescriptor.Service))
}

// Add recursive sub-commands if there are any. This is used for nested services.
Expand Down Expand Up @@ -121,8 +122,6 @@ func (b *Builder) AddMsgServiceCommands(cmd *cobra.Command, cmdDescriptor *autoc
// BuildMsgMethodCommand returns a command that outputs the JSON representation of the message.
func (b *Builder) BuildMsgMethodCommand(descriptor protoreflect.MethodDescriptor, options *autocliv1.RpcCommandOptions) (*cobra.Command, error) {
execFunc := func(cmd *cobra.Command, input protoreflect.Message) error {
cmd.SetContext(context.WithValue(context.Background(), client.ClientContextKey, &b.ClientCtx))

clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
Expand Down
37 changes: 21 additions & 16 deletions client/v2/autocli/msg_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package autocli

import (
"context"
"fmt"
"testing"

Expand All @@ -11,18 +12,22 @@ import (
autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1"
"cosmossdk.io/client/v2/internal/testpb"

"github.com/cosmos/cosmos-sdk/client"
)

var buildModuleMsgCommand = func(moduleName string, b *Builder) (*cobra.Command, error) {
cmd := topLevelCmd(moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
err := b.AddMsgServiceCommands(cmd, bankAutoCLI)
var buildModuleMsgCommand = func(moduleName string, f *fixture) (*cobra.Command, error) {
ctx := context.WithValue(context.Background(), client.ClientContextKey, &f.clientCtx)
cmd := topLevelCmd(ctx, moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
err := f.b.AddMsgServiceCommands(cmd, bankAutoCLI)
return cmd, err
}

func buildCustomModuleMsgCommand(cmdDescriptor *autocliv1.ServiceCommandDescriptor) func(moduleName string, b *Builder) (*cobra.Command, error) {
return func(moduleName string, b *Builder) (*cobra.Command, error) {
cmd := topLevelCmd(moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
err := b.AddMsgServiceCommands(cmd, cmdDescriptor)
func buildCustomModuleMsgCommand(cmdDescriptor *autocliv1.ServiceCommandDescriptor) func(moduleName string, f *fixture) (*cobra.Command, error) {
return func(moduleName string, f *fixture) (*cobra.Command, error) {
ctx := context.WithValue(context.Background(), client.ClientContextKey, &f.clientCtx)
cmd := topLevelCmd(ctx, moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
err := f.b.AddMsgServiceCommands(cmd, cmdDescriptor)
return cmd, err
}
}
Expand All @@ -42,15 +47,15 @@ var bankAutoCLI = &autocliv1.ServiceCommandDescriptor{

func TestMsg(t *testing.T) {
fixture := initFixture(t)
out, err := runCmd(fixture.conn, fixture.b, buildModuleMsgCommand, "send",
out, err := runCmd(fixture, buildModuleMsgCommand, "send",
"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk", "cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk", "1foo",
"--generate-only",
"--output", "json",
)
assert.NilError(t, err)
golden.Assert(t, out.String(), "msg-output.golden")

out, err = runCmd(fixture.conn, fixture.b, buildCustomModuleMsgCommand(&autocliv1.ServiceCommandDescriptor{
out, err = runCmd(fixture, buildCustomModuleMsgCommand(&autocliv1.ServiceCommandDescriptor{
Service: bankv1beta1.Msg_ServiceDesc.ServiceName,
RpcCommandOptions: []*autocliv1.RpcCommandOptions{
{
Expand All @@ -71,7 +76,7 @@ func TestMsg(t *testing.T) {
assert.NilError(t, err)
golden.Assert(t, out.String(), "msg-output.golden")

out, err = runCmd(fixture.conn, fixture.b, buildCustomModuleMsgCommand(&autocliv1.ServiceCommandDescriptor{
out, err = runCmd(fixture, buildCustomModuleMsgCommand(&autocliv1.ServiceCommandDescriptor{
Service: bankv1beta1.Msg_ServiceDesc.ServiceName,
RpcCommandOptions: []*autocliv1.RpcCommandOptions{
{
Expand All @@ -98,12 +103,12 @@ func TestMsg(t *testing.T) {
func TestMsgOptionsError(t *testing.T) {
fixture := initFixture(t)

_, err := runCmd(fixture.conn, fixture.b, buildModuleMsgCommand,
_, err := runCmd(fixture, buildModuleMsgCommand,
"send", "5",
)
assert.ErrorContains(t, err, "accepts 3 arg(s)")

_, err = runCmd(fixture.conn, fixture.b, buildModuleMsgCommand,
_, err = runCmd(fixture, buildModuleMsgCommand,
"send", "foo", "bar", "invalid",
)
assert.ErrorContains(t, err, "invalid argument")
Expand All @@ -112,11 +117,11 @@ func TestMsgOptionsError(t *testing.T) {
func TestHelpMsg(t *testing.T) {
fixture := initFixture(t)

out, err := runCmd(fixture.conn, fixture.b, buildModuleMsgCommand, "-h")
out, err := runCmd(fixture, buildModuleMsgCommand, "-h")
assert.NilError(t, err)
golden.Assert(t, out.String(), "help-toplevel-msg.golden")

out, err = runCmd(fixture.conn, fixture.b, buildModuleMsgCommand, "send", "-h")
out, err = runCmd(fixture, buildModuleMsgCommand, "send", "-h")
assert.NilError(t, err)
golden.Assert(t, out.String(), "help-echo-msg.golden")
}
Expand All @@ -135,7 +140,7 @@ func TestBuildCustomMsgCommand(t *testing.T) {
},
}

cmd, err := b.BuildMsgCommand(appOptions, map[string]*cobra.Command{
cmd, err := b.BuildMsgCommand(context.Background(), appOptions, map[string]*cobra.Command{
"test": {Use: "test", Run: func(cmd *cobra.Command, args []string) {
customCommandCalled = true
}},
Expand All @@ -153,7 +158,7 @@ func TestNotFoundErrorsMsg(t *testing.T) {
b.AddTxConnFlags = nil

buildModuleMsgCommand := func(moduleName string, cmdDescriptor *autocliv1.ServiceCommandDescriptor) (*cobra.Command, error) {
cmd := topLevelCmd(moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))
cmd := topLevelCmd(context.Background(), moduleName, fmt.Sprintf("Transactions commands for the %s module", moduleName))

err := b.AddMsgServiceCommands(cmd, cmdDescriptor)
return cmd, err
Expand Down
Loading
Loading