Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use notifyBurn #399

Merged
merged 4 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_burn_empty.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
50540
50845
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_burn_empty_native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
50540
50845
Original file line number Diff line number Diff line change
@@ -1 +1 @@
125788
126075
Original file line number Diff line number Diff line change
@@ -1 +1 @@
125220
125508
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132648
132935
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132080
132368
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect_native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
146518
146517
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect_sameRange.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
155093
155092
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect_withClose.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
155093
155092
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect_withTakePair.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
154396
154395
Original file line number Diff line number Diff line change
@@ -1 +1 @@
112160
112159
Original file line number Diff line number Diff line change
@@ -1 +1 @@
119974
119973
Original file line number Diff line number Diff line change
@@ -1 +1 @@
119277
119276
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decrease_burnEmpty.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
135480
135785
Original file line number Diff line number Diff line change
@@ -1 +1 @@
128620
128925
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132661
132660
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decrease_take_take.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
120564
120563
Original file line number Diff line number Diff line change
@@ -1 +1 @@
158973
158972
Original file line number Diff line number Diff line change
@@ -1 +1 @@
157884
157883
Original file line number Diff line number Diff line change
@@ -1 +1 @@
142237
142236
Original file line number Diff line number Diff line change
@@ -1 +1 @@
136418
136417
Original file line number Diff line number Diff line change
@@ -1 +1 @@
177562
177561
Original file line number Diff line number Diff line change
@@ -1 +1 @@
148338
148337
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_mint_native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
366111
366110
Original file line number Diff line number Diff line change
@@ -1 +1 @@
374685
374684
Original file line number Diff line number Diff line change
@@ -1 +1 @@
373880
373879
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_mint_onSameTickLower.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
317534
317533
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_mint_onSameTickUpper.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
318204
318203
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_mint_sameRange.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
243773
243772
Original file line number Diff line number Diff line change
@@ -1 +1 @@
419063
419062
Original file line number Diff line number Diff line change
@@ -1 +1 @@
323565
323564
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_mint_withClose.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
420087
420086
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_mint_withSettlePair.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
419116
419115
Original file line number Diff line number Diff line change
@@ -1 +1 @@
455917
455916
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_unsubscribe.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
63036
63058
21 changes: 18 additions & 3 deletions src/PositionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -411,20 +411,34 @@ contract PositionManager is

uint256 liquidity = uint256(_getLiquidity(tokenId, poolKey, info.tickLower(), info.tickUpper()));

address owner = ownerOf(tokenId);

// Clear the position info.
positionInfo[tokenId] = PositionInfoLibrary.EMPTY_POSITION_INFO;
// Burn the token.
_burn(tokenId);

// Can only call modify if there is non zero liquidity.
BalanceDelta feesAccrued;
BalanceDelta liquidityDelta;
if (liquidity > 0) {
(BalanceDelta liquidityDelta, BalanceDelta feesAccrued) =
_modifyLiquidity(info, poolKey, -(liquidity.toInt256()), bytes32(tokenId), hookData);
// do not use _modifyLiquidity as we do not need to notify on modification for burns.
(liquidityDelta, feesAccrued) = poolManager.modifyLiquidity(
poolKey,
IPoolManager.ModifyLiquidityParams({
tickLower: info.tickLower(),
tickUpper: info.tickUpper(),
liquidityDelta: -(liquidity.toInt256()),
salt: bytes32(tokenId)
}),
hookData
);
// Slippage checks should be done on the principal liquidityDelta which is the liquidityDelta - feesAccrued
(liquidityDelta - feesAccrued).validateMinOut(amount0Min, amount1Min);
}

if (info.hasSubscriber()) _unsubscribe(tokenId);
// deletes then notifies the subscriber
if (info.hasSubscriber()) _removeSubscriberAndNotifyBurn(tokenId, owner, info, liquidity, feesAccrued);
}

function _settlePair(Currency currency0, Currency currency1) internal {
Expand Down Expand Up @@ -475,6 +489,7 @@ contract PositionManager is
if (balance > 0) currency.transfer(to, balance);
}

/// @dev if there is a subscriber attached to the position, this function will notify the subscriber
function _modifyLiquidity(
PositionInfo info,
PoolKey memory poolKey,
Expand Down
24 changes: 24 additions & 0 deletions src/base/Notifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,30 @@ abstract contract Notifier is INotifier {
emit Unsubscription(tokenId, address(_subscriber));
}

/// @dev note this function also deletes the subscriber address from the mapping
function _removeSubscriberAndNotifyBurn(
uint256 tokenId,
address owner,
PositionInfo info,
uint256 liquidity,
BalanceDelta feesAccrued
) internal {
ISubscriber _subscriber = subscriber[tokenId];

// remove the subscriber
delete subscriber[tokenId];
hensha256 marked this conversation as resolved.
Show resolved Hide resolved

bool success = _call(
address(_subscriber), abi.encodeCall(ISubscriber.notifyBurn, (tokenId, owner, info, liquidity, feesAccrued))
);

if (!success) {
address(_subscriber).bubbleUpAndRevertWith(
ISubscriber.notifyBurn.selector, BurnNotificationReverted.selector
);
}
}

function _notifyModifyLiquidity(uint256 tokenId, int256 liquidityChange, BalanceDelta feesAccrued) internal {
ISubscriber _subscriber = subscriber[tokenId];

Expand Down
2 changes: 2 additions & 0 deletions src/interfaces/INotifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ interface INotifier {
error SubscriptionReverted(address subscriber, bytes reason);
/// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification
error ModifyLiquidityNotificationReverted(address subscriber, bytes reason);
/// @notice Wraps the revert message of the subscriber contract on a reverting burn notification
error BurnNotificationReverted(address subscriber, bytes reason);
/// @notice Wraps the revert message of the subscriber contract on a reverting transfer notification
error TransferNotificationReverted(address subscriber, bytes reason);
/// @notice Thrown when a tokenId already has a subscriber
Expand Down
9 changes: 9 additions & 0 deletions src/interfaces/ISubscriber.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity ^0.8.0;

import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
import {PositionInfo} from "../libraries/PositionInfoLibrary.sol";

/// @notice Interface that a Subscriber contract should implement to receive updates from the v4 position manager
interface ISubscriber {
Expand All @@ -13,6 +14,14 @@ interface ISubscriber {
/// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft()
/// @param tokenId the token ID of the position
function notifyUnsubscribe(uint256 tokenId) external;
/// @notice Called when a position is burned
/// @param tokenId the token ID of the position
/// @param owner the current owner of the tokenId
/// @param info information about the position
/// @param liquidity the amount of liquidity decreased in the position, may be 0
/// @param feesAccrued the fees accrued by the position if liquidity was decreased
function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)
external;
/// @param tokenId the token ID of the position
/// @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
Expand Down
13 changes: 13 additions & 0 deletions test/mocks/MockBadSubscribers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.20;
import {ISubscriber} from "../../src/interfaces/ISubscriber.sol";
import {PositionManager} from "../../src/PositionManager.sol";
import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
import {PositionInfo} from "../../src/libraries/PositionInfoLibrary.sol";

/// @notice A subscriber contract that returns values from the subscriber entrypoints
contract MockReturnDataSubscriber is ISubscriber {
Expand Down Expand Up @@ -47,6 +48,12 @@ contract MockReturnDataSubscriber is ISubscriber {
notifyModifyLiquidityCount++;
}

function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)
external
{
return;
}

function setReturnDataSize(uint256 _value) external {
memPtr = _value;
}
Expand Down Expand Up @@ -85,6 +92,12 @@ contract MockRevertSubscriber is ISubscriber {
revert TestRevert("notifyModifyLiquidity");
}

function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)
external
{
return;
}

function setRevert(bool _shouldRevert) external {
shouldRevert = _shouldRevert;
}
Expand Down
9 changes: 9 additions & 0 deletions test/mocks/MockSubscriber.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.20;
import {ISubscriber} from "../../src/interfaces/ISubscriber.sol";
import {PositionManager} from "../../src/PositionManager.sol";
import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
import {PositionInfo} from "../../src/libraries/PositionInfoLibrary.sol";

/// @notice A subscriber contract that ingests updates from the v4 position manager
contract MockSubscriber is ISubscriber {
Expand All @@ -12,6 +13,7 @@ contract MockSubscriber is ISubscriber {
uint256 public notifySubscribeCount;
uint256 public notifyUnsubscribeCount;
uint256 public notifyModifyLiquidityCount;
uint256 public notifyBurnCount;
int256 public liquidityChange;
BalanceDelta public feesAccrued;

Expand Down Expand Up @@ -44,4 +46,11 @@ contract MockSubscriber is ISubscriber {
liquidityChange = _liquidityChange;
feesAccrued = _feesAccrued;
}

function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)
external
onlyByPosm
{
notifyBurnCount++;
}
}
9 changes: 5 additions & 4 deletions test/position-managers/PositionManager.notifier.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,8 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
lpm.modifyLiquidities(calls, _deadline);
}

/// @notice burning a position will automatically notify unsubscribe
function test_burn_unsubscribe() public {
/// @notice burning a position will automatically notify burn
function test_notifyBurn_succeeds() public {
uint256 tokenId = lpm.nextTokenId();
mint(config, 100e18, alice, ZERO_BYTES);

Expand All @@ -583,12 +583,13 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
assertEq(lpm.positionInfo(tokenId).hasSubscriber(), true);
assertEq(sub.notifyUnsubscribeCount(), 0);

// burn the position, causing an unsubscribe
// burn the position, causing a notifyBurn
burn(tokenId, config, ZERO_BYTES);

// position is now unsubscribed
assertEq(lpm.positionInfo(tokenId).hasSubscriber(), false);
assertEq(sub.notifyUnsubscribeCount(), 1);
assertEq(sub.notifyUnsubscribeCount(), 0);
assertEq(sub.notifyBurnCount(), 1);
}

/// @notice Test that users cannot forcibly avoid unsubscribe logic via gas limits
Expand Down