round swap fee up and check if protocol fee = swap fee (#905)

* round swap fee up and check if protocol fee = swap fee * format * re-organize * remove console * make easier to read * test changes
Uniswap · Oct 18, 2024 · 3843734 · 3843734
1 parent 3afa83a
commit 3843734
Show file tree

Hide file tree

Showing 42 changed files with 63 additions and 61 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
@@ -1 +1 @@
-144663
+144639
diff --git a/.forge-snapshots/addLiquidity CA fee.snap b/.forge-snapshots/addLiquidity CA fee.snap
@@ -1 +1 @@
-170959
+170928
diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap
@@ -1 +1 @@
-274264
+274240
diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap
@@ -1 +1 @@
-135141
+135120
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
@@ -1 +1 @@
-292855
+292831
diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap
@@ -1 +1 @@
-106345
+106333
diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap
@@ -1 +1 @@
-145766
+145748
diff --git a/.forge-snapshots/erc20 collect protocol fees.snap b/.forge-snapshots/erc20 collect protocol fees.snap
@@ -1 +1 @@
-57454
+57451
diff --git a/.forge-snapshots/extsload getFeeGrowthGlobals.snap b/.forge-snapshots/extsload getFeeGrowthGlobals.snap
@@ -1 +1 @@
-777
+774
diff --git a/.forge-snapshots/extsload getPositionInfo.snap b/.forge-snapshots/extsload getPositionInfo.snap
@@ -1 +1 @@
-952
+949
diff --git a/.forge-snapshots/extsload getTickFeeGrowthOutside.snap b/.forge-snapshots/extsload getTickFeeGrowthOutside.snap
@@ -1 +1 @@
-777
+774
diff --git a/.forge-snapshots/extsload getTickInfo.snap b/.forge-snapshots/extsload getTickInfo.snap
@@ -1 +1 @@
-952
+949
diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap
@@ -1 +1 @@
-51536
+51533
diff --git a/.forge-snapshots/native collect protocol fees.snap b/.forge-snapshots/native collect protocol fees.snap
@@ -1 +1 @@
-59726
+59723
diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap
@@ -1 +1 @@
-23621
+23682
diff --git a/.forge-snapshots/removeLiquidity CA fee.snap b/.forge-snapshots/removeLiquidity CA fee.snap
@@ -1 +1 @@
-141219
+141194
diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap
@@ -1 +1 @@
-130621
+130603
diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap
@@ -1 +1 @@
-112535
+112523
diff --git a/.forge-snapshots/simple addLiquidity second addition same range.snap b/.forge-snapshots/simple addLiquidity second addition same range.snap
@@ -1 +1 @@
-98862
+98850
diff --git a/.forge-snapshots/simple addLiquidity.snap b/.forge-snapshots/simple addLiquidity.snap
@@ -1 +1 @@
-161407
+161395
diff --git a/.forge-snapshots/simple removeLiquidity some liquidity remains.snap b/.forge-snapshots/simple removeLiquidity some liquidity remains.snap
@@ -1 +1 @@
-92995
+92986
diff --git a/.forge-snapshots/simple removeLiquidity.snap b/.forge-snapshots/simple removeLiquidity.snap
@@ -1 +1 @@
-85108
+85099
diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap
@@ -1 +1 @@
-108515
+108434
diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap
@@ -1 +1 @@
-123347
+123263
diff --git a/.forge-snapshots/swap CA custom curve + swap noop.snap b/.forge-snapshots/swap CA custom curve + swap noop.snap
@@ -1 +1 @@
-124653
+124635
diff --git a/.forge-snapshots/swap CA fee on unspecified.snap b/.forge-snapshots/swap CA fee on unspecified.snap
@@ -1 +1 @@
-154773
+154686
diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap
@@ -1 +1 @@
-105637
+105569
diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap
@@ -1 +1 @@
-116717
+116646
diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap
@@ -1 +1 @@
-129281
+129285
diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap
@@ -1 +1 @@
-118725
+118672
diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap
@@ -1 +1 @@
-139747
+139739
diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap
@@ -1 +1 @@
-155188
+155104
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
@@ -1 +1 @@
-206403
+206251
diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap
@@ -1 +1 @@
-139368
+139284
diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap
@@ -1 +1 @@
-132345
+132274
diff --git a/.forge-snapshots/swap with lp fee and protocol fee.snap b/.forge-snapshots/swap with lp fee and protocol fee.snap
diff --git a/.forge-snapshots/swap with return dynamic fee.snap b/.forge-snapshots/swap with return dynamic fee.snap
@@ -1 +1 @@
-145661
+145577
diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap
@@ -1 +1 @@
-147956
+147869
diff --git a/src/libraries/Pool.sol b/src/libraries/Pool.sol
@@ -383,8 +383,11 @@ library Pool {
                 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;
+                    // 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 // 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;
                     amountToProtocol += delta;

diff --git 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
@@ -20,9 +20,11 @@ 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";
 
 contract TestDynamicFees is Test, Deployers, GasSnapshot {
     using StateLibrary for IPoolManager;
+    using ProtocolFeeLibrary for uint16;
 
     DynamicFeesTestHook dynamicFeesHooks = DynamicFeesTestHook(
         address(
@@ -228,16 +230,8 @@ 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);
-
-        assertEq(_fetchPoolLPFee(key), 999999);
+        vm.expectRevert(Pool.InvalidFeeForExactOut.selector);
+        swapRouter.swap(key, params, testSettings, ZERO_BYTES);
     }
 
     function test_swap_100PercentFee_AmountIn_WithProtocol() public {
@@ -274,7 +268,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);
 
@@ -307,7 +301,14 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
 
         BalanceDelta delta = swapRouter.swap(key, params, testSettings, ZERO_BYTES);
 
+        uint24 swapFee = uint16(protocolFee).calculateSwapFee(lpFee);
+
         uint256 expectedProtocolFee = uint256(uint128(-delta.amount0())) * protocolFee0 / 1e6;
+        if (lpFee == 0) {
+            assertEq(protocolFee0, swapFee);
+            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
@@ -71,18 +71,18 @@ 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);
+        // protocolFee + lpFee(1_000_000 - protocolFee) / 1_000_000 (rounded up)
+        uint256 expectedSwapFee = protocolFee + (1e6 - protocolFee) * uint256(lpFee) / 1e6;
+        if (((1e6 - protocolFee) * uint256(lpFee)) % 1e6 != 0) expectedSwapFee++;
 
-        uint256 expectedSwapFee =
-            protocolFee + lpFee * uint256(LPFeeLibrary.MAX_LP_FEE - protocolFee) / LPFeeLibrary.MAX_LP_FEE;
+        assertGe(swapFee, lpFee);
         assertEq(swapFee, uint24(expectedSwapFee));
     }
 }