From d361b550e594874557c768b3ada480ab0fad9a82 Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Wed, 16 Oct 2024 13:21:57 -0400 Subject: [PATCH 1/6] round swap fee up and check if protocol fee = swap fee --- ...o already existing position with salt.snap | 2 +- .forge-snapshots/addLiquidity CA fee.snap | 2 +- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- ...new liquidity to a position with salt.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .../erc20 collect protocol fees.snap | 2 +- .../extsload getFeeGrowthGlobals.snap | 2 +- .../extsload getPositionInfo.snap | 2 +- .../extsload getTickFeeGrowthOutside.snap | 2 +- .forge-snapshots/extsload getTickInfo.snap | 2 +- .forge-snapshots/initialize.snap | 2 +- .../native collect protocol fees.snap | 2 +- .../poolManager bytecode size.snap | 2 +- .forge-snapshots/removeLiquidity CA fee.snap | 2 +- .../removeLiquidity with empty hook.snap | 2 +- .../removeLiquidity with native token.snap | 2 +- ...dLiquidity second addition same range.snap | 2 +- .forge-snapshots/simple addLiquidity.snap | 2 +- ...emoveLiquidity some liquidity remains.snap | 2 +- .forge-snapshots/simple removeLiquidity.snap | 2 +- .forge-snapshots/simple swap with native.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- .../swap CA custom curve + swap noop.snap | 2 +- .../swap CA fee on unspecified.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 +- .../swap with return dynamic fee.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/libraries/Pool.sol | 21 ++++++++---- src/libraries/ProtocolFeeLibrary.sol | 7 ++-- test/DynamicFees.t.sol | 33 ++++++++++++------- test/libraries/ProtocolFeeLibrary.t.sol | 14 ++++---- 41 files changed, 84 insertions(+), 65 deletions(-) diff --git a/.forge-snapshots/add liquidity to already existing position with salt.snap b/.forge-snapshots/add liquidity to already existing position with salt.snap index 7a1586d38..229a6f36b 100644 --- a/.forge-snapshots/add liquidity to already existing position with salt.snap +++ b/.forge-snapshots/add liquidity to already existing position with salt.snap @@ -1 +1 @@ -144651 \ No newline at end of file +144615 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity CA fee.snap b/.forge-snapshots/addLiquidity CA fee.snap index 25b3f6d86..2dbb0582b 100644 --- a/.forge-snapshots/addLiquidity CA fee.snap +++ b/.forge-snapshots/addLiquidity CA fee.snap @@ -1 +1 @@ -170947 \ No newline at end of file +170904 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index a5f14cb49..c402ac3f7 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -274218 \ No newline at end of file +274182 \ 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 a7684fdf5..007796950 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -135141 \ No newline at end of file +135108 \ No newline at end of file diff --git a/.forge-snapshots/create new liquidity to a position with salt.snap b/.forge-snapshots/create new liquidity to a position with salt.snap index cfd48079e..35994a0d0 100644 --- a/.forge-snapshots/create new liquidity to a position with salt.snap +++ b/.forge-snapshots/create new liquidity to a position with salt.snap @@ -1 +1 @@ -292843 \ No newline at end of file +292807 \ 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 f2a99d3bb..2b407e08d 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -106333 \ No newline at end of file +106309 \ 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 81e2b0d13..c6de840fc 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -145754 \ No newline at end of file +145724 \ 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 bb8641235..2a8ab2b2d 100644 --- a/.forge-snapshots/erc20 collect protocol fees.snap +++ b/.forge-snapshots/erc20 collect protocol fees.snap @@ -1 +1 @@ -57442 \ No newline at end of file +57439 \ No newline at end of file diff --git a/.forge-snapshots/extsload getFeeGrowthGlobals.snap b/.forge-snapshots/extsload getFeeGrowthGlobals.snap index 5c1d4541a..fc0b67d07 100644 --- a/.forge-snapshots/extsload getFeeGrowthGlobals.snap +++ b/.forge-snapshots/extsload getFeeGrowthGlobals.snap @@ -1 +1 @@ -777 \ No newline at end of file +774 \ No newline at end of file diff --git a/.forge-snapshots/extsload getPositionInfo.snap b/.forge-snapshots/extsload getPositionInfo.snap index 9f02e1c9e..397787a80 100644 --- a/.forge-snapshots/extsload getPositionInfo.snap +++ b/.forge-snapshots/extsload getPositionInfo.snap @@ -1 +1 @@ -952 \ No newline at end of file +949 \ No newline at end of file diff --git a/.forge-snapshots/extsload getTickFeeGrowthOutside.snap b/.forge-snapshots/extsload getTickFeeGrowthOutside.snap index 5c1d4541a..fc0b67d07 100644 --- a/.forge-snapshots/extsload getTickFeeGrowthOutside.snap +++ b/.forge-snapshots/extsload getTickFeeGrowthOutside.snap @@ -1 +1 @@ -777 \ No newline at end of file +774 \ No newline at end of file diff --git a/.forge-snapshots/extsload getTickInfo.snap b/.forge-snapshots/extsload getTickInfo.snap index 9f02e1c9e..397787a80 100644 --- a/.forge-snapshots/extsload getTickInfo.snap +++ b/.forge-snapshots/extsload getTickInfo.snap @@ -1 +1 @@ -952 \ No newline at end of file +949 \ No newline at end of file diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap index c9e91716c..9599bb1c0 100644 --- a/.forge-snapshots/initialize.snap +++ b/.forge-snapshots/initialize.snap @@ -1 +1 @@ -60025 \ No newline at end of file +60009 \ 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 1c6c78447..a9aafc5e5 100644 --- a/.forge-snapshots/native collect protocol fees.snap +++ b/.forge-snapshots/native collect protocol fees.snap @@ -1 +1 @@ -59726 \ No newline at end of file +59723 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index e085ea34a..e0083410f 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -24073 \ No newline at end of file +24140 \ No newline at end of file diff --git a/.forge-snapshots/removeLiquidity CA fee.snap b/.forge-snapshots/removeLiquidity CA fee.snap index ec2dbf546..c93d8da18 100644 --- a/.forge-snapshots/removeLiquidity CA fee.snap +++ b/.forge-snapshots/removeLiquidity CA fee.snap @@ -1 +1 @@ -141229 \ No newline at end of file +141192 \ 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 12528ddad..5fed9375b 100644 --- a/.forge-snapshots/removeLiquidity with empty hook.snap +++ b/.forge-snapshots/removeLiquidity with empty hook.snap @@ -1 +1 @@ -130597 \ No newline at end of file +130567 \ 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 35f7a9f26..4f897d062 100644 --- a/.forge-snapshots/removeLiquidity with native token.snap +++ b/.forge-snapshots/removeLiquidity with native token.snap @@ -1 +1 @@ -112535 \ No newline at end of file +112513 \ No newline at end of file diff --git a/.forge-snapshots/simple addLiquidity second addition same range.snap b/.forge-snapshots/simple addLiquidity second addition same range.snap index d3a590e01..098d9352b 100644 --- a/.forge-snapshots/simple addLiquidity second addition same range.snap +++ b/.forge-snapshots/simple addLiquidity second addition same range.snap @@ -1 +1 @@ -98850 \ No newline at end of file +98826 \ No newline at end of file diff --git a/.forge-snapshots/simple addLiquidity.snap b/.forge-snapshots/simple addLiquidity.snap index 216625993..3f91cc8f1 100644 --- a/.forge-snapshots/simple addLiquidity.snap +++ b/.forge-snapshots/simple addLiquidity.snap @@ -1 +1 @@ -161395 \ No newline at end of file +161371 \ No newline at end of file diff --git a/.forge-snapshots/simple removeLiquidity some liquidity remains.snap b/.forge-snapshots/simple removeLiquidity some liquidity remains.snap index c2a4006d2..e68338618 100644 --- a/.forge-snapshots/simple removeLiquidity some liquidity remains.snap +++ b/.forge-snapshots/simple removeLiquidity some liquidity remains.snap @@ -1 +1 @@ -92983 \ No newline at end of file +92962 \ No newline at end of file diff --git a/.forge-snapshots/simple removeLiquidity.snap b/.forge-snapshots/simple removeLiquidity.snap index 81dfb1045..38214f3d2 100644 --- a/.forge-snapshots/simple removeLiquidity.snap +++ b/.forge-snapshots/simple removeLiquidity.snap @@ -1 +1 @@ -85096 \ No newline at end of file +85075 \ 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 4bf8e4447..dd35d28f8 100644 --- a/.forge-snapshots/simple swap with native.snap +++ b/.forge-snapshots/simple swap with native.snap @@ -1 +1 @@ -108515 \ No newline at end of file +108422 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index 71f991199..1e8829cfc 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -123335 \ No newline at end of file +123239 \ No newline at end of file diff --git a/.forge-snapshots/swap CA custom curve + swap noop.snap b/.forge-snapshots/swap CA custom curve + swap noop.snap index 2bae0f8b5..4ee133a95 100644 --- a/.forge-snapshots/swap CA custom curve + swap noop.snap +++ b/.forge-snapshots/swap CA custom curve + swap noop.snap @@ -1 +1 @@ -124663 \ No newline at end of file +124633 \ No newline at end of file diff --git a/.forge-snapshots/swap CA fee on unspecified.snap b/.forge-snapshots/swap CA fee on unspecified.snap index 0e69c9078..5a90659c9 100644 --- a/.forge-snapshots/swap CA fee on unspecified.snap +++ b/.forge-snapshots/swap CA fee on unspecified.snap @@ -1 +1 @@ -154783 \ No newline at end of file +154684 \ 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 24cd0f5c3..c8ce4751f 100644 --- a/.forge-snapshots/swap against liquidity with native token.snap +++ b/.forge-snapshots/swap against liquidity with native token.snap @@ -1 +1 @@ -105637 \ No newline at end of file +105557 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index 5cf1c8096..afeb30892 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -116705 \ No newline at end of file +116622 \ 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 9f63f70cf..e42f2a6db 100644 --- a/.forge-snapshots/swap burn 6909 for input.snap +++ b/.forge-snapshots/swap burn 6909 for input.snap @@ -1 +1 @@ -129269 \ No newline at end of file +129261 \ 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 4b0be0884..4e4ef13df 100644 --- a/.forge-snapshots/swap burn native 6909 for input.snap +++ b/.forge-snapshots/swap burn native 6909 for input.snap @@ -1 +1 @@ -118725 \ No newline at end of file +118660 \ 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 63a0a75cf..a3d026bae 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139747 \ No newline at end of file +139727 \ 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 fb2c12b45..7b0c1ad3d 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -155176 \ No newline at end of file +155080 \ 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 9ffba8461..ef1abd51b 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 @@ -206425 \ No newline at end of file +206285 \ 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 7e8319d12..2f7c85061 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -139356 \ No newline at end of file +139284 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index a7fd1c54a..9fb8d971d 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -132343 \ No newline at end of file +132260 \ No newline at end of file diff --git a/.forge-snapshots/swap with return dynamic fee.snap b/.forge-snapshots/swap with return dynamic fee.snap index 8e88648ff..59f45046e 100644 --- a/.forge-snapshots/swap with return dynamic fee.snap +++ b/.forge-snapshots/swap with return dynamic fee.snap @@ -1 +1 @@ -145683 \ No newline at end of file +145611 \ 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 0422eb27f..bb377db1b 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -147966 \ No newline at end of file +147891 \ No newline at end of file diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index 0f64f5e00..e843e2115 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -384,13 +384,20 @@ library Pool { // if the protocol fee is on, calculate how much is owed, decrement feeAmount, and increment protocolFee if (protocolFee > 0) { unchecked { - // step.amountIn does not include the swap fee, as it's already been taken from it, - // so add it back to get the total amountIn and use that to calculate the amount of fees owed to the protocol - // this line cannot overflow due to limits on the size of protocolFee and params.amountSpecified - uint256 delta = (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; - // subtract it from the total fee and add it to the protocol fee - step.feeAmount -= delta; - amountToProtocol += delta; + // if the protocol fee is the same as the swap fee, the entire fee is owed to the protocol + if (swapFee == protocolFee) { + amountToProtocol += step.feeAmount; + step.feeAmount = 0; + } else { + // step.amountIn does not include the swap fee, as it's already been taken from it, + // so add it back to get the total amountIn and use that to calculate the amount of fees owed to the protocol + // this line cannot overflow due to limits on the size of protocolFee and params.amountSpecified + // this rounds down to favor LPs over the protocol + uint256 delta = (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; + // subtract it from the total fee and add it to the protocol fee + step.feeAmount -= delta; + amountToProtocol += delta; + } } } diff --git a/src/libraries/ProtocolFeeLibrary.sol b/src/libraries/ProtocolFeeLibrary.sol index 48adf0fd3..512a49928 100644 --- a/src/libraries/ProtocolFeeLibrary.sol +++ b/src/libraries/ProtocolFeeLibrary.sol @@ -33,16 +33,15 @@ library ProtocolFeeLibrary { // The protocol fee is taken from the input amount first and then the LP fee is taken from the remaining // The swap fee is capped at 100% - // Equivalent to protocolFee + lpFee(1_000_000 - protocolFee) / 1_000_000 + // Equivalent to protocolFee + lpFee(1_000_000 - protocolFee) / 1_000_000 (rounded up) /// @dev here `self` is just a single direction's protocol fee, not a packed type of 2 protocol fees function calculateSwapFee(uint16 self, uint24 lpFee) internal pure returns (uint24 swapFee) { - // protocolFee + lpFee - (protocolFee * lpFee / 1_000_000). Div rounds up to favor LPs over the protocol. + // protocolFee + lpFee - (protocolFee * lpFee / 1_000_000) assembly ("memory-safe") { self := and(self, 0xfff) lpFee := and(lpFee, 0xffffff) let numerator := mul(self, lpFee) - let divRoundingUp := add(div(numerator, PIPS_DENOMINATOR), gt(mod(numerator, PIPS_DENOMINATOR), 0)) - swapFee := sub(add(self, lpFee), divRoundingUp) + swapFee := sub(add(self, lpFee), div(numerator, PIPS_DENOMINATOR)) } } } diff --git a/test/DynamicFees.t.sol b/test/DynamicFees.t.sol index 367a3adbb..1753769dd 100644 --- a/test/DynamicFees.t.sol +++ b/test/DynamicFees.t.sol @@ -20,9 +20,13 @@ import {MockERC20} from "solmate/src/test/utils/mocks/MockERC20.sol"; import {Pool} from "../src/libraries/Pool.sol"; import {BalanceDelta, BalanceDeltaLibrary} from "../src/types/BalanceDelta.sol"; import {StateLibrary} from "../src/libraries/StateLibrary.sol"; +import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; + +import "forge-std/console2.sol"; contract TestDynamicFees is Test, Deployers, GasSnapshot { using StateLibrary for IPoolManager; + using ProtocolFeeLibrary for uint16; DynamicFeesTestHook dynamicFeesHooks = DynamicFeesTestHook( address( @@ -233,16 +237,9 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot { PoolSwapTest.TestSettings memory testSettings = PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false}); - vm.expectEmit(true, true, true, true, address(manager)); - emit Swap(key.toId(), address(swapRouter), -101000000, 100, 79228162514264329670727698909, 1e18, -1, 999999); - - BalanceDelta delta = swapRouter.swap(key, params, testSettings, ZERO_BYTES); - snapLastCall("swap with lp fee and protocol fee"); - - uint256 expectedProtocolFee = uint256(uint128(-delta.amount0())) * 1000 / 1e6; - assertEq(manager.protocolFeesAccrued(currency0), expectedProtocolFee); + vm.expectRevert(Pool.InvalidFeeForExactOut.selector); + swapRouter.swap(key, params, testSettings, ZERO_BYTES); - assertEq(_fetchPoolLPFee(key), 999999); } function test_swap_100PercentFee_AmountIn_WithProtocol() public { @@ -279,7 +276,7 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot { PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false}); vm.expectEmit(true, true, true, true, address(manager)); - emit Swap(key.toId(), address(swapRouter), -100, 98, 79228162514264329749955861424, 1e18, -1, 1122); + emit Swap(key.toId(), address(swapRouter), -100, 98, 79228162514264329749955861424, 1e18, -1, 1123); swapRouter.swap(key, SWAP_PARAMS, testSettings, ZERO_BYTES); @@ -312,7 +309,21 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot { BalanceDelta delta = swapRouter.swap(key, params, testSettings, ZERO_BYTES); - uint256 expectedProtocolFee = uint256(uint128(-delta.amount0())) * protocolFee0 / 1e6; + uint24 swapFee = uint16(protocolFee).calculateSwapFee(lpFee); + + uint256 expectedProtocolFee; + int128 delta0 = -delta.amount0(); + + if (lpFee == 0) { + assertEq(protocolFee0, swapFee); + assembly { + // round up instead of down + expectedProtocolFee := add(div(mul(delta0, protocolFee0), 1000000), gt(mod(mul(delta0, protocolFee0), 1000000), 0)) + } + } else { + expectedProtocolFee = uint256(uint128(delta0)) * protocolFee0 / 1e6; + } + assertEq(manager.protocolFeesAccrued(currency0), expectedProtocolFee); } diff --git a/test/libraries/ProtocolFeeLibrary.t.sol b/test/libraries/ProtocolFeeLibrary.t.sol index ba341a4d6..f179ff192 100644 --- a/test/libraries/ProtocolFeeLibrary.t.sol +++ b/test/libraries/ProtocolFeeLibrary.t.sol @@ -71,18 +71,20 @@ contract ProtocolFeeLibraryTest is Test, GasSnapshot { protocolFee = uint16(bound(protocolFee, 0, ProtocolFeeLibrary.MAX_PROTOCOL_FEE)); lpFee = uint24(bound(lpFee, 0, LPFeeLibrary.MAX_LP_FEE)); uint24 swapFee = ProtocolFeeLibrary.calculateSwapFee(protocolFee, lpFee); - // if lp fee is not the max, the swap fee should never be the max since the protocol fee is taken off first and then the lp fee is taken from the remaining amount if (lpFee < LPFeeLibrary.MAX_LP_FEE) { - assertLt(swapFee, LPFeeLibrary.MAX_LP_FEE); + assertLe(swapFee, LPFeeLibrary.MAX_LP_FEE); } else { - // otherwise it is equal to max, and can therefore never be larger + // if lp fee is equal to max, swap fee can never be larger assertEq(swapFee, LPFeeLibrary.MAX_LP_FEE); } - assertGe(swapFee, lpFee); + uint256 expectedSwapFee; + // protocolFee + lpFee(1_000_000 - protocolFee) / 1_000_000 (rounded up) + assembly ("memory-safe") { + expectedSwapFee := add(protocolFee, add(div(mul(sub(1000000, protocolFee), lpFee), 1000000), gt(mod(mul(sub(1000000, protocolFee), lpFee), 1000000), 0))) + } - uint256 expectedSwapFee = - protocolFee + lpFee * uint256(LPFeeLibrary.MAX_LP_FEE - protocolFee) / LPFeeLibrary.MAX_LP_FEE; + assertGe(swapFee, lpFee); assertEq(swapFee, uint24(expectedSwapFee)); } } From 7fb3a2af47801d2e14e7f49a68bc5d224b62b3a5 Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Wed, 16 Oct 2024 13:30:07 -0400 Subject: [PATCH 2/6] format --- src/libraries/Pool.sol | 3 ++- test/DynamicFees.t.sol | 4 ++-- test/libraries/ProtocolFeeLibrary.t.sol | 9 ++++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index fa871d2aa..888074b32 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -390,7 +390,8 @@ library Pool { // so add it back to get the total amountIn and use that to calculate the amount of fees owed to the protocol // this line cannot overflow due to limits on the size of protocolFee and params.amountSpecified // this rounds down to favor LPs over the protocol - uint256 delta = (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; + uint256 delta = + (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; // subtract it from the total fee and add it to the protocol fee step.feeAmount -= delta; amountToProtocol += delta; diff --git a/test/DynamicFees.t.sol b/test/DynamicFees.t.sol index d16a3ec0e..3ece12e5b 100644 --- a/test/DynamicFees.t.sol +++ b/test/DynamicFees.t.sol @@ -234,7 +234,6 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot { vm.expectRevert(Pool.InvalidFeeForExactOut.selector); swapRouter.swap(key, params, testSettings, ZERO_BYTES); - } function test_swap_100PercentFee_AmountIn_WithProtocol() public { @@ -313,7 +312,8 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot { assertEq(protocolFee0, swapFee); assembly { // round up instead of down - expectedProtocolFee := add(div(mul(delta0, protocolFee0), 1000000), gt(mod(mul(delta0, protocolFee0), 1000000), 0)) + expectedProtocolFee := + add(div(mul(delta0, protocolFee0), 1000000), gt(mod(mul(delta0, protocolFee0), 1000000), 0)) } } else { expectedProtocolFee = uint256(uint128(delta0)) * protocolFee0 / 1e6; diff --git a/test/libraries/ProtocolFeeLibrary.t.sol b/test/libraries/ProtocolFeeLibrary.t.sol index f179ff192..9bd4a79c3 100644 --- a/test/libraries/ProtocolFeeLibrary.t.sol +++ b/test/libraries/ProtocolFeeLibrary.t.sol @@ -81,7 +81,14 @@ contract ProtocolFeeLibraryTest is Test, GasSnapshot { uint256 expectedSwapFee; // protocolFee + lpFee(1_000_000 - protocolFee) / 1_000_000 (rounded up) assembly ("memory-safe") { - expectedSwapFee := add(protocolFee, add(div(mul(sub(1000000, protocolFee), lpFee), 1000000), gt(mod(mul(sub(1000000, protocolFee), lpFee), 1000000), 0))) + expectedSwapFee := + add( + protocolFee, + add( + div(mul(sub(1000000, protocolFee), lpFee), 1000000), + gt(mod(mul(sub(1000000, protocolFee), lpFee), 1000000), 0) + ) + ) } assertGe(swapFee, lpFee); From 30ec0abe429903807bcf8b54368e1ea243810b1e Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Wed, 16 Oct 2024 13:38:42 -0400 Subject: [PATCH 3/6] re-organize --- .../poolManager bytecode size.snap | 2 +- src/libraries/Pool.sol | 19 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 27c79889b..f06e8f4ab 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23688 \ No newline at end of file +23682 \ No newline at end of file diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index 888074b32..6b2b7bfb1 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -381,21 +381,20 @@ library Pool { // if the protocol fee is on, calculate how much is owed, decrement feeAmount, and increment protocolFee if (protocolFee > 0) { unchecked { - // if the protocol fee is the same as the swap fee, the entire fee is owed to the protocol - if (swapFee == protocolFee) { - amountToProtocol += step.feeAmount; - step.feeAmount = 0; - } else { + uint256 delta; + if (swapFee != protocolFee) { // step.amountIn does not include the swap fee, as it's already been taken from it, // so add it back to get the total amountIn and use that to calculate the amount of fees owed to the protocol // this line cannot overflow due to limits on the size of protocolFee and params.amountSpecified // this rounds down to favor LPs over the protocol - uint256 delta = - (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; - // subtract it from the total fee and add it to the protocol fee - step.feeAmount -= delta; - amountToProtocol += delta; + delta = (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; + } else { + // if the protocol fee is the same as the swap fee, the entire fee is owed to the protocol + delta = step.feeAmount; } + // subtract it from the total fee and add it to the protocol fee + step.feeAmount -= delta; + amountToProtocol += delta; } } From 0637abb5f9b12ceb597e5829e530f38bfc6e9921 Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Wed, 16 Oct 2024 13:40:54 -0400 Subject: [PATCH 4/6] remove console --- test/DynamicFees.t.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/DynamicFees.t.sol b/test/DynamicFees.t.sol index 3ece12e5b..a60a0603b 100644 --- a/test/DynamicFees.t.sol +++ b/test/DynamicFees.t.sol @@ -22,8 +22,6 @@ import {BalanceDelta, BalanceDeltaLibrary} from "../src/types/BalanceDelta.sol"; import {StateLibrary} from "../src/libraries/StateLibrary.sol"; import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; -import "forge-std/console2.sol"; - contract TestDynamicFees is Test, Deployers, GasSnapshot { using StateLibrary for IPoolManager; using ProtocolFeeLibrary for uint16; From 4d728552359606f6bd617c220b6d156f74bb44b0 Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Fri, 18 Oct 2024 09:50:17 -0400 Subject: [PATCH 5/6] make easier to read --- src/libraries/Pool.sol | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index 6b2b7bfb1..f4ed08cb5 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -381,17 +381,13 @@ library Pool { // if the protocol fee is on, calculate how much is owed, decrement feeAmount, and increment protocolFee if (protocolFee > 0) { unchecked { - uint256 delta; - if (swapFee != protocolFee) { - // step.amountIn does not include the swap fee, as it's already been taken from it, - // so add it back to get the total amountIn and use that to calculate the amount of fees owed to the protocol - // this line cannot overflow due to limits on the size of protocolFee and params.amountSpecified - // this rounds down to favor LPs over the protocol - delta = (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; - } else { - // if the protocol fee is the same as the swap fee, the entire fee is owed to the protocol - delta = step.feeAmount; - } + // step.amountIn does not include the swap fee, as it's already been taken from it, + // so add it back to get the total amountIn and use that to calculate the amount of fees owed to the protocol + // cannot overflow due to limits on the size of protocolFee and params.amountSpecified + // this rounds down to favor LPs over the protocol + uint256 delta = (swapFee == protocolFee) + ? step.feeAmount // if there is no lp fee, the entire fee is owed to the protocol instead + : (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; // subtract it from the total fee and add it to the protocol fee step.feeAmount -= delta; amountToProtocol += delta; From 8a3c29e43ce4fbc5b6ebf52f9328dd06beef3677 Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Fri, 18 Oct 2024 17:49:07 -0400 Subject: [PATCH 6/6] test changes --- src/libraries/Pool.sol | 2 +- test/DynamicFees.t.sol | 12 ++---------- test/libraries/ProtocolFeeLibrary.t.sol | 13 ++----------- 3 files changed, 5 insertions(+), 22 deletions(-) diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol index f4ed08cb5..fc714f6d2 100644 --- a/src/libraries/Pool.sol +++ b/src/libraries/Pool.sol @@ -386,7 +386,7 @@ library Pool { // cannot overflow due to limits on the size of protocolFee and params.amountSpecified // this rounds down to favor LPs over the protocol uint256 delta = (swapFee == protocolFee) - ? step.feeAmount // if there is no lp fee, the entire fee is owed to the protocol instead + ? step.feeAmount // lp fee is 0, so the entire fee is owed to the protocol instead : (step.amountIn + step.feeAmount) * protocolFee / ProtocolFeeLibrary.PIPS_DENOMINATOR; // subtract it from the total fee and add it to the protocol fee step.feeAmount -= delta; diff --git a/test/DynamicFees.t.sol b/test/DynamicFees.t.sol index a60a0603b..5cab241b3 100644 --- a/test/DynamicFees.t.sol +++ b/test/DynamicFees.t.sol @@ -303,18 +303,10 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot { uint24 swapFee = uint16(protocolFee).calculateSwapFee(lpFee); - uint256 expectedProtocolFee; - int128 delta0 = -delta.amount0(); - + uint256 expectedProtocolFee = uint256(uint128(-delta.amount0())) * protocolFee0 / 1e6; if (lpFee == 0) { assertEq(protocolFee0, swapFee); - assembly { - // round up instead of down - expectedProtocolFee := - add(div(mul(delta0, protocolFee0), 1000000), gt(mod(mul(delta0, protocolFee0), 1000000), 0)) - } - } else { - expectedProtocolFee = uint256(uint128(delta0)) * protocolFee0 / 1e6; + if (((uint256(uint128(-delta.amount0())) * protocolFee0) % 1e6) != 0) expectedProtocolFee++; } assertEq(manager.protocolFeesAccrued(currency0), expectedProtocolFee); diff --git a/test/libraries/ProtocolFeeLibrary.t.sol b/test/libraries/ProtocolFeeLibrary.t.sol index 9bd4a79c3..043ffd214 100644 --- a/test/libraries/ProtocolFeeLibrary.t.sol +++ b/test/libraries/ProtocolFeeLibrary.t.sol @@ -78,18 +78,9 @@ contract ProtocolFeeLibraryTest is Test, GasSnapshot { assertEq(swapFee, LPFeeLibrary.MAX_LP_FEE); } - uint256 expectedSwapFee; // protocolFee + lpFee(1_000_000 - protocolFee) / 1_000_000 (rounded up) - assembly ("memory-safe") { - expectedSwapFee := - add( - protocolFee, - add( - div(mul(sub(1000000, protocolFee), lpFee), 1000000), - gt(mod(mul(sub(1000000, protocolFee), lpFee), 1000000), 0) - ) - ) - } + uint256 expectedSwapFee = protocolFee + (1e6 - protocolFee) * uint256(lpFee) / 1e6; + if (((1e6 - protocolFee) * uint256(lpFee)) % 1e6 != 0) expectedSwapFee++; assertGe(swapFee, lpFee); assertEq(swapFee, uint24(expectedSwapFee));