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.

Sign up for GitHub

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

refactor(x/bank): move validate basic checks to msg server #15782

Merged
merged 18 commits into from
Apr 12, 2023
Merged

refactor(x/bank): move validate basic checks to msg server #15782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d5849ab
refactor(x/bank): move validate basic checks to msg server
atheeshp Apr 11, 2023
86c8852
remove unreachable test cases
atheeshp Apr 12, 2023
21e40e2
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/remove-ba…
atheeshp Apr 12, 2023
cb17a93
fix tests
atheeshp Apr 12, 2023
a26a8b8
review changes
atheeshp Apr 12, 2023
4401561
review changes
atheeshp Apr 12, 2023
4a3db9d
Merge branch 'main' into ap/remove-bank-validatebasic
mergify[bot] Apr 12, 2023
3c6fdb0
Merge branch 'main' into ap/remove-bank-validatebasic
julienrbrt Apr 12, 2023
2859752
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/remove-ba…
atheeshp Apr 12, 2023
198fa05
Merge branch 'main' into ap/remove-bank-validatebasic
julienrbrt Apr 12, 2023
5715f82
Merge branch 'ap/remove-bank-validatebasic' of github.com:cosmos/cosm…
atheeshp Apr 12, 2023
c8bea60
Merge branch 'main' into ap/remove-bank-validatebasic
atheeshp Apr 12, 2023
d021856
Merge branch 'main' into ap/remove-bank-validatebasic
julienrbrt Apr 12, 2023
9ae8a17
Merge branch 'main' into ap/remove-bank-validatebasic
atheeshp Apr 12, 2023
1f5e576
fix lint
atheeshp Apr 12, 2023
922e00a
Merge branch 'ap/remove-bank-validatebasic' of github.com:cosmos/cosm…
atheeshp Apr 12, 2023
166f47c
lint
atheeshp Apr 12, 2023
0f8f38f
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/remove-ba…
atheeshp Apr 12, 2023
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
14 changes: 0 additions & 14 deletions x/authz/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,20 +352,6 @@ func (suite *TestSuite) TestExec() {
return authz.NewMsgExec(grantee, []sdk.Msg{msg})
},
},
{
name: "invalid nested msg",
malleate: func() authz.MsgExec {
return authz.NewMsgExec(grantee, []sdk.Msg{
&banktypes.MsgSend{
Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)),
FromAddress: "invalid_from_address",
ToAddress: grantee.String(),
},
})
},
expErr: true,
errMsg: "invalid from address",
},
}

for _, tc := range testCases {
Expand Down
25 changes: 0 additions & 25 deletions x/bank/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,18 +369,6 @@ func TestMsgSetSendEnabled(t *testing.T) {
false,
)
require.NoError(t, err, "making goodGovProp")
badGovProp, err := govv1.NewMsgSubmitProposal(
[]sdk.Msg{
types.NewMsgSetSendEnabled(govAddr, []*types.SendEnabled{{Denom: "bad coin name!", Enabled: true}}, nil),
},
sdk.Coins{{Denom: "foocoin", Amount: sdkmath.NewInt(5)}},
addr1Str,
"set default send enabled to true",
"Change send enabled",
"Modify send enabled and set to true",
false,
)
require.NoError(t, err, "making badGovProp")

testCases := []appTestCase{
{
Expand Down Expand Up @@ -423,19 +411,6 @@ func TestMsgSetSendEnabled(t *testing.T) {
accSeqs: []uint64{1},
expInError: nil,
},
{
desc: "submitted bad as gov prop",
expSimPass: false,
expPass: false,
msgs: []sdk.Msg{
badGovProp,
},
accSeqs: []uint64{2},
expInError: []string{
"invalid denom: bad coin name!",
"invalid proposal message",
},
},
Comment on lines -426 to -438
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case executes in endblocker after proposal pass, cannot catch here.

Copy link
Member

@julienrbrt julienrbrt Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

With the removal of validate basic, is there a way to verify the proposal message is correct before its execution? That's a pretty unhandy drawback imho.
We need to find a way to improve submit proposal CLI for avoiding that.

Copy link
Member

Choose a reason for hiding this comment

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

user should be simulating to verify, isnt this enough?

Copy link
Member

@julienrbrt julienrbrt Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For other messages, yes, absolutely, but not for messages with nested messages (like gov or group proposal) I do not think it will work.

Copy link
Member

Choose a reason for hiding this comment

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

just talked with atheesh, We think we should extend simulations to support nested messages because more and more apps are using nested messages with delayed execution

Copy link
Member

Choose a reason for hiding this comment

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

This makes sense and would solve the issue indeed

Copy link
Member

Choose a reason for hiding this comment

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

#15809

}

for _, tc := range testCases {
Expand Down
4 changes: 4 additions & 0 deletions x/bank/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ When using '--dry-run' a key name cannot be used, only a bech32 address.
return err
}

if len(coins) == 0 {
return fmt.Errorf("invalid coins")
}

msg := types.NewMsgSend(clientCtx.GetFromAddress(), toAddr, coins)

return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
Expand Down
61 changes: 54 additions & 7 deletions x/bank/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,26 @@ func NewMsgServerImpl(keeper Keeper) types.MsgServer {
}

func (k msgServer) Send(goCtx context.Context, msg *types.MsgSend) (*types.MsgSendResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

if err := k.IsSendEnabledCoins(ctx, msg.Amount...); err != nil {
return nil, err
}

from, err := sdk.AccAddressFromBech32(msg.FromAddress)
if err != nil {
return nil, err
return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid from address: %s", err)
}

to, err := sdk.AccAddressFromBech32(msg.ToAddress)
if err != nil {
return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid to address: %s", err)
}

if !msg.Amount.IsValid() {
return nil, errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String())
}

if !msg.Amount.IsAllPositive() {
return nil, errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String())
}

ctx := sdk.UnwrapSDKContext(goCtx)
if err := k.IsSendEnabledCoins(ctx, msg.Amount...); err != nil {
return nil, err
}

Expand Down Expand Up @@ -67,6 +75,22 @@ func (k msgServer) Send(goCtx context.Context, msg *types.MsgSend) (*types.MsgSe
}

func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*types.MsgMultiSendResponse, error) {
if len(msg.Inputs) == 0 {
return nil, types.ErrNoInputs
}

if len(msg.Inputs) != 1 {
return nil, types.ErrMultipleSenders
}

if len(msg.Outputs) == 0 {
return nil, types.ErrNoOutputs
}

if err := types.ValidateInputOutputs(msg.Inputs[0], msg.Outputs); err != nil {
return nil, err
}

ctx := sdk.UnwrapSDKContext(goCtx)

// NOTE: totalIn == totalOut should already have been checked
Expand Down Expand Up @@ -97,6 +121,10 @@ func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParam
return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority)
}

if err := req.Params.Validate(); err != nil {
return nil, err
}

ctx := sdk.UnwrapSDKContext(goCtx)
if err := k.SetParams(ctx, req.Params); err != nil {
return nil, err
Expand All @@ -110,6 +138,25 @@ func (k msgServer) SetSendEnabled(goCtx context.Context, msg *types.MsgSetSendEn
return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority)
}

seen := map[string]bool{}
for _, se := range msg.SendEnabled {
if _, alreadySeen := seen[se.Denom]; alreadySeen {
return nil, sdkerrors.ErrInvalidRequest.Wrapf("duplicate denom entries found for %q", se.Denom)
}

seen[se.Denom] = true

if err := se.Validate(); err != nil {
return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid SendEnabled denom %q: %s", se.Denom, err)
}
}

for _, denom := range msg.UseDefaultFor {
if err := sdk.ValidateDenom(denom); err != nil {
return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid UseDefaultFor denom %q: %s", denom, err)
}
}

ctx := sdk.UnwrapSDKContext(goCtx)
if len(msg.SendEnabled) > 0 {
k.SetAllSendEnabled(ctx, msg.SendEnabled)
Expand Down
Loading