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 5607964841b..35703d508f0 100644 --- a/x/concentrated-liquidity/export_test.go +++ b/x/concentrated-liquidity/export_test.go @@ -172,8 +172,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) (uint64, sdk.Int, sdk.Int, sdk.Dec, time.Time, error) { @@ -246,8 +250,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 42a2c61ffaa..52b038c7e2f 100644 --- a/x/concentrated-liquidity/fees_test.go +++ b/x/concentrated-liquidity/fees_test.go @@ -1336,7 +1336,7 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition_UpdatingPositio } } -func (s *KeeperTestSuite) TestPreparePositionAccumulator() { +func (s *KeeperTestSuite) TestUpdatePosValueToInitValuePlusGrowthOutside() { validPositionKey := types.KeyFeePositionAccumulator(1) invalidPositionKey := types.KeyFeePositionAccumulator(2) tests := []struct { @@ -1383,7 +1383,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 f39ef5acd0a..0ab11abd681 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 9ba93126a48..d94a70725f6 100644 --- a/x/concentrated-liquidity/lp.go +++ b/x/concentrated-liquidity/lp.go @@ -121,12 +121,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 @@ -144,14 +145,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} } @@ -161,13 +168,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 } @@ -179,8 +181,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. @@ -202,7 +204,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 } @@ -337,12 +339,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 @@ -439,7 +442,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 373a7976e63..e48982b6c98 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" ) @@ -453,6 +453,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{ @@ -461,7 +469,6 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { timeElapsed: defaultTimeElapsed, withdrawWithNonOwner: true, }, - // TODO: test with custom amounts that potentially lead to truncations. } for name, tc := range tests { @@ -510,6 +517,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)) @@ -541,10 +549,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())) @@ -571,32 +579,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 { @@ -605,15 +605,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 17d85eee922..10b446a8733 100644 --- a/x/concentrated-liquidity/math/math.go +++ b/x/concentrated-liquidity/math/math.go @@ -185,6 +185,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 81b471c440f..d0a1231b8d4 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) @@ -664,8 +664,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. @@ -683,10 +683,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. @@ -713,10 +711,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 06d3e683b22..1488378ac21 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/x/concentrated-liquidity/position_test.go @@ -1560,13 +1560,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{ @@ -1615,6 +1616,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 { @@ -1672,15 +1685,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{ @@ -1735,6 +1749,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 { @@ -1751,7 +1779,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) @@ -1837,7 +1869,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 09a62ec34f8..2955cbf387f 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)