More efficient dynamic fee setter #1

markusbkoch · 2023-11-24T10:49:25Z

Overrides getSwapFeePercentage so as to be able to pass userdata as an argument and dynamically set fees without having to write to storage before/after every swap.

markusbkoch · 2023-11-24T11:27:24Z

pkg/pool-weighted/lbp/LiquidityBootstrappingPool.sol

Could we move all these files that have been renamed without changes back to their original locations so as not to clutter the PR?

markusbkoch · 2023-11-24T11:28:28Z

pkg/pool-weighted/hardhat.config.ts

@@ -18,7 +18,18 @@ export default {
    },
  },
  solidity: {
-    compilers: hardhatBaseConfig.compilers,
+    // compilers: hardhatBaseConfig.compilers,


This is probably something we should leave as was to avoid introducing issues to the monorepo

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

pkg/pool-utils/contracts/BasePool.sol

pkg/pool-utils/contracts/BaseMinimalSwapInfoPool.sol

markusbkoch · 2023-11-24T15:08:48Z

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

+
+import "@balancer-labs/v2-solidity-utils/contracts/openzeppelin/Ownable.sol";
+
+contract CustomFeeAuthorizer is Ownable {


I worry this might conflict with Balancer's own access control mechanisms. I think it would make more sense instead to introduce the notion of address public solver; solver must be set on pool creation and can't be edited; solver can edit isCustomFeeSetter and call setSwapFeePercentage

pkg/pool-utils/contracts/BasePool.sol

pkg/pool-weighted/contracts/WeightedPool.sol

markusbkoch · 2023-12-01T10:54:14Z

pkg/pool-weighted/contracts/WeightedPool.sol

+    function setSwapFeePercentage(uint256 swapFeePercentage) public virtual override whenNotPaused {
+        // here msg.sender is used as this function directlly be called by the admin
+        // not via any other contract
+        if(isCustomFeeEnabled && solver == msg.sender){


We don't want governance to be able to set fees in this pool, so this needs to be something like

if (isCustomFeeEnabled) { require(solver == msg.sender,'CALLER_IS_NOT_SOLVER'); _setSwapFeePercentage(swapFeePercentage); } else { super.setSwapFeePercentage(swapFeePercentage); }

markusbkoch · 2023-12-01T10:56:10Z

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

+        isCustomFeeEnabled = true;
+    }
+
+    function _setSolverAddress(address _solver) internal {


Shouldn't this be public, onlySolver?

if we can mark isCustomFeeEnabled as immutable then we can call this function in the constructor based on the value of isCustomFeeEnabled.
But if not then initially 0 address will be marked as solver so we cannot use onlySolver here. But we can have a logic where initially owner() can call this function and after that onlySolver can call that

@markusbkoch who should set the solver? Governance, or is it fixed at pool creation time? Can the existing solver pass the torch to another address?

I don't see any references to _setSolverAddress, so this needs to be worked out anyways.

@markusbkoch who should set the solver? Governance, or is it fixed at pool creation time? Can the existing solver pass the torch to another address?

Governance should not be able to set it. Fixed would be good enough, solver being able to pass it on would be a nice to have

markusbkoch · 2023-12-01T10:56:47Z

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

+        emit feeSetterRemoved(_toRemove);
+    }
+
+    function enableCustomFee() onlySolver() internal {


This needs to be public?

If we want a toggle function for isCustomFeeEnabled then we can mark this function as public.

But if we can mark isCustomFeeEnabled as immutable then we will have to pass the value of isCustomFeeEnabled from WeightedPool constructor so we can remove this function.

markusbkoch · 2023-12-01T11:02:15Z

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

+    event feeSetterRemoved(address indexed feeSetter);
+
+    mapping(address => bool) private isCustomFeeSetter;
+    bool public isCustomFeeEnabled = false ;


Safe to say solvers will always want this to be true. If we could we make this immutable, does the code become more efficient?

If we want to make isCustomFeeEnabled as immutable then we will have to initialize this in the constructor. But if we pass 1 more argument in the constructor of WeightedPool it gives stack to deep error.

jubeira

First pass done!
I've left some minor comments, and I have some questions around how the solver is set / how governance would work. Also, the pool layers make it a bit difficult to reason just by seeing the diff, but is this feature intended to be used just for weighted pools, or for every pool type?

On top of that, it'd be good to see tests to cover the new functionality

jubeira · 2023-12-14T19:51:37Z

pkg/pool-utils/contracts/BasePool.sol

+    function getSwapFeePercentage(bytes memory userData,
+     OperationType _operation) public view virtual returns (uint256) {
+        // this code is just to avoid un-used variable warning.
+        // this will have no impact on the execution
+        // will remove this code in future
+        if(_operation == OperationType.SWAP){
+            userData = hex"00";
+        }


This can be solved by removing the variable name from the declaration.

Anyways I'd consider removing this from BasePool if this is a feature that will only affect WeightedPool.

Anyways I'd consider removing this from BasePool if this is a feature that will only affect WeightedPool.

We'd like this feature in other pools too

jubeira · 2023-12-14T19:53:46Z

pkg/pool-utils/contracts/BaseMinimalSwapInfoPool.sol

@@ -36,6 +36,8 @@ abstract contract BaseMinimalSwapInfoPool is IMinimalSwapInfoPool, BasePool {
    ) public override onlyVault(request.poolId) returns (uint256) {
        _beforeSwapJoinExit();

+        emit SwapFeePercentageChanged(getSwapFeePercentage(request.userData, OperationType.SWAP));


We're doing all the work here to get the right fee and emit the event, and doing it all over again inside _substractSwapFeeAmount. I'd save the value and pass it as an argument.

jubeira · 2023-12-14T19:55:03Z

pkg/pool-weighted/contracts/BaseWeightedPool.sol

+        // _doJoin performs actions specific to type of join
+        // but it's a view function so can not emit event


I'd consider making _doJoin non-view if needed. I'll give this some more thought.

Have you thought about this? Should we make it a non-view function?

After some more thought, I think it's fine as it is.

jubeira · 2023-12-14T19:56:08Z

pkg/pool-weighted/contracts/BaseWeightedPool.sol

+        WeightedPoolUserData.JoinKind kind = userData.joinKind();
+        if(kind == WeightedPoolUserData.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT ||
+         kind == WeightedPoolUserData.JoinKind.TOKEN_IN_FOR_EXACT_BPT_OUT){
+            emit SwapFeePercentageChanged(getSwapFeePercentage(userData, OperationType.JOIN));


Same as above; we're getting the fee % inside _doJoin as well. Maybe it's better to just save it and pass it along.

If I create an extra variable it gives stack too deep error.

jubeira · 2023-12-14T19:56:32Z

pkg/pool-weighted/contracts/BaseWeightedPool.sol

+        WeightedPoolUserData.ExitKind kind = userData.exitKind();
+        if(kind == WeightedPoolUserData.ExitKind.EXACT_BPT_IN_FOR_ONE_TOKEN_OUT ||
+         kind ==  WeightedPoolUserData.ExitKind.BPT_IN_FOR_EXACT_TOKENS_OUT){
+            emit SwapFeePercentageChanged(getSwapFeePercentage(userData, OperationType.EXIT));


Same as above (store and pass)

jubeira · 2023-12-14T19:59:25Z

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

+        isCustomFeeEnabled = true;
+    }
+
+    function _setSolverAddress(address _solver) internal {


@markusbkoch who should set the solver? Governance, or is it fixed at pool creation time? Can the existing solver pass the torch to another address?

jubeira · 2023-12-14T19:59:48Z

pkg/pool-weighted/contracts/CustomFeeAuthorizer.sol

+        isCustomFeeEnabled = true;
+    }
+
+    function _setSolverAddress(address _solver) internal {


I don't see any references to _setSolverAddress, so this needs to be worked out anyways.

jubeira · 2023-12-14T20:01:35Z

pkg/pool-weighted/contracts/WeightedPool.sol

+    function getSwapFeePercentage(bytes memory userData,
+     OperationType _operation) public view virtual override returns (uint256 _fee) {


Alright, this doesn't seem to be a bad pattern. Assuming isCustomFeeEnabled is always true, then only those who can register the custom fee go through a different codepath when compared to the actual weighted pool.

jubeira · 2023-12-14T20:02:09Z

pkg/pool-weighted/contracts/WeightedPool.sol

+            require(solver == msg.sender,'CALLER_IS_NOT_SOLVER');
+             _setSwapFeePercentage(swapFeePercentage);
+        } else {
+            super.setSwapFeePercentage(swapFeePercentage);


If isCustomFeeEnabled is going to be immutable / true, that would break the capability of Governance of setting fees to any pools created from here.

Should Governance always be capable of setting the fee anyways? I'd consider using something like authenticateFor.

No, governance should not be able to set the fess. The solver has certain expectations regarding this pool's fee, governance being able to set the fee could potentially rug the solver

jubeira · 2023-12-14T20:14:05Z

pkg/interfaces/contracts/vault/IBasePool.sol

@@ -45,6 +45,9 @@ interface IBasePool is IPoolSwapStructs {
     * Contracts implementing this function should check that the caller is indeed the Vault before performing any
     * state-changing operations, such as minting pool shares.
     */
+     // enum for transaction type
+    enum OperationType { JOIN, EXIT, SWAP }


@markusbkoch do we want this feature to apply to weighted pools, or to every pool type?

every pool type

srv-smn · 2024-01-07T15:44:05Z

Hey @jubeira,
If I pass any extra parameter in the constructor of WeightedPool contract it gives "stack too deep" error. so making isCustomFeeEnabled immutable will not be possible and can not call some internal function of CustomFeeAuthorizer inside the constructor based on the value of isCustomFeeEnabled. Rather we will have to make the some internal function public in CustomFeeAuthorizer.

srv-smn · 2024-01-09T16:47:22Z

While running the test cases I was getting some some issues. Most of them is resolved. But 1 is remaining.
WeightedPoolFactory constructor arguments sets swap fee: TypeError: pool.getSwapFeePercentage is not a function
I think this is because I had overloaded getSwapFeePercentage function.
Any suggestion how can I resolve this ?

jubeira · 2024-03-05T15:35:44Z

@srv-smn I've addressed most of the comments on top of this branch and opened a PR here: balancer#2558.

Pending:

Address how to set solver address (and potentially change the owner): More efficient dynamic fee setter #1 (comment).
Add tests for this feature (end to end)

Sourav Suman added 4 commits November 16, 2023 09:06

overriden getSwapFeePercent

2c9846f

module changes

0e848c4

emit event SwapFeePercentageChanged

6c37f0d

emit event SwapFeePercentageChanged in swap

8ad4318