Unsubscribe on transfer (#395)

* Unsubscribe on transfer * snapshot
Uniswap · Nov 22, 2024 · 58974e3 · 58974e3
1 parent b3b7c49
commit 58974e3
Show file tree

Hide file tree

Showing 9 changed files with 26 additions and 114 deletions.
diff --git a/.forge-snapshots/PositionManager_subscribe.snap b/.forge-snapshots/PositionManager_subscribe.snap
@@ -1 +1 @@
-88168
+88124
diff --git a/.forge-snapshots/PositionManager_unsubscribe.snap b/.forge-snapshots/PositionManager_unsubscribe.snap
@@ -1 +1 @@
-63080
+63036
diff --git a/.forge-snapshots/positionDescriptor bytecode size.snap b/.forge-snapshots/positionDescriptor bytecode size.snap
@@ -1 +1 @@
-31687
+31806
diff --git a/src/PositionManager.sol b/src/PositionManager.sol
@@ -453,7 +453,7 @@ contract PositionManager is
     /// @dev will revert if pool manager is locked
     function transferFrom(address from, address to, uint256 id) public virtual override onlyIfPoolManagerLocked {
         super.transferFrom(from, to, id);
-        if (positionInfo[id].hasSubscriber()) _notifyTransfer(id, from, to);
+        if (positionInfo[id].hasSubscriber()) _unsubscribe(id);
     }
 
     /// @inheritdoc IPositionManager

diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol
@@ -103,19 +103,6 @@ abstract contract Notifier is INotifier {
         }
     }
 
-    function _notifyTransfer(uint256 tokenId, address previousOwner, address newOwner) internal {
-        ISubscriber _subscriber = subscriber[tokenId];
-
-        bool success =
-            _call(address(_subscriber), abi.encodeCall(ISubscriber.notifyTransfer, (tokenId, previousOwner, newOwner)));
-
-        if (!success) {
-            address(_subscriber).bubbleUpAndRevertWith(
-                ISubscriber.notifyTransfer.selector, TransferNotificationReverted.selector
-            );
-        }
-    }
-
     function _call(address target, bytes memory encodedCall) internal returns (bool success) {
         if (target.code.length == 0) NoCodeSubscriber.selector.revertWith();
         assembly ("memory-safe") {

diff --git a/src/interfaces/ISubscriber.sol b/src/interfaces/ISubscriber.sol
@@ -17,8 +17,4 @@ interface ISubscriber {
     /// @param liquidityChange the change in liquidity on the underlying position
     /// @param feesAccrued the fees to be collected from the position as a result of the modifyLiquidity call
     function notifyModifyLiquidity(uint256 tokenId, int256 liquidityChange, BalanceDelta feesAccrued) external;
-    /// @param tokenId the token ID of the position
-    /// @param previousOwner address of the old owner
-    /// @param newOwner address of the new owner
-    function notifyTransfer(uint256 tokenId, address previousOwner, address newOwner) external;
 }
diff --git a/test/mocks/MockBadSubscribers.sol b/test/mocks/MockBadSubscribers.sol
@@ -12,7 +12,6 @@ contract MockReturnDataSubscriber is ISubscriber {
     uint256 public notifySubscribeCount;
     uint256 public notifyUnsubscribeCount;
     uint256 public notifyModifyLiquidityCount;
-    uint256 public notifyTransferCount;
 
     error NotAuthorizedNotifer(address sender);
 
@@ -48,10 +47,6 @@ contract MockReturnDataSubscriber is ISubscriber {
         notifyModifyLiquidityCount++;
     }
 
-    function notifyTransfer(uint256, address, address) external onlyByPosm {
-        notifyTransferCount++;
-    }
-
     function setReturnDataSize(uint256 _value) external {
         memPtr = _value;
     }
@@ -90,10 +85,6 @@ contract MockRevertSubscriber is ISubscriber {
         revert TestRevert("notifyModifyLiquidity");
     }
 
-    function notifyTransfer(uint256, address, address) external view onlyByPosm {
-        revert TestRevert("notifyTransfer");
-    }
-
     function setRevert(bool _shouldRevert) external {
         shouldRevert = _shouldRevert;
     }

diff --git a/test/mocks/MockSubscriber.sol b/test/mocks/MockSubscriber.sol
@@ -12,7 +12,6 @@ contract MockSubscriber is ISubscriber {
     uint256 public notifySubscribeCount;
     uint256 public notifyUnsubscribeCount;
     uint256 public notifyModifyLiquidityCount;
-    uint256 public notifyTransferCount;
     int256 public liquidityChange;
     BalanceDelta public feesAccrued;
 
@@ -45,8 +44,4 @@ contract MockSubscriber is ISubscriber {
         liquidityChange = _liquidityChange;
         feesAccrued = _feesAccrued;
     }
-
-    function notifyTransfer(uint256, address, address) external onlyByPosm {
-        notifyTransferCount++;
-    }
 }
diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol
@@ -215,7 +215,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(int256(sub.feesAccrued().amount1()), int256(feeRevenue1) - 1 wei);
     }
 
-    function test_notifyTransfer_withTransferFrom_succeeds() public {
+    function test_transferFrom_unsubscribes() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
 
@@ -231,10 +231,12 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
 
         lpm.transferFrom(alice, bob, tokenId);
 
-        assertEq(sub.notifyTransferCount(), 1);
+        assertEq(sub.notifyUnsubscribeCount(), 1);
+        assertEq(lpm.positionInfo(tokenId).hasSubscriber(), false);
+        assertEq(address(lpm.subscriber(tokenId)), address(0));
     }
 
-    function test_notifyTransfer_withTransferFrom_selfDestruct_revert() public {
+    function test_transferFrom_unsubscribes_selfDestruct() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
 
@@ -250,11 +252,14 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         // simulate selfdestruct by etching the bytecode to 0
         vm.etch(address(sub), ZERO_BYTES);
 
-        vm.expectRevert(INotifier.NoCodeSubscriber.selector);
+        // unsubscribe happens anyway
         lpm.transferFrom(alice, bob, tokenId);
+
+        assertEq(lpm.positionInfo(tokenId).hasSubscriber(), false);
+        assertEq(address(lpm.subscriber(tokenId)), address(0));
     }
 
-    function test_notifyTransfer_withSafeTransferFrom_succeeds() public {
+    function test_safeTransferFrom_unsubscribes() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
 
@@ -270,10 +275,12 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
 
         lpm.safeTransferFrom(alice, bob, tokenId);
 
-        assertEq(sub.notifyTransferCount(), 1);
+        assertEq(sub.notifyUnsubscribeCount(), 1);
+        assertEq(lpm.positionInfo(tokenId).hasSubscriber(), false);
+        assertEq(address(lpm.subscriber(tokenId)), address(0));
     }
 
-    function test_notifyTransfer_withSafeTransferFrom_selfDestruct_revert() public {
+    function test_safeTransferFrom_unsubscribes_selfDestruct() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
 
@@ -289,11 +296,14 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         // simulate selfdestruct by etching the bytecode to 0
         vm.etch(address(sub), ZERO_BYTES);
 
-        vm.expectRevert(INotifier.NoCodeSubscriber.selector);
+        // unsubscribe happens anyway
         lpm.safeTransferFrom(alice, bob, tokenId);
+
+        assertEq(lpm.positionInfo(tokenId).hasSubscriber(), false);
+        assertEq(address(lpm.subscriber(tokenId)), address(0));
     }
 
-    function test_notifyTransfer_withSafeTransferFromData_succeeds() public {
+    function test_safeTransferFrom_unsubscribes_withData() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
 
@@ -309,7 +319,9 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
 
         lpm.safeTransferFrom(alice, bob, tokenId, "");
 
-        assertEq(sub.notifyTransferCount(), 1);
+        assertEq(sub.notifyUnsubscribeCount(), 1);
+        assertEq(lpm.positionInfo(tokenId).hasSubscriber(), false);
+        assertEq(address(lpm.subscriber(tokenId)), address(0));
     }
 
     function test_unsubscribe_succeeds() public {
@@ -554,75 +566,6 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         lpm.modifyLiquidities(calls, _deadline);
     }
 
-    function test_notifyTransfer_withTransferFrom_wraps_revert() public {
-        uint256 tokenId = lpm.nextTokenId();
-        mint(config, 100e18, alice, ZERO_BYTES);
-
-        // approve this contract to operate on alices liq
-        vm.startPrank(alice);
-        lpm.approve(address(this), tokenId);
-        vm.stopPrank();
-
-        lpm.subscribe(tokenId, address(revertSubscriber), ZERO_BYTES);
-
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                CustomRevert.WrappedError.selector,
-                address(revertSubscriber),
-                ISubscriber.notifyTransfer.selector,
-                abi.encodeWithSelector(MockRevertSubscriber.TestRevert.selector, "notifyTransfer"),
-                abi.encodeWithSelector(INotifier.TransferNotificationReverted.selector)
-            )
-        );
-        lpm.transferFrom(alice, bob, tokenId);
-    }
-
-    function test_notifyTransfer_withSafeTransferFrom_wraps_revert() public {
-        uint256 tokenId = lpm.nextTokenId();
-        mint(config, 100e18, alice, ZERO_BYTES);
-
-        // approve this contract to operate on alices liq
-        vm.startPrank(alice);
-        lpm.approve(address(this), tokenId);
-        vm.stopPrank();
-
-        lpm.subscribe(tokenId, address(revertSubscriber), ZERO_BYTES);
-
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                CustomRevert.WrappedError.selector,
-                address(revertSubscriber),
-                ISubscriber.notifyTransfer.selector,
-                abi.encodeWithSelector(MockRevertSubscriber.TestRevert.selector, "notifyTransfer"),
-                abi.encodeWithSelector(INotifier.TransferNotificationReverted.selector)
-            )
-        );
-        lpm.safeTransferFrom(alice, bob, tokenId);
-    }
-
-    function test_notifyTransfer_withSafeTransferFromData_wraps_revert() public {
-        uint256 tokenId = lpm.nextTokenId();
-        mint(config, 100e18, alice, ZERO_BYTES);
-
-        // approve this contract to operate on alices liq
-        vm.startPrank(alice);
-        lpm.approve(address(this), tokenId);
-        vm.stopPrank();
-
-        lpm.subscribe(tokenId, address(revertSubscriber), ZERO_BYTES);
-
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                CustomRevert.WrappedError.selector,
-                address(revertSubscriber),
-                ISubscriber.notifyTransfer.selector,
-                abi.encodeWithSelector(MockRevertSubscriber.TestRevert.selector, "notifyTransfer"),
-                abi.encodeWithSelector(INotifier.TransferNotificationReverted.selector)
-            )
-        );
-        lpm.safeTransferFrom(alice, bob, tokenId, "");
-    }
-
     /// @notice burning a position will automatically notify unsubscribe
     function test_burn_unsubscribe() public {
         uint256 tokenId = lpm.nextTokenId();