-
Notifications
You must be signed in to change notification settings - Fork 465
feat: ERC20BridgeSampler Unlock Kyber collisions #2575
feat: ERC20BridgeSampler Unlock Kyber collisions #2575
Conversation
7e39e71
to
51dd194
Compare
51dd194
to
dcbb3e1
Compare
ebbd8ae
to
1bb44af
Compare
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()); |
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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 0
s 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.
There was a problem hiding this comment.
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.
packages/asset-swapper/src/utils/market_operation_utils/fills.ts
Outdated
Show resolved
Hide resolved
packages/asset-swapper/src/utils/market_operation_utils/index.ts
Outdated
Show resolved
Hide resolved
packages/asset-swapper/src/utils/market_operation_utils/index.ts
Outdated
Show resolved
Hide resolved
packages/asset-swapper/src/utils/market_operation_utils/index.ts
Outdated
Show resolved
Hide resolved
contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol
Outdated
Show resolved
Hide resolved
contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol
Outdated
Show resolved
Hide resolved
3bc3690
to
6c55a13
Compare
6c55a13
to
b7afdea
Compare
There was a problem hiding this 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
packages/asset-swapper/src/utils/market_operation_utils/index.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Lawrence Forman <lawrence@0xproject.com>
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 actuallyDAI->ETH, ETH->USDC
the Sampler now samples these individual legs rather thanDAI/USDC
. As a result we're sampling closer to the behaviour of thetrade
function. I.e SamplingDAI/USDC
isn't the same asDAI->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:
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 at50 Uni, 50 Kyber
. In the event of no reverts it fills at33 Native, 50 Uni, 17 Kyber
In a 2 way split this matters less.
33 Native, 77 Uniswap
->33 Native, 100 Uniswap
TODO
Follow ups: