diff --git a/.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap b/.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap index 32491bda..16f17b7f 100644 --- a/.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap +++ b/.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap @@ -1 +1 @@ -140844 \ No newline at end of file +140956 \ No newline at end of file diff --git a/.forge-snapshots/V4Router_Bytecode.snap b/.forge-snapshots/V4Router_Bytecode.snap index e5e6f155..79693d11 100644 --- a/.forge-snapshots/V4Router_Bytecode.snap +++ b/.forge-snapshots/V4Router_Bytecode.snap @@ -1 +1 @@ -8068 \ No newline at end of file +8116 \ No newline at end of file diff --git a/src/PositionManager.sol b/src/PositionManager.sol index f94f1525..fabc4d12 100644 --- a/src/PositionManager.sol +++ b/src/PositionManager.sol @@ -48,6 +48,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; @@ -155,9 +156,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. ( @@ -251,18 +252,23 @@ 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 { - int256 currencyDelta = poolManager.currencyDelta(address(this), currency); - if (uint256(currencyDelta) > amountMax) revert ClearExceedsMaxAmount(currency, currencyDelta, amountMax); - poolManager.clear(currency, uint256(currencyDelta)); + /// if the forfeit amount exceeds the user-specified max, the amount is taken instead + function _clearOrTake(Currency currency, uint256 amountMax) internal { + uint256 delta = _getFullCredit(currency); + + // 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); + } } function _settlePair(Currency currency0, Currency currency1) internal { // the locker is the payer when settling address caller = msgSender(); - _settle(currency0, caller, _getFullSettleAmount(currency0)); - _settle(currency1, caller, _getFullSettleAmount(currency1)); + _settle(currency0, caller, _getFullDebt(currency0)); + _settle(currency1, caller, _getFullDebt(currency1)); } /// @dev this is overloaded with ERC721Permit._burn diff --git a/src/V4Router.sol b/src/V4Router.sol index 5adbc03c..a1e4908f 100644 --- a/src/V4Router.sol +++ b/src/V4Router.sol @@ -53,17 +53,17 @@ abstract contract V4Router is IV4Router, BaseActionsRouter, DeltaResolver { if (action == Actions.SETTLE_ALL) { Currency currency = params.decodeCurrency(); // TODO should it have a maxAmountOut added slippage protection? - _settle(currency, msgSender(), _getFullSettleAmount(currency)); + _settle(currency, msgSender(), _getFullDebt(currency)); } else if (action == Actions.SETTLE) { (Currency currency, uint256 amount, bool payerIsUser) = params.decodeCurrencyUint256AndBool(); _settle(currency, _mapPayer(payerIsUser), _mapSettleAmount(amount, currency)); } else if (action == Actions.TAKE_ALL) { (Currency currency, address recipient) = params.decodeCurrencyAndAddress(); - uint256 amount = _getFullTakeAmount(currency); + uint256 amount = _getFullCredit(currency); _take(currency, _mapRecipient(recipient), amount); } else if (action == Actions.TAKE_PORTION) { (Currency currency, address recipient, uint256 bips) = params.decodeCurrencyAddressAndUint256(); - _take(currency, _mapRecipient(recipient), _getFullTakeAmount(currency).calculatePortion(bips)); + _take(currency, _mapRecipient(recipient), _getFullCredit(currency).calculatePortion(bips)); } else { revert UnsupportedAction(action); } diff --git a/src/base/DeltaResolver.sol b/src/base/DeltaResolver.sol index cfed40c4..75bc5649 100644 --- a/src/base/DeltaResolver.sol +++ b/src/base/DeltaResolver.sol @@ -15,9 +15,9 @@ abstract contract DeltaResolver is ImmutableState { using SafeCast for *; /// @notice Emitted trying to settle a positive delta. - error IncorrectUseOfSettle(); + error DeltaNotPositive(Currency currency); /// @notice Emitted trying to take a negative delta. - error IncorrectUseOfTake(); + error DeltaNotNegative(Currency currency); /// @notice Take an amount of currency out of the PoolManager /// @param currency Currency to take @@ -49,17 +49,23 @@ abstract contract DeltaResolver is ImmutableState { /// @param amount The number of tokens to send function _pay(Currency token, address payer, uint256 amount) internal virtual; - function _getFullSettleAmount(Currency currency) internal view returns (uint256 amount) { + /// @notice Obtain the full amount owed by this contract (negative delta) + /// @param currency Currency to get the delta for + /// @return amount The amount owed by this contract as a uint256 + function _getFullDebt(Currency currency) internal view returns (uint256 amount) { int256 _amount = poolManager.currencyDelta(address(this), currency); - // If the amount is positive, it should be taken not settled for. - if (_amount > 0) revert IncorrectUseOfSettle(); + // If the amount is negative, it should be settled not taken. + if (_amount > 0) revert DeltaNotNegative(currency); amount = uint256(-_amount); } - function _getFullTakeAmount(Currency currency) internal view returns (uint256 amount) { + /// @notice Obtain the full credit owed to this contract (positive delta) + /// @param currency Currency to get the delta for + /// @return amount The amount owed to this contract as a uint256 + function _getFullCredit(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(); + // If the amount is negative, it should be taken not settled for. + if (_amount < 0) revert DeltaNotPositive(currency); amount = uint256(_amount); } @@ -68,7 +74,7 @@ abstract contract DeltaResolver is ImmutableState { if (amount == Constants.CONTRACT_BALANCE) { return currency.balanceOfSelf(); } else if (amount == Constants.OPEN_DELTA) { - return _getFullSettleAmount(currency); + return _getFullDebt(currency); } return amount; } @@ -78,7 +84,7 @@ abstract contract DeltaResolver is ImmutableState { if (amount == Constants.CONTRACT_BALANCE) { return currency.balanceOfSelf().toUint128(); } else if (amount == Constants.OPEN_DELTA) { - return _getFullTakeAmount(currency).toUint128(); + return _getFullCredit(currency).toUint128(); } return amount; } diff --git a/src/interfaces/IPositionManager.sol b/src/interfaces/IPositionManager.sol index c2ae7201..0fde8d62 100644 --- a/src/interfaces/IPositionManager.sol +++ b/src/interfaces/IPositionManager.sol @@ -10,7 +10,6 @@ interface IPositionManager is INotifier { error NotApproved(address caller); error DeadlinePassed(); error IncorrectPositionConfigForTokenId(uint256 tokenId); - error ClearExceedsMaxAmount(Currency currency, int256 amount, uint256 maxAmount); /// @notice Batches many liquidity modification calls to pool manager /// @param payload is an encoding of actions, and parameters for those actions diff --git a/src/libraries/Actions.sol b/src/libraries/Actions.sol index e0005b2e..da5d015f 100644 --- a/src/libraries/Actions.sol +++ b/src/libraries/Actions.sol @@ -29,7 +29,7 @@ library Actions { uint256 constant TAKE_PORTION = 0x14; uint256 constant CLOSE_CURRENCY = 0x15; - uint256 constant CLEAR = 0x16; + uint256 constant CLEAR_OR_TAKE = 0x16; uint256 constant SWEEP = 0x17; // minting/burning 6909s to close deltas diff --git a/test/BaseActionsRouter.t.sol b/test/BaseActionsRouter.t.sol index cb8465c0..61020b2c 100644 --- a/test/BaseActionsRouter.t.sol +++ b/test/BaseActionsRouter.t.sol @@ -77,7 +77,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); diff --git a/test/mocks/MockBaseActionsRouter.sol b/test/mocks/MockBaseActionsRouter.sol index a6a033ce..b7630297 100644 --- a/test/mocks/MockBaseActionsRouter.sol +++ b/test/mocks/MockBaseActionsRouter.sol @@ -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); diff --git a/test/position-managers/IncreaseLiquidity.t.sol b/test/position-managers/IncreaseLiquidity.t.sol index c6d076f4..dc413612 100644 --- a/test/position-managers/IncreaseLiquidity.t.sol +++ b/test/position-managers/IncreaseLiquidity.t.sol @@ -20,6 +20,7 @@ import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {IERC20} from "forge-std/interfaces/IERC20.sol"; import {PositionManager} from "../../src/PositionManager.sol"; +import {DeltaResolver} from "../../src/base/DeltaResolver.sol"; import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; import {SlippageCheckLibrary} from "../../src/libraries/SlippageCheck.sol"; import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; @@ -177,8 +178,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); @@ -280,8 +281,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); @@ -692,7 +693,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); @@ -702,6 +704,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), @@ -710,28 +713,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); @@ -742,12 +750,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(DeltaResolver.DeltaNotPositive.selector, currency0)); lpm.modifyLiquidities(calls, _deadline); } } diff --git a/test/position-managers/PositionManager.gas.t.sol b/test/position-managers/PositionManager.gas.t.sol index 84da682b..d0c9517d 100644 --- a/test/position-managers/PositionManager.gas.t.sol +++ b/test/position-managers/PositionManager.gas.t.sol @@ -263,8 +263,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); diff --git a/test/position-managers/PositionManager.t.sol b/test/position-managers/PositionManager.t.sol index adb81fe0..7afae777 100644 --- a/test/position-managers/PositionManager.t.sol +++ b/test/position-managers/PositionManager.t.sol @@ -23,6 +23,7 @@ import {IERC20} from "forge-std/interfaces/IERC20.sol"; import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; import {Actions} from "../../src/libraries/Actions.sol"; import {PositionManager} from "../../src/PositionManager.sol"; +import {DeltaResolver} from "../../src/base/DeltaResolver.sol"; import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; import {SlippageCheckLibrary} from "../../src/libraries/SlippageCheck.sol"; import {BaseActionsRouter} from "../../src/base/BaseActionsRouter.sol"; @@ -218,7 +219,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 = @@ -232,11 +233,17 @@ 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(DeltaResolver.DeltaNotPositive.selector, negativeDeltaCurrency)); lpm.modifyLiquidities(calls, _deadline); } @@ -507,8 +514,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); @@ -523,8 +530,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 @@ -549,14 +558,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(); + + // amount exceeded clear limit, 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(