From ff0e3779bf835f989f09dd22ea9bb306756e0ec4 Mon Sep 17 00:00:00 2001 From: ChefMist <133624774+ChefMist@users.noreply.github.com> Date: Fri, 7 Jun 2024 10:04:07 +0800 Subject: [PATCH 1/2] chore: remove double call to modifyLiquidity --- ...kenTest#testBatchTransferFrom_FromBob.snap | 2 +- .../BinFungibleTokenTest#testBurn.snap | 2 +- .../BinFungibleTokenTest#testMint.snap | 2 +- .../NonfungiblePositionManager#collect.snap | 2 +- ...iblePositionManager#decreaseLiquidity.snap | 2 +- ...iblePositionManager#increaseLiquidity.snap | 2 +- .../NonfungiblePositionManager#mint.snap | 2 +- src/pool-cl/NonfungiblePositionManager.sol | 2 +- src/pool-cl/base/LiquidityManagement.sol | 45 ++++++++++++------- 9 files changed, 36 insertions(+), 25 deletions(-) diff --git a/.forge-snapshots/BinFungibleTokenTest#testBatchTransferFrom_FromBob.snap b/.forge-snapshots/BinFungibleTokenTest#testBatchTransferFrom_FromBob.snap index d99eabc..0ef3935 100644 --- a/.forge-snapshots/BinFungibleTokenTest#testBatchTransferFrom_FromBob.snap +++ b/.forge-snapshots/BinFungibleTokenTest#testBatchTransferFrom_FromBob.snap @@ -1 +1 @@ -54579 \ No newline at end of file +54723 \ No newline at end of file diff --git a/.forge-snapshots/BinFungibleTokenTest#testBurn.snap b/.forge-snapshots/BinFungibleTokenTest#testBurn.snap index 9ba56dd..d7585d6 100644 --- a/.forge-snapshots/BinFungibleTokenTest#testBurn.snap +++ b/.forge-snapshots/BinFungibleTokenTest#testBurn.snap @@ -1 +1 @@ -26887 \ No newline at end of file +26896 \ No newline at end of file diff --git a/.forge-snapshots/BinFungibleTokenTest#testMint.snap b/.forge-snapshots/BinFungibleTokenTest#testMint.snap index 9b41b12..4b59870 100644 --- a/.forge-snapshots/BinFungibleTokenTest#testMint.snap +++ b/.forge-snapshots/BinFungibleTokenTest#testMint.snap @@ -1 +1 @@ -67685 \ No newline at end of file +67373 \ No newline at end of file diff --git a/.forge-snapshots/NonfungiblePositionManager#collect.snap b/.forge-snapshots/NonfungiblePositionManager#collect.snap index 091b6c2..e2d4db9 100644 --- a/.forge-snapshots/NonfungiblePositionManager#collect.snap +++ b/.forge-snapshots/NonfungiblePositionManager#collect.snap @@ -1 +1 @@ -265629 \ No newline at end of file +265663 \ No newline at end of file diff --git a/.forge-snapshots/NonfungiblePositionManager#decreaseLiquidity.snap b/.forge-snapshots/NonfungiblePositionManager#decreaseLiquidity.snap index e30fc66..5e420ab 100644 --- a/.forge-snapshots/NonfungiblePositionManager#decreaseLiquidity.snap +++ b/.forge-snapshots/NonfungiblePositionManager#decreaseLiquidity.snap @@ -1 +1 @@ -176568 \ No newline at end of file +161440 \ No newline at end of file diff --git a/.forge-snapshots/NonfungiblePositionManager#increaseLiquidity.snap b/.forge-snapshots/NonfungiblePositionManager#increaseLiquidity.snap index 036527e..11769d0 100644 --- a/.forge-snapshots/NonfungiblePositionManager#increaseLiquidity.snap +++ b/.forge-snapshots/NonfungiblePositionManager#increaseLiquidity.snap @@ -1 +1 @@ -210284 \ No newline at end of file +195159 \ No newline at end of file diff --git a/.forge-snapshots/NonfungiblePositionManager#mint.snap b/.forge-snapshots/NonfungiblePositionManager#mint.snap index 8fdf5cf..c427d27 100644 --- a/.forge-snapshots/NonfungiblePositionManager#mint.snap +++ b/.forge-snapshots/NonfungiblePositionManager#mint.snap @@ -1 +1 @@ -609324 \ No newline at end of file +607379 \ No newline at end of file diff --git a/src/pool-cl/NonfungiblePositionManager.sol b/src/pool-cl/NonfungiblePositionManager.sol index f1a5c58..ba3406f 100644 --- a/src/pool-cl/NonfungiblePositionManager.sol +++ b/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( diff --git a/src/pool-cl/base/LiquidityManagement.sol b/src/pool-cl/base/LiquidityManagement.sol index 086e504..2fed6c6 100644 --- a/src/pool-cl/base/LiquidityManagement.sol +++ b/src/pool-cl/base/LiquidityManagement.sol @@ -44,32 +44,34 @@ 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); @@ -77,12 +79,17 @@ abstract contract LiquidityManagement is CLPeripheryImmutableState, PeripheryPay 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, we mint the fee to nfp contract so we need to subtract it 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,10 +99,10 @@ 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 @@ -103,6 +110,10 @@ abstract contract LiquidityManagement is CLPeripheryImmutableState, PeripheryPay "" ); + /// @dev first return value of modifyLiquidity is inclusive of fee, we mint the fee to nfp contract so we need to subtract it 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 From 0835a9664dec1f87cae11935b9b0990db43b25dd Mon Sep 17 00:00:00 2001 From: ChefMist <133624774+ChefMist@users.noreply.github.com> Date: Fri, 7 Jun 2024 10:09:12 +0800 Subject: [PATCH 2/2] doc: better comment on why delta = delta - feeDelta --- src/pool-cl/base/LiquidityManagement.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pool-cl/base/LiquidityManagement.sol b/src/pool-cl/base/LiquidityManagement.sol index 2fed6c6..a1c8223 100644 --- a/src/pool-cl/base/LiquidityManagement.sol +++ b/src/pool-cl/base/LiquidityManagement.sol @@ -86,7 +86,7 @@ abstract contract LiquidityManagement is CLPeripheryImmutableState, PeripheryPay "" ); - /// @dev `delta` return value of modifyLiquidity is inclusive of fee, we mint the fee to nfp contract so we need to subtract it from delta + /// @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); @@ -110,7 +110,7 @@ abstract contract LiquidityManagement is CLPeripheryImmutableState, PeripheryPay "" ); - /// @dev first return value of modifyLiquidity is inclusive of fee, we mint the fee to nfp contract so we need to subtract it from delta + /// @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);