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

Implement VIPs for MakerPSM -- PRO-115 #46

Merged
merged 13 commits into from
Dec 27, 2023
Merged

Implement VIPs for MakerPSM -- PRO-115 #46

merged 13 commits into from
Dec 27, 2023

Conversation

duncancmt
Copy link
Collaborator

@duncancmt duncancmt commented Oct 23, 2023

This PR pushed Settler over the contract size. To fix that, this PR removes support for the ZeroEx (protocol v4) VIP. Limit orders are supported by BASIC_SELL wrapping 0xV4 OTC. There's also a significant amount of golfing performed on the UniV2 and UniV3 VIPs

@linear
Copy link

linear bot commented Oct 23, 2023

PRO-115 MakerPSM VIP

@duncancmt
Copy link
Collaborator Author

@dekz now that you're back, what do you think of removing the ZeroEx VIP? it can be served through BASIC_SELL. it would make my life easier because that removes ~2.8K of contract

Comment on lines -77 to +85
| Settler | 0x V4 | USDC/WETH | 174977 | 55.14% |
| Settler | 0x V4 | USDC/WETH | 202822 | 79.83% |
| | | | | |
| 0x V4 | 0x V4 | DAI/WETH | 93311 | 0.00% |
| Settler | Settler | DAI/WETH | 94258 | 1.01% |
| Settler | 0x V4 | DAI/WETH | 145780 | 56.23% |
| Settler | 0x V4 | DAI/WETH | 172912 | 85.31% |
| | | | | |
| 0x V4 | 0x V4 | USDT/WETH | 104423 | 0.00% |
| Settler | Settler | USDT/WETH | 105370 | 0.91% |
| Settler | 0x V4 | USDT/WETH | 160683 | 53.88% |
| Settler | 0x V4 | USDT/WETH | 188244 | 80.27% |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

previous versions of this comparison weren't accurate because they failed to actually transfer the buy token to the taker. this PR rectifies that oversight in the unit test, massively increasing the gas consumed

Comment on lines -58 to +56
let swapCalldata := ptr
let swapCalldata := add(ptr, 0x1c)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A lot of the golfing going on here concerns left-padding (right-aligning) constants instead of right-padding (left-aligning) them. We're able to do that without pessimizing gas consumption and it makes the contract significantly smaller.

Comment on lines +68 to +69
let path := add(encodedPath, 0x20)
let end := add(add(encodedPath, mload(encodedPath)), 0x0c)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only place where gas is pessimized in this changeset. This incurs a small gas penalty for UniV2 single-hop with the benefit of significantly reducing gas consumption for multi-hop

}
default { revert(0, 0) }
let toPool := keccak256(ptr, 85)
let toPool := and(0xffffffffffffffffffffffffffffffffffffffff, keccak256(add(ptr, 0x0b), 0x55))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have to clean this variable for multihop because we store it in memory and to form the calldata for the call to swap(...) or transfer(...). It's probably worth checking both Vyper and Solidity's behavior when there are dirty bits in calldata

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems that at least Solidity 0.5.16 silently cleans dirty bits in calldata

if (!(zeroForOne = token0 < token1)) {
(token0, token1) = (token1, token0);
}
pool = _toPool(token0, fee, token1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_toPool now requires token0 < token1, except this isn't checked. this not only optimizes runtime gas, but also contract size

Comment on lines +271 to +274
mstore(add(swapCallbackData, 0x3f), payer)
mstore(add(swapCallbackData, 0x2b), token1)
mstore(add(swapCallbackData, 0x17), fee)
mstore(add(swapCallbackData, 0x14), token0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These constants are kinda brittle, but packing the callback data like this does mean that we don't have to do any cleaning. This is effectively setting the first 63 bytes of swapCallbackData to abi.encodePacked(token0, fee, token1, payer)

Comment on lines +318 to +326
assembly ("memory-safe") {
{
let firstWord := calldataload(data.offset)
token0 := shr(0x60, firstWord)
fee := shr(0x48, firstWord)
}
token1 := calldataload(add(data.offset, 0xb))
payer := calldataload(add(data.offset, 0x1f))
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is both more gas efficient and less contract size than the previous iteration. This goes along with the changes to _updateSwapCallbackData to use a packed encoding instead of an ABI encoding

@@ -249,6 +290,29 @@ abstract contract SettlerPairTest is BasePairTest {
snapEnd();
}

function testSettler_uniswapV2_multihop() public {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding integration test for multihop really shows the benefit of custody optimization. It also exercises some previously untested codepaths

@duncancmt duncancmt marked this pull request as ready for review October 30, 2023 22:49
@duncancmt duncancmt merged commit c418977 into master Dec 27, 2023
7 of 8 checks passed
@duncancmt duncancmt deleted the makerpsm branch May 16, 2024 00:59
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.

1 participant