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

Save gas and clean the upper bits of computed pool address properly #291

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
5 changes: 4 additions & 1 deletion contracts/modules/Permit2Payments.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ abstract contract Permit2Payments is Payments {
internal
{
uint256 batchLength = batchDetails.length;
for (uint256 i = 0; i < batchLength; ++i) {
for (uint256 i = 0; i < batchLength;) {
if (batchDetails[i].from != owner) revert FromAddressIsNotOwner();
unchecked {
++i;
}
}
PERMIT2.transferFrom(batchDetails);
}
Expand Down
42 changes: 42 additions & 0 deletions contracts/modules/uniswap/v2/TernaryLib.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.5.0;

/// @title Library for replacing ternary operator with efficient bitwise operations
library TernaryLib {
Copy link
Member

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!

Copy link
Member

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?

Copy link
Contributor Author

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.

/// @notice Equivalent to the ternary operator: `condition ? a : b`
function ternary(bool condition, uint256 a, uint256 b) internal pure returns (uint256 res) {
Copy link
Member

Choose a reason for hiding this comment

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

not used anywhere either?

Copy link
Contributor Author

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 {
res := xor(b, mul(xor(a, b), condition))
}
}

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

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

return swapIf(b < a, a, b);
}

/// @notice Sorts two tokens to return token0 and token1
/// @param tokenA The first token to sort
/// @param tokenB The other token to sort
/// @return token0 The smaller token by address value
/// @return token1 The larger token by address value
function sortTokens(address tokenA, address tokenB) internal pure returns (address token0, address token1) {
assembly {
let diff := mul(xor(tokenA, tokenB), lt(tokenB, tokenA))
token0 := xor(tokenA, diff)
token1 := xor(tokenB, diff)
}
}

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opt for switchIf.

assembly {
let diff := mul(xor(a, b), condition)
a := xor(a, diff)
b := xor(b, diff)
}
return (a, b);
}
}
53 changes: 27 additions & 26 deletions contracts/modules/uniswap/v2/UniswapV2Library.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity >=0.8.0;

import {IUniswapV2Pair} from '@uniswap/v2-core/contracts/interfaces/IUniswapV2Pair.sol';
import {TernaryLib} from './TernaryLib.sol';

/// @title Uniswap v2 Helper Library
/// @notice Calculates the recipient address for a command
Expand All @@ -20,7 +21,7 @@ library UniswapV2Library {
pure
returns (address pair)
{
(address token0, address token1) = sortTokens(tokenA, tokenB);
(address token0, address token1) = TernaryLib.sortTokens(tokenA, tokenB);
pair = pairForPreSorted(factory, initCodeHash, token0, token1);
}

Expand All @@ -37,7 +38,7 @@ library UniswapV2Library {
returns (address pair, address token0)
{
address token1;
(token0, token1) = sortTokens(tokenA, tokenB);
(token0, token1) = TernaryLib.sortTokens(tokenA, tokenB);
pair = pairForPreSorted(factory, initCodeHash, token0, token1);
}

Expand All @@ -52,15 +53,22 @@ library UniswapV2Library {
pure
returns (address pair)
{
pair = address(
uint160(
uint256(
keccak256(
abi.encodePacked(hex'ff', factory, keccak256(abi.encodePacked(token0, token1)), initCodeHash)
)
)
)
);
assembly ("memory-safe") {
Copy link
Member

@ewilz ewilz May 8, 2023

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

// Get the free memory pointer.
let fmp := mload(0x40)
// keccak256(abi.encodePacked(token0, token1))
mstore(add(fmp, 0x14), token1)
mstore(fmp, token0)
let pairHash := keccak256(add(fmp, 0x0c), 0x28)
// abi.encodePacked(hex'ff', factory, pairHash, initCodeHash)
mstore(fmp, factory)
fmp := add(fmp, 0x0b)
mstore8(fmp, 0xff)
mstore(add(fmp, 0x15), pairHash)
mstore(add(fmp, 0x35), initCodeHash)
// Compute the CREATE2 pair address and clean the upper bits.
pair := and(keccak256(fmp, 0x55), 0xffffffffffffffffffffffffffffffffffffffff)
}
}

/// @notice Calculates the v2 address for a pair and fetches the reserves for each token
Expand All @@ -79,7 +87,7 @@ library UniswapV2Library {
address token0;
(pair, token0) = pairAndToken0For(factory, initCodeHash, tokenA, tokenB);
(uint256 reserve0, uint256 reserve1,) = IUniswapV2Pair(pair).getReserves();
(reserveA, reserveB) = tokenA == token0 ? (reserve0, reserve1) : (reserve1, reserve0);
(reserveA, reserveB) = TernaryLib.swapIf(tokenA == token0, reserve1, reserve0);
}

/// @notice Given an input asset amount returns the maximum output amount of the other asset
Expand Down Expand Up @@ -129,21 +137,14 @@ library UniswapV2Library {
{
if (path.length < 2) revert InvalidPath();
amount = amountOut;
for (uint256 i = path.length - 1; i > 0; i--) {
uint256 reserveIn;
uint256 reserveOut;
unchecked {
for (uint256 i = path.length - 1; i > 0; --i) {
uint256 reserveIn;
uint256 reserveOut;

(pair, reserveIn, reserveOut) = pairAndReservesFor(factory, initCodeHash, path[i - 1], path[i]);
amount = getAmountIn(amount, reserveIn, reserveOut);
(pair, reserveIn, reserveOut) = pairAndReservesFor(factory, initCodeHash, path[i - 1], path[i]);
amount = getAmountIn(amount, reserveIn, reserveOut);
}
}
}

/// @notice Sorts two tokens to return token0 and token1
/// @param tokenA The first token to sort
/// @param tokenB The other token to sort
/// @return token0 The smaller token by address value
/// @return token1 The larger token by address value
function sortTokens(address tokenA, address tokenB) internal pure returns (address token0, address token1) {
(token0, token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
}
}
26 changes: 15 additions & 11 deletions contracts/modules/uniswap/v2/V2SwapRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {Payments} from '../../Payments.sol';
import {Permit2Payments} from '../../Permit2Payments.sol';
import {Constants} from '../../../libraries/Constants.sol';
import {ERC20} from 'solmate/src/tokens/ERC20.sol';
import {TernaryLib} from './TernaryLib.sol';

/// @title Router for Uniswap v2 Trades
abstract contract V2SwapRouter is RouterImmutables, Permit2Payments {
Expand All @@ -20,22 +21,25 @@ abstract contract V2SwapRouter is RouterImmutables, Permit2Payments {
if (path.length < 2) revert V2InvalidPath();

// cached to save on duplicate operations
(address token0,) = UniswapV2Library.sortTokens(path[0], path[1]);
(address token0,) = TernaryLib.sortTokens(path[0], path[1]);
uint256 finalPairIndex = path.length - 1;
uint256 penultimatePairIndex = finalPairIndex - 1;
for (uint256 i; i < finalPairIndex; i++) {
(address input, address output) = (path[i], path[i + 1]);
(uint256 reserve0, uint256 reserve1,) = IUniswapV2Pair(pair).getReserves();
(uint256 reserveInput, uint256 reserveOutput) =
input == token0 ? (reserve0, reserve1) : (reserve1, reserve0);
uint256 amountInput = ERC20(input).balanceOf(pair) - reserveInput;
uint256 amountOutput = UniswapV2Library.getAmountOut(amountInput, reserveInput, reserveOutput);
(uint256 amount0Out, uint256 amount1Out) =
input == token0 ? (uint256(0), amountOutput) : (amountOutput, uint256(0));
for (uint256 i; i < finalPairIndex; ++i) {
address input = path[i];
uint256 amount0Out;
uint256 amount1Out;
{
(uint256 reserve0, uint256 reserve1,) = IUniswapV2Pair(pair).getReserves();
(uint256 reserveInput, uint256 reserveOutput) =
TernaryLib.swapIf(input == token0, reserve1, reserve0);
uint256 amountInput = ERC20(input).balanceOf(pair) - reserveInput;
uint256 amountOutput = UniswapV2Library.getAmountOut(amountInput, reserveInput, reserveOutput);
(amount0Out, amount1Out) = TernaryLib.swapIf(input == token0, amountOutput, 0);
}
address nextPair;
(nextPair, token0) = i < penultimatePairIndex
? UniswapV2Library.pairAndToken0For(
UNISWAP_V2_FACTORY, UNISWAP_V2_PAIR_INIT_CODE_HASH, output, path[i + 2]
UNISWAP_V2_FACTORY, UNISWAP_V2_PAIR_INIT_CODE_HASH, path[i + 1], path[i + 2]
)
: (recipient, address(0));
IUniswapV2Pair(pair).swap(amount0Out, amount1Out, nextPair, new bytes(0));
Expand Down
3 changes: 1 addition & 2 deletions contracts/modules/uniswap/v3/BytesLib.sol
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// SPDX-License-Identifier: GPL-3.0-or-later

/// @title Library for Bytes Manipulation
pragma solidity ^0.8.0;

import {Constants} from '../../../libraries/Constants.sol';

/// @title Library for Bytes Manipulation
library BytesLib {
error SliceOutOfBounds();

Expand Down
92 changes: 64 additions & 28 deletions contracts/modules/uniswap/v3/V3SwapRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,26 @@ abstract contract V3SwapRouter is RouterImmutables, Permit2Payments, IUniswapV3S
while (true) {
bool hasMultiplePools = path.hasMultiplePools();

// for intermediate swaps, this contract custodies
address _recipient;
assembly {
// hasMultiplePools ? address(this) : recipient
_recipient := xor(recipient, mul(xor(address(), recipient), hasMultiplePools))
}
// the outputs of prior swaps become the inputs to subsequent ones
(int256 amount0Delta, int256 amount1Delta, bool zeroForOne) = _swap(
amountIn.toInt256(),
hasMultiplePools ? address(this) : recipient, // for intermediate swaps, this contract custodies
_recipient,
path.getFirstPool(), // only the first pool is needed
payer, // for intermediate swaps, this contract custodies
true
);

amountIn = uint256(-(zeroForOne ? amount1Delta : amount0Delta));
// amountIn = uint256(-(zeroForOne ? amount1Delta : amount0Delta))
assembly {
// no need to check for underflow here as it will be caught in `toInt256()`
amountIn := sub(0, xor(amount0Delta, mul(xor(amount1Delta, amount0Delta), zeroForOne)))
}

// decide whether to continue or terminate
if (hasMultiplePools) {
Expand Down Expand Up @@ -131,7 +141,12 @@ abstract contract V3SwapRouter is RouterImmutables, Permit2Payments, IUniswapV3S
(int256 amount0Delta, int256 amount1Delta, bool zeroForOne) =
_swap(-amountOut.toInt256(), recipient, path, payer, false);

uint256 amountOutReceived = zeroForOne ? uint256(-amount1Delta) : uint256(-amount0Delta);
uint256 amountOutReceived;
assembly {
// amountOutReceived = uint256(-(zeroForOne ? amount1Delta : amount0Delta))
// no need to check for underflow
amountOutReceived := sub(0, xor(amount0Delta, mul(xor(amount1Delta, amount0Delta), zeroForOne)))
Copy link
Member

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??

Copy link
Contributor Author

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.

Copy link
Member

@ewilz ewilz May 10, 2023

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.

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 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?

}

if (amountOutReceived != amountOut) revert V3InvalidAmountOut();

Expand All @@ -144,34 +159,55 @@ abstract contract V3SwapRouter is RouterImmutables, Permit2Payments, IUniswapV3S
private
returns (int256 amount0Delta, int256 amount1Delta, bool zeroForOne)
{
(address tokenIn, uint24 fee, address tokenOut) = path.decodeFirstPool();

zeroForOne = isExactIn ? tokenIn < tokenOut : tokenOut < tokenIn;
address pool;
{
(address tokenIn, uint24 fee, address tokenOut) = path.decodeFirstPool();
pool = computePoolAddress(tokenIn, tokenOut, fee);
// When isExactIn == 1, zeroForOne = tokenIn < tokenOut = !(tokenOut < tokenIn) = 1 ^ (tokenOut < tokenIn)
// When isExactIn == 0, zeroForOne = tokenOut < tokenIn = 0 ^ (tokenOut < tokenIn)
assembly {
zeroForOne := xor(isExactIn, lt(tokenOut, tokenIn))
}
}

(amount0Delta, amount1Delta) = IUniswapV3Pool(computePoolAddress(tokenIn, tokenOut, fee)).swap(
recipient,
zeroForOne,
amount,
(zeroForOne ? MIN_SQRT_RATIO + 1 : MAX_SQRT_RATIO - 1),
abi.encode(path, payer)
);
uint160 sqrtPriceLimitX96;
// Equivalent to `sqrtPriceLimitX96 = zeroForOne ? MIN_SQRT_RATIO + 1 : MAX_SQRT_RATIO - 1`
assembly {
sqrtPriceLimitX96 :=
xor(
0xfffd8963efd1fc6a506488495d951d5263988d25, // MAX_SQRT_RATIO - 1
mul(
0xfffd8963efd1fc6a506488495d951d53639afb81, // MAX_SQRT_RATIO - 1 ^ MIN_SQRT_RATIO + 1
zeroForOne
)
)
}
(amount0Delta, amount1Delta) =
IUniswapV3Pool(pool).swap(recipient, zeroForOne, amount, sqrtPriceLimitX96, abi.encode(path, payer));
}

function computePoolAddress(address tokenA, address tokenB, uint24 fee) private view returns (address pool) {
if (tokenA > tokenB) (tokenA, tokenB) = (tokenB, tokenA);
pool = address(
uint160(
uint256(
keccak256(
abi.encodePacked(
hex'ff',
UNISWAP_V3_FACTORY,
keccak256(abi.encode(tokenA, tokenB, fee)),
UNISWAP_V3_POOL_INIT_CODE_HASH
)
)
)
)
);
address factory = UNISWAP_V3_FACTORY;
bytes32 initCodeHash = UNISWAP_V3_POOL_INIT_CODE_HASH;
assembly ("memory-safe") {
// Get the free memory pointer.
let fmp := mload(0x40)
// Hash the pool key.
// Equivalent to `if (tokenA > tokenB) (tokenA, tokenB) = (tokenB, tokenA)`
let diff := mul(xor(tokenA, tokenB), lt(tokenB, tokenA))
// poolHash = abi.encode(tokenA, tokenB, fee)
mstore(fmp, xor(tokenA, diff))
mstore(add(fmp, 0x20), xor(tokenB, diff))
mstore(add(fmp, 0x40), fee)
let poolHash := keccak256(fmp, 0x60)
// abi.encodePacked(hex'ff', factory, poolHash, initCodeHash)
mstore(fmp, factory)
fmp := add(fmp, 0x0b)
mstore8(fmp, 0xff)
mstore(add(fmp, 0x15), poolHash)
mstore(add(fmp, 0x35), initCodeHash)
// Compute the CREATE2 pool address and clean the upper bits.
pool := and(keccak256(fmp, 0x55), 0xffffffffffffffffffffffffffffffffffffffff)
}
}
}
3 changes: 0 additions & 3 deletions test/foundry-tests/UniswapV2.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ import {Constants} from '../../contracts/libraries/Constants.sol';
import {Commands} from '../../contracts/libraries/Commands.sol';
import {RouterParameters} from '../../contracts/base/RouterImmutables.sol';

import '@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol';
import '@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol';

abstract contract UniswapV2Test is Test {
address constant RECIPIENT = address(10);
uint256 constant AMOUNT = 1 ether;
Expand Down
Loading