gnosis / dex-services

Off-chain services for the Gnosis Protocol v1.
33 stars 9 forks source link

[Price Estimation] Make price estimation rounding tighter #970

Closed fleupold closed 4 years ago

fleupold commented 4 years ago

Original issue: https://github.com/gnosis/dex-price-estimator/issues/52

Looking into other stablecoin swaps I noticed that spreads are extremely competitive. E.g., at the moment our price estimator offers 0.99690 USDC/DAI while mStable offers marginally better 0.997.

However when actually doing a trade we end up achieving a better price of 0.9979. While this is a great "surprise" for the user it would be nice if we could make our price estimation "tighter", as people might not even attempt to trade otherwise. At the moment we just add another 0.1% rounding buffer on top.

fleupold commented 4 years ago

Summary of the discussion with @marcovc and @twalth3r to explain the current solver rounding:

For now, the goal would be to imitate this rounding behaviour for the price estimate.

We can use the price estimated by the price graph to approximate the rounding buffer in a similar way, however there are two issues with that:

  1. Prices for esoteric tokens may have a significantly different price on GP compared to Kraken/1Inch (our current price sources)
  2. The price at the time of estimation might not be the same as the price at the time of solver invocation.

Point 2.) can be addressed by multiplying the price itself with a reasonable buffer (expected maximal swing in the next n minutes) Point 1.) could be addresses by using the same averaged price source (querying kraken and dex.ag) in the price estimator when computing the rounding buffer.

fleupold commented 4 years ago

Pulling in the external price sources has the added disadvantage that it makes replaying price quotes in the past harder (price estimation is no longer a pure function given the orderbook) - cf. #967. However this is likely going to become more challenging anyways as we start adding "virtual orders" (thus any solution applied there could also be applied to the external price estimations)

twalth3r commented 4 years ago

Here are the rounding buffers that we would have applied for the orders on one of the most recent instances (token pairs stated as sell-token / buy-token).

Looking at DAI, for example, we see these:

DAI   / OWL   --        7571059313617 [DAI]
DAI   / WETH  --        7571059327207 [DAI]
DAI   / USDT  --       99808298974675 [DAI]
DAI   / TUSD  --        7571059313707 [DAI]
DAI   / USDC  --       99794155345060 [DAI]
DAI   / PAX   --        7571059313686 [DAI]
DAI   / GUSD  --   998905893380443552 [DAI]
DAI   / SETH  --        7571059337023 [DAI]
DAI   / SUSD  --        7571059313669 [DAI]
DAI   / SBTC  --        7571059380805 [DAI]
DAI   / WBTC  --     9616576426505494 [DAI]
DAI   / CDAI  --        7588086814677 [DAI]
DAI   / SNX   --        7571059313708 [DAI]
DAI   / CHAI  --        7571059313714 [DAI]
DAI   / GNO   --        7571059315824 [DAI]

This means that for DAI vs. USDT, we deduct ~10*14, which is 0.0001 real-world-DAI (so pretty small). For DAI vs. GUSD, however, this becomes 10*18, so 1 real-world-DAI.

fleupold commented 4 years ago

So our current price estimator would actually suggest unfillable prices when I try to sell 100DAI for GUSD (0.1% of 100DAI << 1DAI).

Another reason why using a fixed percentage (even if we could tighten it for most tokens) is not a good idea.

e00E commented 4 years ago

@marcovc @twalth3r While comparing my implementation to the solver I ran into something I don't understand. I've implemented the code from Rounding.py. There I pass in the the price estimations that we get from external sources which we also give to the solver in the instance file. When I look at the solver output I see that it is not actually using the passed price estimates directly. They are transformed in PriceEstimation.py. I don't know exactly what is happening there but in my instance the prices end up different from the estimations. Looking at that code this seems intended because why else would we need the code in that file. The problem is that this means that the estimated the solver uses in Rounding.py are different than the estimated prices we would use in the price-estimator.

python -m src._run @src/configs/run_single.cfg --useExternalPrices
{
  "tokens": {
    "T0000": {
      "alias": "T0",
      "decimals": 18,
      "externalPrice": 1e+18
    },
    "T0001": {
      "alias": "T1",
      "decimals": 18,
      "externalPrice": 2e+18
    },
    "T0002": {
      "alias": "T2",
      "decimals": 18,
      "externalPrice": 3e+18
    }
  },
  "refToken": "T0000",
  "accounts": {
    "0x0": {
      "T0000": "3000000000000000000000",
      "T0001": "3000000000000000000000",
      "T0002": "3000000000000000000000"
    }
  },
  "orders": [
    {
      "accountID": "0x0",
      "sellToken": "T0001",
      "buyToken": "T0002",
      "sellAmount": "1e10",
      "buyAmount": "1e10",
      "orderID": 0
    }
  ],
  "fee": {
    "token": "T0000",
    "ratio": 0.001
  }
}

Note that I've commented out everything before PriceEstimation.estimate_token_prices in OptModelBase.py - def preprocess(self) and added exit() after Rounding.apply_rounding_buffer(self.batch_auction). In the output I see

[INFO: PriceEstimation.py:233 | regress_prices_from_xrates()]  Estimated token prices [using external prices = True]
[INFO: PriceEstimation.py:240 | regress_prices_from_xrates()]  Estimated price for [T0000] :                      1000000000000000000
[INFO: PriceEstimation.py:240 | regress_prices_from_xrates()]  Estimated price for [T0001] :                      6666666666666665035
[INFO: PriceEstimation.py:240 | regress_prices_from_xrates()]  Estimated price for [T0002] :                     10000000000000000000
marcovc commented 4 years ago

Currently, exchange rates between tokens that have external prices and are not the fee token are fixed to the ratio of their external prices. This is why the exchange rate between T0001/T0002 is correct (6/10 = 2/3). This is the part you were expecting.

Unfortunately, the exchange rate between the fee token and any other token for which we have an external price is not fixed to the ratio of the external prices. This was implemented recently, to account for the fact that, in reality, there is no external price for OWL.

Given that the prices of T0001 and T0002 with respect to T0000 are free, the algorithm uses the limit xrate of the order to fix it. Try adding an order between fee and T0001 or T0002 ?

e00E commented 4 years ago

I changed the example to include orders between all pairs, setting buy and sell amount to 1e10 everywhere.

[INFO: PriceEstimation.py:233 | regress_prices_from_xrates()]  Estimated token prices [using external prices = True]
[INFO: PriceEstimation.py:240 | regress_prices_from_xrates()]  Estimated price for [T0000] :                      1000000000000000000
[INFO: PriceEstimation.py:240 | regress_prices_from_xrates()]  Estimated price for [T0001] :                      1000000000000000000
[INFO: PriceEstimation.py:240 | regress_prices_from_xrates()]  Estimated price for [T0002] :                      1500000000000000367

exchange rates between tokens that have external prices and are not the fee token are fixed to the ratio of their external prices.

I don't understand this. We save a singular price for each token in batch_auction.token_info[token].estimated_price. So how can that singular price be a ratio between tokens that aren't fee. I interpreted this the way that the prices are given in the fee.

Anyway, if this is correct, then which prices do we use on the price-estimator rust side? We can't reasonably port PriceEstimation.py so should we continue to assume that the prices as I am currently using them (given in the fee) are good enough that the rounding buffer doesn't end up too far from the solver's real rounding buffer?

marcovc commented 4 years ago

how can that singular price be a ratio between tokens that aren't fee

Let px(T) be the external price of a token T. Let p(T) be the price that we estimate for T. For every T1, T2 where T1!=fee and T2!=fee we impose that p(T1)/p(T2) = px(T1)/px(T2). In other words, the estimated exchange rate between tokens that are not fee is given by the ratio of their corresponding external prices.

About your second question. Not sure, but note that the ratios between tokens other than the fee are what you expect them to be. I guess the ratio to the fee token can only be obtained using the algorithm in that file. Perhaps you can assume 1e18 without being too far off? Need to think more about this.

e00E commented 4 years ago

Thanks for the explanation. I understand now that we will always get the same result for estimated_xrate = estimated_buy_token_price / estimated_sell_token_price and it does not matter what batch_auction.token_info[token].estimated_price each individually is as long as the ratios are going to match up.

e00E commented 4 years ago

Another thing that came up is how we should handle tokens for which we have no estimated price for whatever reason. It's not clear to me what the solver does in this case. We could fall back to the current rounding buffer that is multiplication with constant or maybe use the max_rounding_volume constant from the solver code (this is what I am going to do unless there is a different opinion).

marcovc commented 4 years ago

The code in PriceEstimation.py uses what is essentially a linear regression to estimate prices p(T1) and p(T2) from a set of known exchange rates xrate(T1, T2):

minimize | 1 - product_{T1,T2} { p(T1)/p(T2) / xrate(T1,T2) } |

when we have the external prices px for a given pair of tokens T1, T2 then xrate(T1, T2)=px(T1)/px(T2). When we don't, we use an estimation (https://github.com/gnosis/dex-solver/blob/4b7596d43cfe4ec2b95dca0872c45e7fbc19b4b6/src/opt/process/PriceEstimation.py#L63-L101).

In which programming language are we here? Do we have a package for statistics, in particular computing L1-norm linear regressions (also search for 'generalized linear models')? If so, then this could be replicated exactly...

e00E commented 4 years ago

We're in Rust. The PR for the new rounding buffer is https://github.com/gnosis/dex-services/pull/1051 . There is probably a linear regression library but I'm starting to feel this is getting too complicated if we re-implement all the code from PriceEstimation.py and Rounding.py. That will be me work to keep in sync with the solver for the future or it might not be relevant anymore if we have some solvers in the future that do different rounding. Does the open-solver do price estimating and rounding in the same way?

nlordell commented 4 years ago

Do you think a fair approximation would be to use pricegraph estimates only to compute the rounding buffer?

marcovc commented 4 years ago

but I'm starting to feel this is getting too complicated if we re-implement all the code from PriceEstimation.py and Rounding.py

Yes I agree. On the other hand, if I understand correctly, it appears there is a dependency loop. If the solver was given price estimates for all tokens as an input, then it wouldn't need to do any more fancy computations. I was wondering if the PriceEstimation code should be factored out (not copied).

Does the open-solver do price estimating and rounding in the same way?

It does rounding in the same way, but computes the price estimates differently.

Do you think a fair approximation would be to use pricegraph estimates only to compute the rounding buffer?

I don't think this question is for me, right? But in general, we need the prices for all tokens to compute the rounding buffers. I guess whatever approximation will do, we just need some.

marcovc commented 4 years ago

Alex asked if you could do the following:

  1. Estimate prices using your code with no rounding buffer.
  2. Use estimated prices to compute rounding buffer.
  3. Estimate prices again now using rounding buffer.

Likely it won't be exactly the same solution as the solver one, but that is not guaranteed anyways.

e00E commented 4 years ago

I made the two new issues above related to your previous comment. We should discuss them in their own threads. I definitely would like to avoid using external price sources but it's a change we should make before this change.

e00E commented 4 years ago

Is now implemented for all routes where it makes sense (everything except markets).