PR feedback

Uniswap · Jul 31, 2024 · 3b0bd3d · 3b0bd3d
1 parent a415237
commit 3b0bd3d
Show file tree

Hide file tree

Showing 29 changed files with 86 additions and 47 deletions.
diff --git a/.forge-snapshots/PositionManager_burn_nonEmpty.snap b/.forge-snapshots/PositionManager_burn_nonEmpty.snap
@@ -1 +1 @@
-129992
+130023
diff --git a/.forge-snapshots/PositionManager_burn_nonEmpty_native.snap b/.forge-snapshots/PositionManager_burn_nonEmpty_native.snap
@@ -1 +1 @@
-122913
+122944
diff --git a/.forge-snapshots/PositionManager_collect.snap b/.forge-snapshots/PositionManager_collect.snap
@@ -1 +1 @@
-150181
+150221
diff --git a/.forge-snapshots/PositionManager_collect_native.snap b/.forge-snapshots/PositionManager_collect_native.snap
@@ -1 +1 @@
-141333
+141373
diff --git a/.forge-snapshots/PositionManager_collect_sameRange.snap b/.forge-snapshots/PositionManager_collect_sameRange.snap
@@ -1 +1 @@
-150181
+150221
diff --git a/.forge-snapshots/PositionManager_decreaseLiquidity.snap b/.forge-snapshots/PositionManager_decreaseLiquidity.snap
@@ -1 +1 @@
-115724
+115764
diff --git a/.forge-snapshots/PositionManager_decreaseLiquidity_native.snap b/.forge-snapshots/PositionManager_decreaseLiquidity_native.snap
@@ -1 +1 @@
-108541
+108573
diff --git a/.forge-snapshots/PositionManager_decrease_burnEmpty.snap b/.forge-snapshots/PositionManager_decrease_burnEmpty.snap
@@ -1 +1 @@
-134142
+134174
diff --git a/.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap b/.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap
@@ -1 +1 @@
-126881
+126913
diff --git a/.forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap b/.forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap
@@ -1 +1 @@
-128440
+128480
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_erc20.snap b/.forge-snapshots/PositionManager_increaseLiquidity_erc20.snap
@@ -1 +1 @@
-152319
+152359
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_native.snap b/.forge-snapshots/PositionManager_increaseLiquidity_native.snap
@@ -1 +1 @@
-134119
+134159
diff --git a/.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap b/.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap
@@ -1 +1 @@
-134800
+134840
diff --git a/.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap b/.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap
@@ -1 +1 @@
-170956
+170996
diff --git a/.forge-snapshots/PositionManager_mint.snap b/.forge-snapshots/PositionManager_mint.snap
@@ -1 +1 @@
-372231
+372271
diff --git a/.forge-snapshots/PositionManager_mint_native.snap b/.forge-snapshots/PositionManager_mint_native.snap
@@ -1 +1 @@
-336931
+336971
diff --git a/.forge-snapshots/PositionManager_mint_nativeWithSweep.snap b/.forge-snapshots/PositionManager_mint_nativeWithSweep.snap
@@ -1 +1 @@
-345575
+345615
diff --git a/.forge-snapshots/PositionManager_mint_onSameTickLower.snap b/.forge-snapshots/PositionManager_mint_onSameTickLower.snap
@@ -1 +1 @@
-314913
+314953
diff --git a/.forge-snapshots/PositionManager_mint_onSameTickUpper.snap b/.forge-snapshots/PositionManager_mint_onSameTickUpper.snap
@@ -1 +1 @@
-315555
+315595
diff --git a/.forge-snapshots/PositionManager_mint_sameRange.snap b/.forge-snapshots/PositionManager_mint_sameRange.snap
@@ -1 +1 @@
-241137
+241177
diff --git a/.forge-snapshots/PositionManager_mint_settleWithBalance_sweep.snap b/.forge-snapshots/PositionManager_mint_settleWithBalance_sweep.snap
@@ -1 +1 @@
-370413
+370453
diff --git a/.forge-snapshots/PositionManager_mint_warmedPool_differentRange.snap b/.forge-snapshots/PositionManager_mint_warmedPool_differentRange.snap
@@ -1 +1 @@
-320931
+320971
diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap
@@ -1 +1 @@
-416619
+416659
diff --git a/src/PositionManager.sol b/src/PositionManager.sol
@@ -25,6 +25,7 @@ import {PositionConfig, PositionConfigLibrary} from "./libraries/PositionConfig.
 import {BaseActionsRouter} from "./base/BaseActionsRouter.sol";
 import {Actions} from "./libraries/Actions.sol";
 import {CalldataDecoder} from "./libraries/CalldataDecoder.sol";
+import {SlippageCheckLibrary} from "./libraries/SlippageCheck.sol";
 
 contract PositionManager is
     IPositionManager,
@@ -43,6 +44,7 @@ contract PositionManager is
     using TransientStateLibrary for IPoolManager;
     using SafeCast for uint256;
     using CalldataDecoder for bytes;
+    using SlippageCheckLibrary for BalanceDelta;
 
     /// @dev The ID of the next token that will be minted. Skips 0
     uint256 public nextTokenId = 1;
@@ -146,8 +148,7 @@ contract PositionManager is
         if (positionConfigs[tokenId] != config.toId()) revert IncorrectPositionConfigForTokenId(tokenId);
         // Note: The tokenId is used as the salt for this position, so every minted position has unique storage in the pool manager.
         BalanceDelta liquidityDelta = _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
-        if (liquidityDelta.amount0() < 0 && amount0Max < uint128(-liquidityDelta.amount0())) revert SlippageExceeded();
-        if (liquidityDelta.amount1() < 0 && amount1Max < uint128(-liquidityDelta.amount1())) revert SlippageExceeded();
+        liquidityDelta.validateMaximumIncreaseSlippage(amount0Max, amount1Max);
     }
 
     /// @dev Calling decrease with 0 liquidity will credit the caller with any underlying fees of the position
@@ -164,9 +165,7 @@ contract PositionManager is
 
         // Note: the tokenId is used as the salt.
         BalanceDelta liquidityDelta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
-        if (uint128(liquidityDelta.amount0()) < amount0Min || uint128(liquidityDelta.amount1()) < amount1Min) {
-            revert SlippageExceeded();
-        }
+        liquidityDelta.validateMinimumOut(amount0Min, amount1Min);
     }
 
     function _mint(
@@ -187,8 +186,7 @@ contract PositionManager is
 
         // _beforeModify is not called here because the tokenId is newly minted
         BalanceDelta liquidityDelta = _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
-        if (amount0Max < uint128(-liquidityDelta.amount0())) revert SlippageExceeded();
-        if (amount1Max < uint128(-liquidityDelta.amount1())) revert SlippageExceeded();
+        liquidityDelta.validateMaximumIn(amount0Max, amount1Max);
         positionConfigs[tokenId] = config.toId();
     }
 
@@ -228,9 +226,7 @@ contract PositionManager is
         // Can only call modify if there is non zero liquidity.
         if (liquidity > 0) {
             liquidityDelta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
-            if (uint128(liquidityDelta.amount0()) < amount0Min || uint128(liquidityDelta.amount1()) < amount1Min) {
-                revert SlippageExceeded();
-            }
+            liquidityDelta.validateMinimumOut(amount0Min, amount1Min);
         }
 
         delete positionConfigs[tokenId];

diff --git a/src/interfaces/IPositionManager.sol b/src/interfaces/IPositionManager.sol
@@ -5,7 +5,6 @@ import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol";
 
 interface IPositionManager {
     error NotApproved(address caller);
-    error SlippageExceeded();
     error DeadlinePassed();
     error IncorrectPositionConfigForTokenId(uint256 tokenId);
 

diff --git a/src/libraries/SlippageCheck.sol b/src/libraries/SlippageCheck.sol
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+pragma solidity ^0.8.0;
+
+import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
+
+/// @title Slippage Check Library
+/// @notice a library for checking if a delta exceeds a maximum ceiling or fails to meet a minimum floor
+library SlippageCheckLibrary {
+    error MaximumAmountExceeded();
+    error MinimumAmountInsufficient();
+
+    /// @notice Revert if one or both deltas does not meet a minimum output
+    /// @dev to be used when removing liquidity to guarantee a minimum output
+    function validateMinimumOut(BalanceDelta delta, uint128 amount0Min, uint128 amount1Min) internal pure {
+        if (uint128(delta.amount0()) < amount0Min || uint128(delta.amount1()) < amount1Min) {
+            revert MinimumAmountInsufficient();
+        }
+    }
+
+    /// @notice Revert if one or both deltas exceeds a maximum input
+    /// @dev to be used when minting liquidity to guarantee a maximum input
+    function validateMaximumIn(BalanceDelta delta, uint128 amount0Max, uint128 amount1Max) internal pure {
+        if (uint128(-delta.amount0()) > amount0Max || uint128(-delta.amount1()) > amount1Max) {
+            revert MaximumAmountExceeded();
+        }
+    }
+
+    /// @notice Revert if one or both deltas exceeds a maximum input
+    /// @dev When increasing liquidity, delta can be positive when excess fees need to be collected
+    /// in those cases, slippage checks are not required
+    function validateMaximumIncreaseSlippage(BalanceDelta delta, uint128 amount0Max, uint128 amount1Max)
+        internal
+        pure
+    {
+        if (
+            delta.amount0() < 0 && amount0Max < uint128(-delta.amount0())
+                || delta.amount1() < 0 && amount1Max < uint128(-delta.amount1())
+        ) revert MaximumAmountExceeded();
+    }
+}
diff --git a/test/position-managers/IncreaseLiquidity.t.sol b/test/position-managers/IncreaseLiquidity.t.sol
@@ -20,6 +20,7 @@ import {IERC20} from "forge-std/interfaces/IERC20.sol";
 
 import {PositionManager} from "../../src/PositionManager.sol";
 import {PositionConfig} from "../../src/libraries/PositionConfig.sol";
+import {SlippageCheckLibrary} from "../../src/libraries/SlippageCheck.sol";
 import {IPositionManager} from "../../src/interfaces/IPositionManager.sol";
 import {Actions} from "../../src/libraries/Actions.sol";
 import {Planner, Plan} from "../shared/Planner.sol";
@@ -378,7 +379,7 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
 
         // revert since amount0Max is too low
         bytes memory calls = getIncreaseEncoded(tokenId, config, 100e18, 1 wei, type(uint128).max, ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MaximumAmountExceeded.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -389,7 +390,7 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
 
         // revert since amount1Max is too low
         bytes memory calls = getIncreaseEncoded(tokenId, config, 100e18, type(uint128).max, 1 wei, ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MaximumAmountExceeded.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -437,7 +438,7 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
         swap(key, true, -10e18, ZERO_BYTES);
 
         bytes memory calls = getIncreaseEncoded(tokenId, config, newLiquidity, slippage, slippage, ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MaximumAmountExceeded.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 

diff --git a/test/position-managers/PositionManager.gas.t.sol b/test/position-managers/PositionManager.gas.t.sol
@@ -386,7 +386,9 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot {
             Actions.DECREASE_LIQUIDITY,
             abi.encode(tokenId, config, 10_000 ether, MIN_SLIPPAGE_DECREASE, MIN_SLIPPAGE_DECREASE, ZERO_BYTES)
         );
-        planner.add(Actions.BURN_POSITION, abi.encode(tokenId, config, 0 wei, 0 wei, ZERO_BYTES));
+        planner.add(
+            Actions.BURN_POSITION, abi.encode(tokenId, config, MIN_SLIPPAGE_DECREASE, MIN_SLIPPAGE_DECREASE, ZERO_BYTES)
+        );
 
         // We must include CLOSE commands.
         bytes memory calls = planner.finalizeModifyLiquidity(config.poolKey);

diff --git a/test/position-managers/PositionManager.t.sol b/test/position-managers/PositionManager.t.sol
@@ -23,6 +23,7 @@ import {IPositionManager} from "../../src/interfaces/IPositionManager.sol";
 import {Actions} from "../../src/libraries/Actions.sol";
 import {PositionManager} from "../../src/PositionManager.sol";
 import {PositionConfig} from "../../src/libraries/PositionConfig.sol";
+import {SlippageCheckLibrary} from "../../src/libraries/SlippageCheck.sol";
 import {BaseActionsRouter} from "../../src/base/BaseActionsRouter.sol";
 
 import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol";
@@ -184,15 +185,15 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
         PositionConfig memory config = PositionConfig({poolKey: key, tickLower: -120, tickUpper: 120});
 
         bytes memory calls = getMintEncoded(config, 1e18, 1 wei, MAX_SLIPPAGE_INCREASE, address(this), ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MaximumAmountExceeded.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
     function test_mint_slippage_revertAmount1() public {
         PositionConfig memory config = PositionConfig({poolKey: key, tickLower: -120, tickUpper: 120});
 
         bytes memory calls = getMintEncoded(config, 1e18, MAX_SLIPPAGE_INCREASE, 1 wei, address(this), ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MaximumAmountExceeded.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -235,7 +236,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
         // swap to move the price and cause a slippage revert
         swap(key, true, -1e18, ZERO_BYTES);
 
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MaximumAmountExceeded.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -346,7 +347,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
 
         bytes memory calls =
             getBurnEncoded(tokenId, config, uint128(-delta.amount0()) + 1 wei, MIN_SLIPPAGE_DECREASE, ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MinimumAmountInsufficient.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -358,7 +359,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
 
         bytes memory calls =
             getBurnEncoded(tokenId, config, MIN_SLIPPAGE_DECREASE, uint128(-delta.amount1()) + 1 wei, ZERO_BYTES);
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MinimumAmountInsufficient.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -393,7 +394,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
         // swap to move the price and cause a slippage revert
         swap(key, true, -1e18, ZERO_BYTES);
 
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MinimumAmountInsufficient.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -470,7 +471,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
         bytes memory calls = getDecreaseEncoded(
             tokenId, config, 1e18, uint128(-delta.amount0()) + 1 wei, MIN_SLIPPAGE_DECREASE, ZERO_BYTES
         );
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MinimumAmountInsufficient.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -483,7 +484,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
         bytes memory calls = getDecreaseEncoded(
             tokenId, config, 1e18, MIN_SLIPPAGE_DECREASE, uint128(-delta.amount1()) + 1 wei, ZERO_BYTES
         );
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MinimumAmountInsufficient.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }
 
@@ -519,7 +520,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers {
         // swap to move the price and cause a slippage revert
         swap(key, true, -1e18, ZERO_BYTES);
 
-        vm.expectRevert(IPositionManager.SlippageExceeded.selector);
+        vm.expectRevert(SlippageCheckLibrary.MinimumAmountInsufficient.selector);
         lpm.modifyLiquidities(calls, _deadline);
     }