update decrease (#133)

* update decrease

* update collect

* update decrease/collect

* remove delta function

* update burn

.forge-snapshots/autocompound_exactUnclaimedFees.snap

@@ -1 +1 @@
-258477
+258575

.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap

@@ -1 +1 @@
-190850
+190948

.forge-snapshots/autocompound_excessFeesCredit.snap

@@ -1 +1 @@
-279016
+279114

.forge-snapshots/decreaseLiquidity_erc20.snap

@@ -1 +1 @@
-190026
+177014

.forge-snapshots/decreaseLiquidity_erc6909.snap

@@ -1 +1 @@
-168894
+177026

.forge-snapshots/increaseLiquidity_erc20.snap

@@ -1 +1 @@
-171241
+171339

.forge-snapshots/increaseLiquidity_erc6909.snap

@@ -1 +1 @@
-146823
+146921

.forge-snapshots/mintWithLiquidity.snap

@@ -1 +1 @@
-466530
+466628

contracts/NonfungiblePositionManager.sol

@@ -86,24 +86,28 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
     function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims)
         public
         isAuthorizedForToken(tokenId)
-        returns (BalanceDelta delta)
+        returns (BalanceDelta delta, BalanceDelta thisDelta)
     {
         TokenPosition memory tokenPos = tokenPositions[tokenId];
-        delta = _lockAndDecreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData, claims);
+        (delta, thisDelta) = _lockAndDecreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData, claims);
     }
 
     function burn(uint256 tokenId, address recipient, bytes calldata hookData, bool claims)
         external
         isAuthorizedForToken(tokenId)
         returns (BalanceDelta delta)
     {
+        // TODO: Burn currently decreases and collects. However its done under different locks.
+        // Replace once we have the execute multicall.
         // remove liquidity
         TokenPosition storage tokenPosition = tokenPositions[tokenId];
         LiquidityRangeId rangeId = tokenPosition.range.toId();
         Position storage position = positions[msg.sender][rangeId];
-        if (0 < position.liquidity) {
-            delta = decreaseLiquidity(tokenId, position.liquidity, hookData, claims);
+        if (position.liquidity > 0) {
+            (delta,) = decreaseLiquidity(tokenId, position.liquidity, hookData, claims);
         }
+
+        collect(tokenId, recipient, hookData, claims);
         require(position.tokensOwed0 == 0 && position.tokensOwed1 == 0, "NOT_EMPTY");
         delete positions[msg.sender][rangeId];
         delete tokenPositions[tokenId];
@@ -114,7 +118,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
 
     // TODO: in v3, we can partially collect fees, but what was the usecase here?
     function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims)
-        external
+        public
         returns (BalanceDelta delta)
     {
         TokenPosition memory tokenPos = tokenPositions[tokenId];

contracts/interfaces/INonfungiblePositionManager.sol

@@ -37,10 +37,11 @@ interface INonfungiblePositionManager {
     /// @param liquidity The amount of liquidity to remove
     /// @param hookData Arbitrary data passed to the hook
     /// @param claims Whether the removed liquidity is sent as ERC-6909 claim tokens
-    /// @return delta Corresponding balance changes as a result of decreasing liquidity
+    /// @return delta Corresponding balance changes as a result of decreasing liquidity applied to user
+    /// @return thisDelta Corresponding balance changes as a result of decreasing liquidity applied to lpm
     function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims)
         external
-        returns (BalanceDelta delta);
+        returns (BalanceDelta delta, BalanceDelta thisDelta);
 
     /// @notice Burn a position and delete the tokenId
     /// @dev It removes liquidity and collects fees if the position is not empty

contracts/libraries/LiquidityDeltaAccounting.sol

@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.20;
+
+import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
+
+import "forge-std/console2.sol";
+
+library LiquidityDeltaAccounting {
+    function split(BalanceDelta liquidityDelta, BalanceDelta callerFeesAccrued, BalanceDelta totalFeesAccrued)
+        internal
+        returns (BalanceDelta callerDelta, BalanceDelta thisDelta)
+    {
+        if (totalFeesAccrued == callerFeesAccrued) {
+            // when totalFeesAccrued == callerFeesAccrued, the caller is not sharing the range
+            // therefore, the caller is responsible for the entire liquidityDelta
+            callerDelta = liquidityDelta;
+        } else {
+            // the delta for increasing liquidity assuming that totalFeesAccrued was not applied
+            BalanceDelta principalDelta = liquidityDelta - totalFeesAccrued;
+
+            // outstanding deltas the caller is responsible for, after their fees are credited to the principal delta
+            callerDelta = principalDelta + callerFeesAccrued;
+
+            // outstanding deltas this contract is responsible for, intuitively the contract is responsible for taking fees external to the caller's accrued fees
+            thisDelta = totalFeesAccrued - callerFeesAccrued;
+        }
+    }
+}

contracts/libraries/Position.sol

@@ -6,18 +6,32 @@ import {BalanceDelta} from "v4-core/types/BalanceDelta.sol";
 
 // Updates Position storage
 library PositionLibrary {
+    error InsufficientLiquidity();
+
     // TODO ensure this is one sstore.
     function addTokensOwed(IBaseLiquidityManagement.Position storage position, BalanceDelta tokensOwed) internal {
         position.tokensOwed0 += uint128(tokensOwed.amount0());
         position.tokensOwed1 += uint128(tokensOwed.amount1());
     }
 
+    function clearTokensOwed(IBaseLiquidityManagement.Position storage position) internal {
+        position.tokensOwed0 = 0;
+        position.tokensOwed1 = 0;
+    }
+
     function addLiquidity(IBaseLiquidityManagement.Position storage position, uint256 liquidity) internal {
         unchecked {
             position.liquidity += liquidity;
         }
     }
 
+    function subtractLiquidity(IBaseLiquidityManagement.Position storage position, uint256 liquidity) internal {
+        if (position.liquidity < liquidity) revert InsufficientLiquidity();
+        unchecked {
+            position.liquidity -= liquidity;
+        }
+    }
+
     // TODO ensure this is one sstore.
     function updateFeeGrowthInside(
         IBaseLiquidityManagement.Position storage position,

test/position-managers/FeeCollection.t.sol

@@ -216,49 +216,10 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers {
     function test_collect_donate() public {}
     function test_collect_donate_sameRange() public {}
 
-    function test_decreaseLiquidity_sameRange(
-        IPoolManager.ModifyLiquidityParams memory params,
-        uint256 liquidityDeltaBob
-    ) public {
-        uint256 tokenIdAlice;
-        uint256 tokenIdBob;
-        params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18);
-        params = createFuzzyLiquidityParams(key, params, SQRT_PRICE_1_1);
-        vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity
-
-        liquidityDeltaBob = bound(liquidityDeltaBob, 100e18, 100_000e18);
-
-        LiquidityRange memory range =
-            LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper});
-        vm.prank(alice);
-        (tokenIdAlice,) = lpm.mint(range, uint256(params.liquidityDelta), block.timestamp + 1, alice, ZERO_BYTES);
-
-        vm.prank(bob);
-        (tokenIdBob,) = lpm.mint(range, liquidityDeltaBob, block.timestamp + 1, bob, ZERO_BYTES);
-
-        // swap to create fees
-        uint256 swapAmount = 0.001e18;
-        swap(key, true, -int256(swapAmount), ZERO_BYTES);
-
-        // alice removes all of her liquidity
-        vm.prank(alice);
-        BalanceDelta aliceDelta = lpm.decreaseLiquidity(tokenIdAlice, uint256(params.liquidityDelta), ZERO_BYTES, true);
-        assertEq(uint256(uint128(aliceDelta.amount0())), manager.balanceOf(alice, currency0.toId()));
-        assertEq(uint256(uint128(aliceDelta.amount1())), manager.balanceOf(alice, currency1.toId()));
-
-        // bob removes half of his liquidity
-        vm.prank(bob);
-        BalanceDelta bobDelta = lpm.decreaseLiquidity(tokenIdBob, liquidityDeltaBob / 2, ZERO_BYTES, true);
-        assertEq(uint256(uint128(bobDelta.amount0())), manager.balanceOf(bob, currency0.toId()));
-        assertEq(uint256(uint128(bobDelta.amount1())), manager.balanceOf(bob, currency1.toId()));
-
-        // position manager holds no fees now
-        assertApproxEqAbs(manager.balanceOf(address(lpm), currency0.toId()), 0, 1 wei);
-        assertApproxEqAbs(manager.balanceOf(address(lpm), currency1.toId()), 0, 1 wei);
-    }
-
     /// @dev Alice and bob create liquidity on the same range
     ///     when alice decreases liquidity, she should only collect her fees
+    /// TODO Add back fuzz test on liquidityDeltaBob
+    /// TODO Assert state changes for lpm balance, position state, and return values
     function test_decreaseLiquidity_sameRange_exact() public {
         // alice and bob create liquidity on the same range [-120, 120]
         LiquidityRange memory range = LiquidityRange({poolKey: key, tickLower: -120, tickUpper: 120});
@@ -281,39 +242,38 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers {
 
         // alice decreases liquidity
         vm.prank(alice);
-        BalanceDelta aliceDelta = lpm.decreaseLiquidity(tokenIdAlice, liquidityAlice, ZERO_BYTES, true);
+        BalanceDelta aliceDelta;
+        BalanceDelta thisDelta;
+        (aliceDelta, thisDelta) = lpm.decreaseLiquidity(tokenIdAlice, liquidityAlice, ZERO_BYTES, true);
 
         uint256 tolerance = 0.000000001 ether;
 
-        // alice claims original principal + her fees
+        uint256 lpmBalance0 = manager.balanceOf(address(lpm), currency0.toId());
+        uint256 lpmBalance1 = manager.balanceOf(address(lpm), currency1.toId());
+
+        // lpm collects alice's principal + all fees accrued on the range
         assertApproxEqAbs(
-            manager.balanceOf(alice, currency0.toId()),
-            uint256(int256(-lpDeltaAlice.amount0()))
-                + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityAlice, liquidityAlice + liquidityBob),
-            tolerance
+            lpmBalance0, uint256(int256(-lpDeltaAlice.amount0())) + swapAmount.mulWadDown(FEE_WAD), tolerance
         );
         assertApproxEqAbs(
-            manager.balanceOf(alice, currency1.toId()),
-            uint256(int256(-lpDeltaAlice.amount1()))
-                + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityAlice, liquidityAlice + liquidityBob),
-            tolerance
+            lpmBalance1, uint256(int256(-lpDeltaAlice.amount1())) + swapAmount.mulWadDown(FEE_WAD), tolerance
         );
 
         // bob decreases half of his liquidity
         vm.prank(bob);
-        BalanceDelta bobDelta = lpm.decreaseLiquidity(tokenIdBob, liquidityBob / 2, ZERO_BYTES, true);
+        BalanceDelta bobDelta;
+        (bobDelta, thisDelta) = lpm.decreaseLiquidity(tokenIdBob, liquidityBob / 2, ZERO_BYTES, true);
 
-        // bob claims half of the original principal + his fees
+        // lpm collects half of bobs principal
+        // the fee amount has already been collected with alice's calls
         assertApproxEqAbs(
-            manager.balanceOf(bob, currency0.toId()),
-            uint256(int256(-lpDeltaBob.amount0()) / 2)
-                + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityBob, liquidityAlice + liquidityBob),
+            manager.balanceOf(address(lpm), currency0.toId()) - lpmBalance0,
+            uint256(int256(-lpDeltaBob.amount0()) / 2),
             tolerance
         );
         assertApproxEqAbs(
-            manager.balanceOf(bob, currency1.toId()),
-            uint256(int256(-lpDeltaBob.amount1()) / 2)
-                + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityBob, liquidityAlice + liquidityBob),
+            manager.balanceOf(address(lpm), currency1.toId()) - lpmBalance1,
+            uint256(int256(-lpDeltaBob.amount1()) / 2),
             tolerance
         );
     }

test/position-managers/NonfungiblePositionManager.t.sol

@@ -247,7 +247,8 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi
 
         uint256 balance0Before = currency0.balanceOfSelf();
         uint256 balance1Before = currency1.balanceOfSelf();
-        BalanceDelta delta = lpm.decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false);
+        (BalanceDelta delta, BalanceDelta thisDelta) =
+            lpm.decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false);
 
         (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
         assertEq(liquidity, uint256(params.liquidityDelta) - decreaseLiquidityDelta);