[CL Message Audit]: AddToPositions (#5167)

* WIP * Trying to fix test * Fix test * Add min amount * Update proto/osmosis/concentrated-liquidity/tx.proto Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> * Update proto/osmosis/concentrated-liquidity/tx.proto Co-authored-by: Adam Tucker <adam@osmosis.team> * Update proto/osmosis/concentrated-liquidity/tx.proto Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> * Update proto/osmosis/concentrated-liquidity/tx.proto Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> * Update proto/osmosis/concentrated-liquidity/tx.proto Co-authored-by: Adam Tucker <adam@osmosis.team> * WIP for comments * Review comments * updates --------- Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Roman <roman@osmosis.team>
osmosis-labs · May 19, 2023 · ba1660f · ba1660f
1 parent 3834970
commit ba1660f
Show file tree

Hide file tree

Showing 11 changed files with 438 additions and 229 deletions.
diff --git a/proto/osmosis/concentrated-liquidity/tx.proto b/proto/osmosis/concentrated-liquidity/tx.proto
@@ -12,6 +12,11 @@ service Msg {
   rpc CreatePosition(MsgCreatePosition) returns (MsgCreatePositionResponse);
   rpc WithdrawPosition(MsgWithdrawPosition)
       returns (MsgWithdrawPositionResponse);
+  // AddToPosition attempts to add amount0 and amount1 to a position
+  // with the given position id.
+  // To maintain backwards-compatibility with future implementations of
+  // charging, this function deletes the old position and creates a new one with
+  // the resulting amount after addition.
   rpc AddToPosition(MsgAddToPosition) returns (MsgAddToPositionResponse);
   rpc CollectFees(MsgCollectFees) returns (MsgCollectFeesResponse);
   rpc CollectIncentives(MsgCollectIncentives)
@@ -80,12 +85,34 @@ message MsgCreatePositionResponse {
 message MsgAddToPosition {
   uint64 position_id = 1 [ (gogoproto.moretags) = "yaml:\"position_id\"" ];
   string sender = 2 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
-  cosmos.base.v1beta1.Coin token_desired0 = 3 [
-    (gogoproto.moretags) = "yaml:\"token_desired0\"",
+  // amount0 represents the amount of token0 willing to put in.
+  string amount0 = 3 [
+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
+    (gogoproto.moretags) = "yaml:\"amount_0\"",
+    (gogoproto.nullable) = false
+  ];
+  // amount1 represents the amount of token1 willing to put in.
+  string amount1 = 4 [
+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
+    (gogoproto.moretags) = "yaml:\"amount1\"",
     (gogoproto.nullable) = false
   ];
-  cosmos.base.v1beta1.Coin token_desired1 = 4 [
-    (gogoproto.moretags) = "yaml:\"token_desired1\"",
+  // token_min_amount0 represents the minimum amount of token0 desired from the
+  // new position being created. Note that this field indicates the min amount0
+  // corresponding to the total liquidity of the position, not just the
+  // liquidity that is being added.
+  string token_min_amount0 = 5 [
+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
+    (gogoproto.moretags) = "yaml:\"token_min_amount0\"",
+    (gogoproto.nullable) = false
+  ];
+  // token_min_amount1 represents the minimum amount of token1 desired from the
+  // new position being created. Note that this field indicates the min amount1
+  // corresponding to the total liquidity of the position, not just the
+  // liquidity that is being added.
+  string token_min_amount1 = 6 [
+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
+    (gogoproto.moretags) = "yaml:\"token_min_amount1\"",
     (gogoproto.nullable) = false
   ];
 }

diff --git a/x/concentrated-liquidity/export_test.go b/x/concentrated-liquidity/export_test.go
@@ -218,8 +218,8 @@ func (k Keeper) CreatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr
 	return k.createPosition(ctx, poolId, owner, tokensProvided, amount0Min, amount1Min, lowerTick, upperTick)
 }
 
-func (k Keeper) AddToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId uint64, amount0Added, amount1Added sdk.Int) (uint64, sdk.Int, sdk.Int, error) {
-	return k.addToPosition(ctx, owner, positionId, amount0Added, amount1Added)
+func (k Keeper) AddToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId uint64, amount0Added, amount1Added, amount0Min, amount1Min sdk.Int) (uint64, sdk.Int, sdk.Int, error) {
+	return k.addToPosition(ctx, owner, positionId, amount0Added, amount1Added, amount0Min, amount1Min)
 }
 
 func (ss *SwapState) UpdateFeeGrowthGlobal(feeChargeTotal sdk.Dec) {

diff --git a/x/concentrated-liquidity/lp.go b/x/concentrated-liquidity/lp.go
@@ -252,13 +252,16 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position
 // For the sake of backwards-compatibility with future implementations of charging, this function deletes the old position and creates
 // a new one with the resulting amount after addition. Note that due to truncation after `withdrawPosition`, there is some rounding error
 // that is upper bounded by 1 unit of the more valuable token.
+// Uses the amount0MinGiven,amount1MinGiven as the minimum token out for creating the new position.
+// Note that these field indicates the min amount corresponding to the total liquidity of the position,
+// not only for the liquidity amount that is being added.
+// Uses amounts withdrawn from the original position if provided min amount is zero.
 // Returns error if
 // - Withdrawing full position fails
 // - Creating new position with added liquidity fails
 // - Position with `positionId` is the last position in the pool
 // - Position is superfluid staked
-// TODO: handle adding to SFS positions
-func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId uint64, amount0Added, amount1Added sdk.Int) (uint64, sdk.Int, sdk.Int, error) {
+func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId uint64, amount0Added, amount1Added, amount0MinGiven, amount1MinGiven sdk.Int) (uint64, sdk.Int, sdk.Int, error) {
 	position, err := k.GetPosition(ctx, positionId)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, err
@@ -269,10 +272,15 @@ func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId
 		return 0, sdk.Int{}, sdk.Int{}, types.NotPositionOwnerError{PositionId: positionId, Address: owner.String()}
 	}
 
+	// if one of the liquidity is negative, or both liquidity being added is zero, error
 	if amount0Added.IsNegative() || amount1Added.IsNegative() {
 		return 0, sdk.Int{}, sdk.Int{}, types.NegativeAmountAddedError{PositionId: position.PositionId, Asset0Amount: amount0Added, Asset1Amount: amount1Added}
 	}
 
+	if amount0Added.IsZero() && amount1Added.IsZero() {
+		return 0, sdk.Int{}, sdk.Int{}, types.ErrZeroLiquidity
+	}
+
 	// If the position is superfluid staked, return error.
 	// This path is handled separately in the superfluid module.
 	positionHasUnderlyingLock, _, err := k.positionHasActiveUnderlyingLockAndUpdate(ctx, positionId)
@@ -306,7 +314,16 @@ func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId
 		return 0, sdk.Int{}, sdk.Int{}, err
 	}
 	tokensProvided := sdk.NewCoins(sdk.NewCoin(pool.GetToken0(), amount0Desired), sdk.NewCoin(pool.GetToken1(), amount1Desired))
-	newPositionId, actualAmount0, actualAmount1, _, _, _, _, err := k.createPosition(ctx, position.PoolId, owner, tokensProvided, amount0Withdrawn, amount1Withdrawn, position.LowerTick, position.UpperTick)
+	minimumAmount0 := amount0Withdrawn
+	minimumAmount1 := amount1Withdrawn
+
+	if !amount0MinGiven.IsZero() {
+		minimumAmount0 = amount0MinGiven
+	}
+	if !amount1MinGiven.IsZero() {
+		minimumAmount1 = amount1MinGiven
+	}
+	newPositionId, actualAmount0, actualAmount1, _, _, _, _, err := k.createPosition(ctx, position.PoolId, owner, tokensProvided, minimumAmount0, minimumAmount1, position.LowerTick, position.UpperTick)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, err
 	}