From 3d8c84680266d80eed501f389e35157d1e059116 Mon Sep 17 00:00:00 2001 From: Alexander Peters Date: Fri, 11 Apr 2025 18:25:51 +0200 Subject: [PATCH] perf(bank): Reduce the number of `ValidateDenom` calls in Coin and Bank (#24431) Co-authored-by: Tuan Tran Co-authored-by: Alex | Interchain Labs --- CHANGELOG.md | 2 + types/coin.go | 22 ++++------- types/coin_test.go | 3 +- types/dec_coin.go | 6 +-- x/bank/keeper/keeper.go | 7 ++++ x/bank/keeper/keeper_test.go | 73 ++++++++++++++++++++++++++++++++++++ x/bank/keeper/send.go | 35 +++++++++-------- 7 files changed, 115 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aef7de410c..0a9473a526 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (x/bank) [#24431](https://github.com/cosmos/cosmos-sdk/pull/24431) Reduce the number of `ValidateDenom` calls in `bank.SendCoins` and `Coin`. + * The `AmountOf()` method on`sdk.Coins` no longer will `panic` if given an invalid denom and will instead return a zero value. * (x/staking) [#24391](https://github.com/cosmos/cosmos-sdk/pull/24391) Replace panics with error results; more verbose error messages * (x/staking) [#24354](https://github.com/cosmos/cosmos-sdk/pull/24354) Optimize validator endblock by reducing bech32 conversions, resulting in significant performance improvement * (client/keys) [#18950](https://github.com/cosmos/cosmos-sdk/pull/18950) Improve ` keys add`, ` keys import` and ` keys rename` by checking name validation. diff --git a/types/coin.go b/types/coin.go index a052208289..a9054ac506 100644 --- a/types/coin.go +++ b/types/coin.go @@ -690,21 +690,21 @@ func (coins Coins) Empty() bool { return len(coins) == 0 } -// AmountOf returns the amount of a denom from coins +// AmountOf returns the amount of a denom from coins. The denom is not validated. func (coins Coins) AmountOf(denom string) math.Int { - mustValidateDenom(denom) - return coins.AmountOfNoDenomValidation(denom) -} - -// AmountOfNoDenomValidation returns the amount of a denom from coins -// without validating the denomination. -func (coins Coins) AmountOfNoDenomValidation(denom string) math.Int { if ok, c := coins.Find(denom); ok { return c.Amount } return math.ZeroInt() } +// AmountOfNoDenomValidation returns the amount of a denom from coins +// without validating the denomination. +// Deprecated: use AmountOf +func (coins Coins) AmountOfNoDenomValidation(denom string) math.Int { + return coins.AmountOf(denom) +} + // Find returns true and coin if the denom exists in coins. Otherwise it returns false // and a zero coin. Uses binary search. // CONTRACT: coins must be valid (sorted). @@ -879,12 +879,6 @@ func ValidateDenom(denom string) error { return nil } -func mustValidateDenom(denom string) { - if err := ValidateDenom(denom); err != nil { - panic(err) - } -} - // ParseCoinNormalized parses and normalize a cli input for one coin type, returning errors if invalid or on an empty string // as well. // Expected format: "{amount}{denomination}" diff --git a/types/coin_test.go b/types/coin_test.go index a1a53baa6a..081bf7c780 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -1130,8 +1130,9 @@ func (s *coinTestSuite) TestSearch() { require.Equal(math.NewInt(tc.amountOfGAS), tc.coins.AmountOf("gas"), i) require.Equal(math.NewInt(tc.amountOfMINERAL), tc.coins.AmountOf("mineral"), i) require.Equal(math.NewInt(tc.amountOfTREE), tc.coins.AmountOf("tree"), i) + require.Equal(math.NewInt(tc.amountOfTREE), tc.coins.AmountOf("tree"), i) } - require.Panics(func() { amountOfCases[0].coins.AmountOf("10Invalid") }) + require.Equal(math.ZeroInt(), amountOfCases[0].coins.AmountOf("10InvalidDenom")) }) zeroCoin := sdk.Coin{} diff --git a/types/dec_coin.go b/types/dec_coin.go index 7e1d16b481..7a431b69c3 100644 --- a/types/dec_coin.go +++ b/types/dec_coin.go @@ -24,7 +24,9 @@ func NewDecCoin(denom string, amount math.Int) DecCoin { // NewDecCoinFromDec creates a new DecCoin instance from a Dec. func NewDecCoinFromDec(denom string, amount math.LegacyDec) DecCoin { - mustValidateDenom(denom) + if err := ValidateDenom(denom); err != nil { + panic(err) + } if amount.IsNegative() { panic(fmt.Sprintf("negative decimal coin amount: %v\n", amount)) @@ -453,8 +455,6 @@ func (coins DecCoins) Empty() bool { // AmountOf returns the amount of a denom from deccoins func (coins DecCoins) AmountOf(denom string) math.LegacyDec { - mustValidateDenom(denom) - switch len(coins) { case 0: return math.LegacyZeroDec() diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go index bfa45d23f6..6883e60aa5 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -357,6 +357,10 @@ func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sd panic(errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to mint tokens", moduleName)) } + if !amounts.IsValid() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amounts.String()) + } + err = k.addCoins(ctx, acc.GetAddress(), amounts) if err != nil { return err @@ -389,6 +393,9 @@ func (k BaseKeeper) BurnCoins(ctx context.Context, moduleName string, amounts sd if !acc.HasPermission(authtypes.Burner) { panic(errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to burn tokens", moduleName)) } + if !amounts.IsValid() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amounts.String()) + } err := k.subUnlockedCoins(ctx, acc.GetAddress(), amounts) if err != nil { diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index e21d475df3..0952c34d8a 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -1051,6 +1051,11 @@ func (suite *KeeperTestSuite) TestSendCoins() { suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0) require.Error(suite.bankKeeper.SendCoins(ctx, accAddrs[0], accAddrs[1], sendAmt)) + // invalid denom rejected + invalidDenomAmounts := []sdk.Coin{newFooCoin(50), {Denom: "123fox", Amount: math.OneInt()}} + gotErr := suite.bankKeeper.SendCoins(ctx, accAddrs[0], accAddrs[1], invalidDenomAmounts) + require.ErrorIs(gotErr, sdkerrors.ErrInvalidCoins) + suite.mockFundAccount(accAddrs[0]) require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[0], balances)) suite.mockSendCoins(ctx, acc0, accAddrs[1]) @@ -1987,6 +1992,36 @@ func (suite *KeeperTestSuite) getTestMetadata() []banktypes.Metadata { } } +func (suite *KeeperTestSuite) TestMintCoinDenomGuard() { + specs := map[string]struct { + amounts sdk.Coins + expErr error + }{ + "valid": { + amounts: sdk.NewCoins(sdk.Coin{Denom: "stake", Amount: math.OneInt()}), + }, + "invalid denom": { + amounts: []sdk.Coin{{Denom: "11stake", Amount: math.OneInt()}}, + expErr: sdkerrors.ErrInvalidCoins, + }, + "invalid denom - multiple": { + amounts: []sdk.Coin{newFooCoin(50), {Denom: "11stake", Amount: math.OneInt()}}, + expErr: sdkerrors.ErrInvalidCoins, + }, + } + for name, spec := range specs { + suite.T().Run(name, func(t *testing.T) { + suite.mockMintCoins(multiPermAcc) + gotErr := suite.bankKeeper.MintCoins(suite.ctx, multiPermAcc.Name, spec.amounts) + if spec.expErr != nil { + suite.Require().ErrorIs(gotErr, spec.expErr) + return + } + suite.Require().NoError(gotErr) + }) + } +} + func (suite *KeeperTestSuite) TestMintCoinRestrictions() { type BankMintingRestrictionFn func(ctx context.Context, coins sdk.Coins) error require := suite.Require() @@ -2049,6 +2084,44 @@ func (suite *KeeperTestSuite) TestMintCoinRestrictions() { } } +func (suite *KeeperTestSuite) TestBurnCoinDenomGuard() { + suite.mockMintCoins(multiPermAcc) + myCoins := sdk.NewCoins(sdk.NewCoin("stake", math.OneInt())) + suite.Require().NoError(suite.bankKeeper.MintCoins(suite.ctx, multiPermAcc.Name, myCoins)) + + specs := map[string]struct { + amounts sdk.Coins + expErr error + }{ + "valid": { + amounts: sdk.NewCoins(sdk.Coin{Denom: "stake", Amount: math.OneInt()}), + }, + "invalid denom": { + amounts: []sdk.Coin{{Denom: "11stake", Amount: math.OneInt()}}, + expErr: sdkerrors.ErrInvalidCoins, + }, + "invalid denom - multiple": { + amounts: []sdk.Coin{newFooCoin(50), {Denom: "11stake", Amount: math.OneInt()}}, + expErr: sdkerrors.ErrInvalidCoins, + }, + } + for name, spec := range specs { + suite.T().Run(name, func(t *testing.T) { + suite.authKeeper.EXPECT().GetModuleAccount(suite.ctx, multiPermAcc.Name).Return(multiPermAcc) + if spec.expErr == nil { + suite.authKeeper.EXPECT().GetAccount(suite.ctx, multiPermAcc.GetAddress()).Return(multiPermAcc) + } + // when + gotErr := suite.bankKeeper.BurnCoins(suite.ctx, multiPermAcc.Name, spec.amounts) + if spec.expErr != nil { + suite.Require().ErrorIs(gotErr, spec.expErr) + return + } + suite.Require().NoError(gotErr) + }) + } +} + func (suite *KeeperTestSuite) TestIsSendEnabledDenom() { ctx, bankKeeper := suite.ctx, suite.bankKeeper require := suite.Require() diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index a4a35c14fa..96fa6f2a08 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -220,6 +220,10 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, // SendCoins transfers amt coins from a sending account to a receiving account. // An error is returned upon failure. func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error { + if !amt.IsValid() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) + } + var err error toAddr, err = k.sendRestriction.apply(ctx, fromAddr, toAddr, amt) if err != nil { @@ -265,19 +269,21 @@ func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccA return nil } -// subUnlockedCoins removes the unlocked amt coins of the given account. An error is -// returned if the resulting balance is negative or the initial amount is invalid. -// A coin_spent event is emitted after. +// subUnlockedCoins removes the unlocked amt coins of the given account. +// An error is returned if the resulting balance is negative. +// +// CONTRACT: The provided amount (amt) must be valid, non-negative coins. +// +// A coin_spent event is emitted after the operation. func (k BaseSendKeeper) subUnlockedCoins(ctx context.Context, addr sdk.AccAddress, amt sdk.Coins) error { - if !amt.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) - } - lockedCoins := k.LockedCoins(ctx, addr) for _, coin := range amt { balance := k.GetBalance(ctx, addr, coin.Denom) - locked := sdk.NewCoin(coin.Denom, lockedCoins.AmountOf(coin.Denom)) + ok, locked := lockedCoins.Find(coin.Denom) + if !ok { + locked = sdk.Coin{Denom: coin.Denom, Amount: math.ZeroInt()} + } spendable, hasNeg := sdk.Coins{balance}.SafeSub(locked) if hasNeg { @@ -287,7 +293,7 @@ func (k BaseSendKeeper) subUnlockedCoins(ctx context.Context, addr sdk.AccAddres if _, hasNeg := spendable.SafeSub(coin); hasNeg { if len(spendable) == 0 { - spendable = sdk.Coins{sdk.NewCoin(coin.Denom, math.ZeroInt())} + spendable = sdk.Coins{sdk.Coin{Denom: coin.Denom, Amount: math.ZeroInt()}} } return errorsmod.Wrapf( sdkerrors.ErrInsufficientFunds, @@ -311,13 +317,12 @@ func (k BaseSendKeeper) subUnlockedCoins(ctx context.Context, addr sdk.AccAddres return nil } -// addCoins increase the addr balance by the given amt. Fails if the provided -// amt is invalid. It emits a coin received event. +// addCoins increases the balance of the given address by the specified amount. +// +// CONTRACT: The provided amount (amt) must be valid, non-negative coins. +// +// It emits a coin_received event after the operation. func (k BaseSendKeeper) addCoins(ctx context.Context, addr sdk.AccAddress, amt sdk.Coins) error { - if !amt.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) - } - for _, coin := range amt { balance := k.GetBalance(ctx, addr, coin.Denom) newBalance := balance.Add(coin)