[3.0] Exchange Rich Reverts (once more, with feeling!)

contracts/exchange/CHANGELOG.json

@@ -17,6 +17,10 @@
             {
                 "note": "Refactor example contracts that use `executeTransaction`",
                 "pr": 1753
+            },
+            {
+                "note": "Upgrade all string reverts to rich reverts",
+                "pr": 1761
             }
         ]
     },

contracts/exchange/contracts/src/Exchange.sol

@@ -26,6 +26,7 @@ import "./MixinWrapperFunctions.sol";
 import "./MixinAssetProxyDispatcher.sol";
 import "./MixinTransactions.sol";
 import "./MixinMatchOrders.sol";
+import "./MixinExchangeRichErrors.sol";
 
 
 // solhint-disable no-empty-blocks
@@ -35,7 +36,8 @@ contract Exchange is
     MixinSignatureValidator,
     MixinTransactions,
     MixinAssetProxyDispatcher,
-    MixinWrapperFunctions
+    MixinWrapperFunctions,
+    MixinExchangeRichErrors
 {
     string constant public VERSION = "3.0.0";
 

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

@@ -20,12 +20,14 @@ pragma solidity ^0.5.5;
 
 import "@0x/contracts-utils/contracts/src/Ownable.sol";
 import "./mixins/MAssetProxyDispatcher.sol";
+import "./mixins/MExchangeRichErrors.sol";
 import "./interfaces/IAssetProxy.sol";
 
 
 contract MixinAssetProxyDispatcher is
     Ownable,
-    MAssetProxyDispatcher
+    MAssetProxyDispatcher,
+    MExchangeRichErrors
 {
     // Mapping from Asset Proxy Id's to their respective Asset Proxy
     mapping (bytes4 => address) public assetProxies;
@@ -40,10 +42,9 @@ contract MixinAssetProxyDispatcher is
         // Ensure that no asset proxy exists with current id.
         bytes4 assetProxyId = IAssetProxy(assetProxy).getProxyId();
         address currentAssetProxy = assetProxies[assetProxyId];
-        require(
-            currentAssetProxy == address(0),
-            "ASSET_PROXY_ALREADY_EXISTS"
-        );
+        if (currentAssetProxy != address(0)) {
+            rrevert(AssetProxyExistsError(currentAssetProxy));
+        }
 
         // Add asset proxy and log registration.
         assetProxies[assetProxyId] = assetProxy;
@@ -65,11 +66,13 @@ contract MixinAssetProxyDispatcher is
     }
 
     /// @dev Forwards arguments to assetProxy and calls `transferFrom`. Either succeeds or throws.
+    /// @param orderHash Hash of the order associated with this transfer.
     /// @param assetData Byte array encoded for the asset.
     /// @param from Address to transfer token from.
     /// @param to Address to transfer token to.
     /// @param amount Amount of token to transfer.
     function dispatchTransferFrom(
+        bytes32 orderHash,
         bytes memory assetData,
         address from,
         address to,
@@ -80,11 +83,14 @@ contract MixinAssetProxyDispatcher is
         // Do nothing if no amount should be transferred.
         if (amount > 0 && from != to) {
             // Ensure assetData length is valid
-            require(
-                assetData.length > 3,
-                "LENGTH_GREATER_THAN_3_REQUIRED"
-            );
-
+            if (assetData.length <= 3) {
+                rrevert(AssetProxyDispatchError(
+                    orderHash,
+                    assetData,
+                    AssetProxyDispatchErrorCodes.INVALID_ASSET_DATA_LENGTH
+                ));
+            }
+
             // Lookup assetProxy. We do not use `LibBytes.readBytes4` for gas efficiency reasons.
             bytes4 assetProxyId;
             assembly {
@@ -96,14 +102,22 @@ contract MixinAssetProxyDispatcher is
             address assetProxy = assetProxies[assetProxyId];
 
             // Ensure that assetProxy exists
-            require(
-                assetProxy != address(0),
-                "ASSET_PROXY_DOES_NOT_EXIST"
-            );
-
+            if (assetProxy == address(0)) {
+                rrevert(AssetProxyDispatchError(
+                    orderHash,
+                    assetData,
+                    AssetProxyDispatchErrorCodes.UNKNOWN_ASSET_PROXY
+                ));
+            }
+
+            // Whether the AssetProxy transfer succeeded.
+            bool didSucceed;
+            // On failure, the revert message returned by the asset proxy.
+            bytes memory revertMessage;
+
             // We construct calldata for the `assetProxy.transferFrom` ABI.
             // The layout of this calldata is in the table below.
-            // 
+            //
             // | Area     | Offset | Length  | Contents                                    |
             // | -------- |--------|---------|-------------------------------------------- |
             // | Header   | 0      | 4       | function selector                           |
@@ -120,19 +134,21 @@ contract MixinAssetProxyDispatcher is
                 /////// Setup State ///////
                 // `cdStart` is the start of the calldata for `assetProxy.transferFrom` (equal to free memory ptr).
                 let cdStart := mload(64)
+                // We reserve 288 bytes from `cdStart` because we'll reuse it later for return data.
+                mstore(64, add(cdStart, 288))
                 // `dataAreaLength` is the total number of words needed to store `assetData`
                 //  As-per the ABI spec, this value is padded up to the nearest multiple of 32,
                 //  and includes 32-bytes for length.
                 let dataAreaLength := and(add(mload(assetData), 63), 0xFFFFFFFFFFFE0)
                 // `cdEnd` is the end of the calldata for `assetProxy.transferFrom`.
                 let cdEnd := add(cdStart, add(132, dataAreaLength))
 
-                
+
                 /////// Setup Header Area ///////
                 // This area holds the 4-byte `transferFromSelector`.
                 // bytes4(keccak256("transferFrom(bytes,address,address,uint256)")) = 0xa85e59e4
                 mstore(cdStart, 0xa85e59e400000000000000000000000000000000000000000000000000000000)
-                
+
                 /////// Setup Params Area ///////
                 // Each parameter is padded to 32-bytes. The entire Params Area is 128 bytes.
                 // Notes:
@@ -142,31 +158,69 @@ contract MixinAssetProxyDispatcher is
                 mstore(add(cdStart, 36), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
                 mstore(add(cdStart, 68), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
                 mstore(add(cdStart, 100), amount)
-                
+
                 /////// Setup Data Area ///////
                 // This area holds `assetData`.
                 let dataArea := add(cdStart, 132)
+                let assetDataArea := assetData
                 // solhint-disable-next-line no-empty-blocks
                 for {} lt(dataArea, cdEnd) {} {
-                    mstore(dataArea, mload(assetData))
+                    mstore(dataArea, mload(assetDataArea))
                     dataArea := add(dataArea, 32)
-                    assetData := add(assetData, 32)
+                    assetDataArea := add(assetDataArea, 32)
                 }
 
                 /////// Call `assetProxy.transferFrom` using the constructed calldata ///////
-                let success := call(
+                didSucceed := call(
                     gas,                    // forward all gas
                     assetProxy,             // call address of asset proxy
                     0,                      // don't send any ETH
                     cdStart,                // pointer to start of input
-                    sub(cdEnd, cdStart),    // length of input  
+                    sub(cdEnd, cdStart),    // length of input
                     cdStart,                // write output over input
-                    512                     // reserve 512 bytes for output
+                    288                     // reserve 288 bytes for output
                 )
-                if iszero(success) {
-                    revert(cdStart, returndatasize())
+
+                if iszero(didSucceed) { // Call reverted.
+                    // Attempt to to decode standard revert messages with the
+                    // signature `Error(string)` so we can upgrade them to
+                    // a rich revert.
+
+                    // Standard revert errors have the following memory layout:
+                    // | Area            | Offset  | Length   | Contents                              |
+                    // --------------------------------------------------------------------------------
+                    // | Selector        | 0       | 4        | bytes4 function selector (0x08c379a0) |
+                    // | Params          |         |          |                                       |
+                    // |                 | 4       | 32       | uint256 offset to data (o)            |
+                    // | Data            |         |          |                                       |
+                    // |                 | 4 + o   | 32       | uint256 length of data (n             |
+                    // |                 | 32 + o  | n        | Left-aligned message bytes            |
+
+                    // Make sure the return value is long enough.
+                    if gt(returndatasize(), 67) {
+                        // Check that the selector matches for a standard revert error.
+                        let selector := and(mload(sub(cdStart, 28)), 0xffffffff)
+                        cdStart := add(cdStart, 4)
+                        if eq(selector, 0x08c379a0) {
+                            // Set revertMessage to the start of the reason string.
+                            revertMessage := add(cdStart, mload(cdStart))
+                            // Truncate the data length if it's larger than our buffer
+                            // size (288 - 32 = 256)
+                            if gt(mload(revertMessage), 256) {
+                                mstore(revertMessage, 256)
+                            }
+                        }
+                    }
                 }
             }
+
+            if (!didSucceed) {
+                rrevert(AssetProxyTransferError(
+                    orderHash,
+                    assetData,
+                    string(revertMessage)
+                ));
+            }
         }
     }
 }