New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More efficient dynamic fee setter #1
base: master
Are you sure you want to change the base?
Changes from 6 commits
2c9846f
0e848c4
6c37f0d
8ad4318
bac31ed
c38d1fb
16f5533
7f40f16
b7af5dd
e5bb1d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're doing all the work here to get the right fee and emit the event, and doing it all over again inside |
||
|
||
uint256 scalingFactorTokenIn = _scalingFactor(request.tokenIn); | ||
uint256 scalingFactorTokenOut = _scalingFactor(request.tokenOut); | ||
|
||
|
@@ -44,7 +46,7 @@ abstract contract BaseMinimalSwapInfoPool is IMinimalSwapInfoPool, BasePool { | |
|
||
if (request.kind == IVault.SwapKind.GIVEN_IN) { | ||
// Fees are subtracted before scaling, to reduce the complexity of the rounding direction analysis. | ||
request.amount = _subtractSwapFeeAmount(request.amount); | ||
request.amount = _subtractSwapFeeAmount(request.amount, request.userData, OperationType.SWAP); | ||
|
||
// All token amounts are upscaled. | ||
request.amount = _upscale(request.amount, scalingFactorTokenIn); | ||
|
@@ -63,7 +65,7 @@ abstract contract BaseMinimalSwapInfoPool is IMinimalSwapInfoPool, BasePool { | |
amountIn = _downscaleUp(amountIn, scalingFactorTokenIn); | ||
|
||
// Fees are added after scaling happens, to reduce the complexity of the rounding direction analysis. | ||
return _addSwapFeeAmount(amountIn); | ||
return _addSwapFeeAmount(amountIn, request.userData, OperationType.SWAP); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,10 +169,22 @@ abstract contract BasePool is | |
* @notice Return the current value of the swap fee percentage. | ||
* @dev This is stored in `_miscData`. | ||
*/ | ||
function getSwapFeePercentage() public view virtual override returns (uint256) { | ||
function getSwapFeePercentage() public view virtual override returns (uint256) { | ||
markusbkoch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return _miscData.decodeUint(_SWAP_FEE_PERCENTAGE_OFFSET, _SWAP_FEE_PERCENTAGE_BIT_LENGTH); | ||
} | ||
|
||
// overloaded method implementation | ||
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"; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be solved by removing the variable name from the declaration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyways I'd consider removing this from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We'd like this feature in other pools too |
||
return getSwapFeePercentage(); | ||
} | ||
|
||
/** | ||
* @notice Return the ProtocolFeesCollector contract. | ||
* @dev This is immutable, and retrieved from the Vault on construction. (It is also immutable in the Vault.) | ||
|
@@ -592,17 +604,18 @@ abstract contract BasePool is | |
/** | ||
* @dev Adds swap fee amount to `amount`, returning a higher value. | ||
*/ | ||
function _addSwapFeeAmount(uint256 amount) internal view returns (uint256) { | ||
function _addSwapFeeAmount(uint256 amount, bytes memory userData, OperationType _operationType) internal view returns (uint256) { | ||
// This returns amount + fee amount, so we round up (favoring a higher fee amount). | ||
return amount.divUp(getSwapFeePercentage().complement()); | ||
return amount.divUp(getSwapFeePercentage(userData, _operationType).complement()); | ||
} | ||
|
||
|
||
/** | ||
* @dev Subtracts swap fee amount from `amount`, returning a lower value. | ||
*/ | ||
function _subtractSwapFeeAmount(uint256 amount) internal view returns (uint256) { | ||
function _subtractSwapFeeAmount(uint256 amount, bytes memory userData, OperationType _operationType) internal view returns (uint256) { | ||
// This returns amount - fee amount, so we round up (favoring a higher fee amount). | ||
uint256 feeAmount = amount.mulUp(getSwapFeePercentage()); | ||
uint256 feeAmount = amount.mulUp(getSwapFeePercentage(userData, _operationType)); | ||
return amount.sub(feeAmount); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -225,6 +225,15 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool { | |
userData | ||
); | ||
|
||
// _doJoin performs actions specific to type of join | ||
// but it's a view function so can not emit event | ||
Comment on lines
+228
to
+229
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd consider making There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you thought about this? Should we make it a non-view function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some more thought, I think it's fine as it is. |
||
|
||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above; we're getting the fee % inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I create an extra variable it gives stack too deep error. |
||
} | ||
|
||
_afterJoinExit( | ||
preJoinExitInvariant, | ||
balances, | ||
|
@@ -280,7 +289,7 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool { | |
normalizedWeights, | ||
amountsIn, | ||
totalSupply, | ||
getSwapFeePercentage() | ||
getSwapFeePercentage(userData, OperationType.JOIN) | ||
); | ||
|
||
_require(bptAmountOut >= minBPTAmountOut, Errors.BPT_OUT_MIN_AMOUNT); | ||
|
@@ -304,7 +313,7 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool { | |
normalizedWeights[tokenIndex], | ||
bptAmountOut, | ||
totalSupply, | ||
getSwapFeePercentage() | ||
getSwapFeePercentage(userData, OperationType.JOIN) | ||
); | ||
|
||
// We join in a single token, so we initialize amountsIn with zeros | ||
|
@@ -353,6 +362,15 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool { | |
userData | ||
); | ||
|
||
// _doExit performs actions specific to type of exit | ||
// but it's a view function so can not emit event | ||
|
||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above (store and pass) |
||
} | ||
|
||
_afterJoinExit( | ||
preJoinExitInvariant, | ||
balances, | ||
|
@@ -407,7 +425,7 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool { | |
normalizedWeights[tokenIndex], | ||
bptAmountIn, | ||
totalSupply, | ||
getSwapFeePercentage() | ||
getSwapFeePercentage(userData, OperationType.EXIT) | ||
); | ||
|
||
// This is an exceptional situation in which the fee is charged on a token out instead of a token in. | ||
|
@@ -448,7 +466,7 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool { | |
normalizedWeights, | ||
amountsOut, | ||
totalSupply, | ||
getSwapFeePercentage() | ||
getSwapFeePercentage(userData, OperationType.EXIT) | ||
); | ||
_require(bptAmountIn <= maxBPTAmountIn, Errors.BPT_IN_MAX_AMOUNT); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.7.0; | ||
|
||
|
||
contract CustomFeeAuthorizer { | ||
event feeSetterAdded(address indexed feeSetter); | ||
event feeSetterRemoved(address indexed feeSetter); | ||
|
||
mapping(address => bool) private isCustomFeeSetter; | ||
bool public isCustomFeeEnabled = false ; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Safe to say solvers will always want this to be true. If we could we make this immutable, does the code become more efficient? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to make |
||
address public solver; | ||
|
||
function canSetCustomFee(address _setterAddress) public view returns(bool _isAuth){ | ||
if(isCustomFeeEnabled){ | ||
_isAuth = (isCustomFeeSetter[_setterAddress] || solver == _setterAddress) ; | ||
}else{ | ||
_isAuth = false; | ||
} | ||
} | ||
|
||
function addCustomFeeSetter(address _toAdd) onlySolver public { | ||
require(_toAdd != address(0)); | ||
require(isCustomFeeEnabled,"Custom Fee Not Enabled"); | ||
isCustomFeeSetter[_toAdd] = true; | ||
emit feeSetterAdded(_toAdd); | ||
} | ||
|
||
function removeCustomFeeSetter(address _toRemove) onlySolver public { | ||
require(_toRemove != msg.sender); | ||
isCustomFeeSetter[_toRemove] = false; | ||
emit feeSetterRemoved(_toRemove); | ||
} | ||
Comment on lines
+21
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @markusbkoch do we want governance to have power over this, or just the solver? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just the solver |
||
|
||
function enableCustomFee() onlySolver() internal { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be public? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want a toggle function for But if we can mark |
||
require(!isCustomFeeEnabled, "Already Enabled"); | ||
isCustomFeeEnabled = true; | ||
} | ||
|
||
function _setSolverAddress(address _solver) internal { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be public, onlySolver? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we can mark There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any references to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
require(_solver != address(0)); | ||
solver = _solver; | ||
} | ||
|
||
modifier onlySolver() { | ||
require(solver == msg.sender,'CALLER_IS_NOT_SOLVER'); | ||
_; | ||
} | ||
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,14 @@ pragma experimental ABIEncoderV2; | |
|
||
import "./BaseWeightedPool.sol"; | ||
import "./WeightedPoolProtocolFees.sol"; | ||
import "./CustomFeeAuthorizer.sol"; | ||
|
||
/** | ||
* @dev Basic Weighted Pool with immutable weights. | ||
*/ | ||
contract WeightedPool is BaseWeightedPool, WeightedPoolProtocolFees { | ||
contract WeightedPool is BaseWeightedPool, WeightedPoolProtocolFees,CustomFeeAuthorizer { | ||
markusbkoch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using FixedPoint for uint256; | ||
using WeightedPoolUserData for bytes; | ||
|
||
uint256 private constant _MAX_TOKENS = 8; | ||
|
||
|
@@ -401,4 +403,50 @@ contract WeightedPool is BaseWeightedPool, WeightedPoolProtocolFees { | |
{ | ||
return super._isOwnerOnlyAction(actionId); | ||
} | ||
|
||
function getSwapFeePercentage(bytes memory userData, | ||
OperationType _operation) public view virtual override returns (uint256 _fee) { | ||
Comment on lines
+407
to
+408
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, this doesn't seem to be a bad pattern. Assuming |
||
// using tx.origin insted of msg.sender as these functions are called | ||
// during join/exit/swap via vault | ||
if(isCustomFeeEnabled && canSetCustomFee(tx.origin)){ | ||
if(_operation == OperationType.JOIN ){ | ||
WeightedPoolUserData.JoinKind kind = userData.joinKind(); | ||
if(kind == WeightedPoolUserData.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT){ | ||
_fee = userData.exactTokensInForBptOutCustomFee(); | ||
} else { | ||
if(kind == WeightedPoolUserData.JoinKind.TOKEN_IN_FOR_EXACT_BPT_OUT){ | ||
_fee = userData.tokenInForExactBptOutCustomFee(); | ||
} | ||
} | ||
}else{ | ||
if(_operation == OperationType.EXIT){ | ||
WeightedPoolUserData.ExitKind kind = userData.exitKind(); | ||
if(kind == WeightedPoolUserData.ExitKind.EXACT_BPT_IN_FOR_ONE_TOKEN_OUT){ | ||
_fee = userData.exactBptInForTokenOutCustomFee(); | ||
} else { | ||
if(kind == WeightedPoolUserData.ExitKind.BPT_IN_FOR_EXACT_TOKENS_OUT){ | ||
_fee = userData.bptInForExactTokensOutCustomFee(); | ||
} | ||
} | ||
} else{ | ||
if(_operation == OperationType.SWAP){ | ||
_fee = userData.swapCustomFee(); | ||
} | ||
} | ||
} | ||
}else{ | ||
_fee = getSwapFeePercentage(); | ||
} | ||
} | ||
|
||
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){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want governance to be able to set fees in this pool, so this needs to be something like
|
||
_setSwapFeePercentage(swapFeePercentage); | ||
}else { | ||
super.setSwapFeePercentage(swapFeePercentage); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Should Governance always be capable of setting the fee anyways? I'd consider using something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} | ||
// used if else so that it should not break the existing flow | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,18 @@ export default { | |
}, | ||
}, | ||
solidity: { | ||
compilers: hardhatBaseConfig.compilers, | ||
// compilers: hardhatBaseConfig.compilers, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably something we should leave as was to avoid introducing issues to the monorepo |
||
compilers: [ | ||
{ | ||
version: '0.7.1', | ||
settings: { | ||
optimizer: { | ||
enabled: true, | ||
runs: 10, | ||
}, | ||
}, | ||
}, | ||
], | ||
overrides: { ...hardhatBaseConfig.overrides(name) }, | ||
}, | ||
warnings: hardhatBaseConfig.warnings, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markusbkoch do we want this feature to apply to weighted pools, or to every pool type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every pool type