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

Update gas model #171

Merged
merged 9 commits into from
May 5, 2022
Merged

Update gas model #171

merged 9 commits into from
May 5, 2022

Conversation

MartinquaXD
Copy link
Contributor

@MartinquaXD MartinquaXD commented Apr 27, 2022

Looking into simulations of settlements with ZeroExInteractions revealed that the gas model for those is way too high. Since the approach to estimate those interactions was based off of existing estimation methods for other AMM interactions I estimated them all again with the same (hopefully more accurate) approach.

AFAIK the previous approach for gas cost estimation was to find a transactions which only had a single interaction of the type we are interested in and use that. For some interactions the min over a long period of time was used for some only a few samples were looked at. Also note that the UnwrapWethInteraction used the median value whereas every other interaction used the minimum.

The new approach is to use the ethereum.traces table which contains tracks gas_used for each function call. This is better for our purposes since it gives us more data to look at (transaction only containing the interaction vs. every transaction containing such interaction) and also discounts any gas overhead a transaction might additionally have.

The used queries are all in the form of:

select min(gas_used) as min_gas,
       avg(gas_used) as avg_gas,
       max(gas_used) as max_gas,
       percentile_cont(0.5) WITHIN GROUP (ORDER BY gas_used) as median_gas
from ethereum.traces
where "to" = '<amm_contract_address>'
    and position('<sig hash of function our settlement contract calls>' in input) = 1
    and tx_success = true
    and success = true
    and block_time > now() - interval '3 months'

I decided to NOT use the lower bound anymore and instead used the median amount for our gas cost model since that seemed more appropriate.

The changes will cause our overall gas cost estimations to decrease and will make 0x and Balancer way more competitive against UniswapV2.

Interaction Old Value New Value
UniswapV2 94_696 90_171
0x 157_300 66_358
Balancer 120_000 88_892
UnnwrapWETH 14_192 9_223

Test Plan

Manual tests simulating transactions with tenderly and using the gas profiler to compare those values against values returned by each dune query.

@MartinquaXD MartinquaXD requested a review from a team as a code owner April 27, 2022 07:24
@nlordell
Copy link
Contributor

nlordell commented Apr 27, 2022

Very nice PR!

Let me know if you think we should keep using the lower bounds for every value or maybe use the median instead of average.

The issue with using the minimum is that this includes optimal gas cases instead of the "usual". For example, specifically with WETH unwrapping, this represents the gas cost of unwrapping exactly all your WETH and not the general case of unwrapping some of it. The reason for the difference is setting some storage to 0 (so, in this case setting your WETH balance to 0) gives you a gas refund that you don't get in the general case (setting your WETH balance from some non-zero X to some non-zero Y).

The same can be said for the other interactions - it is likely to represent the gas cost of trading exactly all your tokens and not just trading some of your tokens (and I assume the latter is much more likely for settlements because of non-zero buffers held by the settlement contract).

@MartinquaXD
Copy link
Contributor Author

@nlordell can this be merged?
Since you seem to agree with the defaults I chose there is no reason to change anything, right?

@nlordell
Copy link
Contributor

nlordell commented May 4, 2022

Yes. For some reason I had misread:

I decided to use NOT use the lower bound anymore

as

I decided to use the lower bound

🙈

I think a median would be slightly more correct - but I'm not sure how easy it is to do.

@MartinquaXD
Copy link
Contributor Author

For some reason I had misread...

Extra word snuck in my sentence which made it confusing. 😅 (removed it).
I will try to compute the median and see how much that changes the amounts.

@MartinquaXD
Copy link
Contributor Author

MartinquaXD commented May 5, 2022

Updated PR and its description to reference median gas used.
I believe this change could greatly influence how our solvers route liquidity.
Could therefore maybe @marcovc or @josojo also have a look and see whether those values seem reasonable to you?
Also is there a good way to see whether those estimates more closely match the actual outcome of existing solutions?

@nlordell
Copy link
Contributor

nlordell commented May 5, 2022

Also is there a good way to see whether those estimates more closely match the actual outcome of existing solutions?

You can take a look at @vkgnosis's fee analysis tool. You can see fee coverage over the past month and compare to how it changes after implementing this change.

@marcovc
Copy link
Contributor

marcovc commented May 5, 2022

@MartinquaXD The query you use is similar to the one you were showing me the other day right? If so, I can confirm that the values match what I get by querying dex trades on dune.

@MartinquaXD MartinquaXD merged commit fd5f7cf into main May 5, 2022
@MartinquaXD MartinquaXD deleted the update-gas-model branch May 5, 2022 17:03
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants