From 96caf36cea2e3aa7b161b512d2acdfdb3db8c5ce Mon Sep 17 00:00:00 2001 From: "Matt, Park" <45252226+mattverse@users.noreply.github.com> Date: Thu, 18 May 2023 03:11:59 +0900 Subject: [PATCH] [CL Message Audit]: MsgWithdrawPosition (#5135) * WIP * Add test cases * Uncomment tests * Uncomment more tests * Add lock test * And uncomment test * Update x/concentrated-liquidity/incentives.go Co-authored-by: Roman * Romans feedback * Update x/concentrated-liquidity/lp.go Co-authored-by: Adam Tucker * Update x/concentrated-liquidity/lp_test.go Co-authored-by: Adam Tucker * Adams review --------- Co-authored-by: Roman Co-authored-by: Adam Tucker --- osmoutils/accum/accum.go | 1 + x/concentrated-liquidity/export_test.go | 12 ++- x/concentrated-liquidity/fees.go | 9 +- x/concentrated-liquidity/fees_test.go | 4 +- x/concentrated-liquidity/incentives.go | 13 +-- x/concentrated-liquidity/incentives_test.go | 4 +- x/concentrated-liquidity/lp.go | 31 ++++--- x/concentrated-liquidity/lp_test.go | 96 +++++++++++++++------ x/concentrated-liquidity/math/math.go | 1 + x/concentrated-liquidity/position.go | 24 +++--- x/concentrated-liquidity/position_test.go | 68 +++++++++++---- x/concentrated-liquidity/tick.go | 9 +- 12 files changed, 178 insertions(+), 94 deletions(-) diff --git a/osmoutils/accum/accum.go b/osmoutils/accum/accum.go index bd50d015d0c..5e43ae235f8 100644 --- a/osmoutils/accum/accum.go +++ b/osmoutils/accum/accum.go @@ -398,6 +398,7 @@ func (accum AccumulatorObject) GetValue() sdk.DecCoins { // ClaimRewards claims the rewards for the given address, and returns the amount of rewards claimed. // Upon claiming the rewards, the position at the current address is reset to have no // unclaimed rewards. The position's accumulator is also set to the current accumulator value. +// The position state is removed if the position shares is equal to zero. // // Returns error if // - no position exists for the given address diff --git a/x/concentrated-liquidity/export_test.go b/x/concentrated-liquidity/export_test.go index 92d8757e9b3..03a95b0de32 100644 --- a/x/concentrated-liquidity/export_test.go +++ b/x/concentrated-liquidity/export_test.go @@ -206,8 +206,12 @@ func ValidateTickRangeIsValid(tickSpacing uint64, lowerTick int64, upperTick int return validateTickRangeIsValid(tickSpacing, lowerTick, upperTick) } -func PreparePositionAccumulator(feeAccumulator accum.AccumulatorObject, positionKey string, feeGrowthOutside sdk.DecCoins) error { - return preparePositionAccumulator(feeAccumulator, positionKey, feeGrowthOutside) +func UpdatePosValueToInitValuePlusGrowthOutside(feeAccumulator accum.AccumulatorObject, positionKey string, feeGrowthOutside sdk.DecCoins) error { + return updatePositionToInitValuePlusGrowthOutside(feeAccumulator, positionKey, feeGrowthOutside) +} + +func UpdatePositionToInitValuePlusGrowthOutside(accumulator accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) error { + return updatePositionToInitValuePlusGrowthOutside(accumulator, positionKey, growthOutside) } func (k Keeper) CreatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, tokensProvided sdk.Coins, amount0Min, amount1Min sdk.Int, lowerTick, upperTick int64) (positionId uint64, actualAmount0 sdk.Int, actualAmount1 sdk.Int, liquidityDelta sdk.Dec, joinTime time.Time, lowerTickResult int64, upperTickResult int64, err error) { @@ -280,8 +284,8 @@ func GetUptimeTrackerValues(uptimeTrackers []model.UptimeTracker) []sdk.DecCoins return getUptimeTrackerValues(uptimeTrackers) } -func PrepareAccumAndClaimRewards(accum accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) (sdk.Coins, sdk.DecCoins, error) { - return prepareAccumAndClaimRewards(accum, positionKey, growthOutside) +func UpdateAccumAndClaimRewards(accum accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) (sdk.Coins, sdk.DecCoins, error) { + return updateAccumAndClaimRewards(accum, positionKey, growthOutside) } func (k Keeper) ClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, error) { diff --git a/x/concentrated-liquidity/fees.go b/x/concentrated-liquidity/fees.go index 1f9cc119f1e..6d9ccb28464 100644 --- a/x/concentrated-liquidity/fees.go +++ b/x/concentrated-liquidity/fees.go @@ -100,7 +100,7 @@ func (k Keeper) initOrUpdatePositionFeeAccumulator(ctx sdk.Context, poolId uint6 // At time t, we track fee growth inside from 0 to t. // Then, the update happens at time t + 1. The call below makes the position's // accumulator to be "fee growth inside from 0 to t + fee growth outside from 0 to t + 1". - err = preparePositionAccumulator(feeAccumulator, positionKey, feeGrowthOutside) + err = updatePositionToInitValuePlusGrowthOutside(feeAccumulator, positionKey, feeGrowthOutside) if err != nil { return err } @@ -275,7 +275,7 @@ func (k Keeper) prepareClaimableFees(ctx sdk.Context, positionId uint64) (sdk.Co } // Claim rewards, set the unclaimed rewards to zero, and update the position's accumulator value to reflect the current accumulator value. - feesClaimed, _, err := prepareAccumAndClaimRewards(feeAccumulator, positionKey, feeGrowthOutside) + feesClaimed, _, err := updateAccumAndClaimRewards(feeAccumulator, positionKey, feeGrowthOutside) if err != nil { return nil, err } @@ -295,12 +295,11 @@ func calculateFeeGrowth(targetTick int64, ticksFeeGrowthOppositeDirectionOfLastT return ticksFeeGrowthOppositeDirectionOfLastTraversal } -// preparePositionAccumulator is called prior to updating unclaimed rewards, +// updatePositionToInitValuePlusGrowthOutside is called prior to updating unclaimed rewards, // as we must set the position's accumulator value to the sum of // - the fee/uptime growth inside at position creation time (position.InitAccumValue) // - fee/uptime growth outside at the current block time (feeGrowthOutside/uptimeGrowthOutside) -// CONTRACT: position accumulator value prior to this call is equal to the growth inside the position at the time of last update. -func preparePositionAccumulator(accumulator accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) error { +func updatePositionToInitValuePlusGrowthOutside(accumulator accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) error { position, err := accum.GetPosition(accumulator, positionKey) if err != nil { return err diff --git a/x/concentrated-liquidity/fees_test.go b/x/concentrated-liquidity/fees_test.go index 295184239c6..e9c72b7de86 100644 --- a/x/concentrated-liquidity/fees_test.go +++ b/x/concentrated-liquidity/fees_test.go @@ -1335,7 +1335,7 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition_UpdatingPositio } } -func (s *KeeperTestSuite) TestPreparePositionAccumulator() { +func (s *KeeperTestSuite) TestUpdatePosValueToInitValuePlusGrowthOutside() { validPositionKey := types.KeyFeePositionAccumulator(1) invalidPositionKey := types.KeyFeePositionAccumulator(2) tests := []struct { @@ -1382,7 +1382,7 @@ func (s *KeeperTestSuite) TestPreparePositionAccumulator() { } // System under test. - err = cl.PreparePositionAccumulator(poolFeeAccumulator, positionKey, tc.feeGrowthOutside) + err = cl.UpdatePosValueToInitValuePlusGrowthOutside(poolFeeAccumulator, positionKey, tc.feeGrowthOutside) if tc.expectError != nil { s.Require().Error(err) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 43af2160a30..5670f329e6a 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -677,7 +677,7 @@ func (k Keeper) initOrUpdatePositionUptimeAccumulators(ctx sdk.Context, poolId u } } else { // Prep accum since we claim rewards first under the hood before any update (otherwise we would overpay) - err = preparePositionAccumulator(curUptimeAccum, positionName, globalUptimeGrowthOutsideRange[uptimeIndex]) + err = updatePositionToInitValuePlusGrowthOutside(curUptimeAccum, positionName, globalUptimeGrowthOutsideRange[uptimeIndex]) if err != nil { return err } @@ -694,7 +694,7 @@ func (k Keeper) initOrUpdatePositionUptimeAccumulators(ctx sdk.Context, poolId u return nil } -// prepareAccumAndClaimRewards claims and returns the rewards that `positionKey` is entitled to, updating the accumulator's value before +// updateAccumAndClaimRewards claims and returns the rewards that `positionKey` is entitled to, updating the accumulator's value before // and after claiming to ensure that rewards are never overdistributed. // CONTRACT: position accumulator value prior to this call is equal to the growth inside the position at the time of last update. // Returns error if: @@ -702,14 +702,15 @@ func (k Keeper) initOrUpdatePositionUptimeAccumulators(ctx sdk.Context, poolId u // - fails to claim rewards // - fails to check if position record exists // - fails to update position accumulator with the current growth inside the position -func prepareAccumAndClaimRewards(accum accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) (sdk.Coins, sdk.DecCoins, error) { +func updateAccumAndClaimRewards(accum accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) (sdk.Coins, sdk.DecCoins, error) { // Set the position's accumulator value to it's initial value at creation time plus the growth outside at this moment. - err := preparePositionAccumulator(accum, positionKey, growthOutside) + err := updatePositionToInitValuePlusGrowthOutside(accum, positionKey, growthOutside) if err != nil { return sdk.Coins{}, sdk.DecCoins{}, err } // Claim rewards, set the unclaimed rewards to zero, and update the position's accumulator value to reflect the current accumulator value. + // Removes the position state from accum if remaining liquidity is zero for the position. incentivesClaimedCurrAccum, dust, err := accum.ClaimRewards(positionKey) if err != nil { return sdk.Coins{}, sdk.DecCoins{}, err @@ -747,7 +748,7 @@ func moveRewardsToNewPositionAndDeleteOldAcc(ctx sdk.Context, accum accum.Accumu return types.ModifySamePositionAccumulatorError{PositionAccName: oldPositionName} } - if err := preparePositionAccumulator(accum, oldPositionName, growthOutside); err != nil { + if err := updatePositionToInitValuePlusGrowthOutside(accum, oldPositionName, growthOutside); err != nil { return err } @@ -826,7 +827,7 @@ func (k Keeper) claimAllIncentivesForPosition(ctx sdk.Context, positionId uint64 // If the accumulator contains the position, claim the position's incentives. if hasPosition { - collectedIncentivesForUptime, dust, err := prepareAccumAndClaimRewards(uptimeAccum, positionName, uptimeGrowthOutside[uptimeIndex]) + collectedIncentivesForUptime, dust, err := updateAccumAndClaimRewards(uptimeAccum, positionName, uptimeGrowthOutside[uptimeIndex]) if err != nil { return sdk.Coins{}, sdk.Coins{}, err } diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 68e2ed667b4..0bea1561d1b 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -3259,7 +3259,7 @@ func (s *KeeperTestSuite) TestCreateIncentive() { } } -func (s *KeeperTestSuite) TestPrepareAccumAndClaimRewards() { +func (s *KeeperTestSuite) TestUpdateAccumAndClaimRewards() { validPositionKey := types.KeyFeePositionAccumulator(1) invalidPositionKey := types.KeyFeePositionAccumulator(2) tests := map[string]struct { @@ -3307,7 +3307,7 @@ func (s *KeeperTestSuite) TestPrepareAccumAndClaimRewards() { poolFeeAccumulator.AddToAccumulator(tc.growthOutside.Add(tc.growthInside...)) // System under test. - amountClaimed, _, err := cl.PrepareAccumAndClaimRewards(poolFeeAccumulator, positionKey, tc.growthOutside) + amountClaimed, _, err := cl.UpdateAccumAndClaimRewards(poolFeeAccumulator, positionKey, tc.growthOutside) if tc.expectError != nil { s.Require().Error(err) diff --git a/x/concentrated-liquidity/lp.go b/x/concentrated-liquidity/lp.go index 4d97abf6cc5..9caeebb3e96 100644 --- a/x/concentrated-liquidity/lp.go +++ b/x/concentrated-liquidity/lp.go @@ -127,12 +127,13 @@ func (k Keeper) createPosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr // WithdrawPosition attempts to withdraw liquidityAmount from a position with the given pool id in the given tick range. // On success, returns a positive amount of each token withdrawn. +// If we are attempting to withdraw all liquidity available in the position, we also collect fees and incentives for the position. // When the last position within a pool is removed, this function calls an AfterLastPoolPosistionRemoved listener // Currently, it creates twap records. Assumming that pool had all liqudity drained and then re-initialized, // the whole twap state is completely reset. This is because when there is no liquidity in pool, spot price // is undefined. // Additionally, when the last position is removed by calling this method, the current sqrt price and current -// tick are set to zero. +// tick of the pool are set to zero. // Returns error if // - the provided owner does not own the position being withdrawn // - there is no position in the given tick ranges @@ -150,14 +151,20 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position return sdk.Int{}, sdk.Int{}, types.NotPositionOwnerError{PositionId: positionId, Address: owner.String()} } + // Defense in depth, requestedLiquidityAmountToWithdraw should always be a positive value. + if requestedLiquidityAmountToWithdraw.IsNegative() { + return sdk.Int{}, sdk.Int{}, types.InsufficientLiquidityError{Actual: requestedLiquidityAmountToWithdraw, Available: position.Liquidity} + } + // If underlying lock exists in state, validate unlocked conditions are met before withdrawing liquidity. - // If unlocked conditions are met, remove the link between the position and the underlying lock. + // If the underlying lock for the position has been matured, remove the link between the position and the underlying lock. positionHasActiveUnderlyingLock, lockId, err := k.positionHasActiveUnderlyingLockAndUpdate(ctx, positionId) if err != nil { return sdk.Int{}, sdk.Int{}, err } + + // If an underlying lock for the position exists, and the lock is not mature, return error. if positionHasActiveUnderlyingLock { - // Lock is not mature, return error. return sdk.Int{}, sdk.Int{}, types.LockNotMatureError{PositionId: position.PositionId, LockId: lockId} } @@ -167,13 +174,8 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position return sdk.Int{}, sdk.Int{}, err } - // Check if the provided tick range is valid according to the pool's tick spacing and module parameters. - if err := validateTickRangeIsValid(pool.GetTickSpacing(), position.LowerTick, position.UpperTick); err != nil { - return sdk.Int{}, sdk.Int{}, err - } - // Retrieve the position in the pool for the provided owner and tick range. - availableLiquidity, err := k.GetPositionLiquidity(ctx, positionId) + positionLiquidity, err := k.GetPositionLiquidity(ctx, positionId) if err != nil { return sdk.Int{}, sdk.Int{}, err } @@ -185,8 +187,8 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position // Check if the requested liquidity amount to withdraw is less than or equal to the available liquidity for the position. // If it is greater than the available liquidity, return an error. - if requestedLiquidityAmountToWithdraw.GT(availableLiquidity) { - return sdk.Int{}, sdk.Int{}, types.InsufficientLiquidityError{Actual: requestedLiquidityAmountToWithdraw, Available: availableLiquidity} + if requestedLiquidityAmountToWithdraw.GT(positionLiquidity) { + return sdk.Int{}, sdk.Int{}, types.InsufficientLiquidityError{Actual: requestedLiquidityAmountToWithdraw, Available: positionLiquidity} } // Calculate the change in liquidity for the pool based on the requested amount to withdraw. @@ -208,7 +210,7 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position // If the requested liquidity amount to withdraw is equal to the available liquidity, delete the position from state. // Ensure we collect any outstanding fees and incentives prior to deleting the position from state. This claiming // process also clears position records from fee and incentive accumulators. - if requestedLiquidityAmountToWithdraw.Equal(availableLiquidity) { + if requestedLiquidityAmountToWithdraw.Equal(positionLiquidity) { if _, err := k.collectFees(ctx, owner, positionId); err != nil { return sdk.Int{}, sdk.Int{}, err } @@ -343,12 +345,13 @@ func (k Keeper) UpdatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr currentTick := pool.GetCurrentTick().Int64() - // update tickInfo state for lower tick and upper tick + // update lower tickInfo state err = k.initOrUpdateTick(ctx, poolId, currentTick, lowerTick, liquidityDelta, false) if err != nil { return sdk.Int{}, sdk.Int{}, err } + // update upper tickInfo state err = k.initOrUpdateTick(ctx, poolId, currentTick, upperTick, liquidityDelta, true) if err != nil { return sdk.Int{}, sdk.Int{}, err @@ -445,7 +448,7 @@ func (k Keeper) initializeInitialPositionForPool(ctx sdk.Context, pool types.Con return nil } -// uninitializePool uninitializes a pool if it has no liquidity. +// uninitializePool reinitializes a pool if it has no liquidity. // It does so by setting the current square root price and tick to zero. // This is necessary for the twap to correctly detect a spot price error // when there is no liquidity in the pool. diff --git a/x/concentrated-liquidity/lp_test.go b/x/concentrated-liquidity/lp_test.go index d613081ad01..84512ff04ca 100644 --- a/x/concentrated-liquidity/lp_test.go +++ b/x/concentrated-liquidity/lp_test.go @@ -2,14 +2,14 @@ package concentrated_liquidity_test import ( "errors" - "fmt" "time" sdk "github.com/cosmos/cosmos-sdk/types" distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types" - "github.com/osmosis-labs/osmosis/osmomath" + "github.com/osmosis-labs/osmosis/osmoutils" cl "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity" + "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/model" clmodel "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/model" types "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/types" ) @@ -482,6 +482,14 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { }, timeElapsed: defaultTimeElapsed, }, + "error: try withdrawing negative liquidity": { + setupConfig: baseCase, + sutConfigOverwrite: &lpTest{ + liquidityAmount: baseCase.liquidityAmount.Sub(baseCase.liquidityAmount.Mul(sdk.NewDec(2))), + expectedError: types.InsufficientLiquidityError{Actual: baseCase.liquidityAmount.Sub(baseCase.liquidityAmount.Mul(sdk.NewDec(2))), Available: baseCase.liquidityAmount}, + }, + timeElapsed: defaultTimeElapsed, + }, "error: attempt to withdraw a position that does not belong to the caller": { setupConfig: baseCase, sutConfigOverwrite: &lpTest{ @@ -490,7 +498,6 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { timeElapsed: defaultTimeElapsed, withdrawWithNonOwner: true, }, - // TODO: test with custom amounts that potentially lead to truncations. } for name, tc := range tests { @@ -539,6 +546,7 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { s.setListenerMockOnConcentratedLiquidityKeeper() s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(tc.timeElapsed)) + store := s.Ctx.KVStore(s.App.GetKey(types.StoreKey)) // Set global fee growth to 1 ETH and charge the fee to the pool. globalFeeGrowth := sdk.NewDecCoin(ETH, sdk.NewInt(1)) @@ -570,10 +578,10 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { expectedFullIncentivesFromAllUptimes := expectedIncentivesFromUptimeGrowth(defaultUptimeGrowth, liquidityCreated, types.SupportedUptimes[len(types.SupportedUptimes)-1], defaultMultiplier) s.FundAcc(pool.GetIncentivesAddress(), expectedFullIncentivesFromAllUptimes) - // Note the pool and owner balances before collecting fees. - poolBalanceBeforeCollect := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetAddress()) - incentivesBalanceBeforeCollect := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetIncentivesAddress()) - ownerBalancerBeforeCollect := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) + // Note the pool and owner balances before withdrawal of the position. + poolBalanceBeforeWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetAddress()) + incentivesBalanceBeforeWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetIncentivesAddress()) + ownerBalancerBeforeWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) expectedPoolBalanceDelta := expectedFeesClaimed.Add(sdk.NewCoin(ETH, config.amount0Expected.Abs())).Add(sdk.NewCoin(USDC, config.amount1Expected.Abs())) @@ -600,32 +608,24 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { // If the remaining liquidity is zero, all fees and incentives should be collected and the position should be deleted. // Check if all fees and incentives were collected. - poolBalanceAfterCollect := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetAddress()) - incentivesBalanceAfterCollect := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetIncentivesAddress()) - ownerBalancerAfterCollect := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) + poolBalanceAfterWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetAddress()) + incentivesBalanceAfterWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetIncentivesAddress()) + ownerBalancerAfterWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) communityPoolBalanceAfter := s.App.BankKeeper.GetAllBalances(s.Ctx, s.App.AccountKeeper.GetModuleAddress(distributiontypes.ModuleName)) + // owner should only have tokens equivilent to the delta balance of the pool expectedOwnerBalanceDelta := expectedPoolBalanceDelta.Add(expectedIncentivesClaimed...) - actualOwnerBalancerDelta := ownerBalancerAfterCollect.Sub(ownerBalancerBeforeCollect) + actualOwnerBalancerDelta := ownerBalancerAfterWithdraw.Sub(ownerBalancerBeforeWithdraw) communityPoolBalanceDelta := communityPoolBalanceAfter.Sub(communityPoolBalanceBefore) + actualIncentivesClaimed := incentivesBalanceBeforeWithdraw.Sub(incentivesBalanceAfterWithdraw).Sub(communityPoolBalanceDelta) - actualIncentivesClaimed := incentivesBalanceBeforeCollect.Sub(incentivesBalanceAfterCollect).Sub(communityPoolBalanceDelta) - - s.Require().Equal(expectedPoolBalanceDelta.String(), poolBalanceBeforeCollect.Sub(poolBalanceAfterCollect).String()) - - // TODO: Investigate why full range liquidity positions are slightly under claiming incentives here - // https://github.com/osmosis-labs/osmosis/issues/4897 - errTolerance := osmomath.ErrTolerance{ - AdditiveTolerance: sdk.NewDec(3), - RoundingDir: osmomath.RoundDown, - } - + s.Require().Equal(expectedPoolBalanceDelta.String(), poolBalanceBeforeWithdraw.Sub(poolBalanceAfterWithdraw).String()) s.Require().NotEmpty(expectedOwnerBalanceDelta) for _, coin := range expectedOwnerBalanceDelta { expected := expectedOwnerBalanceDelta.AmountOf(coin.Denom) actual := actualOwnerBalancerDelta.AmountOf(coin.Denom) - s.Require().Equal(0, errTolerance.Compare(expected, actual), fmt.Sprintf("expected %s, actual %s", expected, actual)) + s.Require().True(expected.Equal(actual)) } if tc.timeElapsed > 0 { @@ -634,15 +634,59 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { for _, coin := range expectedIncentivesClaimed { expected := expectedIncentivesClaimed.AmountOf(coin.Denom) actual := actualIncentivesClaimed.AmountOf(coin.Denom) - s.Require().Equal(0, errTolerance.Compare(expected, actual), fmt.Sprintf("expected %s, actual %s", expected, actual)) + s.Require().True(expected.Equal(actual)) } + // if the position's expected remaining liquidity is equal to zero, we check if all state + // have been correctly deleted. if expectedRemainingLiquidity.IsZero() { - // Check that the positionLiquidity was deleted. - positionLiquidity, err := concentratedLiquidityKeeper.GetPositionLiquidity(s.Ctx, config.positionId) + // Check that the position was deleted. + position, err := concentratedLiquidityKeeper.GetPosition(s.Ctx, config.positionId) s.Require().Error(err) s.Require().ErrorAs(err, &types.PositionIdNotFoundError{PositionId: config.positionId}) + s.Require().Equal(clmodel.Position{}, position) + + isPositionOwner, err := concentratedLiquidityKeeper.IsPositionOwner(s.Ctx, owner, config.poolId, config.positionId) + s.Require().NoError(err) + s.Require().False(isPositionOwner) + + // Since the positionLiquidity is deleted, retrieving it should return an error. + positionLiquidity, err := s.App.ConcentratedLiquidityKeeper.GetPositionLiquidity(s.Ctx, config.positionId) + s.Require().Error(err) + s.Require().ErrorIs(err, types.PositionIdNotFoundError{PositionId: config.positionId}) s.Require().Equal(sdk.Dec{}, positionLiquidity) + + // check underlying stores were correctly deleted + emptyPositionStruct := clmodel.Position{} + positionIdToPositionKey := types.KeyPositionId(config.positionId) + osmoutils.Get(store, positionIdToPositionKey, &position) + s.Require().Equal(model.Position{}, emptyPositionStruct) + + // Retrieve the position ID from the store via owner/poolId key and compare to expected values. + ownerPoolIdToPositionIdKey := types.KeyAddressPoolIdPositionId(s.TestAccs[0], defaultPoolId, DefaultPositionId) + positionIdBytes := store.Get(ownerPoolIdToPositionIdKey) + s.Require().Nil(positionIdBytes) + + // Retrieve the position ID from the store via poolId key and compare to expected values. + poolIdtoPositionIdKey := types.KeyPoolPositionPositionId(defaultPoolId, DefaultPositionId) + positionIdBytes = store.Get(poolIdtoPositionIdKey) + s.Require().Nil(positionIdBytes) + + // Retrieve the position ID to underlying lock ID mapping from the store and compare to expected values. + positionIdToLockIdKey := types.KeyPositionIdForLock(DefaultPositionId) + underlyingLockIdBytes := store.Get(positionIdToLockIdKey) + s.Require().Nil(underlyingLockIdBytes) + + // Retrieve the lock ID to position ID mapping from the store and compare to expected values. + lockIdToPositionIdKey := types.KeyLockIdForPositionId(config.underlyingLockId) + positionIdBytes = store.Get(lockIdToPositionIdKey) + s.Require().Nil(positionIdBytes) + + // ensure that the lock is still there if there was lock that was existing before the withdraw process + if tc.createLockLocked || tc.createLockUnlocked || tc.createLockUnlocking { + _, err = s.App.LockupKeeper.GetLockByID(s.Ctx, 1) + s.Require().NoError(err) + } } else { // Check that the position was updated. s.validatePositionUpdate(s.Ctx, config.positionId, expectedRemainingLiquidity) diff --git a/x/concentrated-liquidity/math/math.go b/x/concentrated-liquidity/math/math.go index bb6fdcd808b..386d2e601da 100644 --- a/x/concentrated-liquidity/math/math.go +++ b/x/concentrated-liquidity/math/math.go @@ -191,6 +191,7 @@ func GetLiquidityFromAmounts(sqrtPrice, sqrtPriceA, sqrtPriceB sdk.Dec, amount0, return liquidity } +// AddLiquidity adds or subtracts liquidityB from liquidityA, depending on whether liquidityB is positive or negative. func AddLiquidity(liquidityA, liquidityB sdk.Dec) (finalLiquidity sdk.Dec) { if liquidityB.LT(sdk.ZeroDec()) { return liquidityA.Sub(liquidityB.Abs()) diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go index 420d14065d9..603427c689c 100644 --- a/x/concentrated-liquidity/position.go +++ b/x/concentrated-liquidity/position.go @@ -1,7 +1,6 @@ package concentrated_liquidity import ( - "errors" "fmt" "strconv" "strings" @@ -173,6 +172,11 @@ func (k Keeper) GetUserPositions(ctx sdk.Context, addr sdk.AccAddress, poolId ui } // SetPosition sets the position information for a given user in a given pool. +// This includes creating state entries of: +// - position id -> position object mapping +// - address-pool-positionID -> position object mapping +// - pool id -> position id mapping +// - (if exists) position id <> lock id mapping. func (k Keeper) SetPosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, @@ -195,10 +199,6 @@ func (k Keeper) SetPosition(ctx sdk.Context, Liquidity: liquidity, } - // TODO: The following state mappings are not properly implemented in genState. - // (i.e. if you state export, these mappings are not retained.) - // https://github.com/osmosis-labs/osmosis/issues/4875 - // Set the position ID to position mapping. positionIdKey := types.KeyPositionId(positionId) osmoutils.MustSet(store, positionIdKey, &position) @@ -666,8 +666,8 @@ func (k Keeper) setPositionIdToLock(ctx sdk.Context, positionId, underlyingLockI store.Set(lockIdKey, sdk.Uint64ToBigEndian(positionId)) } -// RemovePositionIdToLock removes both the positionId to lock mapping and the lock to positionId mapping in state. -func (k Keeper) RemovePositionIdToLock(ctx sdk.Context, positionId, underlyingLockId uint64) { +// RemovePositionIdForLockId removes both the positionId to lock mapping and the lock to positionId mapping in state. +func (k Keeper) RemovePositionIdForLockId(ctx sdk.Context, positionId, underlyingLockId uint64) { store := ctx.KVStore(k.storeKey) // Get the position ID to lock mappings. @@ -685,10 +685,8 @@ func (k Keeper) RemovePositionIdToLock(ctx sdk.Context, positionId, underlyingLo func (k Keeper) PositionHasActiveUnderlyingLock(ctx sdk.Context, positionId uint64) (hasActiveUnderlyingLock bool, lockId uint64, err error) { // Get the lock ID for the position. lockId, err = k.GetLockIdFromPositionId(ctx, positionId) - if errors.Is(err, types.PositionIdToLockNotFoundError{PositionId: positionId}) { + if err != nil { return false, 0, nil - } else if err != nil { - return false, 0, err } // Check if the underlying lock is mature. @@ -715,10 +713,10 @@ func (k Keeper) positionHasActiveUnderlyingLockAndUpdate(ctx sdk.Context, positi // Defense in depth check. If we have an active underlying lock but no lock ID, return an error. return false, 0, types.PositionIdToLockNotFoundError{PositionId: positionId} } + // If the position does not have an active underlying lock but still has a lock ID associated with it, + // remove the link between the position and the underlying lock since the lock is mature. if !hasActiveUnderlyingLock && lockId != 0 { - // If the position does not have an active underlying lock but still has a lock ID associated with it, - // remove the link between the position and the underlying lock since the lock is mature. - k.RemovePositionIdToLock(ctx, positionId, lockId) + k.RemovePositionIdForLockId(ctx, positionId, lockId) return false, 0, nil } return hasActiveUnderlyingLock, lockId, nil diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go index 3b1ed50c920..dc11fe4069c 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/x/concentrated-liquidity/position_test.go @@ -1566,13 +1566,14 @@ func (s *KeeperTestSuite) TestPositionHasActiveUnderlyingLock() { defaultPositionCoins := sdk.NewCoins(DefaultCoin0, DefaultCoin1) type testParams struct { - name string - createPosition func(s *KeeperTestSuite) (uint64, uint64) - expectedHasActiveLock bool - expectedHasActiveLockAfterTimeUpdate bool - expectedLockError bool - expectedPositionLockID uint64 - expectedGetPositionLockIdErr bool + name string + createPosition func(s *KeeperTestSuite) (uint64, uint64) + expectedHasActiveLock bool + expectedHasActiveLockAfterTimeUpdate bool + expectedLockError bool + expectedPositionHasActiveUnderlyingLockErr bool + expectedPositionLockID uint64 + expectedGetPositionLockIdErr bool } tests := []testParams{ @@ -1621,6 +1622,18 @@ func (s *KeeperTestSuite) TestPositionHasActiveUnderlyingLock() { expectedPositionLockID: 0, expectedGetPositionLockIdErr: true, }, + { + name: "invalid position, invalid lock: should return false", + createPosition: func(s *KeeperTestSuite) (uint64, uint64) { + return 100, 0 + }, + expectedHasActiveLock: false, + expectedHasActiveLockAfterTimeUpdate: false, + expectedLockError: true, + expectedPositionHasActiveUnderlyingLockErr: true, + expectedPositionLockID: 0, + expectedGetPositionLockIdErr: true, + }, } for _, tc := range tests { @@ -1678,15 +1691,16 @@ func (s *KeeperTestSuite) TestPositionHasActiveUnderlyingLockAndUpdate() { defaultPositionCoins := sdk.NewCoins(DefaultCoin0, DefaultCoin1) type testParams struct { - name string - createPosition func(s *KeeperTestSuite) (uint64, uint64) - expectedHasActiveLock bool - expectedHasActiveLockAfterTimeUpdate bool - expectedLockError bool - expectedPositionLockID uint64 - expectedPositionLockIDAfterTimeUpdate uint64 - expectedGetPositionLockIdErr bool - expectedGetPositionLockIdErrAfterTimeUpdate bool + name string + createPosition func(s *KeeperTestSuite) (uint64, uint64) + expectedHasActiveLock bool + expectedHasActiveLockAfterTimeUpdate bool + expectedLockError bool + expectedPositionLockID uint64 + expectedPositionLockIDAfterTimeUpdate uint64 + expectedGetPositionLockIdErr bool + expectedGetPositionLockIdErrAfterTimeUpdate bool + expectedPositionHasActiveUnderlyingLockAndUpdate bool } tests := []testParams{ @@ -1741,6 +1755,20 @@ func (s *KeeperTestSuite) TestPositionHasActiveUnderlyingLockAndUpdate() { expectedGetPositionLockIdErr: true, expectedGetPositionLockIdErrAfterTimeUpdate: true, }, + { + name: "invalid position without lock ", + // we return invalid lock id with no-op to trigger error + createPosition: func(s *KeeperTestSuite) (uint64, uint64) { + return 10, 0 + }, + expectedHasActiveLock: false, + expectedHasActiveLockAfterTimeUpdate: false, + expectedLockError: true, + expectedPositionLockID: 0, + expectedPositionLockIDAfterTimeUpdate: 0, + expectedGetPositionLockIdErr: true, + expectedGetPositionLockIdErrAfterTimeUpdate: true, + }, } for _, tc := range tests { @@ -1757,7 +1785,11 @@ func (s *KeeperTestSuite) TestPositionHasActiveUnderlyingLockAndUpdate() { // System under test (mutative) hasActiveLockInState, retrievedLockID, err := s.App.ConcentratedLiquidityKeeper.PositionHasActiveUnderlyingLockAndUpdate(s.Ctx, positionID) - s.Require().NoError(err) + if tc.expectedPositionHasActiveUnderlyingLockAndUpdate { + s.Require().Error(err) + } else { + s.Require().NoError(err) + } s.Require().Equal(tc.expectedHasActiveLock, hasActiveLockInState) s.Require().Equal(tc.expectedPositionLockID, retrievedLockID) @@ -1843,7 +1875,7 @@ func (s *KeeperTestSuite) TestPositionToLockCRUD() { s.Require().Equal(positionId, retrievedPositionId) // Remove the lockId from the position - s.App.ConcentratedLiquidityKeeper.RemovePositionIdToLock(s.Ctx, positionId, retrievedLockId) + s.App.ConcentratedLiquidityKeeper.RemovePositionIdForLockId(s.Ctx, positionId, retrievedLockId) // Check if position has lock in state, should not retrievedLockId, err = s.App.ConcentratedLiquidityKeeper.GetLockIdFromPositionId(s.Ctx, positionId) diff --git a/x/concentrated-liquidity/tick.go b/x/concentrated-liquidity/tick.go index c9521ddcb07..badc8f2f363 100644 --- a/x/concentrated-liquidity/tick.go +++ b/x/concentrated-liquidity/tick.go @@ -22,10 +22,11 @@ import ( // The given currentTick value is used to determine the strategy for updating the fee accumulator. // We update the tick's fee growth opposite direction of last traversal accumulator to the fee growth global when tick index is <= current tick. // Otherwise, it is set to zero. +// Note that liquidityDelta can be either positive or negative depending on whether we are adding or removing liquidity. // if we are initializing or updating an upper tick, we subtract the liquidityIn from the LiquidityNet // if we are initializing or updating a lower tick, we add the liquidityIn from the LiquidityNet // WARNING: this method may mutate the pool, make sure to refetch the pool after calling this method. -func (k Keeper) initOrUpdateTick(ctx sdk.Context, poolId uint64, currentTick int64, tickIndex int64, liquidityIn sdk.Dec, upper bool) (err error) { +func (k Keeper) initOrUpdateTick(ctx sdk.Context, poolId uint64, currentTick int64, tickIndex int64, liquidityDelta sdk.Dec, upper bool) (err error) { tickInfo, err := k.GetTickInfo(ctx, poolId, tickIndex) if err != nil { return err @@ -54,15 +55,15 @@ func (k Keeper) initOrUpdateTick(ctx sdk.Context, poolId uint64, currentTick int // note that liquidityIn can be either positive or negative. // If negative, this would work as a subtraction from liquidityBefore - liquidityAfter := math.AddLiquidity(liquidityBefore, liquidityIn) + liquidityAfter := math.AddLiquidity(liquidityBefore, liquidityDelta) tickInfo.LiquidityGross = liquidityAfter // calculate liquidityNet, which we take into account and track depending on whether liquidityIn is positive or negative if upper { - tickInfo.LiquidityNet = tickInfo.LiquidityNet.Sub(liquidityIn) + tickInfo.LiquidityNet = tickInfo.LiquidityNet.Sub(liquidityDelta) } else { - tickInfo.LiquidityNet = tickInfo.LiquidityNet.Add(liquidityIn) + tickInfo.LiquidityNet = tickInfo.LiquidityNet.Add(liquidityDelta) } k.SetTickInfo(ctx, poolId, tickIndex, tickInfo)