From 97ae555f939041b6d43016c4d0cc963a98cc6307 Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Mon, 23 Jan 2023 15:00:03 +0200 Subject: [PATCH 1/5] fix: replace the Coin.IsEqual with Coin.Equal The method IsEqual throw panic if the denoms of two coins are not equal. This lead to unexpected behavior of the, becasue in most of the cases the users expected an error to be returned Issue: #3246 --- types/coin.go | 2 +- types/coin_test.go | 24 +++++++++--------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/types/coin.go b/types/coin.go index 5a2e2970f895..063dc2354db0 100644 --- a/types/coin.go +++ b/types/coin.go @@ -662,7 +662,7 @@ func (coins Coins) IsEqual(coinsB Coins) bool { coinsB = coinsB.Sort() for i := 0; i < len(coins); i++ { - if !coins[i].IsEqual(coinsB[i]) { + if !coins[i].Equal(coinsB[i]) { return false } } diff --git a/types/coin_test.go b/types/coin_test.go index cb012b55d5d8..3ccd5e2e2aba 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -513,25 +513,19 @@ func (s *coinTestSuite) TestEqualCoins() { inputOne sdk.Coins inputTwo sdk.Coins expected bool - panics bool }{ - {sdk.Coins{}, sdk.Coins{}, true, false}, - {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, true, false}, - {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, true, false}, - {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom2, 0)}, false, true}, - {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 1)}, false, false}, - {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, false, false}, - {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, true, false}, + {sdk.Coins{}, sdk.Coins{}, true}, + {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, true}, + {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, true}, + {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom2, 0)}, false}, + {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 1)}, false}, + {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, false}, + {sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, sdk.Coins{sdk.NewInt64Coin(testDenom1, 0), sdk.NewInt64Coin(testDenom2, 1)}, true}, } for tcnum, tc := range cases { - tc := tc - if tc.panics { - s.Require().Panics(func() { tc.inputOne.IsEqual(tc.inputTwo) }) - } else { - res := tc.inputOne.IsEqual(tc.inputTwo) - s.Require().Equal(tc.expected, res, "Equality is differed from exported. tc #%d, expected %b, actual %b.", tcnum, tc.expected, res) - } + res := tc.inputOne.IsEqual(tc.inputTwo) + s.Require().Equal(tc.expected, res, "Equality is differed from exported. tc #%d, expected %b, actual %b.", tcnum, tc.expected, res) } } From 699b1853d9273283135092bf9b1ce46bfade5f21 Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Mon, 23 Jan 2023 15:21:47 +0200 Subject: [PATCH 2/5] hygiene: rename method IsEqual to Equal Rename method IsEqual of the type Coins to Equal to follow standardize terminology for 'Equal; function on structs Refs: #3246 --- .../distribution/keeper/delegation_test.go | 2 +- types/coin.go | 8 ++++---- types/coin_test.go | 10 +++++----- x/auth/migrations/v2/store_test.go | 4 ++-- x/auth/tx/builder.go | 2 +- x/auth/vesting/types/vesting_account.go | 2 +- x/auth/vesting/types/vesting_account_test.go | 12 ++++++------ x/bank/app_test.go | 2 +- x/bank/keeper/genesis.go | 2 +- x/bank/keeper/invariants.go | 2 +- x/bank/types/genesis.go | 2 +- x/bank/types/msgs.go | 2 +- x/distribution/keeper/delegation_test.go | 2 +- x/distribution/keeper/genesis.go | 2 +- x/distribution/keeper/invariants.go | 2 +- x/gov/abci_test.go | 4 ++-- x/gov/genesis.go | 2 +- x/gov/keeper/deposit_test.go | 2 +- x/gov/types/v1beta1/params.go | 2 +- x/slashing/app_test.go | 4 ++-- x/staking/app_test.go | 12 ++++++------ x/staking/keeper/genesis.go | 4 ++-- 22 files changed, 43 insertions(+), 43 deletions(-) diff --git a/tests/integration/distribution/keeper/delegation_test.go b/tests/integration/distribution/keeper/delegation_test.go index 49ad3341da3d..d5668bc6bd34 100644 --- a/tests/integration/distribution/keeper/delegation_test.go +++ b/tests/integration/distribution/keeper/delegation_test.go @@ -805,7 +805,7 @@ func Test100PercentCommissionReward(t *testing.T) { assert.NilError(t, err) zeroRewards := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, math.ZeroInt())} - assert.Assert(t, rewards.IsEqual(zeroRewards)) + assert.Assert(t, rewards.Equal(zeroRewards)) events := ctx.EventManager().Events() lastEvent := events[len(events)-1] diff --git a/types/coin.go b/types/coin.go index 063dc2354db0..662bbe9735d7 100644 --- a/types/coin.go +++ b/types/coin.go @@ -463,7 +463,7 @@ func (coins Coins) SafeQuoInt(x Int) (Coins, bool) { // a.IsAllLTE(a.Max(b)) // b.IsAllLTE(a.Max(b)) // a.IsAllLTE(c) && b.IsAllLTE(c) == a.Max(b).IsAllLTE(c) -// a.Add(b...).IsEqual(a.Min(b).Add(a.Max(b)...)) +// a.Add(b...).Equal(a.Min(b).Add(a.Max(b)...)) // // E.g. // {1A, 3B, 2C}.Max({4A, 2B, 2C} == {4A, 3B, 2C}) @@ -509,7 +509,7 @@ func (coins Coins) Max(coinsB Coins) Coins { // a.Min(b).IsAllLTE(a) // a.Min(b).IsAllLTE(b) // c.IsAllLTE(a) && c.IsAllLTE(b) == c.IsAllLTE(a.Min(b)) -// a.Add(b...).IsEqual(a.Min(b).Add(a.Max(b)...)) +// a.Add(b...).Equal(a.Min(b).Add(a.Max(b)...)) // // E.g. // {1A, 3B, 2C}.Min({4A, 2B, 2C} == {1A, 2B, 2C}) @@ -652,8 +652,8 @@ func (coins Coins) IsZero() bool { return true } -// IsEqual returns true if the two sets of Coins have the same value -func (coins Coins) IsEqual(coinsB Coins) bool { +// Equal returns true if the two sets of Coins have the same value +func (coins Coins) Equal(coinsB Coins) bool { if len(coins) != len(coinsB) { return false } diff --git a/types/coin_test.go b/types/coin_test.go index 3ccd5e2e2aba..2cebe11f2c1c 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -524,7 +524,7 @@ func (s *coinTestSuite) TestEqualCoins() { } for tcnum, tc := range cases { - res := tc.inputOne.IsEqual(tc.inputTwo) + res := tc.inputOne.Equal(tc.inputTwo) s.Require().Equal(tc.expected, res, "Equality is differed from exported. tc #%d, expected %b, actual %b.", tcnum, tc.expected, res) } } @@ -573,7 +573,7 @@ func TestCoinsAddCoalescesDuplicateDenominations(t *testing.T) { {"den", sdk.NewInt(11)}, } - if !got.IsEqual(want) { + if !got.Equal(want) { t.Fatalf("Wrong result\n\tGot: %s\n\tWant: %s", got, want) } } @@ -865,8 +865,8 @@ func (s *coinTestSuite) TestMinMax() { for _, tc := range cases { min := tc.input1.Min(tc.input2) max := tc.input1.Max(tc.input2) - s.Require().True(min.IsEqual(tc.min), tc.name) - s.Require().True(max.IsEqual(tc.max), tc.name) + s.Require().True(min.Equal(tc.min), tc.name) + s.Require().True(max.Equal(tc.max), tc.name) } } @@ -1168,7 +1168,7 @@ func (s *coinTestSuite) TestNewCoins() { continue } got := sdk.NewCoins(tt.coins...) - s.Require().True(got.IsEqual(tt.want)) + s.Require().True(got.Equal(tt.want)) } } diff --git a/x/auth/migrations/v2/store_test.go b/x/auth/migrations/v2/store_test.go index 9ff1e92ab840..725b21945ebb 100644 --- a/x/auth/migrations/v2/store_test.go +++ b/x/auth/migrations/v2/store_test.go @@ -645,8 +645,8 @@ func trackingCorrected(ctx sdk.Context, t *testing.T, ak keeper.AccountKeeper, a vDA, ok := baseAccount.(exported.VestingAccount) require.True(t, ok) - vestedOk := expDelVesting.IsEqual(vDA.GetDelegatedVesting()) - freeOk := expDelFree.IsEqual(vDA.GetDelegatedFree()) + vestedOk := expDelVesting.Equal(vDA.GetDelegatedVesting()) + freeOk := expDelFree.Equal(vDA.GetDelegatedFree()) require.True(t, vestedOk, vDA.GetDelegatedVesting().String()) require.True(t, freeOk, vDA.GetDelegatedFree().String()) } diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index 2bee58151cff..2c4da2181fa8 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -426,7 +426,7 @@ func (w *wrapper) AddAuxSignerData(data tx.AuxSignerData) error { } } if w.tx.AuthInfo.Tip != nil && data.SignDoc.Tip != nil { - if !w.tx.AuthInfo.Tip.Amount.IsEqual(data.SignDoc.Tip.Amount) { + if !w.tx.AuthInfo.Tip.Amount.Equal(data.SignDoc.Tip.Amount) { return sdkerrors.ErrInvalidRequest.Wrapf("TxBuilder has tip %+v, got %+v in AuxSignerData", w.tx.AuthInfo.Tip.Amount, data.SignDoc.Tip.Amount) } if w.tx.AuthInfo.Tip.Tipper != data.SignDoc.Tip.Tipper { diff --git a/x/auth/vesting/types/vesting_account.go b/x/auth/vesting/types/vesting_account.go index 26a95d84d9d0..e2581a8d22e2 100644 --- a/x/auth/vesting/types/vesting_account.go +++ b/x/auth/vesting/types/vesting_account.go @@ -352,7 +352,7 @@ func (pva PeriodicVestingAccount) Validate() error { if endTime != pva.EndTime { return errors.New("vesting end time does not match length of all vesting periods") } - if !originalVesting.IsEqual(pva.OriginalVesting) { + if !originalVesting.Equal(pva.OriginalVesting) { return errors.New("original vesting coins does not match the sum of all coins in vesting periods") } diff --git a/x/auth/vesting/types/vesting_account_test.go b/x/auth/vesting/types/vesting_account_test.go index 3d37a6c56d4b..b5220831af35 100644 --- a/x/auth/vesting/types/vesting_account_test.go +++ b/x/auth/vesting/types/vesting_account_test.go @@ -249,7 +249,7 @@ func TestSpendableCoinsDelVestingAcc(t *testing.T) { // schedule dva := types.NewDelayedVestingAccount(bacc, origCoins, endTime.Unix()) lockedCoins := dva.LockedCoins(now) - require.True(t, lockedCoins.IsEqual(origCoins)) + require.True(t, lockedCoins.Equal(origCoins)) // require that all coins are spendable after the maturation of the vesting // schedule @@ -258,14 +258,14 @@ func TestSpendableCoinsDelVestingAcc(t *testing.T) { // require that all coins are still vesting after some time lockedCoins = dva.LockedCoins(now.Add(12 * time.Hour)) - require.True(t, lockedCoins.IsEqual(origCoins)) + require.True(t, lockedCoins.Equal(origCoins)) // delegate some locked coins // require that locked is reduced delegatedAmount := sdk.NewCoins(sdk.NewInt64Coin(stakeDenom, 50)) dva.TrackDelegation(now.Add(12*time.Hour), origCoins, delegatedAmount) lockedCoins = dva.LockedCoins(now.Add(12 * time.Hour)) - require.True(t, lockedCoins.IsEqual(origCoins.Sub(delegatedAmount...))) + require.True(t, lockedCoins.Equal(origCoins.Sub(delegatedAmount...))) } func TestTrackDelegationDelVestingAcc(t *testing.T) { @@ -613,18 +613,18 @@ func TestSpendableCoinsPermLockedVestingAcc(t *testing.T) { // schedule plva := types.NewPermanentLockedAccount(bacc, origCoins) lockedCoins := plva.LockedCoins(now) - require.True(t, lockedCoins.IsEqual(origCoins)) + require.True(t, lockedCoins.Equal(origCoins)) // require that all coins are still locked at end time lockedCoins = plva.LockedCoins(endTime) - require.True(t, lockedCoins.IsEqual(origCoins)) + require.True(t, lockedCoins.Equal(origCoins)) // delegate some locked coins // require that locked is reduced delegatedAmount := sdk.NewCoins(sdk.NewInt64Coin(stakeDenom, 50)) plva.TrackDelegation(now.Add(12*time.Hour), origCoins, delegatedAmount) lockedCoins = plva.LockedCoins(now.Add(12 * time.Hour)) - require.True(t, lockedCoins.IsEqual(origCoins.Sub(delegatedAmount...))) + require.True(t, lockedCoins.Equal(origCoins.Sub(delegatedAmount...))) } func TestTrackDelegationPermLockedVestingAcc(t *testing.T) { diff --git a/x/bank/app_test.go b/x/bank/app_test.go index ac177fd9e7b0..b4f44c9cd2d9 100644 --- a/x/bank/app_test.go +++ b/x/bank/app_test.go @@ -129,7 +129,7 @@ func createTestSuite(t *testing.T, genesisAccounts []authtypes.GenesisAccount) s // CheckBalance checks the balance of an account. func checkBalance(t *testing.T, baseApp *baseapp.BaseApp, addr sdk.AccAddress, balances sdk.Coins, keeper bankkeeper.Keeper) { ctxCheck := baseApp.NewContext(true, tmproto.Header{}) - require.True(t, balances.IsEqual(keeper.GetAllBalances(ctxCheck, addr))) + require.True(t, balances.Equal(keeper.GetAllBalances(ctxCheck, addr))) } func TestSendNotEnoughBalance(t *testing.T) { diff --git a/x/bank/keeper/genesis.go b/x/bank/keeper/genesis.go index b934430c4d16..e214570e5035 100644 --- a/x/bank/keeper/genesis.go +++ b/x/bank/keeper/genesis.go @@ -31,7 +31,7 @@ func (k BaseKeeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) { totalSupply = totalSupply.Add(balance.Coins...) } - if !genState.Supply.Empty() && !genState.Supply.IsEqual(totalSupply) { + if !genState.Supply.Empty() && !genState.Supply.Equal(totalSupply) { panic(fmt.Errorf("genesis supply is incorrect, expected %v, got %v", genState.Supply, totalSupply)) } diff --git a/x/bank/keeper/invariants.go b/x/bank/keeper/invariants.go index dead9dca0334..5db22af69f81 100644 --- a/x/bank/keeper/invariants.go +++ b/x/bank/keeper/invariants.go @@ -62,7 +62,7 @@ func TotalSupply(k Keeper) sdk.Invariant { return false }) - broken := !expectedTotal.IsEqual(supply) + broken := !expectedTotal.Equal(supply) return sdk.FormatInvariant(types.ModuleName, "total supply", fmt.Sprintf( diff --git a/x/bank/types/genesis.go b/x/bank/types/genesis.go index 54d290f5ed49..861374e093ea 100644 --- a/x/bank/types/genesis.go +++ b/x/bank/types/genesis.go @@ -69,7 +69,7 @@ func (gs GenesisState) Validate() error { return err } - if !gs.Supply.IsEqual(totalSupply) { + if !gs.Supply.Equal(totalSupply) { return fmt.Errorf("genesis supply is incorrect, expected %v, got %v", gs.Supply, totalSupply) } } diff --git a/x/bank/types/msgs.go b/x/bank/types/msgs.go index 81be11f5ac67..5f0ad3197f29 100644 --- a/x/bank/types/msgs.go +++ b/x/bank/types/msgs.go @@ -186,7 +186,7 @@ func ValidateInputsOutputs(inputs []Input, outputs []Output) error { } // make sure inputs and outputs match - if !totalIn.IsEqual(totalOut) { + if !totalIn.Equal(totalOut) { return ErrInputOutputMismatch } diff --git a/x/distribution/keeper/delegation_test.go b/x/distribution/keeper/delegation_test.go index 3d4b09dfd13a..1d23c107e659 100644 --- a/x/distribution/keeper/delegation_test.go +++ b/x/distribution/keeper/delegation_test.go @@ -961,7 +961,7 @@ func Test100PercentCommissionReward(t *testing.T) { require.NoError(t, err) zeroRewards := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, math.ZeroInt())} - require.True(t, rewards.IsEqual(zeroRewards)) + require.True(t, rewards.Equal(zeroRewards)) events := ctx.EventManager().Events() lastEvent := events[len(events)-1] diff --git a/x/distribution/keeper/genesis.go b/x/distribution/keeper/genesis.go index ef80f1fd14db..497a732753f6 100644 --- a/x/distribution/keeper/genesis.go +++ b/x/distribution/keeper/genesis.go @@ -93,7 +93,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data types.GenesisState) { if balances.IsZero() { k.authKeeper.SetModuleAccount(ctx, moduleAcc) } - if !balances.IsEqual(moduleHoldingsInt) { + if !balances.Equal(moduleHoldingsInt) { panic(fmt.Sprintf("distribution module balance does not match the module holdings: %s <-> %s", balances, moduleHoldingsInt)) } } diff --git a/x/distribution/keeper/invariants.go b/x/distribution/keeper/invariants.go index 50300b977379..8519a4e408bd 100644 --- a/x/distribution/keeper/invariants.go +++ b/x/distribution/keeper/invariants.go @@ -147,7 +147,7 @@ func ModuleAccountInvariant(k Keeper) sdk.Invariant { macc := k.GetDistributionAccount(ctx) balances := k.bankKeeper.GetAllBalances(ctx, macc.GetAddress()) - broken := !balances.IsEqual(expectedInt) + broken := !balances.Equal(expectedInt) return sdk.FormatInvariant( types.ModuleName, "ModuleAccount coins", fmt.Sprintf("\texpected ModuleAccount coins: %s\n"+ diff --git a/x/gov/abci_test.go b/x/gov/abci_test.go index f58b86daccb5..00f6d34b4673 100644 --- a/x/gov/abci_test.go +++ b/x/gov/abci_test.go @@ -323,7 +323,7 @@ func TestProposalPassedEndblocker(t *testing.T) { moduleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) deposits := initialModuleAccCoins.Add(proposal.TotalDeposit...).Add(proposalCoins...) - require.True(t, moduleAccCoins.IsEqual(deposits)) + require.True(t, moduleAccCoins.Equal(deposits)) err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "") require.NoError(t, err) @@ -336,7 +336,7 @@ func TestProposalPassedEndblocker(t *testing.T) { macc = suite.GovKeeper.GetGovernanceAccount(ctx) require.NotNil(t, macc) - require.True(t, suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()).IsEqual(initialModuleAccCoins)) + require.True(t, suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()).Equal(initialModuleAccCoins)) } func TestEndBlockerProposalHandlerFailed(t *testing.T) { diff --git a/x/gov/genesis.go b/x/gov/genesis.go index ebae2d8b12bf..2e97bd1583e0 100644 --- a/x/gov/genesis.go +++ b/x/gov/genesis.go @@ -47,7 +47,7 @@ func InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, k } // check if total deposits equals balance, if it doesn't panic because there were export/import errors - if !balance.IsEqual(totalDeposits) { + if !balance.Equal(totalDeposits) { panic(fmt.Sprintf("expected module account was %s but we got %s", balance.String(), totalDeposits.String())) } } diff --git a/x/gov/keeper/deposit_test.go b/x/gov/keeper/deposit_test.go index 747ed74f78a2..e9e19f7cc45e 100644 --- a/x/gov/keeper/deposit_test.go +++ b/x/gov/keeper/deposit_test.go @@ -35,7 +35,7 @@ func TestDeposits(t *testing.T) { addr0Initial := bankKeeper.GetAllBalances(ctx, TestAddrs[0]) addr1Initial := bankKeeper.GetAllBalances(ctx, TestAddrs[1]) - require.True(t, sdk.NewCoins(proposal.TotalDeposit...).IsEqual(sdk.NewCoins())) + require.True(t, sdk.NewCoins(proposal.TotalDeposit...).Equal(sdk.NewCoins())) // Check no deposits at beginning _, found := govKeeper.GetDeposit(ctx, proposalID, TestAddrs[1]) diff --git a/x/gov/types/v1beta1/params.go b/x/gov/types/v1beta1/params.go index 3a8ec8eb782b..b7eb24349095 100644 --- a/x/gov/types/v1beta1/params.go +++ b/x/gov/types/v1beta1/params.go @@ -37,7 +37,7 @@ func DefaultDepositParams() DepositParams { // Equal checks equality of DepositParams func (dp DepositParams) Equal(dp2 DepositParams) bool { - return dp.MinDeposit.IsEqual(dp2.MinDeposit) && dp.MaxDepositPeriod == dp2.MaxDepositPeriod + return dp.MinDeposit.Equal(dp2.MinDeposit) && dp.MaxDepositPeriod == dp2.MaxDepositPeriod } // NewTallyParams creates a new TallyParams object diff --git a/x/slashing/app_test.go b/x/slashing/app_test.go index ce3f5df9ee9a..1a2c30f7bc57 100644 --- a/x/slashing/app_test.go +++ b/x/slashing/app_test.go @@ -64,7 +64,7 @@ func TestSlashingMsgs(t *testing.T) { baseApp := app.BaseApp ctxCheck := baseApp.NewContext(true, tmproto.Header{}) - require.True(t, sdk.Coins{genCoin}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr1))) + require.True(t, sdk.Coins{genCoin}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr1))) require.NoError(t, err) @@ -80,7 +80,7 @@ func TestSlashingMsgs(t *testing.T) { txConfig := moduletestutil.MakeTestEncodingConfig().TxConfig _, _, err = sims.SignCheckDeliver(t, txConfig, app.BaseApp, header, []sdk.Msg{createValidatorMsg}, "", []uint64{0}, []uint64{0}, true, true, priv1) require.NoError(t, err) - require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr1))) + require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr1))) header = tmproto.Header{Height: app.LastBlockHeight() + 1} app.BeginBlock(abci.RequestBeginBlock{Header: header}) diff --git a/x/staking/app_test.go b/x/staking/app_test.go index 639565d8bb22..a518deeccc59 100644 --- a/x/staking/app_test.go +++ b/x/staking/app_test.go @@ -57,8 +57,8 @@ func TestStakingMsgs(t *testing.T) { require.NoError(t, err) ctxCheck := app.BaseApp.NewContext(true, tmproto.Header{}) - require.True(t, sdk.Coins{genCoin}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr1))) - require.True(t, sdk.Coins{genCoin}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr2))) + require.True(t, sdk.Coins{genCoin}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr1))) + require.True(t, sdk.Coins{genCoin}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr2))) // create validator description := types.NewDescription("foo_moniker", "", "", "", "") @@ -71,7 +71,7 @@ func TestStakingMsgs(t *testing.T) { txConfig := moduletestutil.MakeTestEncodingConfig().TxConfig _, _, err = simtestutil.SignCheckDeliver(t, txConfig, app.BaseApp, header, []sdk.Msg{createValidatorMsg}, "", []uint64{0}, []uint64{0}, true, true, priv1) require.NoError(t, err) - require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr1))) + require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr1))) header = tmproto.Header{Height: app.LastBlockHeight() + 1} app.BeginBlock(abci.RequestBeginBlock{Header: header}) @@ -99,7 +99,7 @@ func TestStakingMsgs(t *testing.T) { require.Equal(t, description, validator.Description) // delegate - require.True(t, sdk.Coins{genCoin}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr2))) + require.True(t, sdk.Coins{genCoin}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr2))) delegateMsg := types.NewMsgDelegate(addr2, sdk.ValAddress(addr1), bondCoin) header = tmproto.Header{Height: app.LastBlockHeight() + 1} @@ -107,7 +107,7 @@ func TestStakingMsgs(t *testing.T) { require.NoError(t, err) ctxCheck = app.BaseApp.NewContext(true, tmproto.Header{}) - require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr2))) + require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr2))) _, found = stakingKeeper.GetDelegation(ctxCheck, addr2, sdk.ValAddress(addr1)) require.True(t, found) @@ -123,5 +123,5 @@ func TestStakingMsgs(t *testing.T) { require.False(t, found) // balance should be the same because bonding not yet complete - require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.IsEqual(bankKeeper.GetAllBalances(ctxCheck, addr2))) + require.True(t, sdk.Coins{genCoin.Sub(bondCoin)}.Equal(bankKeeper.GetAllBalances(ctxCheck, addr2))) } diff --git a/x/staking/keeper/genesis.go b/x/staking/keeper/genesis.go index 1aeea695e2e6..5b1e27d082d4 100644 --- a/x/staking/keeper/genesis.go +++ b/x/staking/keeper/genesis.go @@ -117,7 +117,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data *types.GenesisState) (res []ab } // if balance is different from bonded coins panic because genesis is most likely malformed - if !bondedBalance.IsEqual(bondedCoins) { + if !bondedBalance.Equal(bondedCoins) { panic(fmt.Sprintf("bonded pool balance is different from bonded coins: %s <-> %s", bondedBalance, bondedCoins)) } @@ -133,7 +133,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data *types.GenesisState) (res []ab // If balance is different from non bonded coins panic because genesis is most // likely malformed. - if !notBondedBalance.IsEqual(notBondedCoins) { + if !notBondedBalance.Equal(notBondedCoins) { panic(fmt.Sprintf("not bonded pool balance is different from not bonded coins: %s <-> %s", notBondedBalance, notBondedCoins)) } From a685604a4c8f043b0adca086d25a7f7c0e7641fd Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Mon, 23 Jan 2023 20:45:54 +0200 Subject: [PATCH 3/5] hygiene: rename method Dec.IsEqual to Dec.Equal Refs: #3246 --- x/group/internal/math/dec.go | 2 +- x/group/internal/math/dec_test.go | 32 +++++++++++++++---------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/x/group/internal/math/dec.go b/x/group/internal/math/dec.go index b9e6e55d95f8..b720624037ff 100644 --- a/x/group/internal/math/dec.go +++ b/x/group/internal/math/dec.go @@ -95,7 +95,7 @@ func (x Dec) Cmp(y Dec) int { return x.dec.Cmp(&y.dec) } -func (x Dec) IsEqual(y Dec) bool { +func (x Dec) Equal(y Dec) bool { return x.dec.Cmp(&y.dec) == 0 } diff --git a/x/group/internal/math/dec_test.go b/x/group/internal/math/dec_test.go index bc29404217ca..27757b01e992 100644 --- a/x/group/internal/math/dec_test.go +++ b/x/group/internal/math/dec_test.go @@ -65,30 +65,30 @@ func TestDec(t *testing.T) { res, err := two.Add(zero) require.NoError(t, err) - require.True(t, res.IsEqual(two)) + require.True(t, res.Equal(two)) res, err = five.Sub(two) require.NoError(t, err) - require.True(t, res.IsEqual(three)) + require.True(t, res.Equal(three)) res, err = onePointOneFive.Add(twoPointThreeFour) require.NoError(t, err) - require.True(t, res.IsEqual(threePointFourNine)) + require.True(t, res.Equal(threePointFourNine)) res, err = threePointFourNine.Sub(two) require.NoError(t, err) - require.True(t, res.IsEqual(onePointFourNine)) + require.True(t, res.Equal(onePointFourNine)) res, err = minusOne.Sub(four) require.NoError(t, err) - require.True(t, res.IsEqual(minusFivePointZero)) + require.True(t, res.Equal(minusFivePointZero)) _, err = four.Quo(zero) require.Error(t, err) res, err = four.Quo(two) require.NoError(t, err) - require.True(t, res.IsEqual(two)) + require.True(t, res.Equal(two)) require.False(t, zero.IsNegative()) require.False(t, one.IsNegative()) @@ -133,7 +133,7 @@ func testAddLeftIdentity(t *rapid.T) { b, err := zero.Add(a) require.NoError(t, err) - require.True(t, a.IsEqual(b)) + require.True(t, a.Equal(b)) } // Property: a + 0 == a @@ -144,7 +144,7 @@ func testAddRightIdentity(t *rapid.T) { b, err := a.Add(zero) require.NoError(t, err) - require.True(t, a.IsEqual(b)) + require.True(t, a.Equal(b)) } // Property: a + b == b + a @@ -158,7 +158,7 @@ func testAddCommutative(t *rapid.T) { d, err := b.Add(a) require.NoError(t, err) - require.True(t, c.IsEqual(d)) + require.True(t, c.Equal(d)) } // Property: (a + b) + c == a + (b + c) @@ -181,7 +181,7 @@ func testAddAssociative(t *rapid.T) { g, err := a.Add(f) require.NoError(t, err) - require.True(t, e.IsEqual(g)) + require.True(t, e.Equal(g)) } // Property: a - 0 == a @@ -192,7 +192,7 @@ func testSubRightIdentity(t *rapid.T) { b, err := a.Sub(zero) require.NoError(t, err) - require.True(t, a.IsEqual(b)) + require.True(t, a.Equal(b)) } // Property: a - a == 0 @@ -203,7 +203,7 @@ func testSubZero(t *rapid.T) { b, err := a.Sub(a) require.NoError(t, err) - require.True(t, b.IsEqual(zero)) + require.True(t, b.Equal(zero)) } // Property: (a - b) + b == a @@ -217,7 +217,7 @@ func testSubAdd(t *rapid.T) { d, err := c.Add(b) require.NoError(t, err) - require.True(t, a.IsEqual(d)) + require.True(t, a.Equal(d)) } // Property: (a + b) - b == a @@ -231,7 +231,7 @@ func testAddSub(t *rapid.T) { d, err := c.Sub(b) require.NoError(t, err) - require.True(t, a.IsEqual(d)) + require.True(t, a.Equal(d)) } // Property: Cmp(a, b) == -Cmp(b, a) @@ -242,12 +242,12 @@ func testCmpInverse(t *rapid.T) { require.Equal(t, a.Cmp(b), -b.Cmp(a)) } -// Property: IsEqual(a, b) == IsEqual(b, a) +// Property: Equal(a, b) == Equal(b, a) func testEqualCommutative(t *rapid.T) { a := genDec.Draw(t, "a") b := genDec.Draw(t, "b") - require.Equal(t, a.IsEqual(b), b.IsEqual(a)) + require.Equal(t, a.Equal(b), b.Equal(a)) } // Property: isNegative(f) == isNegative(NewDecFromString(f.String())) From e693abb378e3f7a6c8eb4249811bdcf2a9a2e74b Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Tue, 24 Jan 2023 00:03:50 +0200 Subject: [PATCH 4/5] fix: Replace the method DecCoin.IsEqual with DecCoin.Equal Refs: #3246 --- CHANGELOG.md | 1 + types/coin.go | 7 ++--- types/coin_test.go | 16 ++++------- types/dec_coin.go | 11 +++----- types/dec_coin_test.go | 44 +++++++++++------------------ x/distribution/keeper/delegation.go | 2 +- 6 files changed, 30 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03a274f41ec1..b06df24e52de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -243,6 +243,7 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf ### Bug Fixes +* (x/auth x/bank x/distribution x/gov x/slashing x/staking x/group) [#14739](https://github.com/cosmos/cosmos-sdk/pull/14739) Replace the method Coin.IsEqual with Coin.Equal. The main difference between the two methods is that the first one rise a panic when denoms are not equal. This panic lead to unexpected behavior * (x/bank) [#14538](https://github.com/cosmos/cosmos-sdk/pull/14538) Validate denom in bank balances GRPC queries. * (baseapp) [#14505](https://github.com/cosmos/cosmos-sdk/pull/14505) PrepareProposal and ProcessProposal now use deliverState for the first block in order to access changes made in InitChain. * (server) [#14441](https://github.com/cosmos/cosmos-sdk/pull/14441) Fix `--log_format` flag not working. diff --git a/types/coin.go b/types/coin.go index 662bbe9735d7..ca7d7d8c3ec6 100644 --- a/types/coin.go +++ b/types/coin.go @@ -92,12 +92,9 @@ func (coin Coin) IsLTE(other Coin) bool { } // IsEqual returns true if the two sets of Coins have the same value +// Deprecated: Use Coin.Equal instead. func (coin Coin) IsEqual(other Coin) bool { - if coin.Denom != other.Denom { - panic(fmt.Sprintf("invalid coin denominations; %s, %s", coin.Denom, other.Denom)) - } - - return coin.Amount.Equal(other.Amount) + return coin.Equal(other) } // Add adds amounts of two coins with same denom. If the coins differ in denom then diff --git a/types/coin_test.go b/types/coin_test.go index 2cebe11f2c1c..0427dcdf4f50 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -59,21 +59,15 @@ func (s *coinTestSuite) TestIsEqualCoin() { inputOne sdk.Coin inputTwo sdk.Coin expected bool - panics bool }{ - {sdk.NewInt64Coin(testDenom1, 1), sdk.NewInt64Coin(testDenom1, 1), true, false}, - {sdk.NewInt64Coin(testDenom1, 1), sdk.NewInt64Coin(testDenom2, 1), false, true}, - {sdk.NewInt64Coin("stake", 1), sdk.NewInt64Coin("stake", 10), false, false}, + {sdk.NewInt64Coin(testDenom1, 1), sdk.NewInt64Coin(testDenom1, 1), true}, + {sdk.NewInt64Coin(testDenom1, 1), sdk.NewInt64Coin(testDenom2, 1), false}, + {sdk.NewInt64Coin("stake", 1), sdk.NewInt64Coin("stake", 10), false}, } for tcIndex, tc := range cases { - tc := tc - if tc.panics { - s.Require().Panics(func() { tc.inputOne.IsEqual(tc.inputTwo) }) - } else { - res := tc.inputOne.IsEqual(tc.inputTwo) - s.Require().Equal(tc.expected, res, "coin equality relation is incorrect, tc #%d", tcIndex) - } + res := tc.inputOne.IsEqual(tc.inputTwo) + s.Require().Equal(tc.expected, res, "coin equality relation is incorrect, tc #%d", tcIndex) } } diff --git a/types/dec_coin.go b/types/dec_coin.go index 42ff885d58a6..ad35aa31f853 100644 --- a/types/dec_coin.go +++ b/types/dec_coin.go @@ -79,12 +79,9 @@ func (coin DecCoin) IsLT(other DecCoin) bool { } // IsEqual returns true if the two sets of Coins have the same value. +// Deprecated: Use DecCoin.Equal instead. func (coin DecCoin) IsEqual(other DecCoin) bool { - if coin.Denom != other.Denom { - panic(fmt.Sprintf("invalid coin denominations; %s, %s", coin.Denom, other.Denom)) - } - - return coin.Amount.Equal(other.Amount) + return coin.Equal(other) } // Add adds amounts of two decimal coins with same denom. @@ -483,8 +480,8 @@ func (coins DecCoins) AmountOf(denom string) Dec { } } -// IsEqual returns true if the two sets of DecCoins have the same value. -func (coins DecCoins) IsEqual(coinsB DecCoins) bool { +// Equal returns true if the two sets of DecCoins have the same value. +func (coins DecCoins) Equal(coinsB DecCoins) bool { if len(coins) != len(coinsB) { return false } diff --git a/types/dec_coin_test.go b/types/dec_coin_test.go index 6340c45c4f13..e072b2aadb90 100644 --- a/types/dec_coin_test.go +++ b/types/dec_coin_test.go @@ -468,7 +468,7 @@ func (s *decCoinTestSuite) TestDecCoinsIntersect() { s.Require().NoError(err, "unexpected parse error in %v", i) exr, err := sdk.ParseDecCoins(tc.expectedResult) s.Require().NoError(err, "unexpected parse error in %v", i) - s.Require().True(in1.Intersect(in2).IsEqual(exr), "in1.cap(in2) != exr in %v", i) + s.Require().True(in1.Intersect(in2).Equal(exr), "in1.cap(in2) != exr in %v", i) } } @@ -1006,48 +1006,43 @@ func (s *decCoinTestSuite) TestDecCoin_IsEqual() { coin sdk.DecCoin otherCoin sdk.DecCoin expectedResult bool - expectedPanic bool }{ { "Different Denom Same Amount", sdk.DecCoin{testDenom1, math.LegacyNewDec(20)}, sdk.DecCoin{testDenom2, math.LegacyNewDec(20)}, - false, true, + false, }, { "Different Denom Different Amount", sdk.DecCoin{testDenom1, math.LegacyNewDec(20)}, sdk.DecCoin{testDenom2, math.LegacyNewDec(10)}, - false, true, + false, }, { "Same Denom Different Amount", sdk.DecCoin{testDenom1, math.LegacyNewDec(20)}, sdk.DecCoin{testDenom1, math.LegacyNewDec(10)}, - false, false, + false, }, { "Same Denom Same Amount", sdk.DecCoin{testDenom1, math.LegacyNewDec(20)}, sdk.DecCoin{testDenom1, math.LegacyNewDec(20)}, - true, false, + true, }, } for i, tc := range testCases { s.T().Run(tc.name, func(t *testing.T) { - if tc.expectedPanic { - s.Require().Panics(func() { tc.coin.IsEqual(tc.otherCoin) }, "Test case #%d: %s", i, tc.name) + res := tc.coin.IsEqual(tc.otherCoin) + if tc.expectedResult { + s.Require().True(res, "Test case #%d: %s", i, tc.name) } else { - res := tc.coin.IsEqual(tc.otherCoin) - if tc.expectedResult { - s.Require().True(res, "Test case #%d: %s", i, tc.name) - } else { - s.Require().False(res, "Test case #%d: %s", i, tc.name) - } + s.Require().False(res, "Test case #%d: %s", i, tc.name) } }) } @@ -1059,14 +1054,13 @@ func (s *decCoinTestSuite) TestDecCoins_IsEqual() { coinsA sdk.DecCoins coinsB sdk.DecCoins expectedResult bool - expectedPanic bool }{ {"Different length sets", sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(3)}, sdk.DecCoin{testDenom1, math.LegacyNewDec(4)}, }, sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(35)}, - }, false, false}, + }, false}, {"Same length - different denoms", sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(3)}, @@ -1074,7 +1068,7 @@ func (s *decCoinTestSuite) TestDecCoins_IsEqual() { }, sdk.DecCoins{ sdk.DecCoin{testDenom2, math.LegacyNewDec(3)}, sdk.DecCoin{testDenom2, math.LegacyNewDec(4)}, - }, false, true}, + }, false}, {"Same length - different amounts", sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(3)}, @@ -1082,7 +1076,7 @@ func (s *decCoinTestSuite) TestDecCoins_IsEqual() { }, sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(41)}, sdk.DecCoin{testDenom1, math.LegacyNewDec(343)}, - }, false, false}, + }, false}, {"Same length - same amounts", sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(33)}, @@ -1090,20 +1084,16 @@ func (s *decCoinTestSuite) TestDecCoins_IsEqual() { }, sdk.DecCoins{ sdk.DecCoin{testDenom1, math.LegacyNewDec(33)}, sdk.DecCoin{testDenom1, math.LegacyNewDec(344)}, - }, true, false}, + }, true}, } for i, tc := range testCases { s.T().Run(tc.name, func(t *testing.T) { - if tc.expectedPanic { - s.Require().Panics(func() { tc.coinsA.IsEqual(tc.coinsB) }, "Test case #%d: %s", i, tc.name) + res := tc.coinsA.Equal(tc.coinsB) + if tc.expectedResult { + s.Require().True(res, "Test case #%d: %s", i, tc.name) } else { - res := tc.coinsA.IsEqual(tc.coinsB) - if tc.expectedResult { - s.Require().True(res, "Test case #%d: %s", i, tc.name) - } else { - s.Require().False(res, "Test case #%d: %s", i, tc.name) - } + s.Require().False(res, "Test case #%d: %s", i, tc.name) } }) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 47617b245e6f..7e3579bf4c1b 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -152,7 +152,7 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val stakingtypes.Vali // defensive edge case may happen on the very final digits // of the decCoins due to operation order of the distribution mechanism. rewards := rewardsRaw.Intersect(outstanding) - if !rewards.IsEqual(rewardsRaw) { + if !rewards.Equal(rewardsRaw) { logger := k.Logger(ctx) logger.Info( "rounding error withdrawing rewards from validator", From 88c88b530fdf3bb0e5be73bd3e542118249d01a1 Mon Sep 17 00:00:00 2001 From: Marko Date: Tue, 24 Jan 2023 10:30:15 +0100 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b06df24e52de..ba9086e6c3d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -243,7 +243,7 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf ### Bug Fixes -* (x/auth x/bank x/distribution x/gov x/slashing x/staking x/group) [#14739](https://github.com/cosmos/cosmos-sdk/pull/14739) Replace the method Coin.IsEqual with Coin.Equal. The main difference between the two methods is that the first one rise a panic when denoms are not equal. This panic lead to unexpected behavior +* (types/coin) [#14739](https://github.com/cosmos/cosmos-sdk/pull/14739) Deprecate the method `Coin.IsEqual` in favour of `Coin.Equal`. The difference between the two methods is that the first one results in a panic when denoms are not equal. This panic lead to unexpected behaviour * (x/bank) [#14538](https://github.com/cosmos/cosmos-sdk/pull/14538) Validate denom in bank balances GRPC queries. * (baseapp) [#14505](https://github.com/cosmos/cosmos-sdk/pull/14505) PrepareProposal and ProcessProposal now use deliverState for the first block in order to access changes made in InitChain. * (server) [#14441](https://github.com/cosmos/cosmos-sdk/pull/14441) Fix `--log_format` flag not working.