Address comments

contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol

@@ -29,7 +29,7 @@ import "./IDevUtils.sol";
 import "./IERC20BridgeSampler.sol";
 import "./IEth2Dai.sol";
 import "./IKyberNetwork.sol";
-import "./IKyberNetworkContract.sol";
+import "./IKyberNetworkProxy.sol";
 import "./IUniswapExchangeQuotes.sol";
 import "./ICurve.sol";
 import "./ILiquidityProvider.sol";
@@ -53,6 +53,10 @@ contract ERC20BridgeSampler is
     uint256 constant internal CURVE_CALL_GAS = 600e3; // 600k
     /// @dev Default gas limit for liquidity provider calls.
     uint256 constant internal DEFAULT_CALL_GAS = 200e3; // 200k
+    /// @dev The Kyber Uniswap Reserve address
+    address constant internal KYBER_UNIWAP_RESERVE = 0x31E085Afd48a1d6e51Cc193153d625e8f0514C7F;
+    /// @dev The Kyber Eth2Dai Reserve address
+    address constant internal KYBER_ETH2DAI_RESERVE = 0x1E158c0e93c30d24e918Ef83d1e0bE23595C3c0f;
 
     address private _devUtilsAddress;
 
@@ -187,15 +191,30 @@ contract ERC20BridgeSampler is
         uint256 numSamples = takerTokenAmounts.length;
         makerTokenAmounts = new uint256[](numSamples);
         address wethAddress = _getWethAddress();
+        uint256 value;
+        address reserve;
         for (uint256 i = 0; i < numSamples; i++) {
             if (takerToken == wethAddress || makerToken == wethAddress) {
-                makerTokenAmounts[i] = _sampleSellFromKyberNetwork(takerToken, makerToken, takerTokenAmounts[i]);
+                // Direct ETH based trade
+                (value, reserve) = _sampleSellFromKyberNetwork(takerToken, makerToken, takerTokenAmounts[i]);
+                // If this fills on an on-chain reserve we remove it as that can introduce collisions
+                if (reserve == KYBER_UNIWAP_RESERVE || reserve == KYBER_ETH2DAI_RESERVE) {
+                    value = 0;
+                }
             } else {
-                uint256 value = _sampleSellFromKyberNetwork(takerToken, wethAddress, takerTokenAmounts[i]);
+                // Hop to ETH
+                (value, reserve) = _sampleSellFromKyberNetwork(takerToken, wethAddress, takerTokenAmounts[i]);
                 if (value != 0) {
-                    makerTokenAmounts[i] = _sampleSellFromKyberNetwork(wethAddress, makerToken, value);
+                    address otherReserve;
+                    (value, otherReserve) = _sampleSellFromKyberNetwork(wethAddress, makerToken, value);
+                    // If this fills on Eth2Dai it is ok as we queried a different market
+                    // If this fills on Uniswap on both legs then this is a hard collision
+                    if (reserve == KYBER_UNIWAP_RESERVE && reserve == otherReserve) {
+                        value = 0;
+                    }
                 }
             }
+            makerTokenAmounts[i] = value;
         }
     }
 
@@ -263,6 +282,31 @@ contract ERC20BridgeSampler is
         }
     }
 
+    /// @dev Sample sell quotes from Eth2Dai/Oasis using a hop to an intermediate token.
+    ///      I.e WBTC/DAI via ETH or WBTC/ETH via DAI
+    /// @param takerToken Address of the taker token (what to sell).
+    /// @param makerToken Address of the maker token (what to buy).
+    /// @param intermediateToken Address of the token to hop to.
+    /// @param takerTokenAmounts Taker token sell amount for each sample.
+    /// @return makerTokenAmounts Maker amounts bought at each taker token
+    ///         amount.
+    function sampleSellsFromEth2DaiHop(
+        address takerToken,
+        address makerToken,
+        address intermediateToken,
+        uint256[] memory takerTokenAmounts
+    )
+        public
+        view
+        returns (uint256[] memory makerTokenAmounts)
+    {
+        if (makerToken == intermediateToken || takerToken == intermediateToken) {
+            return makerTokenAmounts;
+        }
+        uint256[] memory intermediateAmounts = sampleSellsFromEth2Dai(takerToken, intermediateToken, takerTokenAmounts);
+        makerTokenAmounts = sampleSellsFromEth2Dai(intermediateToken, makerToken, intermediateAmounts);
+    }
+
     /// @dev Sample buy quotes from Eth2Dai/Oasis.
     /// @param takerToken Address of the taker token (what to sell).
     /// @param makerToken Address of the maker token (what to buy).
@@ -801,24 +845,24 @@ contract ERC20BridgeSampler is
     )
         private
         view
-        returns (uint256 makerTokenAmount)
+        returns (uint256 makerTokenAmount, address reserve)
     {
         address _takerToken = takerToken == _getWethAddress() ? KYBER_ETH_ADDRESS : takerToken;
         address _makerToken = makerToken == _getWethAddress() ? KYBER_ETH_ADDRESS : makerToken;
         uint256 takerTokenDecimals = _getTokenDecimals(takerToken);
         uint256 makerTokenDecimals = _getTokenDecimals(makerToken);
         (bool didSucceed, bytes memory resultData) = _getKyberNetworkProxyAddress().staticcall.gas(DEFAULT_CALL_GAS)(
             abi.encodeWithSelector(
-                IKyberNetwork(0).kyberNetworkContract.selector
+                IKyberNetworkProxy(0).kyberNetworkContract.selector
             ));
         if (!didSucceed) {
-            return makerTokenAmount;
+            return (0, address(0));
         }
         address kyberNetworkContract = abi.decode(resultData, (address));
         (didSucceed, resultData) =
             kyberNetworkContract.staticcall.gas(KYBER_CALL_GAS)(
                 abi.encodeWithSelector(
-                    IKyberNetworkContract(0).searchBestRate.selector,
+                    IKyberNetwork(0).searchBestRate.selector,
                     _takerToken,
                     _makerToken,
                     takerTokenAmount,
@@ -829,17 +873,15 @@ contract ERC20BridgeSampler is
         if (didSucceed) {
             (reserve, rate) = abi.decode(resultData, (address, uint256));
         } else {
-            return makerTokenAmount;
-        }
-        if (reserve != 0x31E085Afd48a1d6e51Cc193153d625e8f0514C7F || // Uniswap
-            reserve != 0x1E158c0e93c30d24e918Ef83d1e0bE23595C3c0f)   // Eth2Dai
-        {
-            makerTokenAmount =
-                rate *
-                takerTokenAmount *
-                10 ** makerTokenDecimals /
-                10 ** takerTokenDecimals /
-                10 ** 18;
+            return (0, address(0));
         }
+        makerTokenAmount =
+            rate *
+            takerTokenAmount *
+            10 ** makerTokenDecimals /
+            10 ** takerTokenDecimals /
+            10 ** 18;
+
+        return (makerTokenAmount, reserve);
     }
 }

contracts/erc20-bridge-sampler/contracts/src/IKyberNetwork.sol

@@ -21,14 +21,13 @@ pragma solidity ^0.5.9;
 
 interface IKyberNetwork {
 
-    function kyberNetworkContract() external view returns (address);
-
-    function getExpectedRate(
+    function searchBestRate(
         address fromToken,
         address toToken,
-        uint256 fromAmount
+        uint256 fromAmount,
+        bool usePermissionless
     )
         external
         view
-        returns (uint256 expectedRate, uint256 slippageRate);
+        returns (address reserve, uint256 expectedRate);
 }

contracts/erc20-bridge-sampler/contracts/src/IKyberNetworkContract.sol → contracts/erc20-bridge-sampler/contracts/src/IKyberNetworkProxy.sol

@@ -19,15 +19,16 @@
 pragma solidity ^0.5.9;
 
 
-interface IKyberNetworkContract {
+interface IKyberNetworkProxy {
 
-    function searchBestRate(
+    function kyberNetworkContract() external view returns (address);
+
+    function getExpectedRate(
         address fromToken,
         address toToken,
-        uint256 fromAmount,
-        bool usePermissionless
+        uint256 fromAmount
     )
         external
         view
-        returns (address reserve, uint256 expectedRate);
+        returns (uint256 expectedRate, uint256 slippageRate);
 }

contracts/erc20-bridge-sampler/contracts/test/TestERC20BridgeSampler.sol

@@ -24,7 +24,7 @@ import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol";
 import "../src/ERC20BridgeSampler.sol";
 import "../src/IEth2Dai.sol";
 import "../src/IDevUtils.sol";
-import "../src/IKyberNetwork.sol";
+import "../src/IKyberNetworkProxy.sol";
 
 
 library LibDeterministicQuotes {
@@ -210,7 +210,7 @@ contract TestERC20BridgeSamplerKyberNetwork is
         return address(this);
     }
 
-    // IKyberNetworkContract not exposed via IKyberNetwork
+    // IKyberNetwork not exposed via IKyberNetworkProxy
     function searchBestRate(
         address fromToken,
         address toToken,
@@ -225,7 +225,7 @@ contract TestERC20BridgeSamplerKyberNetwork is
         return (address(this), expectedRate);
     }
 
-    // Deterministic `IKyberNetwork.getExpectedRate()`.
+    // Deterministic `IKyberNetworkProxy.getExpectedRate()`.
     function getExpectedRate(
         address fromToken,
         address toToken,

contracts/erc20-bridge-sampler/package.json

@@ -38,7 +38,7 @@
     "config": {
         "publicInterfaceContracts": "ERC20BridgeSampler,IERC20BridgeSampler,ILiquidityProvider,ILiquidityProviderRegistry,DummyLiquidityProviderRegistry,DummyLiquidityProvider",
         "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.",
-        "abis": "./test/generated-artifacts/@(DummyLiquidityProvider|DummyLiquidityProviderRegistry|ERC20BridgeSampler|ICurve|IDevUtils|IERC20BridgeSampler|IEth2Dai|IKyberNetwork|IKyberNetworkContract|ILiquidityProvider|ILiquidityProviderRegistry|IUniswapExchangeQuotes|TestERC20BridgeSampler).json"
+        "abis": "./test/generated-artifacts/@(DummyLiquidityProvider|DummyLiquidityProviderRegistry|ERC20BridgeSampler|ICurve|IDevUtils|IERC20BridgeSampler|IEth2Dai|IKyberNetwork|IKyberNetworkProxy|ILiquidityProvider|ILiquidityProviderRegistry|IUniswapExchangeQuotes|TestERC20BridgeSampler).json"
     },
     "repository": {
         "type": "git",

contracts/erc20-bridge-sampler/test/artifacts.ts

@@ -13,7 +13,7 @@ import * as IDevUtils from '../test/generated-artifacts/IDevUtils.json';
 import * as IERC20BridgeSampler from '../test/generated-artifacts/IERC20BridgeSampler.json';
 import * as IEth2Dai from '../test/generated-artifacts/IEth2Dai.json';
 import * as IKyberNetwork from '../test/generated-artifacts/IKyberNetwork.json';
-import * as IKyberNetworkContract from '../test/generated-artifacts/IKyberNetworkContract.json';
+import * as IKyberNetworkProxy from '../test/generated-artifacts/IKyberNetworkProxy.json';
 import * as ILiquidityProvider from '../test/generated-artifacts/ILiquidityProvider.json';
 import * as ILiquidityProviderRegistry from '../test/generated-artifacts/ILiquidityProviderRegistry.json';
 import * as IUniswapExchangeQuotes from '../test/generated-artifacts/IUniswapExchangeQuotes.json';
@@ -27,7 +27,7 @@ export const artifacts = {
     IERC20BridgeSampler: IERC20BridgeSampler as ContractArtifact,
     IEth2Dai: IEth2Dai as ContractArtifact,
     IKyberNetwork: IKyberNetwork as ContractArtifact,
-    IKyberNetworkContract: IKyberNetworkContract as ContractArtifact,
+    IKyberNetworkProxy: IKyberNetworkProxy as ContractArtifact,
     ILiquidityProvider: ILiquidityProvider as ContractArtifact,
     ILiquidityProviderRegistry: ILiquidityProviderRegistry as ContractArtifact,
     IUniswapExchangeQuotes: IUniswapExchangeQuotes as ContractArtifact,

contracts/erc20-bridge-sampler/test/wrappers.ts

@@ -11,7 +11,7 @@ export * from '../test/generated-wrappers/i_dev_utils';
 export * from '../test/generated-wrappers/i_erc20_bridge_sampler';
 export * from '../test/generated-wrappers/i_eth2_dai';
 export * from '../test/generated-wrappers/i_kyber_network';
-export * from '../test/generated-wrappers/i_kyber_network_contract';
+export * from '../test/generated-wrappers/i_kyber_network_proxy';
 export * from '../test/generated-wrappers/i_liquidity_provider';
 export * from '../test/generated-wrappers/i_liquidity_provider_registry';
 export * from '../test/generated-wrappers/i_uniswap_exchange_quotes';

contracts/erc20-bridge-sampler/tsconfig.json

@@ -17,7 +17,7 @@
         "test/generated-artifacts/IERC20BridgeSampler.json",
         "test/generated-artifacts/IEth2Dai.json",
         "test/generated-artifacts/IKyberNetwork.json",
-        "test/generated-artifacts/IKyberNetworkContract.json",
+        "test/generated-artifacts/IKyberNetworkProxy.json",
         "test/generated-artifacts/ILiquidityProvider.json",
         "test/generated-artifacts/ILiquidityProviderRegistry.json",
         "test/generated-artifacts/IUniswapExchangeQuotes.json",

packages/asset-swapper/src/utils/market_operation_utils/fills.ts

@@ -121,8 +121,10 @@ function dexQuotesToPaths(
     const paths: Fill[][] = [];
     for (let quote of dexQuotes) {
         const path: Fill[] = [];
-        // Drop any non-zero entries. This can occur if the
-        // first few fills on Kyber were UniswapReserves
+        // Drop any non-zero entries. This can occur if the any fills on Kyber were UniswapReserves
+        // We need not worry about Kyber fills going to UniswapReserve as the input amount
+        // we fill is the same as we sampled. I.e we received [0,20,30] output from [1,2,3] input
+        // and we only fill [2,3] on Kyber (as 1 returns 0 output)
         quote = quote.filter(q => !q.output.isZero());
         for (let i = 0; i < quote.length; i++) {
             const sample = quote[i];
@@ -159,12 +161,6 @@ function sourceToFillFlags(source: ERC20BridgeSource): number {
     if (source === ERC20BridgeSource.Kyber) {
         return FillFlags.Kyber;
     }
-    if (source === ERC20BridgeSource.Eth2Dai) {
-        return FillFlags.ConflictsWithKyber;
-    }
-    if (source === ERC20BridgeSource.Uniswap) {
-        return FillFlags.ConflictsWithKyber;
-    }
     return 0;
 }
 
@@ -203,6 +199,7 @@ export function getPathAdjustedSize(path: Fill[], targetInput: BigNumber = POSIT
 }
 
 export function isValidPath(path: Fill[], skipDuplicateCheck: boolean = false): boolean {
+    let flags = 0;
     for (let i = 0; i < path.length; ++i) {
         // Fill must immediately follow its parent.
         if (path[i].parent) {
@@ -218,8 +215,10 @@ export function isValidPath(path: Fill[], skipDuplicateCheck: boolean = false):
                 }
             }
         }
+        flags |= path[i].flags;
     }
-    return true;
+    const conflictFlags = FillFlags.Kyber | FillFlags.ConflictsWithKyber;
+    return (flags & conflictFlags) !== conflictFlags;
 }
 
 export function clipPathToInput(path: Fill[], targetInput: BigNumber = POSITIVE_INF): Fill[] {

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -8,22 +8,10 @@ import { difference } from '../utils';
 
 import { BUY_SOURCES, DEFAULT_GET_MARKET_ORDERS_OPTS, FEE_QUOTE_SOURCES, ONE_ETHER, SELL_SOURCES } from './constants';
 import { createFillPaths, getPathAdjustedRate, getPathAdjustedSlippage } from './fills';
-import {
-    createOrdersFromPath,
-    createSignedOrdersFromRfqtIndicativeQuotes,
-    createSignedOrdersWithFillableAmounts,
-    getNativeOrderTokens,
-} from './orders';
+import { createOrdersFromPath, createSignedOrdersFromRfqtIndicativeQuotes, createSignedOrdersWithFillableAmounts, getNativeOrderTokens } from './orders';
 import { findOptimalPath } from './path_optimizer';
 import { DexOrderSampler, getSampleAmounts } from './sampler';
-import {
-    AggregationError,
-    DexSample,
-    ERC20BridgeSource,
-    GetMarketOrdersOpts,
-    OptimizedMarketOrder,
-    OrderDomain,
-} from './types';
+import { AggregationError, DexSample, ERC20BridgeSource, GetMarketOrdersOpts, OptimizedMarketOrder, OrderDomain } from './types';
 
 async function getRfqtIndicativeQuotesAsync(
     makerAssetData: string,
@@ -343,28 +331,32 @@ export class MarketOperationUtils {
         // Generate a fallback path if native orders are in the optimal paath.
         const nativeSubPath = optimalPath.filter(f => f.source === ERC20BridgeSource.Native);
         if (opts.allowFallback && nativeSubPath.length !== 0) {
+            // We create a fallback path that is exclusive of Native liquidity
+            // This is the optimal on-chain path for the entire input amount
             const nonNativePaths = paths.filter(p => p.length > 0 && p[0].source !== ERC20BridgeSource.Native);
-            // The fallback path is the entire input amount
             const nonNativeOptimalPath = findOptimalPath(side, nonNativePaths, inputAmount, opts.runLimit) || [];
+            // Calculate the slippage of on-chain sources compared to the most optimal path
             const fallbackSlippage = getPathAdjustedSlippage(
                 side,
                 nonNativeOptimalPath,
                 inputAmount,
                 getPathAdjustedRate(side, optimalPath, inputAmount),
             );
             if (fallbackSlippage <= maxFallbackSlippage) {
-                // If the last fill is Native and penultimate is not
-                // then this intention is a partial fill
-                // In this case we drop it as we don't want it to fail
-                // at the end and we don't want to fully fill it
-                // tslint:disable-next-line: no-unused-variable
-                const [last, penultimate, ..._rest] = optimalPath.reverse();
-                const finalNativeFillOrUndefined =
-                    last.source === ERC20BridgeSource.Native && penultimate.source !== ERC20BridgeSource.Native
+                // If the last fill is Native and penultimate is not, then the intention was to partial fill
+                // In this case we drop it entirely as we can't handle a failure at the end and we don't
+                // want to fully fill when it gets prepended to the front below
+                const [last, penultimateIfExists] = optimalPath.slice().reverse();
+                const lastNativeFillIfExists =
+                    last.source === ERC20BridgeSource.Native &&
+                    penultimateIfExists &&
+                    penultimateIfExists.source !== ERC20BridgeSource.Native
                         ? last
                         : undefined;
-
-                optimalPath = [...nativeSubPath.filter(f => f !== finalNativeFillOrUndefined), ...nonNativeOptimalPath];
+                // By prepending native paths to the front they cannot split on-chain sources and incur
+                // an additional protocol fee. I.e [Uniswap,Native,Kyber] becomes [Native,Uniswap,Kyber]
+                // In the previous step we dropped any hanging Native partial fills, as to not fully fill
+                optimalPath = [...nativeSubPath.filter(f => f !== lastNativeFillIfExists), ...nonNativeOptimalPath];
             }
         }
         return createOrdersFromPath(optimalPath, {

packages/contract-addresses/addresses.json

@@ -20,7 +20,7 @@
         "devUtils": "0x74134cf88b21383713e096a5ecf59e297dc7f547",
         "erc20BridgeProxy": "0x8ed95d1746bf1e4dab58d8ed4724f1ef95b20db0",
         "uniswapBridge": "0x36691c4f426eb8f42f150ebde43069a31cb080ad",
-        "erc20BridgeSampler": "0x9e4fdb09dcd4fa2b5cc62da9faf3577f51b4d43e",
+        "erc20BridgeSampler": "0xc0154b14cc60a6661171fdc817f759429a57e184",
         "kyberBridge": "0x1c29670f7a77f1052d30813a0a4f632c78a02610",
         "eth2DaiBridge": "0x991c745401d5b5e469b8c3e2cb02c748f08754f1",
         "chaiBridge": "0x77c31eba23043b9a72d13470f3a3a311344d7438",