chore: remove double call to modifyLiquidity when user add/remove liq…

…uidity (#35)

* chore: remove double call to modifyLiquidity

* doc: better comment on why delta = delta - feeDelta

.forge-snapshots/BinFungibleTokenTest#testBatchTransferFrom_FromBob.snap

@@ -1 +1 @@
-54579
+54723

.forge-snapshots/BinFungibleTokenTest#testBurn.snap

@@ -1 +1 @@
-26887
+26896

.forge-snapshots/BinFungibleTokenTest#testMint.snap

@@ -1 +1 @@
-67685
+67373

.forge-snapshots/NonfungiblePositionManager#collect.snap

@@ -1 +1 @@
-265629
+265663

.forge-snapshots/NonfungiblePositionManager#decreaseLiquidity.snap

@@ -1 +1 @@
-176568
+161440

.forge-snapshots/NonfungiblePositionManager#increaseLiquidity.snap

@@ -1 +1 @@
-210284
+195159

.forge-snapshots/NonfungiblePositionManager#mint.snap

@@ -1 +1 @@
-609324
+607379

src/pool-cl/NonfungiblePositionManager.sol

@@ -396,7 +396,7 @@ contract NonfungiblePositionManager is
         uint128 tokensOwed1 = nftPositionCache.tokensOwed1;
 
         if (nftPositionCache.liquidity > 0) {
-            resetAccumulatedFee(poolKey, nftPositionCache.tickLower, nftPositionCache.tickUpper);
+            mintAccumulatedPositionFee(poolKey, nftPositionCache.tickLower, nftPositionCache.tickUpper);
 
             // todo: about the salt
             CLPosition.Info memory poolManagerPositionInfo = poolManager.getPosition(

src/pool-cl/base/LiquidityManagement.sol

@@ -44,45 +44,52 @@ abstract contract LiquidityManagement is CLPeripheryImmutableState, PeripheryPay
         uint256 amount1Min;
     }
 
-    /// @dev Since in v4 `modifyLiquidity` accumulated fee are claimed and
-    // resynced by default, which can mixup with user's actual settlement
-    // for update liquidity, we claim the fee before further action to avoid this.
-    function resetAccumulatedFee(PoolKey memory poolKey, int24 tickLower, int24 tickUpper) internal {
+    /// @notice Claim accumulated fees from the position and mint them to the NFP contract
+    function mintAccumulatedPositionFee(PoolKey memory poolKey, int24 tickLower, int24 tickUpper) internal {
         CLPosition.Info memory poolManagerPositionInfo =
             poolManager.getPosition(poolKey.toId(), address(this), tickLower, tickUpper, SALT_0);
 
         if (poolManagerPositionInfo.liquidity > 0) {
-            // todo: can we avoid this resetAccumulatedFee call?
             (, BalanceDelta feeDelta) = poolManager.modifyLiquidity(
                 poolKey, ICLPoolManager.ModifyLiquidityParams(tickLower, tickUpper, 0, SALT_0), ""
             );
 
-            if (feeDelta.amount0() > 0) {
-                vault.mint(address(this), poolKey.currency0, uint256(int256(feeDelta.amount0())));
-            }
+            mintFeeDelta(poolKey, feeDelta);
+        }
+    }
 
-            if (feeDelta.amount1() > 0) {
-                vault.mint(address(this), poolKey.currency1, uint256(int256(feeDelta.amount1())));
-            }
+    /// @dev Mint accumulated fee to the contract so user can perform collect() at a later stage
+    function mintFeeDelta(PoolKey memory poolKey, BalanceDelta feeDelta) internal {
+        if (feeDelta.amount0() > 0) {
+            vault.mint(address(this), poolKey.currency0, uint256(int256(feeDelta.amount0())));
+        }
+
+        if (feeDelta.amount1() > 0) {
+            vault.mint(address(this), poolKey.currency1, uint256(int256(feeDelta.amount1())));
         }
     }
 
+    /// @return liquidity The amount of liquidity added to the position
+    /// @return delta The amount of token0 and token1 from liquidity additional. Does not include the fee accumulated in the position.
     function addLiquidity(AddLiquidityParams memory params) internal returns (uint128 liquidity, BalanceDelta delta) {
-        resetAccumulatedFee(params.poolKey, params.tickLower, params.tickUpper);
-
         (uint160 sqrtPriceX96,,,) = poolManager.getSlot0(params.poolKey.toId());
         uint160 sqrtRatioAX96 = TickMath.getSqrtRatioAtTick(params.tickLower);
         uint160 sqrtRatioBX96 = TickMath.getSqrtRatioAtTick(params.tickUpper);
         liquidity = LiquidityAmounts.getLiquidityForAmounts(
             sqrtPriceX96, sqrtRatioAX96, sqrtRatioBX96, params.amount0Desired, params.amount1Desired
         );
 
-        (delta,) = poolManager.modifyLiquidity(
+        BalanceDelta feeDelta;
+        (delta, feeDelta) = poolManager.modifyLiquidity(
             params.poolKey,
             ICLPoolManager.ModifyLiquidityParams(params.tickLower, params.tickUpper, int256(uint256(liquidity)), SALT_0),
             ""
         );
 
+        /// @dev `delta` return value of modifyLiquidity is inclusive of fee. Mint the `feeDelta` to nfp contract so subtract from `delta`
+        delta = delta - feeDelta;
+        mintFeeDelta(params.poolKey, feeDelta);
+
         /// @dev amount0 & amount1 cant be positive here since LPing has been claimed
         if (
             uint256(uint128(-delta.amount0())) < params.amount0Min
@@ -92,17 +99,21 @@ abstract contract LiquidityManagement is CLPeripheryImmutableState, PeripheryPay
         }
     }
 
+    /// @return delta The amount of token0 and token1 from liquidity removal. Does not include the fee accumulated in the position.
     function removeLiquidity(RemoveLiquidityParams memory params) internal returns (BalanceDelta delta) {
-        resetAccumulatedFee(params.poolKey, params.tickLower, params.tickUpper);
-
-        (delta,) = poolManager.modifyLiquidity(
+        BalanceDelta feeDelta;
+        (delta, feeDelta) = poolManager.modifyLiquidity(
             params.poolKey,
             ICLPoolManager.ModifyLiquidityParams(
                 params.tickLower, params.tickUpper, -int256(uint256(params.liquidity)), SALT_0
             ),
             ""
         );
 
+        /// @dev `delta` return value of modifyLiquidity is inclusive of fee. Mint the `feeDelta` to nfp contract so subtract from `delta`
+        delta = delta - feeDelta;
+        mintFeeDelta(params.poolKey, feeDelta);
+
         /// @dev amount0 & amount1 must be positive here since LPing has been claimed
         if (
             uint256(uint128(delta.amount0())) < params.amount0Min