cowprotocol / services

Off-chain services for CoW Protocol
https://cow.fi/
Other
185 stars 71 forks source link

Reduce Number of Unsupported Tokens #125

Closed nlordell closed 3 months ago

nlordell commented 2 years ago

This epic issue captures the work around trying to reduce the number of unsupported tokens.

Some tasks:

We should also start measuring unsupported token counts:

Original issue https://github.com/gnosis/gp-v2-services/issues/1739 by @nlordell

MartinquaXD commented 2 years ago

I researched the topic for a while now and unfortunately found no low hanging fruit. I tried to flesh out the currently preferred idea of simulating estimates in the order book a bit more and thought about alternatives to that idea. I think whatever solution we end up with should be compatible with decentralizing the solver aspect of the protocol with the least amount of maintenance work on our side.

Goal

We only want to accept an order (i.e. add them to the order book) if it is reasonable to expect that it can get filled. Otherwise people will complain about UX when their order eventually expires.
We also want to maximize the number of supported tokens because that is something our user complain about.

Scenario

We have a single order book and multiple solvers which can theoretically support different tokens. This can happen because they have access to private/unique liquidity sources or handle problematic tokens (e.g. fee on transfer) better than other solvers.

Assumption

With a growing number of solvers the diversity of how many solvers can handle a token is likely to increase.
Because there are already quite sophisticated solvers for "normal" tokens the barrier of entrance for those is quite high so it might make sense for some solvers to focus on a specific niche (for example fee-on-transfer tokens) to become profitable. That's why I expect the likelyhood of solvers supporting fringe tokens is also likely to increase.

Problem to solve

How can we know if a token is supported by the available solvers before submitting it to the order book?
Note that some solvers might give out quotes for problematic tokens which might not be correct/executable.

Current approach

Currently we have a component (TraceCallDetector) which acts as a gate keeper to prevent orders with "bad" tokens to enter the order book. This is done by simulating a transfer into and out of the settlement contract for each token and checking that no funds got lost doing that (detect fee-on-transfer tokens). It also tests if the token allows to set an allowance of U256::MAX.
One very important requirement for this to work is that we need to know about on-chain liquidity for each token.

After an order has entered the order book the driver distributes it to all solvers.

This approach is problematic in cases where there is a disconnect between what the order book considers supported and what solvers actually support. Probably most of the time the disconnect comes from solvers knowing about other liquidity sources than we do but theoretically there could be solvers which support fee-on-transfer tokens (which we currently try to filter out).

Alternatives

Keep general architecture but use simulations as a fallback for "bad" tokens

Instead of giving up when the TraceCallDetector detects a "bad" token we could still try to get trade transactions for the order from the solvers and simulate it. If the solver is actually able to provide a working solution we allow the order in the order book.

Downsides

Advantages

Handle price estimation entirely in the driver

Instead of complicating things by simulating transactions in the order book, we could expose an endpoint in the driver which solves a settlement on demand. If someone wants to get a quote or submit an order, we no longer use the TraceCallDetector but let the solvers work on a settlement with a single order. All validation which should happen before submitting an order into the order book already happens during solution competition. If we receive at least 1 executable settlement we can consider this token supported. This can get cached as it does at the moment.

Note that it would make sense to keep the TraceCallDetector (but in the driver) to easily enable solvers to opt-out of fee-on-transfer token trades (without reimplementing that logic themselves). This would for example prevent a bad token from "poisoning" a whole batch settlement in Quasimodo.

Downsides

Advantages

josojo commented 2 years ago

Thanks Martin for doing all the research.

One point I am missing from the write-up is the actual failure cause from most tokens that we end denylisting due to failed simulations. Do you know roughly why the majority fails? It's a fee per trade/transfer, right?

And I also would like to know how uniswap or other exchanges are dealing with it. It seems for example that on uniswap I can trade the LOL token(0x3506b7eff28e41ff0746a432897777973cfb2d46, recently denylisted by us). Do they have different router contracts?

Sorry for not helping, just asking questions :)

MartinquaXD commented 2 years ago

@josojo there are 2 problematic cases for bad token detection:

  1. false negatives (those bypass the detection so we have to manually deny-list them)
  2. false positives (those are flagged as bad although there are exchanges able to trade it)

You are talking about number 1. Many of those tokens are just proxies so we can't see their implementation. I believe those tokens might only take a fee under some conditions, which we didn't trigger during our TraceCallDetector simulation but we trigger them when the solver tries to settle the orders. Another option could be normal but highly volatile tokens (i.e. should not be deny-listed). The suggested solutions mostly deal with scenario 2 by allowing more tokens. I'm not sure if it's possible to reduce false negatives and false positives at the same time, though.

As for your second question: UniswapV2 offers special functions to deal with fee-on-transfer tokens. But I guess this was a PITA so they stopped supporting that in V3. I believe exchanges supporting those tokens route their trades through uniV2 pools.

vkgnosis commented 2 years ago

Probably most of the time the disconnect comes from solvers knowing about other liquidity sources than we do but theoretically there could be solvers which support fee-on-transfer tokens (which we currently try to filter out).

If really the problem is that most of the time we cannot find a source of the token to simulate with on chain then we should be able to mostly fix this. We currently check balancer and univ3 already making it unlikely that a real token is not on either. There are probably more liquidity sources that would be easy to add.

It seems for example that on uniswap I can trade the LOL token

How do you know? It is possible that their UI shows you a trade but it would revert if you executed. Like Martin said univ3 has documented somewhere that fee on transfer tokens are not supported (I posted a link to that on slack but can't find it right now).

I feel that it is not worth supporting fee on transfer tokens. They are usually scummy and they are really hard to implement properly because you don't know in what way they're taking fees. So if most unsupported tokens are like this then I'm fine with the status quo.

MartinquaXD commented 2 years ago

So if most unsupported tokens are like this then I'm fine with the status quo.

I think at least for the second solution it's important to not only see the increased number of supported (maybe shady) tokens but also the reduced maintenance burden for us when more people want to write their own solvers. Also IMO we should enable 3rd party solvers to use liquidity which we didn't integrate ourselves. In that case our current approach would limit how much 3rd parties could take advantage of that because they would never be able to settle trades with tokens which we don't have liquidity for. This is probably nothing which has to happen soon but at some point we should shift our focus from what we want to implement to what we want to enable our integrators to implement. And since we are currently pushing for people to participate in the solver competition and still have to design and agree on an interface for 3rd party solvers we should keep this idea in mind.

vkgnosis commented 2 years ago

For the tokens it is more difficult because we also take our own fees in these tokens. So you could imagine a token with a really weird fee model so that our estimate of much it is worth is off. We probably shouldn't accept such a token even if it was solvable.

fleupold commented 2 years ago

In my mind we should actually require each solver to also give a quote for a single orders (could be simulated by sending them a batch with one order). This way we can ensure users can get a price estimate for each token that is eventually supported. I do see there is some risk with solvers giving bogus estimates (maybe they can be signed and if they cannot be fulfilled there could be some type of penalty).

Most of the disadvantages for the "simulations approach" seem workable to me. The penalty we get from market makers for quoting too frequently is something solvers have to already deal with (as they are not guaranteed to be chosen) and I'd argue that there shouldn't be a conceptual difference between a solver quoting or solving for an order. The remaining parts seem like engineering effort to me (agree it's not a low hanging fruit).

Regarding the rest of the conversation:

One very important requirement for this to work is that we need to know about on-chain liquidity for each token.

As @vkgnosis pointed out this could actually be a low hanging fruit. A while back @bh2smith suggested to use the etherscan API to get token holders for an arbitrary token (I couldn't find their endpoint, but found a free provider for the same purpose).

I agree with @josojo that it would be nice to get more quantitative data on the nature of both false positives as well as false negatives. Can we instrument the failure reason for unsupported tokens and check which ones of them are actually supported e.g. by Uniswap or 1Inch?

github-actions[bot] commented 4 months ago

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