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

Taking fee hook #121

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Taking fee hook #121

wants to merge 17 commits into from

Conversation

Jun1on
Copy link
Contributor

@Jun1on Jun1on commented Jun 7, 2024

Description of changes

Added taking fee hook to example hooks

Currency[] currencies;
}

constructor(IPoolManager _poolManager, uint128 _swapFeeBips) BaseHook(_poolManager) Owned(msg.sender) {

Choose a reason for hiding this comment

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

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);

Choose a reason for hiding this comment

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

nit: specifiedTokenIs0 -> currency0Specified

});
}

function afterSwap(

Choose a reason for hiding this comment

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

would love to see a discussion of pros and cons of taking the fee afterSwap vs. before

assertEq(currency1.balanceOf(TREASURY) / R, expectedFee / R);
}

function testEdgeCase() public {

Choose a reason for hiding this comment

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

exactly what edge case(s) are you testing? would it be possible to break it into separate test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

nit: maybe TakingFee -> FeeTaking

Copy link
Contributor Author

@Jun1on Jun1on left a comment

Choose a reason for hiding this comment

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

reply

assertEq(currency1.balanceOf(TREASURY) / R, expectedFee / R);
}

function testEdgeCase() public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@Jun1on Jun1on force-pushed the taking-fee-hook branch from f382d57 to a526b2d Compare June 14, 2024 15:13
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";
Copy link
Member

Choose a reason for hiding this comment

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

lol we'll have to update this based off your other change in core!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

you can use SafeCallback when that is merged

contract FeeTaking is BaseHook, IUnlockCallback, Owned {
using SafeCast for uint256;

bytes internal constant ZERO_BYTES = bytes("");
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a ZERO_BYTES constant defined in BalanceDeltaLibrary you can use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

i think this is saying "if the swap is an exactOutput swap, take the fee of the absolute value of the amt"?

Copy link
Member

Choose a reason for hiding this comment

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

is that right?

// if fee is on output, get the absolute output amount
if (swapAmount < 0) swapAmount = -swapAmount;

uint256 feeAmount = (uint128(swapAmount) * swapFeeBips) / TOTAL_BIPS;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

instead of require we like to use custom reverts

if (_swapFeeBips > MAX_BIPS) revert FeeTooLarge();

contracts/hooks/examples/FeeTaking.sol Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants