From fb562732fdff603672d871c8456b915f9d4d5abc Mon Sep 17 00:00:00 2001 From: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> Date: Mon, 31 Oct 2022 20:23:47 +0530 Subject: [PATCH] fix: --dry-run flag not working when using tx cmd (#13673) --- CHANGELOG.md | 5 ++ client/cmd.go | 2 +- client/context.go | 16 ++-- client/context_test.go | 118 ++++++++++++++++++++++++++++ client/flags/flags.go | 2 +- client/tx/factory.go | 2 +- crypto/keyring/keyring.go | 4 +- x/auth/client/cli/tx_sign.go | 8 +- x/bank/client/cli/tx.go | 5 +- x/feegrant/client/testutil/suite.go | 2 +- 10 files changed, 147 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8a6572a4d0c..a7be748997b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,8 +112,13 @@ Reverted #12437 due to API breaking changes. ### Bug Fixes +* [#13673](https://github.com/cosmos/cosmos-sdk/pull/13673) Fix `--dry-run` flag not working when using tx command. * [#13046](https://github.com/cosmos/cosmos-sdk/pull/13046) Fix missing return statement in BaseApp.Query. +### API Breaking Changes + +* [#13673](https://github.com/cosmos/cosmos-sdk/pull/13673) The `GetFromFields` function now takes `Context` as an argument and removes `genOnly`. + ## v0.45.7 - 2022-08-04 ### Features diff --git a/client/cmd.go b/client/cmd.go index 6ea3b3e66fba..af0d2d6a7407 100644 --- a/client/cmd.go +++ b/client/cmd.go @@ -235,7 +235,7 @@ func readTxCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, err if clientCtx.From == "" || flagSet.Changed(flags.FlagFrom) { from, _ := flagSet.GetString(flags.FlagFrom) - fromAddr, fromName, keyType, err := GetFromFields(clientCtx.Keyring, from, clientCtx.GenerateOnly) + fromAddr, fromName, keyType, err := GetFromFields(clientCtx, clientCtx.Keyring, from) if err != nil { return clientCtx, err } diff --git a/client/context.go b/client/context.go index eedbdf6fdb65..ecea496c5490 100644 --- a/client/context.go +++ b/client/context.go @@ -333,13 +333,19 @@ func (ctx Context) printOutput(out []byte) error { // GetFromFields returns a from account address, account name and keyring type, given either // an address or key name. If genOnly is true, only a valid Bech32 cosmos // address is returned. -func GetFromFields(kr keyring.Keyring, from string, genOnly bool) (sdk.AccAddress, string, keyring.KeyType, error) { +func GetFromFields(clientCtx Context, kr keyring.Keyring, from string) (sdk.AccAddress, string, keyring.KeyType, error) { if from == "" { return nil, "", 0, nil } - if genOnly { - addr, err := sdk.AccAddressFromBech32(from) + addr, err := sdk.AccAddressFromBech32(from) + switch { + case clientCtx.Simulate: + if err != nil { + return nil, "", 0, errors.Wrap(err, "a valid bech32 address must be provided in simulation mode") + } + return addr, "", 0, nil + case clientCtx.GenerateOnly: if err != nil { return nil, "", 0, errors.Wrap(err, "must provide a valid Bech32 address in generate-only mode") } @@ -348,7 +354,7 @@ func GetFromFields(kr keyring.Keyring, from string, genOnly bool) (sdk.AccAddres } var info keyring.Info - if addr, err := sdk.AccAddressFromBech32(from); err == nil { + if err == nil { info, err = kr.KeyByAddress(addr) if err != nil { return nil, "", 0, err @@ -365,7 +371,7 @@ func GetFromFields(kr keyring.Keyring, from string, genOnly bool) (sdk.AccAddres // NewKeyringFromBackend gets a Keyring object from a backend func NewKeyringFromBackend(ctx Context, backend string) (keyring.Keyring, error) { - if ctx.GenerateOnly || ctx.Simulate { + if ctx.Simulate { return keyring.New(sdk.KeyringServiceName(), keyring.BackendMemory, ctx.KeyringDir, ctx.Input, ctx.KeyringOptions...) } diff --git a/client/context_test.go b/client/context_test.go index 9c2e8d4ab19e..e94fbff3b563 100644 --- a/client/context_test.go +++ b/client/context_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "os" + "strings" "testing" "github.com/spf13/viper" @@ -13,6 +14,7 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/crypto/hd" "github.com/cosmos/cosmos-sdk/crypto/keyring" "github.com/cosmos/cosmos-sdk/testutil/network" "github.com/cosmos/cosmos-sdk/testutil/testdata" @@ -113,3 +115,119 @@ func TestCLIQueryConn(t *testing.T) { require.NoError(t, err) require.Equal(t, "hello", res.Message) } + +func TestGetFromFields(t *testing.T) { + ctx := client.Context{} + path := hd.CreateHDPath(118, 0, 0).String() + + testCases := []struct { + name string + clientCtx client.Context + keyring func() keyring.Keyring + from string + expectedErr string + }{ + { + name: "valid key alice from memory keyring", + keyring: func() keyring.Keyring { + kb := keyring.NewInMemory() + + _, _, err := kb.NewMnemonic("alice", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1) + require.NoError(t, err) + + return kb + }, + from: "alice", + }, + { + name: "valid key alice from test keyring", + keyring: func() keyring.Keyring { + kb, err := keyring.New(t.Name(), keyring.BackendTest, t.TempDir(), nil, ctx.KeyringOptions...) + require.NoError(t, err) + + _, _, err = kb.NewMnemonic("alice", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1) + require.NoError(t, err) + + return kb + }, + from: "alice", + }, + { + name: "address not present in memory keyring", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5", + expectedErr: "key with address 8953EB4F1B47C7982A698DEB7C557D6E4F4CD923 not found: key not found", + }, + { + name: "key is not from test keyring", + keyring: func() keyring.Keyring { + kb, err := keyring.New(t.Name(), keyring.BackendTest, t.TempDir(), nil, ctx.KeyringOptions...) + require.NoError(t, err) + return kb + }, + from: "alice", + expectedErr: "alice.info: key not found", + }, + { + name: "valid bech32 address is allowed when simulation is true", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5", + clientCtx: client.Context{}.WithSimulation(true), + }, + { + name: "only bech32 address is allowed as from key in simulation mode", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "alice", + clientCtx: client.Context{}.WithSimulation(true), + expectedErr: "a valid bech32 address must be provided in simulation mode", + }, + { + name: "valid bech32 address is allowed when generate-only is true", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5", + clientCtx: client.Context{}.WithGenerateOnly(true), + }, + { + name: "only bech32 address is allowed as from key in generate-only mode", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "alice", + clientCtx: client.Context{}.WithGenerateOnly(true), + expectedErr: "must provide a valid Bech32 address in generate-only mode", + }, + { + name: "only bech32 address is allowed as from key even when key is present in the keyring", + keyring: func() keyring.Keyring { + kb, err := keyring.New(t.Name(), keyring.BackendTest, t.TempDir(), nil, ctx.KeyringOptions...) + require.NoError(t, err) + + _, _, err = kb.NewMnemonic("alice", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1) + require.NoError(t, err) + + return kb + }, + clientCtx: client.Context{}.WithGenerateOnly(true), + from: "alice", + expectedErr: "must provide a valid Bech32 address in generate-only mode", + }, + } + + for _, tc := range testCases { + _, _, _, err := client.GetFromFields(tc.clientCtx, tc.keyring(), tc.from) + if tc.expectedErr == "" { + require.NoError(t, err) + } else { + require.True(t, strings.HasPrefix(err.Error(), tc.expectedErr)) + } + + } +} diff --git a/client/flags/flags.go b/client/flags/flags.go index cfc0242c1023..d29036f94fc2 100644 --- a/client/flags/flags.go +++ b/client/flags/flags.go @@ -107,7 +107,7 @@ func AddTxFlagsToCmd(cmd *cobra.Command) { cmd.Flags().Bool(FlagUseLedger, false, "Use a connected Ledger device") cmd.Flags().Float64(FlagGasAdjustment, DefaultGasAdjustment, "adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored ") cmd.Flags().StringP(FlagBroadcastMode, "b", BroadcastSync, "Transaction broadcasting mode (sync|async|block)") - cmd.Flags().Bool(FlagDryRun, false, "ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it") + cmd.Flags().Bool(FlagDryRun, false, "ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)") cmd.Flags().Bool(FlagGenerateOnly, false, "Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase is not accessible)") cmd.Flags().Bool(FlagOffline, false, "Offline mode (does not allow any online functionality") cmd.Flags().BoolP(FlagSkipConfirmation, "y", false, "Skip tx broadcasting prompt confirmation") diff --git a/client/tx/factory.go b/client/tx/factory.go index ebd43b7480ad..6297d00f65ec 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -284,7 +284,7 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) { var pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type - if f.keybase != nil { + if f.simulateAndExecute && f.keybase != nil { infos, _ := f.keybase.List() if len(infos) == 0 { return nil, errors.New("cannot build signature for simulation, key infos slice is empty") diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index d41152978ad2..48afa1aa81f8 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -450,11 +450,11 @@ func (ks keystore) Delete(uid string) error { func (ks keystore) KeyByAddress(address sdk.Address) (Info, error) { ik, err := ks.db.Get(addrHexKeyAsString(address)) if err != nil { - return nil, wrapKeyNotFound(err, fmt.Sprint("key with address", address, "not found")) + return nil, wrapKeyNotFound(err, fmt.Sprint("key with address ", address, " not found")) } if len(ik.Data) == 0 { - return nil, wrapKeyNotFound(err, fmt.Sprint("key with address", address, "not found")) + return nil, wrapKeyNotFound(err, fmt.Sprint("key with address ", address, " not found")) } return ks.key(string(ik.Data)) } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 09be9bbb589d..fa5e233d961b 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -99,7 +99,7 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { } if ms == "" { from, _ := cmd.Flags().GetString(flags.FlagFrom) - _, fromName, _, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly) + _, fromName, _, err := client.GetFromFields(clientCtx, txFactory.Keybase(), from) if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } @@ -108,7 +108,7 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { return err } } else { - multisigAddr, _, _, err := client.GetFromFields(txFactory.Keybase(), ms, clientCtx.GenerateOnly) + multisigAddr, _, _, err := client.GetFromFields(clientCtx, txFactory.Keybase(), ms) if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } @@ -228,7 +228,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } from, _ := cmd.Flags().GetString(flags.FlagFrom) - _, fromName, _, err := client.GetFromFields(txF.Keybase(), from, clientCtx.GenerateOnly) + _, fromName, _, err := client.GetFromFields(clientCtx, txF.Keybase(), from) if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } @@ -238,7 +238,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { multisigAddr, err := sdk.AccAddressFromBech32(multisig) if err != nil { // Bech32 decode error, maybe it's a name, we try to fetch from keyring - multisigAddr, _, _, err = client.GetFromFields(txFactory.Keybase(), multisig, clientCtx.GenerateOnly) + multisigAddr, _, _, err = client.GetFromFields(clientCtx, txFactory.Keybase(), multisig) if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } diff --git a/x/bank/client/cli/tx.go b/x/bank/client/cli/tx.go index 6d2f51d486c3..f9805d2e4252 100644 --- a/x/bank/client/cli/tx.go +++ b/x/bank/client/cli/tx.go @@ -29,8 +29,9 @@ func NewTxCmd() *cobra.Command { func NewSendTxCmd() *cobra.Command { cmd := &cobra.Command{ Use: "send [from_key_or_address] [to_address] [amount]", - Short: `Send funds from one account to another. Note, the'--from' flag is -ignored as it is implied from [from_key_or_address].`, + Short: `Send funds from one account to another. + Note, the'--from' flag is ignored as it is implied from [from_key_or_address]. + When using '--dry-run' a key name cannot be used, only a bech32 address.`, Args: cobra.ExactArgs(3), RunE: func(cmd *cobra.Command, args []string) error { cmd.Flags().Set(flags.FlagFrom, args[0]) diff --git a/x/feegrant/client/testutil/suite.go b/x/feegrant/client/testutil/suite.go index 781bc017b043..fff5475170dc 100644 --- a/x/feegrant/client/testutil/suite.go +++ b/x/feegrant/client/testutil/suite.go @@ -313,7 +313,7 @@ func (s *IntegrationTestSuite) TestNewCmdFeeGrant() { alreadyExistedGrantee := s.addedGrantee clientCtx := val.ClientCtx - fromAddr, fromName, _, err := client.GetFromFields(clientCtx.Keyring, granter.String(), clientCtx.GenerateOnly) + fromAddr, fromName, _, err := client.GetFromFields(clientCtx, clientCtx.Keyring, granter.String()) s.Require().Equal(fromAddr, granter) s.Require().NoError(err)