-
Notifications
You must be signed in to change notification settings - Fork 201
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
Save gas and clean the upper bits of computed pool address properly #291
base: main
Are you sure you want to change the base?
Conversation
saving 100~200 gas
…orPreSorted` and clean the upper bits explicitly saving 500~1000 gas
…lean the upper bits at the same gas
…tion since there weren't V3 Foundry tests Rewritten `computePoolAddress` saves ~500 gas
|
||
/// @notice Sorts two uint256 in ascending order | ||
/// @dev Equivalent to: `a < b ? (a, b) : (b, a)` | ||
function sort2(uint256 a, uint256 b) internal pure returns (uint256, uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
|
||
/// @notice Swaps two uint256 if `condition` is true | ||
/// @dev Equivalent to: `condition ? (b, a) : (a, b)` | ||
function swapIf(bool condition, uint256 a, uint256 b) internal pure returns (uint256, uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is a swap router, swap can easily get confused here. maybe reverseOrderIf
/ reverseIf
/ switchIf
/ switchOrderIf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opt for switchIf
.
pragma solidity >=0.5.0; | ||
|
||
/// @title Library for replacing ternary operator with efficient bitwise operations | ||
library TernaryLib { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slick ternary ops in here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One function is a ternary, while the other is not. I wonder if SortLib
would be more readable and make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only sortTokens
is used for sorting. But the rest of the library is meant for replacement of the built-in ternary operator.
/// @title Library for replacing ternary operator with efficient bitwise operations | ||
library TernaryLib { | ||
/// @notice Equivalent to the ternary operator: `condition ? a : b` | ||
function ternary(bool condition, uint256 a, uint256 b) internal pure returns (uint256 res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used anywhere either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was more of an illustration. Now that overloaded versions of ternary()
on int256
and address
are used in V3SwapRouter
, should we leave this one here for completeness?
) | ||
) | ||
); | ||
assembly ("memory-safe") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like the gas savings here. Wonder if it's worth a comment:
// accomplishes the following:
// address(keccak256(abi.encodePacked(hex'ff', factory, keccak256(abi.encodePacked(token0, token1)), initCodeHash)))
to get a feel for what this is doing at a glance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I think that's pretty helpful for readability, one for the V3Lib would probably make sense too. (sorry, missed that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
assembly { | ||
// amountOutReceived = uint256(-(zeroForOne ? amount1Delta : amount0Delta)) | ||
// no need to check for underflow | ||
amountOutReceived := sub(0, xor(amount0Delta, mul(xor(amount1Delta, amount0Delta), zeroForOne))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could these ternaries in this file get abstracted into the Library with all the xors??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually yes. I was afraid the optimizer wouldn't inline properly. But it seems the gas level remains the same as long as we uncheck the unary -
on int256
unchecked {
uint256 amountOutReceived = uint256(-zeroForOne.ternary(amount1Delta, amount0Delta));
if (amountOutReceived != amountOut) revert V3InvalidAmountOut();
}
For
hasMultiplePools ? address(this) : recipient
using hasMultiplePools.ternary(address(this), recipient)
in place also saves gas due to less stack shuffling.
For zeroForOne := xor(isExactIn, lt(tokenOut, tokenIn))
though, using
zeroForOne = isExactIn.ternary(tokenIn < tokenOut, tokenOut < tokenIn);
wouldn't be cheaper. And writing zeroForOne = isExactIn ^ (tokenOut < tokenIn)
isn't allowed since "Operator ^ not compatible with types bool and bool.".
Writing
sqrtPriceLimitX96 = zeroForOne.ternary(MIN_SQRT_RATIO + 1, MAX_SQRT_RATIO - 1);
instead of xor
and literal hexes in assembly also wouldn't be cheaper. We could define the literal hexes as contract level constants though if that improves readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool thanks for the explanation. We definitely strive to strike a balance between readability and gas savings (or else we could just write low-level code for everything :)
Even with our steepest gas savings tests, these ternaries that cannot be abstracted away would only save < 100 extra gas per full swap, So I'd personally vote to just omit them. Can always get a third opinion..
Also some context, our v2 UniversalRouter has already been audited so we cannot include this, these gas savings would go into V3 of the router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always abstract them away. It will just costs a few more opcodes. I'm okay with replacing sqrtPriceLimitX96
done in assembly with literal hexes with zeroForOne.ternary(MIN_SQRT_RATIO + 1, MAX_SQRT_RATIO - 1)
. But frankly speaking, is
zeroForOne = isExactIn ? tokenIn < tokenOut : tokenOut < tokenIn;
more understandable and cleaner than
assembly {
zeroForOne := xor(isExactIn, lt(tokenOut, tokenIn))
}
with derivations?
Refactor
_v2Swap
to resolve stack too deep errorPreviously
UniversalRouter
can only be compiled via the IR pipeline due to the "stack too deep" error which takes a long time especially in Foundry. After refactoring_v2Swap
and modifyingSignatureTransfer.permitWitnessTransferFrom
toit not only saves gas but the project can also be compiled with
via_ir = false
which allows for faster compilation and testingCompute v2 and v3 pool address using inline assembly and explicitly clean the upper bits
Previously the pool address was computed in pure Solidity and casted via
address(uint160(uint256()))
. However the upper 12 bytes are not explicitly cleaned and the address is later used insolmate::SafeTransferLib.safeTransfer
which is written in inline assembly. When compiled withvia_ir
enabled, all tests pass. However, after making the aforementioned changes, some Foundry tests failed in aERC20.transfer
withvia_ir
disabled. It was discovered that the dirty upper bits of pool address are the culprit, but somehow the IR pipeline may clean the address afterkeccak256
. Nonetheless,pairForPreSorted
andcomputePoolAddress
are rewritten in inline assembly to save gas and clean the upper bits. Closes #290.Add Uniswap v3 Foundry tests for more granular gas comparison
There weren't Uniswap v3 tests in Foundry. In order to validate gas optimizations to be made, test contracts
UniswapV3Test
andV3MockMock
have been added.forge snapshot --diff
is run after each modification to validate the gas savings.Replace conditional statements with bitwise operations
The current Solidity optimizer isn't smart enough to reduce ternary expressions to fewer opcodes and likely translate them to
JUMPI
. ThereforeTernaryLib
utils written in inline assembly have been added to replace ternary expressions as much as possible.sortTokens
is also refactored toTernaryLib
but inlined where appropriate.In general for
x = y ? a : b
, wheny = true
When
y = false
Therefore
x = y ? a : b
is equivalent tox = b ^ (b ^ a) * y
according to the properties ofxor
.