feat: refactor the fee mechanism for both pools #36

chefburger · 2024-05-06T09:23:54Z

This PR tries to update the underlying fee mechanism including lp and protocol fee.

Major changes:

Unified the way the fee percentage is stored, now both lp and protocol fee are denominated in hundredths of a bip
Unified the way how fee is updated, now both (dynamic) lp and protocol fee are updated in a push manner
Updated the charging mechanism i.e. instead of lp fee charging on the input amount first and protocolFee charging on swap fee, now protocolFee is charged on the input amount first and then lp fee is charged on the input amount left (excluding protocol fee)

chefburger · 2024-05-08T02:45:15Z

src/ProtocolFees.sol

@@ -24,11 +27,22 @@ abstract contract Fees is IFees, PausableRole {
        controllerGasLimit = _controllerGasLimit;
    }

+    function _setProtocolFee(PoolId id, uint24 newProtocolFee) internal virtual;


_getPool does not make sense because we may have different pool types

chefburger · 2024-05-08T02:55:07Z

src/pool-bin/BinPoolManager.sol

-        if (swapFee.isSwapFeeTooLarge(SwapFeeLibrary.TEN_PERCENT_FEE)) revert FeeTooLarge();
+        /// @notice init value for dynamic lp fee is 0, but hook can still set it in afterInitialize
+        uint24 lpFee = key.fee.getInitialLPFee();
+        lpFee.validate(LPFeeLibrary.TEN_PERCENT_FEE);


Confirm: are we still keeping 10% for bin pool lp fee ?
It's in fact at max (1 - 0.1%) * 10% = 9.99% charged on input token compare to previous

aren't we setting max fee to 100% just like uni?

"aren't we setting max fee to 100% just like uni?"

Before We were just following the original traderjoe behavior

Confirm: are we still keeping 10% for bin pool lp fee ?

let's create an issue to track this -- so we can answer there and pass the issue link to future devs when they ask about why joe is 10% limit. On a side note, so long this PR does not change the current max 10% fee behavior, then its alright.

It's in fact at max (1 - 0.1%) * 10% = 9.99% charged on input token compare to previous

can you explain a bit more? you mean before this PR the fee is 10% and somehow now is 9.99%? or previously and now is 9.99% ?

Also won't this question be applicable for CLPool as well? in this PR, the max lp fee for CL pool is 99.9%?

"can you explain a bit more? you mean before this PR the fee is 10% and somehow now is 9.99%? or previously and now is 9.99% ?"

sorry not 9.99%, it should be 10.09%

I mean before the PR the fee is 10% because what we set was actually swap fee which includes protocol fee already. But after this PR we are setting lp fee here which does not include protocol fee, although it can still be 10%, but since it's charged after protocol fee on the rest part. The swap fee a bit larger. It's at max (saying 0.1% protocol fee ) 0.1% + (1 - 0.1%) * 10% = 10.09%

Got it, i think thats fine 💪 - we should create an issue to ensure our developer docs state this information down

chefburger · 2024-05-08T02:55:33Z

src/pool-bin/BinPoolManager.sol

-            uint24 swapFee;
-            (delta, feeForProtocol, activeId, swapFee) =
+            BinPool.SwapState memory state;
+            (delta, state) =


stack too deep

chefburger · 2024-05-08T03:05:54Z

src/pool-bin/BinPoolManager.sol

-            emit Swap(id, msg.sender, delta.amount0(), delta.amount1(), activeId, swapFee, feeForProtocol);
+            emit Swap(
+                id, msg.sender, delta.amount0(), delta.amount1(), state.activeId, state.swapFee, state.protocolFee
+            );


The last parameter has been updated from "accumulated protocol fee value" to fee percentage (align with clpool)

chefburger · 2024-05-08T03:09:18Z

src/pool-bin/BinPoolManager.sol


        (amountIn, amountOutLeft, fee) = pools[id].getSwapIn(
-            BinPool.SwapViewParams({swapForY: swapForY, binStep: key.parameters.getBinStep(), fee: totalSwapFee}),
-            amountOut
+            BinPool.SwapViewParams({swapForY: swapForY, binStep: key.parameters.getBinStep(), lpFee: lpFee}), amountOut


For bin-pool, the fee change involves 4 functions:

getSwapIn

getSwapOut

Swap

Mint

chefburger · 2024-05-08T03:12:07Z

src/pool-bin/BinPoolManager.sol


        (amountInLeft, amountOut, fee) = pools[id].getSwapOut(
-            BinPool.SwapViewParams({swapForY: swapForY, binStep: key.parameters.getBinStep(), fee: totalSwapFee}),
-            amountIn
+            BinPool.SwapViewParams({swapForY: swapForY, binStep: key.parameters.getBinStep(), lpFee: lpFee}), amountIn


we pass in lp fee here and calculate swap fee based on lp fee and protocol fee

chefburger · 2024-05-08T03:19:03Z

src/pool-bin/libraries/BinHelper.sol

            fees = feeY.encodeSecond();
+            feeForProtocol = (amountY - receivedAmountY).getCompositionFee(protocolFee).encodeSecond();


Just a heads up:

The getCompositionFee here applies the following formula

amountWithFees * feeBips * (1+feeBips)

Not quite sure the reason, to make sure the calculation rule the same i have used the same formula to calculate feeForProtocol here

chefburger · 2024-05-08T03:21:39Z

src/pool-bin/libraries/BinHelper.sol

    /// @param amountsIn The amounts of tokens to add
    /// @param totalSupply The total supply of the liquidity book
    /// @param shares The share of the liquidity book that the user will receive
    /// @return fees The encoded fees that will be charged
    function getCompositionFees(
        bytes32 binReserves,
-        uint24 fee, // fee: 100 = 0.01%
+        uint24 protocolFee, // fee: 100 = 0.01%
+        uint24 lpFee,


Reason why i pass-in both fee and calculate swap fee inside:

Because protocolFee includes two direction i.e. 0to1 and 1to0. we'll need more gas to figure out which way it's going if we do it outside

chefburger · 2024-05-08T03:25:49Z

src/pool-bin/libraries/BinPool.sol


                if (amountsInWithFees > 0) {
                    amountsLeft = amountsLeft.sub(amountsInWithFees);
                    amountsOut = amountsOut.add(amountsOutOfBin);

-                    bytes32 pFee = totalFees.getExternalFeeAmt(self.slot0.protocolFee);
+                    /// @dev calc protocol fee for current bin, charged on amoutIn and charged before lpFee
+                    bytes32 pFee = amountsInWithFees.getExternalFeeAmt(slot0Cache.protocolFee);


instead of
totalFees * protocolFeePercent : this implies protocol fee is part of swap fee

now we have

amountsInWithFees * protocolFeePercent: this implies protocol fee is charged directly on input amount

chefburger · 2024-05-08T03:30:47Z

src/pool-bin/libraries/math/PackedUint128Math.sol

-        uint128 feeForY = fee1 == 0 ? 0 : amountY / fee1;
+        uint128 feeForX;
+        uint128 feeForY;
+        unchecked {


double check:

is it safe to use unchecked here ? I didn't see any overflow / underflow case

looks good to me too.

maybe can add a comment here for us to double check later since we added this unchecked optimization here eg.

// todo: double check on this unchecked condition

LGTM as well

chefburger · 2024-05-08T04:35:04Z

src/pool-bin/libraries/BinPool.sol


                if (amountsInWithFees > 0) {
                    amountsLeft = amountsLeft.sub(amountsInWithFees);
                    amountsOut = amountsOut.add(amountsOutOfBin);

-                    bytes32 pFee = totalFees.getExternalFeeAmt(self.slot0.protocolFee);
+                    /// @dev calc protocol fee for current bin, totalFee * protocolFee / (protocolFee + lpFee)
+                    bytes32 pFee = totalFee.getExternalFeeAmt(slot0Cache.protocolFee, swapState.swapFee);


Formula here: totalFee * protocolFee / (protocolFee + lpFee)

chefburger · 2024-05-08T04:42:02Z

src/pool-cl/libraries/CLPool.sol

+
+        /// @dev If amountSpecified is the output, also given amountSpecified cant be 0,
+        /// then the tx will always revert if the swap fee is 100%
+        if (!exactInput && (state.swapFee == LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE)) {


This won't happen to bin pool because max lp fee for bin-pool is 10%

chefburger · 2024-05-08T04:45:32Z

src/pool-cl/libraries/CLPool.sol

-            protocolFee: 0,
-            liquidity: cache.liquidityStart
-        });
+        {


For cl-pool, the fee change only involves function swap()

chef-omelette · 2024-05-08T07:22:01Z

This PR tries to update the underlying fee mechanism including lp and protocol fee.

Major changes:

Unified the way the fee percentage is stored, now both lp and protocol fee are denominated in hundredths of a bip

Unified the way how fee is updated, now both (dynamic) lp and protocol fee are updated in a push manner

Updated the charging mechanism i.e. instead of lp fee charging on the input amount first and protocolFee charging on swap fee, now protocolFee is charged on the input amount first and then lp fee is charged on the input amount left (excluding protocol fee)

why 3.? I mean taking swap fee first and then taking a part of it as protocol fee makes more sense to me
imagine the fee for a pool is 0.03% at first, but then we charge protocol fee so now the fee becomes 0.04%

ChefMist · 2024-05-08T13:59:07Z

src/pool-bin/libraries/BinHelper.sol

@@ -148,31 +150,41 @@ library BinHelper {
    /// @dev Returns the composition fees when adding liquidity to the active bin with a different
    ///      composition factor than the bin's one, as it does an implicit swap
    /// @param binReserves The reserves of the bin
-    /// @param fee 100 = 0.01%, 1000 = 0.1%
+    /// @param protocolFee 100 = 0.01%, 1000 = 0.1%
+    /// @param lpFee 100 = 0.01%, 1000 = 0.1%
    /// @param amountsIn The amounts of tokens to add
    /// @param totalSupply The total supply of the liquidity book
    /// @param shares The share of the liquidity book that the user will receive
    /// @return fees The encoded fees that will be charged


can add doc on

//// @return feeForProtocol ...

ChefMist · 2024-05-08T14:33:15Z

src/pool-bin/libraries/BinHelper.sol

        if (receivedAmountX > amountX) {
-            uint128 feeY = (amountY - receivedAmountY).getCompositionFee(fee);
+            protocolFee = protocolFee.getOneForZeroFee();
+
+            uint24 swapFee = uint24(protocolFee).calculateSwapFee(lpFee);
+            uint128 feeY = (amountY - receivedAmountY).getCompositionFee(swapFee);
            fees = feeY.encodeSecond();
+            feeForProtocol = (amountY - receivedAmountY).getCompositionFee(protocolFee).encodeSecond();


(optional) not sure if this save gas.. though this might be slightly more readable

// if received more X than given X, then swap some Y for X if (receivedAmountX > amountX) { protocolFee = protocolFee.getOneForZeroFee(); uint24 swapFee = uint24(protocolFee).calculateSwapFee(lpFee); uint128 amtSwapped = amountY - receivedAmountY; fees = amtSwapped.getCompositionFee(swapFee).encodeSecond(); feeForProtocol = amtSwapped.getCompositionFee(protocolFee).encodeSecond(); } else if (receivedAmountY > amountY) { protocolFee = protocolFee.getZeroForOneFee(); uint24 swapFee = uint24(protocolFee).calculateSwapFee(lpFee); uint128 amtSwapped = amountX - receivedAmountX; fees = amtSwapped.getCompositionFee(swapFee).encodeFirst(); feeForProtocol = amtSwapped.getCompositionFee(protocolFee).encodeFirst(); }

ChefMist · 2024-05-09T02:26:04Z

src/pool-bin/libraries/BinPool.sol

    {
-        if (amountIn == 0) revert BinPool__InsufficientAmountIn();


can consider moving this check up if (amountIn == 0) revert BinPool__InsufficientAmountIn(); so in revert case it will consume less gas

ChefMist · 2024-05-09T02:32:17Z

src/pool-bin/libraries/BinPool.sol

        bool swapForY = params.swapForY;
+        swapState.protocolFee =


(gas savings)

instead of storing protocolFee in swapState, simply

uint24 protocolFee = swapForY ? slot0Cache.protocolFee.getOneForZeroFee() : slot0Cache.protocolFee.getZeroForOneFee(); ``

It seems we still need to put it in SwapState otherwise we will need another return value (Swap event needs protocolFee) which results stack too deep error 😂

ChefMist · 2024-05-09T03:12:59Z

src/pool-cl/libraries/CLPool.sol

-            liquidity: cache.liquidityStart
-        });
+        {
+            uint16 protocolFee = params.zeroForOne


(optional --- maybe can create as issue tracker if keen)
been seeing this if condition quite a bit. thinking if we might want to make our code cleaner.

eg.

uint16 protocolFee = slot0Start.protocolFee.getProtocolFee(param.zeroForOne);

ChefMist

👍 prefer if we get either Snoopy or Omelette approval as well before merge

ChefMist · 2024-05-09T03:28:04Z

src/pool-bin/libraries/math/PackedUint128Math.sol

-    /// @param fee protocolFee
-    function getExternalFeeAmt(bytes32 amount, uint16 fee) internal pure returns (bytes32 z) {
-        if (fee == 0) return 0;
+    /// @param protocolFee protocolFee


missing /// @param swapFee maybe can just copy other places on what protocolFee / swapFee is over here

chefburger · 2024-05-09T03:39:53Z

👍 prefer if we get either Snoopy or Omelette approval as well before merge

hhhh, agree.. The change is bit risky @ChefSnoopy @chef-omelette

chef-omelette · 2024-05-09T05:38:20Z

src/libraries/LPFeeLibrary.sol

+    uint24 public constant ONE_HUNDRED_PERCENT_FEE = 1_000_000;
+    uint24 public constant TEN_PERCENT_FEE = 100_000;


(optional)
I think it's better to rename these to MAX_LP_FEE_CL and MAX_LP_FEE_BIN
it's more self-explanatory

chef-omelette · 2024-05-09T06:00:02Z

src/libraries/ProtocolFeeLibrary.sol

+    function validate(uint24 self) internal pure returns (bool) {
+        if (self != 0) {
+            uint16 fee0 = getZeroForOneFee(self);
+            uint16 fee1 = getOneForZeroFee(self);
+            // The fee is represented in pips and it cannot be greater than the MAX_PROTOCOL_FEE.
+            if ((fee0 > MAX_PROTOCOL_FEE) || (fee1 > MAX_PROTOCOL_FEE)) {
+                return false;
+            }
+        }
+        return true;
+    }


(optional)
just revert instead of returning boolean for simplicity and also to align with lp fee validate

chef-omelette · 2024-05-09T06:03:44Z

src/libraries/ProtocolFeeLibrary.sol

+    function calculateSwapFee(uint24 self, uint24 lpFee) internal pure returns (uint24) {
+        unchecked {
+            uint256 numerator = uint256(self) * uint256(lpFee);
+            return uint24(uint256(self) + lpFee - UnsafeMath.divRoundingUp(numerator, PIPS_DENOMINATOR));


(optional)
can be simplified by using mulDiv(self, lpFee, PIPS_DENOMINATOR)

chef-omelette · 2024-05-09T06:28:35Z

src/pool-cl/libraries/SwapMath.sol

@@ -83,7 +83,7 @@ library SwapMath {
                // we didn't reach the target, so take the remainder of the maximum input as fee
                feeAmount = uint256(amountRemaining) - amountIn;
            } else {
-                feeAmount = FullMath.mulDivRoundingUp(amountIn, feePips, 1e6 - feePips);
+                feeAmount = feePips == 1e6 ? amountIn : FullMath.mulDivRoundingUp(amountIn, feePips, 1e6 - feePips);


just to confirm: can feePips be 1_000_000? seems meaningless to allow it

yes, possible

say protocolFee = whatever, and lpFee = 100%

swapFee = protocolFee + (1 - protocolFee) * 100% = 1

chef-omelette · 2024-05-09T06:31:12Z

src/pool-bin/libraries/math/PackedUint128Math.sol

-        uint128 feeForY = fee1 == 0 ? 0 : amountY / fee1;
+        uint128 feeForX;
+        uint128 feeForY;
+        unchecked {


LGTM as well

ChefSnoopy · 2024-05-09T09:46:48Z

It seems this two need to update ?

chefburger · 2024-05-10T03:25:08Z

It seems this two need to update ?

Oh, you are right. Thx for reminding

chefburger · 2024-05-13T04:26:02Z

merge for now to unblock follow-up PRs, feel free to add more comments @chef-omelette

feat: refactor the fee mechanism for both pools

27264e9

chefburger marked this pull request as draft May 6, 2024 09:24

chefburger added 2 commits May 7, 2024 13:14

fix: make it compiles

746ce1e

fix: updating related test cases

3a57976

chefburger commented May 8, 2024

View reviewed changes

feat: more accuarate calculation for protocolFee

ce613fc

chefburger commented May 8, 2024

View reviewed changes

chefburger requested review from ChefMist and ChefSnoopy May 8, 2024 04:43

chefburger marked this pull request as ready for review May 8, 2024 04:43

chefburger commented May 8, 2024

View reviewed changes

optimization: remove unnecessary struct in clpool#swap

cb7473b

Merge branch 'main' into refactor/protocolFeeLogic

aaee7ec

ChefMist reviewed May 8, 2024

View reviewed changes

ChefMist reviewed May 9, 2024

View reviewed changes

ChefMist previously approved these changes May 9, 2024

View reviewed changes

ChefMist mentioned this pull request May 9, 2024

[Developer doc] Document about fees info #39

Open

ChefMist reviewed May 9, 2024

View reviewed changes

chefburger mentioned this pull request May 9, 2024

Optmization: add helper function to get one direction protocolFee #40

Closed

optimization: comments and small adjustment per review

2a02631

chefburger dismissed ChefMist’s stale review via 2a02631 May 9, 2024 03:48

chef-omelette previously approved these changes May 9, 2024

View reviewed changes

optimization: cache memory variable in clpool#swap

76244d4

chefburger dismissed chef-omelette’s stale review via 76244d4 May 9, 2024 06:38

chore: add gas snapshots back

01dbfcb

ChefSnoopy approved these changes May 10, 2024

View reviewed changes

chefburger merged commit 29822f3 into main May 13, 2024
2 checks passed

chefburger deleted the refactor/protocolFeeLogic branch May 13, 2024 04:26

chefburger mentioned this pull request May 16, 2024

Refactor: remove snapshot for fuzz test #45

Closed

		fees = feeY.encodeSecond();
		feeForProtocol = (amountY - receivedAmountY).getCompositionFee(protocolFee).encodeSecond();

		uint24 public constant ONE_HUNDRED_PERCENT_FEE = 1_000_000;
		uint24 public constant TEN_PERCENT_FEE = 100_000;

feat: refactor the fee mechanism for both pools #36

feat: refactor the fee mechanism for both pools #36

Conversation

chefburger commented May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist May 8, 2024 • edited Loading

Choose a reason for hiding this comment

chefburger May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefburger May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefburger May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefburger May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefburger May 8, 2024 • edited Loading

Choose a reason for hiding this comment

chef-omelette commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefburger commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefSnoopy commented May 9, 2024

chefburger commented May 10, 2024

chefburger commented May 13, 2024

chefburger commented May 6, 2024 •

edited

Loading

ChefMist May 8, 2024 •

edited

Loading

chefburger May 9, 2024 •

edited

Loading

chefburger May 8, 2024 •

edited

Loading

chefburger May 8, 2024 •

edited

Loading

chefburger May 8, 2024 •

edited

Loading

chefburger May 8, 2024 •

edited

Loading