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

Revert using external prices for conversion #2891

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Aug 14, 2024

Description

Reverts #2863

Failing e2e test showed that in cases when price improvements are significant during auction run, using external prices (that were attached to Auction at the auction building time) instead of uniform clearing prices can be very inaccurate.

Closes #2874

How to test

Existing tests.

@sunce86 sunce86 requested a review from a team as a code owner August 14, 2024 11:07
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.

using external prices instead of uniform clearing prices can be very inaccurate.

I disagree with that premise, this really just depends on your point of view. We are assuming that the clearing prices are more correct than the external prices, but I don't think this is a safe assumption (solvers can chose whatever clearing price they want as long as it is "good for the user").

I still think that using predictable, protocol authored prices that are the same for all solutions is the more correct approach (we may have to make our e2e test more accurate in this case)

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 14, 2024

If we have exchange rate between tokens A and B in external prices 1:1, and then, due to significant price change, solver returns uniform clearing prices of 1:2 (which is actually true based on onchain liquidity), then our calculation of fee in sell token would be way off (2 times higher then actual). This practically means that, if solvers are honest and estimate their fee to be equal to gas costs, we would show twice as higher gas cost on UI. How do we explain this to users and solvers?
Yes, solvers can put whatever in uniform clearing prices, but they are honest for now and we are showing gas costs on UI for now. So either we show correct values, or we assume solver can put whatever in UCP making UCP irrelevant for us so we drop the solver fee (gas cost) as an entity completely IMO.

So, I guess the point here is that external prices are always somewhat outdated since the blockchain state changes between auction build and solving.

@fleupold
Copy link
Contributor

Yes, solvers can put whatever in uniform clearing prices, but they are honest for now

This is a terrible argument imo. Even if they are not being malicious, who says that the the solver price is actually the correct one? If the external prices are coming from Coingecko isn't it more likely that Coingecko is correct and the solver had a glitch that gave the user a way too good of a deal (we have seen those terrible slippage settlements in the past)? How often does the Coingecko price outdate significantly between auction cutting and solution submission?

I think in most of the cases it doesn't make a difference and when it does I would rather trust the prices which are also used to normalize surplus than arbitrary prices an external party chose. If the user sees slightly off fee estimates in the UI (which again they might as well if we use UCP as there is no guarantee this one is correct), it's not the end of the world.

cc @fhenneke maybe you have an educated opinion here.

@fleupold
Copy link
Contributor

Another argument is that UCP includes price impact, whereas the external price vector doesn't, which makes it more "correct" to convert small amount such as the network fee.

@MartinquaXD
Copy link
Contributor

I agree with Felix here. The external prices are a fundamental part of the system at the moment and replacing their usage has to be thought about thoroughly. If we are worried about inaccurate external prices IMO we should try to improve their accuracy instead of trusting the solvers on this.
Improving the accuracy of those prices should also be better for the competition as a whole since what solvers optimize for then better matches the reality.

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 15, 2024

I do agree that we should use external prices and we do use them for converting surplus and protocol fees into Ether, however, using external prices to reconstruct what solvers estimated as a fee in sell token is less appropriate.

Fair enough, I will proceed with using external prices and try to fix the failing test somehow.

@sunce86 sunce86 closed this Aug 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2024
@fhenneke
Copy link

fhenneke commented Aug 15, 2024

I prepared a wall of text and now it is closed 😭

So here it comes. (tldr: using external prices is wrong)

Until recent changes I would not have trusted external prices over clearing prices. But we have not seen external prices being much off recently and the typical 1% or 2% do not matter for that the ui use case. Even larger errors do not matter. @shubhagarwal03 had a look at the price deviations recently.

I need to add here that surplus fees are a bit of a underdefined concept. Or at least there are different qunatities one can extract from it and solvers can choose to implement them how they like.

One possible definition:

  • Reduction in surplus (measured as an amount in the surplus token) between what happened on chain compared to what would have happened if uniform clearing prices would have been used.

That definition would already depend on uniform clearing prices and the amount would be in the surplus token.

One solver team use case:

  • Reconstruct what network fee the solver reported for slippage. This is an amount in the sell token. The reconstruction is possible for the reference driver. That driver combines the solver reported fee with a uniform clearing price to end up with a new price vector. (This is then modified again because of protocol fees but lets ignore that for now.) These two price vectors can be used to compute a fee in the surplus token. Knowing the driver implementation, the actual fee in the sell token can be reconstructed using uniform clearing prices. Using external prices gives something similar in value but not the exact thing solvers reported. In this case it is not about which price is more accurate but about reversing how the driver incorporated fees in the sell token into price vectors.

Currently, the autopilot does two steps at the same time: compute an amount in the surplus token and convert it to the sell token. If the second step is done using uniform clearing prices, it exactly recovers what solvers reported. If we add protocol fees to the problem, it actually becomes more convenient for the solver team use case to do no conversion at all and just store an amount in the surplus token.

With the reference driver, solver can misreport clearing prices only if they misreport fees as well. So reconstructing what the solver reported always should use uniform clearing prices.

With external drivers, the concept of network fees derived from price vectors is a bit obsolete. Forcing external drivers to use the same convention for changing price vectors based on fees charged in the sell token does not sound like a good idea. But then recovering network fees does not work anymore or which prices should be used is unclear.

I cannot say much about the UI use case. I would assume that exact values are not as important there. Insead, properties like network fees not being negative or being somewhat close to execution costs would be required. Both of those properties are not guaranteed for external drivers at the moment. For reference drivers, solvers can still report whatever fee they want. So at least the connection to execution costs cannot be guaranteed. Showing users something like "protocol fees converted using whatever prices" + "execution costs divided by number of orders converted using whatever prices" might be a valid alternative. Both do not require a surplus fee value at all.

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 15, 2024

@fhenneke Nice overall writeup. Thanks.

What is super weird if we don't merge this PR is that, we first calculate the fee as a difference between custom prices and uniform clearing prices (so it's in surplus token), and then, if we want to convert back to sell token, we use external prices instead of the same uniform clearing prices we already used to determine the fee (in surplus token) in the first place.

I'd say, long term, with external drivers, we have only two options for network fees:

  1. Ask solvers how much they spent on it and trust this (meaning trust UCP)
  2. Try to determine it ourselves from the onchain data (somehow split the gas used between orders in a settlement) and express it in Ether

Regarding protocol fees, we found the way to reconstruct them from onchain data solely (in surplus token), so presenting them in surplus token would be the most correct way.

@fleupold
Copy link
Contributor

I prepared a wall of text and now it is closed 😭

Reopening is just a click away. I think the argument that the fees are deducting using uniform clearing prices in the first place is a good one that I didn't see before. Let's revert this change then.

Regarding the protocol fee token currency, maybe @cowprotocol/frontend can weigh in here if you would be able to convert/display the protocol fee in the surplus token instead of the sell token (relevant for sell orders).

@fleupold fleupold reopened this Aug 15, 2024
@sunce86 sunce86 enabled auto-merge (squash) August 15, 2024 11:29
@sunce86 sunce86 merged commit 82d6346 into main Aug 15, 2024
10 checks passed
@sunce86 sunce86 deleted the revert-uniform-prices-conversion branch August 15, 2024 11:43
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.

6 participants