Merge pull request #2101 from 0xProject/feat/3.0/cleanup

Revert to old ReentrancyGuard implementation + random cleanup

contracts/asset-proxy/contracts/src/interfaces/IAssetData.sol

@@ -43,35 +43,35 @@ interface IAssetData {
     /// @dev Function signature for encoding ERC1155 assetData.
     /// @param tokenAddress Address of ERC1155 token contract.
     /// @param tokenIds Array of ids of tokens to be transferred. 
-    /// @param tokenValues Array of values that correspond to each token id to be transferred.
+    /// @param values Array of values that correspond to each token id to be transferred.
     ///        Note that each value will be multiplied by the amount being filled in the order before transferring.
     /// @param callbackData Extra data to be passed to receiver's `onERC1155Received` callback function.
     function ERC1155Assets(
         address tokenAddress,
         uint256[] calldata tokenIds,
-        uint256[] calldata tokenValues,
+        uint256[] calldata values,
         bytes calldata callbackData
     )
         external;
 
     /// @dev Function signature for encoding MultiAsset assetData.
-    /// @param amounts Array of amounts that correspond to each asset to be transferred.
+    /// @param values Array of amounts that correspond to each asset to be transferred.
     ///        Note that each value will be multiplied by the amount being filled in the order before transferring.
     /// @param nestedAssetData Array of assetData fields that will be be dispatched to their correspnding AssetProxy contract.
     function MultiAsset(
-        uint256[] calldata amounts,
+        uint256[] calldata values,
         bytes[] calldata nestedAssetData
     )
         external;
 
     /// @dev Function signature for encoding StaticCall assetData.
-    /// @param callTarget Contract that will execute the staticcall.
-    /// @param staticCallData Data that will be executed via staticcall on the callTarget contract.
-    /// @param callResultHash Keccak256 hash of the expected staticcall return data. 
+    /// @param staticCallTargetAddress Address that will execute the staticcall.
+    /// @param staticCallData Data that will be executed via staticcall on the staticCallTargetAddress.
+    /// @param expectedReturnDataHash Keccak-256 hash of the expected staticcall return data. 
     function StaticCall(
-        address callTarget,
+        address staticCallTargetAddress,
         bytes calldata staticCallData,
-        bytes32 callResultHash
+        bytes32 expectedReturnDataHash
     )
         external;
 }

contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol

@@ -103,9 +103,9 @@ library LibExchangeRichErrors {
     bytes4 internal constant ORDER_EPOCH_ERROR_SELECTOR =
         0x4ad31275;
 
-    // bytes4(keccak256("AssetProxyExistsError(address)"))
+    // bytes4(keccak256("AssetProxyExistsError(bytes4,address)"))
     bytes4 internal constant ASSET_PROXY_EXISTS_ERROR_SELECTOR =
-        0xcc8b3b53;
+        0x11c7b720;
 
     // bytes4(keccak256("AssetProxyDispatchError(uint8,bytes32,bytes)"))
     bytes4 internal constant ASSET_PROXY_DISPATCH_ERROR_SELECTOR =
@@ -308,6 +308,14 @@ library LibExchangeRichErrors {
         return TRANSACTION_GAS_PRICE_ERROR_SELECTOR;
     }
 
+    function TransactionInvalidContextErrorSelector()
+        internal
+        pure
+        returns (bytes4)
+    {
+        return TRANSACTION_INVALID_CONTEXT_ERROR_SELECTOR;
+    }
+
     function BatchMatchOrdersError(
         BatchMatchOrdersErrorCodes errorCode
     )
@@ -488,15 +496,17 @@ library LibExchangeRichErrors {
     }
 
     function AssetProxyExistsError(
-        address proxyAddress
+        bytes4 assetProxyId,
+        address assetProxyAddress
     )
         internal
         pure
         returns (bytes memory)
     {
         return abi.encodeWithSelector(
             ASSET_PROXY_EXISTS_ERROR_SELECTOR,
-            proxyAddress
+            assetProxyId,
+            assetProxyAddress
         );
     }
 

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

@@ -46,7 +46,10 @@ contract MixinAssetProxyDispatcher is
         bytes4 assetProxyId = IAssetProxy(assetProxy).getProxyId();
         address currentAssetProxy = assetProxies[assetProxyId];
         if (currentAssetProxy != address(0)) {
-            LibRichErrors.rrevert(LibExchangeRichErrors.AssetProxyExistsError(currentAssetProxy));
+            LibRichErrors.rrevert(LibExchangeRichErrors.AssetProxyExistsError(
+                assetProxyId,
+                currentAssetProxy
+            ));
         }
 
         // Add asset proxy and log registration.

contracts/exchange/contracts/src/libs/LibExchangeRichErrorDecoder.sol

@@ -238,16 +238,18 @@ contract LibExchangeRichErrorDecoder {
 
     /// @dev Decompose an ABI-encoded AssetProxyExistsError.
     /// @param encoded ABI-encoded revert error.
-    /// @return proxyAddress The address of the asset proxy.
+    /// @return assetProxyId Id of asset proxy.
+    /// @return assetProxyAddress The address of the asset proxy.
     function decodeAssetProxyExistsError(bytes memory encoded)
         public
         pure
-        returns (address assetProxyAddress)
+        returns (
+            bytes4 assetProxyId, address assetProxyAddress)
     {
         _assertSelectorBytes(encoded, LibExchangeRichErrors.AssetProxyExistsErrorSelector());
-        (assetProxyAddress) = abi.decode(
+        (assetProxyId, assetProxyAddress) = abi.decode(
             encoded.sliceDestructive(4, encoded.length),
-            (address)
+            (bytes4, address)
         );
     }
 

contracts/exchange/contracts/test/IsolatedExchange.sol

@@ -45,7 +45,7 @@ contract IsolatedExchange is
         Exchange(1337)
     {}
 
-    /// @dev Overriden to only log arguments and revert on certain assetDatas.
+    /// @dev Overridden to only log arguments and revert on certain assetDatas.
     function _dispatchTransferFrom(
         bytes32 orderHash,
         bytes memory assetData,
@@ -69,7 +69,7 @@ contract IsolatedExchange is
         }
     }
 
-    /// @dev Overriden to simplify signature validation.
+    /// @dev Overridden to simplify signature validation.
     ///      Unfortunately, this is `view`, so it can't log arguments.
     function _isValidOrderWithHashSignature(
         LibOrder.Order memory order,

contracts/exchange/contracts/test/ReentrancyTester.sol

@@ -44,12 +44,15 @@ contract ReentrancyTester is
     {}
 
     /// @dev Calls a public function to check if it is reentrant.
+    /// Because this function uses the `nonReentrant` modifier, if
+    /// the function being called is also guarded by the `nonReentrant` modifier,
+    /// it will revert when it returns.
     function isReentrant(bytes calldata fnCallData)
         external
+        nonReentrant
         returns (bool isReentrant)
     {
-        bytes memory callData = abi.encodeWithSelector(this.testReentrantFunction.selector, fnCallData);
-        (bool didSucceed, bytes memory resultData) = address(this).delegatecall(callData);
+        (bool didSucceed, bytes memory resultData) = address(this).delegatecall(fnCallData);
         if (didSucceed) {
             isReentrant = true;
         } else {
@@ -61,18 +64,36 @@ contract ReentrancyTester is
         }
     }
 
-    /// @dev Calls a public function to check if it is reentrant.
-    /// Because this function uses the `nonReentrant` modifier, if
-    /// the function being called is also guarded by the `nonReentrant` modifier,
-    /// it will revert when it returns.
-    function testReentrantFunction(bytes calldata fnCallData)
-        external
-        nonReentrant
+    /// @dev Overridden to revert on unsuccessful fillOrder call.
+    function fillOrderNoThrow(
+        LibOrder.Order memory order,
+        uint256 takerAssetFillAmount,
+        bytes memory signature
+    )
+        public
+        returns (LibFillResults.FillResults memory fillResults)
     {
-        address(this).delegatecall(fnCallData);
+        // ABI encode calldata for `fillOrder`
+        bytes memory fillOrderCalldata = abi.encodeWithSelector(
+            IExchangeCore(address(0)).fillOrder.selector,
+            order,
+            takerAssetFillAmount,
+            signature
+        );
+
+        (bool didSucceed, bytes memory returnData) = address(this).delegatecall(fillOrderCalldata);
+        if (didSucceed) {
+            assert(returnData.length == 128);
+            fillResults = abi.decode(returnData, (LibFillResults.FillResults));
+            return fillResults;
+        }
+        // Revert and rethrow error if unsuccessful
+        assembly {
+            revert(add(returnData, 32), mload(returnData))
+        }
     }
 
-    /// @dev Overriden to always succeed.
+    /// @dev Overridden to always succeed.
     function _fillOrder(
         LibOrder.Order memory order,
         uint256 takerAssetFillAmount,
@@ -87,7 +108,7 @@ contract ReentrancyTester is
         fillResults.takerFeePaid = order.takerFee;
     }
 
-    /// @dev Overriden to always succeed.
+    /// @dev Overridden to always succeed.
     function _fillOrKillOrder(
         LibOrder.Order memory order,
         uint256 takerAssetFillAmount,
@@ -114,7 +135,7 @@ contract ReentrancyTester is
         return resultData;
     }
 
-    /// @dev Overriden to always succeed.
+    /// @dev Overridden to always succeed.
     function _batchMatchOrders(
         LibOrder.Order[] memory leftOrders,
         LibOrder.Order[] memory rightOrders,
@@ -144,7 +165,7 @@ contract ReentrancyTester is
         }
     }
 
-    /// @dev Overriden to always succeed.
+    /// @dev Overridden to always succeed.
     function _matchOrders(
         LibOrder.Order memory leftOrder,
         LibOrder.Order memory rightOrder,
@@ -169,7 +190,7 @@ contract ReentrancyTester is
         });
     }
 
-    /// @dev Overriden to do nothing.
+    /// @dev Overridden to do nothing.
     function _cancelOrder(LibOrder.Order memory order)
         internal
     {}

contracts/exchange/test/core.ts

@@ -618,12 +618,7 @@ blockchainTests.resets('Exchange core', () => {
                     salt: new BigNumber(3),
                 }),
             ];
-            await exchangeWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress, {
-                // HACK(albrow): We need to hardcode the gas estimate here because
-                // the Geth gas estimator doesn't work with the way we use
-                // delegatecall and swallow errors.
-                gas: 600000,
-            });
+            await exchangeWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress);
 
             const newBalances = await erc20Wrapper.getBalancesAsync();
             const fillMakerAssetAmount = signedOrders[2].makerAssetAmount.plus(signedOrders[3].makerAssetAmount);

contracts/exchange/test/dispatcher.ts

@@ -131,7 +131,7 @@ describe('AssetProxyDispatcher', () => {
                 txDefaults,
                 dependencyArtifacts,
             );
-            const expectedError = new ExchangeRevertErrors.AssetProxyExistsError(proxyAddress);
+            const expectedError = new ExchangeRevertErrors.AssetProxyExistsError(AssetProxyId.ERC20, proxyAddress);
             const tx = assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(newErc20TransferProxy.address, {
                 from: owner,
             });

contracts/exchange/test/lib_exchange_rich_error_decoder.ts

@@ -14,6 +14,7 @@ import * as _ from 'lodash';
 import { artifacts, TestLibExchangeRichErrorDecoderContract } from '../src';
 
 blockchainTests.resets('LibExchangeRichErrorDecoder', ({ provider, txDefaults }) => {
+    const ASSET_PROXY_ID_LENGTH = 4;
     const SIGNATURE_LENGTH = 66;
     const ASSET_DATA_LENGTH = 36;
     const ERROR_DATA_LENGTH = 100;
@@ -103,7 +104,10 @@ blockchainTests.resets('LibExchangeRichErrorDecoder', ({ provider, txDefaults })
 
     (() => {
         const assetProxyAddress = addressUtils.generatePseudoRandomAddress();
-        createDecodeTest(ExchangeRevertErrors.AssetProxyExistsError, [assetProxyAddress]);
+        createDecodeTest(ExchangeRevertErrors.AssetProxyExistsError, [
+            hexRandom(ASSET_PROXY_ID_LENGTH),
+            assetProxyAddress,
+        ]);
     })();
 
     (() => {

contracts/exchange/test/wrapper.ts

@@ -223,10 +223,6 @@ blockchainTests.resets('Exchange wrappers', env => {
             );
             await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress, {
                 takerAssetFillAmount,
-                // HACK(albrow): We need to hardcode the gas estimate here because
-                // the Geth gas estimator doesn't work with the way we use
-                // delegatecall and swallow errors.
-                gas: 250000,
             });
             const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -450,10 +446,6 @@ blockchainTests.resets('Exchange wrappers', env => {
             );
             await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress, {
                 takerAssetFillAmount,
-                // HACK(albrow): We need to hardcode the gas estimate here because
-                // the Geth gas estimator doesn't work with the way we use
-                // delegatecall and swallow errors.
-                gas: 280000,
             });
 
             // Verify post-conditions
@@ -688,10 +680,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress, {
                     takerAssetFillAmounts,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 600000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -763,10 +751,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.batchFillOrdersNoThrowAsync(newOrders, takerAddress, {
                     takerAssetFillAmounts,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 450000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -789,10 +773,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, {
                     takerAssetFillAmount,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 6000000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -864,10 +844,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, {
                     takerAssetFillAmount,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 600000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -938,10 +914,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, {
                     takerAssetFillAmount,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 600000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -985,10 +957,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, {
                     makerAssetFillAmount,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 600000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -1060,10 +1028,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, {
                     makerAssetFillAmount,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 600000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
@@ -1135,10 +1099,6 @@ blockchainTests.resets('Exchange wrappers', env => {
                 );
                 await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, {
                     makerAssetFillAmount,
-                    // HACK(albrow): We need to hardcode the gas estimate here because
-                    // the Geth gas estimator doesn't work with the way we use
-                    // delegatecall and swallow errors.
-                    gas: 600000,
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();