consolidate using owner, update burn

.forge-snapshots/autocompound_exactUnclaimedFees.snap

@@ -1 +1 @@
-299933
+299999

.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap

@@ -1 +1 @@
-239395
+239461

.forge-snapshots/autocompound_excessFeesCredit.snap

@@ -1 +1 @@
-320472
+320538

.forge-snapshots/decreaseLiquidity_erc20.snap

@@ -1 +1 @@
-215156
+215178

.forge-snapshots/decreaseLiquidity_erc6909.snap

@@ -1 +1 @@
-215168
+215190

.forge-snapshots/increaseLiquidity_erc20.snap

@@ -1 +1 @@
-194836
+194902

.forge-snapshots/increaseLiquidity_erc6909.snap

@@ -1 +1 @@
-194848
+194914

.forge-snapshots/mintWithLiquidity.snap

@@ -1 +1 @@
-511680
+511702

contracts/NonfungiblePositionManager.sol

@@ -65,6 +65,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
             currencies[i].close(manager, address(this));
         }
 
+        // Should just be returning the netted amount that was settled on behalf of the caller (msgSender)
+        // And any recipient deltas settled earlier.
+
         // TODO: @sara handle the return
         // vanilla: return int128[2]
         // batch: return int128[data.length]
@@ -77,21 +80,21 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
         LiquidityRange calldata range,
         uint256 liquidity,
         uint256 deadline,
-        address recipient,
+        address owner,
         bytes calldata hookData
     ) public payable returns (BalanceDelta delta) {
         // TODO: optimization, read/write manager.isUnlocked to avoid repeated external calls for batched execution
         if (manager.isUnlocked()) {
-            _increaseLiquidity(recipient, range, liquidity, hookData);
+            _increaseLiquidity(owner, range, liquidity, hookData);
 
             // mint receipt token
             uint256 tokenId;
-            _mint(recipient, (tokenId = nextTokenId++));
-            tokenPositions[tokenId] = TokenPosition({owner: recipient, range: range});
+            _mint(owner, (tokenId = nextTokenId++));
+            tokenPositions[tokenId] = TokenPosition({owner: owner, range: range});
         } else {
             msgSender = msg.sender;
             bytes[] memory data = new bytes[](1);
-            data[0] = abi.encodeWithSelector(this.mint.selector, range, liquidity, deadline, recipient, hookData);
+            data[0] = abi.encodeWithSelector(this.mint.selector, range, liquidity, deadline, owner, hookData);
 
             Currency[] memory currencies = new Currency[](2);
             currencies[0] = range.poolKey.currency0;
@@ -156,7 +159,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
             msgSender = msg.sender;
             bytes[] memory data = new bytes[](1);
             data[0] = abi.encodeWithSelector(this.decreaseLiquidity.selector, tokenId, liquidity, hookData, claims);
-            
+
             Currency[] memory currencies = new Currency[](2);
             currencies[0] = tokenPos.range.poolKey.currency0;
             currencies[1] = tokenPos.range.poolKey.currency1;
@@ -165,27 +168,15 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
         }
     }
 
-    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 (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];
+    // TODO return type?
+    function burn(uint256 tokenId) public isAuthorizedForToken(tokenId) returns (BalanceDelta delta) {
+        // TODO: Burn currently requires a decrease and collect call before the token can be deleted. Possible to combine.
+        // We do not need to enforce the pool manager to be unlocked bc this function is purely clearing storage for the minted tokenId.
+        TokenPosition memory tokenPos = tokenPositions[tokenId];
+        // Checks that the full position's liquidity has been removed and all tokens have been collected from tokensOwed.
+        _validateBurn(tokenPos.owner, tokenPos.range);
         delete tokenPositions[tokenId];
-
-        // burn the token
+        // Burn the token.
         _burn(tokenId);
     }
 

contracts/base/BaseLiquidityManagement.sol

@@ -201,6 +201,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb
         position.subtractLiquidity(liquidityToRemove);
     }
 
+    // The recipient may not be the original owner.
     function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal {
         BalanceDelta callerDelta;
         BalanceDelta thisDelta;
@@ -234,6 +235,14 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb
         position.clearTokensOwed();
     }
 
+    function _validateBurn(address owner, LiquidityRange memory range) internal {
+        LiquidityRangeId rangeId = range.toId();
+        Position storage position = positions[owner][rangeId];
+        if (position.liquidity > 0) revert PositionMustBeEmpty();
+        if (position.tokensOwed0 != 0 && position.tokensOwed1 != 0) revert TokensMustBeCollected();
+        delete positions[owner][rangeId];
+    }
+
     function _updateFeeGrowth(LiquidityRange memory range, Position storage position)
         internal
         returns (BalanceDelta callerFeesAccrued)

contracts/interfaces/IBaseLiquidityManagement.sol

@@ -6,6 +6,9 @@ import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
 import {LiquidityRange, LiquidityRangeId} from "../types/LiquidityRange.sol";
 
 interface IBaseLiquidityManagement {
+    error PositionMustBeEmpty();
+    error TokensMustBeCollected();
+
     // details about the liquidity position
     struct Position {
         // the nonce for permits

contracts/interfaces/INonfungiblePositionManager.sol

@@ -43,16 +43,12 @@ interface INonfungiblePositionManager {
         external
         returns (BalanceDelta delta);
 
+    // TODO Can decide if we want burn to auto encode a decrease/collect.
     /// @notice Burn a position and delete the tokenId
-    /// @dev It removes liquidity and collects fees if the position is not empty
+    /// @dev It enforces that there is no open liquidity or tokens to be collected
     /// @param tokenId The ID of the position
-    /// @param recipient The address to send the collected tokens to
-    /// @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 burning the position
-    function burn(uint256 tokenId, address recipient, bytes calldata hookData, bool claims)
-        external
-        returns (BalanceDelta delta);
+    function burn(uint256 tokenId) external returns (BalanceDelta delta);
 
     // TODO: in v3, we can partially collect fees, but what was the usecase here?
     /// @notice Collect fees for a position

test/position-managers/NonfungiblePositionManager.t.sol

@@ -215,7 +215,10 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi
         // burn liquidity
         uint256 balance0BeforeBurn = currency0.balanceOfSelf();
         uint256 balance1BeforeBurn = currency1.balanceOfSelf();
-        BalanceDelta delta = lpm.burn(tokenId, address(this), ZERO_BYTES, false);
+        // TODO, encode this under one call
+        lpm.decreaseLiquidity(tokenId, liquidity, ZERO_BYTES, false);
+        lpm.collect(tokenId, address(this), ZERO_BYTES, false);
+        BalanceDelta delta = lpm.burn(tokenId);
         (,, liquidity,,,,) = lpm.positions(address(this), range.toId());
         assertEq(liquidity, 0);