update decrease/collect

.forge-snapshots/autocompound_exactUnclaimedFees.snap

@@ -1 +1 @@
-258574
+258575

.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap

@@ -1 +1 @@
-190947
+190948

.forge-snapshots/autocompound_excessFeesCredit.snap

@@ -1 +1 @@
-279113
+279114

.forge-snapshots/decreaseLiquidity_erc20.snap

@@ -1 +1 @@
-133893
+177014

.forge-snapshots/decreaseLiquidity_erc6909.snap

@@ -1 +1 @@
-121836
+177026

.forge-snapshots/increaseLiquidity_erc20.snap

@@ -1 +1 @@
-171338
+171339

.forge-snapshots/increaseLiquidity_erc6909.snap

@@ -1 +1 @@
-146920
+146921

.forge-snapshots/mintWithLiquidity.snap

@@ -1 +1 @@
-466627
+466628

contracts/NonfungiblePositionManager.sol

@@ -86,10 +86,10 @@ 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)
@@ -102,7 +102,8 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
         LiquidityRangeId rangeId = tokenPosition.range.toId();
         Position storage position = positions[msg.sender][rangeId];
         if (0 < position.liquidity) {
-            delta = decreaseLiquidity(tokenId, position.liquidity, hookData, claims);
+            (delta,) = decreaseLiquidity(tokenId, position.liquidity, hookData, claims);
+            (delta) = collect(tokenId, recipient, hookData, claims);
         }
         require(position.tokensOwed0 == 0 && position.tokensOwed1 == 0, "NOT_EMPTY");
         delete positions[msg.sender][rangeId];
@@ -114,7 +115,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/base/BaseLiquidityManagement.sol

@@ -77,7 +77,9 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback {
         if (op == LiquidityOperation.INCREASE) {
             return abi.encode(_increaseLiquidityAndZeroOut(owner, range, liquidityChange, hookData, claims));
         } else if (op == LiquidityOperation.DECREASE) {
-            return abi.encode(_decreaseLiquidityAndZeroOut(owner, range, liquidityChange, hookData, claims));
+            (BalanceDelta callerDelta, BalanceDelta thisDelta) =
+                _decreaseLiquidityAndZeroOut(owner, range, liquidityChange, hookData, claims);
+            return abi.encode(callerDelta, thisDelta);
         } else if (op == LiquidityOperation.COLLECT) {
             return abi.encode(_collectAndZeroOut(owner, range, 0, hookData, claims));
         } else {
@@ -215,8 +217,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback {
         );
     }
 
-    /// @dev Note that we do not update tokensOwed since all fees collected from this modify call are automatically sent to the caller.
-    /// Any outstanding amounts owed to the caller from tokensOwed0/tokensOwed1 must be collected explicitly.
+    /// Any outstanding amounts owed to the caller from the underlying modify call must be collected explicitly with `collect`.
     function _decreaseLiquidity(
         address owner,
         LiquidityRange memory range,
@@ -235,6 +236,20 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback {
         // Account for fees accrued to other users on the same range.
         (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued);
 
+        BalanceDelta tokensOwed;
+
+        // Flush the callerDelta, incrementing the tokensOwed to the user and the amount claimable to this contract.
+        if (callerDelta.amount0() > 0) {
+            (tokensOwed, callerDelta, thisDelta) =
+                _moveCallerDeltaToTokensOwed(true, tokensOwed, callerDelta, thisDelta);
+        }
+
+        if (callerDelta.amount1() > 0) {
+            (tokensOwed, callerDelta, thisDelta) =
+                _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta);
+        }
+
+        position.addTokensOwed(tokensOwed);
         position.subtractLiquidity(liquidityToRemove);
     }
 
@@ -244,9 +259,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback {
         uint256 liquidityToRemove,
         bytes memory hookData,
         bool claims
-    ) internal returns (BalanceDelta callerDelta) {
-        // Todo move to transient storage, and potentially bubble up both deltas
-        BalanceDelta thisDelta;
+    ) internal returns (BalanceDelta callerDelta, BalanceDelta thisDelta) {
         (callerDelta, thisDelta) = _decreaseLiquidity(owner, range, liquidityToRemove, hookData);
         _closeCallerDeltas(callerDelta, range.poolKey.currency0, range.poolKey.currency1, owner, claims);
         _closeThisDeltas(thisDelta, range.poolKey.currency0, range.poolKey.currency1);
@@ -258,10 +271,10 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback {
         uint256 liquidityToRemove,
         bytes memory hookData,
         bool claims
-    ) internal returns (BalanceDelta) {
+    ) internal returns (BalanceDelta, BalanceDelta) {
         return abi.decode(
             manager.unlock(abi.encode(LiquidityOperation.DECREASE, owner, range, liquidityToRemove, hookData, claims)),
-            (BalanceDelta)
+            (BalanceDelta, BalanceDelta)
         );
     }
 
@@ -270,6 +283,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback {
         returns (BalanceDelta callerDelta, BalanceDelta thisDelta)
     {
         // Do not add or decrease liquidity, just trigger fee updates.
+        // TODO: Fails when position is empty
         (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, 0, hookData);
 
         Position storage position = positions[owner][range.toId()];

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

test/position-managers/FeeCollection.t.sol

@@ -216,47 +216,6 @@ 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
     function test_decreaseLiquidity_sameRange_exact() public {
@@ -281,39 +240,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);