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

Add optional quoteId parameter to orders #170

Closed
nlordell opened this issue Apr 26, 2022 · 5 comments
Closed

Add optional quoteId parameter to orders #170

nlordell opened this issue Apr 26, 2022 · 5 comments

Comments

@nlordell
Copy link
Contributor

Orders that were created with quotes can optionally include a quote ID. This way the order can be linked to a quote and enable providing more metadata when analyzing order slippage.

@nlordell
Copy link
Contributor Author

Just wanted to add some more context from #231 (review)

Specifically, the plan is to eventually store quotes in the orderbook database (similar to how we store fee measurement - or even replacing the fee measurements). When orders are created with a quoteId, we could then

Additionally, as @MartinquaXD, if we store enough quote information, we could even "guess" what quote was used to create the order (i.e. the most recent quote that matches the order parameters) and use that instead of the explicitly specified one. I would still keep the explicit parameter, in case a client does multiple quotes (on different threads for example) so that the "real" quote that was used can be specified.

ahhda added a commit that referenced this issue May 30, 2022
Partially fulfills #170

This PR adds the ability to set the quoteId parameter while creating Orders. This way the order can be linked to a quote and enable providing more metadata when analyzing order slippage.

Currently, it doesn't involve interaction with the Database i.e. Storing or retrieving the quoteId. This will be a part of a separate PR.

Also api/v1/quote now also returns an ID. Currently, implemented as a counter which gets reset on orderbook restarts.

Users can now pass an optional parameter quoteId while creating orders for easier debugging.
@nlordell
Copy link
Contributor Author

One thing that is not obvious to me is how stored quotes interact with stored min-fee measurements.

Specifically, we currently keep track of min-fee measurements per (token, amount) pair and reuse "live" min-fees for new quotes as long as they have not expired.

Since we also want to store quotes (which includes updated prices), the question becomes:

  1. Do we always compute new min-fees on new quotes? i.e. does the fee update with the price?
  2. Do we track min-fee measurements and quotes separately, i.e. price updates do not necessarily update the quoted fee

I'd be inclined to say we should do 1. just because price updates may cause non-trivial changes to the fee (as changes in routes may lead to a better price at a higher fee).

@vkgnosis
Copy link
Contributor

The reason we originally started keeping track of fee estimates is that we want to give users some grace period during which their original quote will be accepted even if the fee amount moves (because price or gas price changes). If we didn't do this then with some probability users would be unable to submit their order after getting the quote.
Since we were already storing the fee it made sense to also reuse it for new quotes because orders for the same (token, amount) were going to be evaluated against the same set of fee measurements anyway so we might as well take load of the price estimators and the database.
(Later we decided to keep fee measurements forever in the database for debugging.)

The fee estimator needs the following data: gas price, price of the sell token in eth, price estimate of the user's trade (for gas amount). The first two are cheap and already cached. For the latter I notice now that this is exactly the same price estimate that we also create in the quote endpoint:

let buy_token_query = price_estimation::Query {

let query = price_estimation::Query {

Because we have deprecated the other routes, we are only going to query the fee estimator through the quote endpoint. We should share this data between the components. Reduces load on price estimators, reduces latency of the endpoint.

This is relevant evidence for your question. If we are confident that our api will remain this way then it likely makes sense to combine the quoting and fee estimation into one component which is related to what you wrote before "similar to how we store fee measurement - or even replacing the fee measurements".

We can turn your last question around too and ask whether quotes should be cached for a short time just like fee estimates because prices do not change significantly in 30s. This would reduce load from price estimators and the database. From the top of my head this makes sense to me.

At the beginning I wrote that we want to accept orders created with recent fee estimates even if a fresh fee estimate might be higher. This is true but I can also see that it makes to give new quotes the fresh estimate even if we would accept the older one. Technically they could game the system but it is unlikely to happen on a large scale. This is an argument in favor of 1. The downside is more database load.

This isn't exactly a conclusion on 1 vs 2 but it shows some more relevant information and maybe alternative solutions that haven't been named yet.

@nlordell
Copy link
Contributor Author

We can turn your last question around too and ask whether quotes should be cached for a short time just like fee estimates because prices do not change significantly in 30s. This would reduce load from price estimators and the database.

I think the more general question is whether or not we want to cache quotes for the same amount of time as fees. In this "frame of reference", we currently cache quotes for 0s and fees for 30s.

Technically they could game the system but it is unlikely to happen on a large scale.

We can make more recent quotes replace the old ones to mitigate this if it becomes a problem.

Copy link

github-actions bot commented Jun 5, 2024

This issue has been marked as stale because it has been inactive a while. Please update this issue or it will be automatically closed.

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

No branches or pull requests

2 participants