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

gas optimizations for the FullRange hook #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atiselsts
Copy link

Related Issue

#84

Description of changes

This PR attempts to improve the FullRange hook code.

The rationale for increasing the tick spacing is described in #84.

The existing tests show some gas improvements, for most of the tests. However, the main benefits would for large swaps, something that is not captured by the existing tests. A new test testFullRange_swap_LargeSwap is added, with a gas snapshot of its own. With 60 tick spacing the gas cost is 175,523. With the new 0x7000 tick spacing it is just 150,173.

Unfortunately the large tick spacing adds some rounding errors to the liquidity distribution, so most of the existing tests were failing. I modified the tests, mostly by changing from assertEq to assertApproxEqAbs with the accuracy of DUST (30 wei).

Let me know how you feel about increasing the tick spacing, if this is contentious I can make a simple PR that just adds the immutable fields and the other minor changes.

Also I did not attempt to do the more tricky optimizations like adding unchecked to the code, so I believe there's room for further improvements.

@atiselsts atiselsts force-pushed the feature/full-range-gas-optimizations branch from 681427c to df8a14b Compare December 10, 2023 19:13
Copy link
Collaborator

@saucepoint saucepoint left a comment

Choose a reason for hiding this comment

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

I'm generally in favor of this change as it provides an example on the tradeoffs for tick space selection.

Was wondering how you came up with 0x7000?

Also -- you'll need to sign your commits and run forge fmt i believe

@atiselsts atiselsts force-pushed the feature/full-range-gas-optimizations branch from 352e233 to 56536fe Compare December 18, 2023 18:58
@atiselsts
Copy link
Author

I have signed and updated the PR @saucepoint

Some of the less common operations, like the second swap in a tx, actually got a little bit more expensive, but there are big gas savings on the first swap, and the code is simpler as well.

About why use 0x7000 as tick spacing -- good question.

I touched this a bit in issue #84. To expand further:

  • the max value supported by the v4-core is 0x7fff (max of a signed 16-bit integer)
  • however, we can save some gas in calldata by sending zero bytes, so 0x7f00 or 0x7000 are going to be slightly more efficient
  • there's not that big difference from the liquidity perspective between 0x7fff, 0x7f00, or 0x7000 ticks.
  • however, there's a big difference between 0x7000 and a more "round" number such as 0x1000 or (decimal) 10000.

The reasoning above caused me to prefer 0x7000 (28672 in decimal).

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.

3 participants