A few cleanup tasks (#437)

* Fix compiler warnings * todo for mapping transient * fixing tests with fuzzing * remove console logs * Update PoolDonateTest.sol * remove amount overload --------- Co-authored-by: Sara Reynolds <[email protected]>
Uniswap · Dec 14, 2023 · b4c428c · b4c428c
1 parent 1a45074
commit b4c428c
Show file tree

Hide file tree

Showing 14 changed files with 145 additions and 239 deletions.
diff --git a/.forge-snapshots/mint with empty hook.snap b/.forge-snapshots/mint with empty hook.snap
@@ -1 +1 @@
-319005
+319063
diff --git a/.forge-snapshots/mint with native token.snap b/.forge-snapshots/mint with native token.snap
@@ -1 +1 @@
-201786
+201844
diff --git a/.forge-snapshots/mint.snap b/.forge-snapshots/mint.snap
@@ -1 +1 @@
-201706
+201764
diff --git a/foundry.toml b/foundry.toml
@@ -6,14 +6,13 @@ fs_permissions = [{ access = "read-write", path = ".forge-snapshots/"}, { access
 cancun = true
 
 [profile.default.fuzz]
-runs = 100
+runs = 1000
 seed = "0x4444"
 
 [profile.ci.fuzz]
-runs = 1000
+runs = 100000
 
 [profile.ci]
-fuzz_runs = 100000
 solc = "./bin/solc-static-linux"
 
 # See more config options https://github.com/foundry-rs/foundry/tree/master/config
diff --git a/src/PoolManager.sol b/src/PoolManager.sol
@@ -42,6 +42,7 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC6909Claims {
 
     /// @dev Represents the currencies due/owed to each locker.
     /// Must all net to zero when the last lock is released.
+    /// TODO this needs to be transient
     mapping(address locker => mapping(Currency currency => int256 currencyDelta)) public currencyDelta;
 
     /// @inheritdoc IPoolManager
@@ -78,13 +79,13 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC6909Claims {
         return pools[id].positions.get(_owner, tickLower, tickUpper).liquidity;
     }
 
-    function getPosition(PoolId id, address owner, int24 tickLower, int24 tickUpper)
+    function getPosition(PoolId id, address _owner, int24 tickLower, int24 tickUpper)
         external
         view
         override
         returns (Position.Info memory position)
     {
-        return pools[id].positions.get(owner, tickLower, tickUpper);
+        return pools[id].positions.get(_owner, tickLower, tickUpper);
     }
 
     /// @inheritdoc IPoolManager

diff --git a/src/test/PoolModifyPositionTest.sol b/src/test/PoolModifyPositionTest.sol
@@ -53,7 +53,7 @@ contract PoolModifyPositionTest is Test, PoolTestBase {
         (,,, int256 delta1) = _fetchBalances(data.key.currency1, data.sender);
 
         // These assertions only apply in non lock-accessing pools.
-        if (!data.key.hooks.hasPermissionToAccessLock()) {
+        if (!data.key.hooks.hasPermissionToAccessLock() && !data.key.fee.hasHookWithdrawFee()) {
             if (data.params.liquidityDelta > 0) {
                 assert(delta0 > 0 || delta1 > 0 || data.key.hooks.hasPermissionToNoOp());
                 assert(!(delta0 < 0 || delta1 < 0));

diff --git a/src/test/ProtocolFeeControllerTest.sol b/src/test/ProtocolFeeControllerTest.sol
@@ -28,22 +28,22 @@ contract ProtocolFeeControllerTest is IProtocolFeeController {
 
 /// @notice Reverts on call
 contract RevertingProtocolFeeControllerTest is IProtocolFeeController {
-    function protocolFeesForPool(PoolKey memory /* key */ ) external view returns (uint24) {
+    function protocolFeesForPool(PoolKey memory /* key */ ) external pure returns (uint24) {
         revert();
     }
 }
 
 /// @notice Returns an out of bounds protocol fee
 contract OutOfBoundsProtocolFeeControllerTest is IProtocolFeeController {
-    function protocolFeesForPool(PoolKey memory /* key */ ) external view returns (uint24) {
+    function protocolFeesForPool(PoolKey memory /* key */ ) external pure returns (uint24) {
         // set both swap and withdraw fees to 1, which is less than MIN_PROTOCOL_FEE_DENOMINATOR
         return 0x001001;
     }
 }
 
 /// @notice Return a value that overflows a uint24
 contract OverflowProtocolFeeControllerTest is IProtocolFeeController {
-    function protocolFeesForPool(PoolKey memory /* key */ ) external view returns (uint24) {
+    function protocolFeesForPool(PoolKey memory /* key */ ) external pure returns (uint24) {
         assembly {
             let ptr := mload(0x40)
             mstore(ptr, 0xFFFFAAA001)

diff --git a/test/AccessLock.t.sol b/test/AccessLock.t.sol
diff --git a/test/Fees.t.sol b/test/Fees.t.sol
@@ -117,7 +117,7 @@ contract FeesTest is Test, Deployers, GasSnapshot {
     }
 
     function testInitializeHookSwapFee(uint16 fee) public {
-        vm.assume(fee < 2 ** 12);
+        fee = uint16(bound(fee, 0, (2 ** 12) - 1));
 
         (Pool.Slot0 memory slot0,,,) = manager.pools(key0.toId());
         assertEq(getSwapFee(slot0.hookFees), 0);
@@ -133,7 +133,7 @@ contract FeesTest is Test, Deployers, GasSnapshot {
     }
 
     function testInitializeHookWithdrawFee(uint16 fee) public {
-        vm.assume(fee < 2 ** 12);
+        fee = uint16(bound(fee, 0, (2 ** 12) - 1));
 
         (Pool.Slot0 memory slot0,,,) = manager.pools(key1.toId());
         assertEq(getWithdrawFee(slot0.hookFees), 0);
@@ -149,7 +149,8 @@ contract FeesTest is Test, Deployers, GasSnapshot {
     }
 
     function testInitializeBothHookFee(uint16 swapFee, uint16 withdrawFee) public {
-        vm.assume(swapFee < 2 ** 12 && withdrawFee < 2 ** 12);
+        swapFee = uint16(bound(swapFee, 0, (2 ** 12) - 1));
+        withdrawFee = uint16(bound(withdrawFee, 0, (2 ** 12) - 1));
 
         (Pool.Slot0 memory slot0,,,) = manager.pools(key2.toId());
         assertEq(getSwapFee(slot0.hookFees), 0);
@@ -165,7 +166,8 @@ contract FeesTest is Test, Deployers, GasSnapshot {
     }
 
     function testInitializeHookProtocolSwapFee(uint16 hookSwapFee, uint16 protocolSwapFee) public {
-        vm.assume(hookSwapFee < 2 ** 12 && protocolSwapFee < 2 ** 12);
+        hookSwapFee = uint16(bound(hookSwapFee, 0, (2 ** 12) - 1));
+        protocolSwapFee = uint16(bound(protocolSwapFee, 0, (2 ** 12) - 1));
 
         (Pool.Slot0 memory slot0,,,) = manager.pools(key0.toId());
         assertEq(getSwapFee(slot0.hookFees), 0);
@@ -199,10 +201,10 @@ contract FeesTest is Test, Deployers, GasSnapshot {
         uint16 protocolSwapFee,
         uint16 protocolWithdrawFee
     ) public {
-        vm.assume(
-            hookSwapFee < 2 ** 12 && hookWithdrawFee < 2 ** 12 && protocolSwapFee < 2 ** 12
-                && protocolWithdrawFee < 2 ** 12
-        );
+        hookSwapFee = uint16(bound(hookSwapFee, 0, (2 ** 12) - 1));
+        hookWithdrawFee = uint16(bound(hookWithdrawFee, 0, (2 ** 12) - 1));
+        protocolSwapFee = uint16(bound(protocolSwapFee, 0, (2 ** 12) - 1));
+        protocolWithdrawFee = uint16(bound(protocolWithdrawFee, 0, (2 ** 12) - 1));
 
         (Pool.Slot0 memory slot0,,,) = manager.pools(key2.toId());
         assertEq(getSwapFee(slot0.hookFees), 0);
@@ -243,11 +245,15 @@ contract FeesTest is Test, Deployers, GasSnapshot {
 
     function testProtocolFeeOnWithdrawalRemainsZeroIfNoHookWithdrawalFeeSet(
         uint16 hookSwapFee,
-        uint16 protocolWithdrawFee
+        uint8 protocolWithdrawFee0,
+        uint8 protocolWithdrawFee1
     ) public {
-        vm.assume(hookSwapFee < 2 ** 12 && protocolWithdrawFee < 2 ** 12);
-        vm.assume(protocolWithdrawFee >> 6 >= 4);
-        vm.assume(protocolWithdrawFee % 64 >= 4);
+        hookSwapFee = uint16(bound(hookSwapFee, 0, (2 ** 12) - 1));
+
+        protocolWithdrawFee0 = uint8(bound(protocolWithdrawFee0, 4, (2 ** 6) - 1));
+        protocolWithdrawFee1 = uint8(bound(protocolWithdrawFee1, 4, (2 ** 6) - 1));
+
+        uint16 protocolWithdrawFee = (uint16(protocolWithdrawFee0) << 6) | uint16(protocolWithdrawFee1);
 
         // On a pool whose hook has not set a withdraw fee, the protocol should not accrue any value even if it has set a withdraw fee.
         hook.setSwapFee(key0, hookSwapFee);
@@ -298,11 +304,16 @@ contract FeesTest is Test, Deployers, GasSnapshot {
     //     manager.setProtocolFees(key0);
     // }
 
-    function testHookWithdrawFeeProtocolWithdrawFee(uint16 hookWithdrawFee, uint16 protocolWithdrawFee) public {
-        vm.assume(protocolWithdrawFee < 2 ** 12);
-        vm.assume(hookWithdrawFee < 2 ** 12);
-        vm.assume(protocolWithdrawFee >> 6 >= 4);
-        vm.assume(protocolWithdrawFee % 64 >= 4);
+    function testHookWithdrawFeeProtocolWithdrawFee(
+        uint16 hookWithdrawFee,
+        uint8 protocolWithdrawFee0,
+        uint8 protocolWithdrawFee1
+    ) public {
+        hookWithdrawFee = uint16(bound(hookWithdrawFee, 0, (2 ** 12) - 1));
+        protocolWithdrawFee0 = uint8(bound(protocolWithdrawFee0, 4, (2 ** 6) - 1));
+        protocolWithdrawFee1 = uint8(bound(protocolWithdrawFee1, 4, (2 ** 6) - 1));
+
+        uint16 protocolWithdrawFee = (uint16(protocolWithdrawFee0) << 6) | uint16(protocolWithdrawFee1);
 
         hook.setWithdrawFee(key1, hookWithdrawFee);
         manager.setHookFees(key1);
@@ -355,13 +366,19 @@ contract FeesTest is Test, Deployers, GasSnapshot {
         assertEq(manager.hookFeesAccrued(address(key1.hooks), currency1), expectedHookFee1);
     }
 
-    function testNoHookProtocolFee(uint16 protocolSwapFee, uint16 protocolWithdrawFee) public {
-        vm.assume(protocolSwapFee < 2 ** 12 && protocolWithdrawFee < 2 ** 12);
+    function testNoHookProtocolFee(
+        uint8 protocolSwapFee0,
+        uint8 protocolSwapFee1,
+        uint8 protocolWithdrawFee0,
+        uint8 protocolWithdrawFee1
+    ) public {
+        protocolWithdrawFee0 = uint8(bound(protocolWithdrawFee0, 4, (2 ** 6) - 1));
+        protocolWithdrawFee1 = uint8(bound(protocolWithdrawFee1, 4, (2 ** 6) - 1));
+        protocolSwapFee0 = uint8(bound(protocolSwapFee0, 4, (2 ** 6) - 1));
+        protocolSwapFee1 = uint8(bound(protocolSwapFee1, 4, (2 ** 6) - 1));
 
-        vm.assume(protocolSwapFee >> 6 >= 4);
-        vm.assume(protocolSwapFee % 64 >= 4);
-        vm.assume(protocolWithdrawFee >> 6 >= 4);
-        vm.assume(protocolWithdrawFee % 64 >= 4);
+        uint16 protocolWithdrawFee = (uint16(protocolWithdrawFee0) << 6) | uint16(protocolWithdrawFee1);
+        uint16 protocolSwapFee = (uint16(protocolSwapFee1) << 6) | uint16(protocolSwapFee0);
 
         feeController.setSwapFeeForPool(key3.toId(), protocolSwapFee);
         feeController.setWithdrawFeeForPool(key3.toId(), protocolWithdrawFee);
@@ -386,8 +403,6 @@ contract FeesTest is Test, Deployers, GasSnapshot {
         IPoolManager.ModifyPositionParams memory params2 = IPoolManager.ModifyPositionParams(-60, 60, -liquidityDelta);
         modifyPositionRouter.modifyPosition(key3, params2, ZERO_BYTES);
 
-        uint16 protocolSwapFee1 = (protocolSwapFee >> 6);
-
         // No fees should accrue bc there is no hook so the protocol cant take withdraw fees.
         assertEq(manager.protocolFeesAccrued(currency0), 0);
         assertEq(manager.protocolFeesAccrued(currency1), 0);

diff --git a/test/Pool.t.sol b/test/Pool.t.sol
@@ -18,7 +18,8 @@ contract PoolTest is Test {
     Pool.State state;
 
     function testPoolInitialize(uint160 sqrtPriceX96, uint16 protocolFee, uint16 hookFee, uint24 dynamicFee) public {
-        vm.assume(protocolFee < 2 ** 12 && hookFee < 2 ** 12);
+        protocolFee = uint16(bound(protocolFee, 0, (2 ** 12) - 1));
+        hookFee = uint16(bound(hookFee, 0, (2 ** 12) - 1));
 
         if (sqrtPriceX96 < TickMath.MIN_SQRT_RATIO || sqrtPriceX96 >= TickMath.MAX_SQRT_RATIO) {
             vm.expectRevert(TickMath.InvalidSqrtRatio.selector);
@@ -45,8 +46,7 @@ contract PoolTest is Test {
 
     function testModifyPosition(uint160 sqrtPriceX96, Pool.ModifyPositionParams memory params) public {
         // Assumptions tested in PoolManager.t.sol
-        vm.assume(params.tickSpacing >= TickMath.MIN_TICK_SPACING);
-        vm.assume(params.tickSpacing <= TickMath.MAX_TICK_SPACING);
+        params.tickSpacing = int24(bound(params.tickSpacing, TickMath.MIN_TICK_SPACING, TickMath.MAX_TICK_SPACING));
 
         testPoolInitialize(sqrtPriceX96, 0, 0, 0);
 
@@ -91,9 +91,8 @@ contract PoolTest is Test {
 
     function testSwap(uint160 sqrtPriceX96, uint24 swapFee, Pool.SwapParams memory params) public {
         // Assumptions tested in PoolManager.t.sol
-        vm.assume(params.tickSpacing >= TickMath.MIN_TICK_SPACING);
-        vm.assume(params.tickSpacing <= TickMath.MAX_TICK_SPACING);
-        vm.assume(swapFee < 1000000);
+        params.tickSpacing = int24(bound(params.tickSpacing, TickMath.MIN_TICK_SPACING, TickMath.MAX_TICK_SPACING));
+        swapFee = uint24(bound(swapFee, 0, 999999));
 
         testPoolInitialize(sqrtPriceX96, 0, 0, 0);
         Pool.Slot0 memory slot0 = state.slot0;

diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol
@@ -84,8 +84,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     }
 
     function test_mint_succeedsIfInitialized(uint160 sqrtPriceX96) public {
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         vm.expectEmit(true, true, true, true);
         emit ModifyPosition(
@@ -100,8 +99,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     }
 
     function test_mint_succeedsForNativeTokensIfInitialized(uint160 sqrtPriceX96) public {
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         vm.expectEmit(true, true, true, true);
         emit ModifyPosition(
@@ -116,8 +114,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     }
 
     function test_mint_succeedsWithHooksIfInitialized(uint160 sqrtPriceX96) public {
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         address payable mockAddr =
             payable(address(uint160(Hooks.BEFORE_MODIFY_POSITION_FLAG | Hooks.AFTER_MODIFY_POSITION_FLAG)));
@@ -315,8 +312,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     }
 
     function test_swap_failsIfNotInitialized(uint160 sqrtPriceX96) public {
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         key.fee = 100;
         IPoolManager.SwapParams memory params =
@@ -638,8 +634,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     }
 
     function test_donate_failsIfNoLiquidity(uint160 sqrtPriceX96) public {
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         (key,) = initPool(currency0, currency1, IHooks(address(0)), 100, sqrtPriceX96, ZERO_BYTES);
 
@@ -772,8 +767,6 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     }
 
     function test_setProtocolFee_failsWithInvalidProtocolFeeControllers() public {
-        uint24 protocolFee = 4;
-
         (Pool.Slot0 memory slot0,,,) = manager.pools(key.toId());
         assertEq(slot0.protocolFees, 0);
 
@@ -834,8 +827,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
 
     function test_noop_gas(uint160 sqrtPriceX96) public {
         // Assumptions tested in Pool.t.sol
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         address payable hookAddr = payable(
             address(
@@ -875,8 +867,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
 
     function test_noop_succeedsOnAllActions(uint160 sqrtPriceX96) public {
         // Assumptions tested in Pool.t.sol
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         address payable hookAddr = payable(
             address(
@@ -925,8 +916,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
 
     function test_noop_failsOnUninitializedPools(uint160 sqrtPriceX96) public {
         // Assumptions tested in Pool.t.sol
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         address payable hookAddr = payable(
             address(
@@ -963,8 +953,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
 
     function test_noop_failsOnForbiddenFunctions(uint160 sqrtPriceX96) public {
         // Assumptions tested in Pool.t.sol
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         address payable hookAddr = payable(
             address(
@@ -1017,8 +1006,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
 
     function test_noop_failsWithoutNoOpFlag(uint160 sqrtPriceX96) public {
         // Assumptions tested in Pool.t.sol
-        vm.assume(sqrtPriceX96 >= TickMath.MIN_SQRT_RATIO);
-        vm.assume(sqrtPriceX96 < TickMath.MAX_SQRT_RATIO);
+        sqrtPriceX96 = uint160(bound(sqrtPriceX96, TickMath.MIN_SQRT_RATIO, TickMath.MAX_SQRT_RATIO - 1));
 
         address payable hookAddr = payable(
             address(uint160(Hooks.BEFORE_MODIFY_POSITION_FLAG | Hooks.BEFORE_SWAP_FLAG | Hooks.BEFORE_DONATE_FLAG))