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

fix: MsgMultiSend validation allows only a single sender #12610

Merged
merged 6 commits into from
Jul 19, 2022
Merged

fix: MsgMultiSend validation allows only a single sender #12610

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
abc4340
wip
likhita-809 Jul 18, 2022
580861c
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Jul 18, 2022
fe244bd
wip: address review comments
likhita-809 Jul 19, 2022
e2f3642
bump bank's consensus version
likhita-809 Jul 19, 2022
9616d0d
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Jul 19, 2022
e63581a
revert consensus version bump and minor nit
likhita-809 Jul 19, 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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

* (x/bank) [#12610](https://github.com/cosmos/cosmos-sdk/pull/12610) `MsgMultiSend` now allows only a single input.
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Add in UPGRADING.md the alternative. (For me to remember for next version)

* (x/auth) [#12475](https://github.com/cosmos/cosmos-sdk/pull/12475) Migrate `x/auth` to self-managed parameters and deprecate its usage of `x/params`.
* (x/slashing) [#12399](https://github.com/cosmos/cosmos-sdk/pull/12399) Migrate `x/slashing` to self-managed parameters and deprecate its usage of `x/params`.
* (x/mint) [#12363](https://github.com/cosmos/cosmos-sdk/pull/12363) Migrate `x/mint` to self-managed parameters and deprecate it's usage of `x/params`.
Expand Down
217 changes: 79 additions & 138 deletions api/cosmos/bank/v1beta1/tx.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions proto/cosmos/bank/v1beta1/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ message MsgSend {
// MsgSendResponse defines the Msg/Send response type.
message MsgSendResponse {}

// MsgMultiSend represents an arbitrary multi-in, multi-out send message.
// MsgMultiSend represents a single input, multi-out send message.
message MsgMultiSend {
option (cosmos.msg.v1.signer) = "inputs";
option (cosmos.msg.v1.signer) = "input";

option (gogoproto.equal) = false;

repeated Input inputs = 1 [(gogoproto.nullable) = false];
Input input = 1 [(gogoproto.nullable) = false];
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR got merged with a proto breaking change, and the proto package did not get bumped.

We should actually revert this change. @likhita-809 could you create a follow-up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

(thinking about it, my comment #12610 (comment) might have been confusing... I thought there was something to change in the signer proto option, but there's actually nothing to change).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should I revert it to this commit abc4340#diff-4b06aeb8c7fd43a99b1a1c0eb62792aae06649ead661bd26bc843e9a0485336a

Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure which commit it should be, but all the proto changes in this PR should be reverted.

repeated Output outputs = 2 [(gogoproto.nullable) = false];
}

Expand Down
76 changes: 4 additions & 72 deletions x/bank/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,47 +39,31 @@ var (
priv2 = secp256k1.GenPrivKey()
addr2 = sdk.AccAddress(priv2.PubKey().Address())
addr3 = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
priv4 = secp256k1.GenPrivKey()
addr4 = sdk.AccAddress(priv4.PubKey().Address())

coins = sdk.Coins{sdk.NewInt64Coin("foocoin", 10)}
halfCoins = sdk.Coins{sdk.NewInt64Coin("foocoin", 5)}

sendMsg1 = types.NewMsgSend(addr1, addr2, coins)

multiSendMsg1 = &types.MsgMultiSend{
Inputs: []types.Input{types.NewInput(addr1, coins)},
Input: types.NewInput(addr1, coins),
Outputs: []types.Output{types.NewOutput(addr2, coins)},
}
multiSendMsg2 = &types.MsgMultiSend{
Inputs: []types.Input{types.NewInput(addr1, coins)},
Input: types.NewInput(addr1, coins),
Outputs: []types.Output{
types.NewOutput(addr2, halfCoins),
types.NewOutput(addr3, halfCoins),
},
}
multiSendMsg3 = &types.MsgMultiSend{
Inputs: []types.Input{
types.NewInput(addr1, coins),
types.NewInput(addr4, coins),
},
Outputs: []types.Output{
types.NewOutput(addr2, coins),
types.NewOutput(addr3, coins),
},
}
multiSendMsg4 = &types.MsgMultiSend{
Inputs: []types.Input{
types.NewInput(addr2, coins),
},
Input: types.NewInput(addr2, coins),
Outputs: []types.Output{
types.NewOutput(addr1, coins),
},
}
multiSendMsg5 = &types.MsgMultiSend{
Inputs: []types.Input{
types.NewInput(addr1, coins),
},
Input: types.NewInput(addr1, coins),
Outputs: []types.Output{
types.NewOutput(moduleAccAddr, coins),
},
Expand Down Expand Up @@ -234,58 +218,6 @@ func TestMsgMultiSendMultipleOut(t *testing.T) {
}
}

func TestMsgMultiSendMultipleInOut(t *testing.T) {
acc1 := &authtypes.BaseAccount{
Address: addr1.String(),
}
acc2 := &authtypes.BaseAccount{
Address: addr2.String(),
}
acc4 := &authtypes.BaseAccount{
Address: addr4.String(),
}

genAccs := []authtypes.GenesisAccount{acc1, acc2, acc4}
app := simapp.SetupWithGenesisAccounts(t, genAccs)
ctx := app.BaseApp.NewContext(false, tmproto.Header{})

require.NoError(t, testutil.FundAccount(app.BankKeeper, ctx, addr1, sdk.NewCoins(sdk.NewInt64Coin("foocoin", 42))))

require.NoError(t, testutil.FundAccount(app.BankKeeper, ctx, addr2, sdk.NewCoins(sdk.NewInt64Coin("foocoin", 42))))

require.NoError(t, testutil.FundAccount(app.BankKeeper, ctx, addr4, sdk.NewCoins(sdk.NewInt64Coin("foocoin", 42))))

app.Commit()

testCases := []appTestCase{
{
msgs: []sdk.Msg{multiSendMsg3},
accNums: []uint64{0, 2},
accSeqs: []uint64{0, 0},
expSimPass: true,
expPass: true,
privKeys: []cryptotypes.PrivKey{priv1, priv4},
expectedBalances: []expectedBalance{
{addr1, sdk.Coins{sdk.NewInt64Coin("foocoin", 32)}},
{addr4, sdk.Coins{sdk.NewInt64Coin("foocoin", 32)}},
{addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 52)}},
{addr3, sdk.Coins{sdk.NewInt64Coin("foocoin", 10)}},
},
},
}

for _, tc := range testCases {
header := tmproto.Header{Height: app.LastBlockHeight() + 1}
txGen := simapp.MakeTestEncodingConfig().TxConfig
_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, "", tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
require.NoError(t, err)

for _, eb := range tc.expectedBalances {
simapp.CheckBalance(t, app, eb.addr, eb.coins)
}
}
}

func TestMsgMultiSendDependent(t *testing.T) {
acc1 := authtypes.NewBaseAccountWithAddress(addr1)
acc2 := authtypes.NewBaseAccountWithAddress(addr2)
Expand Down
2 changes: 1 addition & 1 deletion x/bank/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ When using '--dry-run' a key name cannot be used, only a bech32 address.
amount = coins.MulInt(totalAddrs)
}

msg := types.NewMsgMultiSend([]types.Input{types.NewInput(clientCtx.FromAddress, amount)}, output)
msg := types.NewMsgMultiSend(types.NewInput(clientCtx.FromAddress, amount), output)

return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
},
Expand Down
75 changes: 34 additions & 41 deletions x/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,14 +359,15 @@ func (suite *IntegrationTestSuite) TestInputOutputNewAccount() {
suite.Require().Nil(app.AccountKeeper.GetAccount(ctx, addr2))
suite.Require().Empty(app.BankKeeper.GetAllBalances(ctx, addr2))

inputs := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
input := types.Input{
Address: addr1.String(),
Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10)),
}
outputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
}

suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))
suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, input, outputs))

expected := sdk.NewCoins(newFooCoin(30), newBarCoin(10))
acc2Balances := app.BankKeeper.GetAllBalances(ctx, addr2)
Expand All @@ -390,30 +391,30 @@ func (suite *IntegrationTestSuite) TestInputOutputCoins() {
acc3 := app.AccountKeeper.NewAccountWithAddress(ctx, addr3)
app.AccountKeeper.SetAccount(ctx, acc3)

inputs := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
input := types.Input{
Address: addr1.String(),
Coins: sdk.NewCoins(newFooCoin(60), newBarCoin(20)),
}
outputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
{Address: addr3.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
}

suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, inputs, []types.Output{}))
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, input, []types.Output{}))
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, input, outputs))

suite.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr1, balances))

insufficientInputs := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))},
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))},
insufficientInput := types.Input{
Address: addr1.String(),
Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100)),
}
insufficientOutputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))},
{Address: addr3.String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))},
}
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, insufficientInputs, insufficientOutputs))
suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, insufficientInput, insufficientOutputs))
suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, input, outputs))

acc1Balances := app.BankKeeper.GetAllBalances(ctx, addr1)
expected := sdk.NewCoins(newFooCoin(30), newBarCoin(10))
Expand Down Expand Up @@ -607,28 +608,29 @@ func (suite *IntegrationTestSuite) TestMsgMultiSendEvents() {
app.AccountKeeper.SetAccount(ctx, acc)
app.AccountKeeper.SetAccount(ctx, acc2)

coins := sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50), sdk.NewInt64Coin(barDenom, 100))
newCoins := sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50))
newCoins2 := sdk.NewCoins(sdk.NewInt64Coin(barDenom, 100))
inputs := []types.Input{
{Address: addr.String(), Coins: newCoins},
{Address: addr2.String(), Coins: newCoins2},
input := types.Input{
Address: addr.String(),
Coins: coins,
}
outputs := []types.Output{
{Address: addr3.String(), Coins: newCoins},
{Address: addr4.String(), Coins: newCoins2},
}

suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, input, outputs))

events := ctx.EventManager().ABCIEvents()
suite.Require().Equal(0, len(events))

// Set addr's coins but not addr2's coins
suite.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr, sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50))))
suite.Require().Error(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))
suite.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr, sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50), sdk.NewInt64Coin(barDenom, 100))))
suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, input, outputs))

events = ctx.EventManager().ABCIEvents()
suite.Require().Equal(8, len(events)) // 7 events because account funding causes extra minting + coin_spent + coin_recv events
suite.Require().Equal(12, len(events)) // 9 events because account funding causes extra minting + coin_spent + coin_recv events

event1 := sdk.Event{
Type: sdk.EventTypeMessage,
Expand All @@ -644,50 +646,41 @@ func (suite *IntegrationTestSuite) TestMsgMultiSendEvents() {
suite.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr, sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50))))
newCoins = sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50))

suite.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr2, sdk.NewCoins(sdk.NewInt64Coin(barDenom, 100))))
suite.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr, sdk.NewCoins(sdk.NewInt64Coin(barDenom, 100))))
newCoins2 = sdk.NewCoins(sdk.NewInt64Coin(barDenom, 100))

suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))
suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, input, outputs))

events = ctx.EventManager().ABCIEvents()
suite.Require().Equal(28, len(events)) // 25 due to account funding + coin_spent + coin_recv events
suite.Require().Equal(30, len(events)) // 27 due to account funding + coin_spent + coin_recv events

event2 := sdk.Event{
Type: sdk.EventTypeMessage,
Type: types.EventTypeTransfer,
Attributes: []abci.EventAttribute{},
}
event2.Attributes = append(
event2.Attributes,
abci.EventAttribute{Key: types.AttributeKeySender, Value: addr2.String()},
abci.EventAttribute{Key: types.AttributeKeyRecipient, Value: addr3.String()},
)
event2.Attributes = append(
event2.Attributes,
abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins.String()})
event3 := sdk.Event{
Type: types.EventTypeTransfer,
Attributes: []abci.EventAttribute{},
}
event3.Attributes = append(
event3.Attributes,
abci.EventAttribute{Key: types.AttributeKeyRecipient, Value: addr3.String()},
abci.EventAttribute{Key: types.AttributeKeyRecipient, Value: addr4.String()},
)
event3.Attributes = append(
event3.Attributes,
abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins.String()})
event4 := sdk.Event{
Type: types.EventTypeTransfer,
Attributes: []abci.EventAttribute{},
}
event4.Attributes = append(
event4.Attributes,
abci.EventAttribute{Key: types.AttributeKeyRecipient, Value: addr4.String()},
)
event4.Attributes = append(
event4.Attributes,
abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins2.String()},
)
// events are shifted due to the funding account events
suite.Require().Equal(abci.Event(event1), events[21])
suite.Require().Equal(abci.Event(event2), events[23])
suite.Require().Equal(abci.Event(event3), events[25])
suite.Require().Equal(abci.Event(event4), events[27])
suite.Require().Equal(abci.Event(event1), events[25])
suite.Require().Equal(abci.Event(event2), events[27])
suite.Require().Equal(abci.Event(event3), events[29])
}

func (suite *IntegrationTestSuite) TestSpendableCoins() {
Expand Down
8 changes: 3 additions & 5 deletions x/bank/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,8 @@ func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*t
ctx := sdk.UnwrapSDKContext(goCtx)

// NOTE: totalIn == totalOut should already have been checked
for _, in := range msg.Inputs {
if err := k.IsSendEnabledCoins(ctx, in.Coins...); err != nil {
return nil, err
}
if err := k.IsSendEnabledCoins(ctx, msg.Input.Coins...); err != nil {
return nil, err
}

for _, out := range msg.Outputs {
Expand All @@ -88,7 +86,7 @@ func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*t
}
}

err := k.InputOutputCoins(ctx, msg.Inputs, msg.Outputs)
err := k.InputOutputCoins(ctx, msg.Input, msg.Outputs)
if err != nil {
return nil, err
}
Expand Down
36 changes: 17 additions & 19 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
type SendKeeper interface {
ViewKeeper

InputOutputCoins(ctx sdk.Context, inputs []types.Input, outputs []types.Output) error
InputOutputCoins(ctx sdk.Context, inputs types.Input, outputs []types.Output) error
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error

GetParams(ctx sdk.Context) types.Params
Expand Down Expand Up @@ -88,32 +88,30 @@ func (k BaseSendKeeper) SetParams(ctx sdk.Context, params types.Params) {
// InputOutputCoins performs multi-send functionality. It accepts a series of
// inputs that correspond to a series of outputs. It returns an error if the
// inputs and outputs don't line up or if any single transfer of tokens fails.
func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, outputs []types.Output) error {
func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, input types.Input, outputs []types.Output) error {
// Safety check ensuring that when sending coins the keeper must maintain the
// Check supply invariant and validity of Coins.
if err := types.ValidateInputsOutputs(inputs, outputs); err != nil {
if err := types.ValidateInputsOutputs(input, outputs); err != nil {
return err
}

for _, in := range inputs {
inAddress, err := sdk.AccAddressFromBech32(in.Address)
if err != nil {
return err
}

err = k.subUnlockedCoins(ctx, inAddress, in.Coins)
if err != nil {
return err
}
inAddress, err := sdk.AccAddressFromBech32(input.Address)
if err != nil {
return err
}

ctx.EventManager().EmitEvent(
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(types.AttributeKeySender, in.Address),
),
)
err = k.subUnlockedCoins(ctx, inAddress, input.Coins)
if err != nil {
return err
}

ctx.EventManager().EmitEvent(
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(types.AttributeKeySender, input.Address),
),
)

for _, out := range outputs {
outAddress, err := sdk.AccAddressFromBech32(out.Address)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion x/bank/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
}

// ConsensusVersion implements AppModule/ConsensusVersion.
func (AppModule) ConsensusVersion() uint64 { return 4 }
func (AppModule) ConsensusVersion() uint64 { return 5 }
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved

// BeginBlock performs a no-op.
func (AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {}
Expand Down
Loading