Quoter update #349
Quoter update #349
Conversation
hensha256
commented
Sep 9, 2024
•
hensha256
commented
- huge simplification of the return parameters - now we just return the gasEstimate, and the quoted amount in unspecified
- removed the cache, its unneeded as it can just be checked within the _swap function with the parameters available
- refactored various functions into libraries for readability
- updated how the revert is caught to be more accurate with custom errors
| /// @return sqrtPriceX96After The sqrt price of the pool after the swap | ||
| /// @return initializedTicksLoaded The number of initialized ticks that the swap loaded | ||
| /// @return amountIn The input quote for the exactOut swap | ||
| /// @return gasEstimate Estimated gas units used for the swap | ||
| function quoteExactOutputSingle(QuoteExactSingleParams memory params) |
There was a problem hiding this comment.
does it make sense to add batched functions so you can get many quotes in one call?
There was a problem hiding this comment.
they use an external multicall contract for multicalling quoters onchain
on mainnet its here
|
|
||
| import {ParseBytes} from "@uniswap/v4-core/src/libraries/ParseBytes.sol"; | ||
|
|
||
| library QuoterRevert { |
There was a problem hiding this comment.
I find this library a little difficult to reason about because all these functions seem to be doing some of the same things but at various lifecycles of a quote (various parsing, and various reverting) but its unclear when to use what.
some suggestions??
- does
validateRevertReasonneed to be its own function? Seems like its just used in this file. Can you collapse it? - for all of the reverting code paths can we decide if we want to use the revert logic in assembly or not?
revertQuoteandbubbleReasonare not consistent with the reverting info. - can you add documentation for whats the difference between those two functions? its unclear.. it seems like
revertQuotecould just usebubbleReason? - do we need `parseAmountUnspecified as its own function as well? it seems like its just used in parseReturnData? Maybe we can change parseReturnData -> parseAmountUnspecified to be more clear since it doesnt parse any other kind of data?
src/base/BaseV4Quoter.sol
Outdated
| // Call this contract with the data in question. Each quote path | ||
| (bool success, bytes memory returnData) = address(this).call(data); | ||
| if (success) return returnData; | ||
| if (returnData.length == 0) revert LockFailure(); |
There was a problem hiding this comment.
not sure I understand why this is necessarily a lock failure?
| assertEq(sqrtPriceX96AfterList[0], 79708304437530892332449657932); | ||
| assertEq(initializedTicksLoadedList.length, 1); | ||
| assertEq(initializedTicksLoadedList[0], 2); | ||
| assertEq(amountIn, 6283); |
There was a problem hiding this comment.
assert gas is > 0? I know it's hard to get accurate on the gas but we know it should at least not be 0
There was a problem hiding this comment.
I've done asserts 50k < gas < 400k
| function _unlockCallback(bytes calldata data) internal override returns (bytes memory) { | ||
| (bool success, bytes memory returnData) = address(this).call(data); | ||
| // Every quote path gathers a quote, and then reverts either with QuoteSwap(quoteAmount) or alternative error | ||
| if (success) revert UnexpectedCallSuccess(); |
There was a problem hiding this comment.
Just starting to look through this but my first thought here.. is I'm wondering if its even possible that this returns true? Like could you write a test that hits this revert?
| // reason -> reason+0x1f is the length of the reason string | ||
| // reason+0x20 -> reason+0x23 is the selector of QuoteSwap | ||
| // reason+0x24 -> reason+0x43 is the amountUnspecified | ||
| // reason+0x24 -> reason+0x43 is the quoteAmount |
There was a problem hiding this comment.
I guess this is more of a comment to all of this natspec but this is kinda confusing.
0x20 -> 0x23 is a 3 byte difference?
0x24 -> 0x43 is a 31 byte difference?
Is something not inclusive?
| uint160[] memory sqrtPriceX96AfterList, | ||
| uint32[] memory initializedTicksLoadedList | ||
| ); | ||
| returns (uint256 amountIn, uint256 gasEstimate); |
There was a problem hiding this comment.
Ok I think we should actually return the BalanceDelta, will follow up async w you
|
|
||
| import {ParseBytes} from "@uniswap/v4-core/src/libraries/ParseBytes.sol"; | ||
|
|
||
| library QuoterRevert { |
|
Looks like gas snapshots just need to be updated |
