From 2eb6cde0e847c0f31a50da764277ed7a482b398b Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Wed, 29 Mar 2023 15:42:33 +0900 Subject: [PATCH] fix: fix broken x/foundation invariant on treasury (#946) * TruncateDecimal first * Refactor CollectFoundationTax * Update tests * Refactor tests * Update CHANGELOG.md * Apply feedbacks on the PR * Revert the changes in CollectFoundationTax's signature * Ensure the behavior does not change --- CHANGELOG.md | 1 + x/foundation/expected_keepers.go | 1 - x/foundation/keeper/internal/abci_test.go | 11 +++-- x/foundation/keeper/internal/keeper_test.go | 40 ++++++++++--------- x/foundation/keeper/internal/treasury.go | 18 +++------ x/foundation/keeper/internal/treasury_test.go | 18 ++++++--- 6 files changed, 48 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e1c20fc62..7dbd58a00a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (swagger) [\#898](https://github.com/line/lbm-sdk/pull/898) fix a bug not added `lbm.tx.v1beta1.Service/GetBlockWithTxs` in swagger * (x/collection) [\#911](https://github.com/line/lbm-sdk/pull/911) Add missing command(TxCmdModify) for CLI * (x/foundation) [\#922](https://github.com/line/lbm-sdk/pull/922) Propagate events in x/foundation through sdk.Results +* (x/foundation) [\#946](https://github.com/line/lbm-sdk/pull/946) Fix broken x/foundation invariant on treasury ### Removed diff --git a/x/foundation/expected_keepers.go b/x/foundation/expected_keepers.go index 6368ff12db..324dd8cbe1 100644 --- a/x/foundation/expected_keepers.go +++ b/x/foundation/expected_keepers.go @@ -17,7 +17,6 @@ type ( BankKeeper interface { GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins - SendCoinsFromModuleToModule(ctx sdk.Context, senderModule string, recipientModule string, amt sdk.Coins) error SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error } diff --git a/x/foundation/keeper/internal/abci_test.go b/x/foundation/keeper/internal/abci_test.go index 43c43db206..274c5e0375 100644 --- a/x/foundation/keeper/internal/abci_test.go +++ b/x/foundation/keeper/internal/abci_test.go @@ -9,8 +9,9 @@ import ( func (s *KeeperTestSuite) TestBeginBlocker() { ctx, _ := s.ctx.CacheContext() + taxRatio := sdk.MustNewDecFromStr("0.123456789") s.impl.SetParams(ctx, foundation.Params{ - FoundationTax: sdk.MustNewDecFromStr("0.5"), + FoundationTax: taxRatio, }) before := s.impl.GetTreasury(ctx) @@ -20,10 +21,14 @@ func (s *KeeperTestSuite) TestBeginBlocker() { // collect internal.BeginBlocker(ctx, s.impl) + tax := sdk.NewDecFromInt(s.balance).MulTruncate(taxRatio).TruncateInt() + // ensure the behavior does not change + s.Require().Equal(sdk.NewInt(121932631), tax) + + expectedAfter := s.balance.Add(tax) after := s.impl.GetTreasury(ctx) s.Require().Equal(1, len(after)) - // s.balance + s.balance * 0.5 - s.Require().Equal(sdk.NewDecFromInt(s.balance.Add(s.balance.Quo(sdk.NewInt(2)))), after[0].Amount) + s.Require().Equal(sdk.NewDecFromInt(expectedAfter), after[0].Amount) } func (s *KeeperTestSuite) TestEndBlocker() { diff --git a/x/foundation/keeper/internal/keeper_test.go b/x/foundation/keeper/internal/keeper_test.go index c340537b44..448f412595 100644 --- a/x/foundation/keeper/internal/keeper_test.go +++ b/x/foundation/keeper/internal/keeper_test.go @@ -23,9 +23,10 @@ type KeeperTestSuite struct { suite.Suite ctx sdk.Context - app *simapp.SimApp - keeper keeper.Keeper - impl internal.Keeper + bankKeeper foundation.BankKeeper + keeper keeper.Keeper + impl internal.Keeper + queryServer foundation.QueryServer msgServer foundation.MsgServer proposalHandler govtypes.Handler @@ -54,18 +55,19 @@ func newMsgCreateDog(name string) sdk.Msg { func (s *KeeperTestSuite) SetupTest() { checkTx := false - s.app = simapp.Setup(checkTx) - testdata.RegisterInterfaces(s.app.InterfaceRegistry()) - testdata.RegisterMsgServer(s.app.MsgServiceRouter(), testdata.MsgServerImpl{}) + app := simapp.Setup(checkTx) + testdata.RegisterInterfaces(app.InterfaceRegistry()) + testdata.RegisterMsgServer(app.MsgServiceRouter(), testdata.MsgServerImpl{}) - s.ctx = s.app.BaseApp.NewContext(checkTx, tmproto.Header{}) - s.keeper = s.app.FoundationKeeper + s.ctx = app.BaseApp.NewContext(checkTx, tmproto.Header{}) + s.bankKeeper = app.BankKeeper + s.keeper = app.FoundationKeeper s.impl = internal.NewKeeper( - s.app.AppCodec(), - s.app.GetKey(foundation.ModuleName), - s.app.MsgServiceRouter(), - s.app.AccountKeeper, - s.app.BankKeeper, + app.AppCodec(), + app.GetKey(foundation.ModuleName), + app.MsgServiceRouter(), + app.AccountKeeper, + app.BankKeeper, authtypes.FeeCollectorName, foundation.DefaultConfig(), foundation.DefaultAuthority().String(), @@ -106,14 +108,14 @@ func (s *KeeperTestSuite) SetupTest() { s.Require().NoError(err) s.impl.SetFoundationInfo(s.ctx, info) - s.balance = sdk.NewInt(1000000) + s.balance = sdk.NewInt(987654321) s.impl.SetPool(s.ctx, foundation.Pool{ Treasury: sdk.NewDecCoinsFromCoins(sdk.NewCoin(sdk.DefaultBondDenom, s.balance)), }) holders := []sdk.AccAddress{ s.stranger, - s.app.AccountKeeper.GetModuleAccount(s.ctx, foundation.TreasuryName).GetAddress(), - s.app.AccountKeeper.GetModuleAccount(s.ctx, authtypes.FeeCollectorName).GetAddress(), + app.AccountKeeper.GetModuleAccount(s.ctx, foundation.TreasuryName).GetAddress(), + app.AccountKeeper.GetModuleAccount(s.ctx, authtypes.FeeCollectorName).GetAddress(), } for _, holder := range holders { amount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, s.balance)) @@ -123,11 +125,11 @@ func (s *KeeperTestSuite) SetupTest() { // because x/bank already has dependency on x/mint, and we must have dependency // on x/bank, it's OK to use x/mint here. minterName := minttypes.ModuleName - err := s.app.BankKeeper.MintCoins(s.ctx, minterName, amount) + err := app.BankKeeper.MintCoins(s.ctx, minterName, amount) s.Require().NoError(err) - minter := s.app.AccountKeeper.GetModuleAccount(s.ctx, minterName).GetAddress() - err = s.app.BankKeeper.SendCoins(s.ctx, minter, holder, amount) + minter := app.AccountKeeper.GetModuleAccount(s.ctx, minterName).GetAddress() + err = app.BankKeeper.SendCoins(s.ctx, minter, holder, amount) s.Require().NoError(err) } diff --git a/x/foundation/keeper/internal/treasury.go b/x/foundation/keeper/internal/treasury.go index 0490308f66..fb2d1457f2 100644 --- a/x/foundation/keeper/internal/treasury.go +++ b/x/foundation/keeper/internal/treasury.go @@ -7,24 +7,16 @@ import ( ) func (k Keeper) CollectFoundationTax(ctx sdk.Context) error { - // fetch and clear the collected fees for the fund, since this is - // called in BeginBlock, collected fees will be from the previous block - feeCollector := k.authKeeper.GetModuleAccount(ctx, k.feeCollectorName) - feesCollectedInt := k.bankKeeper.GetAllBalances(ctx, feeCollector.GetAddress()) + feeCollector := k.authKeeper.GetModuleAccount(ctx, k.feeCollectorName).GetAddress() + feesCollectedInt := k.bankKeeper.GetAllBalances(ctx, feeCollector) feesCollected := sdk.NewDecCoinsFromCoins(feesCollectedInt...) // calculate the tax taxRatio := k.GetFoundationTax(ctx) - tax := feesCollected.MulDecTruncate(taxRatio) + tax, _ := feesCollected.MulDecTruncate(taxRatio).TruncateDecimal() - // update foundation treasury - pool := k.GetPool(ctx) - pool.Treasury = pool.Treasury.Add(tax...) - k.SetPool(ctx, pool) - - // collect tax to the foundation treasury - amount, _ := tax.TruncateDecimal() - if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, foundation.TreasuryName, amount); err != nil { + // collect the tax + if err := k.FundTreasury(ctx, feeCollector, tax); err != nil { return err } diff --git a/x/foundation/keeper/internal/treasury_test.go b/x/foundation/keeper/internal/treasury_test.go index 22e5803a6b..59e729e375 100644 --- a/x/foundation/keeper/internal/treasury_test.go +++ b/x/foundation/keeper/internal/treasury_test.go @@ -2,6 +2,8 @@ package internal_test import ( sdk "github.com/line/lbm-sdk/types" + authtypes "github.com/line/lbm-sdk/x/auth/types" + "github.com/line/lbm-sdk/x/foundation" ) func (s *KeeperTestSuite) TestFundTreasury() { @@ -32,13 +34,16 @@ func (s *KeeperTestSuite) TestFundTreasury() { } s.Require().NoError(err) - after := s.impl.GetTreasury(ctx) - s.Require().Equal(before.Add(sdk.NewDecCoinsFromCoins(amount...)...), after) + expectedAfter := before.Add(sdk.NewDecCoinsFromCoins(amount...)...) + poolAfter := s.impl.GetTreasury(ctx) + s.Require().Equal(sdk.NewDecCoins(expectedAfter...), sdk.NewDecCoins(poolAfter...)) + balanceAfter := sdk.NewDecCoinsFromCoins(s.bankKeeper.GetAllBalances(ctx, authtypes.NewModuleAddress(foundation.TreasuryName))...) + s.Require().Equal(sdk.NewDecCoins(expectedAfter...), sdk.NewDecCoins(balanceAfter...)) }) } } -func (s *KeeperTestSuite) TestWithDrawFromTreasury() { +func (s *KeeperTestSuite) TestWithdrawFromTreasury() { testCases := map[string]struct { amount sdk.Int valid bool @@ -66,8 +71,11 @@ func (s *KeeperTestSuite) TestWithDrawFromTreasury() { } s.Require().NoError(err) - after := s.impl.GetTreasury(ctx) - s.Require().Equal(before.Sub(sdk.NewDecCoinsFromCoins(amount...)), after) + expectedAfter := before.Sub(sdk.NewDecCoinsFromCoins(amount...)) + poolAfter := s.impl.GetTreasury(ctx) + s.Require().Equal(sdk.NewDecCoins(expectedAfter...), sdk.NewDecCoins(poolAfter...)) + balanceAfter := sdk.NewDecCoinsFromCoins(s.bankKeeper.GetAllBalances(ctx, authtypes.NewModuleAddress(foundation.TreasuryName))...) + s.Require().Equal(sdk.NewDecCoins(expectedAfter...), sdk.NewDecCoins(balanceAfter...)) }) } }