feat: refactor the fee mechanism for both pools (#36)

* feat: refactor the fee mechanism for both pools

* fix: make it compiles

* fix: updating related test cases

* feat: more accuarate calculation for protocolFee

* optimization: remove unnecessary struct in clpool#swap

* optimization: comments and small adjustment per review

* optimization: cache memory variable in clpool#swap

* chore: add gas snapshots back

.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap

@@ -1 +1 @@
-181146
+181124

.forge-snapshots/BinHookTest#testInitializeSucceedsWithHook.snap

@@ -1 +1 @@
-136997
+137037

.forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap

@@ -1 +1 @@
-329657
+329719

.forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap

@@ -1 +1 @@
-192259
+192675

.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap

@@ -1 +1 @@
-142035
+142017

.forge-snapshots/BinPoolManagerTest#testFuzzUpdateDynamicLPFee.snap

@@ -0,0 +1 @@
+32585

.forge-snapshots/BinPoolManagerTest#testFuzz_SetMaxBinStep.snap

@@ -1 +1 @@
-30395
+30350

.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap

@@ -1 +1 @@
-158207
+158185

.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap

@@ -1 +1 @@
-303831
+303813

.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap

@@ -1 +1 @@
-139396
+139378

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap

@@ -1 +1 @@
-1013119
+1014201

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap

@@ -1 +1 @@
-341133
+342215

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap

@@ -1 +1 @@
-380471
+380533

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap

@@ -1 +1 @@
-149232
+149294

.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap

@@ -1 +1 @@
-187313
+187651

.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap

@@ -1 +1 @@
-193298
+193636

.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap

@@ -1 +1 @@
-145662
+146078

.forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap

@@ -1 +1 @@
-327364
+327426

.forge-snapshots/BinPoolManagerTest#testNoOpGas_Burn.snap

@@ -1 +1 @@
-76641
+76619

.forge-snapshots/BinPoolManagerTest#testNoOpGas_Initialize.snap

@@ -1 +1 @@
-64622
+64662

.forge-snapshots/BinPoolManagerTest#testSetProtocolFee.snap

@@ -1 +1 @@
-41349
+34422

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap

@@ -1 +1 @@
-401422
+401400

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap

@@ -1 +1 @@
-182410
+182388

.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap

@@ -1 +1 @@
-247549
+247527

.forge-snapshots/CLPoolManagerTest#donateBothTokens.snap

@@ -1 +1 @@
-175518
+175562

.forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap

@@ -1 +1 @@
-116900
+116944

.forge-snapshots/CLPoolManagerTest#initializeWithoutHooks.snap

@@ -1 +1 @@
-59356
+59393

.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap

@@ -1 +1 @@
-128995
+128973

.forge-snapshots/CLPoolManagerTest#swap_againstLiquidity.snap

@@ -1 +1 @@
-148767
+148772

.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap

@@ -1 +1 @@
-177177
+177222

.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap

@@ -1 +1 @@
-24940739
+25086624

.forge-snapshots/CLPoolManagerTest#swap_simple.snap

@@ -1 +1 @@
-78822
+78867

.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap

@@ -1 +1 @@
-158284
+158439

.forge-snapshots/CLPoolManagerTest#swap_withHooks.snap

@@ -1 +1 @@
-95329
+95374

.forge-snapshots/CLPoolManagerTest#swap_withNative.snap

@@ -1 +1 @@
-78825
+78870

.forge-snapshots/CLPoolManagerTest#testFuzzUpdateDynamicLPFee.snap

@@ -0,0 +1 @@
+32293

.forge-snapshots/CLPoolManagerTest#testNoOp_gas_Donate.snap

@@ -1 +1 @@
-53926
+53970

.forge-snapshots/CLPoolManagerTest#testNoOp_gas_Initialize.snap

@@ -1 +1 @@
-65078
+65115

.forge-snapshots/CLPoolManagerTest#testNoOp_gas_ModifyPosition.snap

@@ -1 +1 @@
-60244
+60222

.forge-snapshots/SwapMathTest#SwapOneForZeroExactInCapped.snap

@@ -1 +1 @@
-2197
+2237

.forge-snapshots/SwapMathTest#SwapOneForZeroExactOutCapped.snap

@@ -1 +1 @@
-1954
+1994

.forge-snapshots/SwapMathTest#SwapZeroForOneExactInCapped.snap

@@ -1 +1 @@
-2187
+2227

.forge-snapshots/SwapMathTest#SwapZeroForOneExactOutCapped.snap

@@ -1 +1 @@
-1944
+1984

.forge-snapshots/VaultTest#lockSettledWhenAddLiquidity.snap

@@ -1 +1 @@
-159350
+159328

.forge-snapshots/VaultTest#lockSettledWhenMultiHopSwap.snap

@@ -1 +1 @@
-118273
+118251

.forge-snapshots/VaultTest#lockSettledWhenSwap.snap

@@ -1 +1 @@
-118272
+118250

src/Fees.sol → src/ProtocolFees.sol

@@ -5,12 +5,15 @@ pragma solidity ^0.8.24;
 import {PausableRole} from "./PausableRole.sol";
 import {Currency} from "./types/Currency.sol";
 import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol";
-import {IFees} from "./interfaces/IFees.sol";
+import {IProtocolFees} from "./interfaces/IProtocolFees.sol";
+import {ProtocolFeeLibrary} from "./libraries/ProtocolFeeLibrary.sol";
 import {PoolKey} from "./types/PoolKey.sol";
+import {PoolId, PoolIdLibrary} from "./types/PoolId.sol";
 import {IVault} from "./interfaces/IVault.sol";
 
-abstract contract Fees is IFees, PausableRole {
-    uint8 public constant MIN_PROTOCOL_FEE_DENOMINATOR = 4;
+abstract contract ProtocolFees is IProtocolFees, PausableRole {
+    using PoolIdLibrary for PoolKey;
+    using ProtocolFeeLibrary for uint24;
 
     mapping(Currency currency => uint256) public protocolFeesAccrued;
 
@@ -24,11 +27,22 @@ abstract contract Fees is IFees, PausableRole {
         controllerGasLimit = _controllerGasLimit;
     }
 
+    function _setProtocolFee(PoolId id, uint24 newProtocolFee) internal virtual;
+
+    /// @inheritdoc IProtocolFees
+    function setProtocolFee(PoolKey memory key, uint24 newProtocolFee) external virtual {
+        if (msg.sender != address(protocolFeeController)) revert InvalidCaller();
+        if (!newProtocolFee.validate()) revert FeeTooLarge();
+        PoolId id = key.toId();
+        _setProtocolFee(id, newProtocolFee);
+        emit ProtocolFeeUpdated(id, newProtocolFee);
+    }
+
     /// @notice Fetch the protocol fee for a given pool, returning false if the call fails or the returned fee is 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 fee is 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 protocolFee) {
+    function _fetchProtocolFee(PoolKey memory key) internal returns (bool success, uint24 protocolFee) {
         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
@@ -45,26 +59,12 @@ abstract contract Fees is IFees, PausableRole {
             assembly {
                 returnData := mload(add(_data, 0x20))
             }
-            // Ensure return data does not overflow a uint16 and that the underlying fees are within bounds.
-            (success, protocolFee) = returnData == uint16(returnData) && _isFeeWithinBounds(uint16(returnData))
-                ? (true, uint16(returnData))
-                : (false, 0);
-        }
-    }
 
-    /// @dev the lower 8 bits for fee0 and upper 8 bits for fee1
-    function _isFeeWithinBounds(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;
-            }
+            // Ensure return data does not overflow a uint24 and that the underlying fees are within bounds.
+            (success, protocolFee) = (returnData == uint24(returnData)) && uint24(returnData).validate()
+                ? (true, uint24(returnData))
+                : (false, 0);
         }
-        return true;
     }
 
     function setProtocolFeeController(IProtocolFeeController controller) external onlyOwner {
@@ -77,7 +77,7 @@ abstract contract Fees is IFees, PausableRole {
         override
         returns (uint256 amountCollected)
     {
-        if (msg.sender != owner() && msg.sender != address(protocolFeeController)) revert InvalidProtocolFeeCollector();
+        if (msg.sender != owner() && msg.sender != address(protocolFeeController)) revert InvalidCaller();
 
         amountCollected = (amount == 0) ? protocolFeesAccrued[currency] : amount;
         protocolFeesAccrued[currency] -= amountCollected;

src/interfaces/IPoolManager.sol

@@ -11,26 +11,18 @@ interface IPoolManager {
     /// @notice PoolKey must have currencies where address(currency0) < address(currency1)
     error CurrenciesInitializedOutOfOrder();
 
-    /// @notice Thrown when a call to updateDynamicSwapFee is made by an address that is not the hook,
-    /// or on a pool that does not have a dynamic swap fee.
-    error UnauthorizedDynamicSwapFeeUpdate();
+    /// @notice Thrown when a call to updateDynamicLPFee is made by an address that is not the hook,
+    /// or on a pool is not a dynamic fee pool.
+    error UnauthorizedDynamicLPFeeUpdate();
 
-    /// @notice Emitted when protocol fee is updated
-    /// @dev The event is emitted even if the updated protocolFee is the same as previous protocolFee
-    event ProtocolFeeUpdated(PoolId indexed id, uint16 protocolFee);
+    /// @notice Emitted when lp fee is updated
+    /// @dev The event is emitted even if the updated fee value is the same as previous one
+    event DynamicLPFeeUpdated(PoolId indexed id, uint24 dynamicLPFee);
 
-    /// @notice Emitted when swap fee is updated
-    /// @dev The event is emitted even if the updated swap fee is the same as previous swap fee
-    event DynamicSwapFeeUpdated(PoolId indexed id, uint24 dynamicSwapFee);
-
-    /// @notice Sets the protocol's swap fee for the given pool
-    /// Protocol fee is always a portion of swap fee that is owed. If that underlying fee is 0, no protocol fee will accrue even if it is set to > 0.
-    function setProtocolFee(PoolKey memory key) external;
-
-    /// @notice Updates the pools swap fee for the a pool that has enabled dynamic swap fee.
+    /// @notice Updates lp fee for a dyanmic fee pool
     /// @dev Some of the use case could be:
-    ///   1) when hook#beforeSwap() is called and hook call this function to update the swap fee
-    ///   2) For BinPool only, when hook#beforeMint() is called and hook call this function to update the swap fee
-    ///   3) other use case where the hook might want to on an ad-hoc basis increase/reduce swap fee
-    function updateDynamicSwapFee(PoolKey memory key, uint24 newDynamicSwapFee) external;
+    ///   1) when hook#beforeSwap() is called and hook call this function to update the lp fee
+    ///   2) For BinPool only, when hook#beforeMint() is called and hook call this function to update the lp fee
+    ///   3) other use case where the hook might want to on an ad-hoc basis increase/reduce lp fee
+    function updateDynamicLPFee(PoolKey memory key, uint24 newDynamicLPFee) external;
 }

src/interfaces/IProtocolFeeController.sol

@@ -7,5 +7,5 @@ interface IProtocolFeeController {
     /// @notice Returns the protocol fee 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);
 }

src/interfaces/IFees.sol → src/interfaces/IProtocolFees.sol

@@ -3,25 +3,29 @@ pragma solidity ^0.8.24;
 
 import {Currency} from "../types/Currency.sol";
 import {IProtocolFeeController} from "./IProtocolFeeController.sol";
+import {PoolId} from "../types/PoolId.sol";
+import {PoolKey} from "../types/PoolKey.sol";
 
-interface IFees {
-    /// @notice Thrown when the protocol fee denominator is less than 4. Also thrown when the static or dynamic fee on a pool exceeds the upper limit.
+interface IProtocolFees {
+    /// @notice Thrown when the protocol fee exceeds the upper limit.
     error FeeTooLarge();
     /// @notice Thrown when not enough gas is provided to look up the protocol fee
     error ProtocolFeeCannotBeFetched();
-    /// @notice Thrown when user not authorized to collect protocol fee
-    error InvalidProtocolFeeCollector();
-    /// @notice Thrown when the call to fetch the protocol fee reverts or returns invalid data.
-    error ProtocolFeeControllerCallFailedOrInvalidResult();
+    /// @notice Thrown when user not authorized to set or collect protocol fee
+    error InvalidCaller();
 
-    event ProtocolFeeControllerUpdated(address protocolFeeController);
+    /// @notice Emitted when protocol fee is updated
+    /// @dev The event is emitted even if the updated protocolFee is the same as previous protocolFee
+    event ProtocolFeeUpdated(PoolId indexed id, uint24 protocolFee);
 
-    /// @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);
+    event ProtocolFeeControllerUpdated(address protocolFeeController);
 
     /// @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
+    function setProtocolFee(PoolKey memory key, uint24 newProtocolFee) external;
+
     /// @notice Update the protocol fee controller, called by the owner
     function setProtocolFeeController(IProtocolFeeController controller) external;