From 911f9f59d6be9cf13367be6809b1325a174b1bd3 Mon Sep 17 00:00:00 2001 From: Nick DeLuca Date: Thu, 15 Jul 2021 17:29:11 -0500 Subject: [PATCH] Hook Regression Test + Deposit Refactor (#967) * add regression test for hooks; refactor deposit to commit pool and shares in one place with commit after validation checks; panic on pool and share set methods if the record is invalid * use correct hook ordering; add regression test for order - fails if hooks are not called before or after share record set --- x/swap/keeper/deposit.go | 54 +++------ x/swap/keeper/hooks_test.go | 197 +++++++++++++++++++++++++++++++ x/swap/keeper/invariant_test.go | 19 +++ x/swap/keeper/keeper.go | 54 ++++++++- x/swap/keeper/keeper_test.go | 56 +++++++++ x/swap/keeper/swap_test.go | 4 +- x/swap/keeper/withdraw.go | 20 +--- x/swap/keeper/withdraw_test.go | 2 +- x/swap/types/mocks/swap_hooks.go | 24 ++++ 9 files changed, 368 insertions(+), 62 deletions(-) create mode 100644 x/swap/keeper/hooks_test.go create mode 100644 x/swap/keeper/invariant_test.go create mode 100644 x/swap/types/mocks/swap_hooks.go diff --git a/x/swap/keeper/deposit.go b/x/swap/keeper/deposit.go index 6cca2ddb..b7fe8db6 100644 --- a/x/swap/keeper/deposit.go +++ b/x/swap/keeper/deposit.go @@ -44,14 +44,15 @@ func (k Keeper) Deposit(ctx sdk.Context, depositor sdk.AccAddress, coinA sdk.Coi poolRecord, found := k.GetPool(ctx, poolID) var ( + pool *types.DenominatedPool depositAmount sdk.Coins shares sdk.Int err error ) if found { - depositAmount, shares, err = k.addLiquidityToPool(ctx, poolRecord, depositor, desiredAmount) + pool, depositAmount, shares, err = k.addLiquidityToPool(ctx, poolRecord, depositor, desiredAmount) } else { - depositAmount, shares, err = k.initializePool(ctx, poolID, depositor, desiredAmount) + pool, depositAmount, shares, err = k.initializePool(ctx, poolID, depositor, desiredAmount) } if err != nil { return err @@ -75,6 +76,15 @@ func (k Keeper) Deposit(ctx sdk.Context, depositor sdk.AccAddress, coinA sdk.Coi return sdkerrors.Wrapf(types.ErrSlippageExceeded, "slippage %s > limit %s", slippage, slippageLimit) } + k.updatePool(ctx, poolID, pool) + if shareRecord, hasExistingShares := k.GetDepositorShares(ctx, depositor, poolID); hasExistingShares { + k.BeforePoolDepositModified(ctx, poolID, depositor, shareRecord.SharesOwned) + k.updateShares(ctx, depositor, poolID, shareRecord.SharesOwned.Add(shares)) + } else { + k.updateShares(ctx, depositor, poolID, shares) + k.AfterPoolDepositCreated(ctx, poolID, depositor, shares) + } + err = k.supplyKeeper.SendCoinsFromAccountToModule(ctx, depositor, types.ModuleAccountName, depositAmount) if err != nil { return err @@ -103,52 +113,26 @@ func (k Keeper) depositAllowed(ctx sdk.Context, poolID string) bool { return false } -func (k Keeper) initializePool(ctx sdk.Context, poolID string, depositor sdk.AccAddress, reserves sdk.Coins) (sdk.Coins, sdk.Int, error) { +func (k Keeper) initializePool(ctx sdk.Context, poolID string, depositor sdk.AccAddress, reserves sdk.Coins) (*types.DenominatedPool, sdk.Coins, sdk.Int, error) { if allowed := k.depositAllowed(ctx, poolID); !allowed { - return sdk.Coins{}, sdk.ZeroInt(), sdkerrors.Wrap(types.ErrNotAllowed, fmt.Sprintf("can not create pool '%s'", poolID)) + return nil, sdk.Coins{}, sdk.ZeroInt(), sdkerrors.Wrap(types.ErrNotAllowed, fmt.Sprintf("can not create pool '%s'", poolID)) } pool, err := types.NewDenominatedPool(reserves) if err != nil { - return sdk.Coins{}, sdk.ZeroInt(), err + return nil, sdk.Coins{}, sdk.ZeroInt(), err } - poolRecord := types.NewPoolRecordFromPool(pool) - shareRecord := types.NewShareRecord(depositor, poolRecord.PoolID, pool.TotalShares()) - - k.SetPool(ctx, poolRecord) - k.SetDepositorShares(ctx, shareRecord) - - k.hooks.AfterPoolDepositCreated(ctx, poolRecord.PoolID, depositor, shareRecord.SharesOwned) - - return pool.Reserves(), pool.TotalShares(), nil + return pool, pool.Reserves(), pool.TotalShares(), nil } -func (k Keeper) addLiquidityToPool(ctx sdk.Context, record types.PoolRecord, depositor sdk.AccAddress, desiredAmount sdk.Coins) (sdk.Coins, sdk.Int, error) { +func (k Keeper) addLiquidityToPool(ctx sdk.Context, record types.PoolRecord, depositor sdk.AccAddress, desiredAmount sdk.Coins) (*types.DenominatedPool, sdk.Coins, sdk.Int, error) { pool, err := types.NewDenominatedPoolWithExistingShares(record.Reserves(), record.TotalShares) if err != nil { - return sdk.Coins{}, sdk.ZeroInt(), err + return nil, sdk.Coins{}, sdk.ZeroInt(), err } depositAmount, shares := pool.AddLiquidity(desiredAmount) - poolRecord := types.NewPoolRecordFromPool(pool) - - shareRecord, sharesFound := k.GetDepositorShares(ctx, depositor, poolRecord.PoolID) - if sharesFound { - k.hooks.BeforePoolDepositModified(ctx, poolRecord.PoolID, depositor, shareRecord.SharesOwned) - - shareRecord.SharesOwned = shareRecord.SharesOwned.Add(shares) - } else { - shareRecord = types.NewShareRecord(depositor, poolRecord.PoolID, shares) - } - - k.SetPool(ctx, poolRecord) - k.SetDepositorShares(ctx, shareRecord) - - if !sharesFound { - k.hooks.AfterPoolDepositCreated(ctx, poolRecord.PoolID, depositor, shareRecord.SharesOwned) - } - - return depositAmount, shares, nil + return pool, depositAmount, shares, nil } diff --git a/x/swap/keeper/hooks_test.go b/x/swap/keeper/hooks_test.go new file mode 100644 index 00000000..590a8c3c --- /dev/null +++ b/x/swap/keeper/hooks_test.go @@ -0,0 +1,197 @@ +package keeper_test + +import ( + "github.com/kava-labs/kava/x/swap/types" + "github.com/kava-labs/kava/x/swap/types/mocks" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/mock" +) + +func (suite *keeperTestSuite) TestHooks_DepositAndWithdraw() { + suite.Keeper.ClearHooks() + swapHooks := &mocks.SwapHooks{} + suite.Keeper.SetHooks(swapHooks) + + pool := types.NewAllowedPool("ukava", "usdx") + suite.Require().NoError(pool.Validate()) + suite.Keeper.SetParams(suite.Ctx, types.NewParams(types.NewAllowedPools(pool), types.DefaultSwapFee)) + + balance := sdk.NewCoins( + sdk.NewCoin(pool.TokenA, sdk.NewInt(1000e6)), + sdk.NewCoin(pool.TokenB, sdk.NewInt(1000e6)), + ) + depositor_1 := suite.CreateAccount(balance) + + depositA := sdk.NewCoin(pool.TokenA, sdk.NewInt(10e6)) + depositB := sdk.NewCoin(pool.TokenB, sdk.NewInt(50e6)) + deposit := sdk.NewCoins(depositA, depositB) + + // expected initial shares - geometric mean + expectedShares := sdk.NewInt(22360679) + + // first deposit creates pool - calls AfterPoolDepositCreated with initial shares + swapHooks.On("AfterPoolDepositCreated", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_1.GetAddress(), expectedShares).Once() + err := suite.Keeper.Deposit(suite.Ctx, depositor_1.GetAddress(), depositA, depositB, sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // second deposit adds to pool - calls BeforePoolDepositModified + // shares given are the initial shares, not the shares added to the pool + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_1.GetAddress(), expectedShares).Once() + err = suite.Keeper.Deposit(suite.Ctx, depositor_1.GetAddress(), sdk.NewCoin("ukava", sdk.NewInt(5e6)), sdk.NewCoin("usdx", sdk.NewInt(25e6)), sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // get the shares from the store from the last deposit + shareRecord, found := suite.Keeper.GetDepositorShares(suite.Ctx, depositor_1.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + + // third deposit adds to pool - calls BeforePoolDepositModified + // shares given are the shares added in previous deposit, not the shares added to the pool now + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_1.GetAddress(), shareRecord.SharesOwned).Once() + err = suite.Keeper.Deposit(suite.Ctx, depositor_1.GetAddress(), sdk.NewCoin("ukava", sdk.NewInt(10e6)), sdk.NewCoin("usdx", sdk.NewInt(50e6)), sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + depositor_2 := suite.NewAccountFromAddr( + sdk.AccAddress("depositor 2"), + sdk.NewCoins( + sdk.NewCoin("ukava", sdk.NewInt(100e6)), + sdk.NewCoin("usdx", sdk.NewInt(100e6)), + ), + ) + + // first deposit deposit into pool creates the deposit and calls AfterPoolDepositCreated + expectedShares = sdk.NewInt(2236067) + swapHooks.On("AfterPoolDepositCreated", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_2.GetAddress(), expectedShares).Once() + err = suite.Keeper.Deposit(suite.Ctx, depositor_2.GetAddress(), sdk.NewCoin("ukava", sdk.NewInt(1e6)), sdk.NewCoin("usdx", sdk.NewInt(5e6)), sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // second deposit into pool calls BeforePoolDepositModified with initial shares given + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_2.GetAddress(), expectedShares).Once() + err = suite.Keeper.Deposit(suite.Ctx, depositor_2.GetAddress(), sdk.NewCoin("ukava", sdk.NewInt(2e6)), sdk.NewCoin("usdx", sdk.NewInt(10e6)), sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // get the shares from the store from the last deposit + shareRecord, found = suite.Keeper.GetDepositorShares(suite.Ctx, depositor_2.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + + // third deposit into pool calls BeforePoolDepositModified with shares from last deposit + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_2.GetAddress(), shareRecord.SharesOwned).Once() + err = suite.Keeper.Deposit(suite.Ctx, depositor_2.GetAddress(), sdk.NewCoin("ukava", sdk.NewInt(3e6)), sdk.NewCoin("usdx", sdk.NewInt(15e6)), sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // test hooks with a full withdraw of all shares + shareRecord, found = suite.Keeper.GetDepositorShares(suite.Ctx, depositor_1.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + // all shares given to BeforePoolDepositModified + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_1.GetAddress(), shareRecord.SharesOwned).Once() + err = suite.Keeper.Withdraw(suite.Ctx, depositor_1.GetAddress(), shareRecord.SharesOwned, sdk.NewCoin("ukava", sdk.NewInt(1)), sdk.NewCoin("usdx", sdk.NewInt(1))) + suite.Require().NoError(err) + + // test hooks on partial withdraw + shareRecord, found = suite.Keeper.GetDepositorShares(suite.Ctx, depositor_2.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + partialShares := shareRecord.SharesOwned.Quo(sdk.NewInt(3)) + // all shares given to before deposit modified even with partial withdraw + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_2.GetAddress(), shareRecord.SharesOwned).Once() + err = suite.Keeper.Withdraw(suite.Ctx, depositor_2.GetAddress(), partialShares, sdk.NewCoin("ukava", sdk.NewInt(1)), sdk.NewCoin("usdx", sdk.NewInt(1))) + suite.Require().NoError(err) + + // test hooks on second partial withdraw + shareRecord, found = suite.Keeper.GetDepositorShares(suite.Ctx, depositor_2.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + partialShares = shareRecord.SharesOwned.Quo(sdk.NewInt(2)) + // all shares given to before deposit modified even with partial withdraw + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_2.GetAddress(), shareRecord.SharesOwned).Once() + err = suite.Keeper.Withdraw(suite.Ctx, depositor_2.GetAddress(), partialShares, sdk.NewCoin("ukava", sdk.NewInt(1)), sdk.NewCoin("usdx", sdk.NewInt(1))) + suite.Require().NoError(err) + + // test hooks withdraw all shares with second depositor + shareRecord, found = suite.Keeper.GetDepositorShares(suite.Ctx, depositor_2.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + // all shares given to before deposit modified even with partial withdraw + swapHooks.On("BeforePoolDepositModified", suite.Ctx, types.PoolIDFromCoins(deposit), depositor_2.GetAddress(), shareRecord.SharesOwned).Once() + err = suite.Keeper.Withdraw(suite.Ctx, depositor_2.GetAddress(), shareRecord.SharesOwned, sdk.NewCoin("ukava", sdk.NewInt(1)), sdk.NewCoin("usdx", sdk.NewInt(1))) + suite.Require().NoError(err) + + swapHooks.AssertExpectations(suite.T()) +} + +func (suite *keeperTestSuite) TestHooks_NoPanicsOnNilHooks() { + suite.Keeper.ClearHooks() + + pool := types.NewAllowedPool("ukava", "usdx") + suite.Require().NoError(pool.Validate()) + suite.Keeper.SetParams(suite.Ctx, types.NewParams(types.NewAllowedPools(pool), types.DefaultSwapFee)) + + balance := sdk.NewCoins( + sdk.NewCoin(pool.TokenA, sdk.NewInt(1000e6)), + sdk.NewCoin(pool.TokenB, sdk.NewInt(1000e6)), + ) + depositor := suite.CreateAccount(balance) + + depositA := sdk.NewCoin(pool.TokenA, sdk.NewInt(10e6)) + depositB := sdk.NewCoin(pool.TokenB, sdk.NewInt(50e6)) + deposit := sdk.NewCoins(depositA, depositB) + + // deposit create pool should not panic when hooks are not set + err := suite.Keeper.Deposit(suite.Ctx, depositor.GetAddress(), depositA, depositB, sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // existing deposit should not panic with hooks are not set + err = suite.Keeper.Deposit(suite.Ctx, depositor.GetAddress(), sdk.NewCoin("ukava", sdk.NewInt(5e6)), sdk.NewCoin("usdx", sdk.NewInt(25e6)), sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + // withdraw of shares should not panic when hooks are not set + shareRecord, found := suite.Keeper.GetDepositorShares(suite.Ctx, depositor.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + err = suite.Keeper.Withdraw(suite.Ctx, depositor.GetAddress(), shareRecord.SharesOwned, sdk.NewCoin("ukava", sdk.NewInt(1)), sdk.NewCoin("usdx", sdk.NewInt(1))) + suite.Require().NoError(err) +} + +func (suite *keeperTestSuite) TestHooks_HookOrdering() { + suite.Keeper.ClearHooks() + swapHooks := &mocks.SwapHooks{} + suite.Keeper.SetHooks(swapHooks) + + pool := types.NewAllowedPool("ukava", "usdx") + suite.Require().NoError(pool.Validate()) + suite.Keeper.SetParams(suite.Ctx, types.NewParams(types.NewAllowedPools(pool), types.DefaultSwapFee)) + + balance := sdk.NewCoins( + sdk.NewCoin(pool.TokenA, sdk.NewInt(1000e6)), + sdk.NewCoin(pool.TokenB, sdk.NewInt(1000e6)), + ) + depositor := suite.CreateAccount(balance) + + depositA := sdk.NewCoin(pool.TokenA, sdk.NewInt(10e6)) + depositB := sdk.NewCoin(pool.TokenB, sdk.NewInt(50e6)) + deposit := sdk.NewCoins(depositA, depositB) + + poolID := types.PoolIDFromCoins(deposit) + expectedShares := sdk.NewInt(22360679) + + swapHooks.On("AfterPoolDepositCreated", suite.Ctx, poolID, depositor.GetAddress(), expectedShares).Run(func(args mock.Arguments) { + _, found := suite.Keeper.GetDepositorShares(suite.Ctx, depositor.GetAddress(), poolID) + suite.Require().True(found, "expected after hook to be called after shares are updated") + }) + err := suite.Keeper.Deposit(suite.Ctx, depositor.GetAddress(), depositA, depositB, sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + swapHooks.On("BeforePoolDepositModified", suite.Ctx, poolID, depositor.GetAddress(), expectedShares).Run(func(args mock.Arguments) { + shareRecord, found := suite.Keeper.GetDepositorShares(suite.Ctx, depositor.GetAddress(), poolID) + suite.Require().True(found, "expected share record to exist") + suite.Equal(expectedShares, shareRecord.SharesOwned, "expected hook to be called before shares are updated") + }) + err = suite.Keeper.Deposit(suite.Ctx, depositor.GetAddress(), depositA, depositB, sdk.MustNewDecFromStr("0.0015")) + suite.Require().NoError(err) + + existingShareRecord, found := suite.Keeper.GetDepositorShares(suite.Ctx, depositor.GetAddress(), types.PoolIDFromCoins(deposit)) + suite.Require().True(found) + swapHooks.On("BeforePoolDepositModified", suite.Ctx, poolID, depositor.GetAddress(), existingShareRecord.SharesOwned).Run(func(args mock.Arguments) { + shareRecord, found := suite.Keeper.GetDepositorShares(suite.Ctx, depositor.GetAddress(), poolID) + suite.Require().True(found, "expected share record to exist") + suite.Equal(existingShareRecord.SharesOwned, shareRecord.SharesOwned, "expected hook to be called before shares are updated") + }) + err = suite.Keeper.Withdraw(suite.Ctx, depositor.GetAddress(), existingShareRecord.SharesOwned.Quo(sdk.NewInt(2)), sdk.NewCoin("ukava", sdk.NewInt(1)), sdk.NewCoin("usdx", sdk.NewInt(1))) + suite.Require().NoError(err) +} diff --git a/x/swap/keeper/invariant_test.go b/x/swap/keeper/invariant_test.go new file mode 100644 index 00000000..ab39f78e --- /dev/null +++ b/x/swap/keeper/invariant_test.go @@ -0,0 +1,19 @@ +package keeper_test + +import ( + "testing" + + //"github.com/kava-labs/kava/x/swap" + "github.com/kava-labs/kava/x/swap/testutil" + //"github.com/kava-labs/kava/x/swap/types" + "github.com/stretchr/testify/suite" + //sdk "github.com/cosmos/cosmos-sdk/types" +) + +type invariantTestSuite struct { + testutil.Suite +} + +func TestGenesisTestSuite(t *testing.T) { + suite.Run(t, new(invariantTestSuite)) +} diff --git a/x/swap/keeper/keeper.go b/x/swap/keeper/keeper.go index 1db44f73..8141ffc7 100644 --- a/x/swap/keeper/keeper.go +++ b/x/swap/keeper/keeper.go @@ -1,6 +1,8 @@ package keeper import ( + "fmt" + "github.com/kava-labs/kava/x/swap/types" "github.com/cosmos/cosmos-sdk/codec" @@ -49,6 +51,11 @@ func (k *Keeper) SetHooks(sh types.SwapHooks) *Keeper { return k } +// ClearHooks clears the hooks on the keeper +func (k *Keeper) ClearHooks() { + k.hooks = nil +} + // GetParams returns the params from the store func (k Keeper) GetParams(ctx sdk.Context) types.Params { var p types.Params @@ -81,13 +88,22 @@ func (k Keeper) GetPool(ctx sdk.Context, poolID string) (types.PoolRecord, bool) return record, true } -// SetPool saves a pool record to the store -func (k Keeper) SetPool(ctx sdk.Context, record types.PoolRecord) { +// SetPool_Raw saves a pool record to the store without any validation +func (k Keeper) SetPool_Raw(ctx sdk.Context, record types.PoolRecord) { store := prefix.NewStore(ctx.KVStore(k.key), types.PoolKeyPrefix) bz := k.cdc.MustMarshalBinaryLengthPrefixed(record) store.Set(types.PoolKey(record.PoolID), bz) } +// SetPool saves a pool to the store and panics if the record is invalid +func (k Keeper) SetPool(ctx sdk.Context, record types.PoolRecord) { + if err := record.Validate(); err != nil { + panic(fmt.Sprintf("invalid pool record: %s", err)) + } + + k.SetPool_Raw(ctx, record) +} + // DeletePool deletes a pool record from the store func (k Keeper) DeletePool(ctx sdk.Context, poolID string) { store := prefix.NewStore(ctx.KVStore(k.key), types.PoolKeyPrefix) @@ -138,13 +154,22 @@ func (k Keeper) GetDepositorShares(ctx sdk.Context, depositor sdk.AccAddress, po return record, true } -// SetDepositorShares saves a share record to the store -func (k Keeper) SetDepositorShares(ctx sdk.Context, record types.ShareRecord) { +// SetDepositorShares_Raw saves a share record to the store without validation +func (k Keeper) SetDepositorShares_Raw(ctx sdk.Context, record types.ShareRecord) { store := prefix.NewStore(ctx.KVStore(k.key), types.DepositorPoolSharesPrefix) bz := k.cdc.MustMarshalBinaryLengthPrefixed(record) store.Set(types.DepositorPoolSharesKey(record.Depositor, record.PoolID), bz) } +// SetDepositorShares saves a share record to the store and panics if the record is invalid +func (k Keeper) SetDepositorShares(ctx sdk.Context, record types.ShareRecord) { + if err := record.Validate(); err != nil { + panic(fmt.Sprintf("invalid share record: %s", err)) + } + + k.SetDepositorShares_Raw(ctx, record) +} + // DeleteDepositorShares deletes a share record from the store func (k Keeper) DeleteDepositorShares(ctx sdk.Context, depositor sdk.AccAddress, poolID string) { store := prefix.NewStore(ctx.KVStore(k.key), types.DepositorPoolSharesPrefix) @@ -198,10 +223,29 @@ func (k Keeper) GetAllDepositorSharesByOwner(ctx sdk.Context, owner sdk.AccAddre } // GetDepositorSharesAmount gets a depositor's shares in a pool from the store -func (k *Keeper) GetDepositorSharesAmount(ctx sdk.Context, depositor sdk.AccAddress, poolID string) (sdk.Int, bool) { +func (k Keeper) GetDepositorSharesAmount(ctx sdk.Context, depositor sdk.AccAddress, poolID string) (sdk.Int, bool) { record, found := k.GetDepositorShares(ctx, depositor, poolID) if !found { return sdk.Int{}, false } return record.SharesOwned, true } + +// updatePool updates a pool, deleting the pool record if the shares are zero +func (k Keeper) updatePool(ctx sdk.Context, poolID string, pool *types.DenominatedPool) { + if pool.TotalShares().IsZero() { + k.DeletePool(ctx, poolID) + } else { + k.SetPool(ctx, types.NewPoolRecordFromPool(pool)) + } +} + +// updateShares updates a depositor shares records for a pool, deleting the record if the new shares are zero +func (k Keeper) updateShares(ctx sdk.Context, owner sdk.AccAddress, poolID string, shares sdk.Int) { + if shares.IsZero() { + k.DeleteDepositorShares(ctx, owner, poolID) + } else { + shareRecord := types.NewShareRecord(owner, poolID, shares) + k.SetDepositorShares(ctx, shareRecord) + } +} diff --git a/x/swap/keeper/keeper_test.go b/x/swap/keeper/keeper_test.go index 5b55a720..93fad574 100644 --- a/x/swap/keeper/keeper_test.go +++ b/x/swap/keeper/keeper_test.go @@ -5,6 +5,7 @@ import ( "github.com/kava-labs/kava/x/swap/testutil" "github.com/kava-labs/kava/x/swap/types" + "github.com/kava-labs/kava/x/swap/types/mocks" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/suite" @@ -106,6 +107,17 @@ func (suite *keeperTestSuite) TestPool_Persistance() { suite.Equal(deletedPool, types.PoolRecord{}) } +func (suite *keeperTestSuite) TestPool_PanicsWhenInvalid() { + invalidRecord := types.NewPoolRecord( + sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100e6)), sdk.NewCoin("usdx", sdk.NewInt(100e6))), + i(-1), + ) + + suite.Panics(func() { + suite.Keeper.SetPool(suite.Ctx, invalidRecord) + }, "expected set pool to panic with invalid record") +} + func (suite *keeperTestSuite) TestShare_Persistance() { poolID := "ukava/usdx" depositor := sdk.AccAddress("testAddress1") @@ -127,3 +139,47 @@ func (suite *keeperTestSuite) TestShare_Persistance() { suite.False(ok) suite.Equal(deletedShares, types.ShareRecord{}) } + +func (suite *keeperTestSuite) TestShare_PanicsWhenInvalid() { + depositor, err := sdk.AccAddressFromBech32("kava1mq9qxlhze029lm0frzw2xr6hem8c3k9ts54w0w") + suite.Require().NoError(err) + + invalidRecord := types.NewShareRecord( + depositor, + "hard/usdx", + i(-1), + ) + + suite.Panics(func() { + suite.Keeper.SetDepositorShares(suite.Ctx, invalidRecord) + }, "expected set depositor shares to panic with invalid record") +} + +func (suite *keeperTestSuite) TestHooks() { + // ensure no hooks are set + suite.Keeper.ClearHooks() + + // data + depositor, err := sdk.AccAddressFromBech32("kava1mq9qxlhze029lm0frzw2xr6hem8c3k9ts54w0w") + suite.Require().NoError(err) + + // hooks can be called when not set + suite.Keeper.AfterPoolDepositCreated(suite.Ctx, "ukava/usdx", depositor, sdk.NewInt(1e6)) + suite.Keeper.BeforePoolDepositModified(suite.Ctx, "ukava/usdx", depositor, sdk.NewInt(1e6)) + + // set hooks + swapHooks := &mocks.SwapHooks{} + suite.Keeper.SetHooks(swapHooks) + + // test hook calls are correct + swapHooks.On("AfterPoolDepositCreated", suite.Ctx, "ukava/usdx", depositor, sdk.NewInt(1e6)).Once() + suite.Keeper.AfterPoolDepositCreated(suite.Ctx, "ukava/usdx", depositor, sdk.NewInt(1e6)) + swapHooks.On("BeforePoolDepositModified", suite.Ctx, "ukava/usdx", depositor, sdk.NewInt(1e6)).Once() + suite.Keeper.BeforePoolDepositModified(suite.Ctx, "ukava/usdx", depositor, sdk.NewInt(1e6)) + swapHooks.AssertExpectations(suite.T()) + + // test second set panics + suite.PanicsWithValue("cannot set swap hooks twice", func() { + suite.Keeper.SetHooks(swapHooks) + }, "expected hooks to panic on second set") +} diff --git a/x/swap/keeper/swap_test.go b/x/swap/keeper/swap_test.go index 947605c8..92048eca 100644 --- a/x/swap/keeper/swap_test.go +++ b/x/swap/keeper/swap_test.go @@ -268,7 +268,7 @@ func (suite *keeperTestSuite) TestSwapExactForTokens_PanicOnInvalidPool() { suite.Require().True(found, "expected pool record to exist") poolRecord.TotalShares = sdk.ZeroInt() - suite.Keeper.SetPool(suite.Ctx, poolRecord) + suite.Keeper.SetPool_Raw(suite.Ctx, poolRecord) balance := sdk.NewCoins( sdk.NewCoin("ukava", sdk.NewInt(10e6)), @@ -579,7 +579,7 @@ func (suite *keeperTestSuite) TestSwapForExactTokens_PanicOnInvalidPool() { suite.Require().True(found, "expected pool record to exist") poolRecord.TotalShares = sdk.ZeroInt() - suite.Keeper.SetPool(suite.Ctx, poolRecord) + suite.Keeper.SetPool_Raw(suite.Ctx, poolRecord) balance := sdk.NewCoins( sdk.NewCoin("ukava", sdk.NewInt(10e6)), diff --git a/x/swap/keeper/withdraw.go b/x/swap/keeper/withdraw.go index b43b47c0..1246e51f 100644 --- a/x/swap/keeper/withdraw.go +++ b/x/swap/keeper/withdraw.go @@ -52,8 +52,7 @@ func (k Keeper) Withdraw(ctx sdk.Context, owner sdk.AccAddress, shares sdk.Int, } k.updatePool(ctx, poolID, pool) - - k.hooks.BeforePoolDepositModified(ctx, poolID, owner, shareRecord.SharesOwned) + k.BeforePoolDepositModified(ctx, poolID, owner, shareRecord.SharesOwned) k.updateShares(ctx, owner, poolID, shareRecord.SharesOwned.Sub(shares)) err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleAccountName, owner, withdrawnAmount) @@ -73,20 +72,3 @@ func (k Keeper) Withdraw(ctx sdk.Context, owner sdk.AccAddress, shares sdk.Int, return nil } - -func (k Keeper) updatePool(ctx sdk.Context, poolID string, pool *types.DenominatedPool) { - if pool.TotalShares().IsZero() { - k.DeletePool(ctx, poolID) - } else { - k.SetPool(ctx, types.NewPoolRecordFromPool(pool)) - } -} - -func (k Keeper) updateShares(ctx sdk.Context, owner sdk.AccAddress, poolID string, shares sdk.Int) { - if shares.IsZero() { - k.DeleteDepositorShares(ctx, owner, poolID) - } else { - shareRecord := types.NewShareRecord(owner, poolID, shares) - k.SetDepositorShares(ctx, shareRecord) - } -} diff --git a/x/swap/keeper/withdraw_test.go b/x/swap/keeper/withdraw_test.go index 7f06f931..88784581 100644 --- a/x/swap/keeper/withdraw_test.go +++ b/x/swap/keeper/withdraw_test.go @@ -196,7 +196,7 @@ func (suite *keeperTestSuite) TestWithdraw_PanicOnInvalidPool() { suite.Require().True(found, "expected pool record to exist") poolRecord.TotalShares = sdk.ZeroInt() - suite.Keeper.SetPool(suite.Ctx, poolRecord) + suite.Keeper.SetPool_Raw(suite.Ctx, poolRecord) suite.PanicsWithValue("invalid pool ukava/usdx: invalid pool: total shares must be greater than zero", func() { _ = suite.Keeper.Withdraw(suite.Ctx, owner.GetAddress(), totalShares, reserves[0], reserves[1]) diff --git a/x/swap/types/mocks/swap_hooks.go b/x/swap/types/mocks/swap_hooks.go new file mode 100644 index 00000000..d2449a1d --- /dev/null +++ b/x/swap/types/mocks/swap_hooks.go @@ -0,0 +1,24 @@ +// Code generated by mockery 2.7.4. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + + types "github.com/cosmos/cosmos-sdk/types" +) + +// SwapHooks is an autogenerated mock type for the SwapHooks type +type SwapHooks struct { + mock.Mock +} + +// AfterPoolDepositCreated provides a mock function with given fields: ctx, poolID, depositor, sharedOwned +func (_m *SwapHooks) AfterPoolDepositCreated(ctx types.Context, poolID string, depositor types.AccAddress, sharedOwned types.Int) { + _m.Called(ctx, poolID, depositor, sharedOwned) +} + +// BeforePoolDepositModified provides a mock function with given fields: ctx, poolID, depositor, sharedOwned +func (_m *SwapHooks) BeforePoolDepositModified(ctx types.Context, poolID string, depositor types.AccAddress, sharedOwned types.Int) { + _m.Called(ctx, poolID, depositor, sharedOwned) +}