From df6114203601604d0686237c37d06110005ca7c2 Mon Sep 17 00:00:00 2001 From: Anil Kumar Kammari Date: Wed, 20 Apr 2022 20:24:29 +0530 Subject: [PATCH] fix: Add validation on create gentx (#11693) --- CHANGELOG.md | 1 + types/coin.go | 8 +- x/genutil/client/cli/gentx.go | 4 + x/genutil/client/testutil/suite.go | 173 +++++++++++++++-------------- x/genutil/gentx_test.go | 4 +- 5 files changed, 103 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c55fba21505d..3611076036f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -209,6 +209,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* [\#11693](https://github.com/cosmos/cosmos-sdk/pull/11693) Add validation for gentx cmd. * [\#11558](https://github.com/cosmos/cosmos-sdk/pull/11558) Fix `--dry-run` not working when using tx command. * [\#11354](https://github.com/cosmos/cosmos-sdk/pull/11355) Added missing pagination flag for `bank q total` query. * [\#11197](https://github.com/cosmos/cosmos-sdk/pull/11197) Signing with multisig now works with multisig address which is not in the keyring. diff --git a/types/coin.go b/types/coin.go index cee6053568bc..1b8e465c1d81 100644 --- a/types/coin.go +++ b/types/coin.go @@ -834,13 +834,13 @@ func ParseCoinNormalized(coinStr string) (coin Coin, err error) { return coin, nil } -// ParseCoinsNormalized will parse out a list of coins separated by commas, and normalize them by converting to smallest -// unit. If the parsing is successuful, the provided coins will be sanitized by removing zero coins and sorting the coin +// ParseCoinsNormalized will parse out a list of coins separated by commas, and normalize them by converting to the smallest +// unit. If the parsing is successful, the provided coins will be sanitized by removing zero coins and sorting the coin // set. Lastly a validation of the coin set is executed. If the check passes, ParseCoinsNormalized will return the // sanitized coins. -// Otherwise it will return an error. +// Otherwise, it will return an error. // If an empty string is provided to ParseCoinsNormalized, it returns nil Coins. -// ParseCoinsNormalized supports decimal coins as inputs, and truncate them to int after converted to smallest unit. +// ParseCoinsNormalized supports decimal coins as inputs, and truncate them to int after converted to the smallest unit. // Expected format: "{amount0}{denomination},...,{amountN}{denominationN}" func ParseCoinsNormalized(coinStr string) (Coins, error) { coins, err := ParseDecCoins(coinStr) diff --git a/x/genutil/client/cli/gentx.go b/x/genutil/client/cli/gentx.go index 00ff11ab6f98..9f01d3445b4f 100644 --- a/x/genutil/client/cli/gentx.go +++ b/x/genutil/client/cli/gentx.go @@ -167,6 +167,10 @@ $ %s gentx my-key-name 1000000stake --home=/path/to/home/dir --keyring-backend=o w := bytes.NewBuffer([]byte{}) clientCtx = clientCtx.WithOutput(w) + if err = msg.ValidateBasic(); err != nil { + return err + } + if err = txBldr.PrintUnsignedTx(clientCtx, msg); err != nil { return errors.Wrap(err, "failed to print unsigned std tx") } diff --git a/x/genutil/client/testutil/suite.go b/x/genutil/client/testutil/suite.go index f025160da6c7..755d20154eb3 100644 --- a/x/genutil/client/testutil/suite.go +++ b/x/genutil/client/testutil/suite.go @@ -1,7 +1,6 @@ package testutil import ( - "context" "fmt" "io" "os" @@ -9,10 +8,9 @@ import ( "github.com/stretchr/testify/suite" - "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/simapp" - "github.com/cosmos/cosmos-sdk/testutil" + clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli" "github.com/cosmos/cosmos-sdk/testutil/network" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" @@ -50,84 +48,97 @@ func (s *IntegrationTestSuite) TearDownSuite() { func (s *IntegrationTestSuite) TestGenTxCmd() { val := s.network.Validators[0] - dir := s.T().TempDir() - - cmd := cli.GenTxCmd( - simapp.ModuleBasics, - val.ClientCtx.TxConfig, banktypes.GenesisBalancesIterator{}, val.ClientCtx.HomeDir) - - _, out := testutil.ApplyMockIO(cmd) - clientCtx := val.ClientCtx.WithOutput(out) - - ctx := context.Background() - ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx) - + clientCtx := val.ClientCtx amount := sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(12)) - genTxFile := filepath.Join(dir, "myTx") - cmd.SetArgs([]string{ - fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), - fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, genTxFile), - val.Moniker, - amount.String(), - }) - - err := cmd.ExecuteContext(ctx) - s.Require().NoError(err) - - // validate generated transaction. - open, err := os.Open(genTxFile) - s.Require().NoError(err) - - all, err := io.ReadAll(open) - s.Require().NoError(err) - - tx, err := val.ClientCtx.TxConfig.TxJSONDecoder()(all) - s.Require().NoError(err) - - msgs := tx.GetMsgs() - s.Require().Len(msgs, 1) - - s.Require().Equal(sdk.MsgTypeURL(&types.MsgCreateValidator{}), sdk.MsgTypeURL(msgs[0])) - s.Require().True(val.Address.Equals(msgs[0].GetSigners()[0])) - s.Require().Equal(amount, msgs[0].(*types.MsgCreateValidator).Value) - s.Require().NoError(tx.ValidateBasic()) -} - -func (s *IntegrationTestSuite) TestGenTxCmdPubkey() { - val := s.network.Validators[0] - dir := s.T().TempDir() - - cmd := cli.GenTxCmd( - simapp.ModuleBasics, - val.ClientCtx.TxConfig, - banktypes.GenesisBalancesIterator{}, - val.ClientCtx.HomeDir, - ) - - _, out := testutil.ApplyMockIO(cmd) - clientCtx := val.ClientCtx.WithOutput(out) - ctx := context.Background() - ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx) - - amount := sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(12)) - genTxFile := filepath.Join(dir, "myTx") - - cmd.SetArgs([]string{ - fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), - fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, genTxFile), - fmt.Sprintf("--%s={\"key\":\"BOIkjkFruMpfOFC9oNPhiJGfmY2pHF/gwHdLDLnrnS0=\"}", stakingcli.FlagPubKey), - val.Moniker, - amount.String(), - }) - s.Require().Error(cmd.ExecuteContext(ctx)) - - cmd.SetArgs([]string{ - fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), - fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, genTxFile), - fmt.Sprintf("--%s={\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"BOIkjkFruMpfOFC9oNPhiJGfmY2pHF/gwHdLDLnrnS0=\"}", stakingcli.FlagPubKey), - val.Moniker, - amount.String(), - }) - s.Require().NoError(cmd.ExecuteContext(ctx)) + tests := []struct { + name string + args []string + expError bool + }{ + { + name: "invalid commission rate returns error", + args: []string{ + fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), + fmt.Sprintf("--%s=1", stakingcli.FlagCommissionRate), + val.Moniker, + amount.String(), + }, + expError: true, + }, + { + name: "valid gentx", + args: []string{ + fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), + val.Moniker, + amount.String(), + }, + expError: false, + }, + { + name: "invalid pubkey", + args: []string{ + fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), + fmt.Sprintf("--%s={\"key\":\"BOIkjkFruMpfOFC9oNPhiJGfmY2pHF/gwHdLDLnrnS0=\"}", stakingcli.FlagPubKey), + val.Moniker, + amount.String(), + }, + expError: true, + }, + { + name: "valid pubkey flag", + args: []string{ + fmt.Sprintf("--%s=%s", flags.FlagChainID, s.network.Config.ChainID), + fmt.Sprintf("--%s={\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"BOIkjkFruMpfOFC9oNPhiJGfmY2pHF/gwHdLDLnrnS0=\"}", stakingcli.FlagPubKey), + val.Moniker, + amount.String(), + }, + expError: false, + }, + } + + for _, tc := range tests { + tc := tc + + dir := s.T().TempDir() + genTxFile := filepath.Join(dir, "myTx") + tc.args = append(tc.args, fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, genTxFile)) + + s.Run(tc.name, func() { + cmd := cli.GenTxCmd( + simapp.ModuleBasics, + val.ClientCtx.TxConfig, + banktypes.GenesisBalancesIterator{}, + val.ClientCtx.HomeDir) + + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args) + + if tc.expError { + s.Require().Error(err) + + _, err = os.Open(genTxFile) + s.Require().Error(err) + } else { + s.Require().NoError(err, "test: %s\noutput: %s", tc.name, out.String()) + + // validate generated transaction. + open, err := os.Open(genTxFile) + s.Require().NoError(err) + + all, err := io.ReadAll(open) + s.Require().NoError(err) + + tx, err := val.ClientCtx.TxConfig.TxJSONDecoder()(all) + s.Require().NoError(err) + + msgs := tx.GetMsgs() + s.Require().Len(msgs, 1) + + s.Require().Equal(sdk.MsgTypeURL(&types.MsgCreateValidator{}), sdk.MsgTypeURL(msgs[0])) + s.Require().True(val.Address.Equals(msgs[0].GetSigners()[0])) + s.Require().Equal(amount, msgs[0].(*types.MsgCreateValidator).Value) + s.Require().NoError(tx.ValidateBasic()) + } + }) + } } diff --git a/x/genutil/gentx_test.go b/x/genutil/gentx_test.go index b08ecb9750f2..cf70d84bef38 100644 --- a/x/genutil/gentx_test.go +++ b/x/genutil/gentx_test.go @@ -65,7 +65,7 @@ func (suite *GenTxTestSuite) setAccountBalance(addr sdk.AccAddress, amount int64 acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr) suite.app.AccountKeeper.SetAccount(suite.ctx, acc) - err := testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 25)}) + err := testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, amount)}) suite.Require().NoError(err) bankGenesisState := suite.app.BankKeeper.ExportGenesis(suite.ctx) @@ -231,7 +231,7 @@ func (suite *GenTxTestSuite) TestDeliverGenTxs() { "success", func() { _ = suite.setAccountBalance(addr1, 50) - _ = suite.setAccountBalance(addr2, 0) + _ = suite.setAccountBalance(addr2, 1) msg := banktypes.NewMsgSend(addr1, addr2, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 1)}) tx, err := helpers.GenTx(