LaconicNetwork/cosmos-sdk
10
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 2 Wiki Activity

refactor(x/bank): swap sendrestriction check in InputOutputCoins (backport #21976) (#24053)

Browse Source 
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Alex | Interchain Labs <alex@interchainlabs.io>
This commit is contained in:
mergify[bot] 2025-03-25 21:04:33 +00:00 committed by GitHub
parent f78a75ce96
commit 8a54f41c7d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 94 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
CHANGELOG.md
View File

@ -59,6 +59,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (crypto/ledger) [#24036](https://github.com/cosmos/cosmos-sdk/pull/24036) Improve error message when deriving paths using index > 100
* (gRPC) [#23844](https://github.com/cosmos/cosmos-sdk/pull/23844) Add debug log prints for each gRPC request.
* (server) [#24072](https://github.com/cosmos/cosmos-sdk/pull/24072) Return BlockHeader by shallow copy in server Context.
* (x/bank) [#24053](https://github.com/cosmos/cosmos-sdk/pull/24053) Resolve a foot-gun by swapping send restrictions check in `InputOutputCoins` before coin deduction.
### Bug Fixes

78
x/bank/keeper/keeper_test.go
View File

@ -675,11 +675,16 @@ func (suite *KeeperTestSuite) TestInputOutputCoins() {
{Address: accAddrs[2].String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
}
require.Error(suite.bankKeeper.InputOutputCoins(ctx, input, []banktypes.Output{}))
// test that inputs with no outputs fails
require.ErrorContains(suite.bankKeeper.InputOutputCoins(ctx, input, []banktypes.Output{}), banktypes.ErrInputOutputMismatch.Error())
// accounts has no funds, should error.
suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0)
require.Error(suite.bankKeeper.InputOutputCoins(ctx, input, outputs))
suite.authKeeper.EXPECT().HasAccount(suite.ctx, gomock.Any()).Return(true).Times(len(outputs))
err := suite.bankKeeper.InputOutputCoins(ctx, input, outputs)
require.ErrorContains(err, "insufficient funds")
// fund account now.
suite.mockFundAccount(accAddrs[0])
require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[0], balances))
@ -692,15 +697,17 @@ func (suite *KeeperTestSuite) TestInputOutputCoins() {
{Address: accAddrs[2].String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))},
}
require.Error(suite.bankKeeper.InputOutputCoins(ctx, insufficientInput, insufficientOutputs))
// input: 300foo,100bar ==> output: 600foo,200bar. should fail
err = suite.bankKeeper.InputOutputCoins(ctx, insufficientInput, insufficientOutputs)
require.ErrorContains(err, banktypes.ErrInputOutputMismatch.Error())
// should work with valid input/outputs.
suite.mockInputOutputCoins([]sdk.AccountI{acc0}, accAddrs[1:3])
require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs))
acc1Balances := suite.bankKeeper.GetAllBalances(ctx, accAddrs[0])
expected := sdk.NewCoins(newFooCoin(30), newBarCoin(10))
require.Equal(expected, acc1Balances)
acc2Balances := suite.bankKeeper.GetAllBalances(ctx, accAddrs[1])
require.Equal(expected, acc2Balances)
@ -708,6 +715,39 @@ func (suite *KeeperTestSuite) TestInputOutputCoins() {
require.Equal(expected, acc3Balances)
}
func (suite *KeeperTestSuite) TestInputOutputCoins_AccountCreated() {
ctx := suite.ctx
require := suite.Require()
balances := sdk.NewCoins(newFooCoin(90), newBarCoin(30))
acc0 := authtypes.NewBaseAccountWithAddress(accAddrs[0])
input := banktypes.Input{
Address: accAddrs[0].String(), Coins: sdk.NewCoins(newFooCoin(60), newBarCoin(20)),
}
outputs := []banktypes.Output{
{Address: accAddrs[1].String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
{Address: accAddrs[2].String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
}
suite.mockFundAccount(accAddrs[0])
require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[0], balances))
// the input account should be retrieved.
suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0)
// creates output account 1
suite.authKeeper.EXPECT().HasAccount(suite.ctx, accAddrs[1].Bytes()).Return(false)
suite.authKeeper.EXPECT().NewAccountWithAddress(suite.ctx, accAddrs[1].Bytes())
suite.authKeeper.EXPECT().SetAccount(suite.ctx, gomock.Any())
// creates output account 2
suite.authKeeper.EXPECT().HasAccount(suite.ctx, accAddrs[2].Bytes()).Return(false)
suite.authKeeper.EXPECT().NewAccountWithAddress(suite.ctx, accAddrs[2].Bytes())
suite.authKeeper.EXPECT().SetAccount(suite.ctx, gomock.Any())
require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs))
}
func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
type restrictionArgs struct {
ctx context.Context
@ -774,6 +814,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
fromAddr := accAddrs[0]
fromAcc := authtypes.NewBaseAccountWithAddress(fromAddr)
inputAccs := []sdk.AccountI{fromAcc}
suite.authKeeper.EXPECT().GetAccount(suite.ctx, inputAccs[0].GetAddress()).Return(inputAccs[0]).AnyTimes()
toAddr1 := accAddrs[1]
toAddr2 := accAddrs[2]
@ -858,7 +899,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
expErr: "restriction test error",
expBals: expBals{
from: sdk.NewCoins(newFooCoin(959), newBarCoin(412)),
from: sdk.NewCoins(newFooCoin(959), newBarCoin(500)),
to1: sdk.NewCoins(newFooCoin(15)),
to2: sdk.NewCoins(newFooCoin(26)),
},
@ -887,7 +928,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
},
expBals: expBals{
from: sdk.NewCoins(newFooCoin(948), newBarCoin(400)),
from: sdk.NewCoins(newFooCoin(948), newBarCoin(488)),
to1: sdk.NewCoins(newFooCoin(26)),
to2: sdk.NewCoins(newFooCoin(26), newBarCoin(12)),
},
@ -917,8 +958,8 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
expErr: "second restriction error",
expBals: expBals{
from: sdk.NewCoins(newFooCoin(904), newBarCoin(400)),
to1: sdk.NewCoins(newFooCoin(38)),
from: sdk.NewCoins(newFooCoin(948), newBarCoin(488)),
to1: sdk.NewCoins(newFooCoin(26)),
to2: sdk.NewCoins(newFooCoin(26), newBarCoin(12)),
},
},
@ -946,8 +987,8 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
},
expBals: expBals{
from: sdk.NewCoins(newFooCoin(904), newBarCoin(365)),
to1: sdk.NewCoins(newFooCoin(38), newBarCoin(25)),
from: sdk.NewCoins(newFooCoin(948), newBarCoin(453)),
to1: sdk.NewCoins(newFooCoin(26), newBarCoin(25)),
to2: sdk.NewCoins(newFooCoin(26), newBarCoin(22)),
},
},
@ -960,7 +1001,6 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
actualRestrictionArgs = nil
suite.bankKeeper.SetSendRestriction(tc.fn)
ctx := suite.ctx
suite.mockInputOutputCoins(inputAccs, tc.outputAddrs)
input := banktypes.Input{
Address: fromAddr.String(),
Coins: tc.inputCoins,
@ -970,6 +1010,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
testFunc := func() {
err = suite.bankKeeper.InputOutputCoins(ctx, input, tc.outputs)
}
suite.authKeeper.EXPECT().HasAccount(gomock.Any(), gomock.Any()).Return(true).Times(len(tc.outputAddrs))
suite.Require().NotPanics(testFunc, "InputOutputCoins")
if len(tc.expErr) > 0 {
suite.Assert().EqualError(err, tc.expErr, "InputOutputCoins error")
@ -1404,10 +1445,12 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
}
suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0)
suite.authKeeper.EXPECT().HasAccount(gomock.Any(), gomock.Any()).Return(true).Times(len(outputs))
require.Error(suite.bankKeeper.InputOutputCoins(ctx, input, outputs))
events := ctx.EventManager().ABCIEvents()
require.Equal(0, len(events))
require.Equal(1, len(events))
// Set addr's coins but not accAddrs[1]'s coins
suite.mockFundAccount(accAddrs[0])
@ -1417,7 +1460,7 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs))
events = ctx.EventManager().ABCIEvents()
require.Equal(12, len(events)) // 12 events because account funding causes extra minting + coin_spent + coin_recv events
require.Equal(13, len(events)) // 13 events because account funding causes extra minting + coin_spent + coin_recv events
event1 := sdk.Event{
Type: sdk.EventTypeMessage,
@ -1442,7 +1485,7 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs))
events = ctx.EventManager().ABCIEvents()
require.Equal(30, len(events)) // 27 due to account funding + coin_spent + coin_recv events
require.Equal(31, len(events)) // 31 due to account funding + coin_spent + coin_recv events
event2 := sdk.Event{
Type: banktypes.EventTypeTransfer,
@ -1464,10 +1507,9 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
abci.EventAttribute{Key: banktypes.AttributeKeySender, Value: accAddrs[0].String()},
abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins2.String()},
)
// events are shifted due to the funding account events
require.Equal(abci.Event(event1), events[25])
require.Equal(abci.Event(event2), events[27])
require.Equal(abci.Event(event3), events[29])
require.Contains(events, abci.Event(event1))
require.Contains(events, abci.Event(event2))
require.Contains(events, abci.Event(event3))
}
func (suite *KeeperTestSuite) TestSpendableCoins() {

51
x/bank/keeper/send.go
View File

@ -152,11 +152,6 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input,
return err
}
err = k.subUnlockedCoins(ctx, inAddress, input.Coins)
if err != nil {
return err
}
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx.EventManager().EmitEvent(
sdk.NewEvent(
@ -165,9 +160,16 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input,
),
)
var outAddress sdk.AccAddress
// ensure all coins can be sent
type toSend struct {
AddressStr string
Address []byte
Coins sdk.Coins
}
sending := make([]toSend, 0)
for _, out := range outputs {
outAddress, err = k.ak.AddressCodec().StringToBytes(out.Address)
outAddress, err := k.ak.AddressCodec().StringToBytes(out.Address)
if err != nil {
return err
}
@ -177,18 +179,11 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input,
return err
}
if err := k.addCoins(ctx, outAddress, out.Coins); err != nil {
return err
}
sdkCtx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeTransfer,
sdk.NewAttribute(types.AttributeKeyRecipient, outAddress.String()),
sdk.NewAttribute(types.AttributeKeySender, input.Address),
sdk.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()),
),
)
sending = append(sending, toSend{
Address: outAddress,
AddressStr: out.Address,
Coins: out.Coins,
})
// Create account if recipient does not exist.
//
@ -201,6 +196,24 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input,
}
}
if err := k.subUnlockedCoins(ctx, inAddress, input.Coins); err != nil {
return err
}
for _, out := range sending {
if err := k.addCoins(ctx, out.Address, out.Coins); err != nil {
return err
}
sdkCtx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeTransfer,
sdk.NewAttribute(types.AttributeKeyRecipient, out.AddressStr),
sdk.NewAttribute(types.AttributeKeySender, input.Address),
sdk.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()),
),
)
}
return nil
}