From 0cf3ff8209c4fa40775c54fb8304512aae7da89a Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Wed, 15 Jan 2020 16:49:06 -0500 Subject: [PATCH] `@0x/contracts-erc20-bridge-sampler`: Increase gas forwarded to kyber and eth2dai. `@0x/contracts-erc20-bridge-sampler`: Bail as soon as any quote from a DEX fails. `@0x/contracts-erc20-bridge-sampler`: Fix broken tests. --- contracts/erc20-bridge-sampler/CHANGELOG.json | 4 ++ .../contracts/src/ERC20BridgeSampler.sol | 44 +++++++++++++------ .../contracts/test/TestERC20BridgeSampler.sol | 6 ++- .../test/erc20-bridge-sampler.ts | 20 ++++----- 4 files changed, 49 insertions(+), 25 deletions(-) diff --git a/contracts/erc20-bridge-sampler/CHANGELOG.json b/contracts/erc20-bridge-sampler/CHANGELOG.json index 89389500ec..72ab8c0049 100644 --- a/contracts/erc20-bridge-sampler/CHANGELOG.json +++ b/contracts/erc20-bridge-sampler/CHANGELOG.json @@ -5,6 +5,10 @@ { "note": "Add batch functions to query quotes", "pr": 2427 + }, + { + "note": "Early exit if a DEX sample fails", + "pr": 2427 } ] }, diff --git a/contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol b/contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol index 9400c659d2..149c50312b 100644 --- a/contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol +++ b/contracts/erc20-bridge-sampler/contracts/src/ERC20BridgeSampler.sol @@ -37,9 +37,9 @@ contract ERC20BridgeSampler is DeploymentConstants { bytes4 constant internal ERC20_PROXY_ID = 0xf47261b0; // bytes4(keccak256("ERC20Token(address)")); - uint256 constant internal KYBER_SAMPLE_CALL_GAS = 600e3; + uint256 constant internal KYBER_SAMPLE_CALL_GAS = 1500e3; uint256 constant internal UNISWAP_SAMPLE_CALL_GAS = 150e3; - uint256 constant internal ETH2DAI_SAMPLE_CALL_GAS = 250e3; + uint256 constant internal ETH2DAI_SAMPLE_CALL_GAS = 1000e3; /// @dev Query batches of native orders and sample sell quotes on multiple DEXes at once. /// @param orders Batches of Native orders to query. @@ -105,7 +105,6 @@ contract ERC20BridgeSampler is } } - /// @dev Query native orders and sample sell quotes on multiple DEXes at once. /// @param orders Native orders to query. /// @param orderSignatures Signatures for each respective order in `orders`. @@ -346,6 +345,8 @@ contract ERC20BridgeSampler is uint256 rate = 0; if (didSucceed) { rate = abi.decode(resultData, (uint256)); + } else { + break; } makerTokenAmounts[i] = rate * @@ -386,6 +387,8 @@ contract ERC20BridgeSampler is uint256 buyAmount = 0; if (didSucceed) { buyAmount = abi.decode(resultData, (uint256)); + } else{ + break; } makerTokenAmounts[i] = buyAmount; } @@ -421,6 +424,8 @@ contract ERC20BridgeSampler is uint256 sellAmount = 0; if (didSucceed) { sellAmount = abi.decode(resultData, (uint256)); + } else { + break; } takerTokenAmounts[i] = sellAmount; } @@ -449,26 +454,28 @@ contract ERC20BridgeSampler is IUniswapExchangeQuotes makerTokenExchange = makerToken == _getWethAddress() ? IUniswapExchangeQuotes(0) : _getUniswapExchange(makerToken); for (uint256 i = 0; i < numSamples; i++) { + bool didSucceed = true; if (makerToken == _getWethAddress()) { - makerTokenAmounts[i] = _callUniswapExchangePriceFunction( + (makerTokenAmounts[i], didSucceed) = _callUniswapExchangePriceFunction( address(takerTokenExchange), takerTokenExchange.getTokenToEthInputPrice.selector, takerTokenAmounts[i] ); } else if (takerToken == _getWethAddress()) { - makerTokenAmounts[i] = _callUniswapExchangePriceFunction( + (makerTokenAmounts[i], didSucceed) = _callUniswapExchangePriceFunction( address(makerTokenExchange), makerTokenExchange.getEthToTokenInputPrice.selector, takerTokenAmounts[i] ); } else { - uint256 ethBought = _callUniswapExchangePriceFunction( + uint256 ethBought; + (ethBought, didSucceed) = _callUniswapExchangePriceFunction( address(takerTokenExchange), takerTokenExchange.getTokenToEthInputPrice.selector, takerTokenAmounts[i] ); if (ethBought != 0) { - makerTokenAmounts[i] = _callUniswapExchangePriceFunction( + (makerTokenAmounts[i], didSucceed) = _callUniswapExchangePriceFunction( address(makerTokenExchange), makerTokenExchange.getEthToTokenInputPrice.selector, ethBought @@ -477,6 +484,9 @@ contract ERC20BridgeSampler is makerTokenAmounts[i] = 0; } } + if (!didSucceed) { + break; + } } } @@ -503,26 +513,28 @@ contract ERC20BridgeSampler is IUniswapExchangeQuotes makerTokenExchange = makerToken == _getWethAddress() ? IUniswapExchangeQuotes(0) : _getUniswapExchange(makerToken); for (uint256 i = 0; i < numSamples; i++) { + bool didSucceed = true; if (makerToken == _getWethAddress()) { - takerTokenAmounts[i] = _callUniswapExchangePriceFunction( + (takerTokenAmounts[i], didSucceed) = _callUniswapExchangePriceFunction( address(takerTokenExchange), takerTokenExchange.getTokenToEthOutputPrice.selector, makerTokenAmounts[i] ); } else if (takerToken == _getWethAddress()) { - takerTokenAmounts[i] = _callUniswapExchangePriceFunction( + (takerTokenAmounts[i], didSucceed) = _callUniswapExchangePriceFunction( address(makerTokenExchange), makerTokenExchange.getEthToTokenOutputPrice.selector, makerTokenAmounts[i] ); } else { - uint256 ethSold = _callUniswapExchangePriceFunction( + uint256 ethSold; + (ethSold, didSucceed) = _callUniswapExchangePriceFunction( address(makerTokenExchange), makerTokenExchange.getEthToTokenOutputPrice.selector, makerTokenAmounts[i] ); if (ethSold != 0) { - takerTokenAmounts[i] = _callUniswapExchangePriceFunction( + (takerTokenAmounts[i], didSucceed) = _callUniswapExchangePriceFunction( address(takerTokenExchange), takerTokenExchange.getTokenToEthOutputPrice.selector, ethSold @@ -531,6 +543,9 @@ contract ERC20BridgeSampler is takerTokenAmounts[i] = 0; } } + if (!didSucceed) { + break; + } } } @@ -558,12 +573,13 @@ contract ERC20BridgeSampler is ) private view - returns (uint256 outputAmount) + returns (uint256 outputAmount, bool didSucceed) { if (uniswapExchangeAddress == address(0)) { - return 0; + return (outputAmount, didSucceed); } - (bool didSucceed, bytes memory resultData) = + bytes memory resultData; + (didSucceed, resultData) = uniswapExchangeAddress.staticcall.gas(UNISWAP_SAMPLE_CALL_GAS)( abi.encodeWithSelector( functionSelector, diff --git a/contracts/erc20-bridge-sampler/contracts/test/TestERC20BridgeSampler.sol b/contracts/erc20-bridge-sampler/contracts/test/TestERC20BridgeSampler.sol index 4c98ca2c56..09ad84d197 100644 --- a/contracts/erc20-bridge-sampler/contracts/test/TestERC20BridgeSampler.sol +++ b/contracts/erc20-bridge-sampler/contracts/test/TestERC20BridgeSampler.sol @@ -338,7 +338,11 @@ contract TestERC20BridgeSampler is bytes32 orderHash = keccak256(abi.encode(order.salt)); // Everything else is derived from the hash. orderInfo.orderHash = orderHash; - orderInfo.orderStatus = LibOrder.OrderStatus(uint256(orderHash) % MAX_ORDER_STATUS); + if (uint256(orderHash) % 100 > 90) { + orderInfo.orderStatus = LibOrder.OrderStatus.FULLY_FILLED; + } else { + orderInfo.orderStatus = LibOrder.OrderStatus.FILLABLE; + } orderInfo.orderTakerAssetFilledAmount = uint256(orderHash) % order.takerAssetAmount; fillableTakerAssetAmount = order.takerAssetAmount - orderInfo.orderTakerAssetFilledAmount; diff --git a/contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts b/contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts index 26c9cae9f9..c134c46d8f 100644 --- a/contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts +++ b/contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts @@ -195,7 +195,7 @@ blockchainTests('erc20-bridge-sampler', env => { function getDeterministicFillableTakerAssetAmount(order: Order): BigNumber { const hash = getPackedHash(hexUtils.toHex(order.salt, 32)); - const orderStatus = new BigNumber(hash).mod(255).toNumber(); + const orderStatus = new BigNumber(hash).mod(100).toNumber() > 90 ? 5 : 3; const isValidSignature = !!new BigNumber(hash).mod(2).toNumber(); if (orderStatus !== 3 || !isValidSignature) { return constants.ZERO_AMOUNT; @@ -208,7 +208,7 @@ blockchainTests('erc20-bridge-sampler', env => { return order.makerAssetAmount .times(takerAmount) .div(order.takerAssetAmount) - .integerValue(BigNumber.ROUND_DOWN); + .integerValue(BigNumber.ROUND_UP); } function getERC20AssetData(tokenAddress: string): string { @@ -255,7 +255,7 @@ blockchainTests('erc20-bridge-sampler', env => { describe('getOrderFillableTakerAssetAmounts()', () => { it('returns the expected amount for each order', async () => { const orders = createOrders(MAKER_TOKEN, TAKER_TOKEN); - const signatures: string[] = _.times(orders.length, hexUtils.random); + const signatures: string[] = _.times(orders.length, i => hexUtils.random()); const expected = orders.map(getDeterministicFillableTakerAssetAmount); const actual = await testContract.getOrderFillableTakerAssetAmounts(orders, signatures).callAsync(); expect(actual).to.deep.eq(expected); @@ -269,7 +269,7 @@ blockchainTests('erc20-bridge-sampler', env => { it('returns zero for an order with zero maker asset amount', async () => { const orders = createOrders(MAKER_TOKEN, TAKER_TOKEN, 1); orders[0].makerAssetAmount = constants.ZERO_AMOUNT; - const signatures: string[] = _.times(orders.length, hexUtils.random); + const signatures: string[] = _.times(orders.length, i => hexUtils.random()); const actual = await testContract.getOrderFillableTakerAssetAmounts(orders, signatures).callAsync(); expect(actual).to.deep.eq([constants.ZERO_AMOUNT]); }); @@ -277,7 +277,7 @@ blockchainTests('erc20-bridge-sampler', env => { it('returns zero for an order with zero taker asset amount', async () => { const orders = createOrders(MAKER_TOKEN, TAKER_TOKEN, 1); orders[0].takerAssetAmount = constants.ZERO_AMOUNT; - const signatures: string[] = _.times(orders.length, hexUtils.random); + const signatures: string[] = _.times(orders.length, i => hexUtils.random()); const actual = await testContract.getOrderFillableTakerAssetAmounts(orders, signatures).callAsync(); expect(actual).to.deep.eq([constants.ZERO_AMOUNT]); }); @@ -293,7 +293,7 @@ blockchainTests('erc20-bridge-sampler', env => { describe('getOrderFillableMakerAssetAmounts()', () => { it('returns the expected amount for each order', async () => { const orders = createOrders(MAKER_TOKEN, TAKER_TOKEN); - const signatures: string[] = _.times(orders.length, hexUtils.random); + const signatures: string[] = _.times(orders.length, i => hexUtils.random()); const expected = orders.map(getDeterministicFillableMakerAssetAmount); const actual = await testContract.getOrderFillableMakerAssetAmounts(orders, signatures).callAsync(); expect(actual).to.deep.eq(expected); @@ -307,7 +307,7 @@ blockchainTests('erc20-bridge-sampler', env => { it('returns zero for an order with zero maker asset amount', async () => { const orders = createOrders(MAKER_TOKEN, TAKER_TOKEN, 1); orders[0].makerAssetAmount = constants.ZERO_AMOUNT; - const signatures: string[] = _.times(orders.length, hexUtils.random); + const signatures: string[] = _.times(orders.length, i => hexUtils.random()); const actual = await testContract.getOrderFillableMakerAssetAmounts(orders, signatures).callAsync(); expect(actual).to.deep.eq([constants.ZERO_AMOUNT]); }); @@ -315,7 +315,7 @@ blockchainTests('erc20-bridge-sampler', env => { it('returns zero for an order with zero taker asset amount', async () => { const orders = createOrders(MAKER_TOKEN, TAKER_TOKEN, 1); orders[0].takerAssetAmount = constants.ZERO_AMOUNT; - const signatures: string[] = _.times(orders.length, hexUtils.random); + const signatures: string[] = _.times(orders.length, i => hexUtils.random()); const actual = await testContract.getOrderFillableMakerAssetAmounts(orders, signatures).callAsync(); expect(actual).to.deep.eq([constants.ZERO_AMOUNT]); }); @@ -330,7 +330,7 @@ blockchainTests('erc20-bridge-sampler', env => { describe('queryOrdersAndSampleSells()', () => { const ORDERS = createOrders(MAKER_TOKEN, TAKER_TOKEN); - const SIGNATURES: string[] = _.times(ORDERS.length, hexUtils.random); + const SIGNATURES: string[] = _.times(ORDERS.length, i => hexUtils.random()); before(async () => { await testContract.createTokenExchanges([MAKER_TOKEN, TAKER_TOKEN]).awaitTransactionSuccessAsync(); @@ -436,7 +436,7 @@ blockchainTests('erc20-bridge-sampler', env => { describe('queryOrdersAndSampleBuys()', () => { const ORDERS = createOrders(MAKER_TOKEN, TAKER_TOKEN); - const SIGNATURES: string[] = _.times(ORDERS.length, hexUtils.random); + const SIGNATURES: string[] = _.times(ORDERS.length, i => hexUtils.random()); before(async () => { await testContract.createTokenExchanges([MAKER_TOKEN, TAKER_TOKEN]).awaitTransactionSuccessAsync();