Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

feat: ERC20BridgeSampler Unlock Kyber collisions #2575

Merged
merged 6 commits into from
May 6, 2020

Conversation

dekz
Copy link
Member

@dekz dekz commented Apr 30, 2020

Description

Detect when Kyber is using a Uniswap or Eth2Dai bridge and return 0. When the reserve isn't this bridge we return the makerAmount.

Since a Kyber DAI/USDC trade is actually DAI->ETH, ETH->USDC the Sampler now samples these individual legs rather than DAI/USDC. As a result we're sampling closer to the behaviour of the trade function. I.e Sampling DAI/USDC isn't the same as DAI->ETH, ETH->USDC and results vary.

When sampling Kyber via the ERC20BridgeSampler it will ONLY return amounts when Kyber Reserves are hit. This means its possible for the Kyber sampler to return:

0, // UniswapReserve
0, // UniswapReserve
100, // KyberReserve
200, // KyberReserve
0, // UniswapReserve

We also move Native to the front and on-chain to the end for a new fallback path strategy. If a Native order was previously after an on-chain at the end, it is dropped.

Since Native orders are now all at the front, they cannot split an on-chain fill and incur an additional protocol fee.

Fallback is now the optimal route of on-chain sources (previously a 3rd source). Native orders are then simply prepended to the front. This means that in the optimal case (no Native reverts) a middle on-chain source may get filled more than optimal (by the Native amount).

For example, picture a sell of 100 DAI where the optimal route is a 3 way split between 0x Native, Uniswap and Kyber. The fallback is 50/50 Uni/Kyber. The route is adjusted to fill: 33 Native, 50 Uni, 50 Kyber. In the event of Native reverts it fills at 50 Uni, 50 Kyber. In the event of no reverts it fills at 33 Native, 50 Uni, 17 Kyber

In a 2 way split this matters less. 33 Native, 77 Uniswap -> 33 Native, 100 Uniswap

TODO

  • redeploy Mainnet sampler with verified source

Follow ups:

  • permissioned vs permissionless Kyber Reserves. Requires redeploy of KyberBridge

@dekz dekz force-pushed the feat/erc20-bridge-sampler/unlock-kyber-collisions branch 6 times, most recently from 7e39e71 to 51dd194 Compare May 3, 2020 03:22
@coveralls
Copy link

coveralls commented May 3, 2020

Coverage Status

Coverage decreased (-0.3%) to 79.361% when pulling b7c127d on feat/erc20-bridge-sampler/unlock-kyber-collisions into fe36cd8 on development.

@dekz dekz force-pushed the feat/erc20-bridge-sampler/unlock-kyber-collisions branch from 51dd194 to dcbb3e1 Compare May 3, 2020 04:09
@dekz dekz marked this pull request as ready for review May 3, 2020 04:56
@dekz dekz requested a review from dorothy-zbornak May 3, 2020 09:02
@dekz dekz force-pushed the feat/erc20-bridge-sampler/unlock-kyber-collisions branch from ebbd8ae to 1bb44af Compare May 4, 2020 21:47
const path: Fill[] = [];
// Drop any non-zero entries. This can occur if the
// first few fills on Kyber were UniswapReserves
quote = quote.filter(q => !q.output.isZero());
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there an issue if we try to partial fill this path? If we fill at less than the input of the first valid Fill node, then we'll be hitting Uniswap? If Kyber is last then maybe this won't be such a big deal since Kyber will re-route, but could throw off a subsequent Uniswap fill if it isn't. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought this too originally but didn't seem to encounter it.

Let's say the samples for Kyber looks like: [0, 0, 1000] Where 0s are other reserves, we only unlock kyber at the third stop (enough volume for it to switch). In the routing we'd only select Kyber for at least this amount, so we're going to hit the right reserve. But that could all change at execution time.

I expected to see it in simbot, but it didn't seem to occur, so I removed sorting Kyber last.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can gather compelling data on pairs where this scenario is most likely then I'm fine with this hack. We should def document this reasoning and accepted risk in the code though. Otherwise I'll forget one day and revert this change.

@dekz dekz force-pushed the feat/erc20-bridge-sampler/unlock-kyber-collisions branch from 3bc3690 to 6c55a13 Compare May 5, 2020 12:30
@dekz dekz force-pushed the feat/erc20-bridge-sampler/unlock-kyber-collisions branch from 6c55a13 to b7afdea Compare May 5, 2020 23:29
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak left a comment

Choose a reason for hiding this comment

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

one more Q then let's give this a try

dekz and others added 2 commits May 7, 2020 06:57
@dekz dekz merged commit fb0311e into development May 6, 2020
@dekz dekz deleted the feat/erc20-bridge-sampler/unlock-kyber-collisions branch May 6, 2020 21:56
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.

3 participants