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.

Sign up for GitHub

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

posm: CLEAR_OR_TAKE #252

Merged
merged 9 commits into from
Aug 2, 2024
Merged

posm: CLEAR_OR_TAKE #252

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da06567
clear or take
saucepoint Aug 2, 2024
2ca05a5
misc code comment
saucepoint Aug 2, 2024
294826a
Merge branch 'main' into posm-clear-or-take
saucepoint Aug 2, 2024
c4be59b
update naming; use delta resolver helper for clearOrTake
saucepoint Aug 2, 2024
65dbcea
merge in main; fix broken naming
saucepoint Aug 2, 2024
f8c5688
merge main; fix conflicts; regenerate gas
saucepoint Aug 2, 2024
430eddb
nits
saucepoint Aug 2, 2024
9bacfdf
reorder
saucepoint Aug 2, 2024
f538c29
nits
saucepoint Aug 2, 2024
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_nonEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
129852
129834
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_burn_nonEmpty_native.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
122773
122756
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149984
149962
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect_native.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
141136
141114
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_collect_sameRange.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149984
149962
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decreaseLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
115527
115505
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decreaseLiquidity_native.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
108384
108366
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decrease_burnEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
133885
133868
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decrease_burnEmpty_native.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
126624
126607
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
128243
128221
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
170759
170737
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
140581
140655
20 changes: 14 additions & 6 deletions src/PositionManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ contract PositionManager is
using StateLibrary for IPoolManager;
using TransientStateLibrary for IPoolManager;
using SafeCast for uint256;
using SafeCast for int256;
using CalldataDecoder for bytes;
using SlippageCheckLibrary for BalanceDelta;

Expand Down Expand Up @@ -111,9 +112,9 @@ contract PositionManager is
} else if (action == Actions.CLOSE_CURRENCY) {
Currency currency = params.decodeCurrency();
_close(currency);
} else if (action == Actions.CLEAR) {
} else if (action == Actions.CLEAR_OR_TAKE) {
(Currency currency, uint256 amountMax) = params.decodeCurrencyAndUint256();
_clear(currency, amountMax);
_clearOrTake(currency, amountMax);
} else if (action == Actions.BURN_POSITION) {
// Will automatically decrease liquidity to 0 if the position is not already empty.
(
Expand Down Expand Up @@ -208,11 +209,18 @@ contract PositionManager is
}

/// @dev integrators may elect to forfeit positive deltas with clear
/// provides a safety check that amount-to-clear is less than a user-provided maximum
function _clear(Currency currency, uint256 amountMax) internal {
/// if the forfeit amount exceeds the user-specified max, the amount is taken instead
function _clearOrTake(Currency currency, uint256 amountMax) internal {
int256 currencyDelta = poolManager.currencyDelta(address(this), currency);
if (uint256(currencyDelta) > amountMax) revert ClearExceedsMaxAmount(currency, currencyDelta, amountMax);
poolManager.clear(currency, uint256(currencyDelta));
if (currencyDelta < 0) revert CannotClearNegativeDelta(currency);
uint256 delta = uint256(currencyDelta); // safe since we know its positive
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if this should be replaced with _getFullTakeAmount? the error might need to be updated

    function _getFullTakeAmount(Currency currency) internal view returns (uint256 amount) {
        int256 _amount = poolManager.currencyDelta(address(this), currency);
        // If the amount is negative, it should be settled not taken.
        if (_amount < 0) revert IncorrectUseOfTake();
        amount = uint256(_amount);
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we could change both of those functions to be instead

_getFullNegativeDelta and _getFullPositiveDelta and the error be more general like DeltaIsNotNegative()/DeltaIsNotPositive()


// forfeit the delta if its less than or equal to the user-specified limit
if (delta <= amountMax) {
poolManager.clear(currency, delta);
} else {
_take(currency, _msgSender(), delta);
}
}

/// @dev uses this addresses balance to settle a negative delta
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/IPositionManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ interface IPositionManager {
error NotApproved(address caller);
error DeadlinePassed();
error IncorrectPositionConfigForTokenId(uint256 tokenId);
error ClearExceedsMaxAmount(Currency currency, int256 amount, uint256 maxAmount);
error CannotClearNegativeDelta(Currency currency);

/// @notice Maps the ERC721 tokenId to a configId, which is a keccak256 hash of the position's pool key, and range (tickLower, tickUpper)
/// Enforces that a minted ERC721 token is tied to one range on one pool.
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Actions.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ library Actions {

uint256 constant CLOSE_CURRENCY = 0x16;
uint256 constant CLOSE_PAIR = 0x17;
uint256 constant CLEAR = 0x18;
uint256 constant CLEAR_OR_TAKE = 0x18;
uint256 constant SWEEP = 0x19;

// minting/burning 6909s to close deltas
Expand Down
2 changes: 1 addition & 1 deletion test/BaseActionsRouter.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ contract BaseActionsRouterTest is Test, Deployers, GasSnapshot {
function test_clear_suceeds() public {
Plan memory plan = Planner.init();
for (uint256 i = 0; i < 10; i++) {
plan.add(Actions.CLEAR, "");
plan.add(Actions.CLEAR_OR_TAKE, "");
}

assertEq(router.clearCount(), 0);
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/MockBaseActionsRouter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ contract MockBaseActionsRouter is BaseActionsRouter, ReentrancyLock {
} else {
if (action == Actions.SETTLE) _settle(params);
else if (action == Actions.TAKE) _take(params);
else if (action == Actions.CLEAR) _clear(params);
else if (action == Actions.CLEAR_OR_TAKE) _clear(params);
else if (action == Actions.MINT_6909) _mint6909(params);
else if (action == Actions.BURN_6909) _burn6909(params);
else revert UnsupportedAction(action);
Expand Down
49 changes: 28 additions & 21 deletions test/position-managers/IncreaseLiquidity.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
Actions.INCREASE_LIQUIDITY,
abi.encode(tokenIdAlice, config, liquidityDelta, MAX_SLIPPAGE_INCREASE, MAX_SLIPPAGE_INCREASE, ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency0, 18 wei)); // alice is willing to forfeit 18 wei
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency1, 18 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency0, 18 wei)); // alice is willing to forfeit 18 wei
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency1, 18 wei));
bytes memory calls = planner.encode();

vm.prank(alice);
Expand Down Expand Up @@ -279,8 +279,8 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
Actions.INCREASE_LIQUIDITY,
abi.encode(tokenIdAlice, config, liquidityDelta, MAX_SLIPPAGE_INCREASE, MAX_SLIPPAGE_INCREASE, ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency0, 1 wei)); // alice is willing to forfeit 1 wei
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency1, 1 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency0, 1 wei)); // alice is willing to forfeit 1 wei
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency1, 1 wei));
bytes memory calls = planner.encode();

vm.prank(alice);
Expand Down Expand Up @@ -653,7 +653,8 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
assertEq(currency1.balanceOf(address(this)), balanceBefore1 - amount1);
}

function test_increaseLiquidity_clearExceeds_revert() public {
/// @dev if clearing exceeds the max amount, the amount is taken instead
function test_increaseLiquidity_clearExceedsThenTake() public {
uint256 tokenId = lpm.nextTokenId();
mint(config, 1000e18, address(this), ZERO_BYTES);

Expand All @@ -663,6 +664,7 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {

// calculate the amount of liquidity to add, using half of the proceeds
uint256 amountToReinvest = amountToDonate / 2;
uint256 amountToReclaim = amountToDonate / 2; // expect to reclaim the other half of the fee revenue
uint256 liquidityDelta = LiquidityAmounts.getLiquidityForAmounts(
SQRT_PRICE_1_1,
TickMath.getSqrtPriceAtTick(config.tickLower),
Expand All @@ -671,28 +673,33 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
amountToReinvest
);

// set the max-forfeit to less than the amount we expect to claim
uint256 maxClear = amountToReclaim - 2 wei;

Plan memory planner = Planner.init();
planner.add(
Actions.INCREASE_LIQUIDITY,
abi.encode(tokenId, config, liquidityDelta, MAX_SLIPPAGE_INCREASE, MAX_SLIPPAGE_INCREASE, ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency0, amountToReinvest - 2 wei));
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency1, amountToReinvest - 2 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency0, maxClear));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency1, maxClear));
bytes memory calls = planner.encode();

// revert since we're forfeiting beyond the max tolerance
vm.expectRevert(
abi.encodeWithSelector(
IPositionManager.ClearExceedsMaxAmount.selector,
config.poolKey.currency0,
int256(amountToReinvest - 1 wei), // imprecision, PM expects us to collect half of the fees (minus 1 wei)
uint256(amountToReinvest - 2 wei) // the maximum amount we were willing to forfeit
)
);
uint256 balance0Before = currency0.balanceOf(address(this));
uint256 balance1Before = currency1.balanceOf(address(this));

// expect to take the excess, as it exceeds the amount to clear
lpm.modifyLiquidities(calls, _deadline);
BalanceDelta delta = getLastDelta();

assertEq(uint128(delta.amount0()), amountToReclaim - 1 wei); // imprecision
assertEq(uint128(delta.amount1()), amountToReclaim - 1 wei);

assertEq(currency0.balanceOf(address(this)), balance0Before + amountToReclaim - 1 wei);
assertEq(currency1.balanceOf(address(this)), balance1Before + amountToReclaim - 1 wei);
}

/// @dev clearing a negative delta reverts in core with SafeCastOverflow
/// @dev clearing a negative delta reverts
function test_increaseLiquidity_clearNegative_revert() public {
uint256 tokenId = lpm.nextTokenId();
mint(config, 1000e18, address(this), ZERO_BYTES);
Expand All @@ -703,12 +710,12 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
Actions.INCREASE_LIQUIDITY,
abi.encode(tokenId, config, 100e18, MAX_SLIPPAGE_INCREASE, MAX_SLIPPAGE_INCREASE, ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency0, type(uint256).max));
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency1, type(uint256).max));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency0, type(uint256).max));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency1, type(uint256).max));
bytes memory calls = planner.encode();

// revert since we're forfeiting beyond the max tolerance
vm.expectRevert(SafeCast.SafeCastOverflow.selector);
// revert since we're trying to clear a negative delta
vm.expectRevert(abi.encodeWithSelector(IPositionManager.CannotClearNegativeDelta.selector, currency0));
lpm.modifyLiquidities(calls, _deadline);
}
}
4 changes: 2 additions & 2 deletions test/position-managers/PositionManager.gas.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,8 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot {
Actions.INCREASE_LIQUIDITY,
abi.encode(tokenIdAlice, config, liquidityDelta, MAX_SLIPPAGE_INCREASE, MAX_SLIPPAGE_INCREASE, ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency0, halfTokensOwedAlice + 1 wei));
planner.add(Actions.CLEAR, abi.encode(config.poolKey.currency1, halfTokensOwedAlice + 1 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency0, halfTokensOwedAlice + 1 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(config.poolKey.currency1, halfTokensOwedAlice + 1 wei));
bytes memory calls = planner.encode();

vm.prank(alice);
Expand Down
44 changes: 31 additions & 13 deletions test/position-managers/PositionManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
assertEq(currency1.balanceOf(alice), balance1BeforeAlice);
}

/// @dev test that clear does not work on minting
/// @dev clear cannot be used on mint (negative delta)
function test_fuzz_mint_clear_revert(IPoolManager.ModifyLiquidityParams memory seedParams) public {
IPoolManager.ModifyLiquidityParams memory params = createFuzzyLiquidityParams(key, seedParams, SQRT_PRICE_1_1);
uint256 liquidityToAdd =
Expand All @@ -196,11 +196,19 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
Actions.MINT_POSITION,
abi.encode(config, liquidityToAdd, MAX_SLIPPAGE_INCREASE, MAX_SLIPPAGE_INCREASE, address(this), ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(key.currency0, type(uint256).max));
planner.add(Actions.CLEAR, abi.encode(key.currency1, type(uint256).max));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(key.currency0, type(uint256).max));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(key.currency1, type(uint256).max));
bytes memory calls = planner.encode();

vm.expectRevert(SafeCast.SafeCastOverflow.selector);
Currency negativeDeltaCurrency = currency0;
// because we're fuzzing the range, single-sided mint with currency1 means currency0Delta = 0 and currency1Delta < 0
if (config.tickUpper <= 0) {
negativeDeltaCurrency = currency1;
}

vm.expectRevert(
abi.encodeWithSelector(IPositionManager.CannotClearNegativeDelta.selector, negativeDeltaCurrency)
);
lpm.modifyLiquidities(calls, _deadline);
}

Expand Down Expand Up @@ -469,8 +477,8 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
tokenId, config, decreaseLiquidityDelta, MIN_SLIPPAGE_DECREASE, MIN_SLIPPAGE_DECREASE, ZERO_BYTES
)
);
planner.add(Actions.CLEAR, abi.encode(key.currency0, type(uint256).max));
planner.add(Actions.CLEAR, abi.encode(key.currency1, type(uint256).max));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(key.currency0, type(uint256).max));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(key.currency1, type(uint256).max));
bytes memory calls = planner.encode();

lpm.modifyLiquidities(calls, _deadline);
Expand All @@ -485,8 +493,10 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
assertEq(currency1.balanceOfSelf(), balance1Before);
}

/// @dev Clearing on decrease reverts if it exceeds user threshold
function test_fuzz_decreaseLiquidity_clearRevert(IPoolManager.ModifyLiquidityParams memory params) public {
/// @dev Clearing on decrease will take tokens if the amount exceeds the clear limit
function test_fuzz_decreaseLiquidity_clearExceedsThenTake(IPoolManager.ModifyLiquidityParams memory params)
public
{
// use fuzzer for tick range
params = createFuzzyLiquidityParams(key, params, SQRT_PRICE_1_1);
vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity
Expand All @@ -511,14 +521,22 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
Actions.DECREASE_LIQUIDITY,
abi.encode(tokenId, config, liquidityToRemove, MIN_SLIPPAGE_DECREASE, MIN_SLIPPAGE_DECREASE, ZERO_BYTES)
);
planner.add(Actions.CLEAR, abi.encode(key.currency0, amount0 - 1 wei));
planner.add(Actions.CLEAR, abi.encode(key.currency1, amount1 - 1 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(key.currency0, amount0 - 1 wei));
planner.add(Actions.CLEAR_OR_TAKE, abi.encode(key.currency1, amount1 - 1 wei));
bytes memory calls = planner.encode();

vm.expectRevert(
abi.encodeWithSelector(IPositionManager.ClearExceedsMaxAmount.selector, currency0, amount0, amount0 - 1 wei)
);
uint256 balance0Before = currency0.balanceOfSelf();
uint256 balance1Before = currency1.balanceOfSelf();

// expect to take the tokens
lpm.modifyLiquidities(calls, _deadline);
BalanceDelta delta = getLastDelta();

// clear failed, so we should have the tokens
assertEq(uint128(delta.amount0()), amount0);
assertEq(uint128(delta.amount1()), amount1);
assertEq(currency0.balanceOfSelf(), balance0Before + amount0);
assertEq(currency1.balanceOfSelf(), balance1Before + amount1);
}

function test_decreaseLiquidity_collectFees(
Expand Down
Loading