Rethrow Wallet and Validator errors

5 files changed, 53 insertions, 22 deletions

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
index 6bfbb5107..017da742e 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
@@ -293,8 +293,20 @@ contract MixinSignatureValidator is
                 cdStart,          // write input over output
                 32                // output size is 32 bytes
             )
-            // Signature is valid if call did not revert and returned true
-            isValid := and(success, mload(cdStart))
+
+            switch success
+            case 0 {
+                // Revert with `Error("WALLET_ERROR")`
+                mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
+                mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
+                mstore(64, 0x0000000c57414c4c45545f4552524f5200000000000000000000000000000000)
+                mstore(96, 0)
+                revert(0, 100)
+            }
+            case 1 {
+                // Signature is valid if call did not revert and returned true
+                isValid := mload(cdStart)
+            }
         }
         return isValid;
     }
@@ -331,8 +343,20 @@ contract MixinSignatureValidator is
                 cdStart,           // write input over output
                 32                 // output size is 32 bytes
             )
-            // Signature is valid if call did not revert and returned true
-            isValid := and(success, mload(cdStart))
+
+            switch success
+            case 0 {
+                // Revert with `Error("VALIDATOR_ERROR")`
+                mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
+                mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
+                mstore(64, 0x0000000f56414c494441544f525f4552524f5200000000000000000000000000)
+                mstore(96, 0)
+                revert(0, 100)
+            }
+            case 1 {
+                // Signature is valid if call did not revert and returned true
+                isValid := mload(cdStart)
+            }
         }
         return isValid;
     }
diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index fdfb40155..f9d8b7783 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -195,7 +195,7 @@ describe('Exchange core', () => {
             signedOrder.signature = `0x0${SignatureType.Wallet}`;
             await expectTransactionFailedAsync(
                 exchangeWrapper.fillOrderAsync(signedOrder, takerAddress),
-                RevertReason.InvalidOrderSignature,
+                RevertReason.WalletError,
             );
         });
 
@@ -209,13 +209,10 @@ describe('Exchange core', () => {
                 ),
                 constants.AWAIT_TRANSACTION_MINED_MS,
             );
-            signedOrder = await orderFactory.newSignedOrderAsync({
-                makerAddress: maliciousValidator.address,
-            });
             signedOrder.signature = `${maliciousValidator.address}0${SignatureType.Validator}`;
             await expectTransactionFailedAsync(
                 exchangeWrapper.fillOrderAsync(signedOrder, takerAddress),
-                RevertReason.InvalidOrderSignature,
+                RevertReason.ValidatorError,
             );
         });
     });
diff --git a/packages/contracts/test/exchange/signature_validator.ts b/packages/contracts/test/exchange/signature_validator.ts
index 947ebffcc..5b64d853c 100644
--- a/packages/contracts/test/exchange/signature_validator.ts
+++ b/packages/contracts/test/exchange/signature_validator.ts
@@ -352,7 +352,7 @@ describe('MixinSignatureValidator', () => {
             expect(isValidSignature).to.be.false();
         });
 
-        it('should return false when `isValidSignature` attempts to update state and not allow state to be updated when SignatureType=Wallet', async () => {
+        it('should revert when `isValidSignature` attempts to update state and SignatureType=Wallet', async () => {
             // Create EIP712 signature
             const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
             const orderHashBuffer = ethUtil.toBuffer(orderHashHex);
@@ -365,13 +365,14 @@ describe('MixinSignatureValidator', () => {
                 ethUtil.toBuffer(`0x${SignatureType.Wallet}`),
             ]);
             const signatureHex = ethUtil.bufferToHex(signature);
-            // Validate signature
-            const isValid = await signatureValidator.publicIsValidSignature.callAsync(
-                orderHashHex,
-                maliciousWallet.address,
-                signatureHex,
+            await expectContractCallFailed(
+                signatureValidator.publicIsValidSignature.callAsync(
+                    orderHashHex,
+                    maliciousWallet.address,
+                    signatureHex,
+                ),
+                RevertReason.WalletError,
             );
-            expect(isValid).to.be.equal(false);
         });
 
         it('should return true when SignatureType=Validator, signature is valid and validator is approved', async () => {
@@ -404,18 +405,16 @@ describe('MixinSignatureValidator', () => {
             expect(isValidSignature).to.be.false();
         });
 
-        it('should return false when `isValidSignature` attempts to update state and not allow state to be updated when SignatureType=Validator', async () => {
+        it('should revert when `isValidSignature` attempts to update state and SignatureType=Validator', async () => {
             const validatorAddress = ethUtil.toBuffer(`${maliciousValidator.address}`);
             const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`);
             const signature = Buffer.concat([validatorAddress, signatureType]);
             const signatureHex = ethUtil.bufferToHex(signature);
             const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
-            const isValid = await signatureValidator.publicIsValidSignature.callAsync(
-                orderHashHex,
-                maliciousValidator.address,
-                signatureHex,
+            await expectContractCallFailed(
+                signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex),
+                RevertReason.ValidatorError,
             );
-            expect(isValid).to.be.equal(false);
         });
         it('should return false when SignatureType=Validator, signature is valid and validator is not approved', async () => {
             // Set approval of signature validator to false
diff --git a/packages/types/CHANGELOG.json b/packages/types/CHANGELOG.json
index fabc80ecf..980a12ad3 100644
--- a/packages/types/CHANGELOG.json
+++ b/packages/types/CHANGELOG.json
@@ -1,5 +1,14 @@
 [
     {
+        "version": "1.0.1-rc.6",
+        "changes": [
+            {
+                "note": "Add WalletError and ValidatorError revert reasons",
+                "pr": 1012
+            }
+        ]
+    },
+    {
         "version": "1.0.1-rc.5",
         "changes": [
             {
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
index 298fa77d4..2831d816d 100644
--- a/packages/types/src/index.ts
+++ b/packages/types/src/index.ts
@@ -220,6 +220,8 @@ export enum RevertReason {
     Erc721InvalidSpender = 'ERC721_INVALID_SPENDER',
     Erc721ZeroOwner = 'ERC721_ZERO_OWNER',
     Erc721InvalidSelector = 'ERC721_INVALID_SELECTOR',
+    WalletError = 'WALLET_ERROR',
+    ValidatorError = 'VALIDATOR_ERROR',
 }
 
 export enum StatusCodes {