From 2ea7b8ae255e70ec28b1ead8948ad0212268da53 Mon Sep 17 00:00:00 2001 From: Brandon Weng <18161326+BrandonWeng@users.noreply.github.com> Date: Fri, 30 Jun 2023 13:44:33 -0400 Subject: [PATCH] Fix --dry-run for TXs --- FORKED_CHANGELOG.md | 1 + client/cmd.go | 2 +- client/context.go | 26 ++++--- client/context_test.go | 112 ++++++++++++++++++++++++++++ client/flags/flags.go | 4 +- client/tx/factory.go | 41 ++++++---- x/auth/client/cli/tx_sign.go | 8 +- x/bank/client/cli/tx.go | 5 +- x/feegrant/client/testutil/suite.go | 2 +- 9 files changed, 168 insertions(+), 33 deletions(-) diff --git a/FORKED_CHANGELOG.md b/FORKED_CHANGELOG.md index 1f149dd71..a4879e7dd 100644 --- a/FORKED_CHANGELOG.md +++ b/FORKED_CHANGELOG.md @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#2](https://github.com/sei-protocol/sei-cosmos/pull/2) Fix GRPC bug * [\#88](https://github.com/sei-protocol/sei-cosmos/pull/88) Fix Rollback bug not actually rolling back CMS version +* [https://github.com/cosmos/cosmos-sdk/commit/4f1cc3aeac7884cf1f522b87af04a455e92db7cf] ### Improvements diff --git a/client/cmd.go b/client/cmd.go index 6ea3b3e66..af0d2d6a7 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 eedbdf6fd..ad2dc2a2a 100644 --- a/client/context.go +++ b/client/context.go @@ -3,6 +3,7 @@ package client import ( "bufio" "encoding/json" + "fmt" "io" "os" @@ -11,7 +12,6 @@ import ( "gopkg.in/yaml.v2" "github.com/gogo/protobuf/proto" - "github.com/pkg/errors" rpcclient "github.com/tendermint/tendermint/rpc/client" "github.com/cosmos/cosmos-sdk/codec" @@ -330,25 +330,31 @@ func (ctx Context) printOutput(out []byte) error { return nil } -// 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) { +// GetFromFields returns a from account address, account name and keyring type, given either an address or key name. +// If clientCtx.Simulate is true the keystore is not accessed and a valid address must be provided +// If clientCtx.GenerateOnly is true the keystore is only accessed if a key name is provided +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, "must provide a valid Bech32 address in generate-only mode") + return nil, "", 0, fmt.Errorf("a valid bech32 address must be provided in simulation mode: %w", err) } return addr, "", 0, nil + + case clientCtx.GenerateOnly: + if err == nil { + return addr, "", 0, nil + } } 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 9c2e8d4ab..1acd61ed6 100644 --- a/client/context_test.go +++ b/client/context_test.go @@ -13,6 +13,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 +114,114 @@ func TestCLIQueryConn(t *testing.T) { require.NoError(t, err) require.Equal(t, "hello", res.Message) } + +func TestGetFromFields(t *testing.T) { + path := hd.CreateHDPath(118, 0, 0).String() + testCases := []struct { + name string + clientCtx client.Context + keyring func() keyring.Keyring + from string + expectedErr string + }{ + { + name: "from 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: "from keyring", + keyring: func() keyring.Keyring { + kb, err := keyring.New(t.Name(), keyring.BackendTest, t.TempDir(), nil) + require.NoError(t, err) + + _, _, err = kb.NewMnemonic("alice", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1) + require.NoError(t, err) + + return kb + }, + from: "alice", + }, + { + name: "key not found, in memory", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5", + expectedErr: "key not found", + }, + { + keyring: func() keyring.Keyring { + kb, err := keyring.New(t.Name(), keyring.BackendTest, t.TempDir(), nil) + require.NoError(t, err) + return kb + }, + from: "alice", + expectedErr: "alice.info: key not found", + }, + { + name: "bech32 address", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5", + clientCtx: client.Context{}.WithSimulation(true), + }, + { + name: "invalid bech32 address, simulation", + 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: "generate only mode, valid bech32 address", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5", + clientCtx: client.Context{}.WithGenerateOnly(true), + }, + { + name: "invalid bech32 address, generate only", + keyring: func() keyring.Keyring { + return keyring.NewInMemory() + }, + from: "alice", + clientCtx: client.Context{}.WithGenerateOnly(true), + expectedErr: "alice.info: key not found", + }, + { + keyring: func() keyring.Keyring { + kb, err := keyring.New(t.Name(), keyring.BackendTest, t.TempDir(), nil) + 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", + }, + } + + for _, tc := range testCases { + println("Running test case: ", tc.name) + _, _, _, err := client.GetFromFields(tc.clientCtx, tc.keyring(), tc.from) + if tc.expectedErr == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tc.expectedErr) + } + } +} diff --git a/client/flags/flags.go b/client/flags/flags.go index cc46ee677..9420d7e01 100644 --- a/client/flags/flags.go +++ b/client/flags/flags.go @@ -108,8 +108,8 @@ 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(FlagGenerateOnly, false, "Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase is not accessible)") + 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 only accessed when providing a key name)") cmd.Flags().Bool(FlagOffline, false, "Offline mode (does not allow any online functionality") cmd.Flags().BoolP(FlagSkipConfirmation, "y", false, "Skip tx broadcasting prompt confirmation") cmd.Flags().String(FlagKeyringBackend, DefaultKeyringBackend, "Select keyring's backend (os|file|kwallet|pass|test|memory)") diff --git a/client/tx/factory.go b/client/tx/factory.go index ebd43b748..11ca0807a 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -279,19 +279,9 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) { return nil, err } - // use the first element from the list of keys in order to generate a valid - // pubkey that supports multiple algorithms - - var pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type - - if 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") - } - - // take the first info record just for simulation purposes - pk = infos[0].GetPubKey() + pk, err := f.getSimPK() + if err != nil { + return nil, err } // Create an empty signature literal as the ante handler will populate with a @@ -310,6 +300,31 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) { return f.txConfig.TxEncoder()(txb.GetTx()) } +// getSimPK gets the public key to use for building a simulation tx. +// Note, we should only check for keys in the keybase if we are in simulate and execute mode, +// e.g. when using --gas=auto. +// When using --dry-run, we are is simulation mode only and should not check the keybase. +// Ref: https://github.com/cosmos/cosmos-sdk/issues/11283 +func (f Factory) getSimPK() (cryptotypes.PubKey, error) { + var ( + pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type + ) + + // Use the first element from the list of keys in order to generate a valid + // pubkey that supports multiple algorithms. + if f.simulateAndExecute && f.keybase != nil { + records, _ := f.keybase.List() + if len(records) == 0 { + return nil, errors.New("cannot build signature for simulation, key records slice is empty") + } + + // take the first record just for simulation purposes + pk = records[0].GetPubKey() + } + + return pk, nil +} + // Prepare ensures the account defined by ctx.GetFromAddress() exists and // if the account number and/or the account sequence number are zero (not set), // they will be queried for and set on the provided Factory. A new Factory with diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index b656f9401..dab0be61d 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -101,7 +101,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) } @@ -110,7 +110,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) } @@ -229,7 +229,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) } @@ -237,7 +237,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { overwrite, _ := f.GetBool(flagOverwrite) if multisig != "" { // Bech32 decode error, maybe it's a name, we try to fetch from keyring - multisigAddr, multisigName, _, err := client.GetFromFields(txF.Keybase(), multisig, clientCtx.GenerateOnly) + multisigAddr, multisigName, _, err := client.GetFromFields(clientCtx, txF.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 6d2f51d48..20c524411 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 66550ae5e..98e27c584 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)