Skip to content
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

Remove hook fees and protocol fee on withdrawal #432

Merged
merged 7 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1 +1 @@
193083
192918
2 changes: 1 addition & 1 deletion .forge-snapshots/cached dynamic fee, no hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
148446
148280
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 1 token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
139060
139280
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 2 tokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
186482
186702
2 changes: 1 addition & 1 deletion .forge-snapshots/erc20 collect protocol fees.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
26991
26968
2 changes: 1 addition & 1 deletion .forge-snapshots/gas overhead of no-op lock.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
15224
15246
2 changes: 1 addition & 1 deletion .forge-snapshots/initialize.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
78563
75775
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
319063
318670
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
201844
201474
2 changes: 1 addition & 1 deletion .forge-snapshots/mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
201764
201394
2 changes: 1 addition & 1 deletion .forge-snapshots/mintWithEmptyHookEOAInitiated.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
250959
250566
2 changes: 1 addition & 1 deletion .forge-snapshots/modify position with noop.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
56530
56582
2 changes: 1 addition & 1 deletion .forge-snapshots/native collect protocol fees.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
38657
38634
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
26329
24030
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap with native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
197331
197165
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
205899
205733
2 changes: 1 addition & 1 deletion .forge-snapshots/simpleSwapEOAInitiated.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
177187
176999
2 changes: 1 addition & 1 deletion .forge-snapshots/simpleSwapNativeEOAInitiated.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
173849
173661
Original file line number Diff line number Diff line change
@@ -1 +1 @@
127924
127793
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
115415
115284
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn claim for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
134509
134457
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint output as claim.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
218114
217926
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
192325
192160
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
115393
115262
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with noop.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
49690
49888
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
198919
198762
55 changes: 9 additions & 46 deletions src/Fees.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pragma solidity ^0.8.19;

import {Currency, CurrencyLibrary} from "./types/Currency.sol";
import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol";
import {IHookFeeManager} from "./interfaces/IHookFeeManager.sol";
import {IFees} from "./interfaces/IFees.sol";
import {FeeLibrary} from "./libraries/FeeLibrary.sol";
import {Pool} from "./libraries/Pool.sol";
Expand All @@ -22,8 +21,6 @@ abstract contract Fees is IFees, Owned {

mapping(Currency currency => uint256) public protocolFeesAccrued;

mapping(address hookAddress => mapping(Currency currency => uint256)) public hookFeesAccrued;

IProtocolFeeController public protocolFeeController;

uint256 private immutable controllerGasLimit;
Expand All @@ -35,15 +32,16 @@ abstract contract Fees is IFees, Owned {
/// @notice Fetch the protocol fees for a given pool, returning false if the call fails or the returned fees are invalid.
/// @dev to prevent an invalid protocol fee controller from blocking pools from being initialized
/// the success of this function is NOT checked on initialize and if the call fails, the protocol fees are set to 0.
/// @dev the success of this function must be checked when called in setProtocolFees
function _fetchProtocolFees(PoolKey memory key) internal returns (bool success, uint24 protocolFees) {
/// @dev the success of this function must be checked when called in setProtocolFee
function _fetchProtocolFee(PoolKey memory key) internal returns (bool success, uint16 protocolFees) {
if (address(protocolFeeController) != address(0)) {
// note that EIP-150 mandates that calls requesting more than 63/64ths of remaining gas
// will be allotted no more than this amount, so controllerGasLimit must be set with this
// in mind.
if (gasleft() < controllerGasLimit) revert ProtocolFeeCannotBeFetched();

(bool _success, bytes memory _data) = address(protocolFeeController).call{gas: controllerGasLimit}(
abi.encodeWithSelector(IProtocolFeeController.protocolFeesForPool.selector, key)
abi.encodeWithSelector(IProtocolFeeController.protocolFeeForPool.selector, key)
);
// Ensure that the return data fits within a word
if (!_success || _data.length > 32) return (false, 0);
Expand All @@ -52,44 +50,22 @@ abstract contract Fees is IFees, Owned {
assembly {
returnData := mload(add(_data, 0x20))
}
// Ensure return data does not overflow a uint24 and that the underlying fees are within bounds.
(success, protocolFees) = returnData == uint24(returnData) && _isValidProtocolFees(uint24(returnData))
? (true, uint24(returnData))
// Ensure return data does not overflow a uint16 and that the underlying fees are within bounds.
(success, protocolFees) = returnData == uint16(returnData) && _isFeeWithinBounds(uint16(returnData))
? (true, uint16(returnData))
: (false, 0);
}
}

/// @notice There is no cap on the hook fee, but it is specified as a percentage taken on the amount after the protocol fee is applied, if there is a protocol fee.
function _fetchHookFees(PoolKey memory key) internal view returns (uint24 hookFees) {
if (address(key.hooks) != address(0)) {
try IHookFeeManager(address(key.hooks)).getHookFees(key) returns (uint24 hookFeesRaw) {
uint24 swapFeeMask = key.fee.hasHookSwapFee() ? 0xFFF000 : 0;
uint24 withdrawFeeMask = key.fee.hasHookWithdrawFee() ? 0xFFF : 0;
uint24 fullFeeMask = swapFeeMask | withdrawFeeMask;
hookFees = hookFeesRaw & fullFeeMask;
} catch {}
}
}

function _fetchDynamicSwapFee(PoolKey memory key) internal view returns (uint24 dynamicSwapFee) {
dynamicSwapFee = IDynamicFeeManager(address(key.hooks)).getFee(msg.sender, key);
if (dynamicSwapFee >= MAX_SWAP_FEE) revert FeeTooLarge();
}

function _isValidProtocolFees(uint24 protocolFees) internal pure returns (bool) {
if (protocolFees != 0) {
uint16 protocolSwapFee = uint16(protocolFees >> 12);
uint16 protocolWithdrawFee = uint16(protocolFees & 0xFFF);
return _isFeeWithinBounds(protocolSwapFee) && _isFeeWithinBounds(protocolWithdrawFee);
}
return true;
}

/// @dev Only the lower 12 bits are used here to encode the fee denominator.
function _isFeeWithinBounds(uint16 fee) internal pure returns (bool) {
if (fee != 0) {
uint16 fee0 = fee % 64;
uint16 fee1 = fee >> 6;
uint16 fee0 = fee % 256;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this? If we want to increase protocol fee granularity I think thats a separate discussion. I think this means the fee can be a lot smaller and I thought we intentionally had it bound before (both from a min and a max).

In any case it's worth a comment on what the min fee value is now that there are 8 bits that can be used instead of 6.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont personally remember us giving it a lower bound for any reason other than running out of space in slot0 - but happy to change it back!

My reasoning was just that increasing bits only really increases flexibility of what fee you want to set - and our bound on >=4 will still hold.

4 bits gives a granularity of 6.67%.
6 bits gives a granularity of 1.58%
8 bits gives a granularity of 0.38%

So with 6 bits its impossible to set a fee close to 1% or close to 2% - it has to be 0%, 1.58%, or 3.16%. 8 bits gives more flexibility for that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's fine, can you add a comment? I just wasn't expecting that to change, not that its a bad change but just deserves some reasoning :)

uint16 fee1 = fee >> 8;
// The fee is specified as a denominator so it cannot be LESS than the MIN_PROTOCOL_FEE_DENOMINATOR (unless it is 0).
if (
(fee0 != 0 && fee0 < MIN_PROTOCOL_FEE_DENOMINATOR) || (fee1 != 0 && fee1 < MIN_PROTOCOL_FEE_DENOMINATOR)
Expand All @@ -115,17 +91,4 @@ abstract contract Fees is IFees, Owned {
protocolFeesAccrued[currency] -= amountCollected;
currency.transfer(recipient, amountCollected);
}

function collectHookFees(address recipient, Currency currency, uint256 amount)
external
returns (uint256 amountCollected)
{
address hookAddress = msg.sender;

amountCollected = (amount == 0) ? hookFeesAccrued[hookAddress][currency] : amount;
recipient = (recipient == address(0)) ? hookAddress : recipient;

hookFeesAccrued[hookAddress][currency] -= amountCollected;
currency.transfer(recipient, amountCollected);
}
}
48 changes: 10 additions & 38 deletions src/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {NoDelegateCall} from "./NoDelegateCall.sol";
import {Owned} from "./Owned.sol";
import {IHooks} from "./interfaces/IHooks.sol";
import {IDynamicFeeManager} from "./interfaces/IDynamicFeeManager.sol";
import {IHookFeeManager} from "./interfaces/IHookFeeManager.sol";
import {IPoolManager} from "./interfaces/IPoolManager.sol";
import {ILockCallback} from "./interfaces/callback/ILockCallback.sol";
import {Fees} from "./Fees.sol";
Expand Down Expand Up @@ -57,11 +56,11 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
external
view
override
returns (uint160 sqrtPriceX96, int24 tick, uint24 protocolFees, uint24 hookFees)
returns (uint160 sqrtPriceX96, int24 tick, uint16 protocolFee)
{
Pool.Slot0 memory slot0 = pools[id].slot0;

return (slot0.sqrtPriceX96, slot0.tick, slot0.protocolFees, slot0.hookFees);
return (slot0.sqrtPriceX96, slot0.tick, slot0.protocolFee);
}

/// @inheritdoc IPoolManager
Expand Down Expand Up @@ -130,10 +129,10 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
}

PoolId id = key.toId();
(, uint24 protocolFees) = _fetchProtocolFees(key);
(, uint16 protocolFee) = _fetchProtocolFee(key);
uint24 swapFee = key.fee.isDynamicFee() ? _fetchDynamicSwapFee(key) : key.fee.getStaticFee();

tick = pools[id].initialize(sqrtPriceX96, protocolFees, _fetchHookFees(key), swapFee);
tick = pools[id].initialize(sqrtPriceX96, protocolFee, swapFee);

if (key.hooks.shouldCallAfterInitialize()) {
if (
Expand Down Expand Up @@ -217,8 +216,7 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
}
}

Pool.FeeAmounts memory feeAmounts;
(delta, feeAmounts) = pools[id].modifyPosition(
delta = pools[id].modifyPosition(
Pool.ModifyPositionParams({
owner: msg.sender,
tickLower: params.tickLower,
Expand All @@ -230,21 +228,6 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {

_accountPoolBalanceDelta(key, delta);

unchecked {
if (feeAmounts.feeForProtocol0 > 0) {
protocolFeesAccrued[key.currency0] += feeAmounts.feeForProtocol0;
}
if (feeAmounts.feeForProtocol1 > 0) {
protocolFeesAccrued[key.currency1] += feeAmounts.feeForProtocol1;
}
if (feeAmounts.feeForHook0 > 0) {
hookFeesAccrued[address(key.hooks)][key.currency0] += feeAmounts.feeForHook0;
}
if (feeAmounts.feeForHook1 > 0) {
hookFeesAccrued[address(key.hooks)][key.currency1] += feeAmounts.feeForHook1;
}
}

if (key.hooks.shouldCallAfterModifyPosition()) {
if (
key.hooks.afterModifyPosition(msg.sender, key, params, delta, hookData)
Expand Down Expand Up @@ -286,10 +269,9 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
}

uint256 feeForProtocol;
uint256 feeForHook;
uint24 swapFee;
Pool.SwapState memory state;
(delta, feeForProtocol, feeForHook, swapFee, state) = pools[id].swap(
(delta, feeForProtocol, swapFee, state) = pools[id].swap(
Pool.SwapParams({
tickSpacing: key.tickSpacing,
zeroForOne: params.zeroForOne,
Expand All @@ -305,9 +287,6 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
if (feeForProtocol > 0) {
protocolFeesAccrued[params.zeroForOne ? key.currency0 : key.currency1] += feeForProtocol;
}
if (feeForHook > 0) {
hookFeesAccrued[address(key.hooks)][params.zeroForOne ? key.currency0 : key.currency1] += feeForHook;
}
}

if (key.hooks.shouldCallAfterSwap()) {
Expand Down Expand Up @@ -391,19 +370,12 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
_burn(currency, amount);
}

function setProtocolFees(PoolKey memory key) external {
(bool success, uint24 newProtocolFees) = _fetchProtocolFees(key);
function setProtocolFee(PoolKey memory key) external {
(bool success, uint16 newProtocolFee) = _fetchProtocolFee(key);
if (!success) revert ProtocolFeeControllerCallFailedOrInvalidResult();
PoolId id = key.toId();
pools[id].setProtocolFees(newProtocolFees);
emit ProtocolFeeUpdated(id, newProtocolFees);
}

function setHookFees(PoolKey memory key) external {
uint24 newHookFees = _fetchHookFees(key);
PoolId id = key.toId();
pools[id].setHookFees(newHookFees);
emit HookFeeUpdated(id, newHookFees);
pools[id].setProtocolFee(newProtocolFee);
emit ProtocolFeeUpdated(id, newProtocolFee);
}

function updateDynamicSwapFee(PoolKey memory key) external {
Expand Down
3 changes: 0 additions & 3 deletions src/interfaces/IFees.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,4 @@ interface IFees {

/// @notice Given a currency address, returns the protocol fees accrued in that currency
function protocolFeesAccrued(Currency) external view returns (uint256);

/// @notice Given a hook and a currency address, returns the fees accrued
function hookFeesAccrued(address, Currency) external view returns (uint256);
}
13 changes: 0 additions & 13 deletions src/interfaces/IHookFeeManager.sol

This file was deleted.

Loading