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

Use external prices for conversion of fees in autopilot #2863

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Aug 6, 2024

Description

Related to discussion in the issue: #2862 (comment)

Use external price vector to convert "fee in surplus token" into "fee in sell token" (instead of uniform clearing prices).

This is important so that we make sure that it's always true:

total_fee = executed_surplus_fee + protocol fees

If we use uniform clearing prices for executed_surplus_fee and external prices for protocol fees I think above equation won't stand.

How to test

Existing unit test

@sunce86 sunce86 requested a review from a team as a code owner August 6, 2024 12:36
@sunce86 sunce86 mentioned this pull request Aug 6, 2024
3 tasks
assert_eq!(
solution.native_fee(&auction.prices).0,
eth::U256::from(6890975030480504u128)
eth::U256::from(6752697350740628u128)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we see that for this specific example, difference is ~2% if we use external prices vector for conversion. Seems acceptable.

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 6, 2024

@fhenneke FYI this change affects:
settlement_observation::fee
order_execution::executed_surplus_fee

I don't expect this to have any significant effect on solver scripts.

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

We already trust external price vectors for the surplus computation, so I think it make sense to use them for fee computation as well.

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

LGTM

@sunce86 sunce86 enabled auto-merge (squash) August 7, 2024 06:08
@sunce86 sunce86 merged commit 01b9761 into main Aug 7, 2024
10 checks passed
@sunce86 sunce86 deleted the conversion-to-sell-token branch August 7, 2024 06:14
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2024
@fhenneke
Copy link

fhenneke commented Aug 7, 2024

This will probably impact slippage and/or solver rewards.

If we want to reconstruct network fees (in sell token and in ETH) given protocol fees (in surplus token) and the total fee (in ETH), would we need external prices for all buy and sell tokens?

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 7, 2024

If we want to reconstruct network fees (in sell token and in ETH) given protocol fees (in surplus token) and the total fee (in ETH), would we need external prices for all buy and sell tokens?

Not sure what's the context of the question but given the protocol fees in surplus token, you need at least external price of buy token if you want to convert protocol fee to ETH for sell orders.
And also, if you want to convert protocol fee to sell token for sell orders you also need external price for both sell and buy token.
So I think the answer is yes.

@fleupold
Copy link
Contributor

fleupold commented Aug 7, 2024

But this is not an issue, is it? The autopilot filters out orders for which we cannot determine the price of the buy or sell token (which is required for evaluating surplus in ETH)

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 7, 2024

I think we don't give strong guarantees for COW AMM orders:

  1. We try to get all prices but if some are missing we are not filtering cow amm order owners for those that are missing (this is a bug maybe?).
  2. Reference driver would filter out solutions that miss external price but external driver who knows
  3. Autopilot does not check if all orders in the /solve response have prices (at the competition time).

This can be resolved with #2720

@fhenneke
Copy link

fhenneke commented Aug 7, 2024

The slippage query will have to be adapted (it uses surplus_fee for exactly computing network fees) and we might need to upload additional data to dune. At the moment, external prices are uploaded for protocol fee tokens only. With this PR we need it for sell and buy tokens.

The midterm plan is to compute slippage off-chain and then it does not make as much of a difference.

@fleupold
Copy link
Contributor

fleupold commented Aug 7, 2024

(this is a bug maybe?).

yes, if we cannot compute native prices the AMM should not tradable because we won't be able to compute surplus. That's a bug.

sunce86 added a commit that referenced this pull request Aug 9, 2024
# Description
Fixes
#2863 (comment)

Do not send cow amm order owner to driver if auction prices don't
contain prices for that cow amm traded tokens.
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.

5 participants