From 405ca6e56db160ba75448be776c095daef7b05ae Mon Sep 17 00:00:00 2001 From: atheesh Date: Thu, 13 Apr 2023 17:09:36 +0530 Subject: [PATCH 1/9] refactor(x/gov): move `ValidateBasic` to msg server --- x/gov/keeper/msg_server.go | 121 ++++++++ x/gov/keeper/msg_server_test.go | 499 +++++++++++++++++++++++++++++-- x/gov/types/v1/msgs.go | 137 --------- x/gov/types/v1/msgs_test.go | 135 --------- x/gov/types/v1beta1/msgs.go | 93 ------ x/gov/types/v1beta1/msgs_test.go | 135 --------- 6 files changed, 594 insertions(+), 526 deletions(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index eb94ecb18312..234cc348cad3 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -6,10 +6,13 @@ import ( "strconv" "cosmossdk.io/errors" + "cosmossdk.io/math" "github.com/armon/go-metrics" "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/x/gov/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" @@ -29,6 +32,48 @@ var _ v1.MsgServer = msgServer{} // SubmitProposal implements the MsgServer.SubmitProposal method. func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitProposal) (*v1.MsgSubmitProposalResponse, error) { + if msg.Title == "" { + return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "proposal title cannot be empty") + } + if msg.Summary == "" { + return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "proposal summary cannot be empty") + } + + if _, err := sdk.AccAddressFromBech32(msg.Proposer); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) + } + + deposit := sdk.NewCoins(msg.InitialDeposit...) + if !deposit.IsValid() { + return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) + } + + if deposit.IsAnyNegative() { + return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) + } + + // Check that either metadata or Msgs length is non nil. + if len(msg.Messages) == 0 && len(msg.Metadata) == 0 { + return nil, errors.Wrap(types.ErrNoProposalMsgs, "either metadata or Msgs length must be non-nil") + } + + msgs, err := msg.GetMsgs() + if err != nil { + return nil, err + } + + for idx, msg := range msgs { + m, ok := msg.(sdk.HasValidateBasic) + if !ok { + continue + } + + if err := m.ValidateBasic(); err != nil { + return nil, errors.Wrap(types.ErrInvalidProposalMsg, + fmt.Sprintf("msg: %d, err: %s", idx, err.Error())) + } + } + ctx := sdk.UnwrapSDKContext(goCtx) initialDeposit := msg.GetInitialDeposit() @@ -85,6 +130,10 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos // CancelProposals implements the MsgServer.CancelProposal method. func (k msgServer) CancelProposal(goCtx context.Context, msg *v1.MsgCancelProposal) (*v1.MsgCancelProposalResponse, error) { + if _, err := sdk.AccAddressFromBech32(msg.Proposer); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) + } + ctx := sdk.UnwrapSDKContext(goCtx) _, err := k.authKeeper.StringToBytes(msg.Proposer) if err != nil { @@ -139,6 +188,14 @@ func (k msgServer) ExecLegacyContent(goCtx context.Context, msg *v1.MsgExecLegac // Vote implements the MsgServer.Vote method. func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResponse, error) { + if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) + } + + if !v1.ValidVoteOption(msg.Option) { + return nil, errors.Wrap(types.ErrInvalidVote, msg.Option.String()) + } + ctx := sdk.UnwrapSDKContext(goCtx) accAddr, err := k.authKeeper.StringToBytes(msg.Voter) if err != nil { @@ -162,6 +219,38 @@ func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResp // VoteWeighted implements the MsgServer.VoteWeighted method. func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) (*v1.MsgVoteWeightedResponse, error) { + if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) + } + if len(msg.Options) == 0 { + return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, v1.WeightedVoteOptions(msg.Options).String()) + } + + totalWeight := math.LegacyNewDec(0) + usedOptions := make(map[v1.VoteOption]bool) + for _, option := range msg.Options { + if !option.IsValid() { + return nil, errors.Wrap(types.ErrInvalidVote, option.String()) + } + weight, err := sdk.NewDecFromStr(option.Weight) + if err != nil { + return nil, errors.Wrapf(types.ErrInvalidVote, "invalid weight: %s", err) + } + totalWeight = totalWeight.Add(weight) + if usedOptions[option.Option] { + return nil, errors.Wrap(types.ErrInvalidVote, "duplicated vote option") + } + usedOptions[option.Option] = true + } + + if totalWeight.GT(math.LegacyNewDec(1)) { + return nil, errors.Wrap(types.ErrInvalidVote, "total weight overflow 1.00") + } + + if totalWeight.LT(math.LegacyNewDec(1)) { + return nil, errors.Wrap(types.ErrInvalidVote, "total weight lower than 1.00") + } + ctx := sdk.UnwrapSDKContext(goCtx) accAddr, accErr := k.authKeeper.StringToBytes(msg.Voter) if accErr != nil { @@ -185,6 +274,19 @@ func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) // Deposit implements the MsgServer.Deposit method. func (k msgServer) Deposit(goCtx context.Context, msg *v1.MsgDeposit) (*v1.MsgDepositResponse, error) { + if _, err := sdk.AccAddressFromBech32(msg.Depositor); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid depositor address: %s", err) + } + + amount := sdk.NewCoins(msg.Amount...) + if !amount.IsValid() { + return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, amount.String()) + } + + if amount.IsAnyNegative() { + return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, amount.String()) + } + ctx := sdk.UnwrapSDKContext(goCtx) accAddr, err := k.authKeeper.StringToBytes(msg.Depositor) if err != nil { @@ -221,6 +323,14 @@ func (k msgServer) UpdateParams(goCtx context.Context, msg *v1.MsgUpdateParams) return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority) } + if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err) + } + + if err := msg.Params.ValidateBasic(); err != nil { + return nil, err + } + ctx := sdk.UnwrapSDKContext(goCtx) if err := k.SetParams(ctx, msg.Params); err != nil { return nil, err @@ -243,6 +353,17 @@ func NewLegacyMsgServerImpl(govAcct string, v1Server v1.MsgServer) v1beta1.MsgSe var _ v1beta1.MsgServer = legacyMsgServer{} func (k legacyMsgServer) SubmitProposal(goCtx context.Context, msg *v1beta1.MsgSubmitProposal) (*v1beta1.MsgSubmitProposalResponse, error) { + content := msg.GetContent() + if content == nil { + return nil, errors.Wrap(types.ErrInvalidProposalContent, "missing content") + } + if !v1beta1.IsValidProposalType(content.ProposalType()) { + return nil, errors.Wrap(types.ErrInvalidProposalType, content.ProposalType()) + } + if err := content.ValidateBasic(); err != nil { + return nil, err + } + contentMsg, err := v1.NewLegacyContent(msg.GetContent(), k.govAcct) if err != nil { return nil, fmt.Errorf("error converting legacy content into proposal message: %w", err) diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index 4f6cd477c765..2d8f67c66ee3 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -5,6 +5,7 @@ import ( "strings" "time" + "cosmossdk.io/math" sdkmath "cosmossdk.io/math" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" "github.com/cosmos/cosmos-sdk/testutil/testdata" @@ -44,6 +45,66 @@ func (suite *KeeperTestSuite) TestSubmitProposalReq() { expErr bool expErrMsg string }{ + "invalid addr": { + preRun: func() (*v1.MsgSubmitProposal, error) { + return v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + initialDeposit, + "", + strings.Repeat("1", 300), + "Proposal", + "description of proposal", + false, + ) + }, + expErr: true, + expErrMsg: "invalid proposer address", + }, + "empty msgs and metadata": { + preRun: func() (*v1.MsgSubmitProposal, error) { + return v1.NewMsgSubmitProposal( + nil, + initialDeposit, + proposer.String(), + "", + "Proposal", + "description of proposal", + false, + ) + }, + expErr: true, + expErrMsg: "no messages proposed", + }, + "empty title": { + preRun: func() (*v1.MsgSubmitProposal, error) { + return v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + initialDeposit, + proposer.String(), + "", + "", + "description of proposal", + false, + ) + }, + expErr: true, + expErrMsg: "proposal title cannot be empty", + }, + "empty description": { + preRun: func() (*v1.MsgSubmitProposal, error) { + return v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + initialDeposit, + proposer.String(), + "", + "Proposal", + "", + false, + ) + }, + expErr: true, + expErrMsg: "proposal summary cannot be empty", + }, "metadata too long": { preRun: func() (*v1.MsgSubmitProposal, error) { return v1.NewMsgSubmitProposal( @@ -179,6 +240,7 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { cases := map[string]struct { preRun func() uint64 expErr bool + expErrMsg string proposalID uint64 depositor sdk.AccAddress }{ @@ -188,6 +250,7 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { }, depositor: proposer, expErr: true, + expErrMsg: "proposal is not found", }, "valid proposal but invalid proposer": { preRun: func() uint64 { @@ -195,6 +258,15 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { }, depositor: addrs[1], expErr: true, + expErrMsg: "invalid proposer", + }, + "empty proposer": { + preRun: func() uint64 { + return proposalID + }, + depositor: sdk.AccAddress{}, + expErr: true, + expErrMsg: "invalid proposer address: empty address string is not allowed", }, "all good": { preRun: func() uint64 { @@ -212,6 +284,7 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { _, err := suite.msgSrvr.CancelProposal(suite.ctx, cancelProposalReq) if tc.expErr { suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.expErrMsg) } else { suite.Require().NoError(err) } @@ -259,6 +332,26 @@ func (suite *KeeperTestSuite) TestVoteReq() { metadata string voter sdk.AccAddress }{ + "empty voter": { + preRun: func() uint64 { + return proposalID + }, + option: v1.VoteOption_VOTE_OPTION_YES, + voter: sdk.AccAddress{}, + metadata: "", + expErr: true, + expErrMsg: "invalid voter address", + }, + "wrong vote option": { + preRun: func() uint64 { + return proposalID + }, + option: v1.VoteOption(0x13), + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "invalid vote option", + }, "vote on inactive proposal": { preRun: func() uint64 { msg, err := v1.NewMsgSubmitProposal( @@ -346,8 +439,8 @@ func (suite *KeeperTestSuite) TestVoteReq() { func (suite *KeeperTestSuite) TestVoteWeightedReq() { suite.reset() govAcct := suite.govKeeper.GetGovernanceAccount(suite.ctx).GetAddress() - addrs := suite.addrs - proposer := addrs[0] + + proposer := simtestutil.AddTestAddrsIncremental(suite.bankKeeper, suite.stakingKeeper, suite.ctx, 1, sdkmath.NewInt(50000000))[0] coins := sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(100))) minDeposit := suite.govKeeper.GetParams(suite.ctx).MinDeposit @@ -380,10 +473,104 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { vote *v1.MsgVote expErr bool expErrMsg string - option v1.VoteOption + option v1.WeightedVoteOptions metadata string voter sdk.AccAddress }{ + "empty voter": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{ + v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(1)), + }, + voter: sdk.AccAddress{}, + metadata: "", + expErr: true, + expErrMsg: "invalid voter address", + }, + "weights sum > 1": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{ + v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(1)), + v1.NewWeightedVoteOption(v1.OptionAbstain, math.LegacyNewDec(1)), + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "total weight overflow 1.00: invalid vote option", + }, + "duplicate vote options": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{ + v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), + v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "duplicated vote option", + }, + "zero weight": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{ + v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(0)), + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: `option:VOTE_OPTION_YES weight:"0.000000000000000000" : invalid vote option`, + }, + "negative weight": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{ + v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(-1)), + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: `option:VOTE_OPTION_YES weight:"-1.000000000000000000" : invalid vote option`, + }, + "empty options": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{}, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "invalid request", + }, + "invalid vote option": { + preRun: func() uint64 { + return proposalID + }, + option: v1.NewNonSplitVoteOption(v1.VoteOption(0x13)), + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "invalid vote option", + }, + "weight sum < 1": { + preRun: func() uint64 { + return proposalID + }, + option: v1.WeightedVoteOptions{ // weight sum <1 + v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "total weight lower than 1.00: invalid vote option", + }, "vote on inactive proposal": { preRun: func() uint64 { msg, err := v1.NewMsgSubmitProposal( @@ -402,7 +589,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { suite.Require().NotNil(res.ProposalId) return res.ProposalId }, - option: v1.VoteOption_VOTE_OPTION_YES, + option: v1.NewNonSplitVoteOption(v1.VoteOption_VOTE_OPTION_YES), voter: proposer, metadata: "", expErr: true, @@ -412,7 +599,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { preRun: func() uint64 { return proposalID }, - option: v1.VoteOption_VOTE_OPTION_YES, + option: v1.NewNonSplitVoteOption(v1.VoteOption_VOTE_OPTION_YES), voter: proposer, metadata: strings.Repeat("a", 300), expErr: true, @@ -422,7 +609,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { preRun: func() uint64 { return proposalID }, - option: v1.VoteOption_VOTE_OPTION_YES, + option: v1.NewNonSplitVoteOption(v1.VoteOption_VOTE_OPTION_YES), voter: sdk.AccAddress(strings.Repeat("a", 300)), metadata: "", expErr: true, @@ -446,7 +633,33 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { suite.Require().NotNil(res.ProposalId) return res.ProposalId }, - option: v1.VoteOption_VOTE_OPTION_YES, + option: v1.NewNonSplitVoteOption(v1.VoteOption_VOTE_OPTION_YES), + voter: proposer, + metadata: "", + expErr: false, + }, + "all good with split votes": { + preRun: func() uint64 { + msg, err := v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + minDeposit, + proposer.String(), + "", + "Proposal", + "description of proposal", + false, + ) + suite.Require().NoError(err) + + res, err := suite.msgSrvr.SubmitProposal(suite.ctx, msg) + suite.Require().NoError(err) + suite.Require().NotNil(res.ProposalId) + return res.ProposalId + }, + option: v1.WeightedVoteOptions{ + v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), + v1.NewWeightedVoteOption(v1.OptionAbstain, sdk.NewDecWithPrec(5, 1)), + }, voter: proposer, metadata: "", expErr: false, @@ -456,7 +669,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { for name, tc := range cases { suite.Run(name, func() { pID := tc.preRun() - voteReq := v1.NewMsgVoteWeighted(tc.voter, pID, v1.NewNonSplitVoteOption(tc.option), tc.metadata) + voteReq := v1.NewMsgVoteWeighted(tc.voter, pID, tc.option, tc.metadata) _, err := suite.msgSrvr.VoteWeighted(suite.ctx, voteReq) if tc.expErr { suite.Require().Error(err) @@ -503,7 +716,7 @@ func (suite *KeeperTestSuite) TestDepositReq() { proposalID uint64 depositor sdk.AccAddress deposit sdk.Coins - options v1.WeightedVoteOptions + expErrMsg string }{ "wrong proposal id": { preRun: func() uint64 { @@ -512,7 +725,16 @@ func (suite *KeeperTestSuite) TestDepositReq() { depositor: proposer, deposit: coins, expErr: true, - options: v1.NewNonSplitVoteOption(v1.OptionYes), + expErrMsg: "0: unknown proposal", + }, + "empty depositor": { + preRun: func() uint64 { + return pID + }, + depositor: sdk.AccAddress{}, + deposit: minDeposit, + expErr: true, + expErrMsg: "invalid depositor address", }, "all good": { preRun: func() uint64 { @@ -521,7 +743,6 @@ func (suite *KeeperTestSuite) TestDepositReq() { depositor: proposer, deposit: minDeposit, expErr: false, - options: v1.NewNonSplitVoteOption(v1.OptionYes), }, } @@ -532,6 +753,7 @@ func (suite *KeeperTestSuite) TestDepositReq() { _, err := suite.msgSrvr.Deposit(suite.ctx, depositReq) if tc.expErr { suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.expErrMsg) } else { suite.Require().NoError(err) } @@ -549,9 +771,70 @@ func (suite *KeeperTestSuite) TestLegacyMsgSubmitProposal() { minDeposit := suite.govKeeper.GetParams(suite.ctx).MinDeposit cases := map[string]struct { - preRun func() (*v1beta1.MsgSubmitProposal, error) - expErr bool + preRun func() (*v1beta1.MsgSubmitProposal, error) + expErr bool + expErrMsg string }{ + "empty title": { + preRun: func() (*v1beta1.MsgSubmitProposal, error) { + content := v1beta1.NewTextProposal("", "I am test") + return v1beta1.NewMsgSubmitProposal( + content, + initialDeposit, + proposer, + ) + }, + expErr: true, + expErrMsg: "proposal title cannot be blank", + }, + "empty description": { + preRun: func() (*v1beta1.MsgSubmitProposal, error) { + content := v1beta1.NewTextProposal("test", "") + return v1beta1.NewMsgSubmitProposal( + content, + initialDeposit, + proposer, + ) + }, + expErr: true, + expErrMsg: "proposal description cannot be blank", + }, + "empty proposer": { + preRun: func() (*v1beta1.MsgSubmitProposal, error) { + content := v1beta1.NewTextProposal("test", "I am test") + return v1beta1.NewMsgSubmitProposal( + content, + initialDeposit, + sdk.AccAddress{}, + ) + }, + expErr: true, + expErrMsg: "invalid proposer address: empty address string is not allowed", + }, + "title text length > max limit allowed": { + preRun: func() (*v1beta1.MsgSubmitProposal, error) { + content := v1beta1.NewTextProposal(strings.Repeat("#", v1beta1.MaxTitleLength*2), "I am test") + return v1beta1.NewMsgSubmitProposal( + content, + initialDeposit, + proposer, + ) + }, + expErr: true, + expErrMsg: "proposal title is longer than max length of 140: invalid proposal content", + }, + "description text length > max limit allowed": { + preRun: func() (*v1beta1.MsgSubmitProposal, error) { + content := v1beta1.NewTextProposal("test", strings.Repeat("#", v1beta1.MaxDescriptionLength*2)) + return v1beta1.NewMsgSubmitProposal( + content, + initialDeposit, + proposer, + ) + }, + expErr: true, + expErrMsg: "proposal description is longer than max length of 10000: invalid proposal content", + }, "all good": { preRun: func() (*v1beta1.MsgSubmitProposal, error) { return v1beta1.NewMsgSubmitProposal( @@ -581,6 +864,7 @@ func (suite *KeeperTestSuite) TestLegacyMsgSubmitProposal() { res, err := suite.legacyMsgSrvr.SubmitProposal(suite.ctx, msg) if c.expErr { suite.Require().Error(err) + suite.Require().Contains(err.Error(), c.expErrMsg) } else { suite.Require().NoError(err) suite.Require().NotNil(res.ProposalId) @@ -628,6 +912,26 @@ func (suite *KeeperTestSuite) TestLegacyMsgVote() { metadata string voter sdk.AccAddress }{ + "empty voter": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.OptionYes, + voter: sdk.AccAddress{}, + metadata: "", + expErr: true, + expErrMsg: "invalid voter address", + }, + "wrong vote option": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.VoteOption(0x13), + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "invalid vote option", + }, "vote on inactive proposal": { preRun: func() uint64 { msg, err := v1.NewMsgSubmitProposal( @@ -739,10 +1043,133 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { vote *v1beta1.MsgVote expErr bool expErrMsg string - option v1beta1.VoteOption + option v1beta1.WeightedVoteOptions metadata string voter sdk.AccAddress }{ + "empty voter": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(1), + }, + }, + voter: sdk.AccAddress{}, + metadata: "", + expErr: true, + expErrMsg: "invalid voter address", + }, + "weights sum > 1": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(1), + }, + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionAbstain, + Weight: math.LegacyNewDec(1), + }, + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "total weight overflow 1.00: invalid vote option", + }, + "duplicate vote options": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: sdk.NewDecWithPrec(5, 1), + }, + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: sdk.NewDecWithPrec(5, 1), + }, + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "duplicated vote option", + }, + "zero weight": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(0), + }, + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: `option:VOTE_OPTION_YES weight:"0.000000000000000000" : invalid vote option`, + }, + "negative weight": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(-1), + }, + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: `option:VOTE_OPTION_YES weight:"-1.000000000000000000" : invalid vote option`, + }, + "empty options": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{}, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "invalid request", + }, + "invalid vote option": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.VoteOption(0x13), + Weight: sdk.NewDecWithPrec(5, 1), + }, + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "invalid vote option", + }, + "weight sum < 1": { + preRun: func() uint64 { + return proposalID + }, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: sdk.NewDecWithPrec(5, 1), + }, + }, + voter: proposer, + metadata: "", + expErr: true, + expErrMsg: "total weight lower than 1.00: invalid vote option", + }, "vote on inactive proposal": { preRun: func() uint64 { msg, err := v1.NewMsgSubmitProposal( @@ -761,7 +1188,12 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { suite.Require().NotNil(res.ProposalId) return res.ProposalId }, - option: v1beta1.OptionYes, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(1), + }, + }, voter: proposer, metadata: "", expErr: true, @@ -771,7 +1203,12 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { preRun: func() uint64 { return proposalID }, - option: v1beta1.OptionYes, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(1), + }, + }, voter: sdk.AccAddress(strings.Repeat("a", 300)), metadata: "", expErr: true, @@ -795,7 +1232,12 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { suite.Require().NotNil(res.ProposalId) return res.ProposalId }, - option: v1beta1.OptionYes, + option: v1beta1.WeightedVoteOptions{ + v1beta1.WeightedVoteOption{ + Option: v1beta1.OptionYes, + Weight: math.LegacyNewDec(1), + }, + }, voter: proposer, metadata: "", expErr: false, @@ -805,7 +1247,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { for name, tc := range cases { suite.Run(name, func() { pID := tc.preRun() - voteReq := v1beta1.NewMsgVoteWeighted(tc.voter, pID, v1beta1.NewNonSplitVoteOption(tc.option)) + voteReq := v1beta1.NewMsgVoteWeighted(tc.voter, pID, tc.option) _, err := suite.legacyMsgSrvr.VoteWeighted(suite.ctx, voteReq) if tc.expErr { suite.Require().Error(err) @@ -849,10 +1291,10 @@ func (suite *KeeperTestSuite) TestLegacyMsgDeposit() { cases := map[string]struct { preRun func() uint64 expErr bool + expErrMsg string proposalID uint64 depositor sdk.AccAddress deposit sdk.Coins - options v1beta1.WeightedVoteOptions }{ "wrong proposal id": { preRun: func() uint64 { @@ -861,7 +1303,16 @@ func (suite *KeeperTestSuite) TestLegacyMsgDeposit() { depositor: proposer, deposit: coins, expErr: true, - options: v1beta1.NewNonSplitVoteOption(v1beta1.OptionYes), + expErrMsg: "unknown proposal", + }, + "empty depositer": { + preRun: func() uint64 { + return pID + }, + depositor: sdk.AccAddress{}, + deposit: coins, + expErr: true, + expErrMsg: "invalid depositor address: empty address string is not allowed", }, "all good": { preRun: func() uint64 { @@ -870,7 +1321,6 @@ func (suite *KeeperTestSuite) TestLegacyMsgDeposit() { depositor: proposer, deposit: minDeposit, expErr: false, - options: v1beta1.NewNonSplitVoteOption(v1beta1.OptionYes), }, } @@ -881,6 +1331,7 @@ func (suite *KeeperTestSuite) TestLegacyMsgDeposit() { _, err := suite.legacyMsgSrvr.Deposit(suite.ctx, depositReq) if tc.expErr { suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.expErrMsg) } else { suite.Require().NoError(err) } @@ -916,7 +1367,7 @@ func (suite *KeeperTestSuite) TestMsgUpdateParams() { } }, expErr: true, - expErrMsg: "invalid authority address", + expErrMsg: "invalid authority", }, { name: "invalid min deposit", @@ -1140,10 +1591,6 @@ func (suite *KeeperTestSuite) TestMsgUpdateParams() { suite.Run(tc.name, func() { msg := tc.input() exec := func(updateParams *v1.MsgUpdateParams) error { - if err := msg.ValidateBasic(); err != nil { - return err - } - if _, err := suite.msgSrvr.UpdateParams(suite.ctx, updateParams); err != nil { return err } diff --git a/x/gov/types/v1/msgs.go b/x/gov/types/v1/msgs.go index d3c59a6e1e32..7ba8e69a97e6 100644 --- a/x/gov/types/v1/msgs.go +++ b/x/gov/types/v1/msgs.go @@ -1,18 +1,11 @@ package v1 import ( - "fmt" - - errorsmod "cosmossdk.io/errors" - "cosmossdk.io/math" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" sdktx "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" "github.com/cosmos/cosmos-sdk/x/gov/codec" - "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" ) @@ -65,53 +58,6 @@ func (m *MsgSubmitProposal) SetMsgs(msgs []sdk.Msg) error { return nil } -// ValidateBasic implements the sdk.Msg interface. -func (m MsgSubmitProposal) ValidateBasic() error { - if m.Title == "" { - return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "proposal title cannot be empty") - } - if m.Summary == "" { - return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "proposal summary cannot be empty") - } - - if _, err := sdk.AccAddressFromBech32(m.Proposer); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) - } - - deposit := sdk.NewCoins(m.InitialDeposit...) - if !deposit.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) - } - - if deposit.IsAnyNegative() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) - } - - // Check that either metadata or Msgs length is non nil. - if len(m.Messages) == 0 && len(m.Metadata) == 0 { - return errorsmod.Wrap(types.ErrNoProposalMsgs, "either metadata or Msgs length must be non-nil") - } - - msgs, err := m.GetMsgs() - if err != nil { - return err - } - - for idx, msg := range msgs { - m, ok := msg.(sdk.HasValidateBasic) - if !ok { - continue - } - - if err := m.ValidateBasic(); err != nil { - return errorsmod.Wrap(types.ErrInvalidProposalMsg, - fmt.Sprintf("msg: %d, err: %s", idx, err.Error())) - } - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (m MsgSubmitProposal) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&m) @@ -134,22 +80,6 @@ func NewMsgDeposit(depositor sdk.AccAddress, proposalID uint64, amount sdk.Coins return &MsgDeposit{proposalID, depositor.String(), amount} } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgDeposit) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Depositor); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid depositor address: %s", err) - } - amount := sdk.NewCoins(msg.Amount...) - if !amount.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amount.String()) - } - if amount.IsAnyNegative() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amount.String()) - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgDeposit) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) @@ -167,18 +97,6 @@ func NewMsgVote(voter sdk.AccAddress, proposalID uint64, option VoteOption, meta return &MsgVote{proposalID, voter.String(), option, metadata} } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgVote) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) - } - if !ValidVoteOption(msg.Option) { - return errorsmod.Wrap(types.ErrInvalidVote, msg.Option.String()) - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgVote) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) @@ -196,43 +114,6 @@ func NewMsgVoteWeighted(voter sdk.AccAddress, proposalID uint64, options Weighte return &MsgVoteWeighted{proposalID, voter.String(), options, metadata} } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgVoteWeighted) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) - } - if len(msg.Options) == 0 { - return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, WeightedVoteOptions(msg.Options).String()) - } - - totalWeight := math.LegacyNewDec(0) - usedOptions := make(map[VoteOption]bool) - for _, option := range msg.Options { - if !option.IsValid() { - return errorsmod.Wrap(types.ErrInvalidVote, option.String()) - } - weight, err := sdk.NewDecFromStr(option.Weight) - if err != nil { - return errorsmod.Wrapf(types.ErrInvalidVote, "Invalid weight: %s", err) - } - totalWeight = totalWeight.Add(weight) - if usedOptions[option.Option] { - return errorsmod.Wrap(types.ErrInvalidVote, "Duplicated vote option") - } - usedOptions[option.Option] = true - } - - if totalWeight.GT(math.LegacyNewDec(1)) { - return errorsmod.Wrap(types.ErrInvalidVote, "Total weight overflow 1.00") - } - - if totalWeight.LT(math.LegacyNewDec(1)) { - return errorsmod.Wrap(types.ErrInvalidVote, "Total weight lower than 1.00") - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgVoteWeighted) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) @@ -281,15 +162,6 @@ func (c MsgExecLegacyContent) UnpackInterfaces(unpacker codectypes.AnyUnpacker) return unpacker.UnpackAny(c.Content, &content) } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgUpdateParams) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err) - } - - return msg.Params.ValidateBasic() -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgUpdateParams) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) @@ -310,15 +182,6 @@ func NewMsgCancelProposal(proposalID uint64, proposer string) *MsgCancelProposal } } -// ValidateBasic implements Msg -func (msg MsgCancelProposal) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Proposer); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) - } - - return nil -} - // GetSignBytes implements Msg func (msg MsgCancelProposal) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) diff --git a/x/gov/types/v1/msgs_test.go b/x/gov/types/v1/msgs_test.go index d0dfe7d1700c..06e7610c3bb5 100644 --- a/x/gov/types/v1/msgs_test.go +++ b/x/gov/types/v1/msgs_test.go @@ -4,13 +4,11 @@ import ( "fmt" "testing" - "cosmossdk.io/math" "github.com/stretchr/testify/require" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" - "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" ) var ( @@ -38,139 +36,6 @@ func TestMsgDepositGetSignBytes(t *testing.T) { require.Equal(t, expected, string(res)) } -// test ValidateBasic for MsgDeposit -func TestMsgDeposit(t *testing.T) { - tests := []struct { - proposalID uint64 - depositorAddr sdk.AccAddress - depositAmount sdk.Coins - expectPass bool - }{ - {0, addrs[0], coinsPos, true}, - {1, sdk.AccAddress{}, coinsPos, false}, - {1, addrs[0], coinsZero, true}, - {1, addrs[0], coinsMulti, true}, - } - - for i, tc := range tests { - msg := v1.NewMsgDeposit(tc.depositorAddr, tc.proposalID, tc.depositAmount) - if tc.expectPass { - require.NoError(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.Error(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -// test ValidateBasic for MsgVote -func TestMsgVote(t *testing.T) { - tests := []struct { - proposalID uint64 - voterAddr sdk.AccAddress - option v1.VoteOption - metadata string - expectPass bool - }{ - {0, addrs[0], v1.OptionYes, metadata, true}, - {0, sdk.AccAddress{}, v1.OptionYes, "", false}, - {0, addrs[0], v1.OptionNo, metadata, true}, - {0, addrs[0], v1.OptionNoWithVeto, "", true}, - {0, addrs[0], v1.OptionAbstain, "", true}, - {0, addrs[0], v1.VoteOption(0x13), "", false}, - } - - for i, tc := range tests { - msg := v1.NewMsgVote(tc.voterAddr, tc.proposalID, tc.option, tc.metadata) - if tc.expectPass { - require.Nil(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.NotNil(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -// test ValidateBasic for MsgVoteWeighted -func TestMsgVoteWeighted(t *testing.T) { - tests := []struct { - proposalID uint64 - voterAddr sdk.AccAddress - options v1.WeightedVoteOptions - metadata string - expectPass bool - }{ - {0, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), metadata, true}, - {0, sdk.AccAddress{}, v1.NewNonSplitVoteOption(v1.OptionYes), "", false}, - {0, addrs[0], v1.NewNonSplitVoteOption(v1.OptionNo), "", true}, - {0, addrs[0], v1.NewNonSplitVoteOption(v1.OptionNoWithVeto), "", true}, - {0, addrs[0], v1.NewNonSplitVoteOption(v1.OptionAbstain), "", true}, - {0, addrs[0], v1.WeightedVoteOptions{ // weight sum > 1 - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(1)), - v1.NewWeightedVoteOption(v1.OptionAbstain, math.LegacyNewDec(1)), - }, "", false}, - {0, addrs[0], v1.WeightedVoteOptions{ // duplicate option - v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), - v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), - }, "", false}, - {0, addrs[0], v1.WeightedVoteOptions{ // zero weight - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(0)), - }, "", false}, - {0, addrs[0], v1.WeightedVoteOptions{ // negative weight - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(-1)), - }, "", false}, - {0, addrs[0], v1.WeightedVoteOptions{}, "", false}, - {0, addrs[0], v1.NewNonSplitVoteOption(v1.VoteOption(0x13)), "", false}, - {0, addrs[0], v1.WeightedVoteOptions{ // weight sum <1 - v1.NewWeightedVoteOption(v1.OptionYes, sdk.NewDecWithPrec(5, 1)), - }, "", false}, - } - - for i, tc := range tests { - msg := v1.NewMsgVoteWeighted(tc.voterAddr, tc.proposalID, tc.options, tc.metadata) - if tc.expectPass { - require.Nil(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.NotNil(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -func TestMsgSubmitProposal_ValidateBasic(t *testing.T) { - // Valid msg - msg1, err := v1.NewLegacyContent(v1beta1.NewTextProposal("Title", "description"), addrs[0].String()) - require.NoError(t, err) - // Invalid msg - msg2, err := v1.NewLegacyContent(v1beta1.NewTextProposal("Title", "description"), "foo") - require.NoError(t, err) - - tests := []struct { - name string - proposer string - initialDeposit sdk.Coins - messages []sdk.Msg - metadata, title, summary string - expedited bool - expErr bool - }{ - {"invalid addr", "", coinsPos, []sdk.Msg{msg1}, metadata, "Title", "Summary", false, true}, - {"empty msgs and metadata", addrs[0].String(), coinsPos, nil, "", "Title", "Summary", false, true}, - {"empty title and summary", addrs[0].String(), coinsPos, nil, "", "", "", false, true}, - {"invalid msg", addrs[0].String(), coinsPos, []sdk.Msg{msg1, msg2}, metadata, "Title", "Summary", false, true}, - {"valid with no Msg", addrs[0].String(), coinsPos, nil, metadata, "Title", "Summary", false, false}, - {"valid with no metadata", addrs[0].String(), coinsPos, []sdk.Msg{msg1}, "", "Title", "Summary", false, false}, - {"valid with everything", addrs[0].String(), coinsPos, []sdk.Msg{msg1}, metadata, "Title", "Summary", true, false}, - } - - for _, tc := range tests { - msg, err := v1.NewMsgSubmitProposal(tc.messages, tc.initialDeposit, tc.proposer, tc.metadata, tc.title, tc.summary, tc.expedited) - require.NoError(t, err) - if tc.expErr { - require.Error(t, msg.ValidateBasic(), "test: %s", tc.name) - } else { - require.NoError(t, msg.ValidateBasic(), "test: %s", tc.name) - } - } -} - // this tests that Amino JSON MsgSubmitProposal.GetSignBytes() still works with Content as Any using the ModuleCdc func TestMsgSubmitProposal_GetSignBytes(t *testing.T) { testcases := []struct { diff --git a/x/gov/types/v1beta1/msgs.go b/x/gov/types/v1beta1/msgs.go index 50fc1ca40a3f..0cc741fd3bc5 100644 --- a/x/gov/types/v1beta1/msgs.go +++ b/x/gov/types/v1beta1/msgs.go @@ -3,17 +3,12 @@ package v1beta1 import ( "fmt" - "cosmossdk.io/math" "github.com/cosmos/gogoproto/proto" - errorsmod "cosmossdk.io/errors" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" "github.com/cosmos/cosmos-sdk/x/gov/codec" - "github.com/cosmos/cosmos-sdk/x/gov/types" ) // Governance message types and routes @@ -86,32 +81,6 @@ func (m *MsgSubmitProposal) SetContent(content Content) error { return nil } -// ValidateBasic implements the sdk.Msg interface. -func (m MsgSubmitProposal) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(m.Proposer); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) - } - if !m.InitialDeposit.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, m.InitialDeposit.String()) - } - if m.InitialDeposit.IsAnyNegative() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, m.InitialDeposit.String()) - } - - content := m.GetContent() - if content == nil { - return errorsmod.Wrap(types.ErrInvalidProposalContent, "missing content") - } - if !IsValidProposalType(content.ProposalType()) { - return errorsmod.Wrap(types.ErrInvalidProposalType, content.ProposalType()) - } - if err := content.ValidateBasic(); err != nil { - return err - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (m MsgSubmitProposal) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&m) @@ -131,27 +100,10 @@ func (m MsgSubmitProposal) UnpackInterfaces(unpacker codectypes.AnyUnpacker) err } // NewMsgDeposit creates a new MsgDeposit instance -// - func NewMsgDeposit(depositor sdk.AccAddress, proposalID uint64, amount sdk.Coins) *MsgDeposit { return &MsgDeposit{proposalID, depositor.String(), amount} } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgDeposit) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Depositor); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid depositor address: %s", err) - } - if !msg.Amount.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String()) - } - if msg.Amount.IsAnyNegative() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String()) - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgDeposit) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) @@ -169,18 +121,6 @@ func NewMsgVote(voter sdk.AccAddress, proposalID uint64, option VoteOption) *Msg return &MsgVote{proposalID, voter.String(), option} } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgVote) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) - } - if !ValidVoteOption(msg.Option) { - return errorsmod.Wrap(types.ErrInvalidVote, msg.Option.String()) - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgVote) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) @@ -198,39 +138,6 @@ func NewMsgVoteWeighted(voter sdk.AccAddress, proposalID uint64, options Weighte return &MsgVoteWeighted{proposalID, voter.String(), options} } -// ValidateBasic implements the sdk.Msg interface. -func (msg MsgVoteWeighted) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) - } - if len(msg.Options) == 0 { - return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, WeightedVoteOptions(msg.Options).String()) - } - - totalWeight := math.LegacyNewDec(0) - usedOptions := make(map[VoteOption]bool) - for _, option := range msg.Options { - if !ValidWeightedVoteOption(option) { - return errorsmod.Wrap(types.ErrInvalidVote, option.String()) - } - totalWeight = totalWeight.Add(option.Weight) - if usedOptions[option.Option] { - return errorsmod.Wrap(types.ErrInvalidVote, "Duplicated vote option") - } - usedOptions[option.Option] = true - } - - if totalWeight.GT(math.LegacyNewDec(1)) { - return errorsmod.Wrap(types.ErrInvalidVote, "Total weight overflow 1.00") - } - - if totalWeight.LT(math.LegacyNewDec(1)) { - return errorsmod.Wrap(types.ErrInvalidVote, "Total weight lower than 1.00") - } - - return nil -} - // GetSignBytes returns the message bytes to sign over. func (msg MsgVoteWeighted) GetSignBytes() []byte { bz := codec.Amino.MustMarshalJSON(&msg) diff --git a/x/gov/types/v1beta1/msgs_test.go b/x/gov/types/v1beta1/msgs_test.go index 84c592bf4768..a147458d8775 100644 --- a/x/gov/types/v1beta1/msgs_test.go +++ b/x/gov/types/v1beta1/msgs_test.go @@ -1,10 +1,8 @@ package v1beta1 import ( - "strings" "testing" - "cosmossdk.io/math" "github.com/stretchr/testify/require" sdk "github.com/cosmos/cosmos-sdk/types" @@ -24,45 +22,6 @@ func init() { coinsMulti.Sort() } -// test ValidateBasic for MsgCreateValidator -func TestMsgSubmitProposal(t *testing.T) { - tests := []struct { - title, description string - proposalType string - proposerAddr sdk.AccAddress - initialDeposit sdk.Coins - expectPass bool - }{ - {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsPos, true}, - {"", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsPos, false}, - {"Test Proposal", "", ProposalTypeText, addrs[0], coinsPos, false}, - {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, sdk.AccAddress{}, coinsPos, false}, - {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsZero, true}, - {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsMulti, true}, - {strings.Repeat("#", MaxTitleLength*2), "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsMulti, false}, - {"Test Proposal", strings.Repeat("#", MaxDescriptionLength*2), ProposalTypeText, addrs[0], coinsMulti, false}, - } - - for i, tc := range tests { - content, ok := ContentFromProposalType(tc.title, tc.description, tc.proposalType) - require.True(t, ok) - - msg, err := NewMsgSubmitProposal( - content, - tc.initialDeposit, - tc.proposerAddr, - ) - - require.NoError(t, err) - - if tc.expectPass { - require.NoError(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.Error(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - func TestMsgDepositGetSignBytes(t *testing.T) { addr := sdk.AccAddress("addr1") msg := NewMsgDeposit(addr, 0, coinsPos) @@ -72,100 +31,6 @@ func TestMsgDepositGetSignBytes(t *testing.T) { require.Equal(t, expected, string(res)) } -// test ValidateBasic for MsgDeposit -func TestMsgDeposit(t *testing.T) { - tests := []struct { - proposalID uint64 - depositorAddr sdk.AccAddress - depositAmount sdk.Coins - expectPass bool - }{ - {0, addrs[0], coinsPos, true}, - {1, sdk.AccAddress{}, coinsPos, false}, - {1, addrs[0], coinsZero, true}, - {1, addrs[0], coinsMulti, true}, - } - - for i, tc := range tests { - msg := NewMsgDeposit(tc.depositorAddr, tc.proposalID, tc.depositAmount) - if tc.expectPass { - require.NoError(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.Error(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -// test ValidateBasic for MsgVote -func TestMsgVote(t *testing.T) { - tests := []struct { - proposalID uint64 - voterAddr sdk.AccAddress - option VoteOption - expectPass bool - }{ - {0, addrs[0], OptionYes, true}, - {0, sdk.AccAddress{}, OptionYes, false}, - {0, addrs[0], OptionNo, true}, - {0, addrs[0], OptionNoWithVeto, true}, - {0, addrs[0], OptionAbstain, true}, - {0, addrs[0], VoteOption(0x13), false}, - } - - for i, tc := range tests { - msg := NewMsgVote(tc.voterAddr, tc.proposalID, tc.option) - if tc.expectPass { - require.Nil(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.NotNil(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -// test ValidateBasic for MsgVoteWeighted -func TestMsgVoteWeighted(t *testing.T) { - tests := []struct { - proposalID uint64 - voterAddr sdk.AccAddress - options WeightedVoteOptions - expectPass bool - }{ - {0, addrs[0], NewNonSplitVoteOption(OptionYes), true}, - {0, sdk.AccAddress{}, NewNonSplitVoteOption(OptionYes), false}, - {0, addrs[0], NewNonSplitVoteOption(OptionNo), true}, - {0, addrs[0], NewNonSplitVoteOption(OptionNoWithVeto), true}, - {0, addrs[0], NewNonSplitVoteOption(OptionAbstain), true}, - {0, addrs[0], WeightedVoteOptions{ // weight sum > 1 - WeightedVoteOption{Option: OptionYes, Weight: math.LegacyNewDec(1)}, - WeightedVoteOption{Option: OptionAbstain, Weight: math.LegacyNewDec(1)}, - }, false}, - {0, addrs[0], WeightedVoteOptions{ // duplicate option - WeightedVoteOption{Option: OptionYes, Weight: math.LegacyNewDecWithPrec(5, 1)}, - WeightedVoteOption{Option: OptionYes, Weight: math.LegacyNewDecWithPrec(5, 1)}, - }, false}, - {0, addrs[0], WeightedVoteOptions{ // zero weight - WeightedVoteOption{Option: OptionYes, Weight: math.LegacyNewDec(0)}, - }, false}, - {0, addrs[0], WeightedVoteOptions{ // negative weight - WeightedVoteOption{Option: OptionYes, Weight: math.LegacyNewDec(-1)}, - }, false}, - {0, addrs[0], WeightedVoteOptions{}, false}, - {0, addrs[0], NewNonSplitVoteOption(VoteOption(0x13)), false}, - {0, addrs[0], WeightedVoteOptions{ // weight sum <1 - WeightedVoteOption{Option: OptionYes, Weight: math.LegacyNewDecWithPrec(5, 1)}, - }, false}, - } - - for i, tc := range tests { - msg := NewMsgVoteWeighted(tc.voterAddr, tc.proposalID, tc.options) - if tc.expectPass { - require.Nil(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.NotNil(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - // this tests that Amino JSON MsgSubmitProposal.GetSignBytes() still works with Content as Any using the ModuleCdc func TestMsgSubmitProposal_GetSignBytes(t *testing.T) { msg, err := NewMsgSubmitProposal(NewTextProposal("test", "abcd"), sdk.NewCoins(), sdk.AccAddress{}) From 44c756aeb0b11d3e458cda871b300f5af316d792 Mon Sep 17 00:00:00 2001 From: atheesh Date: Thu, 13 Apr 2023 17:16:26 +0530 Subject: [PATCH 2/9] fix tests --- x/gov/keeper/msg_server_test.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index 2d8f67c66ee3..aaef64e48d22 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -270,7 +270,21 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { }, "all good": { preRun: func() uint64 { - return proposalID + msg, err := v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + coins, + proposer.String(), + "", + "Proposal", + "description of proposal", + false, + ) + suite.Require().NoError(err) + + res, err := suite.msgSrvr.SubmitProposal(suite.ctx, msg) + suite.Require().NoError(err) + suite.Require().NotNil(res.ProposalId) + return res.ProposalId }, depositor: proposer, expErr: false, @@ -763,9 +777,7 @@ func (suite *KeeperTestSuite) TestDepositReq() { // legacy msg server tests func (suite *KeeperTestSuite) TestLegacyMsgSubmitProposal() { - addrs := suite.addrs - proposer := addrs[0] - + proposer := simtestutil.AddTestAddrsIncremental(suite.bankKeeper, suite.stakingKeeper, suite.ctx, 1, sdkmath.NewInt(50000000))[0] coins := sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(100))) initialDeposit := coins minDeposit := suite.govKeeper.GetParams(suite.ctx).MinDeposit From 10123f233187a9c86fb90b65c0e6d81a848b7556 Mon Sep 17 00:00:00 2001 From: atheesh Date: Thu, 13 Apr 2023 17:43:59 +0530 Subject: [PATCH 3/9] review changes --- x/gov/keeper/msg_server.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index 234cc348cad3..b6b9c50e96bf 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -323,10 +323,6 @@ func (k msgServer) UpdateParams(goCtx context.Context, msg *v1.MsgUpdateParams) return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority) } - if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { - return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err) - } - if err := msg.Params.ValidateBasic(); err != nil { return nil, err } From 854edfc3a3106612cce483e858e8f6a6c1328964 Mon Sep 17 00:00:00 2001 From: atheesh Date: Thu, 13 Apr 2023 18:25:17 +0530 Subject: [PATCH 4/9] fix lint --- x/gov/keeper/msg_server.go | 21 ++++++++++----------- x/gov/keeper/msg_server_test.go | 27 +++++++++++++-------------- x/gov/types/v1/msgs_test.go | 3 --- x/gov/types/v1beta1/msgs_test.go | 5 ----- 4 files changed, 23 insertions(+), 33 deletions(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index b6b9c50e96bf..6c8da1555cfe 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -12,7 +12,6 @@ import ( "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "github.com/cosmos/cosmos-sdk/x/gov/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" @@ -54,7 +53,7 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos // Check that either metadata or Msgs length is non nil. if len(msg.Messages) == 0 && len(msg.Metadata) == 0 { - return nil, errors.Wrap(types.ErrNoProposalMsgs, "either metadata or Msgs length must be non-nil") + return nil, errors.Wrap(govtypes.ErrNoProposalMsgs, "either metadata or Msgs length must be non-nil") } msgs, err := msg.GetMsgs() @@ -69,7 +68,7 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos } if err := m.ValidateBasic(); err != nil { - return nil, errors.Wrap(types.ErrInvalidProposalMsg, + return nil, errors.Wrap(govtypes.ErrInvalidProposalMsg, fmt.Sprintf("msg: %d, err: %s", idx, err.Error())) } } @@ -193,7 +192,7 @@ func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResp } if !v1.ValidVoteOption(msg.Option) { - return nil, errors.Wrap(types.ErrInvalidVote, msg.Option.String()) + return nil, errors.Wrap(govtypes.ErrInvalidVote, msg.Option.String()) } ctx := sdk.UnwrapSDKContext(goCtx) @@ -230,25 +229,25 @@ func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) usedOptions := make(map[v1.VoteOption]bool) for _, option := range msg.Options { if !option.IsValid() { - return nil, errors.Wrap(types.ErrInvalidVote, option.String()) + return nil, errors.Wrap(govtypes.ErrInvalidVote, option.String()) } weight, err := sdk.NewDecFromStr(option.Weight) if err != nil { - return nil, errors.Wrapf(types.ErrInvalidVote, "invalid weight: %s", err) + return nil, errors.Wrapf(govtypes.ErrInvalidVote, "invalid weight: %s", err) } totalWeight = totalWeight.Add(weight) if usedOptions[option.Option] { - return nil, errors.Wrap(types.ErrInvalidVote, "duplicated vote option") + return nil, errors.Wrap(govtypes.ErrInvalidVote, "duplicated vote option") } usedOptions[option.Option] = true } if totalWeight.GT(math.LegacyNewDec(1)) { - return nil, errors.Wrap(types.ErrInvalidVote, "total weight overflow 1.00") + return nil, errors.Wrap(govtypes.ErrInvalidVote, "total weight overflow 1.00") } if totalWeight.LT(math.LegacyNewDec(1)) { - return nil, errors.Wrap(types.ErrInvalidVote, "total weight lower than 1.00") + return nil, errors.Wrap(govtypes.ErrInvalidVote, "total weight lower than 1.00") } ctx := sdk.UnwrapSDKContext(goCtx) @@ -351,10 +350,10 @@ var _ v1beta1.MsgServer = legacyMsgServer{} func (k legacyMsgServer) SubmitProposal(goCtx context.Context, msg *v1beta1.MsgSubmitProposal) (*v1beta1.MsgSubmitProposalResponse, error) { content := msg.GetContent() if content == nil { - return nil, errors.Wrap(types.ErrInvalidProposalContent, "missing content") + return nil, errors.Wrap(govtypes.ErrInvalidProposalContent, "missing content") } if !v1beta1.IsValidProposalType(content.ProposalType()) { - return nil, errors.Wrap(types.ErrInvalidProposalType, content.ProposalType()) + return nil, errors.Wrap(govtypes.ErrInvalidProposalType, content.ProposalType()) } if err := content.ValidateBasic(); err != nil { return nil, err diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index aaef64e48d22..2daf895cb566 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -5,7 +5,6 @@ import ( "strings" "time" - "cosmossdk.io/math" sdkmath "cosmossdk.io/math" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" "github.com/cosmos/cosmos-sdk/testutil/testdata" @@ -496,7 +495,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { return proposalID }, option: v1.WeightedVoteOptions{ - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(1)), + v1.NewWeightedVoteOption(v1.OptionYes, sdkmath.LegacyNewDec(1)), }, voter: sdk.AccAddress{}, metadata: "", @@ -508,8 +507,8 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { return proposalID }, option: v1.WeightedVoteOptions{ - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(1)), - v1.NewWeightedVoteOption(v1.OptionAbstain, math.LegacyNewDec(1)), + v1.NewWeightedVoteOption(v1.OptionYes, sdkmath.LegacyNewDec(1)), + v1.NewWeightedVoteOption(v1.OptionAbstain, sdkmath.LegacyNewDec(1)), }, voter: proposer, metadata: "", @@ -534,7 +533,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { return proposalID }, option: v1.WeightedVoteOptions{ - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(0)), + v1.NewWeightedVoteOption(v1.OptionYes, sdkmath.LegacyNewDec(0)), }, voter: proposer, metadata: "", @@ -546,7 +545,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { return proposalID }, option: v1.WeightedVoteOptions{ - v1.NewWeightedVoteOption(v1.OptionYes, math.LegacyNewDec(-1)), + v1.NewWeightedVoteOption(v1.OptionYes, sdkmath.LegacyNewDec(-1)), }, voter: proposer, metadata: "", @@ -1066,7 +1065,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(1), + Weight: sdkmath.LegacyNewDec(1), }, }, voter: sdk.AccAddress{}, @@ -1081,11 +1080,11 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(1), + Weight: sdkmath.LegacyNewDec(1), }, v1beta1.WeightedVoteOption{ Option: v1beta1.OptionAbstain, - Weight: math.LegacyNewDec(1), + Weight: sdkmath.LegacyNewDec(1), }, }, voter: proposer, @@ -1119,7 +1118,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(0), + Weight: sdkmath.LegacyNewDec(0), }, }, voter: proposer, @@ -1134,7 +1133,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(-1), + Weight: sdkmath.LegacyNewDec(-1), }, }, voter: proposer, @@ -1203,7 +1202,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(1), + Weight: sdkmath.LegacyNewDec(1), }, }, voter: proposer, @@ -1218,7 +1217,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(1), + Weight: sdkmath.LegacyNewDec(1), }, }, voter: sdk.AccAddress(strings.Repeat("a", 300)), @@ -1247,7 +1246,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { option: v1beta1.WeightedVoteOptions{ v1beta1.WeightedVoteOption{ Option: v1beta1.OptionYes, - Weight: math.LegacyNewDec(1), + Weight: sdkmath.LegacyNewDec(1), }, }, voter: proposer, diff --git a/x/gov/types/v1/msgs_test.go b/x/gov/types/v1/msgs_test.go index 06e7610c3bb5..456e7429a573 100644 --- a/x/gov/types/v1/msgs_test.go +++ b/x/gov/types/v1/msgs_test.go @@ -13,14 +13,11 @@ import ( var ( coinsPos = sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 1000)) - coinsZero = sdk.NewCoins() coinsMulti = sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 1000), sdk.NewInt64Coin("foo", 10000)) addrs = []sdk.AccAddress{ sdk.AccAddress("test1"), sdk.AccAddress("test2"), } - - metadata = "metadata" ) func init() { diff --git a/x/gov/types/v1beta1/msgs_test.go b/x/gov/types/v1beta1/msgs_test.go index a147458d8775..d041ccb834cc 100644 --- a/x/gov/types/v1beta1/msgs_test.go +++ b/x/gov/types/v1beta1/msgs_test.go @@ -10,12 +10,7 @@ import ( var ( coinsPos = sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 1000)) - coinsZero = sdk.NewCoins() coinsMulti = sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 1000), sdk.NewInt64Coin("foo", 10000)) - addrs = []sdk.AccAddress{ - sdk.AccAddress("test1"), - sdk.AccAddress("test2"), - } ) func init() { From 98b39b598f42753aec5e42f655db365fe79f1401 Mon Sep 17 00:00:00 2001 From: atheesh Date: Fri, 14 Apr 2023 15:00:10 +0530 Subject: [PATCH 5/9] review changes --- x/gov/keeper/msg_server.go | 57 ++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index 6c8da1555cfe..8b3f9d844428 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -56,23 +56,10 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos return nil, errors.Wrap(govtypes.ErrNoProposalMsgs, "either metadata or Msgs length must be non-nil") } - msgs, err := msg.GetMsgs() - if err != nil { + if err := validateMsgs(msg); err != nil { return nil, err } - for idx, msg := range msgs { - m, ok := msg.(sdk.HasValidateBasic) - if !ok { - continue - } - - if err := m.ValidateBasic(); err != nil { - return nil, errors.Wrap(govtypes.ErrInvalidProposalMsg, - fmt.Sprintf("msg: %d, err: %s", idx, err.Error())) - } - } - ctx := sdk.UnwrapSDKContext(goCtx) initialDeposit := msg.GetInitialDeposit() @@ -277,13 +264,8 @@ func (k msgServer) Deposit(goCtx context.Context, msg *v1.MsgDeposit) (*v1.MsgDe return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid depositor address: %s", err) } - amount := sdk.NewCoins(msg.Amount...) - if !amount.IsValid() { - return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, amount.String()) - } - - if amount.IsAnyNegative() { - return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, amount.String()) + if err := validateAmount(msg.Amount); err != nil { + return nil, err } ctx := sdk.UnwrapSDKContext(goCtx) @@ -428,3 +410,36 @@ func (k legacyMsgServer) Deposit(goCtx context.Context, msg *v1beta1.MsgDeposit) } return &v1beta1.MsgDepositResponse{}, nil } + +func validateAmount(amount sdk.Coins) error { + if !amount.IsValid() { + return sdkerrors.ErrInvalidCoins.Wrap(amount.String()) + } + + if !amount.IsAllPositive() { + return sdkerrors.ErrInvalidCoins.Wrap(amount.String()) + } + + return nil +} + +func validateMsgs(msg *v1.MsgSubmitProposal) error { + msgs, err := msg.GetMsgs() + if err != nil { + return err + } + + for idx, msg := range msgs { + m, ok := msg.(sdk.HasValidateBasic) + if !ok { + continue + } + + if err := m.ValidateBasic(); err != nil { + return errors.Wrap(govtypes.ErrInvalidProposalMsg, + fmt.Sprintf("msg: %d, err: %s", idx, err.Error())) + } + } + + return nil +} From 35eddc38b7e46bf145be6ab7172252bd34f2cdc5 Mon Sep 17 00:00:00 2001 From: atheesh Date: Mon, 17 Apr 2023 11:43:25 +0530 Subject: [PATCH 6/9] review changes --- x/gov/keeper/msg_server.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index 8b3f9d844428..9946d194fce3 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -42,13 +42,8 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) } - deposit := sdk.NewCoins(msg.InitialDeposit...) - if !deposit.IsValid() { - return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) - } - - if deposit.IsAnyNegative() { - return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) + if err := validateAmount(sdk.NewCoins(msg.InitialDeposit...)); err != nil { + return nil, err } // Check that either metadata or Msgs length is non nil. @@ -264,7 +259,7 @@ func (k msgServer) Deposit(goCtx context.Context, msg *v1.MsgDeposit) (*v1.MsgDe return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid depositor address: %s", err) } - if err := validateAmount(msg.Amount); err != nil { + if err := validateAmount(sdk.NewCoins(msg.Amount...)); err != nil { return nil, err } From b0be3c8038f6244b11cb961a5669b2ccb28119ea Mon Sep 17 00:00:00 2001 From: atheesh Date: Mon, 17 Apr 2023 16:17:24 +0530 Subject: [PATCH 7/9] fix tests --- x/gov/keeper/msg_server.go | 60 +++++++++++---------------------- x/gov/keeper/msg_server_test.go | 20 +++++++++-- 2 files changed, 37 insertions(+), 43 deletions(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index 9946d194fce3..055a3da8c871 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -38,7 +38,8 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "proposal summary cannot be empty") } - if _, err := sdk.AccAddressFromBech32(msg.Proposer); err != nil { + proposer, err := k.authKeeper.StringToBytes(msg.GetProposer()) + if err != nil { return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) } @@ -51,25 +52,19 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos return nil, errors.Wrap(govtypes.ErrNoProposalMsgs, "either metadata or Msgs length must be non-nil") } - if err := validateMsgs(msg); err != nil { + proposalMsgs, err := msg.GetMsgs() + if err != nil { return nil, err } - ctx := sdk.UnwrapSDKContext(goCtx) - - initialDeposit := msg.GetInitialDeposit() - - if err := k.validateInitialDeposit(ctx, initialDeposit, msg.Expedited); err != nil { + if err := validateMsgs(proposalMsgs); err != nil { return nil, err } - proposer, err := k.authKeeper.StringToBytes(msg.GetProposer()) - if err != nil { - return nil, err - } + ctx := sdk.UnwrapSDKContext(goCtx) + initialDeposit := msg.GetInitialDeposit() - proposalMsgs, err := msg.GetMsgs() - if err != nil { + if err := k.validateInitialDeposit(ctx, initialDeposit, msg.Expedited); err != nil { return nil, err } @@ -111,16 +106,12 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos // CancelProposals implements the MsgServer.CancelProposal method. func (k msgServer) CancelProposal(goCtx context.Context, msg *v1.MsgCancelProposal) (*v1.MsgCancelProposalResponse, error) { - if _, err := sdk.AccAddressFromBech32(msg.Proposer); err != nil { - return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) - } - - ctx := sdk.UnwrapSDKContext(goCtx) _, err := k.authKeeper.StringToBytes(msg.Proposer) if err != nil { - return nil, err + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) } + ctx := sdk.UnwrapSDKContext(goCtx) if err := k.Keeper.CancelProposal(ctx, msg.ProposalId, msg.Proposer); err != nil { return nil, err } @@ -169,7 +160,8 @@ func (k msgServer) ExecLegacyContent(goCtx context.Context, msg *v1.MsgExecLegac // Vote implements the MsgServer.Vote method. func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResponse, error) { - if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { + accAddr, err := k.authKeeper.StringToBytes(msg.Voter) + if err != nil { return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) } @@ -178,10 +170,6 @@ func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResp } ctx := sdk.UnwrapSDKContext(goCtx) - accAddr, err := k.authKeeper.StringToBytes(msg.Voter) - if err != nil { - return nil, err - } err = k.Keeper.AddVote(ctx, msg.ProposalId, accAddr, v1.NewNonSplitVoteOption(msg.Option), msg.Metadata) if err != nil { return nil, err @@ -200,9 +188,11 @@ func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResp // VoteWeighted implements the MsgServer.VoteWeighted method. func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) (*v1.MsgVoteWeightedResponse, error) { - if _, err := sdk.AccAddressFromBech32(msg.Voter); err != nil { - return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", err) + accAddr, accErr := k.authKeeper.StringToBytes(msg.Voter) + if accErr != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid voter address: %s", accErr) } + if len(msg.Options) == 0 { return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, v1.WeightedVoteOptions(msg.Options).String()) } @@ -233,10 +223,6 @@ func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) } ctx := sdk.UnwrapSDKContext(goCtx) - accAddr, accErr := k.authKeeper.StringToBytes(msg.Voter) - if accErr != nil { - return nil, accErr - } err := k.Keeper.AddVote(ctx, msg.ProposalId, accAddr, msg.Options, msg.Metadata) if err != nil { return nil, err @@ -255,7 +241,8 @@ func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) // Deposit implements the MsgServer.Deposit method. func (k msgServer) Deposit(goCtx context.Context, msg *v1.MsgDeposit) (*v1.MsgDepositResponse, error) { - if _, err := sdk.AccAddressFromBech32(msg.Depositor); err != nil { + accAddr, err := k.authKeeper.StringToBytes(msg.Depositor) + if err != nil { return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid depositor address: %s", err) } @@ -264,10 +251,6 @@ func (k msgServer) Deposit(goCtx context.Context, msg *v1.MsgDeposit) (*v1.MsgDe } ctx := sdk.UnwrapSDKContext(goCtx) - accAddr, err := k.authKeeper.StringToBytes(msg.Depositor) - if err != nil { - return nil, err - } votingStarted, err := k.Keeper.AddDeposit(ctx, msg.ProposalId, accAddr, msg.Amount) if err != nil { return nil, err @@ -418,12 +401,7 @@ func validateAmount(amount sdk.Coins) error { return nil } -func validateMsgs(msg *v1.MsgSubmitProposal) error { - msgs, err := msg.GetMsgs() - if err != nil { - return err - } - +func validateMsgs(msgs []sdk.Msg) error { for idx, msg := range msgs { m, ok := msg.(sdk.HasValidateBasic) if !ok { diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index 2daf895cb566..f54f1d48758b 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -20,8 +20,9 @@ const ( ) var ( - longAddress = "cosmos1v9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpg0s5ed" - longAddressError = "address max length is 255" + longAddress = "cosmos1v9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpv9skzctpg0s5ed" + longAddressError = "address max length is 255" + emptyAddressError = "empty address string is not allowed" ) func (suite *KeeperTestSuite) TestSubmitProposalReq() { @@ -39,6 +40,8 @@ func (suite *KeeperTestSuite) TestSubmitProposalReq() { Amount: coins, } + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) + cases := map[string]struct { preRun func() (*v1.MsgSubmitProposal, error) expErr bool @@ -236,6 +239,8 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { suite.Require().NotNil(res.ProposalId) proposalID := res.ProposalId + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) + cases := map[string]struct { preRun func() uint64 expErr bool @@ -261,6 +266,7 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { }, "empty proposer": { preRun: func() uint64 { + return proposalID }, depositor: sdk.AccAddress{}, @@ -331,6 +337,7 @@ func (suite *KeeperTestSuite) TestVoteReq() { suite.Require().NoError(err) suite.acctKeeper.EXPECT().StringToBytes(longAddress).Return(nil, errors.New(longAddressError)).AnyTimes() + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) res, err := suite.msgSrvr.SubmitProposal(suite.ctx, msg) suite.Require().NoError(err) @@ -480,6 +487,7 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() { proposalID := res.ProposalId suite.acctKeeper.EXPECT().StringToBytes(longAddress).Return(nil, errors.New(longAddressError)).AnyTimes() + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) cases := map[string]struct { preRun func() uint64 @@ -723,6 +731,8 @@ func (suite *KeeperTestSuite) TestDepositReq() { suite.Require().NotNil(res.ProposalId) pID := res.ProposalId + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) + cases := map[string]struct { preRun func() uint64 expErr bool @@ -781,6 +791,8 @@ func (suite *KeeperTestSuite) TestLegacyMsgSubmitProposal() { initialDeposit := coins minDeposit := suite.govKeeper.GetParams(suite.ctx).MinDeposit + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) + cases := map[string]struct { preRun func() (*v1beta1.MsgSubmitProposal, error) expErr bool @@ -914,6 +926,7 @@ func (suite *KeeperTestSuite) TestLegacyMsgVote() { proposalID := res.ProposalId suite.acctKeeper.EXPECT().StringToBytes(longAddress).Return(nil, errors.New(longAddressError)).AnyTimes() + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) cases := map[string]struct { preRun func() uint64 @@ -1048,6 +1061,7 @@ func (suite *KeeperTestSuite) TestLegacyVoteWeighted() { proposalID := res.ProposalId suite.acctKeeper.EXPECT().StringToBytes(longAddress).Return(nil, errors.New(longAddressError)).AnyTimes() + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) cases := map[string]struct { preRun func() uint64 @@ -1299,6 +1313,8 @@ func (suite *KeeperTestSuite) TestLegacyMsgDeposit() { suite.Require().NotNil(res.ProposalId) pID := res.ProposalId + suite.acctKeeper.EXPECT().StringToBytes("").Return(nil, errors.New(emptyAddressError)) + cases := map[string]struct { preRun func() uint64 expErr bool From 8f89607623d8373584ed466e38baab2a1bcba619 Mon Sep 17 00:00:00 2001 From: atheesh Date: Mon, 17 Apr 2023 18:53:26 +0530 Subject: [PATCH 8/9] fix lint --- x/gov/keeper/msg_server_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index f54f1d48758b..cc45f577ca02 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -266,7 +266,6 @@ func (suite *KeeperTestSuite) TestCancelProposalReq() { }, "empty proposer": { preRun: func() uint64 { - return proposalID }, depositor: sdk.AccAddress{}, From 414b3c1b175216461ed5535e1c9c447d16c77dd2 Mon Sep 17 00:00:00 2001 From: atheesh Date: Mon, 17 Apr 2023 20:25:24 +0530 Subject: [PATCH 9/9] fix test --- x/gov/keeper/msg_server.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index 055a3da8c871..274c881b82b2 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -43,7 +43,7 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid proposer address: %s", err) } - if err := validateAmount(sdk.NewCoins(msg.InitialDeposit...)); err != nil { + if err := validateDeposit(sdk.NewCoins(msg.InitialDeposit...)); err != nil { return nil, err } @@ -401,6 +401,18 @@ func validateAmount(amount sdk.Coins) error { return nil } +func validateDeposit(deposit sdk.Coins) error { + if !deposit.IsValid() { + return errors.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) + } + + if deposit.IsAnyNegative() { + return errors.Wrap(sdkerrors.ErrInvalidCoins, deposit.String()) + } + + return nil +} + func validateMsgs(msgs []sdk.Msg) error { for idx, msg := range msgs { m, ok := msg.(sdk.HasValidateBasic)