From 0f394200191754bbf704a50a53418662ad5dd6da Mon Sep 17 00:00:00 2001
From: Sara Reynolds <30504811+snreynolds@users.noreply.github.com>
Date: Sun, 30 Jun 2024 20:12:14 -0400
Subject: [PATCH] Sra/edits (#137)

* consolidate using owner, update burn

* fix: accrue deltas to caller in increase
---
 .../autocompound_exactUnclaimedFees.snap      |  2 +-
 ...exactUnclaimedFees_exactCustodiedFees.snap |  2 +-
 .../autocompound_excessFeesCredit.snap        |  2 +-
 .forge-snapshots/decreaseLiquidity_erc20.snap |  2 +-
 .../decreaseLiquidity_erc6909.snap            |  2 +-
 .forge-snapshots/increaseLiquidity_erc20.snap |  2 +-
 .../increaseLiquidity_erc6909.snap            |  2 +-
 .forge-snapshots/mintWithLiquidity.snap       |  2 +-
 contracts/NonfungiblePositionManager.sol      | 54 +++++++++----------
 contracts/base/BaseLiquidityManagement.sol    | 15 +++++-
 .../interfaces/IBaseLiquidityManagement.sol   |  3 ++
 .../INonfungiblePositionManager.sol           | 10 ++--
 .../libraries/TransientLiquidityDelta.sol     |  6 ++-
 .../NonfungiblePositionManager.t.sol          |  5 +-
 14 files changed, 62 insertions(+), 47 deletions(-)

diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap
index c931974c..8dd96b22 100644
--- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap
+++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap
@@ -1 +1 @@
-299933
\ No newline at end of file
+300126
\ No newline at end of file
diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap
index aeba6588..f9df8100 100644
--- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap
+++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap
@@ -1 +1 @@
-239395
\ No newline at end of file
+239588
\ No newline at end of file
diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap
index 4c0c834b..eed2bed5 100644
--- a/.forge-snapshots/autocompound_excessFeesCredit.snap
+++ b/.forge-snapshots/autocompound_excessFeesCredit.snap
@@ -1 +1 @@
-320472
\ No newline at end of file
+320665
\ No newline at end of file
diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap
index 7b956e12..5e4cd19e 100644
--- a/.forge-snapshots/decreaseLiquidity_erc20.snap
+++ b/.forge-snapshots/decreaseLiquidity_erc20.snap
@@ -1 +1 @@
-215156
\ No newline at end of file
+215187
\ No newline at end of file
diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap
index f251c34d..e21ae2c4 100644
--- a/.forge-snapshots/decreaseLiquidity_erc6909.snap
+++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap
@@ -1 +1 @@
-215168
\ No newline at end of file
+215199
\ No newline at end of file
diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap
index ae25726e..37a69b54 100644
--- a/.forge-snapshots/increaseLiquidity_erc20.snap
+++ b/.forge-snapshots/increaseLiquidity_erc20.snap
@@ -1 +1 @@
-194836
\ No newline at end of file
+195029
\ No newline at end of file
diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap
index 218e7c7a..23ce9120 100644
--- a/.forge-snapshots/increaseLiquidity_erc6909.snap
+++ b/.forge-snapshots/increaseLiquidity_erc6909.snap
@@ -1 +1 @@
-194848
\ No newline at end of file
+195041
\ No newline at end of file
diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap
index 185c902a..39d02c26 100644
--- a/.forge-snapshots/mintWithLiquidity.snap
+++ b/.forge-snapshots/mintWithLiquidity.snap
@@ -1 +1 @@
-511680
\ No newline at end of file
+512049
\ No newline at end of file
diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol
index 5a9ec848..594e4997 100644
--- a/contracts/NonfungiblePositionManager.sol
+++ b/contracts/NonfungiblePositionManager.sol
@@ -20,6 +20,8 @@ import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientSta
 import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol";
 import {TransientLiquidityDelta} from "./libraries/TransientLiquidityDelta.sol";
 
+import "forge-std/console2.sol";
+
 contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidityManagement, ERC721Permit {
     using CurrencyLibrary for Currency;
     using CurrencySettleTake for Currency;
@@ -39,12 +41,18 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
     // TODO: TSTORE this jawn
     address internal msgSender;
 
+    // TODO: Context is inherited through ERC721 and will be not useful to use _msgSender() which will be address(this) with our current mutlicall.
+    function _msgSenderInternal() internal override returns (address) {
+        return msgSender;
+    }
+
     constructor(IPoolManager _manager)
         BaseLiquidityManagement(_manager)
         ERC721Permit("Uniswap V4 Positions NFT-V1", "UNI-V4-POS", "1")
     {}
 
     function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) public returns (bytes memory) {
+        msgSender = msg.sender;
         return manager.unlock(abi.encode(data, currencies));
     }
 
@@ -65,6 +73,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 +88,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;
@@ -131,7 +142,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
         if (manager.isUnlocked()) {
             _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData);
         } else {
-            msgSender = msg.sender;
             bytes[] memory data = new bytes[](1);
             data[0] = abi.encodeWithSelector(this.increaseLiquidity.selector, tokenId, liquidity, hookData, claims);
 
@@ -153,10 +163,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
         if (manager.isUnlocked()) {
             _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData);
         } else {
-            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 +174,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);
     }
 
@@ -198,7 +195,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
         if (manager.isUnlocked()) {
             _collect(recipient, tokenPos.range, hookData);
         } else {
-            msgSender = msg.sender;
             bytes[] memory data = new bytes[](1);
             data[0] = abi.encodeWithSelector(this.collect.selector, tokenId, recipient, hookData, claims);
 
diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol
index 662c54a0..c1c1308b 100644
--- a/contracts/base/BaseLiquidityManagement.sol
+++ b/contracts/base/BaseLiquidityManagement.sol
@@ -49,6 +49,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb
 
     constructor(IPoolManager _manager) ImmutableState(_manager) {}
 
+    function _msgSenderInternal() internal virtual returns (address);
+
     function _closeCallerDeltas(
         BalanceDelta callerDeltas,
         Currency currency0,
@@ -120,7 +122,9 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb
             (tokensOwed, callerDelta, thisDelta) =
                 _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta);
         }
-        callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1);
+
+        // Accrue all deltas to the caller.
+        callerDelta.flush(_msgSenderInternal(), range.poolKey.currency0, range.poolKey.currency1);
         thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1);
 
         position.addTokensOwed(tokensOwed);
@@ -201,6 +205,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 +239,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)
diff --git a/contracts/interfaces/IBaseLiquidityManagement.sol b/contracts/interfaces/IBaseLiquidityManagement.sol
index b5c07dd8..6bcb6e5b 100644
--- a/contracts/interfaces/IBaseLiquidityManagement.sol
+++ b/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
diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol
index 32c12756..dc7fdb71 100644
--- a/contracts/interfaces/INonfungiblePositionManager.sol
+++ b/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
diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol
index 86f85c85..9dca43a5 100644
--- a/contracts/libraries/TransientLiquidityDelta.sol
+++ b/contracts/libraries/TransientLiquidityDelta.sol
@@ -6,11 +6,15 @@ import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDe
 import {Currency} from "@uniswap/v4-core/src/types/Currency.sol";
 
 import {CurrencySettleTake} from "../libraries/CurrencySettleTake.sol";
+import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol";
+
+import "forge-std/console2.sol";
 
 /// @title a library to store callers' currency deltas in transient storage
 /// @dev this library implements the equivalent of a mapping, as transient storage can only be accessed in assembly
 library TransientLiquidityDelta {
     using CurrencySettleTake for Currency;
+    using TransientStateLibrary for IPoolManager;
 
     /// @notice calculates which storage slot a delta should be stored in for a given caller and currency
     function _computeSlot(address caller_, Currency currency) internal pure returns (bytes32 hashSlot) {
@@ -58,7 +62,7 @@ library TransientLiquidityDelta {
             delta := tload(hashSlot)
         }
 
-        // close the delta by paying or taking
+        // TODO support claims field
         if (delta < 0) {
             currency.settle(manager, holder, uint256(-delta), false);
         } else {
diff --git a/test/position-managers/NonfungiblePositionManager.t.sol b/test/position-managers/NonfungiblePositionManager.t.sol
index 5330b731..4b8a7791 100644
--- a/test/position-managers/NonfungiblePositionManager.t.sol
+++ b/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);