Uniswap · dianakocsis · Oct 18, 2024 · Oct 16, 2024 · Oct 16, 2024 · Oct 16, 2024
@@ -1 +1 @@
-144663
+144639
@@ -1 +1 @@
-170959
+170928
@@ -1 +1 @@
-274264
+274240
@@ -1 +1 @@
-135141
+135120
@@ -1 +1 @@
-292855
+292831
@@ -1 +1 @@
-106345
+106333
@@ -1 +1 @@
-145766
+145748
@@ -1 +1 @@
-57454
+57451
@@ -1 +1 @@
-777
+774
@@ -1 +1 @@
-952
+949
@@ -1 +1 @@
-777
+774
@@ -1 +1 @@
-952
+949
@@ -1 +1 @@
-51536
+51533
@@ -1 +1 @@
-59726
+59723
@@ -1 +1 @@
-23621
+23682
@@ -1 +1 @@
-141219
+141194
@@ -1 +1 @@
-130621
+130603
@@ -1 +1 @@
-112535
+112523
@@ -1 +1 @@
-98862
+98850
@@ -1 +1 @@
-161407
+161395
@@ -1 +1 @@
-92995
+92986
@@ -1 +1 @@
-85108
+85099
@@ -1 +1 @@
-108515
+108434
@@ -1 +1 @@
-123347
+123263
@@ -1 +1 @@
-124653
+124635
@@ -1 +1 @@
-154773
+154686
@@ -1 +1 @@
-105637
+105569
@@ -1 +1 @@
-116717
+116646
@@ -1 +1 @@
-129281
+129285
@@ -1 +1 @@
-118725
+118672
@@ -1 +1 @@
-139747
+139739
@@ -1 +1 @@
-155188
+155104
@@ -1 +1 @@
-206403
+206251
@@ -1 +1 @@
-139368
+139284
@@ -1 +1 @@
-132345
+132274
@@ -1 +1 @@
-145661
+145577
@@ -1 +1 @@
-147956
+147869
@@ -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;

@@ -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))
         }
     }
 }
@@ -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);
     }
 

@@ -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));
     }
 }