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

beforeSwap returns dynamic fee #648

Merged
merged 26 commits into from
May 17, 2024
Merged

beforeSwap returns dynamic fee #648

merged 26 commits into from
May 17, 2024

Conversation

saucepoint
Copy link
Collaborator

@saucepoint saucepoint commented May 14, 2024

Related Issue

Closes #554

  • define the sentinel somewhere (LPFeeLibrary?)

Description of changes

Change beforeSwap to return a uint24 representing a dynamic fee. This improves gas for swaps where the dynamic fee is updated on every swap since it does not rely on updateDynamicSwapFee (external call + sstore)

The return value is honored i.f.f. the PoolKey has a dynamic fee flag AND the value is less than or equal to the max swap fee (1 million / 100%)


The sentinel value is type(uint24).max (or anything greater than 1 million). A swap fee of 0 is valid

@saucepoint saucepoint mentioned this pull request May 14, 2024
1 task
Copy link

github-actions bot commented May 14, 2024

Forge code coverage:

File % Lines % Statements % Branches % Funcs
src/ERC6909.sol 91.30% (21/23) 85.71% (24/28) 100.00% (4/4) 85.71% (6/7)
src/ERC6909Claims.sol 100.00% (6/6) 100.00% (8/8) 100.00% (4/4) 100.00% (1/1)
src/Extsload.sol 100.00% (3/3) 100.00% (4/4) 100.00% (0/0) 100.00% (3/3)
src/Exttload.sol 100.00% (1/1) 100.00% (1/1) 100.00% (0/0) 50.00% (1/2)
src/NoDelegateCall.sol 100.00% (3/3) 100.00% (5/5) 100.00% (2/2) 100.00% (3/3)
src/PoolManager.sol 100.00% (82/82) 99.11% (111/112) 97.22% (35/36) 100.00% (18/18)
src/ProtocolFees.sol 100.00% (20/20) 97.06% (33/34) 91.67% (11/12) 100.00% (6/6)
src/libraries/BitMath.sol 100.00% (47/47) 100.00% (57/57) 100.00% (36/36) 100.00% (2/2)
src/libraries/CurrencyDelta.sol 100.00% (4/4) 100.00% (6/6) 100.00% (0/0) 100.00% (3/3)
src/libraries/CurrencySettleTake.sol 100.00% (10/10) 100.00% (10/10) 50.00% (3/6) 100.00% (2/2)
src/libraries/FullMath.sol 100.00% (29/29) 100.00% (33/33) 100.00% (8/8) 100.00% (2/2)
src/libraries/Hooks.sol 100.00% (78/78) 99.31% (143/144) 98.28% (57/58) 100.00% (15/15)
src/libraries/LPFeeLibrary.sol 100.00% (11/11) 100.00% (20/20) 100.00% (4/4) 100.00% (7/7)
src/libraries/LiquidityMath.sol 100.00% (2/2) 100.00% (1/1) 100.00% (1/1) 100.00% (1/1)
src/libraries/Lock.sol 100.00% (4/4) 100.00% (4/4) 100.00% (0/0) 100.00% (3/3)
src/libraries/NonZeroDeltaCount.sol 100.00% (6/6) 100.00% (6/6) 100.00% (0/0) 100.00% (3/3)
src/libraries/ParseBytes.sol 100.00% (3/3) 100.00% (3/3) 100.00% (0/0) 100.00% (3/3)
src/libraries/Pool.sol 99.34% (150/151) 98.17% (161/164) 94.32% (83/88) 100.00% (13/13)
src/libraries/PoolGetters.sol 66.67% (2/3) 66.67% (2/3) 100.00% (0/0) 66.67% (2/3)
src/libraries/Position.sol 100.00% (11/11) 100.00% (12/12) 100.00% (4/4) 100.00% (2/2)
src/libraries/ProtocolFeeLibrary.sol 100.00% (10/10) 100.00% (18/18) 100.00% (4/4) 100.00% (4/4)
src/libraries/Reserves.sol 100.00% (8/8) 100.00% (13/13) 100.00% (6/6) 100.00% (3/3)
src/libraries/SafeCast.sol 100.00% (10/10) 100.00% (26/26) 100.00% (10/10) 100.00% (6/6)
src/libraries/SqrtPriceMath.sol 100.00% (32/32) 100.00% (66/66) 87.50% (21/24) 100.00% (8/8)
src/libraries/StateLibrary.sol 100.00% (65/65) 100.00% (94/94) 100.00% (4/4) 100.00% (14/14)
src/libraries/SwapMath.sol 100.00% (23/23) 100.00% (30/30) 100.00% (12/12) 100.00% (1/1)
src/libraries/TickBitmap.sol 100.00% (19/19) 100.00% (34/34) 100.00% (6/6) 100.00% (3/3)
src/libraries/TickMath.sol 100.00% (94/94) 100.00% (148/148) 97.83% (45/46) 100.00% (4/4)
src/libraries/TransientStateLibrary.sol 88.89% (8/9) 85.71% (12/14) 100.00% (0/0) 75.00% (3/4)
src/libraries/UnsafeMath.sol 100.00% (1/1) 100.00% (1/1) 100.00% (0/0) 100.00% (1/1)
src/types/BalanceDelta.sol 100.00% (2/2) 100.00% (2/2) 100.00% (0/0) 100.00% (2/2)
src/types/BeforeSwapDelta.sol 100.00% (2/2) 100.00% (2/2) 100.00% (0/0) 100.00% (2/2)
src/types/Currency.sol 100.00% (15/15) 91.67% (22/24) 80.00% (8/10) 100.00% (6/6)
src/types/PoolId.sol 100.00% (1/1) 100.00% (1/1) 100.00% (0/0) 100.00% (1/1)
Total 84.23% (1373/1630) 84.40% (1893/2243) 61.71% (519/841) 77.53% (307/396)

@saucepoint
Copy link
Collaborator Author

there's an alternative implementation where key.fee.isDynamic() is not passed to callHookWithReturnDeltaAndFee (to avoid writing lpFee = type(uint24).max twice)

it's not as clean, but saves 9 gas lul

https://github.com/Uniswap/v4-core/compare/before-return-swap-fee...temp-before-return-swap-fee?expand=1

Copy link
Member

@snreynolds snreynolds left a comment

Choose a reason for hiding this comment

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

Im a bit torn on this one - it definitely makes the hook callsites more messy, and I don't actually love exposing multiple ways of doing the same thing but it is a significant gas difference if a hook does want to change the fee on every swap...

src/libraries/Hooks.sol Outdated Show resolved Hide resolved
src/libraries/Hooks.sol Outdated Show resolved Hide resolved
.forge-snapshots/swap with return dynamic fee.snap Outdated Show resolved Hide resolved
@snreynolds
Copy link
Member

snreynolds commented May 14, 2024

there's an alternative implementation where key.fee.isDynamic() is not passed to callHookWithReturnDeltaAndFee (to avoid writing lpFee = type(uint24).max twice)

it's not as clean, but saves 9 gas lul

https://github.com/Uniswap/v4-core/compare/before-return-swap-fee...temp-before-return-swap-fee?expand=1

I actually dont mind this impl now that we still have to check canReturnDelta after the callsite... if that makes sense. Like the way I suggested was cleaner before Alice's PR

Copy link
Contributor

@hensha256 hensha256 left a comment

Choose a reason for hiding this comment

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

Also torn... its making some other non-standard cases more expensive too sadly

src/libraries/Hooks.sol Outdated Show resolved Hide resolved
.forge-snapshots/swap CA custom curve + swap noop.snap Outdated Show resolved Hide resolved
@saucepoint
Copy link
Collaborator Author

I actually dont mind this impl now that we still have to check canReturnDelta after the callsite... if that makes sense. Like the way I suggested was cleaner before Alice's PR

went ahead and made the change because 9 gas 😎

* Optimise beforeSwap return fee

* add comments
* Optimise beforeSwap return fee

* add comments

* use parse return lib

* add selector, fix comments

* rename, optimize

---------

Co-authored-by: Alice Henshaw <henshawalice@gmail.com>
Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com>
src/libraries/Pool.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@hensha256 hensha256 left a comment

Choose a reason for hiding this comment

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

Few small things!

src/libraries/Pool.sol Outdated Show resolved Hide resolved
src/PoolManager.sol Show resolved Hide resolved
src/libraries/Hooks.sol Show resolved Hide resolved
src/libraries/LPFeeLibrary.sol Outdated Show resolved Hide resolved
test/DynamicReturnFees.t.sol Outdated Show resolved Hide resolved
test/DynamicReturnFees.t.sol Outdated Show resolved Hide resolved
test/DynamicReturnFees.t.sol Outdated Show resolved Hide resolved
Copy link
Member

@snreynolds snreynolds left a comment

Choose a reason for hiding this comment

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

I think its good, the one thing I don't love is that hooks default to return 0 if they dont need to use that fee field EXCEPT if you're a dynamic fee hook and you dont want to override - you shouldn't return 0, you need to return the flag. Feels footgunny but don't have a great workaround

src/libraries/Hooks.sol Outdated Show resolved Hide resolved
src/libraries/Pool.sol Outdated Show resolved Hide resolved
src/libraries/Hooks.sol Outdated Show resolved Hide resolved
uint24 public constant DYNAMIC_FEE_FLAG = 0x800000;
uint24 public constant BEFORE_SWAP_FEE_OVERRIDE_FLAG = 0x800000;
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to use the same flag - we can just name this FEE_FLAG and have it defined once

Copy link
Contributor

Choose a reason for hiding this comment

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

yeeah i did think that - just wasnt sure if the double naming makes the code more readable in other places

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, i think if we want to use one, DYNAMIC_FEE_FLAG works

used for the poolkey and for fee-overrides, which makes sense to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

consolidating to one constant, we might have 2 redundant functions:

    function isDynamicFee(uint24 self) internal pure returns (bool) {
        return self & DYNAMIC_FEE_FLAG != 0;
    }
    
    /// @notice returns true if the fee has the override flag set (top bit of the uint24)
    function isOverride(uint24 self) internal pure returns (bool) {
        return self & DYNAMIC_FEE_FLAG != 0;
    }

kind of like having isOverride, feels a lot clearer for

        // if the beforeSwap hook returned a valid fee override, use that as the LP fee, otherwise load from storage
       uint24 lpFee =
           params.lpFeeOverride.isOverride() ? params.lpFeeOverride.removeOverrideAndValidate() : slot0Start.lpFee;

src/libraries/Pool.sol Outdated Show resolved Hide resolved
if (!lpFee.isValid()) lpFee = slot0Start.lpFee;
}
uint24 lpFee =
params.lpFeeOverride.isOverride() ? params.lpFeeOverride.removeOverrideAndValidate() : slot0Start.lpFee;
Copy link
Contributor

Choose a reason for hiding this comment

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

sooo much better ❤️

hensha256
hensha256 previously approved these changes May 16, 2024
Copy link
Contributor

@hensha256 hensha256 left a comment

Choose a reason for hiding this comment

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

🚀

saucepoint and others added 3 commits May 16, 2024 23:53
Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com>
Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com>
Copy link
Contributor

@hensha256 hensha256 left a comment

Choose a reason for hiding this comment

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

lfg

@saucepoint saucepoint merged commit 975bb80 into main May 17, 2024
6 checks passed
@saucepoint saucepoint deleted the before-return-swap-fee branch May 17, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return swapFee in beforeSwap for hooks with relevant permission
3 participants