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: --max-msgs with generate-only should not throw error #10842

Merged
merged 31 commits into from
Jan 18, 2022
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
e4c093d
fix: max msgs (WIP)
atheeshp Dec 14, 2021
ce38f34
Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/max-msg…
atheeshp Dec 14, 2021
7620a56
Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/max-msg…
atheeshp Dec 27, 2021
267ed83
add tests
atheeshp Dec 27, 2021
bdf7046
fix tests
atheeshp Dec 28, 2021
11b3014
Merge branch 'master' into ap/max-msgs-all-rewards
atheeshp Jan 3, 2022
9912321
Merge branch 'master' into ap/max-msgs-all-rewards
alexanderbez Jan 3, 2022
daa339a
Merge branch 'master' into ap/max-msgs-all-rewards
atheeshp Jan 5, 2022
cbe7eff
fix tess
atheeshp Jan 5, 2022
986eeb2
Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/max-msg…
atheeshp Jan 5, 2022
32d4a88
Merge branch 'ap/max-msgs-all-rewards' of github.com:cosmos/cosmos-sd…
atheeshp Jan 5, 2022
3ec48fe
udpate tests
atheeshp Jan 6, 2022
5469613
update tests
atheeshp Jan 6, 2022
6101381
Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/max-msg…
atheeshp Jan 6, 2022
862fc09
update tests
atheeshp Jan 6, 2022
fa554db
update changelog
atheeshp Jan 6, 2022
bb03e6d
update tests
atheeshp Jan 6, 2022
967766e
revert tests
atheeshp Jan 6, 2022
24e4a3a
udpate tests
atheeshp Jan 6, 2022
d6a72d2
Merge branch 'master' into ap/max-msgs-all-rewards
atheeshp Jan 6, 2022
44c9361
Merge branch 'master' into ap/max-msgs-all-rewards
atheeshp Jan 6, 2022
a23e0ec
update tests
atheeshp Jan 6, 2022
c8adb66
Merge branch 'ap/max-msgs-all-rewards' of github.com:cosmos/cosmos-sd…
atheeshp Jan 6, 2022
0340a08
fix tests
atheeshp Jan 7, 2022
2c8ad69
Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/max-msg…
atheeshp Jan 7, 2022
6ad9160
add separate suite for withdraw all msg
atheeshp Jan 7, 2022
410800e
Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/max-msg…
atheeshp Jan 8, 2022
bf2d705
fix tests
atheeshp Jan 8, 2022
e7621c8
Merge branch 'master' into ap/max-msgs-all-rewards
atheeshp Jan 11, 2022
b765c02
Merge branch 'master' into ap/max-msgs-all-rewards
alexanderbez Jan 17, 2022
16d7ebd
Merge branch 'master' into ap/max-msgs-all-rewards
mergify[bot] Jan 18, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

### State Machine Breaking
Expand Down
2 changes: 1 addition & 1 deletion x/distribution/client/cli/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
12 changes: 5 additions & 7 deletions x/distribution/client/testutil/cli_test.go
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Contributor Author

@atheeshp atheeshp Jan 7, 2022

Choose a reason for hiding this comment

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

Added a new suite here, since there are few tests are failing if I change NumValidators = 2 in existing integration suite (seems like non-determinism) and couldn't test them on local machine since the tests are keep on throwing timeout error.

suite.Run(t, new(WithdrawAllTestSuite))
}
6 changes: 6 additions & 0 deletions x/distribution/client/testutil/grpc_query_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
50 changes: 34 additions & 16 deletions x/distribution/client/testutil/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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()
}

Expand Down Expand Up @@ -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",
Expand All @@ -140,7 +144,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorOutstandingRewards() {
},
false,
`rewards:
- amount: "1164.240000000000000000"
- amount: "232.260000000000000000"
denom: stake`,
},
}
Expand Down Expand Up @@ -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",
Expand All @@ -203,7 +207,7 @@ func (s *IntegrationTestSuite) TestGetCmdQueryValidatorCommission() {
},
false,
`commission:
- amount: "464.520000000000000000"
- amount: "116.130000000000000000"
denom: stake`,
},
}
Expand Down Expand Up @@ -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)",
Expand All @@ -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",
Expand All @@ -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()),
},
{
Expand All @@ -383,7 +387,7 @@ total:
},
false,
`rewards:
- amount: "387.100000000000000000"
- amount: "193.550000000000000000"
denom: stake`,
},
}
Expand Down Expand Up @@ -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",
Expand All @@ -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),
Copy link
Contributor Author

@atheeshp atheeshp Jan 8, 2022

Choose a reason for hiding this comment

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

updated this (made changes accordingly), because some times tests are getting timeout with BroadcastSync.

fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
},
true, 0, nil,
Expand All @@ -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{},
Expand Down
123 changes: 123 additions & 0 deletions x/distribution/client/testutil/withdraw_all_suite.go
Original file line number Diff line number Diff line change
@@ -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")))
}