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

perf(bank): Reduce the number of ValidateDenom calls in Coin and Bank (#24431)

Browse Source 
Co-authored-by: Tuan Tran <tuantran@notional.ventures>
Co-authored-by: Alex | Interchain Labs <alex@interchainlabs.io>
This commit is contained in:
Alexander Peters 2025-04-11 18:25:51 +02:00 committed by GitHub
parent 9236abd4a5
commit 3d8c846802
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 115 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
CHANGELOG.md
View File

@ -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 `<appd> keys add`, `<appd> keys import` and `<appd> keys rename` by checking name validation.

22
types/coin.go
View File

@ -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}"

3
types/coin_test.go
View File

@ -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{}

6
types/dec_coin.go
View File

@ -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()

7
x/bank/keeper/keeper.go
View File

@ -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 {

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

@ -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()

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

@ -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)