diff --git a/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol b/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol index ab03171884..9012eab99a 100644 --- a/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol +++ b/contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol @@ -31,8 +31,6 @@ import "@0x/contracts-utils/contracts/src/LibBytes.sol"; contract OrderTransferSimulationUtils is LibExchangeRichErrorDecoder { - bool second; - using LibBytes for bytes; enum OrderTransferResults { @@ -69,12 +67,11 @@ contract OrderTransferSimulationUtils is public returns (OrderTransferResults orderTransferResults) { - LibFillResults.FillResults memory fillResults = LibFillResults.calculateFillResultsTrace( + LibFillResults.FillResults memory fillResults = LibFillResults.calculateFillResults( order, takerAssetFillAmount, _EXCHANGE.protocolFeeMultiplier(), - tx.gasprice, - second + tx.gasprice ); bytes[] memory assetData = new bytes[](2); diff --git a/contracts/dev-utils/contracts/src/OrderValidationUtils.sol b/contracts/dev-utils/contracts/src/OrderValidationUtils.sol index 7f18035de6..080fd94db0 100644 --- a/contracts/dev-utils/contracts/src/OrderValidationUtils.sol +++ b/contracts/dev-utils/contracts/src/OrderValidationUtils.sol @@ -16,7 +16,7 @@ */ -pragma solidity ^0.5.5; +pragma solidity ^0.5.9; pragma experimental ABIEncoderV2; import "@0x/contracts-exchange/contracts/src/interfaces/IExchange.sol"; @@ -127,6 +127,11 @@ contract OrderValidationUtils is fillableTakerAssetAmount ) == OrderTransferResults.TransfersSuccessful ? fillableTakerAssetAmount : 0; + /* + // Perform taker asset validation + fillableTakerAssetAmount = _isTakerAssetDataValid(order) ? fillableTakerAssetAmount : 0; + */ + return (orderInfo, fillableTakerAssetAmount, isValidSignature); } @@ -154,10 +159,6 @@ contract OrderValidationUtils is isValidSignature = new bool[](length); for (uint256 i = 0; i != length; i++) { - if (i == 1) { - second = true; - } - (ordersInfo[i], fillableTakerAssetAmounts[i], isValidSignature[i]) = getOrderRelevantState( orders[i], signatures[i] @@ -184,4 +185,49 @@ contract OrderValidationUtils is transferableAssetAmount = LibSafeMath.min256(balance, allowance); return transferableAssetAmount; } + + /// @dev This function handles the edge cases around taker validation. This function + /// currently attempts to find duplicate ERC721 token's in the taker + /// multiAssetData. + /// @param order The order that should be validated. + /// @return Whether or not the order should be considered valid. + function _isTakerAssetDataValid(LibOrder.Order memory order) + internal + pure + returns (bool) + { + bytes memory takerAssetData = order.takerAssetData; + + // Only process the taker asset data if it is multiAssetData. + bytes4 assetProxyId = takerAssetData.readBytes4(0); + if (assetProxyId != IAssetData(address(0)).MultiAsset.selector) { + return true; + } + + // Get array of values and array of assetDatas + (, uint256[] memory assetAmounts, bytes[] memory nestedAssetData) = decodeMultiAssetData(takerAssetData); + + uint256 length = nestedAssetData.length; + for (uint256 i = 0; i != length; i++) { + // NOTE(jalextowle): As an optimization, we will break out of this function + // as soon as it is determined that there are no possible ERC721 duplicates. + bool hasSeenERC721 = false; + + bytes4 nestedAssetProxyId = nestedAssetData[i].readBytes4(0); + if (nestedAssetProxyId == IAssetData(address(0)).ERC721Token.selector) { + hasSeenERC721 = true; + for (uint256 j = i; j != length; j++) { + if (nestedAssetData[i].equals(nestedAssetData[j])) { + return false; + } + } + } + + if (!hasSeenERC721) { + break; + } + } + + return true; + } } diff --git a/contracts/exchange-libs/contracts/src/LibFillResults.sol b/contracts/exchange-libs/contracts/src/LibFillResults.sol index 5e2e2ad768..9f151dd521 100644 --- a/contracts/exchange-libs/contracts/src/LibFillResults.sol +++ b/contracts/exchange-libs/contracts/src/LibFillResults.sol @@ -49,51 +49,6 @@ library LibFillResults { uint256 profitInRightMakerAsset; // Profit taken from the right maker } - event TestEvent(string file_name, string value_name, uint256 value); - - /// FIXME: Remove this function after investigation - /// @dev Calculates amounts filled and fees paid by maker and taker. - /// @param order to be filled. - /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. - /// @param protocolFeeMultiplier The current protocol fee of the exchange contract. - /// @param gasPrice The gasprice of the transaction. This is provided so that the function call can continue - /// to be pure rather than view. - /// @return fillResults Amounts filled and fees paid by maker and taker. - function calculateFillResultsTrace( - LibOrder.Order memory order, - uint256 takerAssetFilledAmount, - uint256 protocolFeeMultiplier, - uint256 gasPrice, - bool shouldTrace - ) - internal - pure - returns (FillResults memory fillResults) - { - // Compute proportional transfer amounts - fillResults.takerAssetFilledAmount = takerAssetFilledAmount; - fillResults.makerAssetFilledAmount = LibMath.safeGetPartialAmountFloor( - takerAssetFilledAmount, - order.takerAssetAmount, - order.makerAssetAmount - ); - fillResults.makerFeePaid = LibMath.safeGetPartialAmountFloor( - takerAssetFilledAmount, - order.takerAssetAmount, - order.makerFee - ); - fillResults.takerFeePaid = LibMath.safeGetPartialAmountFloor( - takerAssetFilledAmount, - order.takerAssetAmount, - order.takerFee - ); - - // Compute the protocol fee that should be paid for a single fill. - fillResults.protocolFeePaid = gasPrice.safeMul(protocolFeeMultiplier); - - return fillResults; - } - /// @dev Calculates amounts filled and fees paid by maker and taker. /// @param order to be filled. /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. diff --git a/contracts/integrations/test/dev-utils/order_validation_utils.ts b/contracts/integrations/test/dev-utils/order_validation_utils.ts index 0f90bb1de5..20750629f0 100644 --- a/contracts/integrations/test/dev-utils/order_validation_utils.ts +++ b/contracts/integrations/test/dev-utils/order_validation_utils.ts @@ -22,6 +22,7 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils', let feeErc20Token: DummyERC20TokenContract; let erc20Proxy: ERC20ProxyContract; let exchange: ExchangeContract; + let deployment: DeploymentManager; let erc20AssetData: string; let erc20AssetData2: string; @@ -33,7 +34,7 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils', before(async () => { [takerAddress, owner] = await env.getAccountAddressesAsync(); - const deployment = await DeploymentManager.deployAsync(env, { + deployment = await DeploymentManager.deployAsync(env, { numErc20TokensToDeploy: 3, numErc721TokensToDeploy: 1, owner, @@ -60,9 +61,6 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils', feeRecipientAddress: constants.NULL_ADDRESS, }, }); - /* - await Promise.all(deployment.tokens.erc20.map(async token => maker.configureERC20TokenAsync(token))); - */ const [tokenID] = await maker.configureERC721TokenAsync(deployment.tokens.erc721[0]); erc721AssetData = assetDataUtils.encodeERC721AssetData(deployment.tokens.erc721[0].address, tokenID); }); @@ -165,11 +163,35 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils', .callAsync(); expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT); }); - it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in a multi-asset proxy order', async () => { + it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in the maker side of a multi-asset proxy order', async () => { const multiAssetData = await devUtils .encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc721AssetData, erc721AssetData]) .callAsync(); - signedOrder = await maker.signOrderAsync({ makerAssetData: multiAssetData }); + signedOrder = await maker.signOrderAsync({ + makerAssetData: multiAssetData, + makerAssetAmount: new BigNumber(1), + takerAssetData: erc721AssetData, + takerAssetAmount: new BigNumber(1), + makerFee: constants.ZERO_AMOUNT, + takerFee: constants.ZERO_AMOUNT, + }); + const [, fillableTakerAssetAmount] = await devUtils + .getOrderRelevantState(signedOrder, signedOrder.signature) + .callAsync(); + expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT); + }); + it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in the taker side of a multi-asset proxy order', async () => { + const multiAssetData = await devUtils + .encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc721AssetData, erc721AssetData]) + .callAsync(); + signedOrder = await maker.signOrderAsync({ + makerAssetData: erc721AssetData, + makerAssetAmount: new BigNumber(1), + takerAssetData: multiAssetData, + takerAssetAmount: new BigNumber(1), + makerFee: constants.ZERO_AMOUNT, + takerFee: constants.ZERO_AMOUNT, + }); const [, fillableTakerAssetAmount] = await devUtils .getOrderRelevantState(signedOrder, signedOrder.signature) .callAsync();