From 43cf8ebb2b9f05313f434b5cd58bfddf5d06413f Mon Sep 17 00:00:00 2001 From: atheeshp <59333759+atheeshp@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:47:51 +0530 Subject: [PATCH] fix: `--max-msgs` with `generate-only` should not throw error (#10842) ## Description Closes: #10841 --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) --- CHANGELOG.md | 1 + x/distribution/client/cli/tx.go | 2 +- x/distribution/client/testutil/cli_test.go | 12 +- .../client/testutil/grpc_query_suite.go | 6 + x/distribution/client/testutil/suite.go | 50 ++++--- .../client/testutil/withdraw_all_suite.go | 123 ++++++++++++++++++ 6 files changed, 170 insertions(+), 24 deletions(-) create mode 100644 x/distribution/client/testutil/withdraw_all_suite.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 54a6b6c18..77e819e84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,6 +188,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ibc-denom. * [\#10593](https://github.com/cosmos/cosmos-sdk/pull/10593) Update swagger-ui to v4.1.0 to fix xss vulnerability. * [\#10674](https://github.com/cosmos/cosmos-sdk/pull/10674) Fix issue with `Error.Wrap` and `Error.Wrapf` usage with `errors.Is`. +* [\#10842](https://github.com/cosmos/cosmos-sdk/pull/10842) Fix error when `--generate-only`, `--max-msgs` fags set while executing `WithdrawAllRewards` command. * [\#10897](https://github.com/cosmos/cosmos-sdk/pull/10897) Fix: set a non-zero value on gas overflow. * [#9790](https://github.com/cosmos/cosmos-sdk/pull/10687) Fix behavior of `DecCoins.MulDecTruncate`. diff --git a/x/distribution/client/cli/tx.go b/x/distribution/client/cli/tx.go index d9ec9feee..9971bc938 100644 --- a/x/distribution/client/cli/tx.go +++ b/x/distribution/client/cli/tx.go @@ -168,7 +168,7 @@ $ %[1]s tx distribution withdraw-all-rewards --from mykey } chunkSize, _ := cmd.Flags().GetInt(FlagMaxMessagesPerTx) - if clientCtx.BroadcastMode != flags.BroadcastBlock && chunkSize > 0 { + if !clientCtx.GenerateOnly && clientCtx.BroadcastMode != flags.BroadcastBlock && chunkSize > 0 { return fmt.Errorf("cannot use broadcast mode %[1]s with %[2]s != 0", clientCtx.BroadcastMode, FlagMaxMessagesPerTx) } diff --git a/x/distribution/client/testutil/cli_test.go b/x/distribution/client/testutil/cli_test.go index 742c1eb68..d7ab22d41 100644 --- a/x/distribution/client/testutil/cli_test.go +++ b/x/distribution/client/testutil/cli_test.go @@ -1,21 +1,19 @@ -// +build norace - package testutil import ( "testing" - "github.com/cosmos/cosmos-sdk/testutil/network" - "github.com/stretchr/testify/suite" ) func TestIntegrationTestSuite(t *testing.T) { - cfg := network.DefaultConfig() - cfg.NumValidators = 1 - suite.Run(t, NewIntegrationTestSuite(cfg)) + suite.Run(t, new(IntegrationTestSuite)) } func TestGRPCQueryTestSuite(t *testing.T) { suite.Run(t, new(GRPCQueryTestSuite)) } + +func TestWithdrawAllSuite(t *testing.T) { + suite.Run(t, new(WithdrawAllTestSuite)) +} diff --git a/x/distribution/client/testutil/grpc_query_suite.go b/x/distribution/client/testutil/grpc_query_suite.go index b36e0d1d7..956449a39 100644 --- a/x/distribution/client/testutil/grpc_query_suite.go +++ b/x/distribution/client/testutil/grpc_query_suite.go @@ -37,6 +37,12 @@ func (s *GRPCQueryTestSuite) SetupSuite() { s.Require().NoError(err) } +// TearDownSuite cleans up the curret test network after _each_ test. +func (s *GRPCQueryTestSuite) TearDownSuite() { + s.T().Log("tearing down integration test suite1") + s.network.Cleanup() +} + func (s *GRPCQueryTestSuite) TestQueryParamsGRPC() { val := s.network.Validators[0] baseURL := val.APIAddress diff --git a/x/distribution/client/testutil/suite.go b/x/distribution/client/testutil/suite.go index 3be8d5410..07fe6e50d 100644 --- a/x/distribution/client/testutil/suite.go +++ b/x/distribution/client/testutil/suite.go @@ -29,13 +29,17 @@ func NewIntegrationTestSuite(cfg network.Config) *IntegrationTestSuite { return &IntegrationTestSuite{cfg: cfg} } -// SetupTest creates a new network for _each_ integration test. We create a new +// SetupSuite creates a new network for _each_ integration test. We create a new // network for each test because there are some state modifications that are // needed to be made in order to make useful queries. However, we don't want // these state changes to be present in other tests. -func (s *IntegrationTestSuite) SetupTest() { +func (s *IntegrationTestSuite) SetupSuite() { s.T().Log("setting up integration test suite") + cfg := network.DefaultConfig() + cfg.NumValidators = 1 + s.cfg = cfg + genesisState := s.cfg.GenesisState var mintData minttypes.GenesisState s.Require().NoError(s.cfg.Codec.UnmarshalJSON(genesisState[minttypes.ModuleName], &mintData)) @@ -57,9 +61,9 @@ func (s *IntegrationTestSuite) SetupTest() { s.Require().NoError(err) } -// TearDownTest cleans up the curret test network after _each_ test. -func (s *IntegrationTestSuite) TearDownTest() { - s.T().Log("tearing down integration test suite") +// TearDownSuite cleans up the curret test network after _each_ test. +func (s *IntegrationTestSuite) TearDownSuite() { + s.T().Log("tearing down integration test suite1") s.network.Cleanup() } @@ -129,7 +133,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorOutstandingRewards() { fmt.Sprintf("--%s=json", tmcli.OutputFlag), }, false, - `{"rewards":[{"denom":"stake","amount":"1164.240000000000000000"}]}`, + `{"rewards":[{"denom":"stake","amount":"232.260000000000000000"}]}`, }, { "text output", @@ -140,7 +144,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorOutstandingRewards() { }, false, `rewards: -- amount: "1164.240000000000000000" +- amount: "232.260000000000000000" denom: stake`, }, } @@ -192,7 +196,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorCommission() { fmt.Sprintf("--%s=json", tmcli.OutputFlag), }, false, - `{"commission":[{"denom":"stake","amount":"464.520000000000000000"}]}`, + `{"commission":[{"denom":"stake","amount":"116.130000000000000000"}]}`, }, { "text output", @@ -203,7 +207,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorCommission() { }, false, `commission: -- amount: "464.520000000000000000" +- amount: "116.130000000000000000" denom: stake`, }, } @@ -345,7 +349,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryDelegatorRewards() { fmt.Sprintf("--%s=json", tmcli.OutputFlag), }, false, - fmt.Sprintf(`{"rewards":[{"validator_address":"%s","reward":[{"denom":"stake","amount":"387.100000000000000000"}]}],"total":[{"denom":"stake","amount":"387.100000000000000000"}]}`, valAddr.String()), + fmt.Sprintf(`{"rewards":[{"validator_address":"%s","reward":[{"denom":"stake","amount":"193.550000000000000000"}]}],"total":[{"denom":"stake","amount":"193.550000000000000000"}]}`, valAddr.String()), }, { "json output (specific validator)", @@ -355,7 +359,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryDelegatorRewards() { fmt.Sprintf("--%s=json", tmcli.OutputFlag), }, false, - `{"rewards":[{"denom":"stake","amount":"387.100000000000000000"}]}`, + `{"rewards":[{"denom":"stake","amount":"193.550000000000000000"}]}`, }, { "text output", @@ -367,11 +371,11 @@ func (s *IntegrationTestSuite) TestGetCmdQueryDelegatorRewards() { false, fmt.Sprintf(`rewards: - reward: - - amount: "387.100000000000000000" + - amount: "193.550000000000000000" denom: stake validator_address: %s total: -- amount: "387.100000000000000000" +- amount: "193.550000000000000000" denom: stake`, valAddr.String()), }, { @@ -383,7 +387,7 @@ total: }, false, `rewards: -- amount: "387.100000000000000000" +- amount: "193.550000000000000000" denom: stake`, }, } @@ -685,7 +689,21 @@ func (s *IntegrationTestSuite) TestGetCmdSubmitProposal() { "deposit": -324foocoin }` + // fund some tokens to the community pool + args := []string{sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(5431))).String(), + fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String())} + invalidPropFile := testutil.WriteToNewTempFile(s.T(), invalidProp) + cmd := cli.NewFundCommunityPoolCmd() + out, err := clitestutil.ExecTestCLICmd(val.ClientCtx, cmd, args) + s.Require().NoError(err) + + var txResp sdk.TxResponse + s.Require().NoError(val.ClientCtx.Codec.UnmarshalJSON(out.Bytes(), &txResp), out.String()) + s.Require().Equal(uint32(0), txResp.Code) validProp := fmt.Sprintf(`{ "title": "Community Pool Spend", @@ -709,7 +727,7 @@ func (s *IntegrationTestSuite) TestGetCmdSubmitProposal() { invalidPropFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), }, true, 0, nil, @@ -720,7 +738,7 @@ func (s *IntegrationTestSuite) TestGetCmdSubmitProposal() { validPropFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), // sync mode as there are no funds yet + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), }, false, 0, &sdk.TxResponse{}, diff --git a/x/distribution/client/testutil/withdraw_all_suite.go b/x/distribution/client/testutil/withdraw_all_suite.go new file mode 100644 index 000000000..7b459a4df --- /dev/null +++ b/x/distribution/client/testutil/withdraw_all_suite.go @@ -0,0 +1,123 @@ +package testutil + +import ( + "fmt" + "strings" + + "github.com/cosmos/cosmos-sdk/client/flags" + "github.com/cosmos/cosmos-sdk/crypto/hd" + "github.com/cosmos/cosmos-sdk/crypto/keyring" + clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli" + "github.com/cosmos/cosmos-sdk/testutil/network" + sdk "github.com/cosmos/cosmos-sdk/types" + banktestutil "github.com/cosmos/cosmos-sdk/x/bank/client/testutil" + "github.com/cosmos/cosmos-sdk/x/distribution/client/cli" + stakingcli "github.com/cosmos/cosmos-sdk/x/staking/client/cli" + "github.com/stretchr/testify/suite" +) + +type WithdrawAllTestSuite struct { + suite.Suite + + cfg network.Config + network *network.Network +} + +func (s *WithdrawAllTestSuite) SetupSuite() { + cfg := network.DefaultConfig() + cfg.NumValidators = 2 + s.cfg = cfg + + s.T().Log("setting up integration test suite") + network, err := network.New(s.T(), s.T().TempDir(), s.cfg) + s.Require().NoError(err) + s.network = network + + _, err = s.network.WaitForHeight(1) + s.Require().NoError(err) +} + +// TearDownSuite cleans up the curret test network after _each_ test. +func (s *WithdrawAllTestSuite) TearDownSuite() { + s.T().Log("tearing down integration test suite") + s.network.Cleanup() +} + +// This test requires multiple validators, if I add this test to `IntegrationTestSuite` by increasing +// `NumValidators` the existing tests are leading to non-determnism so created new suite for this test. +func (s *WithdrawAllTestSuite) TestNewWithdrawAllRewardsGenerateOnly() { + require := s.Require() + val := s.network.Validators[0] + val1 := s.network.Validators[1] + clientCtx := val.ClientCtx + + info, _, err := val.ClientCtx.Keyring.NewMnemonic("newAccount", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1) + require.NoError(err) + + pubkey, err := info.GetPubKey() + require.NoError(err) + + newAddr := sdk.AccAddress(pubkey.Address()) + _, err = banktestutil.MsgSendExec( + val.ClientCtx, + val.Address, + newAddr, + sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(2000))), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + ) + require.NoError(err) + + // delegate 500 tokens to validator1 + args := []string{ + val.ValAddress.String(), + sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(500)).String(), + fmt.Sprintf("--%s=%s", flags.FlagFrom, newAddr.String()), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + } + cmd := stakingcli.NewDelegateCmd() + _, err = clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + require.NoError(err) + + // delegate 500 tokens to validator2 + args = []string{ + val1.ValAddress.String(), + sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(500)).String(), + fmt.Sprintf("--%s=%s", flags.FlagFrom, newAddr.String()), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + } + _, err = clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + require.NoError(err) + + args = []string{ + fmt.Sprintf("--%s=%s", flags.FlagFrom, newAddr.String()), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), + fmt.Sprintf("--%s=1", cli.FlagMaxMessagesPerTx), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + } + cmd = cli.NewWithdrawAllRewardsCmd() + out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + require.NoError(err) + // expect 2 transactions in the generated file when --max-msgs in a tx set 1. + s.Require().Equal(2, len(strings.Split(strings.Trim(out.String(), "\n"), "\n"))) + + args = []string{ + fmt.Sprintf("--%s=%s", flags.FlagFrom, newAddr.String()), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), + fmt.Sprintf("--%s=2", cli.FlagMaxMessagesPerTx), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + } + cmd = cli.NewWithdrawAllRewardsCmd() + out, err = clitestutil.ExecTestCLICmd(clientCtx, cmd, args) + require.NoError(err) + // expect 1 transaction in the generated file when --max-msgs in a tx set 2, since there are only delegations. + s.Require().Equal(1, len(strings.Split(strings.Trim(out.String(), "\n"), "\n"))) +}