Taking fee hook #121
Taking fee hook #121
Conversation
| Currency[] currencies; | ||
| } | ||
|
|
||
| constructor(IPoolManager _poolManager, uint128 _swapFeeBips) BaseHook(_poolManager) Owned(msg.sender) { |
There was a problem hiding this comment.
might want to allow setting owner other than msg.sender to be more generalizable
| bytes calldata | ||
| ) external override returns (bytes4, int128) { | ||
| // fee will be in the unspecified token of the swap | ||
| bool specifiedTokenIs0 = (params.amountSpecified < 0 == params.zeroForOne); |
There was a problem hiding this comment.
nit: specifiedTokenIs0 -> currency0Specified
| }); | ||
| } | ||
|
|
||
| function afterSwap( |
There was a problem hiding this comment.
would love to see a discussion of pros and cons of taking the fee afterSwap vs. before
test/TakingFee.t.sol
Outdated
| assertEq(currency1.balanceOf(TREASURY) / R, expectedFee / R); | ||
| } | ||
|
|
||
| function testEdgeCase() public { |
There was a problem hiding this comment.
exactly what edge case(s) are you testing? would it be possible to break it into separate test cases?
There was a problem hiding this comment.
the first swap exhausts the pool of its supply of currency1 so that it is only left with 1 wei. in the second swap, the user wants exact of currency0, so the fee would be taken in currency1.
this test previously failed before ERC6909 implementation because the pool had insufficient tokens to transfer.
There was a problem hiding this comment.
nit: maybe TakingFee -> FeeTaking
test/TakingFee.t.sol
Outdated
| assertEq(currency1.balanceOf(TREASURY) / R, expectedFee / R); | ||
| } | ||
|
|
||
| function testEdgeCase() public { |
There was a problem hiding this comment.
the first swap exhausts the pool of its supply of currency1 so that it is only left with 1 wei. in the second swap, the user wants exact of currency0, so the fee would be taken in currency1.
this test previously failed before ERC6909 implementation because the pool had insufficient tokens to transfer.
| import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol"; | ||
| import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; | ||
| import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; | ||
| import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; |
There was a problem hiding this comment.
lol we'll have to update this based off your other change in core!
There was a problem hiding this comment.
we should mass rename everything here once that goes through
| import {Owned} from "solmate/auth/Owned.sol"; | ||
| import {IUnlockCallback} from "@uniswap/v4-core/src/interfaces/callback/IUnlockCallback.sol"; | ||
|
|
||
| contract FeeTaking is BaseHook, IUnlockCallback, Owned { |
There was a problem hiding this comment.
you can use SafeCallback when that is merged
| contract FeeTaking is BaseHook, IUnlockCallback, Owned { | ||
| using SafeCast for uint256; | ||
|
|
||
| bytes internal constant ZERO_BYTES = bytes(""); |
There was a problem hiding this comment.
I think there is a ZERO_BYTES constant defined in BalanceDeltaLibrary you can use
There was a problem hiding this comment.
BalanceDelta.sol has ZERO_DELTA but not ZERO_BYTES
i saw other example hooks use
bytes internal constant ZERO_BYTES = bytes("");
| import {Owned} from "solmate/auth/Owned.sol"; | ||
| import {IUnlockCallback} from "@uniswap/v4-core/src/interfaces/callback/IUnlockCallback.sol"; | ||
|
|
||
| contract FeeTaking is BaseHook, IUnlockCallback, Owned { |
There was a problem hiding this comment.
It would be cool to think about a way to make this not Owned potentially
| swapFeeBips = _swapFeeBips; | ||
| } | ||
|
|
||
| function getHookPermissions() public pure override returns (Hooks.Permissions memory) { |
There was a problem hiding this comment.
Add natspec! Can you just provide some context to the flags that are enabled.
| bool currency0Specified = (params.amountSpecified < 0 == params.zeroForOne); | ||
| (Currency feeCurrency, int128 swapAmount) = | ||
| (currency0Specified) ? (key.currency1, delta.amount1()) : (key.currency0, delta.amount0()); | ||
| // if fee is on output, get the absolute output amount |
There was a problem hiding this comment.
i think this is saying "if the swap is an exactOutput swap, take the fee of the absolute value of the amt"?
| // if fee is on output, get the absolute output amount | ||
| if (swapAmount < 0) swapAmount = -swapAmount; | ||
|
|
||
| uint256 feeAmount = (uint128(swapAmount) * swapFeeBips) / TOTAL_BIPS; |
There was a problem hiding this comment.
so the fee amount is calculated on the specified currency but applied in the unspecified currency.. I think this is odd... What if the two currencies have different decimals? or the conversion between them is not 1-1.
ie maybe this ends up being a fee amount of like 10000 in currency0 which is worth like $0.50 in USD but that same amount in currency1 is $1000 in USD?
There was a problem hiding this comment.
jk I read this wrong - I thought swapAmount meant currencySpecified. I think writing a lib like I suggest above plus renaming the amount to amountUnspecified could help here with readability
| return (BaseHook.afterSwap.selector, feeAmount.toInt128()); | ||
| } | ||
|
|
||
| function setSwapFeeBips(uint128 _swapFeeBips) external onlyOwner { |
There was a problem hiding this comment.
in the world where there is no owner, maybe this hook is just initialized with a fee?
| } | ||
|
|
||
| function setSwapFeeBips(uint128 _swapFeeBips) external onlyOwner { | ||
| require(_swapFeeBips <= MAX_BIPS); |
There was a problem hiding this comment.
instead of require we like to use custom reverts
if (_swapFeeBips > MAX_BIPS) revert FeeTooLarge();
Description of changes
Added taking fee hook to example hooks