From 75ffb0b2295b168cd6c27ecedd0c382559d7d3b4 Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Thu, 28 Mar 2024 10:43:07 -0300 Subject: [PATCH 01/18] PoolId test (#536) --- test/libraries/PoolId.t.sol | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 test/libraries/PoolId.t.sol diff --git a/test/libraries/PoolId.t.sol b/test/libraries/PoolId.t.sol new file mode 100644 index 000000000..fc8fc48d2 --- /dev/null +++ b/test/libraries/PoolId.t.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {PoolId, PoolIdLibrary} from "src/types/PoolId.sol"; +import {PoolKey} from "src/types/PoolKey.sol"; + +contract PoolIdTest is Test { + using PoolIdLibrary for PoolKey; + + function test_fuzz_toId(PoolKey memory poolKey) public { + bytes memory encodedKey = abi.encode(poolKey); + bytes32 expectedHash = keccak256(encodedKey); + assertEq(PoolId.unwrap(poolKey.toId()), expectedHash, "hashes not equal"); + } +} From 444ffe3fa634f5667e75429da988aa634ac44107 Mon Sep 17 00:00:00 2001 From: Sara Reynolds <30504811+snreynolds@users.noreply.github.com> Date: Thu, 28 Mar 2024 10:46:10 -0400 Subject: [PATCH 02/18] add swap fee lib tests (#534) * add swap fee lib tests * comments --- src/PoolManager.sol | 2 +- src/libraries/SwapFeeLibrary.sol | 2 +- test/libraries/SwapFeeLibrary.t.sol | 81 +++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 test/libraries/SwapFeeLibrary.t.sol diff --git a/src/PoolManager.sol b/src/PoolManager.sol index cfea4768c..0a6fb95df 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -112,7 +112,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim if (key.currency0 >= key.currency1) revert CurrenciesOutOfOrderOrEqual(); if (!key.hooks.isValidHookAddress(key.fee)) revert Hooks.HookAddressNotValid(address(key.hooks)); - uint24 swapFee = key.fee.getSwapFee(); + uint24 swapFee = key.fee.getInitialSwapFee(); key.hooks.beforeInitialize(key, sqrtPriceX96, hookData); diff --git a/src/libraries/SwapFeeLibrary.sol b/src/libraries/SwapFeeLibrary.sol index d90b8c654..7fec6095b 100644 --- a/src/libraries/SwapFeeLibrary.sol +++ b/src/libraries/SwapFeeLibrary.sol @@ -23,7 +23,7 @@ library SwapFeeLibrary { if (self > MAX_SWAP_FEE) revert FeeTooLarge(); } - function getSwapFee(uint24 self) internal pure returns (uint24 swapFee) { + function getInitialSwapFee(uint24 self) internal pure returns (uint24 swapFee) { // the initial fee for a dynamic fee pool is 0 if (self.isDynamicFee()) return 0; swapFee = self & STATIC_FEE_MASK; diff --git a/test/libraries/SwapFeeLibrary.t.sol b/test/libraries/SwapFeeLibrary.t.sol new file mode 100644 index 000000000..a1976e1dd --- /dev/null +++ b/test/libraries/SwapFeeLibrary.t.sol @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import "src/libraries/SwapFeeLibrary.sol"; +import "forge-std/Test.sol"; + +contract SwapFeeLibraryTest is Test { + function test_isDynamicFee_returnsTrue() public { + uint24 dynamicFee = 0x800000; + assertTrue(SwapFeeLibrary.isDynamicFee(dynamicFee)); + } + + function test_isDynamicFee_returnsTrue_forMaxValue() public { + uint24 dynamicFee = 0xFFFFFF; + assertTrue(SwapFeeLibrary.isDynamicFee(dynamicFee)); + } + + function test_isDynamicFee_returnsFalse() public { + uint24 dynamicFee = 0x7FFFFF; + assertFalse(SwapFeeLibrary.isDynamicFee(dynamicFee)); + } + + function test_fuzz_isDynamicFee(uint24 fee) public { + assertEq((fee >> 23 == 1), SwapFeeLibrary.isDynamicFee(fee)); + } + + function test_validate_doesNotRevertWithNoFee() public pure { + uint24 fee = 0; + SwapFeeLibrary.validate(fee); + } + + function test_validate_doesNotRevert() public pure { + uint24 fee = 500000; // 50% + SwapFeeLibrary.validate(fee); + } + + function test_validate_doesNotRevertWithMaxFee() public pure { + uint24 maxFee = 1000000; // 100% + SwapFeeLibrary.validate(maxFee); + } + + function test_validate_revertsWithFeeTooLarge() public { + uint24 fee = 1000001; + vm.expectRevert(SwapFeeLibrary.FeeTooLarge.selector); + SwapFeeLibrary.validate(fee); + } + + function test_fuzz_validate(uint24 fee) public { + if (fee > 1000000) { + vm.expectRevert(SwapFeeLibrary.FeeTooLarge.selector); + } + SwapFeeLibrary.validate(fee); + } + + function test_getInitialSwapFee_forStaticFeeIsCorrect() public { + uint24 staticFee = 3000; // 30 bps + assertEq(SwapFeeLibrary.getInitialSwapFee(staticFee), staticFee); + } + + function test_getInitialSwapFee_revertsWithFeeTooLarge_forStaticFee() public { + uint24 staticFee = 1000001; + vm.expectRevert(SwapFeeLibrary.FeeTooLarge.selector); + SwapFeeLibrary.getInitialSwapFee(staticFee); + } + + function test_getInitialSwapFee_forDynamicFeeIsZero() public { + uint24 dynamicFee = 0x800BB8; + assertEq(SwapFeeLibrary.getInitialSwapFee(dynamicFee), 0); + } + + function test_fuzz_getInitialSwapFee(uint24 fee) public { + if (fee >> 23 == 1) { + assertEq(SwapFeeLibrary.getInitialSwapFee(fee), 0); + } else if (fee > 1000000) { + vm.expectRevert(SwapFeeLibrary.FeeTooLarge.selector); + SwapFeeLibrary.getInitialSwapFee(fee); + } else { + assertEq(SwapFeeLibrary.getInitialSwapFee(fee), fee); + } + } +} From b230769238879e1d4f58ffa57a4696b0c390d188 Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Thu, 28 Mar 2024 12:12:26 -0300 Subject: [PATCH 03/18] Use solmate owned (#520) * Use solmate owned * Add onlyOwner test --------- Co-authored-by: Sara Reynolds <30504811+snreynolds@users.noreply.github.com> --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .../erc20 collect protocol fees.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .../native collect protocol fees.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap burn 6909 for input.snap | 2 +- .../swap burn native 6909 for input.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/Owned.sol | 29 ------------- src/PoolManager.sol | 1 - src/ProtocolFees.sol | 4 +- src/interfaces/IProtocolFees.sol | 2 + test/Owned.t.sol | 43 ------------------- test/PoolManager.t.sol | 15 +++++-- 29 files changed, 39 insertions(+), 101 deletions(-) delete mode 100644 src/Owned.sol delete mode 100644 test/Owned.t.sol diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index e2bd4e7ac..a5c8b7976 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265799 \ No newline at end of file +265795 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index 2e5687233..bbd4c5e60 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140642 \ No newline at end of file +140638 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index b36da47c1..bf6bedbf8 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145959 \ No newline at end of file +145955 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index 3829a9cc6..174ef3d97 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101654 \ No newline at end of file +101564 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index f59cb30b4..1f2f552d1 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -128667 \ No newline at end of file +128619 \ No newline at end of file diff --git a/.forge-snapshots/erc20 collect protocol fees.snap b/.forge-snapshots/erc20 collect protocol fees.snap index 36cf22341..b9fe27a3f 100644 --- a/.forge-snapshots/erc20 collect protocol fees.snap +++ b/.forge-snapshots/erc20 collect protocol fees.snap @@ -1 +1 @@ -24938 \ No newline at end of file +24961 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index df1ae0e82..56805387d 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51819 \ No newline at end of file +51797 \ No newline at end of file diff --git a/.forge-snapshots/native collect protocol fees.snap b/.forge-snapshots/native collect protocol fees.snap index 5d4c1b5c0..984502b79 100644 --- a/.forge-snapshots/native collect protocol fees.snap +++ b/.forge-snapshots/native collect protocol fees.snap @@ -1 +1 @@ -36611 \ No newline at end of file +36634 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 3cf278ca1..6c49d3b64 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23026 \ No newline at end of file +23071 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index 8c729b8ed..7f8f52f0d 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -56574 \ No newline at end of file +56486 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index ab62f35c6..8251926b6 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -148760 \ No newline at end of file +148672 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index 24c0b40b7..4f0ad04f6 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -150224 \ No newline at end of file +150136 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index df22d79b2..bd76bf91b 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -133409 \ No newline at end of file +133341 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index ba806de9b..360f528c3 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -147285 \ No newline at end of file +147217 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index addfbca9d..f03dcc1dc 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72868 \ No newline at end of file +72800 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index 386cc244e..6fa3193ed 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60871 \ No newline at end of file +60803 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 0061a05c6..84c55f1aa 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80985 \ No newline at end of file +80875 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index ab62c1cbb..953cdd6da 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76940 \ No newline at end of file +76830 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index 566df1219..ad7bf33b6 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139268 \ No newline at end of file +139223 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index 982481321..809e04d35 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -156077 \ No newline at end of file +156032 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index b978e8a3e..acc92877f 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -90176 \ No newline at end of file +90108 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index 160b64063..d9ad4853c 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60849 \ No newline at end of file +60781 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index 98aa37ccb..b362d08e8 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -141181 \ No newline at end of file +141091 \ No newline at end of file diff --git a/src/Owned.sol b/src/Owned.sol deleted file mode 100644 index 7f31fbab8..000000000 --- a/src/Owned.sol +++ /dev/null @@ -1,29 +0,0 @@ -// SPDX-License-Identifier: BUSL-1.1 -pragma solidity ^0.8.20; - -contract Owned { - address public owner; - bytes12 private STORAGE_PLACEHOLDER; - - error InvalidCaller(); - - /// @notice Emitted when the owner of the factory is changed - /// @param oldOwner The owner before the owner was changed - /// @param newOwner The owner after the owner was changed - event OwnerChanged(address indexed oldOwner, address indexed newOwner); - - modifier onlyOwner() { - if (msg.sender != owner) revert InvalidCaller(); - _; - } - - constructor() { - owner = msg.sender; - emit OwnerChanged(address(0), msg.sender); - } - - function setOwner(address _owner) external onlyOwner { - emit OwnerChanged(owner, _owner); - owner = _owner; - } -} diff --git a/src/PoolManager.sol b/src/PoolManager.sol index 0a6fb95df..9f7546dc2 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -10,7 +10,6 @@ import {Currency, CurrencyLibrary} from "./types/Currency.sol"; import {PoolKey} from "./types/PoolKey.sol"; import {TickMath} from "./libraries/TickMath.sol"; import {NoDelegateCall} from "./NoDelegateCall.sol"; -import {Owned} from "./Owned.sol"; import {IHooks} from "./interfaces/IHooks.sol"; import {IPoolManager} from "./interfaces/IPoolManager.sol"; import {IUnlockCallback} from "./interfaces/callback/IUnlockCallback.sol"; diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol index b70ad12b8..7d727b8ab 100644 --- a/src/ProtocolFees.sol +++ b/src/ProtocolFees.sol @@ -6,7 +6,7 @@ import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol"; import {IProtocolFees} from "./interfaces/IProtocolFees.sol"; import {Pool} from "./libraries/Pool.sol"; import {PoolKey} from "./types/PoolKey.sol"; -import {Owned} from "./Owned.sol"; +import {Owned} from "solmate/auth/Owned.sol"; abstract contract ProtocolFees is IProtocolFees, Owned { using CurrencyLibrary for Currency; @@ -19,7 +19,7 @@ abstract contract ProtocolFees is IProtocolFees, Owned { uint256 private immutable controllerGasLimit; - constructor(uint256 _controllerGasLimit) { + constructor(uint256 _controllerGasLimit) Owned(msg.sender) { controllerGasLimit = _controllerGasLimit; } diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol index 2113f17b5..943e064ac 100644 --- a/src/interfaces/IProtocolFees.sol +++ b/src/interfaces/IProtocolFees.sol @@ -10,6 +10,8 @@ interface IProtocolFees { error ProtocolFeeControllerCallFailedOrInvalidResult(); /// @notice Thrown when a pool does not have a dynamic fee. error FeeNotDynamic(); + /// @notice Thrown when collectProtocolFees is not called by the controller. + error InvalidCaller(); event ProtocolFeeControllerUpdated(address protocolFeeController); diff --git a/test/Owned.t.sol b/test/Owned.t.sol deleted file mode 100644 index 9bbeda0bf..000000000 --- a/test/Owned.t.sol +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.20; - -import {Test} from "forge-std/Test.sol"; -import {Vm} from "forge-std/Vm.sol"; -import {Owned} from "../src/Owned.sol"; - -contract OwnedTest is Test { - Owned owned; - - function testConstructor(address owner) public { - deployOwnedWithOwner(owner); - - assertEq(owner, owned.owner()); - } - - function testSetOwnerFromOwner(address oldOwner, address nextOwner) public { - // set the old owner as the owner - deployOwnedWithOwner(oldOwner); - - // old owner passes over ownership - vm.prank(oldOwner); - owned.setOwner(nextOwner); - assertEq(nextOwner, owned.owner()); - } - - function testSetOwnerFromNonOwner(address oldOwner, address nextOwner) public { - // set the old owner as the owner - deployOwnedWithOwner(oldOwner); - - if (oldOwner != nextOwner) { - vm.startPrank(nextOwner); - vm.expectRevert(Owned.InvalidCaller.selector); - owned.setOwner(nextOwner); - vm.stopPrank(); - } - } - - function deployOwnedWithOwner(address owner) internal { - vm.prank(owner); - owned = new Owned(); - } -} diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index d0e2125ad..ef8e67975 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -8,7 +8,6 @@ import {IPoolManager} from "../src/interfaces/IPoolManager.sol"; import {IProtocolFees} from "../src/interfaces/IProtocolFees.sol"; import {IProtocolFeeController} from "../src/interfaces/IProtocolFeeController.sol"; import {PoolManager} from "../src/PoolManager.sol"; -import {Owned} from "../src/Owned.sol"; import {TickMath} from "../src/libraries/TickMath.sol"; import {Pool} from "../src/libraries/Pool.sol"; import {Deployers} from "./utils/Deployers.sol"; @@ -69,7 +68,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { snapSize("poolManager bytecode size", address(manager)); } - function test_feeControllerSet() public { + function test_setProtocolFeeController_succeeds() public { deployFreshManager(); assertEq(address(manager.protocolFeeController()), address(0)); vm.expectEmit(false, false, false, true, address(manager)); @@ -78,6 +77,16 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { assertEq(address(manager.protocolFeeController()), address(feeController)); } + function test_setProtocolFeeController_failsIfNotOwner() public { + deployFreshManager(); + assertEq(address(manager.protocolFeeController()), address(0)); + + vm.prank(address(1)); // not the owner address + vm.expectRevert("UNAUTHORIZED"); + manager.setProtocolFeeController(feeController); + assertEq(address(manager.protocolFeeController()), address(0)); + } + function test_addLiquidity_failsIfNotInitialized() public { vm.expectRevert(Pool.PoolNotInitialized.selector); modifyLiquidityRouter.modifyLiquidity(uninitializedKey, LIQ_PARAMS, ZERO_BYTES); @@ -984,7 +993,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { } function test_collectProtocolFees_revertsIfCallerIsNotController() public { - vm.expectRevert(Owned.InvalidCaller.selector); + vm.expectRevert(IProtocolFees.InvalidCaller.selector); manager.collectProtocolFees(address(1), currency0, 0); } From ffb42d3d936a3bb4f270c44bff279e779e4f1812 Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Mon, 1 Apr 2024 12:20:49 -0300 Subject: [PATCH 04/18] BalanceDelta tests (#537) --- test/types/BalanceDelta.t.sol | 89 +++++++++++++++++++++++++++++++---- 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/test/types/BalanceDelta.t.sol b/test/types/BalanceDelta.t.sol index aafe83f2e..adfa9bec3 100644 --- a/test/types/BalanceDelta.t.sol +++ b/test/types/BalanceDelta.t.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {BalanceDelta, toBalanceDelta} from "../../src/types/BalanceDelta.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); + } } From 1dd936fdc2bfc02f68901d363cf83b716a171590 Mon Sep 17 00:00:00 2001 From: diana Date: Mon, 1 Apr 2024 13:31:32 -0400 Subject: [PATCH 05/18] update tests to use the IPoolManager (#531) * update tests to use the IPoolManager * change test name * use interface in amounthelpers and use getAmount1ForLiquidity for amount1 * move error declaration into pool manager interface * use correct key in test --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .../erc20 collect protocol fees.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .../native collect protocol fees.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/PoolManager.sol | 8 +++ src/interfaces/IPoolManager.sol | 9 ++++ src/interfaces/IProtocolFees.sol | 14 +++-- src/libraries/PoolGetters.sol | 8 +++ test/PoolManager.t.sol | 54 +++++++++---------- test/PoolManagerInitialize.t.sol | 49 ++++++++--------- test/utils/AmountHelpers.sol | 5 +- test/utils/Deployers.sol | 2 +- 26 files changed, 108 insertions(+), 77 deletions(-) diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index a5c8b7976..7e9c62cf8 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265795 \ No newline at end of file +265751 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index bbd4c5e60..946e57776 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140638 \ No newline at end of file +140594 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index bf6bedbf8..518c90533 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145955 \ No newline at end of file +145911 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index 174ef3d97..f0d1cb40d 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101564 \ No newline at end of file +101542 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index 1f2f552d1..d82a00f46 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -128619 \ No newline at end of file +132574 \ No newline at end of file diff --git a/.forge-snapshots/erc20 collect protocol fees.snap b/.forge-snapshots/erc20 collect protocol fees.snap index b9fe27a3f..562fd9896 100644 --- a/.forge-snapshots/erc20 collect protocol fees.snap +++ b/.forge-snapshots/erc20 collect protocol fees.snap @@ -1 +1 @@ -24961 \ No newline at end of file +24939 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index 56805387d..90c6c2fb0 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51797 \ No newline at end of file +51775 \ No newline at end of file diff --git a/.forge-snapshots/native collect protocol fees.snap b/.forge-snapshots/native collect protocol fees.snap index 984502b79..fec1b8b95 100644 --- a/.forge-snapshots/native collect protocol fees.snap +++ b/.forge-snapshots/native collect protocol fees.snap @@ -1 +1 @@ -36634 \ No newline at end of file +36612 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 6c49d3b64..a7c7206c5 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23071 \ No newline at end of file +23157 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index bd76bf91b..1264c8a97 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -133341 \ No newline at end of file +133319 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 360f528c3..2b5a2229c 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -147217 \ No newline at end of file +147195 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index f03dcc1dc..8b1d4669b 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72800 \ No newline at end of file +72778 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index 6fa3193ed..d9ad4853c 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60803 \ No newline at end of file +60781 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index ad7bf33b6..d321f6ecc 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139223 \ No newline at end of file +139201 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index 809e04d35..6b631cbfb 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -156032 \ No newline at end of file +156010 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index acc92877f..c4d64ba27 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -90108 \ No newline at end of file +90086 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index d9ad4853c..0aaebcccb 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60781 \ No newline at end of file +60759 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index b362d08e8..cdda320b8 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -141091 \ No newline at end of file +141069 \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index 9f7546dc2..0b6ee0a3f 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -342,4 +342,12 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim function getPoolBitmapInfo(PoolId id, int16 word) external view returns (uint256 tickBitmap) { return pools[id].getPoolBitmapInfo(word); } + + function getFeeGrowthGlobals(PoolId id) + external + view + returns (uint256 feeGrowthGlobal0x128, uint256 feeGrowthGlobal1x128) + { + return pools[id].getFeeGrowthGlobals(); + } } diff --git a/src/interfaces/IPoolManager.sol b/src/interfaces/IPoolManager.sol index 752afdfec..e5bd89ee7 100644 --- a/src/interfaces/IPoolManager.sol +++ b/src/interfaces/IPoolManager.sol @@ -36,6 +36,9 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { /// or on a pool that does not have a dynamic swap fee. error UnauthorizedDynamicSwapFeeUpdate(); + /// @notice Thrown when the call to fetch the protocol fee reverts or returns invalid data. + error ProtocolFeeControllerCallFailedOrInvalidResult(); + /// @notice Emitted when a new pool is initialized /// @param id The abi encoded hash of the pool key struct for the new pool /// @param currency0 The first currency of the pool by address sort order @@ -110,6 +113,12 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { /// @notice Getter for the bitmap given the poolId and word position function getPoolBitmapInfo(PoolId id, int16 word) external view returns (uint256 tickBitmap); + /// @notice Getter for the fee growth globals for the given poolId + function getFeeGrowthGlobals(PoolId id) + external + view + returns (uint256 feeGrowthGlobal0, uint256 feeGrowthGlobal1); + /// @notice Get the position struct for a specified pool and position function getPosition(PoolId id, address owner, int24 tickLower, int24 tickUpper) external diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol index 943e064ac..9024b23c1 100644 --- a/src/interfaces/IProtocolFees.sol +++ b/src/interfaces/IProtocolFees.sol @@ -2,14 +2,12 @@ pragma solidity ^0.8.19; import {Currency} from "../types/Currency.sol"; +import {IProtocolFeeController} from "./IProtocolFeeController.sol"; interface IProtocolFees { /// @notice Thrown when not enough gas is provided to look up the protocol fee error ProtocolFeeCannotBeFetched(); - /// @notice Thrown when the call to fetch the protocol fee reverts or returns invalid data. - error ProtocolFeeControllerCallFailedOrInvalidResult(); - /// @notice Thrown when a pool does not have a dynamic fee. - error FeeNotDynamic(); + /// @notice Thrown when collectProtocolFees is not called by the controller. error InvalidCaller(); @@ -20,4 +18,12 @@ interface IProtocolFees { /// @notice Given a currency address, returns the protocol fees accrued in that currency function protocolFeesAccrued(Currency) external view returns (uint256); + + /// @notice Sets the protocol fee controller + function setProtocolFeeController(IProtocolFeeController) external; + + /// @notice Collects the protocol fees for a given recipient and currency, returning the amount collected + function collectProtocolFees(address, Currency, uint256) external returns (uint256); + + function protocolFeeController() external view returns (IProtocolFeeController); } diff --git a/src/libraries/PoolGetters.sol b/src/libraries/PoolGetters.sol index 6a8eac5c3..a05761b34 100644 --- a/src/libraries/PoolGetters.sol +++ b/src/libraries/PoolGetters.sol @@ -11,4 +11,12 @@ library PoolGetters { function getPoolBitmapInfo(Pool.State storage pool, int16 word) internal view returns (uint256 tickBitmap) { return pool.tickBitmap[word]; } + + function getFeeGrowthGlobals(Pool.State storage pool) + internal + view + returns (uint256 feeGrowthGlobal0x128, uint256 feeGrowthGlobal1x128) + { + return (pool.feeGrowthGlobal0X128, pool.feeGrowthGlobal1X128); + } } diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index ef8e67975..7008daef3 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -759,8 +759,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { feeController.setSwapFeeForPool(key.toId(), protocolFee); manager.setProtocolFee(key); - (Pool.Slot0 memory slot0,,,) = manager.pools(key.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, protocolFee); // Add liquidity - Fees dont accrue for positive liquidity delta. IPoolManager.ModifyLiquidityParams memory params = LIQ_PARAMS; @@ -810,7 +810,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { // test successful donation if pool has liquidity function test_donate_succeedsWhenPoolHasLiquidity() public { - (, uint256 feeGrowthGlobal0X128, uint256 feeGrowthGlobal1X128,) = manager.pools(key.toId()); + (uint256 feeGrowthGlobal0X128, uint256 feeGrowthGlobal1X128) = manager.getFeeGrowthGlobals(key.toId()); assertEq(feeGrowthGlobal0X128, 0); assertEq(feeGrowthGlobal1X128, 0); @@ -818,19 +818,19 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { donateRouter.donate(key, 100, 200, ZERO_BYTES); snapEnd(); - (, feeGrowthGlobal0X128, feeGrowthGlobal1X128,) = manager.pools(key.toId()); + (feeGrowthGlobal0X128, feeGrowthGlobal1X128) = manager.getFeeGrowthGlobals(key.toId()); assertEq(feeGrowthGlobal0X128, 34028236692093846346337); assertEq(feeGrowthGlobal1X128, 68056473384187692692674); } function test_donate_succeedsForNativeTokensWhenPoolHasLiquidity() public { - (, uint256 feeGrowthGlobal0X128, uint256 feeGrowthGlobal1X128,) = manager.pools(nativeKey.toId()); + (uint256 feeGrowthGlobal0X128, uint256 feeGrowthGlobal1X128) = manager.getFeeGrowthGlobals(nativeKey.toId()); assertEq(feeGrowthGlobal0X128, 0); assertEq(feeGrowthGlobal1X128, 0); donateRouter.donate{value: 100}(nativeKey, 100, 200, ZERO_BYTES); - (, feeGrowthGlobal0X128, feeGrowthGlobal1X128,) = manager.pools(nativeKey.toId()); + (feeGrowthGlobal0X128, feeGrowthGlobal1X128) = manager.getFeeGrowthGlobals(nativeKey.toId()); assertEq(feeGrowthGlobal0X128, 34028236692093846346337); assertEq(feeGrowthGlobal1X128, 68056473384187692692674); } @@ -941,43 +941,43 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { } function test_setProtocolFee_updatesProtocolFeeForInitializedPool(uint16 protocolFee) public { - (Pool.Slot0 memory slot0,,,) = manager.pools(key.toId()); - assertEq(slot0.protocolFee, 0); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, 0); feeController.setSwapFeeForPool(key.toId(), protocolFee); uint8 fee0 = uint8(protocolFee >> 8); uint8 fee1 = uint8(protocolFee % 256); if ((0 < fee0 && fee0 < 4) || (0 < fee1 && fee1 < 4)) { - vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); } else { vm.expectEmit(false, false, false, true); emit ProtocolFeeUpdated(key.toId(), protocolFee); manager.setProtocolFee(key); - (slot0,,,) = manager.pools(key.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, protocolFee); } } function test_setProtocolFee_failsWithInvalidProtocolFeeControllers() public { - (Pool.Slot0 memory slot0,,,) = manager.pools(key.toId()); - assertEq(slot0.protocolFee, 0); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, 0); manager.setProtocolFeeController(revertingFeeController); - vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); manager.setProtocolFeeController(outOfBoundsFeeController); - vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); manager.setProtocolFeeController(overflowFeeController); - vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); manager.setProtocolFeeController(invalidReturnSizeFeeController); - vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); } @@ -988,8 +988,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { feeController.setSwapFeeForPool(uninitializedKey.toId(), uint16(protocolFee)); manager.initialize(uninitializedKey, SQRT_RATIO_1_1, ZERO_BYTES); - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0ProtocolFee, protocolFee); } function test_collectProtocolFees_revertsIfCallerIsNotController() public { @@ -1004,8 +1004,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { feeController.setSwapFeeForPool(key.toId(), uint16(protocolFee)); manager.setProtocolFee(key); - (Pool.Slot0 memory slot0,,,) = manager.pools(key.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, protocolFee); swapRouter.swap( key, @@ -1032,8 +1032,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { feeController.setSwapFeeForPool(key.toId(), uint16(protocolFee)); manager.setProtocolFee(key); - (Pool.Slot0 memory slot0,,,) = manager.pools(key.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, protocolFee); swapRouter.swap( key, @@ -1060,8 +1060,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { feeController.setSwapFeeForPool(nativeKey.toId(), uint16(protocolFee)); manager.setProtocolFee(nativeKey); - (Pool.Slot0 memory slot0,,,) = manager.pools(nativeKey.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId()); + assertEq(slot0ProtocolFee, protocolFee); swapRouter.swap{value: 10000}( nativeKey, @@ -1089,8 +1089,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { feeController.setSwapFeeForPool(nativeKey.toId(), uint16(protocolFee)); manager.setProtocolFee(nativeKey); - (Pool.Slot0 memory slot0,,,) = manager.pools(nativeKey.toId()); - assertEq(slot0.protocolFee, protocolFee); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId()); + assertEq(slot0ProtocolFee, protocolFee); swapRouter.swap{value: 10000}( nativeKey, diff --git a/test/PoolManagerInitialize.t.sol b/test/PoolManagerInitialize.t.sol index 56108d126..c19edd45f 100644 --- a/test/PoolManagerInitialize.t.sol +++ b/test/PoolManagerInitialize.t.sol @@ -78,9 +78,9 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { emit Initialize(key0.toId(), key0.currency0, key0.currency1, key0.fee, key0.tickSpacing, key0.hooks); manager.initialize(key0, sqrtPriceX96, ZERO_BYTES); - (Pool.Slot0 memory slot0,,,) = manager.pools(key0.toId()); - assertEq(slot0.sqrtPriceX96, sqrtPriceX96); - assertEq(slot0.protocolFee, 0); + (uint160 slot0SqrtPriceX96,, uint16 slot0ProtocolFee,) = manager.getSlot0(key0.toId()); + assertEq(slot0SqrtPriceX96, sqrtPriceX96); + assertEq(slot0ProtocolFee, 0); } } @@ -100,10 +100,11 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.sqrtPriceX96, sqrtPriceX96); - assertEq(slot0.protocolFee, 0); - assertEq(slot0.tick, TickMath.getTickAtSqrtRatio(sqrtPriceX96)); + (uint160 slot0SqrtPriceX96, int24 slot0Tick, uint16 slot0ProtocolFee,) = + manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0SqrtPriceX96, sqrtPriceX96); + assertEq(slot0ProtocolFee, 0); + assertEq(slot0Tick, TickMath.getTickAtSqrtRatio(sqrtPriceX96)); } function test_initialize_succeedsWithHooks(uint160 sqrtPriceX96) public { @@ -122,8 +123,8 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { uninitializedKey.hooks = IHooks(mockAddr); int24 tick = manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.sqrtPriceX96, sqrtPriceX96, "sqrtPrice"); + (uint160 slot0SqrtPriceX96,,,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0SqrtPriceX96, sqrtPriceX96, "sqrtPrice"); bytes32 beforeSelector = MockHooks.beforeInitialize.selector; bytes memory beforeParams = abi.encode(address(this), uninitializedKey, sqrtPriceX96, ZERO_BYTES); @@ -169,8 +170,8 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { uninitializedKey.hooks = mockHooks; manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.sqrtPriceX96, sqrtPriceX96); + (uint160 slot0SqrtPriceX96,,,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0SqrtPriceX96, sqrtPriceX96); } function test_initialize_revertsWithIdenticalTokens(uint160 sqrtPriceX96) public { @@ -204,12 +205,12 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { manager.initialize(uninitializedKey, SQRT_RATIO_1_1, ZERO_BYTES); - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.sqrtPriceX96, SQRT_RATIO_1_1); + (uint160 slot0SqrtPriceX96,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0SqrtPriceX96, SQRT_RATIO_1_1); if ((0 < fee0 && fee0 < 4) || (0 < fee1 && fee1 < 4)) { - assertEq(slot0.protocolFee, 0); + assertEq(slot0ProtocolFee, 0); } else { - assertEq(slot0.protocolFee, protocolFee); + assertEq(slot0ProtocolFee, protocolFee); } } @@ -316,10 +317,10 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.protocolFee, 0); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0ProtocolFee, 0); // call to setProtocolFee should also revert - vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(uninitializedKey); } @@ -340,8 +341,8 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.protocolFee, 0); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0ProtocolFee, 0); } function test_initialize_succeedsWithOverflowFeeController(uint160 sqrtPriceX96) public { @@ -361,8 +362,8 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.protocolFee, 0); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0ProtocolFee, 0); } function test_initialize_succeedsWithWrongReturnSizeFeeController(uint160 sqrtPriceX96) public { @@ -382,8 +383,8 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (Pool.Slot0 memory slot0,,,) = manager.pools(uninitializedKey.toId()); - assertEq(slot0.protocolFee, 0); + (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0ProtocolFee, 0); } function test_initialize_gas() public { diff --git a/test/utils/AmountHelpers.sol b/test/utils/AmountHelpers.sol index a956816ce..298fd3f77 100644 --- a/test/utils/AmountHelpers.sol +++ b/test/utils/AmountHelpers.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.20; import {LiquidityAmounts} from "./LiquidityAmounts.sol"; import {IPoolManager} from "../../src/interfaces/IPoolManager.sol"; -import {PoolManager} from "../../src/PoolManager.sol"; import {PoolId, PoolIdLibrary} from "../../src/types/PoolId.sol"; import {TickMath} from "../../src/libraries/TickMath.sol"; import {PoolKey} from "../../src/types/PoolKey.sol"; @@ -12,7 +11,7 @@ import {PoolKey} from "../../src/types/PoolKey.sol"; /// @notice Helps calculate amounts for bounding fuzz tests library AmountHelpers { function getMaxAmountInForPool( - PoolManager manager, + IPoolManager manager, IPoolManager.ModifyLiquidityParams memory params, PoolKey memory key ) public view returns (uint256 amount0, uint256 amount1) { @@ -24,6 +23,6 @@ library AmountHelpers { uint160 sqrtPriceX96Upper = TickMath.getSqrtRatioAtTick(params.tickUpper); amount0 = LiquidityAmounts.getAmount0ForLiquidity(sqrtPriceX96Lower, sqrtPriceX96, liquidity); - amount1 = LiquidityAmounts.getAmount0ForLiquidity(sqrtPriceX96Upper, sqrtPriceX96, liquidity); + amount1 = LiquidityAmounts.getAmount1ForLiquidity(sqrtPriceX96Upper, sqrtPriceX96, liquidity); } } diff --git a/test/utils/Deployers.sol b/test/utils/Deployers.sol index e87866101..13af1028c 100644 --- a/test/utils/Deployers.sol +++ b/test/utils/Deployers.sol @@ -52,7 +52,7 @@ contract Deployers { // Global variables Currency internal currency0; Currency internal currency1; - PoolManager manager; + IPoolManager manager; PoolModifyLiquidityTest modifyLiquidityRouter; PoolSwapTest swapRouter; PoolDonateTest donateRouter; From 45db18c582f1ff898c4e80fd42dec510f2907253 Mon Sep 17 00:00:00 2001 From: diana Date: Mon, 1 Apr 2024 21:42:49 -0400 Subject: [PATCH 06/18] Change protocol fee from denominator to bips (#504) * Change protocol fee from denominator to bips * switch protocol fee to uint24 * add comment and make a constant * protocol fee library * make 1e4 a constant * make constant clearer and remove masking * move logic to protocol fee library * save gas on initialize and remove unnecessary import * save gas * use correct assertions * remove withdraw comment --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .../erc20 collect protocol fees.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .../native collect protocol fees.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap burn 6909 for input.snap | 2 +- .../swap burn native 6909 for input.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/PoolManager.sol | 6 +- src/ProtocolFees.sol | 27 ++---- src/interfaces/IPoolManager.sol | 7 +- src/interfaces/IProtocolFeeController.sol | 2 +- src/interfaces/IProtocolFees.sol | 5 +- src/libraries/Pool.sol | 27 +++--- src/libraries/ProtocolFeeLibrary.sol | 30 ++++++ src/test/ProtocolFeeControllerTest.sol | 22 ++--- test/PoolManager.t.sol | 97 +++++++++---------- test/PoolManagerInitialize.t.sol | 28 +++--- test/libraries/Pool.t.sol | 4 +- 31 files changed, 154 insertions(+), 141 deletions(-) create mode 100644 src/libraries/ProtocolFeeLibrary.sol diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index 7e9c62cf8..a5c8b7976 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265751 \ No newline at end of file +265795 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index 946e57776..bbd4c5e60 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140594 \ No newline at end of file +140638 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index 518c90533..bf6bedbf8 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145911 \ No newline at end of file +145955 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index f0d1cb40d..174ef3d97 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101542 \ No newline at end of file +101564 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index d82a00f46..7d08b1b04 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -132574 \ No newline at end of file +132618 \ No newline at end of file diff --git a/.forge-snapshots/erc20 collect protocol fees.snap b/.forge-snapshots/erc20 collect protocol fees.snap index 562fd9896..b9fe27a3f 100644 --- a/.forge-snapshots/erc20 collect protocol fees.snap +++ b/.forge-snapshots/erc20 collect protocol fees.snap @@ -1 +1 @@ -24939 \ No newline at end of file +24961 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index 90c6c2fb0..7dbfdfbcb 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51775 \ No newline at end of file +51772 \ No newline at end of file diff --git a/.forge-snapshots/native collect protocol fees.snap b/.forge-snapshots/native collect protocol fees.snap index fec1b8b95..984502b79 100644 --- a/.forge-snapshots/native collect protocol fees.snap +++ b/.forge-snapshots/native collect protocol fees.snap @@ -1 +1 @@ -36612 \ No newline at end of file +36634 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index a7c7206c5..5344365e5 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23157 \ No newline at end of file +23114 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index 1264c8a97..cdb4e3f84 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -133319 \ No newline at end of file +133389 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 2b5a2229c..115c5c0ae 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -147195 \ No newline at end of file +147265 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 8b1d4669b..04e526e37 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72778 \ No newline at end of file +72848 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index d9ad4853c..afac93fdf 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60781 \ No newline at end of file +60851 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 84c55f1aa..994be676d 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80875 \ No newline at end of file +80881 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index 953cdd6da..811270183 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76830 \ No newline at end of file +76878 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index d321f6ecc..7f96b17cd 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139201 \ No newline at end of file +139229 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index 6b631cbfb..a8579dc7e 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -156010 \ No newline at end of file +156080 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index c4d64ba27..129249194 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -90086 \ No newline at end of file +90156 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index 0aaebcccb..bd043a483 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60759 \ No newline at end of file +60829 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index cdda320b8..30d350371 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -141069 \ No newline at end of file +141139 \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index 0b6ee0a3f..ef038af62 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -52,7 +52,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim external view override - returns (uint160 sqrtPriceX96, int24 tick, uint16 protocolFee, uint24 swapFee) + returns (uint160 sqrtPriceX96, int24 tick, uint24 protocolFee, uint24 swapFee) { Pool.Slot0 memory slot0 = pools[id].slot0; @@ -116,7 +116,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim key.hooks.beforeInitialize(key, sqrtPriceX96, hookData); PoolId id = key.toId(); - (, uint16 protocolFee) = _fetchProtocolFee(key); + (, uint24 protocolFee) = _fetchProtocolFee(key); tick = pools[id].initialize(sqrtPriceX96, protocolFee, swapFee); @@ -297,7 +297,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } function setProtocolFee(PoolKey memory key) external { - (bool success, uint16 newProtocolFee) = _fetchProtocolFee(key); + (bool success, uint24 newProtocolFee) = _fetchProtocolFee(key); if (!success) revert ProtocolFeeControllerCallFailedOrInvalidResult(); PoolId id = key.toId(); pools[id].setProtocolFee(newProtocolFee); diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol index 7d727b8ab..f9d913238 100644 --- a/src/ProtocolFees.sol +++ b/src/ProtocolFees.sol @@ -4,14 +4,13 @@ pragma solidity ^0.8.19; import {Currency, CurrencyLibrary} from "./types/Currency.sol"; import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol"; import {IProtocolFees} from "./interfaces/IProtocolFees.sol"; -import {Pool} from "./libraries/Pool.sol"; import {PoolKey} from "./types/PoolKey.sol"; +import {ProtocolFeeLibrary} from "./libraries/ProtocolFeeLibrary.sol"; import {Owned} from "solmate/auth/Owned.sol"; abstract contract ProtocolFees is IProtocolFees, Owned { using CurrencyLibrary for Currency; - - uint8 public constant MIN_PROTOCOL_FEE_DENOMINATOR = 4; + using ProtocolFeeLibrary for uint24; mapping(Currency currency => uint256) public protocolFeesAccrued; @@ -27,7 +26,7 @@ abstract contract ProtocolFees is IProtocolFees, Owned { /// @dev to prevent an invalid protocol fee controller from blocking pools from being initialized /// the success of this function is NOT checked on initialize and if the call fails, the protocol fees are set to 0. /// @dev the success of this function must be checked when called in setProtocolFee - function _fetchProtocolFee(PoolKey memory key) internal returns (bool success, uint16 protocolFees) { + function _fetchProtocolFee(PoolKey memory key) internal returns (bool success, uint24 protocolFees) { if (address(protocolFeeController) != address(0)) { // note that EIP-150 mandates that calls requesting more than 63/64ths of remaining gas // will be allotted no more than this amount, so controllerGasLimit must be set with this @@ -44,27 +43,13 @@ abstract contract ProtocolFees is IProtocolFees, Owned { assembly { returnData := mload(add(_data, 0x20)) } - // Ensure return data does not overflow a uint16 and that the underlying fees are within bounds. - (success, protocolFees) = returnData == uint16(returnData) && _isValidProtocolFee(uint16(returnData)) - ? (true, uint16(returnData)) + // Ensure return data does not overflow a uint24 and that the underlying fees are within bounds. + (success, protocolFees) = (returnData == uint24(returnData)) && uint24(returnData).validate() + ? (true, uint24(returnData)) : (false, 0); } } - function _isValidProtocolFee(uint16 fee) internal pure returns (bool) { - if (fee != 0) { - uint16 fee0 = fee % 256; - uint16 fee1 = fee >> 8; - // The fee is specified as a denominator so it cannot be LESS than the MIN_PROTOCOL_FEE_DENOMINATOR (unless it is 0). - if ( - (fee0 != 0 && fee0 < MIN_PROTOCOL_FEE_DENOMINATOR) || (fee1 != 0 && fee1 < MIN_PROTOCOL_FEE_DENOMINATOR) - ) { - return false; - } - } - return true; - } - function setProtocolFeeController(IProtocolFeeController controller) external onlyOwner { protocolFeeController = controller; emit ProtocolFeeControllerUpdated(address(controller)); diff --git a/src/interfaces/IPoolManager.sol b/src/interfaces/IPoolManager.sol index e5bd89ee7..f7b3954a1 100644 --- a/src/interfaces/IPoolManager.sol +++ b/src/interfaces/IPoolManager.sol @@ -36,9 +36,6 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { /// or on a pool that does not have a dynamic swap fee. error UnauthorizedDynamicSwapFeeUpdate(); - /// @notice Thrown when the call to fetch the protocol fee reverts or returns invalid data. - error ProtocolFeeControllerCallFailedOrInvalidResult(); - /// @notice Emitted when a new pool is initialized /// @param id The abi encoded hash of the pool key struct for the new pool /// @param currency0 The first currency of the pool by address sort order @@ -84,7 +81,7 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { uint24 fee ); - event ProtocolFeeUpdated(PoolId indexed id, uint16 protocolFee); + event ProtocolFeeUpdated(PoolId indexed id, uint24 protocolFee); /// @notice Returns the constant representing the maximum tickSpacing for an initialized pool key function MAX_TICK_SPACING() external view returns (int24); @@ -96,7 +93,7 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { function getSlot0(PoolId id) external view - returns (uint160 sqrtPriceX96, int24 tick, uint16 protocolFee, uint24 swapFee); + returns (uint160 sqrtPriceX96, int24 tick, uint24 protocolFee, uint24 swapFee); /// @notice Get the current value of liquidity of the given pool function getLiquidity(PoolId id) external view returns (uint128 liquidity); diff --git a/src/interfaces/IProtocolFeeController.sol b/src/interfaces/IProtocolFeeController.sol index fbe8799c8..7f2d72ae5 100644 --- a/src/interfaces/IProtocolFeeController.sol +++ b/src/interfaces/IProtocolFeeController.sol @@ -7,5 +7,5 @@ interface IProtocolFeeController { /// @notice Returns the protocol fees for a pool given the conditions of this contract /// @param poolKey The pool key to identify the pool. The controller may want to use attributes on the pool /// to determine the protocol fee, hence the entire key is needed. - function protocolFeeForPool(PoolKey memory poolKey) external view returns (uint16); + function protocolFeeForPool(PoolKey memory poolKey) external view returns (uint24); } diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol index 9024b23c1..a38a8f7b6 100644 --- a/src/interfaces/IProtocolFees.sol +++ b/src/interfaces/IProtocolFees.sol @@ -7,15 +7,14 @@ import {IProtocolFeeController} from "./IProtocolFeeController.sol"; interface IProtocolFees { /// @notice Thrown when not enough gas is provided to look up the protocol fee error ProtocolFeeCannotBeFetched(); + /// @notice Thrown when the call to fetch the protocol fee reverts or returns invalid data. + error ProtocolFeeControllerCallFailedOrInvalidResult(); /// @notice Thrown when collectProtocolFees is not called by the controller. error InvalidCaller(); event ProtocolFeeControllerUpdated(address protocolFeeController); - /// @notice Returns the minimum denominator for the protocol fee, which restricts it to a maximum of 25% - function MIN_PROTOCOL_FEE_DENOMINATOR() external view returns (uint8); - /// @notice Given a currency address, returns the protocol fees accrued in that currency function protocolFeesAccrued(Currency) external view returns (uint256); diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index 70652d799..0165360e6 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -10,6 +10,7 @@ import {TickMath} from "./TickMath.sol"; import {SqrtPriceMath} from "./SqrtPriceMath.sol"; import {SwapMath} from "./SwapMath.sol"; import {BalanceDelta, toBalanceDelta} from "../types/BalanceDelta.sol"; +import {ProtocolFeeLibrary} from "./ProtocolFeeLibrary.sol"; library Pool { using SafeCast for *; @@ -17,6 +18,7 @@ library Pool { using Position for mapping(bytes32 => Position.Info); using Position for Position.Info; using Pool for State; + using ProtocolFeeLibrary for uint24; /// @notice Thrown when tickLower is not below tickUpper /// @param tickLower The invalid tickLower @@ -64,11 +66,10 @@ library Pool { uint160 sqrtPriceX96; // the current tick int24 tick; - // protocol swap fee represented as integer denominator (1/x), taken as a % of the LP swap fee - // upper 8 bits are for 1->0, and the lower 8 are for 0->1 - // the minimum permitted denominator is 4 - meaning the maximum protocol fee is 25% - // granularity is increments of 0.38% (100/type(uint8).max) - uint16 protocolFee; + // protocol swap fee, taken as a % of the LP swap fee + // upper 12 bits are for 1->0, and the lower 12 are for 0->1 + // the maximum is 2500 - meaning the maximum protocol fee is 25% + uint24 protocolFee; // used for the swap fee, either static at initialize or dynamic via hook uint24 swapFee; } @@ -103,7 +104,7 @@ library Pool { if (tickUpper > TickMath.MAX_TICK) revert TickUpperOutOfBounds(tickUpper); } - function initialize(State storage self, uint160 sqrtPriceX96, uint16 protocolFee, uint24 swapFee) + function initialize(State storage self, uint160 sqrtPriceX96, uint24 protocolFee, uint24 swapFee) internal returns (int24 tick) { @@ -114,7 +115,7 @@ library Pool { self.slot0 = Slot0({sqrtPriceX96: sqrtPriceX96, tick: tick, protocolFee: protocolFee, swapFee: swapFee}); } - function setProtocolFee(State storage self, uint16 protocolFee) internal { + function setProtocolFee(State storage self, uint24 protocolFee) internal { if (self.isNotInitialized()) revert PoolNotInitialized(); self.slot0.protocolFee = protocolFee; @@ -252,7 +253,7 @@ library Pool { // liquidity at the beginning of the swap uint128 liquidityStart; // the protocol fee for the input token - uint8 protocolFee; + uint16 protocolFee; } // the top level state of the swap, the results of which are recorded in storage at the end @@ -323,7 +324,9 @@ library Pool { SwapCache memory cache = SwapCache({ liquidityStart: self.liquidity, - protocolFee: params.zeroForOne ? uint8(slot0Start.protocolFee % 256) : uint8(slot0Start.protocolFee >> 8) + protocolFee: params.zeroForOne + ? slot0Start.protocolFee.getZeroForOneFee() + : slot0Start.protocolFee.getOneForZeroFee() }); bool exactInput = params.amountSpecified < 0; @@ -383,9 +386,9 @@ library Pool { // if the protocol fee is on, calculate how much is owed, decrement feeAmount, and increment protocolFee if (cache.protocolFee > 0) { - // A: calculate the amount of the fee that should go to the protocol - uint256 delta = step.feeAmount / cache.protocolFee; - // A: subtract it from the regular fee and add it to the protocol fee + // calculate the amount of the fee that should go to the protocol + uint256 delta = step.feeAmount * cache.protocolFee / ProtocolFeeLibrary.BIPS_DENOMINATOR; + // subtract it from the regular fee and add it to the protocol fee unchecked { step.feeAmount -= delta; feeForProtocol += delta; diff --git a/src/libraries/ProtocolFeeLibrary.sol b/src/libraries/ProtocolFeeLibrary.sol new file mode 100644 index 000000000..4bddc6df1 --- /dev/null +++ b/src/libraries/ProtocolFeeLibrary.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.20; + +library ProtocolFeeLibrary { + // Max protocol fee is 25% (2500 bips) + uint16 public constant MAX_PROTOCOL_FEE = 2500; + + // Total bips + uint16 internal constant BIPS_DENOMINATOR = 10_000; + + function getZeroForOneFee(uint24 self) internal pure returns (uint16) { + return uint16(self % 4096); + } + + function getOneForZeroFee(uint24 self) internal pure returns (uint16) { + return uint16(self >> 12); + } + + function validate(uint24 self) internal pure returns (bool) { + if (self != 0) { + uint16 fee0 = getZeroForOneFee(self); + uint16 fee1 = getOneForZeroFee(self); + // The fee is represented in bips so it cannot be GREATER than the MAX_PROTOCOL_FEE. + if ((fee0 > MAX_PROTOCOL_FEE) || (fee1 > MAX_PROTOCOL_FEE)) { + return false; + } + } + return true; + } +} diff --git a/src/test/ProtocolFeeControllerTest.sol b/src/test/ProtocolFeeControllerTest.sol index 1b46080d8..30aec61a7 100644 --- a/src/test/ProtocolFeeControllerTest.sol +++ b/src/test/ProtocolFeeControllerTest.sol @@ -9,36 +9,36 @@ import {PoolKey} from "../types/PoolKey.sol"; contract ProtocolFeeControllerTest is IProtocolFeeController { using PoolIdLibrary for PoolKey; - mapping(PoolId => uint16) public swapFeeForPool; + mapping(PoolId => uint24) public protocolFee; - function protocolFeeForPool(PoolKey memory key) external view returns (uint16) { - return swapFeeForPool[key.toId()]; + function protocolFeeForPool(PoolKey memory key) external view returns (uint24) { + return protocolFee[key.toId()]; } // for tests to set pool protocol fees - function setSwapFeeForPool(PoolId id, uint16 fee) external { - swapFeeForPool[id] = fee; + function setProtocolFeeForPool(PoolId id, uint24 fee) external { + protocolFee[id] = fee; } } /// @notice Reverts on call contract RevertingProtocolFeeControllerTest is IProtocolFeeController { - function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint16) { + function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint24) { revert(); } } /// @notice Returns an out of bounds protocol fee contract OutOfBoundsProtocolFeeControllerTest is IProtocolFeeController { - function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint16) { - // set both swap and withdraw fees to 1, which is less than MIN_PROTOCOL_FEE_DENOMINATOR - return 0x001001; + function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint24) { + // set both swap fees to 2501, which is greater than MAX_PROTOCOL_FEE + return 0x9C59C5; } } /// @notice Return a value that overflows a uint24 contract OverflowProtocolFeeControllerTest is IProtocolFeeController { - function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint16) { + function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint24) { assembly { let ptr := mload(0x40) mstore(ptr, 0xFFFFAAA001) @@ -49,7 +49,7 @@ contract OverflowProtocolFeeControllerTest is IProtocolFeeController { /// @notice Returns data that is larger than a word contract InvalidReturnSizeProtocolFeeControllerTest is IProtocolFeeController { - function protocolFeeForPool(PoolKey memory /* key */ ) external view returns (uint16) { + function protocolFeeForPool(PoolKey memory /* key */ ) external view returns (uint24) { address a = address(this); assembly { let ptr := mload(0x40) diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index 7008daef3..73d6d86dc 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -29,12 +29,14 @@ import {Position} from "../src/libraries/Position.sol"; import {Constants} from "./utils/Constants.sol"; import {SafeCast} from "../src/libraries/SafeCast.sol"; import {AmountHelpers} from "./utils/AmountHelpers.sol"; +import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; contract PoolManagerTest is Test, Deployers, GasSnapshot { using Hooks for IHooks; using PoolIdLibrary for PoolKey; using SwapFeeLibrary for uint24; using CurrencyLibrary for Currency; + using ProtocolFeeLibrary for uint24; event UnlockCallback(); event ProtocolFeeControllerUpdated(address feeController); @@ -51,13 +53,15 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { int24 tick, uint24 fee ); - event ProtocolFeeUpdated(PoolId indexed id, uint16 protocolFee); + event ProtocolFeeUpdated(PoolId indexed id, uint24 protocolFee); event Transfer( address caller, address indexed sender, address indexed receiver, uint256 indexed id, uint256 amount ); PoolEmptyUnlockTest emptyUnlockRouter; + uint24 constant MAX_FEE_BOTH_TOKENS = (2500 << 12) | 2500; // 2500 2500 + function setUp() public { initializeManagerRoutersAndPoolsWithLiq(IHooks(address(0))); @@ -750,16 +754,16 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { snapEnd(); } - function test_swap_accruesProtocolFees(uint8 protocolFee1, uint8 protocolFee0) public { - protocolFee0 = uint8(bound(protocolFee0, 4, type(uint8).max)); - protocolFee1 = uint8(bound(protocolFee1, 4, type(uint8).max)); + function test_swap_accruesProtocolFees(uint16 protocolFee0, uint16 protocolFee1) public { + protocolFee0 = uint16(bound(protocolFee0, 1, 2500)); + protocolFee1 = uint16(bound(protocolFee1, 1, 2500)); - uint16 protocolFee = (uint16(protocolFee1) << 8) | (uint16(protocolFee0) & uint16(0xFF)); + uint24 protocolFee = (uint24(protocolFee1) << 12) | uint24(protocolFee0); - feeController.setSwapFeeForPool(key.toId(), protocolFee); + feeController.setProtocolFeeForPool(key.toId(), protocolFee); manager.setProtocolFee(key); - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId()); assertEq(slot0ProtocolFee, protocolFee); // Add liquidity - Fees dont accrue for positive liquidity delta. @@ -784,7 +788,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { swapRouter.swap(key, swapParams, PoolSwapTest.TestSettings(true, true, false), ZERO_BYTES); uint256 expectedTotalSwapFee = uint256(-swapParams.amountSpecified) * key.fee / 1e6; - uint256 expectedProtocolFee = expectedTotalSwapFee / protocolFee1; + uint256 expectedProtocolFee = expectedTotalSwapFee * protocolFee1 / 1e4; assertEq(manager.protocolFeesAccrued(currency0), 0); assertEq(manager.protocolFeesAccrued(currency1), expectedProtocolFee); } @@ -940,15 +944,15 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { manager.burn(address(this), key.currency0.toId(), 1); } - function test_setProtocolFee_updatesProtocolFeeForInitializedPool(uint16 protocolFee) public { - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + function test_setProtocolFee_updatesProtocolFeeForInitializedPool(uint24 protocolFee) public { + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId()); assertEq(slot0ProtocolFee, 0); - feeController.setSwapFeeForPool(key.toId(), protocolFee); + feeController.setProtocolFeeForPool(key.toId(), protocolFee); - uint8 fee0 = uint8(protocolFee >> 8); - uint8 fee1 = uint8(protocolFee % 256); - if ((0 < fee0 && fee0 < 4) || (0 < fee1 && fee1 < 4)) { - vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + uint16 fee0 = protocolFee.getZeroForOneFee(); + uint16 fee1 = protocolFee.getOneForZeroFee(); + if ((fee0 > 2500) || (fee1 > 2500)) { + vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); } else { vm.expectEmit(false, false, false, true); @@ -961,35 +965,32 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { } function test_setProtocolFee_failsWithInvalidProtocolFeeControllers() public { - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId()); assertEq(slot0ProtocolFee, 0); manager.setProtocolFeeController(revertingFeeController); - vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); manager.setProtocolFeeController(outOfBoundsFeeController); - vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); manager.setProtocolFeeController(overflowFeeController); - vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); manager.setProtocolFeeController(invalidReturnSizeFeeController); - vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(key); } function test_collectProtocolFees_initializesWithProtocolFeeIfCalled() public { - uint16 protocolFee = 1028; // 00000100 00000100 - - // sets the upper 12 bits - feeController.setSwapFeeForPool(uninitializedKey.toId(), uint16(protocolFee)); + feeController.setProtocolFeeForPool(uninitializedKey.toId(), MAX_FEE_BOTH_TOKENS); manager.initialize(uninitializedKey, SQRT_RATIO_1_1, ZERO_BYTES); - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); - assertEq(slot0ProtocolFee, protocolFee); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS); } function test_collectProtocolFees_revertsIfCallerIsNotController() public { @@ -998,18 +999,17 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { } function test_collectProtocolFees_ERC20_accumulateFees_gas() public { - uint16 protocolFee = 1028; // 00000100 00000100 uint256 expectedFees = 7; - feeController.setSwapFeeForPool(key.toId(), uint16(protocolFee)); + feeController.setProtocolFeeForPool(key.toId(), MAX_FEE_BOTH_TOKENS); manager.setProtocolFee(key); - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); - assertEq(slot0ProtocolFee, protocolFee); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS); swapRouter.swap( key, - IPoolManager.SwapParams(true, 10000, SQRT_RATIO_1_2), + IPoolManager.SwapParams(true, -10000, SQRT_RATIO_1_2), PoolSwapTest.TestSettings(true, true, false), ZERO_BYTES ); @@ -1026,42 +1026,40 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { } function test_collectProtocolFees_ERC20_returnsAllFeesIf0IsProvidedAsParameter() public { - uint16 protocolFee = 1028; // 00000100 00000100 uint256 expectedFees = 7; - feeController.setSwapFeeForPool(key.toId(), uint16(protocolFee)); + feeController.setProtocolFeeForPool(key.toId(), MAX_FEE_BOTH_TOKENS); manager.setProtocolFee(key); - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(key.toId()); - assertEq(slot0ProtocolFee, protocolFee); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId()); + assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS); swapRouter.swap( key, - IPoolManager.SwapParams(true, 10000, SQRT_RATIO_1_2), + IPoolManager.SwapParams(false, -10000, TickMath.MAX_SQRT_RATIO - 1), PoolSwapTest.TestSettings(true, true, false), ZERO_BYTES ); - assertEq(manager.protocolFeesAccrued(currency0), expectedFees); - assertEq(manager.protocolFeesAccrued(currency1), 0); - assertEq(currency0.balanceOf(address(1)), 0); - vm.prank(address(feeController)); - manager.collectProtocolFees(address(1), currency0, 0); - assertEq(currency0.balanceOf(address(1)), expectedFees); assertEq(manager.protocolFeesAccrued(currency0), 0); + assertEq(manager.protocolFeesAccrued(currency1), expectedFees); + assertEq(currency1.balanceOf(address(1)), 0); + vm.prank(address(feeController)); + manager.collectProtocolFees(address(1), currency1, 0); + assertEq(currency1.balanceOf(address(1)), expectedFees); + assertEq(manager.protocolFeesAccrued(currency1), 0); } function test_collectProtocolFees_nativeToken_accumulateFees_gas() public { - uint16 protocolFee = 1028; // 00000100 00000100 uint256 expectedFees = 7; Currency nativeCurrency = CurrencyLibrary.NATIVE; // set protocol fee before initializing the pool as it is fetched on initialization - feeController.setSwapFeeForPool(nativeKey.toId(), uint16(protocolFee)); + feeController.setProtocolFeeForPool(nativeKey.toId(), MAX_FEE_BOTH_TOKENS); manager.setProtocolFee(nativeKey); - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId()); - assertEq(slot0ProtocolFee, protocolFee); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId()); + assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS); swapRouter.swap{value: 10000}( nativeKey, @@ -1082,15 +1080,14 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { } function test_collectProtocolFees_nativeToken_returnsAllFeesIf0IsProvidedAsParameter() public { - uint16 protocolFee = 1028; // 00000100 00000100 uint256 expectedFees = 7; Currency nativeCurrency = CurrencyLibrary.NATIVE; - feeController.setSwapFeeForPool(nativeKey.toId(), uint16(protocolFee)); + feeController.setProtocolFeeForPool(nativeKey.toId(), MAX_FEE_BOTH_TOKENS); manager.setProtocolFee(nativeKey); - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId()); - assertEq(slot0ProtocolFee, protocolFee); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId()); + assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS); swapRouter.swap{value: 10000}( nativeKey, diff --git a/test/PoolManagerInitialize.t.sol b/test/PoolManagerInitialize.t.sol index c19edd45f..9d47632eb 100644 --- a/test/PoolManagerInitialize.t.sol +++ b/test/PoolManagerInitialize.t.sol @@ -21,11 +21,13 @@ import {PoolId, PoolIdLibrary} from "../src/types/PoolId.sol"; import {SwapFeeLibrary} from "../src/libraries/SwapFeeLibrary.sol"; import {ProtocolFeeControllerTest} from "../src/test/ProtocolFeeControllerTest.sol"; import {IProtocolFeeController} from "../src/interfaces/IProtocolFeeController.sol"; +import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { using Hooks for IHooks; using PoolIdLibrary for PoolKey; using SwapFeeLibrary for uint24; + using ProtocolFeeLibrary for uint24; event Initialize( PoolId indexed poolId, @@ -78,7 +80,7 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { emit Initialize(key0.toId(), key0.currency0, key0.currency1, key0.fee, key0.tickSpacing, key0.hooks); manager.initialize(key0, sqrtPriceX96, ZERO_BYTES); - (uint160 slot0SqrtPriceX96,, uint16 slot0ProtocolFee,) = manager.getSlot0(key0.toId()); + (uint160 slot0SqrtPriceX96,, uint24 slot0ProtocolFee,) = manager.getSlot0(key0.toId()); assertEq(slot0SqrtPriceX96, sqrtPriceX96); assertEq(slot0ProtocolFee, 0); } @@ -100,7 +102,7 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); - (uint160 slot0SqrtPriceX96, int24 slot0Tick, uint16 slot0ProtocolFee,) = + (uint160 slot0SqrtPriceX96, int24 slot0Tick, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); assertEq(slot0SqrtPriceX96, sqrtPriceX96); assertEq(slot0ProtocolFee, 0); @@ -196,18 +198,18 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); } - function test_initialize_fetchFeeWhenController(uint16 protocolFee) public { + function test_initialize_fetchFeeWhenController(uint24 protocolFee) public { manager.setProtocolFeeController(feeController); - feeController.setSwapFeeForPool(uninitializedKey.toId(), protocolFee); + feeController.setProtocolFeeForPool(uninitializedKey.toId(), protocolFee); - uint8 fee0 = uint8(protocolFee >> 8); - uint8 fee1 = uint8(protocolFee % 256); + uint16 fee0 = protocolFee.getZeroForOneFee(); + uint16 fee1 = protocolFee.getOneForZeroFee(); manager.initialize(uninitializedKey, SQRT_RATIO_1_1, ZERO_BYTES); - (uint160 slot0SqrtPriceX96,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + (uint160 slot0SqrtPriceX96,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); assertEq(slot0SqrtPriceX96, SQRT_RATIO_1_1); - if ((0 < fee0 && fee0 < 4) || (0 < fee1 && fee1 < 4)) { + if ((fee0 > 2500) || (fee1 > 2500)) { assertEq(slot0ProtocolFee, 0); } else { assertEq(slot0ProtocolFee, protocolFee); @@ -317,10 +319,10 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); assertEq(slot0ProtocolFee, 0); // call to setProtocolFee should also revert - vm.expectRevert(IPoolManager.ProtocolFeeControllerCallFailedOrInvalidResult.selector); + vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector); manager.setProtocolFee(uninitializedKey); } @@ -341,7 +343,7 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); assertEq(slot0ProtocolFee, 0); } @@ -362,7 +364,7 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); assertEq(slot0ProtocolFee, 0); } @@ -383,7 +385,7 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot { ); manager.initialize(uninitializedKey, sqrtPriceX96, ZERO_BYTES); // protocol fees should default to 0 - (,, uint16 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); + (,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId()); assertEq(slot0ProtocolFee, 0); } diff --git a/test/libraries/Pool.t.sol b/test/libraries/Pool.t.sol index 6874038a1..2af3df6a7 100644 --- a/test/libraries/Pool.t.sol +++ b/test/libraries/Pool.t.sol @@ -121,9 +121,9 @@ contract PoolTest is Test { state.swap(params); if (params.zeroForOne) { - assertLe(state.slot0.sqrtPriceX96, params.sqrtPriceLimitX96); - } else { assertGe(state.slot0.sqrtPriceX96, params.sqrtPriceLimitX96); + } else { + assertLe(state.slot0.sqrtPriceX96, params.sqrtPriceLimitX96); } } } From a4f6335d846a0e56aba50bea3b3a777e6df9a956 Mon Sep 17 00:00:00 2001 From: diana Date: Mon, 1 Apr 2024 22:10:56 -0400 Subject: [PATCH 07/18] Nonzero delta count test (#543) * remove unnecessary constants * add more asserts --- test/libraries/NonZeroDeltaCount.t.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/libraries/NonZeroDeltaCount.t.sol b/test/libraries/NonZeroDeltaCount.t.sol index 5dbcf34b0..523324f01 100644 --- a/test/libraries/NonZeroDeltaCount.t.sol +++ b/test/libraries/NonZeroDeltaCount.t.sol @@ -5,16 +5,16 @@ import {Test} from "forge-std/Test.sol"; import {NonZeroDeltaCount} from "src/libraries/NonZeroDeltaCount.sol"; contract NonZeroDeltaCountTest is Test { - address constant ADDRESS_AS = 0xaAaAaAaaAaAaAaaAaAAAAAAAAaaaAaAaAaaAaaAa; - address constant ADDRESS_BS = 0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB; - function test_incrementNonzeroDeltaCount() public { + assertEq(NonZeroDeltaCount.read(), 0); NonZeroDeltaCount.increment(); assertEq(NonZeroDeltaCount.read(), 1); } function test_decrementNonzeroDeltaCount() public { + assertEq(NonZeroDeltaCount.read(), 0); NonZeroDeltaCount.increment(); + assertEq(NonZeroDeltaCount.read(), 1); NonZeroDeltaCount.decrement(); assertEq(NonZeroDeltaCount.read(), 0); } @@ -22,6 +22,7 @@ contract NonZeroDeltaCountTest is Test { // Reading from right to left. Bit of 0: call increase. Bit of 1: call decrease. // The library allows over over/underflow so we dont check for that here function test_fuzz_nonZeroDeltaCount(uint256 instructions) public { + assertEq(NonZeroDeltaCount.read(), 0); uint256 expectedCount; for (uint256 i = 0; i < 256; i++) { if ((instructions & (1 << i)) == 0) { From 9f76d92c06f019d1ae60d1269b4d219bc70eaa0e Mon Sep 17 00:00:00 2001 From: Sara Reynolds <30504811+snreynolds@users.noreply.github.com> Date: Tue, 2 Apr 2024 14:38:42 -0400 Subject: [PATCH 08/18] coverage (#528) --- .github/workflows/coverage.yml | 89 ++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 .github/workflows/coverage.yml diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml new file mode 100644 index 000000000..4de459a7d --- /dev/null +++ b/.github/workflows/coverage.yml @@ -0,0 +1,89 @@ +name: code coverage + +on: + pull_request: + branches: + - main + +jobs: + comment-forge-coverage: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + + steps: + - name: Checkout code + uses: actions/checkout@v3 + with: + token: ${{ secrets.GITHUB_TOKEN }} + + - name: Install foundry + uses: foundry-rs/foundry-toolchain@v1 + with: + version: nightly + - name: Run Forge build + run: | + forge --version + forge build --sizes + id: build + - name: Run forge coverage + id: coverage + run: | + { + echo 'COVERAGE<> "$GITHUB_OUTPUT" + env: + FOUNDRY_RPC_URL: '${{ secrets.RPC_URL }}' + + - name: Check coverage is updated + uses: actions/github-script@v5 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const fs = require('fs'); + const file = "coverage.txt" + if(!fs.existsSync(file)) { + console.log("Nothing to check"); + return + } + const currentCoverage = fs.readFileSync(file, "utf8").trim(); + const newCoverage = (`${{ steps.coverage.outputs.COVERAGE }}`).trim(); + if (newCoverage != currentCoverage) { + core.setFailed(`Code coverage not updated. Run : forge coverage | grep -v 'test/' | tail -n +6 > coverage.txt`); + } + + - name: Comment on PR + id: comment + uses: actions/github-script@v5 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const {data: comments} = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + }) + + const botComment = comments.find(comment => comment.user.id === 41898282) + + const output = `${{ steps.coverage.outputs.COVERAGE }}`; + const commentBody = `Forge code coverage:\n${output}\n`; + + if (botComment) { + github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body: commentBody + }) + } else { + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: commentBody + }); + } \ No newline at end of file From b3bc509a0945099e06076d512160724b57f31443 Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Tue, 2 Apr 2024 20:19:05 -0300 Subject: [PATCH 09/18] remove noDelegateCall from most functions (#549) --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap burn 6909 for input.snap | 2 +- .../swap burn native 6909 for input.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/PoolManager.sol | 22 ++++++------------- 22 files changed, 28 insertions(+), 36 deletions(-) diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index a5c8b7976..c26bc5b0a 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265795 \ No newline at end of file +265667 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index bbd4c5e60..0d36d0aae 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140638 \ No newline at end of file +140510 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index bf6bedbf8..2ffee9f1e 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145955 \ No newline at end of file +145827 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index 174ef3d97..c87dfbb86 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101564 \ No newline at end of file +101500 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index 7d08b1b04..d467feba8 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -132618 \ No newline at end of file +132490 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index 7dbfdfbcb..9204af3e4 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51772 \ No newline at end of file +51829 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 5344365e5..6c49d3b64 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23114 \ No newline at end of file +23071 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index 7f8f52f0d..83381ddd6 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -56486 \ No newline at end of file +56358 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 8251926b6..9b9ed4ace 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -148672 \ No newline at end of file +148544 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index 4f0ad04f6..61fd7628b 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -150136 \ No newline at end of file +150008 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index cdb4e3f84..568794ae5 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -133389 \ No newline at end of file +133261 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 115c5c0ae..caa2efb80 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -147265 \ No newline at end of file +147137 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 04e526e37..6a6162bc9 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72848 \ No newline at end of file +72720 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index afac93fdf..b785ab3a9 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60851 \ No newline at end of file +60723 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 994be676d..530b6c153 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80881 \ No newline at end of file +80753 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index 811270183..916f7995b 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76878 \ No newline at end of file +76750 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index 7f96b17cd..367ad18bb 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139229 \ No newline at end of file +139101 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index a8579dc7e..295504505 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -156080 \ No newline at end of file +155952 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index 129249194..3fda2369b 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -90156 \ No newline at end of file +90028 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index bd043a483..54f8b894d 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60829 \ No newline at end of file +60701 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index 30d350371..b8761440e 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -141139 \ No newline at end of file +141011 \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index ef038af62..cdaffcefc 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -103,6 +103,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim function initialize(PoolKey memory key, uint160 sqrtPriceX96, bytes calldata hookData) external override + noDelegateCall returns (int24 tick) { // see TickBitmap.sol for overflow conditions that can arise from tick spacing being too large @@ -127,7 +128,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } /// @inheritdoc IPoolManager - function unlock(bytes calldata data) external override returns (bytes memory result) { + function unlock(bytes calldata data) external override noDelegateCall returns (bytes memory result) { if (Lock.isUnlocked()) revert AlreadyUnlocked(); Lock.unlock(); @@ -171,7 +172,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim PoolKey memory key, IPoolManager.ModifyLiquidityParams memory params, bytes calldata hookData - ) external override noDelegateCall onlyWhenUnlocked returns (BalanceDelta delta) { + ) external override onlyWhenUnlocked returns (BalanceDelta delta) { PoolId id = key.toId(); _checkPoolInitialized(id); @@ -198,7 +199,6 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim function swap(PoolKey memory key, IPoolManager.SwapParams memory params, bytes calldata hookData) external override - noDelegateCall onlyWhenUnlocked returns (BalanceDelta delta) { @@ -239,7 +239,6 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim function donate(PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData) external override - noDelegateCall onlyWhenUnlocked returns (BalanceDelta delta) { @@ -256,7 +255,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } /// @inheritdoc IPoolManager - function take(Currency currency, address to, uint256 amount) external override noDelegateCall onlyWhenUnlocked { + function take(Currency currency, address to, uint256 amount) external override onlyWhenUnlocked { // subtraction must be safe _accountDelta(currency, -(amount.toInt128())); if (!currency.isNative()) reservesOf[currency] -= amount; @@ -264,14 +263,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } /// @inheritdoc IPoolManager - function settle(Currency currency) - external - payable - override - noDelegateCall - onlyWhenUnlocked - returns (uint256 paid) - { + function settle(Currency currency) external payable override onlyWhenUnlocked returns (uint256 paid) { if (currency.isNative()) { paid = msg.value; } else { @@ -284,14 +276,14 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } /// @inheritdoc IPoolManager - function mint(address to, uint256 id, uint256 amount) external override noDelegateCall onlyWhenUnlocked { + function mint(address to, uint256 id, uint256 amount) external override onlyWhenUnlocked { // subtraction must be safe _accountDelta(CurrencyLibrary.fromId(id), -(amount.toInt128())); _mint(to, id, amount); } /// @inheritdoc IPoolManager - function burn(address from, uint256 id, uint256 amount) external override noDelegateCall onlyWhenUnlocked { + function burn(address from, uint256 id, uint256 amount) external override onlyWhenUnlocked { _accountDelta(CurrencyLibrary.fromId(id), amount.toInt128()); _burnFrom(from, id, amount); } From 27acc2599644f6788dc48f8749e92b81a4901b44 Mon Sep 17 00:00:00 2001 From: Adrien Husson Date: Wed, 3 Apr 2024 20:26:49 +0200 Subject: [PATCH 10/18] [copy of #340] perf: optimize poolKey.toId() (skip abi.encode) (#550) * perf: optimize PoolKey.toId() (skip abi.encode) * Update contracts/types/PoolId.sol Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> * Update test/foundry-tests/types/PoolKey.t.sol Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> * doc: explain toId optim + comment-style assembly guard * Update contracts/types/PoolId.sol Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> * Merge branch 'main' into perf/optimize-toId * remove redundant PoolKey.t.sol --------- Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> --- .forge-snapshots/addLiquidity with empty hook.snap | 2 +- .forge-snapshots/addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .forge-snapshots/donate gas with 2 tokens.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .forge-snapshots/poolManager bytecode size.snap | 2 +- .forge-snapshots/removeLiquidity with empty hook.snap | 2 +- .forge-snapshots/removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- .../swap against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .forge-snapshots/swap burn 6909 for input.snap | 2 +- .forge-snapshots/swap burn native 6909 for input.snap | 2 +- .forge-snapshots/swap mint native output as 6909.snap | 2 +- .forge-snapshots/swap mint output as 6909.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .forge-snapshots/update dynamic fee in before swap.snap | 2 +- src/types/PoolId.sol | 8 ++++++-- 22 files changed, 27 insertions(+), 23 deletions(-) diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index a5c8b7976..01b25590d 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265795 \ No newline at end of file +265439 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index bbd4c5e60..a737e37bc 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140638 \ No newline at end of file +140282 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index bf6bedbf8..118c0428d 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145955 \ No newline at end of file +145599 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index 174ef3d97..25bf12d0a 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101564 \ No newline at end of file +101209 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index 7d08b1b04..42ff76825 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -132618 \ No newline at end of file +132263 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index 7dbfdfbcb..14e96f0a2 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51772 \ No newline at end of file +51123 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 5344365e5..760e35e70 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23114 \ No newline at end of file +23017 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index 7f8f52f0d..b470b4a66 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -56486 \ No newline at end of file +56130 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 8251926b6..e569bc9fa 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -148672 \ No newline at end of file +148316 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index 4f0ad04f6..1489edeca 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -150136 \ No newline at end of file +149780 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index cdb4e3f84..7663e9d7e 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -133389 \ No newline at end of file +133033 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 115c5c0ae..9b951ac51 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -147265 \ No newline at end of file +146909 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 04e526e37..4802de299 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72848 \ No newline at end of file +72492 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index afac93fdf..6f1fd8d8e 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60851 \ No newline at end of file +60495 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 994be676d..187a61d66 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80881 \ No newline at end of file +80525 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index 811270183..9e76c775c 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76878 \ No newline at end of file +76522 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index 7f96b17cd..d093ebce1 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139229 \ No newline at end of file +138873 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index a8579dc7e..e0642c382 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -156080 \ No newline at end of file +155724 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index 129249194..8da1b010f 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -90156 \ No newline at end of file +89799 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index bd043a483..28e83565f 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60829 \ No newline at end of file +60473 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index 30d350371..a889839b7 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -141139 \ No newline at end of file +140425 \ No newline at end of file diff --git a/src/types/PoolId.sol b/src/types/PoolId.sol index 0f3c438be..88387a8d9 100644 --- a/src/types/PoolId.sol +++ b/src/types/PoolId.sol @@ -7,7 +7,11 @@ type PoolId is bytes32; /// @notice Library for computing the ID of a pool library PoolIdLibrary { - function toId(PoolKey memory poolKey) internal pure returns (PoolId) { - return PoolId.wrap(keccak256(abi.encode(poolKey))); + /// @notice Returns value equal to keccak256(abi.encode(poolKey)) + function toId(PoolKey memory poolKey) internal pure returns (PoolId poolId) { + /// @solidity memory-safe-assembly + assembly { + poolId := keccak256(poolKey, mul(32, 5)) + } } } From 8a4ebb071120b52788a671f5674f7d0c30350079 Mon Sep 17 00:00:00 2001 From: diana Date: Fri, 5 Apr 2024 14:25:12 -0400 Subject: [PATCH 11/18] skip calls to hooks when msg.sender is hook contract + test (#503) * skip calls to hooks when msg.sender is hook contract + test * move into callHook * changes * linting * re-add forge snapshots * switch back to self instead of hook * more tests * call again and assert counter is 2 * make more clear * clearAllHookPermisssionsMask * add comments and switch back to uint256 for now * allowance does not get decremented if the sender address is the contract address * fix claims * remove import console.sol --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap burn 6909 for input.snap | 2 +- .../swap burn native 6909 for input.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/libraries/Hooks.sol | 25 +- src/test/PoolDonateTest.sol | 14 +- src/test/PoolSwapTest.sol | 15 +- src/test/SkipCallsTestHook.sol | 207 +++++++++++++++++ test/ERC6909Claims.t.sol | 13 +- test/SkipCallsTestHook.t.sol | 213 ++++++++++++++++++ 26 files changed, 472 insertions(+), 55 deletions(-) create mode 100644 src/test/SkipCallsTestHook.sol create mode 100644 test/SkipCallsTestHook.t.sol diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index a57d22ba0..b8989b888 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265311 \ No newline at end of file +265373 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index 1be361e02..d447a901c 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140154 \ No newline at end of file +140136 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index 993394194..a4f8803b5 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145471 \ No newline at end of file +145453 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index 7c4e06da7..5999220c5 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101145 \ No newline at end of file +101049 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index 6423ce1f4..bef388da8 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -132135 \ No newline at end of file +132039 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 39c68e33e..5d39173db 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -22974 \ No newline at end of file +22962 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index c94433f36..60c50e8f3 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -56002 \ No newline at end of file +55984 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 36e50fa2b..91d24dd8f 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -148188 \ No newline at end of file +148170 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index 245df4831..b5f49cf1d 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -149652 \ No newline at end of file +149634 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index 797d10395..11e0aff71 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -132905 \ No newline at end of file +132814 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 483955831..51754821b 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -146781 \ No newline at end of file +146690 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 6671e8870..3ea3bdda3 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72364 \ No newline at end of file +72273 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index c31794dbd..bb8dceb17 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60367 \ No newline at end of file +60276 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 4cc58cdca..2c5d23970 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80397 \ No newline at end of file +80306 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index e67a9a599..7cf446276 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76394 \ No newline at end of file +76303 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index f504a2867..ef5b6e1ca 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -138745 \ No newline at end of file +138654 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index d2db9e035..b4ba009f6 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -155596 \ No newline at end of file +155505 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index a901423df..34409c289 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -89671 \ No newline at end of file +89580 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index 29498b964..14da3b121 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60345 \ No newline at end of file +60254 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index d6bca4c74..ac74b3045 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -140297 \ No newline at end of file +140246 \ No newline at end of file diff --git a/src/libraries/Hooks.sol b/src/libraries/Hooks.sol index 77873154e..6fed8bea9 100644 --- a/src/libraries/Hooks.sol +++ b/src/libraries/Hooks.sol @@ -71,13 +71,13 @@ library Hooks { } /// @notice Ensures that the hook address includes at least one hook flag or dynamic fees, or is the 0 address - /// @param hook The hook to verify - function isValidHookAddress(IHooks hook, uint24 fee) internal pure returns (bool) { + /// @param self The hook to verify + function isValidHookAddress(IHooks self, uint24 fee) internal pure returns (bool) { // If there is no hook contract set, then fee cannot be dynamic // If a hook contract is set, it must have at least 1 flag set, or have a dynamic fee - return address(hook) == address(0) + return address(self) == address(0) ? !fee.isDynamicFee() - : (uint160(address(hook)) >= AFTER_DONATE_FLAG || fee.isDynamicFee()); + : (uint160(address(self)) >= AFTER_DONATE_FLAG || fee.isDynamicFee()); } /// @notice performs a hook call using the given calldata on the given hook @@ -96,6 +96,7 @@ library Hooks { /// @notice performs a hook call using the given calldata on the given hook function callHook(IHooks self, bytes memory data) internal { + if (msg.sender == address(self)) return; (bytes4 expectedSelector, bytes4 selector) = _callHook(self, data); if (selector != expectedSelector) { @@ -132,9 +133,9 @@ library Hooks { IPoolManager.ModifyLiquidityParams memory params, bytes calldata hookData ) internal { - if (params.liquidityDelta > 0 && key.hooks.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) { + if (params.liquidityDelta > 0 && self.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) { self.callHook(abi.encodeWithSelector(IHooks.beforeAddLiquidity.selector, msg.sender, key, params, hookData)); - } else if (params.liquidityDelta <= 0 && key.hooks.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) { + } else if (params.liquidityDelta <= 0 && self.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) { self.callHook( abi.encodeWithSelector(IHooks.beforeRemoveLiquidity.selector, msg.sender, key, params, hookData) ); @@ -149,11 +150,11 @@ library Hooks { BalanceDelta delta, bytes calldata hookData ) internal { - if (params.liquidityDelta > 0 && key.hooks.hasPermission(AFTER_ADD_LIQUIDITY_FLAG)) { + if (params.liquidityDelta > 0 && self.hasPermission(AFTER_ADD_LIQUIDITY_FLAG)) { self.callHook( abi.encodeWithSelector(IHooks.afterAddLiquidity.selector, msg.sender, key, params, delta, hookData) ); - } else if (params.liquidityDelta <= 0 && key.hooks.hasPermission(AFTER_REMOVE_LIQUIDITY_FLAG)) { + } else if (params.liquidityDelta <= 0 && self.hasPermission(AFTER_REMOVE_LIQUIDITY_FLAG)) { self.callHook( abi.encodeWithSelector(IHooks.afterRemoveLiquidity.selector, msg.sender, key, params, delta, hookData) ); @@ -164,7 +165,7 @@ library Hooks { function beforeSwap(IHooks self, PoolKey memory key, IPoolManager.SwapParams memory params, bytes calldata hookData) internal { - if (key.hooks.hasPermission(BEFORE_SWAP_FLAG)) { + if (self.hasPermission(BEFORE_SWAP_FLAG)) { self.callHook(abi.encodeWithSelector(IHooks.beforeSwap.selector, msg.sender, key, params, hookData)); } } @@ -177,7 +178,7 @@ library Hooks { BalanceDelta delta, bytes calldata hookData ) internal { - if (key.hooks.hasPermission(AFTER_SWAP_FLAG)) { + if (self.hasPermission(AFTER_SWAP_FLAG)) { self.callHook(abi.encodeWithSelector(IHooks.afterSwap.selector, msg.sender, key, params, delta, hookData)); } } @@ -186,7 +187,7 @@ library Hooks { function beforeDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData) internal { - if (key.hooks.hasPermission(BEFORE_DONATE_FLAG)) { + if (self.hasPermission(BEFORE_DONATE_FLAG)) { self.callHook( abi.encodeWithSelector(IHooks.beforeDonate.selector, msg.sender, key, amount0, amount1, hookData) ); @@ -197,7 +198,7 @@ library Hooks { function afterDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData) internal { - if (key.hooks.hasPermission(AFTER_DONATE_FLAG)) { + if (self.hasPermission(AFTER_DONATE_FLAG)) { self.callHook( abi.encodeWithSelector(IHooks.afterDonate.selector, msg.sender, key, amount0, amount1, hookData) ); diff --git a/src/test/PoolDonateTest.sol b/src/test/PoolDonateTest.sol index d568c32e0..55d95c392 100644 --- a/src/test/PoolDonateTest.sol +++ b/src/test/PoolDonateTest.sol @@ -43,23 +43,17 @@ contract PoolDonateTest is PoolTestBase { CallbackData memory data = abi.decode(rawData, (CallbackData)); - (, uint256 poolBalanceBefore0, int256 deltaBefore0) = - _fetchBalances(data.key.currency0, data.sender, address(this)); - (, uint256 poolBalanceBefore1, int256 deltaBefore1) = - _fetchBalances(data.key.currency1, data.sender, address(this)); + (,, int256 deltaBefore0) = _fetchBalances(data.key.currency0, data.sender, address(this)); + (,, int256 deltaBefore1) = _fetchBalances(data.key.currency1, data.sender, address(this)); require(deltaBefore0 == 0, "deltaBefore0 is not 0"); require(deltaBefore1 == 0, "deltaBefore1 is not 0"); BalanceDelta delta = manager.donate(data.key, data.amount0, data.amount1, data.hookData); - (, uint256 poolBalanceAfter0, int256 deltaAfter0) = - _fetchBalances(data.key.currency0, data.sender, address(this)); - (, uint256 poolBalanceAfter1, int256 deltaAfter1) = - _fetchBalances(data.key.currency1, data.sender, address(this)); + (,, int256 deltaAfter0) = _fetchBalances(data.key.currency0, data.sender, address(this)); + (,, int256 deltaAfter1) = _fetchBalances(data.key.currency1, data.sender, address(this)); - require(poolBalanceBefore0 == poolBalanceAfter0, "poolBalanceBefore0 is not equal to poolBalanceAfter0"); - require(poolBalanceBefore1 == poolBalanceAfter1, "poolBalanceBefore1 is not equal to poolBalanceAfter1"); require(deltaAfter0 == -int256(data.amount0), "deltaAfter0 is not equal to -int256(data.amount0)"); require(deltaAfter1 == -int256(data.amount1), "deltaAfter1 is not equal to -int256(data.amount1)"); diff --git a/src/test/PoolSwapTest.sol b/src/test/PoolSwapTest.sol index 0817017b3..255fb678a 100644 --- a/src/test/PoolSwapTest.sol +++ b/src/test/PoolSwapTest.sol @@ -52,23 +52,16 @@ contract PoolSwapTest is PoolTestBase { CallbackData memory data = abi.decode(rawData, (CallbackData)); - (, uint256 poolBalanceBefore0, int256 deltaBefore0) = - _fetchBalances(data.key.currency0, data.sender, address(this)); - (, uint256 poolBalanceBefore1, int256 deltaBefore1) = - _fetchBalances(data.key.currency1, data.sender, address(this)); + (,, int256 deltaBefore0) = _fetchBalances(data.key.currency0, data.sender, address(this)); + (,, int256 deltaBefore1) = _fetchBalances(data.key.currency1, data.sender, address(this)); require(deltaBefore0 == 0, "deltaBefore0 is not equal to 0"); require(deltaBefore1 == 0, "deltaBefore1 is not equal to 0"); BalanceDelta delta = manager.swap(data.key, data.params, data.hookData); - (, uint256 poolBalanceAfter0, int256 deltaAfter0) = - _fetchBalances(data.key.currency0, data.sender, address(this)); - (, uint256 poolBalanceAfter1, int256 deltaAfter1) = - _fetchBalances(data.key.currency1, data.sender, address(this)); - - require(poolBalanceBefore0 == poolBalanceAfter0, "poolBalanceBefore0 is not equal to poolBalanceAfter0"); - require(poolBalanceBefore1 == poolBalanceAfter1, "poolBalanceBefore1 is not equal to poolBalanceAfter1"); + (,, int256 deltaAfter0) = _fetchBalances(data.key.currency0, data.sender, address(this)); + (,, int256 deltaAfter1) = _fetchBalances(data.key.currency1, data.sender, address(this)); if (data.params.zeroForOne) { if (data.params.amountSpecified < 0) { diff --git a/src/test/SkipCallsTestHook.sol b/src/test/SkipCallsTestHook.sol new file mode 100644 index 000000000..0ebc72185 --- /dev/null +++ b/src/test/SkipCallsTestHook.sol @@ -0,0 +1,207 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import {Hooks} from "../libraries/Hooks.sol"; +import {BaseTestHooks} from "./BaseTestHooks.sol"; +import {IHooks} from "../interfaces/IHooks.sol"; +import {IPoolManager} from "../interfaces/IPoolManager.sol"; +import {PoolKey} from "../types/PoolKey.sol"; +import {BalanceDelta} from "../types/BalanceDelta.sol"; +import {PoolId, PoolIdLibrary} from "../types/PoolId.sol"; +import {IERC20Minimal} from "../interfaces/external/IERC20Minimal.sol"; +import {CurrencyLibrary, Currency} from "../types/Currency.sol"; +import {PoolTestBase} from "./PoolTestBase.sol"; +import {Constants} from "../../test/utils/Constants.sol"; +import {Test} from "forge-std/Test.sol"; + +contract SkipCallsTestHook is BaseTestHooks, Test { + using PoolIdLibrary for PoolKey; + using Hooks for IHooks; + + uint256 public counter; + IPoolManager manager; + + function setManager(IPoolManager _manager) external { + manager = _manager; + } + + function beforeInitialize(address, PoolKey calldata key, uint160 sqrtPriceX96, bytes calldata hookData) + external + override + returns (bytes4) + { + counter++; + _initialize(key, sqrtPriceX96, hookData); + return IHooks.beforeInitialize.selector; + } + + function afterInitialize(address, PoolKey calldata key, uint160 sqrtPriceX96, int24, bytes calldata hookData) + external + override + returns (bytes4) + { + counter++; + _initialize(key, sqrtPriceX96, hookData); + return IHooks.afterInitialize.selector; + } + + function beforeAddLiquidity( + address, + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams calldata params, + bytes calldata hookData + ) external override returns (bytes4) { + counter++; + _addLiquidity(key, params, hookData); + return IHooks.beforeAddLiquidity.selector; + } + + function afterAddLiquidity( + address, + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams calldata params, + BalanceDelta, + bytes calldata hookData + ) external override returns (bytes4) { + counter++; + _addLiquidity(key, params, hookData); + return IHooks.afterAddLiquidity.selector; + } + + function beforeRemoveLiquidity( + address, + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams calldata params, + bytes calldata hookData + ) external override returns (bytes4) { + counter++; + _removeLiquidity(key, params, hookData); + return IHooks.beforeRemoveLiquidity.selector; + } + + function afterRemoveLiquidity( + address, + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams calldata params, + BalanceDelta, + bytes calldata hookData + ) external override returns (bytes4) { + counter++; + _removeLiquidity(key, params, hookData); + return IHooks.afterRemoveLiquidity.selector; + } + + function beforeSwap(address, PoolKey calldata key, IPoolManager.SwapParams calldata params, bytes calldata hookData) + external + override + returns (bytes4) + { + counter++; + _swap(key, params, hookData); + return IHooks.beforeSwap.selector; + } + + function afterSwap( + address, + PoolKey calldata key, + IPoolManager.SwapParams calldata params, + BalanceDelta, + bytes calldata hookData + ) external override returns (bytes4) { + counter++; + _swap(key, params, hookData); + return IHooks.afterSwap.selector; + } + + function beforeDonate(address, PoolKey calldata key, uint256 amt0, uint256 amt1, bytes calldata hookData) + external + override + returns (bytes4) + { + counter++; + _donate(key, amt0, amt1, hookData); + return IHooks.beforeDonate.selector; + } + + function afterDonate(address, PoolKey calldata key, uint256 amt0, uint256 amt1, bytes calldata hookData) + external + override + returns (bytes4) + { + counter++; + _donate(key, amt0, amt1, hookData); + return IHooks.afterDonate.selector; + } + + function _initialize(PoolKey memory key, uint160 sqrtPriceX96, bytes calldata hookData) public { + // initialize a new pool with different fee + key.fee = 2000; + IPoolManager(manager).initialize(key, sqrtPriceX96, hookData); + } + + function _swap(PoolKey calldata key, IPoolManager.SwapParams memory params, bytes calldata hookData) public { + IPoolManager(manager).swap(key, params, hookData); + address payer = abi.decode(hookData, (address)); + int256 delta0 = IPoolManager(manager).currencyDelta(address(this), key.currency0); + assertEq(delta0, params.amountSpecified); + int256 delta1 = IPoolManager(manager).currencyDelta(address(this), key.currency1); + assert(delta1 > 0); + IERC20Minimal(Currency.unwrap(key.currency0)).transferFrom(payer, address(manager), uint256(-delta0)); + manager.settle(key.currency0); + manager.take(key.currency1, payer, uint256(delta1)); + } + + function _addLiquidity( + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams memory params, + bytes calldata hookData + ) public { + IPoolManager(manager).modifyLiquidity(key, params, hookData); + address payer = abi.decode(hookData, (address)); + int256 delta0 = IPoolManager(manager).currencyDelta(address(this), key.currency0); + int256 delta1 = IPoolManager(manager).currencyDelta(address(this), key.currency1); + + assert(delta0 < 0 || delta1 < 0); + assert(!(delta0 > 0 || delta1 > 0)); + + IERC20Minimal(Currency.unwrap(key.currency0)).transferFrom(payer, address(manager), uint256(-delta0)); + manager.settle(key.currency0); + IERC20Minimal(Currency.unwrap(key.currency1)).transferFrom(payer, address(manager), uint256(-delta1)); + manager.settle(key.currency1); + } + + function _removeLiquidity( + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams memory params, + bytes calldata hookData + ) public { + // first hook needs to add liquidity for itself + IPoolManager.ModifyLiquidityParams memory newParams = + IPoolManager.ModifyLiquidityParams({tickLower: -120, tickUpper: 120, liquidityDelta: 1e18}); + IPoolManager(manager).modifyLiquidity(key, newParams, hookData); + // hook removes liquidity + IPoolManager(manager).modifyLiquidity(key, params, hookData); + address payer = abi.decode(hookData, (address)); + int256 delta0 = IPoolManager(manager).currencyDelta(address(this), key.currency0); + int256 delta1 = IPoolManager(manager).currencyDelta(address(this), key.currency1); + + assert(delta0 < 0 || delta1 < 0); + assert(!(delta0 > 0 || delta1 > 0)); + + IERC20Minimal(Currency.unwrap(key.currency0)).transferFrom(payer, address(manager), uint256(-delta0)); + manager.settle(key.currency0); + IERC20Minimal(Currency.unwrap(key.currency1)).transferFrom(payer, address(manager), uint256(-delta1)); + manager.settle(key.currency1); + } + + function _donate(PoolKey calldata key, uint256 amt0, uint256 amt1, bytes calldata hookData) public { + IPoolManager(manager).donate(key, amt0, amt1, hookData); + address payer = abi.decode(hookData, (address)); + int256 delta0 = IPoolManager(manager).currencyDelta(address(this), key.currency0); + int256 delta1 = IPoolManager(manager).currencyDelta(address(this), key.currency1); + IERC20Minimal(Currency.unwrap(key.currency0)).transferFrom(payer, address(manager), uint256(-delta0)); + IERC20Minimal(Currency.unwrap(key.currency1)).transferFrom(payer, address(manager), uint256(-delta1)); + manager.settle(key.currency0); + manager.settle(key.currency1); + } +} diff --git a/test/ERC6909Claims.t.sol b/test/ERC6909Claims.t.sol index fc01cde5a..dbf3dc478 100644 --- a/test/ERC6909Claims.t.sol +++ b/test/ERC6909Claims.t.sol @@ -31,7 +31,11 @@ contract ERC6909ClaimsTest is Test { if (mintAmount == type(uint256).max) { assertEq(token.allowance(sender, address(this), id), type(uint256).max); } else { - assertEq(token.allowance(sender, address(this), id), mintAmount - transferAmount); + if (sender != address(this)) { + assertEq(token.allowance(sender, address(this), id), mintAmount - transferAmount); + } else { + assertEq(token.allowance(sender, address(this), id), mintAmount); + } } assertEq(token.balanceOf(sender, id), mintAmount - transferAmount); } @@ -255,7 +259,11 @@ contract ERC6909ClaimsTest is Test { if (mintAmount == type(uint256).max) { assertEq(token.allowance(sender, address(this), id), type(uint256).max); } else { - assertEq(token.allowance(sender, address(this), id), mintAmount - transferAmount); + if (sender != address(this)) { + assertEq(token.allowance(sender, address(this), id), mintAmount - transferAmount); + } else { + assertEq(token.allowance(sender, address(this), id), mintAmount); + } } if (sender == receiver) { @@ -367,6 +375,7 @@ contract ERC6909ClaimsTest is Test { token.mint(sender, id, amount); + vm.assume(sender != address(this)); token.transferFrom(sender, receiver, id, amount); } } diff --git a/test/SkipCallsTestHook.t.sol b/test/SkipCallsTestHook.t.sol new file mode 100644 index 000000000..d12609b0f --- /dev/null +++ b/test/SkipCallsTestHook.t.sol @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {Vm} from "forge-std/Vm.sol"; +import {PoolId, PoolIdLibrary} from "../src/types/PoolId.sol"; +import {Hooks} from "../src/libraries/Hooks.sol"; +import {SwapFeeLibrary} from "../src/libraries/SwapFeeLibrary.sol"; +import {IPoolManager} from "../src/interfaces/IPoolManager.sol"; +import {IProtocolFees} from "../src/interfaces/IProtocolFees.sol"; +import {IHooks} from "../src/interfaces/IHooks.sol"; +import {PoolKey} from "../src/types/PoolKey.sol"; +import {PoolManager} from "../src/PoolManager.sol"; +import {PoolSwapTest} from "../src/test/PoolSwapTest.sol"; +import {Deployers} from "./utils/Deployers.sol"; +import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol"; +import {Currency, CurrencyLibrary} from "../src/types/Currency.sol"; +import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol"; +import {Constants} from "../test/utils/Constants.sol"; +import {SkipCallsTestHook} from "../src/test/SkipCallsTestHook.sol"; + +contract SkipCallsTest is Test, Deployers, GasSnapshot { + using PoolIdLibrary for PoolKey; + + IPoolManager.SwapParams swapParams = + IPoolManager.SwapParams({zeroForOne: true, amountSpecified: -100, sqrtPriceLimitX96: SQRT_RATIO_1_2}); + + PoolSwapTest.TestSettings testSettings = + PoolSwapTest.TestSettings({withdrawTokens: true, settleUsingTransfer: true, currencyAlreadySent: false}); + + uint160 clearAllHookPermisssionsMask; + uint256 hookPermissionCount = 10; + + function setUp() public { + clearAllHookPermisssionsMask = ~uint160(0) >> hookPermissionCount; + } + + function deploy(SkipCallsTestHook skipCallsTestHook) private { + SkipCallsTestHook impl = new SkipCallsTestHook(); + vm.etch(address(skipCallsTestHook), address(impl).code); + deployFreshManagerAndRouters(); + skipCallsTestHook.setManager(IPoolManager(manager)); + (currency0, currency1) = deployMintAndApprove2Currencies(); + + assertEq(skipCallsTestHook.counter(), 0); + + (key,) = initPool(currency0, currency1, IHooks(address(skipCallsTestHook)), 3000, SQRT_RATIO_1_1, ZERO_BYTES); + } + + function approveAndAddLiquidity(SkipCallsTestHook skipCallsTestHook) private { + MockERC20(Currency.unwrap(key.currency0)).approve(address(skipCallsTestHook), Constants.MAX_UINT256); + MockERC20(Currency.unwrap(key.currency1)).approve(address(skipCallsTestHook), Constants.MAX_UINT256); + modifyLiquidityRouter.modifyLiquidity(key, LIQ_PARAMS, abi.encode(address(this))); + } + + function test_beforeInitialize_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.BEFORE_INITIALIZE_FLAG)) + ); + + // initializes pool and increments counter + deploy(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 1); + } + + function test_afterInitialize_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.AFTER_INITIALIZE_FLAG)) + ); + + // initializes pool and increments counter + deploy(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 1); + } + + function test_beforeAddLiquidity_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.BEFORE_ADD_LIQUIDITY_FLAG)) + ); + + deploy(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // adds liquidity and increments counter + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 1); + // adds liquidity again and increments counter + modifyLiquidityRouter.modifyLiquidity(key, LIQ_PARAMS, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_afterAddLiquidity_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.AFTER_ADD_LIQUIDITY_FLAG)) + ); + + deploy(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // adds liquidity and increments counter + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 1); + // adds liquidity and increments counter again + modifyLiquidityRouter.modifyLiquidity(key, LIQ_PARAMS, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_beforeRemoveLiquidity_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.BEFORE_REMOVE_LIQUIDITY_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // removes liquidity and increments counter + modifyLiquidityRouter.modifyLiquidity(key, REMOVE_LIQ_PARAMS, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 1); + // adds liquidity again + modifyLiquidityRouter.modifyLiquidity(key, LIQ_PARAMS, abi.encode(address(this))); + // removes liquidity again and increments counter + modifyLiquidityRouter.modifyLiquidity(key, REMOVE_LIQ_PARAMS, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_afterRemoveLiquidity_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // removes liquidity and increments counter + modifyLiquidityRouter.modifyLiquidity(key, REMOVE_LIQ_PARAMS, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 1); + // adds liquidity again + modifyLiquidityRouter.modifyLiquidity(key, LIQ_PARAMS, abi.encode(address(this))); + // removes liquidity again and increments counter + modifyLiquidityRouter.modifyLiquidity(key, REMOVE_LIQ_PARAMS, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_beforeSwap_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.BEFORE_SWAP_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // swaps and increments counter + swapRouter.swap(key, swapParams, testSettings, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 1); + // swaps again and increments counter + swapRouter.swap(key, swapParams, testSettings, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_afterSwap_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.AFTER_SWAP_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // swaps and increments counter + swapRouter.swap(key, swapParams, testSettings, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 1); + // swaps again and increments counter + swapRouter.swap(key, swapParams, testSettings, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_beforeDonate_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.BEFORE_DONATE_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // donates and increments counter + donateRouter.donate(key, 100, 200, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 1); + // donates again and increments counter + donateRouter.donate(key, 100, 200, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } + + function test_afterDonate_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.AFTER_DONATE_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // donates and increments counter + donateRouter.donate(key, 100, 200, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 1); + // donates again and increments counter + donateRouter.donate(key, 100, 200, abi.encode(address(this))); + assertEq(skipCallsTestHook.counter(), 2); + } +} From 095811e66478cf1bd3ffef6db01e53f02fd5f8a3 Mon Sep 17 00:00:00 2001 From: hyunchel <3271191+hyunchel@users.noreply.github.com> Date: Fri, 5 Apr 2024 14:57:15 -0400 Subject: [PATCH 12/18] Allow max int value on max liquidity check (#563) This commit updates a tick spacing fuzz test to include maximum integer value of liquidity as a valid number. Replaces PR #368 Fixes issue #370 --- test/Tick.t.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/Tick.t.sol b/test/Tick.t.sol index 565572ddb..b99d3a128 100644 --- a/test/Tick.t.sol +++ b/test/Tick.t.sol @@ -495,7 +495,8 @@ contract TickTest is Test, GasSnapshot { assertEq((maxTick - minTick) % tickSpacing, 0); uint256 numTicks = uint256(int256((maxTick - minTick) / tickSpacing)) + 1; - // max liquidity at every tick is less than the cap - assertGt(type(uint128).max, uint256(maxLiquidityPerTick) * numTicks); + + // sum of max liquidity on each tick is at most the cap + assertGe(type(uint128).max, uint256(maxLiquidityPerTick) * numTicks); } } From d6df729d1698f86b468c616c2970ebbd7e664e6e Mon Sep 17 00:00:00 2001 From: Sara Reynolds <30504811+snreynolds@users.noreply.github.com> Date: Fri, 5 Apr 2024 17:06:20 -0400 Subject: [PATCH 13/18] move protocol fee logic to ProtocolFees.sol (#546) * move protocol fee logic to ProtocolFees.sol * check for protocol fee == 0 * inheritdoc, use internal mapping --- .../poolManager bytecode size.snap | 2 +- src/PoolManager.sol | 20 +++--- src/ProtocolFees.sol | 61 +++++++++++++------ src/interfaces/IPoolManager.sol | 6 -- src/interfaces/IProtocolFees.sol | 12 +++- test/PoolManager.t.sol | 5 +- 6 files changed, 63 insertions(+), 43 deletions(-) diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 5d39173db..6a63e7beb 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -22962 \ No newline at end of file +22965 \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index cdaffcefc..a77dc91bc 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -47,6 +47,10 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim constructor(uint256 controllerGasLimit) ProtocolFees(controllerGasLimit) {} + function _getPool(PoolId id) internal view override returns (Pool.State storage) { + return pools[id]; + } + /// @inheritdoc IPoolManager function getSlot0(PoolId id) external @@ -221,11 +225,9 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim _accountPoolBalanceDelta(key, delta); - // the fee is on the input currency - unchecked { - if (feeForProtocol > 0) { - protocolFeesAccrued[params.zeroForOne ? key.currency0 : key.currency1] += feeForProtocol; - } + // The fee is on the input currency. + if (feeForProtocol > 0) { + _updateProtocolFees(params.zeroForOne ? key.currency0 : key.currency1, feeForProtocol); } emit Swap( @@ -288,14 +290,6 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim _burnFrom(from, id, amount); } - function setProtocolFee(PoolKey memory key) external { - (bool success, uint24 newProtocolFee) = _fetchProtocolFee(key); - if (!success) revert ProtocolFeeControllerCallFailedOrInvalidResult(); - PoolId id = key.toId(); - pools[id].setProtocolFee(newProtocolFee); - emit ProtocolFeeUpdated(id, newProtocolFee); - } - function updateDynamicSwapFee(PoolKey memory key, uint24 newDynamicSwapFee) external { if (!key.fee.isDynamicFee() || msg.sender != address(key.hooks)) revert UnauthorizedDynamicSwapFeeUpdate(); newDynamicSwapFee.validate(); diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol index f9d913238..4775f19db 100644 --- a/src/ProtocolFees.sol +++ b/src/ProtocolFees.sol @@ -1,16 +1,20 @@ // SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.8.19; -import {Currency, CurrencyLibrary} from "./types/Currency.sol"; -import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol"; -import {IProtocolFees} from "./interfaces/IProtocolFees.sol"; -import {PoolKey} from "./types/PoolKey.sol"; -import {ProtocolFeeLibrary} from "./libraries/ProtocolFeeLibrary.sol"; +import {Currency, CurrencyLibrary} from "src/types/Currency.sol"; +import {IProtocolFeeController} from "src/interfaces/IProtocolFeeController.sol"; +import {IProtocolFees} from "src/interfaces/IProtocolFees.sol"; +import {PoolKey} from "src/types/PoolKey.sol"; +import {ProtocolFeeLibrary} from "src/libraries/ProtocolFeeLibrary.sol"; import {Owned} from "solmate/auth/Owned.sol"; +import {PoolId, PoolIdLibrary} from "src/types/PoolId.sol"; +import {Pool} from "src/libraries/Pool.sol"; abstract contract ProtocolFees is IProtocolFees, Owned { using CurrencyLibrary for Currency; using ProtocolFeeLibrary for uint24; + using PoolIdLibrary for PoolKey; + using Pool for Pool.State; mapping(Currency currency => uint256) public protocolFeesAccrued; @@ -22,6 +26,35 @@ abstract contract ProtocolFees is IProtocolFees, Owned { controllerGasLimit = _controllerGasLimit; } + /// @inheritdoc IProtocolFees + function setProtocolFeeController(IProtocolFeeController controller) external onlyOwner { + protocolFeeController = controller; + emit ProtocolFeeControllerUpdated(address(controller)); + } + + /// @inheritdoc IProtocolFees + function setProtocolFee(PoolKey memory key) external { + (bool success, uint24 newProtocolFee) = _fetchProtocolFee(key); + if (!success) revert ProtocolFeeControllerCallFailedOrInvalidResult(); + PoolId id = key.toId(); + _getPool(id).setProtocolFee(newProtocolFee); + emit ProtocolFeeUpdated(id, newProtocolFee); + } + + /// @inheritdoc IProtocolFees + function collectProtocolFees(address recipient, Currency currency, uint256 amount) + external + returns (uint256 amountCollected) + { + if (msg.sender != address(protocolFeeController)) revert InvalidCaller(); + + amountCollected = (amount == 0) ? protocolFeesAccrued[currency] : amount; + protocolFeesAccrued[currency] -= amountCollected; + currency.transfer(recipient, amountCollected); + } + + function _getPool(PoolId id) internal virtual returns (Pool.State storage); + /// @notice Fetch the protocol fees for a given pool, returning false if the call fails or the returned fees are invalid. /// @dev to prevent an invalid protocol fee controller from blocking pools from being initialized /// the success of this function is NOT checked on initialize and if the call fails, the protocol fees are set to 0. @@ -50,19 +83,9 @@ abstract contract ProtocolFees is IProtocolFees, Owned { } } - function setProtocolFeeController(IProtocolFeeController controller) external onlyOwner { - protocolFeeController = controller; - emit ProtocolFeeControllerUpdated(address(controller)); - } - - function collectProtocolFees(address recipient, Currency currency, uint256 amount) - external - returns (uint256 amountCollected) - { - if (msg.sender != address(protocolFeeController)) revert InvalidCaller(); - - amountCollected = (amount == 0) ? protocolFeesAccrued[currency] : amount; - protocolFeesAccrued[currency] -= amountCollected; - currency.transfer(recipient, amountCollected); + function _updateProtocolFees(Currency currency, uint256 amount) internal { + unchecked { + protocolFeesAccrued[currency] += amount; + } } } diff --git a/src/interfaces/IPoolManager.sol b/src/interfaces/IPoolManager.sol index f7b3954a1..b256d8a91 100644 --- a/src/interfaces/IPoolManager.sol +++ b/src/interfaces/IPoolManager.sol @@ -81,8 +81,6 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { uint24 fee ); - event ProtocolFeeUpdated(PoolId indexed id, uint24 protocolFee); - /// @notice Returns the constant representing the maximum tickSpacing for an initialized pool key function MAX_TICK_SPACING() external view returns (int24); @@ -193,10 +191,6 @@ interface IPoolManager is IProtocolFees, IERC6909Claims { /// @notice Called by the user to pay what is owed function settle(Currency token) external payable returns (uint256 paid); - /// @notice Sets the protocol's swap fee for the given pool - /// Protocol fees are always a portion of the LP swap fee that is owed. If that fee is 0, no protocol fees will accrue even if it is set to > 0. - function setProtocolFee(PoolKey memory key) external; - /// @notice Updates the pools swap fees for the a pool that has enabled dynamic swap fees. function updateDynamicSwapFee(PoolKey memory key, uint24 newDynamicSwapFee) external; diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol index a38a8f7b6..2e09fc08b 100644 --- a/src/interfaces/IProtocolFees.sol +++ b/src/interfaces/IProtocolFees.sol @@ -1,8 +1,10 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.19; -import {Currency} from "../types/Currency.sol"; -import {IProtocolFeeController} from "./IProtocolFeeController.sol"; +import {Currency} from "src/types/Currency.sol"; +import {IProtocolFeeController} from "src/interfaces/IProtocolFeeController.sol"; +import {PoolId} from "src/types/PoolId.sol"; +import {PoolKey} from "src/types/PoolKey.sol"; interface IProtocolFees { /// @notice Thrown when not enough gas is provided to look up the protocol fee @@ -15,9 +17,15 @@ interface IProtocolFees { event ProtocolFeeControllerUpdated(address protocolFeeController); + event ProtocolFeeUpdated(PoolId indexed id, uint24 protocolFee); + /// @notice Given a currency address, returns the protocol fees accrued in that currency function protocolFeesAccrued(Currency) external view returns (uint256); + /// @notice Sets the protocol's swap fee for the given pool + /// Protocol fees are always a portion of the LP swap fee that is owed. If that fee is 0, no protocol fees will accrue even if it is set to > 0. + function setProtocolFee(PoolKey memory key) external; + /// @notice Sets the protocol fee controller function setProtocolFeeController(IProtocolFeeController) external; diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index 73d6d86dc..e313ad34e 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -30,6 +30,7 @@ import {Constants} from "./utils/Constants.sol"; import {SafeCast} from "../src/libraries/SafeCast.sol"; import {AmountHelpers} from "./utils/AmountHelpers.sol"; import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; +import {IProtocolFees} from "../src/interfaces/IProtocolFees.sol"; contract PoolManagerTest is Test, Deployers, GasSnapshot { using Hooks for IHooks; @@ -53,7 +54,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { int24 tick, uint24 fee ); - event ProtocolFeeUpdated(PoolId indexed id, uint24 protocolFee); + event Transfer( address caller, address indexed sender, address indexed receiver, uint256 indexed id, uint256 amount ); @@ -956,7 +957,7 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { manager.setProtocolFee(key); } else { vm.expectEmit(false, false, false, true); - emit ProtocolFeeUpdated(key.toId(), protocolFee); + emit IProtocolFees.ProtocolFeeUpdated(key.toId(), protocolFee); manager.setProtocolFee(key); (,, slot0ProtocolFee,) = manager.getSlot0(key.toId()); From 11aa1f5b86b529d6894d25f304bb2b3b5c132bbe Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Fri, 5 Apr 2024 18:19:19 -0300 Subject: [PATCH 14/18] Move msg.sender check to modifier (#564) * Move msg.sender check to modifier * Removing old check * remove extra line * Update SkipCallsTestHook.t.sol * snap rename --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap burn 6909 for input.snap | 2 +- .../swap burn native 6909 for input.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- ...wap skips hook call if hook is caller.snap | 1 + .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/libraries/Hooks.sol | 19 +++++++++++++++---- test/SkipCallsTestHook.t.sol | 16 ++++++++++++++++ 24 files changed, 53 insertions(+), 25 deletions(-) create mode 100644 .forge-snapshots/swap skips hook call if hook is caller.snap diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index b8989b888..019c77f29 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265373 \ No newline at end of file +265381 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index d447a901c..d072cabb1 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140136 \ No newline at end of file +140224 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index a4f8803b5..267b1c2f5 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145453 \ No newline at end of file +145541 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index 5999220c5..0715154f4 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -101049 \ No newline at end of file +101137 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index bef388da8..0bc63eb2a 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -132039 \ No newline at end of file +132127 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index 662b3527e..9576f45df 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51180 \ No newline at end of file +51268 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 6a63e7beb..2654fd3e0 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -22965 \ No newline at end of file +23083 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index 60c50e8f3..a7b4c4134 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -55984 \ No newline at end of file +56072 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 91d24dd8f..5710db49c 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -148170 \ No newline at end of file +148258 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index b5f49cf1d..1bcdd5bfe 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -149634 \ No newline at end of file +149722 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index 11e0aff71..92d965caa 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -132814 \ No newline at end of file +132902 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 51754821b..e03d94df2 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -146690 \ No newline at end of file +146778 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 3ea3bdda3..3039612cb 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72273 \ No newline at end of file +72361 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index bb8dceb17..13788d81c 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60276 \ No newline at end of file +60364 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 2c5d23970..07f2cf54d 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80306 \ No newline at end of file +80394 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index 7cf446276..505243eb6 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76303 \ No newline at end of file +76391 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index ef5b6e1ca..cad93df0b 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -138654 \ No newline at end of file +138742 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index b4ba009f6..134896538 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -155505 \ No newline at end of file +155593 \ No newline at end of file diff --git a/.forge-snapshots/swap skips hook call if hook is caller.snap b/.forge-snapshots/swap skips hook call if hook is caller.snap new file mode 100644 index 000000000..41ac906bf --- /dev/null +++ b/.forge-snapshots/swap skips hook call if hook is caller.snap @@ -0,0 +1 @@ +156254 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index 34409c289..1456957a2 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -89580 \ No newline at end of file +89668 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index 14da3b121..2f3e10ebe 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60254 \ No newline at end of file +60342 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index ac74b3045..f44d09345 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -140246 \ No newline at end of file +140294 \ No newline at end of file diff --git a/src/libraries/Hooks.sol b/src/libraries/Hooks.sol index 6fed8bea9..895876a2a 100644 --- a/src/libraries/Hooks.sol +++ b/src/libraries/Hooks.sol @@ -96,7 +96,6 @@ library Hooks { /// @notice performs a hook call using the given calldata on the given hook function callHook(IHooks self, bytes memory data) internal { - if (msg.sender == address(self)) return; (bytes4 expectedSelector, bytes4 selector) = _callHook(self, data); if (selector != expectedSelector) { @@ -104,9 +103,17 @@ library Hooks { } } + /// @notice modifier to prevent calling a hook if they initiated the action + modifier noSelfCall(IHooks self) { + if (msg.sender != address(self)) { + _; + } + } + /// @notice calls beforeInitialize hook if permissioned and validates return value function beforeInitialize(IHooks self, PoolKey memory key, uint160 sqrtPriceX96, bytes calldata hookData) internal + noSelfCall(self) { if (self.hasPermission(BEFORE_INITIALIZE_FLAG)) { self.callHook( @@ -118,6 +125,7 @@ library Hooks { /// @notice calls afterInitialize hook if permissioned and validates return value function afterInitialize(IHooks self, PoolKey memory key, uint160 sqrtPriceX96, int24 tick, bytes calldata hookData) internal + noSelfCall(self) { if (self.hasPermission(AFTER_INITIALIZE_FLAG)) { self.callHook( @@ -132,7 +140,7 @@ library Hooks { PoolKey memory key, IPoolManager.ModifyLiquidityParams memory params, bytes calldata hookData - ) internal { + ) internal noSelfCall(self) { if (params.liquidityDelta > 0 && self.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) { self.callHook(abi.encodeWithSelector(IHooks.beforeAddLiquidity.selector, msg.sender, key, params, hookData)); } else if (params.liquidityDelta <= 0 && self.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) { @@ -149,7 +157,7 @@ library Hooks { IPoolManager.ModifyLiquidityParams memory params, BalanceDelta delta, bytes calldata hookData - ) internal { + ) internal noSelfCall(self) { if (params.liquidityDelta > 0 && self.hasPermission(AFTER_ADD_LIQUIDITY_FLAG)) { self.callHook( abi.encodeWithSelector(IHooks.afterAddLiquidity.selector, msg.sender, key, params, delta, hookData) @@ -164,6 +172,7 @@ library Hooks { /// @notice calls beforeSwap hook if permissioned and validates return value function beforeSwap(IHooks self, PoolKey memory key, IPoolManager.SwapParams memory params, bytes calldata hookData) internal + noSelfCall(self) { if (self.hasPermission(BEFORE_SWAP_FLAG)) { self.callHook(abi.encodeWithSelector(IHooks.beforeSwap.selector, msg.sender, key, params, hookData)); @@ -177,7 +186,7 @@ library Hooks { IPoolManager.SwapParams memory params, BalanceDelta delta, bytes calldata hookData - ) internal { + ) internal noSelfCall(self) { if (self.hasPermission(AFTER_SWAP_FLAG)) { self.callHook(abi.encodeWithSelector(IHooks.afterSwap.selector, msg.sender, key, params, delta, hookData)); } @@ -186,6 +195,7 @@ library Hooks { /// @notice calls beforeDonate hook if permissioned and validates return value function beforeDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData) internal + noSelfCall(self) { if (self.hasPermission(BEFORE_DONATE_FLAG)) { self.callHook( @@ -197,6 +207,7 @@ library Hooks { /// @notice calls afterDonate hook if permissioned and validates return value function afterDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData) internal + noSelfCall(self) { if (self.hasPermission(AFTER_DONATE_FLAG)) { self.callHook( diff --git a/test/SkipCallsTestHook.t.sol b/test/SkipCallsTestHook.t.sol index d12609b0f..317e2aaf4 100644 --- a/test/SkipCallsTestHook.t.sol +++ b/test/SkipCallsTestHook.t.sol @@ -160,6 +160,22 @@ contract SkipCallsTest is Test, Deployers, GasSnapshot { assertEq(skipCallsTestHook.counter(), 2); } + function test_gas_beforeSwap_skipIfCalledByHook() public { + SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( + address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.BEFORE_SWAP_FLAG)) + ); + + deploy(skipCallsTestHook); + approveAndAddLiquidity(skipCallsTestHook); + assertEq(skipCallsTestHook.counter(), 0); + + // swaps and increments counter + snapStart("swap skips hook call if hook is caller"); + swapRouter.swap(key, swapParams, testSettings, abi.encode(address(this))); + snapEnd(); + assertEq(skipCallsTestHook.counter(), 1); + } + function test_afterSwap_skipIfCalledByHook() public { SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook( address(uint160(type(uint160).max & clearAllHookPermisssionsMask | Hooks.AFTER_SWAP_FLAG)) From f80b503a78ce17637e59b1cfec61df96b6f339c1 Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Sun, 7 Apr 2024 22:45:00 -0400 Subject: [PATCH 15/18] revert back to relative pathing (#571) --- src/ProtocolFees.sol | 14 +++++++------- src/interfaces/IProtocolFees.sol | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol index 4775f19db..b7d2b062b 100644 --- a/src/ProtocolFees.sol +++ b/src/ProtocolFees.sol @@ -1,14 +1,14 @@ // SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.8.19; -import {Currency, CurrencyLibrary} from "src/types/Currency.sol"; -import {IProtocolFeeController} from "src/interfaces/IProtocolFeeController.sol"; -import {IProtocolFees} from "src/interfaces/IProtocolFees.sol"; -import {PoolKey} from "src/types/PoolKey.sol"; -import {ProtocolFeeLibrary} from "src/libraries/ProtocolFeeLibrary.sol"; +import {Currency, CurrencyLibrary} from "./types/Currency.sol"; +import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol"; +import {IProtocolFees} from "./interfaces/IProtocolFees.sol"; +import {PoolKey} from "./types/PoolKey.sol"; +import {ProtocolFeeLibrary} from "./libraries/ProtocolFeeLibrary.sol"; import {Owned} from "solmate/auth/Owned.sol"; -import {PoolId, PoolIdLibrary} from "src/types/PoolId.sol"; -import {Pool} from "src/libraries/Pool.sol"; +import {PoolId, PoolIdLibrary} from "./types/PoolId.sol"; +import {Pool} from "./libraries/Pool.sol"; abstract contract ProtocolFees is IProtocolFees, Owned { using CurrencyLibrary for Currency; diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol index 2e09fc08b..aeed1a0e4 100644 --- a/src/interfaces/IProtocolFees.sol +++ b/src/interfaces/IProtocolFees.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.19; -import {Currency} from "src/types/Currency.sol"; -import {IProtocolFeeController} from "src/interfaces/IProtocolFeeController.sol"; -import {PoolId} from "src/types/PoolId.sol"; -import {PoolKey} from "src/types/PoolKey.sol"; +import {Currency} from "../types/Currency.sol"; +import {IProtocolFeeController} from "../interfaces/IProtocolFeeController.sol"; +import {PoolId} from "../types/PoolId.sol"; +import {PoolKey} from "../types/PoolKey.sol"; interface IProtocolFees { /// @notice Thrown when not enough gas is provided to look up the protocol fee From c5a23f0cfad79ec59136ec5db614a6427172f5bb Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Mon, 8 Apr 2024 09:38:05 -0500 Subject: [PATCH 16/18] Simple storage and memory caching optimizations in `Pool` (#567) * Cache `tick` and `sqrtPriceX96` in `Pool.modifyLiquidity` This commit slightly modifies the `Pool` library to reduce gas usage. An intermediary variable `tick` and `sqrtPriceX96` are created to reduce the number of calls to `self.slot0`. This change results in smaller footprint in the snapshots, indicating reduction in gas consumption. * Cache `zeroForOne` in stack in `Pool.swap` Parameters in Pool.sol are now more concisely referenced with `zeroForOne` variable. This affects all the occurrences of `params.zeroForOne`, where it is replaced with `zeroForOne`. The snapshot files are also updated to reflect this change. This simplification helps to reduce bytecode size and increase overall efficiency of the code. * Cache `self.liquidity` and remove clone to memory in `Position.update` This commit focuses on optimizing the `Position.update` function from within the `Position.sol` file. It introduces a cached variable `liquidity`, thereby eliminating the need to clone `self` to memory. This change results in a more efficient use of memory and optimizes code execution. The snapshots have been updated to reflect the adjusted bytecode sizes for the various operations. * Cache `params.liquidityDelta` to stack in `Pool.modifyLiquidity` This commit primarily caches the variable `params.liquidityDelta` to stack in the `Pool.modifyLiquidity` method inside Pool.sol to improve efficiency. This tiny optimization reduces bytecode size and makes the underlying calculations faster and more efficient. This change is reflected in the updated .snap files for poolManager bytecode size and various test cases involving liquidity management. * Cache `tickLower` and `tickUpper` to stack in `Pool.modifyLiquidity` The changes in the code have optimized memory use in the `Pool.modifyLiquidity` function, by caching `tickLower` and `tickUpper` to the stack rather than repeatedly accessing them from memory. This minor adjustment not only optimizes memory usage, but also potentially improves the function performance by reducing the number of memory operations. --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- ...p against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap burn 6909 for input.snap | 2 +- .../swap burn native 6909 for input.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- ...wap skips hook call if hook is caller.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/libraries/Pool.sol | 95 +++++++++---------- src/libraries/Position.sol | 23 ++--- src/types/BalanceDelta.sol | 3 +- 22 files changed, 76 insertions(+), 83 deletions(-) diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index 019c77f29..decf71dbd 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -265381 \ No newline at end of file +264723 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index d072cabb1..e6cfc565b 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -140224 \ No newline at end of file +139579 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index 267b1c2f5..e113a319d 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -145541 \ No newline at end of file +144896 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 2654fd3e0..2d3bae8c7 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23083 \ No newline at end of file +22844 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index a7b4c4134..5a86ea086 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -56072 \ No newline at end of file +55383 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 5710db49c..162121eb0 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -148258 \ No newline at end of file +147569 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index 1bcdd5bfe..a201f0c6f 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -149722 \ No newline at end of file +149033 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index 92d965caa..cb05f85de 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -132902 \ No newline at end of file +132802 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index e03d94df2..cb5d31d48 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -146778 \ No newline at end of file +146678 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 3039612cb..54cdeca3d 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72361 \ No newline at end of file +72297 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index 13788d81c..0fbcbd755 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60364 \ No newline at end of file +60300 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 07f2cf54d..460e15d21 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80394 \ No newline at end of file +80330 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index 505243eb6..29835c4f3 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76391 \ No newline at end of file +76327 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index cad93df0b..d5557151f 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -138742 \ No newline at end of file +138678 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index 134896538..2afd20d3a 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -155593 \ No newline at end of file +155493 \ No newline at end of file diff --git a/.forge-snapshots/swap skips hook call if hook is caller.snap b/.forge-snapshots/swap skips hook call if hook is caller.snap index 41ac906bf..1907608b4 100644 --- a/.forge-snapshots/swap skips hook call if hook is caller.snap +++ b/.forge-snapshots/swap skips hook call if hook is caller.snap @@ -1 +1 @@ -156254 \ No newline at end of file +156090 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index 1456957a2..271cf9e55 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -89668 \ No newline at end of file +89568 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index 2f3e10ebe..e851edf42 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60342 \ No newline at end of file +60278 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index f44d09345..562eefa2b 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -140294 \ No newline at end of file +140194 \ No newline at end of file diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index 0165360e6..242a74197 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -156,7 +156,10 @@ library Pool { internal returns (BalanceDelta result) { - checkTicks(params.tickLower, params.tickUpper); + int128 liquidityDelta = params.liquidityDelta; + int24 tickLower = params.tickLower; + int24 tickUpper = params.tickUpper; + checkTicks(tickLower, tickUpper); uint256 feesOwed0; uint256 feesOwed1; @@ -164,82 +167,77 @@ library Pool { ModifyLiquidityState memory state; // if we need to update the ticks, do it - if (params.liquidityDelta != 0) { + if (liquidityDelta != 0) { (state.flippedLower, state.liquidityGrossAfterLower) = - updateTick(self, params.tickLower, params.liquidityDelta, false); - (state.flippedUpper, state.liquidityGrossAfterUpper) = - updateTick(self, params.tickUpper, params.liquidityDelta, true); + updateTick(self, tickLower, liquidityDelta, false); + (state.flippedUpper, state.liquidityGrossAfterUpper) = updateTick(self, tickUpper, liquidityDelta, true); - if (params.liquidityDelta > 0) { + // `>` and `>=` are logically equivalent here but `>=` is cheaper + if (liquidityDelta >= 0) { uint128 maxLiquidityPerTick = tickSpacingToMaxLiquidityPerTick(params.tickSpacing); if (state.liquidityGrossAfterLower > maxLiquidityPerTick) { - revert TickLiquidityOverflow(params.tickLower); + revert TickLiquidityOverflow(tickLower); } if (state.liquidityGrossAfterUpper > maxLiquidityPerTick) { - revert TickLiquidityOverflow(params.tickUpper); + revert TickLiquidityOverflow(tickUpper); } } if (state.flippedLower) { - self.tickBitmap.flipTick(params.tickLower, params.tickSpacing); + self.tickBitmap.flipTick(tickLower, params.tickSpacing); } if (state.flippedUpper) { - self.tickBitmap.flipTick(params.tickUpper, params.tickSpacing); + self.tickBitmap.flipTick(tickUpper, params.tickSpacing); } } - (state.feeGrowthInside0X128, state.feeGrowthInside1X128) = - getFeeGrowthInside(self, params.tickLower, params.tickUpper); + (state.feeGrowthInside0X128, state.feeGrowthInside1X128) = getFeeGrowthInside(self, tickLower, tickUpper); - (feesOwed0, feesOwed1) = self.positions.get(params.owner, params.tickLower, params.tickUpper).update( - params.liquidityDelta, state.feeGrowthInside0X128, state.feeGrowthInside1X128 - ); + Position.Info storage position = self.positions.get(params.owner, tickLower, tickUpper); + (feesOwed0, feesOwed1) = + position.update(liquidityDelta, state.feeGrowthInside0X128, state.feeGrowthInside1X128); // clear any tick data that is no longer needed - if (params.liquidityDelta < 0) { + if (liquidityDelta < 0) { if (state.flippedLower) { - clearTick(self, params.tickLower); + clearTick(self, tickLower); } if (state.flippedUpper) { - clearTick(self, params.tickUpper); + clearTick(self, tickUpper); } } } - if (params.liquidityDelta != 0) { - if (self.slot0.tick < params.tickLower) { + if (liquidityDelta != 0) { + int24 tick = self.slot0.tick; + uint160 sqrtPriceX96 = self.slot0.sqrtPriceX96; + if (tick < tickLower) { // current tick is below the passed range; liquidity can only become in range by crossing from left to // right, when we'll need _more_ currency0 (it's becoming more valuable) so user must provide it result = toBalanceDelta( SqrtPriceMath.getAmount0Delta( - TickMath.getSqrtRatioAtTick(params.tickLower), - TickMath.getSqrtRatioAtTick(params.tickUpper), - params.liquidityDelta + TickMath.getSqrtRatioAtTick(tickLower), TickMath.getSqrtRatioAtTick(tickUpper), liquidityDelta ).toInt128(), 0 ); - } else if (self.slot0.tick < params.tickUpper) { + } else if (tick < tickUpper) { result = toBalanceDelta( - SqrtPriceMath.getAmount0Delta( - self.slot0.sqrtPriceX96, TickMath.getSqrtRatioAtTick(params.tickUpper), params.liquidityDelta - ).toInt128(), - SqrtPriceMath.getAmount1Delta( - TickMath.getSqrtRatioAtTick(params.tickLower), self.slot0.sqrtPriceX96, params.liquidityDelta - ).toInt128() + SqrtPriceMath.getAmount0Delta(sqrtPriceX96, TickMath.getSqrtRatioAtTick(tickUpper), liquidityDelta) + .toInt128(), + SqrtPriceMath.getAmount1Delta(TickMath.getSqrtRatioAtTick(tickLower), sqrtPriceX96, liquidityDelta) + .toInt128() ); - self.liquidity = params.liquidityDelta < 0 - ? self.liquidity - uint128(-params.liquidityDelta) - : self.liquidity + uint128(params.liquidityDelta); + self.liquidity = liquidityDelta < 0 + ? self.liquidity - uint128(-liquidityDelta) + : self.liquidity + uint128(liquidityDelta); } else { // current tick is above the passed range; liquidity can only become in range by crossing from right to // left, when we'll need _more_ currency1 (it's becoming more valuable) so user must provide it result = toBalanceDelta( 0, SqrtPriceMath.getAmount1Delta( - TickMath.getSqrtRatioAtTick(params.tickLower), - TickMath.getSqrtRatioAtTick(params.tickUpper), - params.liquidityDelta + TickMath.getSqrtRatioAtTick(tickLower), TickMath.getSqrtRatioAtTick(tickUpper), liquidityDelta ).toInt128() ); } @@ -306,7 +304,8 @@ library Pool { Slot0 memory slot0Start = self.slot0; swapFee = slot0Start.swapFee; - if (params.zeroForOne) { + bool zeroForOne = params.zeroForOne; + if (zeroForOne) { if (params.sqrtPriceLimitX96 >= slot0Start.sqrtPriceX96) { revert PriceLimitAlreadyExceeded(slot0Start.sqrtPriceX96, params.sqrtPriceLimitX96); } @@ -324,9 +323,7 @@ library Pool { SwapCache memory cache = SwapCache({ liquidityStart: self.liquidity, - protocolFee: params.zeroForOne - ? slot0Start.protocolFee.getZeroForOneFee() - : slot0Start.protocolFee.getOneForZeroFee() + protocolFee: zeroForOne ? slot0Start.protocolFee.getZeroForOneFee() : slot0Start.protocolFee.getOneForZeroFee() }); bool exactInput = params.amountSpecified < 0; @@ -336,7 +333,7 @@ library Pool { amountCalculated: 0, sqrtPriceX96: slot0Start.sqrtPriceX96, tick: slot0Start.tick, - feeGrowthGlobalX128: params.zeroForOne ? self.feeGrowthGlobal0X128 : self.feeGrowthGlobal1X128, + feeGrowthGlobalX128: zeroForOne ? self.feeGrowthGlobal0X128 : self.feeGrowthGlobal1X128, liquidity: cache.liquidityStart }); @@ -346,7 +343,7 @@ library Pool { step.sqrtPriceStartX96 = state.sqrtPriceX96; (step.tickNext, step.initialized) = - self.tickBitmap.nextInitializedTickWithinOneWord(state.tick, params.tickSpacing, params.zeroForOne); + self.tickBitmap.nextInitializedTickWithinOneWord(state.tick, params.tickSpacing, zeroForOne); // ensure that we do not overshoot the min/max tick, as the tick bitmap is not aware of these bounds if (step.tickNext < TickMath.MIN_TICK) { @@ -362,7 +359,7 @@ library Pool { (state.sqrtPriceX96, step.amountIn, step.amountOut, step.feeAmount) = SwapMath.computeSwapStep( state.sqrtPriceX96, ( - params.zeroForOne + zeroForOne ? step.sqrtPriceNextX96 < params.sqrtPriceLimitX96 : step.sqrtPriceNextX96 > params.sqrtPriceLimitX96 ) ? params.sqrtPriceLimitX96 : step.sqrtPriceNextX96, @@ -409,13 +406,13 @@ library Pool { int128 liquidityNet = Pool.crossTick( self, step.tickNext, - (params.zeroForOne ? state.feeGrowthGlobalX128 : self.feeGrowthGlobal0X128), - (params.zeroForOne ? self.feeGrowthGlobal1X128 : state.feeGrowthGlobalX128) + (zeroForOne ? state.feeGrowthGlobalX128 : self.feeGrowthGlobal0X128), + (zeroForOne ? self.feeGrowthGlobal1X128 : state.feeGrowthGlobalX128) ); // if we're moving leftward, we interpret liquidityNet as the opposite sign // safe because liquidityNet cannot be type(int128).min unchecked { - if (params.zeroForOne) liquidityNet = -liquidityNet; + if (zeroForOne) liquidityNet = -liquidityNet; } state.liquidity = liquidityNet < 0 @@ -424,7 +421,7 @@ library Pool { } unchecked { - state.tick = params.zeroForOne ? step.tickNext - 1 : step.tickNext; + state.tick = zeroForOne ? step.tickNext - 1 : step.tickNext; } } else if (state.sqrtPriceX96 != step.sqrtPriceStartX96) { // recompute unless we're on a lower tick boundary (i.e. already transitioned ticks), and haven't moved @@ -438,14 +435,14 @@ library Pool { if (cache.liquidityStart != state.liquidity) self.liquidity = state.liquidity; // update fee growth global - if (params.zeroForOne) { + if (zeroForOne) { self.feeGrowthGlobal0X128 = state.feeGrowthGlobalX128; } else { self.feeGrowthGlobal1X128 = state.feeGrowthGlobalX128; } unchecked { - if (params.zeroForOne == exactInput) { + if (zeroForOne == exactInput) { result = toBalanceDelta( (params.amountSpecified - state.amountSpecifiedRemaining).toInt128(), state.amountCalculated.toInt128() diff --git a/src/libraries/Position.sol b/src/libraries/Position.sol index cb0149d6a..2a439f0a5 100644 --- a/src/libraries/Position.sol +++ b/src/libraries/Position.sol @@ -29,7 +29,7 @@ library Position { function get(mapping(bytes32 => Info) storage self, address owner, int24 tickLower, int24 tickUpper) internal view - returns (Position.Info storage position) + returns (Info storage position) { position = self[keccak256(abi.encodePacked(owner, tickLower, tickUpper))]; } @@ -47,26 +47,23 @@ library Position { uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128 ) internal returns (uint256 feesOwed0, uint256 feesOwed1) { - Info memory _self = self; + uint128 liquidity = self.liquidity; uint128 liquidityNext; if (liquidityDelta == 0) { - if (_self.liquidity == 0) revert CannotUpdateEmptyPosition(); // disallow pokes for 0 liquidity positions - liquidityNext = _self.liquidity; + if (liquidity == 0) revert CannotUpdateEmptyPosition(); // disallow pokes for 0 liquidity positions + liquidityNext = liquidity; } else { - liquidityNext = liquidityDelta < 0 - ? _self.liquidity - uint128(-liquidityDelta) - : _self.liquidity + uint128(liquidityDelta); + liquidityNext = + liquidityDelta < 0 ? liquidity - uint128(-liquidityDelta) : liquidity + uint128(liquidityDelta); } // calculate accumulated fees. overflow in the subtraction of fee growth is expected unchecked { - feesOwed0 = FullMath.mulDiv( - feeGrowthInside0X128 - _self.feeGrowthInside0LastX128, _self.liquidity, FixedPoint128.Q128 - ); - feesOwed1 = FullMath.mulDiv( - feeGrowthInside1X128 - _self.feeGrowthInside1LastX128, _self.liquidity, FixedPoint128.Q128 - ); + feesOwed0 = + FullMath.mulDiv(feeGrowthInside0X128 - self.feeGrowthInside0LastX128, liquidity, FixedPoint128.Q128); + feesOwed1 = + FullMath.mulDiv(feeGrowthInside1X128 - self.feeGrowthInside1LastX128, liquidity, FixedPoint128.Q128); } // update the position diff --git a/src/types/BalanceDelta.sol b/src/types/BalanceDelta.sol index 7a6e53406..49ae295e0 100644 --- a/src/types/BalanceDelta.sol +++ b/src/types/BalanceDelta.sol @@ -9,8 +9,7 @@ using BalanceDeltaLibrary for BalanceDelta global; function toBalanceDelta(int128 _amount0, int128 _amount1) pure returns (BalanceDelta balanceDelta) { /// @solidity memory-safe-assembly assembly { - balanceDelta := - or(shl(128, _amount0), and(0x00000000000000000000000000000000ffffffffffffffffffffffffffffffff, _amount1)) + balanceDelta := or(shl(128, _amount0), and(0xffffffffffffffffffffffffffffffff, _amount1)) } } From 9f50d5df8bb5c2172f16306090d0653e289c1556 Mon Sep 17 00:00:00 2001 From: Mikhail Abramov Date: Mon, 8 Apr 2024 17:07:27 +0200 Subject: [PATCH 17/18] Replace modulo with bit operations (#483) --- .forge-snapshots/addLiquidity with empty hook.snap | 2 +- .forge-snapshots/addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- ...ipTick_gasCostOfFlippingATickThatResultsInDeletingAWord.snap | 2 +- .../flipTick_gasCostOfFlippingFirstTickInWordToInitialized.snap | 2 +- ...flipTick_gasCostOfFlippingSecondTickInWordToInitialized.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- ...tializedTickWithinOneWord_lteFalse_gasCostForEntireWord.snap | 2 +- ...izedTickWithinOneWord_lteFalse_gasCostJustBelowBoundary.snap | 2 +- ...InitializedTickWithinOneWord_lteFalse_gasCostOnBoundary.snap | 2 +- ...itializedTickWithinOneWord_lteTrue_gasCostForEntireWord.snap | 2 +- ...lizedTickWithinOneWord_lteTrue_gasCostJustBelowBoundary.snap | 2 +- ...tInitializedTickWithinOneWord_lteTrue_gasCostOnBoundary.snap | 2 +- .forge-snapshots/poolManager bytecode size.snap | 2 +- .forge-snapshots/removeLiquidity with empty hook.snap | 2 +- .forge-snapshots/removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- .forge-snapshots/swap against liquidity with native token.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .forge-snapshots/swap burn 6909 for input.snap | 2 +- .forge-snapshots/swap burn native 6909 for input.snap | 2 +- .forge-snapshots/swap mint native output as 6909.snap | 2 +- .forge-snapshots/swap mint output as 6909.snap | 2 +- .forge-snapshots/swap skips hook call if hook is caller.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .forge-snapshots/update dynamic fee in before swap.snap | 2 +- src/libraries/ProtocolFeeLibrary.sol | 2 +- src/libraries/TickBitmap.sol | 2 +- src/libraries/TickMath.sol | 2 +- 32 files changed, 32 insertions(+), 32 deletions(-) diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index decf71dbd..602dee3b3 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -264723 \ No newline at end of file +264715 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index e6cfc565b..96a4e444d 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -139579 \ No newline at end of file +139575 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index e113a319d..bb319b812 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -144896 \ No newline at end of file +144892 \ No newline at end of file diff --git a/.forge-snapshots/flipTick_gasCostOfFlippingATickThatResultsInDeletingAWord.snap b/.forge-snapshots/flipTick_gasCostOfFlippingATickThatResultsInDeletingAWord.snap index 3819e9389..e3b929b4e 100644 --- a/.forge-snapshots/flipTick_gasCostOfFlippingATickThatResultsInDeletingAWord.snap +++ b/.forge-snapshots/flipTick_gasCostOfFlippingATickThatResultsInDeletingAWord.snap @@ -1 +1 @@ -5409 \ No newline at end of file +5407 \ No newline at end of file diff --git a/.forge-snapshots/flipTick_gasCostOfFlippingFirstTickInWordToInitialized.snap b/.forge-snapshots/flipTick_gasCostOfFlippingFirstTickInWordToInitialized.snap index 0cd31fe26..e602141e2 100644 --- a/.forge-snapshots/flipTick_gasCostOfFlippingFirstTickInWordToInitialized.snap +++ b/.forge-snapshots/flipTick_gasCostOfFlippingFirstTickInWordToInitialized.snap @@ -1 +1 @@ -22506 \ No newline at end of file +22504 \ No newline at end of file diff --git a/.forge-snapshots/flipTick_gasCostOfFlippingSecondTickInWordToInitialized.snap b/.forge-snapshots/flipTick_gasCostOfFlippingSecondTickInWordToInitialized.snap index 38966a2c0..aa0e4d419 100644 --- a/.forge-snapshots/flipTick_gasCostOfFlippingSecondTickInWordToInitialized.snap +++ b/.forge-snapshots/flipTick_gasCostOfFlippingSecondTickInWordToInitialized.snap @@ -1 +1 @@ -5515 \ No newline at end of file +5513 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index 9576f45df..33f4191f2 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -51268 \ No newline at end of file +51266 \ No newline at end of file diff --git a/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostForEntireWord.snap b/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostForEntireWord.snap index 13f668d19..0f3866130 100644 --- a/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostForEntireWord.snap +++ b/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostForEntireWord.snap @@ -1 +1 @@ -2592 \ No newline at end of file +2578 \ No newline at end of file diff --git a/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostJustBelowBoundary.snap b/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostJustBelowBoundary.snap index 13f668d19..0f3866130 100644 --- a/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostJustBelowBoundary.snap +++ b/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostJustBelowBoundary.snap @@ -1 +1 @@ -2592 \ No newline at end of file +2578 \ No newline at end of file diff --git a/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostOnBoundary.snap b/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostOnBoundary.snap index 13f668d19..0f3866130 100644 --- a/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostOnBoundary.snap +++ b/.forge-snapshots/nextInitializedTickWithinOneWord_lteFalse_gasCostOnBoundary.snap @@ -1 +1 @@ -2592 \ No newline at end of file +2578 \ No newline at end of file diff --git a/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostForEntireWord.snap b/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostForEntireWord.snap index 0c52fc6da..a3412f309 100644 --- a/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostForEntireWord.snap +++ b/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostForEntireWord.snap @@ -1 +1 @@ -2591 \ No newline at end of file +2556 \ No newline at end of file diff --git a/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostJustBelowBoundary.snap b/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostJustBelowBoundary.snap index 7b34ebbf2..47fd2ab34 100644 --- a/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostJustBelowBoundary.snap +++ b/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostJustBelowBoundary.snap @@ -1 +1 @@ -2900 \ No newline at end of file +2865 \ No newline at end of file diff --git a/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostOnBoundary.snap b/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostOnBoundary.snap index 0c52fc6da..a3412f309 100644 --- a/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostOnBoundary.snap +++ b/.forge-snapshots/nextInitializedTickWithinOneWord_lteTrue_gasCostOnBoundary.snap @@ -1 +1 @@ -2591 \ No newline at end of file +2556 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 2d3bae8c7..e7b20daf1 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -22844 \ No newline at end of file +22769 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index 5a86ea086..84cc40d24 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -55383 \ No newline at end of file +55375 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 162121eb0..279f2a24c 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -147569 \ No newline at end of file +147561 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index a201f0c6f..19a57a673 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -149033 \ No newline at end of file +149025 \ No newline at end of file diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap index cb05f85de..bb2d92b47 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -132802 \ No newline at end of file +132600 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index cb5d31d48..a150e2e48 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -146678 \ No newline at end of file +146476 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap index 54cdeca3d..82b18dab6 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -72297 \ No newline at end of file +72132 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index 0fbcbd755..2f58c626a 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -60300 \ No newline at end of file +60135 \ No newline at end of file diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap index 460e15d21..edbef057b 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -80330 \ No newline at end of file +80312 \ No newline at end of file diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap index 29835c4f3..795e97fab 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -76327 \ No newline at end of file +76162 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index d5557151f..9fbea3b86 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -138678 \ No newline at end of file +138660 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index 2afd20d3a..d8ad01be3 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -155493 \ No newline at end of file +155291 \ No newline at end of file diff --git a/.forge-snapshots/swap skips hook call if hook is caller.snap b/.forge-snapshots/swap skips hook call if hook is caller.snap index 1907608b4..828d0d44f 100644 --- a/.forge-snapshots/swap skips hook call if hook is caller.snap +++ b/.forge-snapshots/swap skips hook call if hook is caller.snap @@ -1 +1 @@ -156090 \ No newline at end of file +155723 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index 271cf9e55..5d173b590 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -89568 \ No newline at end of file +89366 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index e851edf42..e182446d6 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -60278 \ No newline at end of file +60113 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index 562eefa2b..6a2708470 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -140194 \ No newline at end of file +139992 \ No newline at end of file diff --git a/src/libraries/ProtocolFeeLibrary.sol b/src/libraries/ProtocolFeeLibrary.sol index 4bddc6df1..28d75dc3d 100644 --- a/src/libraries/ProtocolFeeLibrary.sol +++ b/src/libraries/ProtocolFeeLibrary.sol @@ -9,7 +9,7 @@ library ProtocolFeeLibrary { uint16 internal constant BIPS_DENOMINATOR = 10_000; function getZeroForOneFee(uint24 self) internal pure returns (uint16) { - return uint16(self % 4096); + return uint16(self & (4096 - 1)); } function getOneForZeroFee(uint24 self) internal pure returns (uint16) { diff --git a/src/libraries/TickBitmap.sol b/src/libraries/TickBitmap.sol index 0bdefcaa1..b04d5d19e 100644 --- a/src/libraries/TickBitmap.sol +++ b/src/libraries/TickBitmap.sol @@ -19,7 +19,7 @@ library TickBitmap { function position(int24 tick) internal pure returns (int16 wordPos, uint8 bitPos) { unchecked { wordPos = int16(tick >> 8); - bitPos = uint8(int8(tick % 256)); + bitPos = uint8(int8(tick & (256 - 1))); } } diff --git a/src/libraries/TickMath.sol b/src/libraries/TickMath.sol index cd8e5cf01..4c9b95b94 100644 --- a/src/libraries/TickMath.sol +++ b/src/libraries/TickMath.sol @@ -76,7 +76,7 @@ library TickMath { // this divides by 1<<32 rounding up to go from a Q128.128 to a Q128.96. // we then downcast because we know the result always fits within 160 bits due to our tick input constraint // we round up in the division so getTickAtSqrtRatio of the output price is always consistent - sqrtPriceX96 = uint160((ratio >> 32) + (ratio % (1 << 32) == 0 ? 0 : 1)); + sqrtPriceX96 = uint160((ratio >> 32) + (ratio & ((1 << 32) - 1) == 0 ? 0 : 1)); } } From 126ec8670566e17707683edf05461a106bead129 Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Mon, 8 Apr 2024 10:55:11 -0500 Subject: [PATCH 18/18] do `abi.encodePacked` with inline assembly in `Position.get` (#279) * do `abi.encodePacked` with inline assembly * Update gas snapshots * Add test for `Position.get` * Update gas snapshots * Update gas snapshots --- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- .forge-snapshots/addLiquidity.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- .forge-snapshots/removeLiquidity.snap | 2 +- src/libraries/Position.sol | 11 ++++++++- test/libraries/Position.t.sol | 24 +++++++++++++++++++ 9 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 test/libraries/Position.t.sol diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index 602dee3b3..9d842f32a 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -264715 \ No newline at end of file +264566 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index 96a4e444d..f004ea00c 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -139575 \ No newline at end of file +139426 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap index bb319b812..d098236d6 100644 --- a/.forge-snapshots/addLiquidity.snap +++ b/.forge-snapshots/addLiquidity.snap @@ -1 +1 @@ -144892 \ No newline at end of file +144743 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index e7b20daf1..838b1c452 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -22769 \ No newline at end of file +22598 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap index 84cc40d24..cc078112e 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -55375 \ No newline at end of file +55226 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap index 279f2a24c..52870c04c 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -147561 \ No newline at end of file +147412 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap index 19a57a673..1c6861c96 100644 --- a/.forge-snapshots/removeLiquidity.snap +++ b/.forge-snapshots/removeLiquidity.snap @@ -1 +1 @@ -149025 \ No newline at end of file +148876 \ No newline at end of file diff --git a/src/libraries/Position.sol b/src/libraries/Position.sol index 2a439f0a5..ec0b5f3e4 100644 --- a/src/libraries/Position.sol +++ b/src/libraries/Position.sol @@ -31,7 +31,16 @@ library Position { view returns (Info storage position) { - position = self[keccak256(abi.encodePacked(owner, tickLower, tickUpper))]; + // positionKey = keccak256(abi.encodePacked(owner, tickLower, tickUpper)) + bytes32 positionKey; + /// @solidity memory-safe-assembly + assembly { + mstore(0x06, tickUpper) // [0x23, 0x26) + mstore(0x03, tickLower) // [0x20, 0x23) + mstore(0, owner) // [0x0c, 0x20) + positionKey := keccak256(0x0c, 0x1a) + } + position = self[positionKey]; } /// @notice Credits accumulated fees to a user's position diff --git a/test/libraries/Position.t.sol b/test/libraries/Position.t.sol new file mode 100644 index 000000000..cda1da7ca --- /dev/null +++ b/test/libraries/Position.t.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {Position} from "src/libraries/Position.sol"; + +contract PositionTest is Test { + using Position for mapping(bytes32 => Position.Info); + + mapping(bytes32 => Position.Info) internal positions; + + function test_get_fuzz(address owner, int24 tickLower, int24 tickUpper) public { + bytes32 positionKey = keccak256(abi.encodePacked(owner, tickLower, tickUpper)); + Position.Info storage expectedPosition = positions[positionKey]; + Position.Info storage position = positions.get(owner, tickLower, tickUpper); + bytes32 expectedPositionSlot; + bytes32 positionSlot; + assembly ("memory-safe") { + expectedPositionSlot := expectedPosition.slot + positionSlot := position.slot + } + assertEq(positionSlot, expectedPositionSlot, "slots not equal"); + } +}