In @0x/contracts-exchange: More efficient revert string extraction in MixinAssetProxyDispatcher.sol

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

@@ -113,7 +113,7 @@ contract MixinAssetProxyDispatcher is
             // Whether the AssetProxy transfer succeeded.
             bool didSucceed;
             // On failure, the revert message returned by the asset proxy.
-            bytes memory revertMessage;
+            string memory revertMessage;
 
             // We construct calldata for the `assetProxy.transferFrom` ABI.
             // The layout of this calldata is in the table below.
@@ -134,8 +134,6 @@ 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.
@@ -143,7 +141,6 @@ contract MixinAssetProxyDispatcher is
                 // `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
@@ -177,40 +174,35 @@ contract MixinAssetProxyDispatcher is
                     0,                      // don't send any ETH
                     cdStart,                // pointer to start of input
                     sub(cdEnd, cdStart),    // length of input
-                    cdStart,                // write output over input
+                    0,                      // don't store the returndata
-                    288                     // reserve 288 bytes for output
+                    0                       // don't store the returndata
                 )
 
                 if iszero(didSucceed) { // Call reverted.
-                    // Attempt to to decode standard revert messages with the
+                    // The asset proxies always revert with a standard string error,
-                    // signature `Error(string)` so we can upgrade them to
+                    // which have the signature `Error(string)` and are ABI encoded
-                    // a rich revert.
+                    // the same way as calldata for a function call is.
+                    // We will extract the string payload from this to upgrade it
+                    // to a rich revert.
 
-                    // Standard revert errors have the following memory layout:
+                    // Asset proxy 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)            |
+                    // |                 | 4       | 32       | uint256 offset to data (always 32)    |
                     // | Data            |         |          |                                       |
-                    // |                 | 4 + o   | 32       | uint256 length of data (n             |
+                    // |                 | 36      | 32       | uint256 length of data (n)            |
-                    // |                 | 32 + o  | n        | Left-aligned message bytes            |
+                    // |                 | 68      | n        | Left-aligned message bytes            |
 
-                    // Make sure the return value is long enough.
+                    // Get the size of the string data/payload.
-                    if gt(returndatasize(), 67) {
+                    let revertDataSize := sub(returndatasize(), 36)
-                        // Check that the selector matches for a standard revert error.
+                    // We need to move the free memory pointer because we
-                        let selector := and(mload(sub(cdStart, 28)), 0xffffffff)
+                    // still have solidity logic that executes after this assembly.
-                        cdStart := add(cdStart, 4)
+                    mstore(64, add(cdStart, revertDataSize))
-                        if eq(selector, 0x08c379a0) {
+                    // Copy the revert string data.
-                            // Set revertMessage to the start of the reason string.
+                    returndatacopy(cdStart, 36, revertDataSize)
-                            revertMessage := add(cdStart, mload(cdStart))
+                    revertMessage := cdStart
-                            // Truncate the data length if it's larger than our buffer
-                            // size (288 - 32 = 256)
-                            if gt(mload(revertMessage), 256) {
-                                mstore(revertMessage, 256)
-                            }
-                        }
-                    }
                 }
             }
 
@@ -218,7 +210,7 @@ contract MixinAssetProxyDispatcher is
                 rrevert(AssetProxyTransferError(
                     orderHash,
                     assetData,
-                    string(revertMessage)
+                    revertMessage
                 ));
             }
         }

contracts/exchange/test/core.ts

@@ -54,7 +54,7 @@ const expect = chai.expect;
 const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 
 // tslint:disable:no-unnecessary-type-assertion
-describe('Exchange core', () => {
+describe.only('Exchange core', () => {
     let chainId: number;
     let makerAddress: string;
     let owner: string;
@@ -700,6 +700,7 @@ describe('Exchange core', () => {
             const expectedError = new ExchangeRevertErrors.AssetProxyTransferError(
                 orderHashHex,
                 signedOrder.makerAssetData,
+                RevertReason.TransferFailed,
             );
             const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount });
             return expect(tx).to.revertWith(expectedError);
@@ -726,6 +727,7 @@ describe('Exchange core', () => {
             const expectedError = new ExchangeRevertErrors.AssetProxyTransferError(
                 orderHashHex,
                 signedOrder.takerAssetData,
+                RevertReason.TransferFailed,
             );
             const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount });
             return expect(tx).to.revertWith(expectedError);

contracts/exchange/test/utils/fill_order_combinatorial_utils.ts

@@ -954,7 +954,7 @@ function validationErrorToRevertError(order: Order, reason: RevertReason): Rever
         case RevertReason.InvalidFillPrice:
             return new ExchangeRevertErrors.FillError(orderHash, ExchangeRevertErrors.FillErrorCode.InvalidFillPrice);
         case RevertReason.TransferFailed:
-            return new ExchangeRevertErrors.AssetProxyTransferError(orderHash, undefined, 'TRANSFER_FAILED');
+            return new ExchangeRevertErrors.AssetProxyTransferError(orderHash, undefined, RevertReason.TransferFailed);
         default:
             return new StringRevertError(reason);
     }