From a9301b8c989586141405f3d59695ac85b17b70f8 Mon Sep 17 00:00:00 2001 From: ChefMist <133624774+ChefMist@users.noreply.github.com> Date: Wed, 8 May 2024 12:56:44 +0800 Subject: [PATCH] [Part 1 of 2] feat: minor tweaks (#30) * chore: minor gas opt safeCast * test: update test * gas: updated forge gas --- ...oolManagerTest#addLiquidity_fromEmpty.snap | 2 +- ...ManagerTest#addLiquidity_fromNonEmpty.snap | 2 +- ...lManagerTest#addLiquidity_nativeToken.snap | 2 +- ...anagerTest#removeLiquidity_toNonEmpty.snap | 2 +- ...oolManagerTest#swap_runOutOfLiquidity.snap | 2 +- ...CLPositionTest#Position_update_remove.snap | 2 +- .../LiquidityMathTest#addDeltaNegtive.snap | 2 +- .../LiquidityMathTest#addDeltaPositive.snap | 2 +- .forge-snapshots/TickTest#update.snap | 2 +- src/pool-cl/libraries/LiquidityMath.sol | 13 +-- test/libraries/BalanceDelta.t.sol | 87 +++++++++++++++++-- test/pool-cl/CLPoolManager.t.sol | 3 +- test/pool-cl/libraries/CLPool.t.sol | 2 +- test/pool-cl/libraries/CLPosition.t.sol | 3 +- test/pool-cl/libraries/LiquidityMath.t.sol | 7 +- 15 files changed, 105 insertions(+), 28 deletions(-) diff --git a/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap b/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap index 0797313d..5441f030 100644 --- a/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap +++ b/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap @@ -1 +1 @@ -401866 \ No newline at end of file +401422 \ No newline at end of file diff --git a/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap b/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap index 2e48818b..87bb66dc 100644 --- a/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap +++ b/.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap @@ -1 +1 @@ -182854 \ No newline at end of file +182410 \ No newline at end of file diff --git a/.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap b/.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap index 4016b72f..7ada3d1d 100644 --- a/.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap +++ b/.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap @@ -1 +1 @@ -247993 \ No newline at end of file +247549 \ No newline at end of file diff --git a/.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap b/.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap index 63e482c1..887d205b 100644 --- a/.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap +++ b/.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap @@ -1 +1 @@ -129601 \ No newline at end of file +128995 \ No newline at end of file diff --git a/.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap b/.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap index 3a7673b9..134ba728 100644 --- a/.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap +++ b/.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap @@ -1 +1 @@ -24940941 \ No newline at end of file +24940739 \ No newline at end of file diff --git a/.forge-snapshots/CLPositionTest#Position_update_remove.snap b/.forge-snapshots/CLPositionTest#Position_update_remove.snap index da9ac390..870e31e0 100644 --- a/.forge-snapshots/CLPositionTest#Position_update_remove.snap +++ b/.forge-snapshots/CLPositionTest#Position_update_remove.snap @@ -1 +1 @@ -1899 \ No newline at end of file +1697 \ No newline at end of file diff --git a/.forge-snapshots/LiquidityMathTest#addDeltaNegtive.snap b/.forge-snapshots/LiquidityMathTest#addDeltaNegtive.snap index 0e92c3c0..d2c5ed21 100644 --- a/.forge-snapshots/LiquidityMathTest#addDeltaNegtive.snap +++ b/.forge-snapshots/LiquidityMathTest#addDeltaNegtive.snap @@ -1 +1 @@ -306 \ No newline at end of file +116 \ No newline at end of file diff --git a/.forge-snapshots/LiquidityMathTest#addDeltaPositive.snap b/.forge-snapshots/LiquidityMathTest#addDeltaPositive.snap index b7c52fb1..95c8a676 100644 --- a/.forge-snapshots/LiquidityMathTest#addDeltaPositive.snap +++ b/.forge-snapshots/LiquidityMathTest#addDeltaPositive.snap @@ -1 +1 @@ -212 \ No newline at end of file +113 \ No newline at end of file diff --git a/.forge-snapshots/TickTest#update.snap b/.forge-snapshots/TickTest#update.snap index 7eff560d..7b7e6841 100644 --- a/.forge-snapshots/TickTest#update.snap +++ b/.forge-snapshots/TickTest#update.snap @@ -1 +1 @@ -134926 \ No newline at end of file +134819 \ No newline at end of file diff --git a/src/pool-cl/libraries/LiquidityMath.sol b/src/pool-cl/libraries/LiquidityMath.sol index 49e00cc6..c2ae640b 100644 --- a/src/pool-cl/libraries/LiquidityMath.sol +++ b/src/pool-cl/libraries/LiquidityMath.sol @@ -9,11 +9,14 @@ library LiquidityMath { /// @param y The delta by which liquidity should be changed /// @return z The liquidity delta function addDelta(uint128 x, int128 y) internal pure returns (uint128 z) { - // after 0.8.0 overflow is checked by default - if (y < 0) { - z = x - uint128(-y); - } else { - z = x + uint128(y); + assembly { + z := add(x, y) + + if shr(128, z) { + // store 0x93dafdf1, error SafeCastOverflow at memory 0 address and revert from pointer 28, to byte 32 + mstore(0x0, 0x93dafdf1) + revert(0x1c, 0x04) + } } } } diff --git a/test/libraries/BalanceDelta.t.sol b/test/libraries/BalanceDelta.t.sol index dc1dcf5b..f611d6a2 100644 --- a/test/libraries/BalanceDelta.t.sol +++ b/test/libraries/BalanceDelta.t.sol @@ -5,7 +5,7 @@ import {Test} from "forge-std/Test.sol"; import {BalanceDelta, toBalanceDelta} from "../../src/types/BalanceDelta.sol"; contract TestBalanceDelta is Test { - function testToBalanceDelta() public { + function test_toBalanceDelta() public { BalanceDelta balanceDelta = toBalanceDelta(0, 0); assertEq(balanceDelta.amount0(), 0); assertEq(balanceDelta.amount1(), 0); @@ -27,17 +27,52 @@ contract TestBalanceDelta is Test { assertEq(balanceDelta.amount1(), type(int128).min); } - function testToBalanceDelta(int128 x, int128 y) public { + function test_fuzz_toBalanceDelta(int128 x, int128 y) public { + BalanceDelta balanceDelta = toBalanceDelta(x, y); + int256 expectedBD = int256(uint256(bytes32(abi.encodePacked(x, y)))); + assertEq(BalanceDelta.unwrap(balanceDelta), expectedBD); + } + + function test_fuzz_amount0_amount1(int128 x, int128 y) public { BalanceDelta balanceDelta = toBalanceDelta(x, y); assertEq(balanceDelta.amount0(), x); assertEq(balanceDelta.amount1(), y); } - function testAdd(int128 a, int128 b, int128 c, int128 d) public { + function test_add() public { + BalanceDelta balanceDelta = toBalanceDelta(0, 0) + toBalanceDelta(0, 0); + assertEq(balanceDelta.amount0(), 0); + assertEq(balanceDelta.amount1(), 0); + + balanceDelta = toBalanceDelta(-1000, 1000) + toBalanceDelta(1000, -1000); + assertEq(balanceDelta.amount0(), 0); + assertEq(balanceDelta.amount1(), 0); + + balanceDelta = + toBalanceDelta(type(int128).min, type(int128).max) + toBalanceDelta(type(int128).max, type(int128).min); + assertEq(balanceDelta.amount0(), -1); + assertEq(balanceDelta.amount1(), -1); + + balanceDelta = toBalanceDelta(type(int128).max / 2 + 1, type(int128).max / 2 + 1) + + toBalanceDelta(type(int128).max / 2, type(int128).max / 2); + assertEq(balanceDelta.amount0(), type(int128).max); + assertEq(balanceDelta.amount1(), type(int128).max); + } + + function test_add_revertsOnOverflow() public { + // should revert because type(int128).max + 1 is not possible + vm.expectRevert(); + toBalanceDelta(type(int128).max, 0) + toBalanceDelta(1, 0); + + vm.expectRevert(); + toBalanceDelta(0, type(int128).max) + toBalanceDelta(0, 1); + } + + function test_fuzz_add(int128 a, int128 b, int128 c, int128 d) public { int256 ac = int256(a) + c; int256 bd = int256(b) + d; - // make sure the addition doesn't overflow + // if the addition overflows it should revert if (ac != int128(ac) || bd != int128(bd)) { vm.expectRevert(); } @@ -47,16 +82,52 @@ contract TestBalanceDelta is Test { assertEq(balanceDelta.amount1(), bd); } - function testSub(int128 a, int128 b, int128 c, int128 d) public { + function test_sub() public { + BalanceDelta balanceDelta = toBalanceDelta(0, 0) - toBalanceDelta(0, 0); + assertEq(balanceDelta.amount0(), 0); + assertEq(balanceDelta.amount1(), 0); + + balanceDelta = toBalanceDelta(-1000, 1000) - toBalanceDelta(1000, -1000); + assertEq(balanceDelta.amount0(), -2000); + assertEq(balanceDelta.amount1(), 2000); + + balanceDelta = + toBalanceDelta(-1000, -1000) - toBalanceDelta(-(type(int128).min + 1000), -(type(int128).min + 1000)); + assertEq(balanceDelta.amount0(), type(int128).min); + assertEq(balanceDelta.amount1(), type(int128).min); + + balanceDelta = toBalanceDelta(type(int128).min / 2, type(int128).min / 2) + - toBalanceDelta(-(type(int128).min / 2), -(type(int128).min / 2)); + assertEq(balanceDelta.amount0(), type(int128).min); + assertEq(balanceDelta.amount1(), type(int128).min); + } + + function test_sub_revertsOnUnderflow() public { + // should revert because type(int128).min - 1 is not possible + vm.expectRevert(); + toBalanceDelta(type(int128).min, 0) - toBalanceDelta(1, 0); + + vm.expectRevert(); + toBalanceDelta(0, type(int128).min) - toBalanceDelta(0, 1); + } + + function test_fuzz_sub(int128 a, int128 b, int128 c, int128 d) public { int256 ac = int256(a) - c; int256 bd = int256(b) - d; - // make sure the subtraction doesn't underflow - vm.assume(ac == int128(ac)); - vm.assume(bd == int128(bd)); + // if the subtraction underflows it should revert + if (ac != int128(ac) || bd != int128(bd)) { + vm.expectRevert(); + } BalanceDelta balanceDelta = toBalanceDelta(a, b) - toBalanceDelta(c, d); assertEq(balanceDelta.amount0(), ac); assertEq(balanceDelta.amount1(), bd); } + + function test_fuzz_eq(int128 a, int128 b, int128 c, int128 d) public { + bool isEqual = (toBalanceDelta(a, b) == toBalanceDelta(c, d)); + if (a == c && b == d) assertTrue(isEqual); + else assertFalse(isEqual); + } } diff --git a/test/pool-cl/CLPoolManager.t.sol b/test/pool-cl/CLPoolManager.t.sol index 1fb0ac54..e6ac669a 100644 --- a/test/pool-cl/CLPoolManager.t.sol +++ b/test/pool-cl/CLPoolManager.t.sol @@ -34,6 +34,7 @@ import {ProtocolFeeControllerTest} from "./helpers/ProtocolFeeControllerTest.sol import {IProtocolFeeController} from "../../src/interfaces/IProtocolFeeController.sol"; import {CLFeeManagerHook} from "./helpers/CLFeeManagerHook.sol"; import {CLNoOpTestHook} from "./helpers/CLNoOpTestHook.sol"; +import {SafeCast} from "../../src/libraries/SafeCast.sol"; contract CLPoolManagerTest is Test, Deployers, TokenFixture, GasSnapshot { using PoolIdLibrary for PoolKey; @@ -984,7 +985,7 @@ contract CLPoolManagerTest is Test, Deployers, TokenFixture, GasSnapshot { IERC20(Currency.unwrap(currency0)).approve(address(router), 1e30 ether); IERC20(Currency.unwrap(currency1)).approve(address(router), 1e30 ether); - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(SafeCast.SafeCastOverflow.selector); router.modifyPosition( key, ICLPoolManager.ModifyLiquidityParams({tickLower: 46000, tickUpper: 46050, liquidityDelta: -1}), "" ); diff --git a/test/pool-cl/libraries/CLPool.t.sol b/test/pool-cl/libraries/CLPool.t.sol index 76c678da..5ce73acb 100644 --- a/test/pool-cl/libraries/CLPool.t.sol +++ b/test/pool-cl/libraries/CLPool.t.sol @@ -55,7 +55,7 @@ contract PoolTest is Test { } else if (params.tickUpper > TickMath.MAX_TICK) { vm.expectRevert(abi.encodeWithSelector(Tick.TickUpperOutOfBounds.selector, params.tickUpper)); } else if (params.liquidityDelta < 0) { - vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); + vm.expectRevert(SafeCast.SafeCastOverflow.selector); } else if (params.liquidityDelta == 0) { vm.expectRevert(CLPosition.CannotUpdateEmptyPosition.selector); } else if (params.liquidityDelta > int128(Tick.tickSpacingToMaxLiquidityPerTick(params.tickSpacing))) { diff --git a/test/pool-cl/libraries/CLPosition.t.sol b/test/pool-cl/libraries/CLPosition.t.sol index 45194f11..cd97adba 100644 --- a/test/pool-cl/libraries/CLPosition.t.sol +++ b/test/pool-cl/libraries/CLPosition.t.sol @@ -7,6 +7,7 @@ import {Test} from "forge-std/Test.sol"; import {CLPosition} from "../../../src/pool-cl/libraries/CLPosition.sol"; import {CLPool} from "../../../src/pool-cl/libraries/CLPool.sol"; import {FixedPoint128} from "../../../src/pool-cl/libraries/FixedPoint128.sol"; +import {SafeCast} from "../../../src/libraries/SafeCast.sol"; contract CLPositionTest is Test, GasSnapshot { using CLPosition for mapping(bytes32 => CLPosition.Info); @@ -31,7 +32,7 @@ contract CLPositionTest is Test, GasSnapshot { if (liquidityDelta == 0) { vm.expectRevert(CLPosition.CannotUpdateEmptyPosition.selector); } else if (liquidityDelta < 0) { - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(SafeCast.SafeCastOverflow.selector); } (uint256 feesOwed0, uint256 feesOwed1) = info.update(liquidityDelta, feeGrowthInside0X128, feeGrowthInside1X128); diff --git a/test/pool-cl/libraries/LiquidityMath.t.sol b/test/pool-cl/libraries/LiquidityMath.t.sol index cee9bd89..8562be9c 100644 --- a/test/pool-cl/libraries/LiquidityMath.t.sol +++ b/test/pool-cl/libraries/LiquidityMath.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.24; import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol"; import {stdError} from "forge-std/StdError.sol"; import {Test} from "forge-std/Test.sol"; +import {SafeCast} from "../../../src/libraries/SafeCast.sol"; import {LiquidityMath} from "../../../src/pool-cl/libraries/LiquidityMath.sol"; contract LiquidityMathTest is Test, GasSnapshot { @@ -22,16 +23,16 @@ contract LiquidityMathTest is Test, GasSnapshot { } function testAddDeltaOverflow() public { - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(SafeCast.SafeCastOverflow.selector); LiquidityMath.addDelta(2 ** 128 - 15, 15); } function testAddDeltaUnderflow() public { // underflow - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(SafeCast.SafeCastOverflow.selector); LiquidityMath.addDelta(0, -1); - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(SafeCast.SafeCastOverflow.selector); LiquidityMath.addDelta(3, -4); } }