From 58974e36e7c342901d7bca2e50340dfafc03d4ff Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Fri, 22 Nov 2024 12:20:05 +0000 Subject: [PATCH] Unsubscribe on transfer (#395) * Unsubscribe on transfer * snapshot --- .../PositionManager_subscribe.snap | 2 +- .../PositionManager_unsubscribe.snap | 2 +- .../positionDescriptor bytecode size.snap | 2 +- src/PositionManager.sol | 2 +- src/base/Notifier.sol | 13 --- src/interfaces/ISubscriber.sol | 4 - test/mocks/MockBadSubscribers.sol | 9 -- test/mocks/MockSubscriber.sol | 5 - .../PositionManager.notifier.t.sol | 101 ++++-------------- 9 files changed, 26 insertions(+), 114 deletions(-) diff --git a/.forge-snapshots/PositionManager_subscribe.snap b/.forge-snapshots/PositionManager_subscribe.snap index b0cbf20a..74ca20b5 100644 --- a/.forge-snapshots/PositionManager_subscribe.snap +++ b/.forge-snapshots/PositionManager_subscribe.snap @@ -1 +1 @@ -88168 \ No newline at end of file +88124 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_unsubscribe.snap b/.forge-snapshots/PositionManager_unsubscribe.snap index 9bf5d646..c9bfebd4 100644 --- a/.forge-snapshots/PositionManager_unsubscribe.snap +++ b/.forge-snapshots/PositionManager_unsubscribe.snap @@ -1 +1 @@ -63080 \ No newline at end of file +63036 \ No newline at end of file diff --git a/.forge-snapshots/positionDescriptor bytecode size.snap b/.forge-snapshots/positionDescriptor bytecode size.snap index b89166c6..29912b34 100644 --- a/.forge-snapshots/positionDescriptor bytecode size.snap +++ b/.forge-snapshots/positionDescriptor bytecode size.snap @@ -1 +1 @@ -31687 \ No newline at end of file +31806 \ No newline at end of file diff --git a/src/PositionManager.sol b/src/PositionManager.sol index 428fb4b7..73253dbf 100644 --- 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 index 8f6b3f02..b85d3e6e 100644 --- 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 index 1e6f0476..0c410079 100644 --- 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 index 28f15182..d6e6e0c3 100644 --- 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 index 1e1fda15..ef4c2e86 100644 --- 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 index 3ec0e0c3..599bfbe1 100644 --- 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();