diff --git a/contracts/dev-utils/CHANGELOG.json b/contracts/dev-utils/CHANGELOG.json index 37287903d7..2ea687bc83 100644 --- a/contracts/dev-utils/CHANGELOG.json +++ b/contracts/dev-utils/CHANGELOG.json @@ -21,6 +21,10 @@ { "note": "Refactor `OrderValidationUtils` to calculate `fillableTakerAssetAmount`", "pr": 1848 + }, + { + "note": "Add `OrderTransferSimulationUtils` contract for simulating order transfers on-chain", + "pr": 1868 } ] } diff --git a/contracts/dev-utils/compiler.json b/contracts/dev-utils/compiler.json index eccc11fef8..b78e297573 100644 --- a/contracts/dev-utils/compiler.json +++ b/contracts/dev-utils/compiler.json @@ -22,5 +22,10 @@ } } }, - "contracts": ["src/DevUtils.sol", "src/LibAssetData.sol", "src/LibTransactionDecoder.sol"] + "contracts": [ + "src/DevUtils.sol", + "src/LibAssetData.sol", + "src/LibTransactionDecoder.sol", + "src/OrderTransferSimulationUtils.sol" + ] } diff --git a/contracts/dev-utils/contracts/src/DevUtils.sol b/contracts/dev-utils/contracts/src/DevUtils.sol index 8e2e1d3f63..5ec9418c62 100644 --- a/contracts/dev-utils/contracts/src/DevUtils.sol +++ b/contracts/dev-utils/contracts/src/DevUtils.sol @@ -20,16 +20,19 @@ pragma solidity ^0.5.5; pragma experimental ABIEncoderV2; import "./OrderValidationUtils.sol"; +import "./OrderTransferSimulationUtils.sol"; import "./LibTransactionDecoder.sol"; // solhint-disable no-empty-blocks contract DevUtils is OrderValidationUtils, + OrderTransferSimulationUtils, LibTransactionDecoder { constructor (address _exchange) public OrderValidationUtils(_exchange) + OrderTransferSimulationUtils(_exchange) {} } \ No newline at end of file diff --git a/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol b/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol new file mode 100644 index 0000000000..9a7c5a9208 --- /dev/null +++ b/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol @@ -0,0 +1,154 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; + + +import "@0x/contracts-exchange/contracts/src/interfaces/IExchange.sol"; +import "@0x/contracts-exchange/contracts/src/libs/LibExchangeRichErrorDecoder.sol"; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; +import "@0x/contracts-utils/contracts/src/LibBytes.sol"; + + +contract OrderTransferSimulationUtils is + LibExchangeRichErrorDecoder +{ + using LibBytes for bytes; + + enum OrderTransferResults { + TakerAssetDataFailed, // Transfer of takerAsset failed + MakerAssetDataFailed, // Transfer of makerAsset failed + TakerFeeAssetDataFailed, // Transfer of takerFeeAsset failed + MakerFeeAssetDataFailed, // Transfer of makerFeeAsset failed + TransfersSuccessful // All transfers in the order were successful + } + + // simulateDispatchTransferFromCalls(bytes[],address[],address[],uint256[]) + bytes4 constant internal _SIMULATE_DISPATCH_TRANSFER_FROM_CALLS_SELECTOR = 0xb04fbddd; + + // keccak256(abi.encodeWithSignature("Error(string)", "TRANSFERS_SUCCESSFUL")); + bytes32 constant internal _TRANSFERS_SUCCESSFUL_RESULT_HASH = 0xf43f26ea5a94b478394a975e856464913dc1a8a1ca70939d974aa7c238aa0ce0; + + // solhint-disable var-name-mixedcase + IExchange internal _EXCHANGE; + // solhint-enable var-name-mixedcase + + constructor (address _exchange) + public + { + _EXCHANGE = IExchange(_exchange); + } + + /// @dev Simulates all of the transfers within an order and returns the index of the first failed transfer. + /// @param order The order to simulate transfers for. + /// @param takerAddress The address of the taker that will fill the order. + /// @param takerAssetFillAmount The amount of takerAsset that the taker wished to fill. + /// @return The index of the first failed transfer (or 4 if all transfers are successful). + function getSimulatedOrderTransferResults( + LibOrder.Order memory order, + address takerAddress, + uint256 takerAssetFillAmount + ) + public + returns (OrderTransferResults orderTransferResults) + { + // Create input arrays + bytes[] memory assetData = new bytes[](4); + address[] memory fromAddresses = new address[](4); + address[] memory toAddresses = new address[](4); + uint256[] memory amounts = new uint256[](4); + + // Transfer `takerAsset` from taker to maker + assetData[0] = order.takerAssetData; + fromAddresses[0] = takerAddress; + toAddresses[0] = order.makerAddress; + amounts[0] = order.takerAssetAmount; + + // Transfer `makerAsset` from maker to taker + assetData[1] = order.makerAssetData; + fromAddresses[1] = order.makerAddress; + toAddresses[1] = takerAddress; + amounts[1] = order.makerAssetAmount; + + // Transfer `takerFeeAsset` from taker to feeRecipient + assetData[2] = order.takerFeeAssetData; + fromAddresses[2] = takerAddress; + toAddresses[2] = order.feeRecipientAddress; + amounts[2] = order.takerFee; + + // Transfer `makerFeeAsset` from maker to feeRecipient + assetData[3] = order.makerFeeAssetData; + fromAddresses[3] = order.makerAddress; + toAddresses[3] = order.feeRecipientAddress; + amounts[3] = order.makerFee; + + // Encode data for `simulateDispatchTransferFromCalls(assetData, fromAddresses, toAddresses, amounts)` + bytes memory simulateDispatchTransferFromCallsData = abi.encodeWithSelector( + _SIMULATE_DISPATCH_TRANSFER_FROM_CALLS_SELECTOR, + assetData, + fromAddresses, + toAddresses, + amounts + ); + + // Perform call and catch revert + (, bytes memory returnData) = address(_EXCHANGE).call(simulateDispatchTransferFromCallsData); + + bytes4 selector = returnData.readBytes4(0); + if (selector == ASSET_PROXY_DISPATCH_ERROR_SELECTOR) { + // Decode AssetProxyDispatchError and return index of failed transfer + (, bytes32 failedTransferIndex,) = decodeAssetProxyDispatchError(returnData); + return OrderTransferResults(uint8(uint256(failedTransferIndex))); + } else if (selector == ASSET_PROXY_TRANSFER_ERROR_SELECTOR) { + // Decode AssetProxyTransferError and return index of failed transfer + (bytes32 failedTransferIndex, ,) = decodeAssetProxyTransferError(returnData); + return OrderTransferResults(uint8(uint256(failedTransferIndex))); + } else if (keccak256(returnData) == _TRANSFERS_SUCCESSFUL_RESULT_HASH) { + // All transfers were successful + return OrderTransferResults.TransfersSuccessful; + } else { + revert("UNKNOWN_RETURN_DATA"); + } + } + + /// @dev Simulates all of the transfers for each given order and returns the indices of each first failed transfer. + /// @param orders Array of orders to individually simulate transfers for. + /// @param takerAddresses Array of addresses of takers that will fill each order. + /// @param takerAssetFillAmounts Array of amounts of takerAsset that will be filled for each order. + /// @return The indices of the first failed transfer (or 4 if all transfers are successful) for each order. + function getSimulatedOrdersTransferResults( + LibOrder.Order[] memory orders, + address[] memory takerAddresses, + uint256[] memory takerAssetFillAmounts + ) + public + returns (OrderTransferResults[] memory orderTransferResults) + { + uint256 length = orders.length; + orderTransferResults = new OrderTransferResults[](length); + for (uint256 i = 0; i != length; i++) { + orderTransferResults[i] = getSimulatedOrderTransferResults( + orders[i], + takerAddresses[i], + takerAssetFillAmounts[i] + ); + } + return orderTransferResults; + } +} diff --git a/contracts/dev-utils/package.json b/contracts/dev-utils/package.json index 996b786e70..56c57cc0bf 100644 --- a/contracts/dev-utils/package.json +++ b/contracts/dev-utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(DevUtils|LibAssetData|LibTransactionDecoder).json", + "abis": "./generated-artifacts/@(DevUtils|LibAssetData|LibTransactionDecoder|OrderTransferSimulationUtils).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/dev-utils/src/artifacts.ts b/contracts/dev-utils/src/artifacts.ts index 022784e6dc..8356ce7563 100644 --- a/contracts/dev-utils/src/artifacts.ts +++ b/contracts/dev-utils/src/artifacts.ts @@ -8,8 +8,10 @@ import { ContractArtifact } from 'ethereum-types'; import * as DevUtils from '../generated-artifacts/DevUtils.json'; import * as LibAssetData from '../generated-artifacts/LibAssetData.json'; import * as LibTransactionDecoder from '../generated-artifacts/LibTransactionDecoder.json'; +import * as OrderTransferSimulationUtils from '../generated-artifacts/OrderTransferSimulationUtils.json'; export const artifacts = { DevUtils: DevUtils as ContractArtifact, - LibTransactionDecoder: LibTransactionDecoder as ContractArtifact, LibAssetData: LibAssetData as ContractArtifact, + LibTransactionDecoder: LibTransactionDecoder as ContractArtifact, + OrderTransferSimulationUtils: OrderTransferSimulationUtils as ContractArtifact, }; diff --git a/contracts/dev-utils/src/wrappers.ts b/contracts/dev-utils/src/wrappers.ts index e07c65436d..13c6397a9d 100644 --- a/contracts/dev-utils/src/wrappers.ts +++ b/contracts/dev-utils/src/wrappers.ts @@ -6,3 +6,4 @@ export * from '../generated-wrappers/dev_utils'; export * from '../generated-wrappers/lib_asset_data'; export * from '../generated-wrappers/lib_transaction_decoder'; +export * from '../generated-wrappers/order_transfer_simulation_utils'; diff --git a/contracts/dev-utils/test/order_validation_utils.ts b/contracts/dev-utils/test/order_validation_utils.ts index 725e4ccbe2..0e08a28b82 100644 --- a/contracts/dev-utils/test/order_validation_utils.ts +++ b/contracts/dev-utils/test/order_validation_utils.ts @@ -20,7 +20,7 @@ import { } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, orderHashUtils } from '@0x/order-utils'; -import { SignedOrder } from '@0x/types'; +import { OrderTransferResults, SignedOrder } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import * as chai from 'chai'; @@ -30,7 +30,7 @@ chaiSetup.configure(); const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); -describe('OrderValidationUtils', () => { +describe('OrderValidationUtils/OrderTransferSimulatorUtils', () => { let makerAddress: string; let takerAddress: string; let owner: string; @@ -412,6 +412,7 @@ describe('OrderValidationUtils', () => { }); describe('getOrderRelevantStates', async () => { it('should return the correct information for multiple orders', async () => { + signedOrder = await orderFactory.newSignedOrderAsync(); await erc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerAssetAmount); await erc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerAssetAmount, { from: makerAddress, @@ -443,5 +444,149 @@ describe('OrderValidationUtils', () => { expect(isValidSignature[1]).to.equal(false); }); }); + describe('getSimulatedOrderTransferResults', () => { + beforeEach(async () => { + signedOrder = await orderFactory.newSignedOrderAsync(); + }); + it('should return TakerAssetDataFailed if the takerAsset transfer fails', async () => { + const orderTransferResults = await devUtils.getSimulatedOrderTransferResults.callAsync( + signedOrder, + takerAddress, + signedOrder.takerAssetAmount, + ); + expect(orderTransferResults).to.equal(OrderTransferResults.TakerAssetDataFailed); + }); + it('should return MakerAssetDataFailed if the makerAsset transfer fails', async () => { + await erc20Token2.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerAssetAmount, { + from: owner, + }); + await erc20Token2.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerAssetAmount, { + from: takerAddress, + }); + const orderTransferResults = await devUtils.getSimulatedOrderTransferResults.callAsync( + signedOrder, + takerAddress, + signedOrder.takerAssetAmount, + ); + expect(orderTransferResults).to.equal(OrderTransferResults.MakerAssetDataFailed); + }); + it('should return TakerFeeAssetDataFailed if the takerFeeAsset transfer fails', async () => { + await erc20Token2.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerAssetAmount, { + from: owner, + }); + await erc20Token2.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerAssetAmount, { + from: takerAddress, + }); + await erc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerAssetAmount, { + from: owner, + }); + await erc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerAssetAmount, { + from: makerAddress, + }); + const orderTransferResults = await devUtils.getSimulatedOrderTransferResults.callAsync( + signedOrder, + takerAddress, + signedOrder.takerAssetAmount, + ); + expect(orderTransferResults).to.equal(OrderTransferResults.TakerFeeAssetDataFailed); + }); + it('should return MakerFeeAssetDataFailed if the makerFeeAsset transfer fails', async () => { + await erc20Token2.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerAssetAmount, { + from: owner, + }); + await erc20Token2.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerAssetAmount, { + from: takerAddress, + }); + await erc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerAssetAmount, { + from: owner, + }); + await erc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerAssetAmount, { + from: makerAddress, + }); + await feeErc20Token.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerFee, { + from: owner, + }); + await feeErc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerFee, { + from: takerAddress, + }); + const orderTransferResults = await devUtils.getSimulatedOrderTransferResults.callAsync( + signedOrder, + takerAddress, + signedOrder.takerAssetAmount, + ); + expect(orderTransferResults).to.equal(OrderTransferResults.MakerFeeAssetDataFailed); + }); + it('should return TransfersSuccessful if all transfers succeed', async () => { + await erc20Token2.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerAssetAmount, { + from: owner, + }); + await erc20Token2.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerAssetAmount, { + from: takerAddress, + }); + await erc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerAssetAmount, { + from: owner, + }); + await erc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerAssetAmount, { + from: makerAddress, + }); + await feeErc20Token.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerFee, { + from: owner, + }); + await feeErc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerFee, { + from: takerAddress, + }); + await feeErc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerFee, { + from: owner, + }); + await feeErc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerFee, { + from: makerAddress, + }); + const orderTransferResults = await devUtils.getSimulatedOrderTransferResults.callAsync( + signedOrder, + takerAddress, + signedOrder.takerAssetAmount, + ); + expect(orderTransferResults).to.equal(OrderTransferResults.TransfersSuccessful); + }); + }); + describe('getSimulatedOrdersTransferResults', async () => { + it('should simulate the transfers of each order independently from one another', async () => { + // Set balances and allowances to exactly enough to fill a single order + await erc20Token2.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerAssetAmount, { + from: owner, + }); + await erc20Token2.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerAssetAmount, { + from: takerAddress, + }); + await erc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerAssetAmount, { + from: owner, + }); + await erc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerAssetAmount, { + from: makerAddress, + }); + await feeErc20Token.setBalance.awaitTransactionSuccessAsync(takerAddress, signedOrder.takerFee, { + from: owner, + }); + await feeErc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.takerFee, { + from: takerAddress, + }); + await feeErc20Token.setBalance.awaitTransactionSuccessAsync(makerAddress, signedOrder.makerFee, { + from: owner, + }); + await feeErc20Token.approve.awaitTransactionSuccessAsync(erc20Proxy.address, signedOrder.makerFee, { + from: makerAddress, + }); + const [ + orderTransferResults1, + orderTransferResults2, + ] = await devUtils.getSimulatedOrdersTransferResults.callAsync( + [signedOrder, signedOrder], + [takerAddress, takerAddress], + [signedOrder.takerAssetAmount, signedOrder.takerAssetAmount], + ); + expect(orderTransferResults1).to.equal(OrderTransferResults.TransfersSuccessful); + expect(orderTransferResults2).to.equal(OrderTransferResults.TransfersSuccessful); + }); + }); }); // tslint:disable:max-file-line-count diff --git a/contracts/dev-utils/tsconfig.json b/contracts/dev-utils/tsconfig.json index 7a4c9021ed..c44c01df21 100644 --- a/contracts/dev-utils/tsconfig.json +++ b/contracts/dev-utils/tsconfig.json @@ -5,7 +5,8 @@ "files": [ "generated-artifacts/DevUtils.json", "generated-artifacts/LibAssetData.json", - "generated-artifacts/LibTransactionDecoder.json" + "generated-artifacts/LibTransactionDecoder.json", + "generated-artifacts/OrderTransferSimulationUtils.json" ], "exclude": ["./deploy/solc/solc_bin"] } diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index e4845efbda..2f41bef7d0 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -73,6 +73,10 @@ { "note": "Return a FillResults array for batch fill variants", "pr": 1834 + }, + { + "note": "Add `MixinTransferSimulator` contract for simulating multiple transfers on-chain", + "pr": 1868 } ] }, diff --git a/contracts/exchange/contracts/src/Exchange.sol b/contracts/exchange/contracts/src/Exchange.sol index d0481f31bb..0479590a03 100644 --- a/contracts/exchange/contracts/src/Exchange.sol +++ b/contracts/exchange/contracts/src/Exchange.sol @@ -22,6 +22,7 @@ pragma experimental ABIEncoderV2; import "./MixinMatchOrders.sol"; import "./MixinSignatureValidator.sol"; import "./MixinWrapperFunctions.sol"; +import "./MixinTransferSimulator.sol"; // solhint-disable no-empty-blocks @@ -31,7 +32,8 @@ import "./MixinWrapperFunctions.sol"; contract Exchange is MixinSignatureValidator, MixinMatchOrders, - MixinWrapperFunctions + MixinWrapperFunctions, + MixinTransferSimulator { string constant public VERSION = "3.0.0"; @@ -45,6 +47,8 @@ contract Exchange is MixinSignatureValidator() MixinTransactions() MixinAssetProxyDispatcher() + MixinTransferSimulator() MixinWrapperFunctions() + MixinExchangeRichErrors() {} } diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index 063c247762..efa8cd6d20 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -519,16 +519,7 @@ contract MixinExchangeCore is order.makerAddress, fillResults.takerAssetFilledAmount ); - if (order.makerAddress != order.feeRecipientAddress) { - // Transfer maker fee -> feeRecipient - _dispatchTransferFrom( - orderHash, - order.makerFeeAssetData, - order.makerAddress, - order.feeRecipientAddress, - fillResults.makerFeePaid - ); - } + // Transfer maker -> taker _dispatchTransferFrom( orderHash, @@ -537,15 +528,23 @@ contract MixinExchangeCore is takerAddress, fillResults.makerAssetFilledAmount ); - if (takerAddress != order.feeRecipientAddress) { - // Transfer taker fee -> feeRecipient - _dispatchTransferFrom( - orderHash, - order.takerFeeAssetData, - takerAddress, - order.feeRecipientAddress, - fillResults.takerFeePaid - ); - } + + // Transfer taker fee -> feeRecipient + _dispatchTransferFrom( + orderHash, + order.takerFeeAssetData, + takerAddress, + order.feeRecipientAddress, + fillResults.takerFeePaid + ); + + // Transfer maker fee -> feeRecipient + _dispatchTransferFrom( + orderHash, + order.makerFeeAssetData, + order.makerAddress, + order.feeRecipientAddress, + fillResults.makerFeePaid + ); } } diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index e82432a534..ff206d8ab5 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -291,16 +291,7 @@ contract MixinMatchOrders is leftOrder.makerAddress, matchedFillResults.left.takerAssetFilledAmount ); - if (leftOrder.makerAddress != leftFeeRecipientAddress) { - // Left maker fee -> left fee recipient - _dispatchTransferFrom( - leftOrderHash, - leftOrder.makerFeeAssetData, - leftOrder.makerAddress, - leftFeeRecipientAddress, - matchedFillResults.left.makerFeePaid - ); - } + // Left maker asset -> right maker _dispatchTransferFrom( leftOrderHash, @@ -309,16 +300,24 @@ contract MixinMatchOrders is rightOrder.makerAddress, matchedFillResults.right.takerAssetFilledAmount ); - if (rightOrder.makerAddress != rightFeeRecipientAddress) { - // Right maker fee -> right fee recipient - _dispatchTransferFrom( - rightOrderHash, - rightOrder.makerFeeAssetData, - rightOrder.makerAddress, - rightFeeRecipientAddress, - matchedFillResults.right.makerFeePaid - ); - } + + // Right maker fee -> right fee recipient + _dispatchTransferFrom( + rightOrderHash, + rightOrder.makerFeeAssetData, + rightOrder.makerAddress, + rightFeeRecipientAddress, + matchedFillResults.right.makerFeePaid + ); + + // Left maker fee -> left fee recipient + _dispatchTransferFrom( + leftOrderHash, + leftOrder.makerFeeAssetData, + leftOrder.makerAddress, + leftFeeRecipientAddress, + matchedFillResults.left.makerFeePaid + ); // Settle taker profits. _dispatchTransferFrom( @@ -336,39 +335,34 @@ contract MixinMatchOrders is ) { // Fee recipients and taker fee assets are identical, so we can // transfer them in one go. - if (takerAddress != leftFeeRecipientAddress) { - _dispatchTransferFrom( - leftOrderHash, - leftOrder.takerFeeAssetData, - takerAddress, - leftFeeRecipientAddress, - _safeAdd( - matchedFillResults.left.takerFeePaid, - matchedFillResults.right.takerFeePaid - ) - ); - } - } else { - if (takerAddress != leftFeeRecipientAddress) { - // taker fee -> left fee recipient - _dispatchTransferFrom( - leftOrderHash, - leftOrder.takerFeeAssetData, - takerAddress, - leftFeeRecipientAddress, - matchedFillResults.left.takerFeePaid - ); - } - if (takerAddress != rightFeeRecipientAddress) { - // taker fee -> right fee recipient - _dispatchTransferFrom( - rightOrderHash, - rightOrder.takerFeeAssetData, - takerAddress, - rightFeeRecipientAddress, + _dispatchTransferFrom( + leftOrderHash, + leftOrder.takerFeeAssetData, + takerAddress, + leftFeeRecipientAddress, + _safeAdd( + matchedFillResults.left.takerFeePaid, matchedFillResults.right.takerFeePaid - ); - } + ) + ); + } else { + // Right taker fee -> right fee recipient + _dispatchTransferFrom( + rightOrderHash, + rightOrder.takerFeeAssetData, + takerAddress, + rightFeeRecipientAddress, + matchedFillResults.right.takerFeePaid + ); + + // Left taker fee -> left fee recipient + _dispatchTransferFrom( + leftOrderHash, + leftOrder.takerFeeAssetData, + takerAddress, + leftFeeRecipientAddress, + matchedFillResults.left.takerFeePaid + ); } } } diff --git a/contracts/exchange/contracts/src/MixinTransferSimulator.sol b/contracts/exchange/contracts/src/MixinTransferSimulator.sol new file mode 100644 index 0000000000..e517916413 --- /dev/null +++ b/contracts/exchange/contracts/src/MixinTransferSimulator.sol @@ -0,0 +1,60 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; +pragma experimental ABIEncoderV2; + +import "./MixinAssetProxyDispatcher.sol"; + + +contract MixinTransferSimulator is + MixinAssetProxyDispatcher +{ + /// @dev This function may be used to simulate any amount of transfers + /// As they would occur through the Exchange contract. Note that this function + /// will always revert, even if all transfers are successful. However, it may + /// be used with eth_call or with a try/catch pattern in order to simulate + /// the results of the transfers. + /// @param assetData Array of asset details, each encoded per the AssetProxy contract specification. + /// @param fromAddresses Array containing the `from` addresses that correspond with each transfer. + /// @param toAddresses Array containing the `to` addresses that correspond with each transfer. + /// @param amounts Array containing the amounts that correspond to each transfer. + /// @return This function does not return a value. However, it will always revert with + /// `Error("TRANSFERS_SUCCESSFUL")` if all of the transfers were successful. + function simulateDispatchTransferFromCalls( + bytes[] memory assetData, + address[] memory fromAddresses, + address[] memory toAddresses, + uint256[] memory amounts + ) + public + { + uint256 length = assetData.length; + for (uint256 i = 0; i != length; i++) { + _dispatchTransferFrom( + // The index is passed in as `orderHash` so that a failed transfer can be quickly identified when catching the error + bytes32(i), + assetData[i], + fromAddresses[i], + toAddresses[i], + amounts[i] + ); + } + revert("TRANSFERS_SUCCESSFUL"); + } +} \ No newline at end of file diff --git a/contracts/exchange/contracts/src/interfaces/IExchange.sol b/contracts/exchange/contracts/src/interfaces/IExchange.sol index 66f32f5474..7337a4c3e0 100644 --- a/contracts/exchange/contracts/src/interfaces/IExchange.sol +++ b/contracts/exchange/contracts/src/interfaces/IExchange.sol @@ -25,6 +25,7 @@ import "./ISignatureValidator.sol"; import "./ITransactions.sol"; import "./IAssetProxyDispatcher.sol"; import "./IWrapperFunctions.sol"; +import "./ITransferSimulator.sol"; // solhint-disable no-empty-blocks @@ -34,5 +35,6 @@ contract IExchange is ISignatureValidator, ITransactions, IAssetProxyDispatcher, + ITransferSimulator, IWrapperFunctions {} diff --git a/contracts/exchange/contracts/src/interfaces/ITransferSimulator.sol b/contracts/exchange/contracts/src/interfaces/ITransferSimulator.sol new file mode 100644 index 0000000000..d01ec76fa6 --- /dev/null +++ b/contracts/exchange/contracts/src/interfaces/ITransferSimulator.sol @@ -0,0 +1,43 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; +pragma experimental ABIEncoderV2; + + +contract ITransferSimulator { + + /// @dev This function may be used to simulate any amount of transfers + /// As they would occur through the Exchange contract. Note that this function + /// will always revert, even if all transfers are successful. However, it may + /// be used with eth_call or with a try/catch pattern in order to simulate + /// the results of the transfers. + /// @param assetData Array of asset details, each encoded per the AssetProxy contract specification. + /// @param fromAddresses Array containing the `from` addresses that correspond with each transfer. + /// @param toAddresses Array containing the `to` addresses that correspond with each transfer. + /// @param amounts Array containing the amounts that correspond to each transfer. + /// @return This function does not return a value. However, it will always revert with + /// `Error("TRANSFERS_SUCCESSFUL")` if all of the transfers were successful. + function simulateDispatchTransferFromCalls( + bytes[] memory assetData, + address[] memory fromAddresses, + address[] memory toAddresses, + uint256[] memory amounts + ) + public; +} diff --git a/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol b/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol index 989e41a722..62eafa959b 100644 --- a/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol +++ b/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol @@ -17,12 +17,15 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; import "../src/MixinAssetProxyDispatcher.sol"; +import "../src/MixinTransferSimulator.sol"; contract TestAssetProxyDispatcher is - MixinAssetProxyDispatcher + MixinAssetProxyDispatcher, + MixinTransferSimulator { function dispatchTransferFrom( bytes32 orderHash, diff --git a/contracts/exchange/test/core.ts b/contracts/exchange/test/core.ts index 75fc6cc74c..5c5cc9c3c1 100644 --- a/contracts/exchange/test/core.ts +++ b/contracts/exchange/test/core.ts @@ -144,66 +144,39 @@ describe('Exchange core', () => { exchange.address, ); // Configure ERC20Proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(multiAssetProxy.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await erc20Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, { + from: owner, + }); + await erc20Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(multiAssetProxy.address, { + from: owner, + }); // Configure ERC721Proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await erc721Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await erc721Proxy.addAuthorizedAddress.sendTransactionAsync(multiAssetProxy.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await erc721Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, { + from: owner, + }); + await erc721Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(multiAssetProxy.address, { + from: owner, + }); // Configure ERC1155Proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await erc1155Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await erc1155Proxy.addAuthorizedAddress.sendTransactionAsync(multiAssetProxy.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await erc1155Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, { + from: owner, + }); + await erc1155Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(multiAssetProxy.address, { + from: owner, + }); // Configure MultiAssetProxy - await web3Wrapper.awaitTransactionSuccessAsync( - await multiAssetProxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await multiAssetProxy.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await multiAssetProxy.registerAssetProxy.sendTransactionAsync(erc721Proxy.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await multiAssetProxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, { + from: owner, + }); + await multiAssetProxy.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); + await multiAssetProxy.registerAssetProxy.awaitTransactionSuccessAsync(erc721Proxy.address, { + from: owner, + }); // Configure Exchange exchangeWrapper = new ExchangeWrapper(exchange, provider); @@ -277,7 +250,7 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetData: await assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), }); - await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId); + await reentrantErc20Token.setReentrantFunction.awaitTransactionSuccessAsync(functionId); const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); @@ -319,20 +292,14 @@ describe('Exchange core', () => { it('should revert if `isValidSignature` tries to update state when SignatureType=Wallet', async () => { const maliciousMakerAddress = maliciousWallet.address; - await web3Wrapper.awaitTransactionSuccessAsync( - await erc20TokenA.setBalance.sendTransactionAsync( - maliciousMakerAddress, - constants.INITIAL_ERC20_BALANCE, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + await erc20TokenA.setBalance.awaitTransactionSuccessAsync( + maliciousMakerAddress, + constants.INITIAL_ERC20_BALANCE, ); - await web3Wrapper.awaitTransactionSuccessAsync( - await maliciousWallet.approveERC20.sendTransactionAsync( - erc20TokenA.address, - erc20Proxy.address, - constants.INITIAL_ERC20_ALLOWANCE, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + await maliciousWallet.approveERC20.awaitTransactionSuccessAsync( + erc20TokenA.address, + erc20Proxy.address, + constants.INITIAL_ERC20_ALLOWANCE, ); signedOrder = await orderFactory.newSignedOrderAsync({ makerAddress: maliciousMakerAddress, @@ -352,13 +319,10 @@ describe('Exchange core', () => { it('should revert if `isValidSignature` tries to update state when SignatureType=Validator', async () => { const isApproved = true; - await web3Wrapper.awaitTransactionSuccessAsync( - await exchange.setSignatureValidatorApproval.sendTransactionAsync( - maliciousValidator.address, - isApproved, - { from: makerAddress }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + await exchange.setSignatureValidatorApproval.awaitTransactionSuccessAsync( + maliciousValidator.address, + isApproved, + { from: makerAddress }, ); signedOrder.signature = `${maliciousValidator.address}0${SignatureType.Validator}`; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); @@ -418,6 +382,167 @@ describe('Exchange core', () => { }); }); + describe('Fill transfer ordering', () => { + it('should allow the maker to exchange assets received by the taker', async () => { + // Set maker/taker assetData to the same asset + const takerAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + const takerAssetAmount = new BigNumber(1); + const makerAssetData = assetDataUtils.encodeMultiAssetData([takerAssetAmount], [takerAssetData]); + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData, + takerAssetData, + makerAssetAmount: takerAssetAmount, + takerAssetAmount, + makerFee: constants.ZERO_AMOUNT, + takerFee: constants.ZERO_AMOUNT, + }); + // Set maker balance to 0 so that the asset must be received by the taker in order for the fill to succeed + await erc20TokenA.setBalance.awaitTransactionSuccessAsync(makerAddress, constants.ZERO_AMOUNT, { + from: owner, + }); + const txReceipt = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + const logs = txReceipt.logs; + const transferLogs = _.filter( + logs, + log => (log as LogWithDecodedArgs).event === 'Transfer', + ); + expect(transferLogs.length).to.be.equal(2); + expect((transferLogs[0] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenA.address, + ); + expect((transferLogs[0] as LogWithDecodedArgs).args._from).to.be.equal( + takerAddress, + ); + expect((transferLogs[0] as LogWithDecodedArgs).args._to).to.be.equal( + makerAddress, + ); + expect( + (transferLogs[0] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.takerAssetAmount); + expect((transferLogs[1] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenA.address, + ); + expect((transferLogs[1] as LogWithDecodedArgs).args._from).to.be.equal( + makerAddress, + ); + expect((transferLogs[1] as LogWithDecodedArgs).args._to).to.be.equal( + takerAddress, + ); + expect( + (transferLogs[1] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.makerAssetAmount); + }); + it('should allow the taker to pay fees with an asset that received by the maker', async () => { + const makerAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + signedOrder = await orderFactory.newSignedOrderAsync({ + takerFeeAssetData: makerAssetData, + makerFee: constants.ZERO_AMOUNT, + takerFee: new BigNumber(1), + }); + // Set taker balance to 0 so that the asset must be received by the maker in order for the fill to succeed + await erc20TokenA.setBalance.awaitTransactionSuccessAsync(takerAddress, constants.ZERO_AMOUNT, { + from: owner, + }); + const txReceipt = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + const logs = txReceipt.logs; + const transferLogs = _.filter( + logs, + log => (log as LogWithDecodedArgs).event === 'Transfer', + ); + expect(transferLogs.length).to.be.equal(3); + expect((transferLogs[0] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenB.address, + ); + expect((transferLogs[0] as LogWithDecodedArgs).args._from).to.be.equal( + takerAddress, + ); + expect((transferLogs[0] as LogWithDecodedArgs).args._to).to.be.equal( + makerAddress, + ); + expect( + (transferLogs[0] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.takerAssetAmount); + expect((transferLogs[1] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenA.address, + ); + expect((transferLogs[1] as LogWithDecodedArgs).args._from).to.be.equal( + makerAddress, + ); + expect((transferLogs[1] as LogWithDecodedArgs).args._to).to.be.equal( + takerAddress, + ); + expect( + (transferLogs[1] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.makerAssetAmount); + expect((transferLogs[2] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenA.address, + ); + expect((transferLogs[2] as LogWithDecodedArgs).args._from).to.be.equal( + takerAddress, + ); + expect((transferLogs[2] as LogWithDecodedArgs).args._to).to.be.equal( + feeRecipientAddress, + ); + expect( + (transferLogs[2] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.takerFee); + }); + it('should allow the maker to pay fees with an asset that received by the taker', async () => { + const takerAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenB.address); + signedOrder = await orderFactory.newSignedOrderAsync({ + makerFeeAssetData: takerAssetData, + makerFee: new BigNumber(1), + takerFee: constants.ZERO_AMOUNT, + }); + // Set maker balance to 0 so that the asset must be received by the taker in order for the fill to succeed + await erc20TokenB.setBalance.awaitTransactionSuccessAsync(makerAddress, constants.ZERO_AMOUNT, { + from: owner, + }); + const txReceipt = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + const logs = txReceipt.logs; + const transferLogs = _.filter( + logs, + log => (log as LogWithDecodedArgs).event === 'Transfer', + ); + expect(transferLogs.length).to.be.equal(3); + expect((transferLogs[0] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenB.address, + ); + expect((transferLogs[0] as LogWithDecodedArgs).args._from).to.be.equal( + takerAddress, + ); + expect((transferLogs[0] as LogWithDecodedArgs).args._to).to.be.equal( + makerAddress, + ); + expect( + (transferLogs[0] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.takerAssetAmount); + expect((transferLogs[1] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenA.address, + ); + expect((transferLogs[1] as LogWithDecodedArgs).args._from).to.be.equal( + makerAddress, + ); + expect((transferLogs[1] as LogWithDecodedArgs).args._to).to.be.equal( + takerAddress, + ); + expect( + (transferLogs[1] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.makerAssetAmount); + expect((transferLogs[2] as LogWithDecodedArgs).address).to.be.equal( + erc20TokenB.address, + ); + expect((transferLogs[2] as LogWithDecodedArgs).args._from).to.be.equal( + makerAddress, + ); + expect((transferLogs[2] as LogWithDecodedArgs).args._to).to.be.equal( + feeRecipientAddress, + ); + expect( + (transferLogs[2] as LogWithDecodedArgs).args._value, + ).to.be.bignumber.equal(signedOrder.makerFee); + }); + }); describe('Testing exchange of ERC20 tokens with no return values', () => { before(async () => { noReturnErc20Token = await DummyNoReturnERC20TokenContract.deployFrom0xArtifactAsync( @@ -429,17 +554,14 @@ describe('Exchange core', () => { constants.DUMMY_TOKEN_DECIMALS, constants.DUMMY_TOKEN_TOTAL_SUPPLY, ); - await web3Wrapper.awaitTransactionSuccessAsync( - await noReturnErc20Token.setBalance.sendTransactionAsync(makerAddress, constants.INITIAL_ERC20_BALANCE), - constants.AWAIT_TRANSACTION_MINED_MS, + await noReturnErc20Token.setBalance.awaitTransactionSuccessAsync( + makerAddress, + constants.INITIAL_ERC20_BALANCE, ); - await web3Wrapper.awaitTransactionSuccessAsync( - await noReturnErc20Token.approve.sendTransactionAsync( - erc20Proxy.address, - constants.INITIAL_ERC20_ALLOWANCE, - { from: makerAddress }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + await noReturnErc20Token.approve.awaitTransactionSuccessAsync( + erc20Proxy.address, + constants.INITIAL_ERC20_ALLOWANCE, + { from: makerAddress }, ); }); it('should transfer the correct amounts when makerAssetAmount === takerAssetAmount', async () => { diff --git a/contracts/exchange/test/dispatcher.ts b/contracts/exchange/test/dispatcher.ts index 3e67628a79..befb88d601 100644 --- a/contracts/exchange/test/dispatcher.ts +++ b/contracts/exchange/test/dispatcher.ts @@ -17,8 +17,8 @@ import { } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, ExchangeRevertErrors } from '@0x/order-utils'; -import { AssetProxyId } from '@0x/types'; -import { BigNumber, OwnableRevertErrors } from '@0x/utils'; +import { AssetProxyId, RevertReason } from '@0x/types'; +import { BigNumber, OwnableRevertErrors, StringRevertError } from '@0x/utils'; import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; @@ -39,7 +39,8 @@ describe('AssetProxyDispatcher', () => { let makerAddress: string; let takerAddress: string; - let zrxToken: DummyERC20TokenContract; + let erc20TokenA: DummyERC20TokenContract; + let erc20TokenB: DummyERC20TokenContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; let assetProxyDispatcher: TestAssetProxyDispatcherContract; @@ -61,8 +62,11 @@ describe('AssetProxyDispatcher', () => { erc20Wrapper = new ERC20Wrapper(provider, usedAddresses, owner); erc721Wrapper = new ERC721Wrapper(provider, usedAddresses, owner); - const numDummyErc20ToDeploy = 1; - [zrxToken] = await erc20Wrapper.deployDummyTokensAsync(numDummyErc20ToDeploy, constants.DUMMY_TOKEN_DECIMALS); + const numDummyErc20ToDeploy = 2; + [erc20TokenA, erc20TokenB] = await erc20Wrapper.deployDummyTokensAsync( + numDummyErc20ToDeploy, + constants.DUMMY_TOKEN_DECIMALS, + ); erc20Proxy = await erc20Wrapper.deployProxyAsync(); await erc20Wrapper.setBalancesAndAllowancesAsync(); @@ -73,18 +77,12 @@ describe('AssetProxyDispatcher', () => { provider, txDefaults, ); - await web3Wrapper.awaitTransactionSuccessAsync( - await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(assetProxyDispatcher.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await erc721Proxy.addAuthorizedAddress.sendTransactionAsync(assetProxyDispatcher.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await erc20Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(assetProxyDispatcher.address, { + from: owner, + }); + await erc721Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(assetProxyDispatcher.address, { + from: owner, + }); }); beforeEach(async () => { await blockchainLifecycle.startAsync(); @@ -94,39 +92,33 @@ describe('AssetProxyDispatcher', () => { }); describe('registerAssetProxy', () => { it('should record proxy upon registration', async () => { - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20); expect(proxyAddress).to.be.equal(erc20Proxy.address); }); it('should be able to record multiple proxies', async () => { // Record first proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); let proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20); expect(proxyAddress).to.be.equal(erc20Proxy.address); // Record another proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc721Proxy.address, { - from: owner, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc721Proxy.address, { + from: owner, + }); proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC721); expect(proxyAddress).to.be.equal(erc721Proxy.address); }); it('should throw if a proxy with the same id is already registered', async () => { // Initial registration - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20); expect(proxyAddress).to.be.equal(erc20Proxy.address); // Deploy a new version of the ERC20 Transfer Proxy contract @@ -153,7 +145,9 @@ describe('AssetProxyDispatcher', () => { it('should log an event with correct arguments when an asset proxy is registered', async () => { const logDecoder = new LogDecoder(web3Wrapper, artifacts); const txReceipt = await logDecoder.getTxWithDecodedLogsAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), + await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { + from: owner, + }), ); const logs = txReceipt.logs; const log = logs[0] as LogWithDecodedArgs; @@ -164,10 +158,9 @@ describe('AssetProxyDispatcher', () => { describe('getAssetProxy', () => { it('should return correct address of registered proxy', async () => { - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20); expect(proxyAddress).to.be.equal(erc20Proxy.address); }); @@ -182,59 +175,51 @@ describe('AssetProxyDispatcher', () => { const orderHash = orderUtils.generatePseudoRandomOrderHash(); it('should dispatch transfer to registered proxy', async () => { // Register ERC20 proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); // Construct metadata for ERC20 proxy - const encodedAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); + const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); const amount = new BigNumber(10); - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.dispatchTransferFrom.sendTransactionAsync( - orderHash, - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: owner }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + await assetProxyDispatcher.dispatchTransferFrom.awaitTransactionSuccessAsync( + orderHash, + encodedAssetData, + makerAddress, + takerAddress, + amount, + { from: owner }, ); // Verify transfer was successful const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances[makerAddress][zrxToken.address]).to.be.bignumber.equal( - erc20Balances[makerAddress][zrxToken.address].minus(amount), + expect(newBalances[makerAddress][erc20TokenA.address]).to.be.bignumber.equal( + erc20Balances[makerAddress][erc20TokenA.address].minus(amount), ); - expect(newBalances[takerAddress][zrxToken.address]).to.be.bignumber.equal( - erc20Balances[takerAddress][zrxToken.address].plus(amount), + expect(newBalances[takerAddress][erc20TokenA.address]).to.be.bignumber.equal( + erc20Balances[takerAddress][erc20TokenA.address].plus(amount), ); }); it('should not dispatch a transfer if amount == 0', async () => { // Register ERC20 proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); // Construct metadata for ERC20 proxy - const encodedAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); + const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); const amount = constants.ZERO_AMOUNT; - const txReceipt = await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.dispatchTransferFrom.sendTransactionAsync( - orderHash, - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: owner }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + const txReceipt = await assetProxyDispatcher.dispatchTransferFrom.awaitTransactionSuccessAsync( + orderHash, + encodedAssetData, + makerAddress, + takerAddress, + amount, + { from: owner }, ); expect(txReceipt.logs.length).to.be.equal(0); const newBalances = await erc20Wrapper.getBalancesAsync(); @@ -243,26 +228,22 @@ describe('AssetProxyDispatcher', () => { it('should not dispatch a transfer if from == to', async () => { // Register ERC20 proxy - await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); // Construct metadata for ERC20 proxy - const encodedAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); + const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); const amount = new BigNumber(10); - const txReceipt = await web3Wrapper.awaitTransactionSuccessAsync( - await assetProxyDispatcher.dispatchTransferFrom.sendTransactionAsync( - orderHash, - encodedAssetData, - makerAddress, - makerAddress, - amount, - { from: owner }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + const txReceipt = await assetProxyDispatcher.dispatchTransferFrom.awaitTransactionSuccessAsync( + orderHash, + encodedAssetData, + makerAddress, + makerAddress, + amount, + { from: owner }, ); expect(txReceipt.logs.length).to.be.equal(0); const newBalances = await erc20Wrapper.getBalancesAsync(); @@ -271,7 +252,7 @@ describe('AssetProxyDispatcher', () => { it('should throw if dispatching to unregistered proxy', async () => { // Construct metadata for ERC20 proxy - const encodedAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); + const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(10); const expectedError = new ExchangeRevertErrors.AssetProxyDispatchError( @@ -289,6 +270,130 @@ describe('AssetProxyDispatcher', () => { ); return expect(tx).to.revertWith(expectedError); }); + + it('should should revert with the correct error when assetData length < 4 bytes', async () => { + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); + const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address).slice(0, 8); + const amount = new BigNumber(1); + const expectedError = new ExchangeRevertErrors.AssetProxyDispatchError( + ExchangeRevertErrors.AssetProxyDispatchErrorCode.InvalidAssetDataLength, + orderHash, + encodedAssetData, + ); + const tx = assetProxyDispatcher.dispatchTransferFrom.sendTransactionAsync( + orderHash, + encodedAssetData, + makerAddress, + takerAddress, + amount, + { from: owner }, + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert with the reason provided by the AssetProxy when a transfer fails', async () => { + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); + await erc20TokenA.approve.awaitTransactionSuccessAsync(erc20Proxy.address, constants.ZERO_AMOUNT, { + from: makerAddress, + }); + const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + const amount = new BigNumber(1); + const nestedError = new StringRevertError(RevertReason.TransferFailed).encode(); + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( + orderHash, + encodedAssetData, + nestedError, + ); + const tx = assetProxyDispatcher.dispatchTransferFrom.sendTransactionAsync( + orderHash, + encodedAssetData, + makerAddress, + takerAddress, + amount, + { from: owner }, + ); + return expect(tx).to.revertWith(expectedError); + }); + }); + describe('simulateDispatchTransferFromCalls', () => { + it('should revert with the information specific to the failed transfer', async () => { + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); + const assetDataA = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + const assetDataB = assetDataUtils.encodeERC20AssetData(erc20TokenB.address); + await erc20TokenB.approve.awaitTransactionSuccessAsync(erc20Proxy.address, constants.ZERO_AMOUNT, { + from: makerAddress, + }); + const transferIndexAsBytes32 = '0x0000000000000000000000000000000000000000000000000000000000000001'; + const nestedError = new StringRevertError(RevertReason.TransferFailed).encode(); + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( + transferIndexAsBytes32, + assetDataB, + nestedError, + ); + const tx = assetProxyDispatcher.simulateDispatchTransferFromCalls.sendTransactionAsync( + [assetDataA, assetDataB], + [makerAddress, makerAddress], + [takerAddress, takerAddress], + [new BigNumber(1), new BigNumber(1)], + ); + return expect(tx).to.revertWith(expectedError); + }); + it('should forward the revert reason from the underlying failed transfer', async () => { + const assetDataA = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + const assetDataB = assetDataUtils.encodeERC20AssetData(erc20TokenB.address); + const transferIndexAsBytes32 = '0x0000000000000000000000000000000000000000000000000000000000000000'; + const expectedError = new ExchangeRevertErrors.AssetProxyDispatchError( + ExchangeRevertErrors.AssetProxyDispatchErrorCode.UnknownAssetProxy, + transferIndexAsBytes32, + assetDataA, + ); + const tx = assetProxyDispatcher.simulateDispatchTransferFromCalls.sendTransactionAsync( + [assetDataA, assetDataB], + [makerAddress, makerAddress], + [takerAddress, takerAddress], + [new BigNumber(1), new BigNumber(1)], + ); + return expect(tx).to.revertWith(expectedError); + }); + it('should revert with TRANSFERS_SUCCESSFUL if no transfers fail', async () => { + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); + const assetDataA = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + const assetDataB = assetDataUtils.encodeERC20AssetData(erc20TokenB.address); + const tx = assetProxyDispatcher.simulateDispatchTransferFromCalls.sendTransactionAsync( + [assetDataA, assetDataB], + [makerAddress, makerAddress], + [takerAddress, takerAddress], + [new BigNumber(1), new BigNumber(1)], + ); + return expect(tx).to.revertWith(RevertReason.TransfersSuccessful); + }); + it('should not modify balances if all transfers are successful', async () => { + await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { + from: owner, + }); + const assetDataA = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); + const assetDataB = assetDataUtils.encodeERC20AssetData(erc20TokenB.address); + const balances = await erc20Wrapper.getBalancesAsync(); + try { + await assetProxyDispatcher.simulateDispatchTransferFromCalls.awaitTransactionSuccessAsync( + [assetDataA, assetDataB], + [makerAddress, makerAddress], + [takerAddress, takerAddress], + [new BigNumber(1), new BigNumber(1)], + ); + } catch (err) { + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(newBalances).to.deep.equal(balances); + } + }); }); }); // tslint:enable:no-unnecessary-type-assertion diff --git a/packages/types/CHANGELOG.json b/packages/types/CHANGELOG.json index 904f83521a..8d0b2b588c 100644 --- a/packages/types/CHANGELOG.json +++ b/packages/types/CHANGELOG.json @@ -29,6 +29,10 @@ { "note": "Add `expirationTimeSeconds` to `ZeroExTransaction` type", "pr": 1832 + }, + { + "note": "Add `TransfersSuccessful` revert reason and `OrderTransferResults` enum", + "pr": 1868 } ] }, diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 76130faac3..266e26badd 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -323,6 +323,7 @@ export enum RevertReason { InvalidValuesOffset = 'INVALID_VALUES_OFFSET', InvalidDataOffset = 'INVALID_DATA_OFFSET', InvalidAssetDataLength = 'INVALID_ASSET_DATA_LENGTH', + TransfersSuccessful = 'TRANSFERS_SUCCESSFUL', } export enum StatusCodes { @@ -785,3 +786,11 @@ export enum OrderStatus { FullyFilled, Cancelled, } + +export enum OrderTransferResults { + TakerAssetDataFailed, + MakerAssetDataFailed, + TakerFeeAssetDataFailed, + MakerFeeAssetDataFailed, + TransfersSuccessful, +}