Fix order-utils tests

7 files changed, 123 insertions, 81 deletions

diff --git a/packages/order-utils/src/index.ts b/packages/order-utils/src/index.ts
index 5b4e20119..b844fbfcb 100644
--- a/packages/order-utils/src/index.ts
+++ b/packages/order-utils/src/index.ts
@@ -1,10 +1,10 @@
 export { orderHashUtils } from './order_hash';
-export { isValidSignatureAsync, ecSignOrderHashAsync } from './signature_utils';
+export { isValidSignatureAsync, ecSignOrderHashAsync, addSignedMessagePrefix } from './signature_utils';
 export { orderFactory } from './order_factory';
 export { constants } from './constants';
 export { crypto } from './crypto';
 export { generatePseudoRandomSalt } from './salt';
-export { OrderError } from './types';
+export { OrderError, MessagePrefixType, MessagePrefixOpts } from './types';
 export { AbstractBalanceAndProxyAllowanceFetcher } from './abstract/abstract_balance_and_proxy_allowance_fetcher';
 export { AbstractOrderFilledCancelledFetcher } from './abstract/abstract_order_filled_cancelled_fetcher';
 export { RemainingFillableCalculator } from './remaining_fillable_calculator';
diff --git a/packages/order-utils/src/order_factory.ts b/packages/order-utils/src/order_factory.ts
index fe341b845..3f3dc524c 100644
--- a/packages/order-utils/src/order_factory.ts
+++ b/packages/order-utils/src/order_factory.ts
@@ -1,12 +1,12 @@
-import { Provider, SignedOrder } from '@0xproject/types';
+import { ECSignature, Provider, SignedOrder } from '@0xproject/types';
 import { BigNumber } from '@0xproject/utils';
+import * as ethUtil from 'ethereumjs-util';
 import * as _ from 'lodash';
 
 import { orderHashUtils } from './order_hash';
 import { generatePseudoRandomSalt } from './salt';
-import { ecSignOrderHashAsync, getVRSHexString } from './signature_utils';
-
-const SHOULD_ADD_PERSONAL_MESSAGE_PREFIX = false;
+import { ecSignOrderHashAsync } from './signature_utils';
+import { MessagePrefixType } from './types';
 
 export const orderFactory = {
     async createSignedOrderAsync(
@@ -44,14 +44,25 @@ export const orderFactory = {
             expirationTimeSeconds,
         };
         const orderHash = orderHashUtils.getOrderHashHex(order);
-        const ecSignature = await ecSignOrderHashAsync(
-            provider,
-            orderHash,
-            makerAddress,
-            SHOULD_ADD_PERSONAL_MESSAGE_PREFIX,
-        );
+        const messagePrefixOpts = {
+            prefixType: MessagePrefixType.EthSign,
+            shouldAddPrefixBeforeCallingEthSign: false,
+        };
+        const ecSignature = await ecSignOrderHashAsync(provider, orderHash, makerAddress, messagePrefixOpts);
         const signature = getVRSHexString(ecSignature);
         const signedOrder: SignedOrder = _.assign(order, { signature });
         return signedOrder;
     },
 };
+
+function getVRSHexString(ecSignature: ECSignature): string {
+    const vrs = `0x${intToHex(ecSignature.v)}${ethUtil.stripHexPrefix(ecSignature.r)}${ethUtil.stripHexPrefix(
+        ecSignature.s,
+    )}`;
+    return vrs;
+}
+
+function intToHex(i: number): string {
+    const hex = ethUtil.bufferToHex(ethUtil.toBuffer(i));
+    return hex;
+}
diff --git a/packages/order-utils/src/order_hash.ts b/packages/order-utils/src/order_hash.ts
index 3d0db5b0c..a4e36ab89 100644
--- a/packages/order-utils/src/order_hash.ts
+++ b/packages/order-utils/src/order_hash.ts
@@ -9,7 +9,7 @@ import * as _ from 'lodash';
 import { assert } from './assert';
 import { crypto } from './crypto';
 
-const INVALID_TAKER_FORMAT = 'instance.taker is not of a type(s) string';
+const INVALID_TAKER_FORMAT = 'instance.takerAddress is not of a type(s) string';
 
 export const orderHashUtils = {
     /**
@@ -34,7 +34,7 @@ export const orderHashUtils = {
      */
     getOrderHashHex(order: SignedOrder | Order): string {
         try {
-            assert.doesConformToSchema('order', order, schemas.orderSchema);
+            assert.doesConformToSchema('order', order, schemas.orderSchema, [schemas.hexSchema]);
         } catch (error) {
             if (_.includes(error.message, INVALID_TAKER_FORMAT)) {
                 const errMsg =
diff --git a/packages/order-utils/src/signature_utils.ts b/packages/order-utils/src/signature_utils.ts
index 106bbf4e8..8cd4264ab 100644
--- a/packages/order-utils/src/signature_utils.ts
+++ b/packages/order-utils/src/signature_utils.ts
@@ -8,12 +8,13 @@ import { artifacts } from './artifacts';
 import { assert } from './assert';
 import { ExchangeContract } from './generated_contract_wrappers/exchange';
 import { ISignerContract } from './generated_contract_wrappers/i_signer';
-import { OrderError } from './types';
+import { MessagePrefixOpts, MessagePrefixType, OrderError } from './types';
 
 /**
  * Verifies that the provided signature is valid according to the 0x Protocol smart contracts
  * @param   data          The hex encoded data signed by the supplied signature.
- * @param   signature     An object containing the elliptic curve signature parameters.
+ * @param   signature     A hex encoded 0x Protocol signature made up of: [SignatureType][TypeSpecificData].
+ *          E.g [SignatureType.EIP712][vrs]
  * @param   signerAddress The hex encoded address that signed the data, producing the supplied signature.
  * @return  Whether the signature is valid for the supplied signerAddress and data.
  */
@@ -41,10 +42,8 @@ export async function isValidSignatureAsync(
         // types use ECRecover...
         case SignatureType.Ecrecover: {
             const ecSignature = parseECSignature(signature);
-            const dataBuff = ethUtil.toBuffer(data);
-            const msgHashBuff = ethUtil.hashPersonalMessage(dataBuff);
-            const msgHash = ethUtil.bufferToHex(msgHashBuff);
-            return isValidECSignature(msgHash, ecSignature, signerAddress);
+            const prefixedMessageHex = addSignedMessagePrefix(data, MessagePrefixType.EthSign);
+            return isValidECSignature(prefixedMessageHex, ecSignature, signerAddress);
         }
 
         case SignatureType.EIP712: {
@@ -53,11 +52,9 @@ export async function isValidSignatureAsync(
         }
 
         case SignatureType.Trezor: {
-            const dataBuff = ethUtil.toBuffer(data);
-            const msgHashBuff = hashTrezorPersonalMessage(dataBuff);
-            const msgHash = ethUtil.bufferToHex(msgHashBuff);
+            const prefixedMessageHex = addSignedMessagePrefix(data, MessagePrefixType.Trezor);
             const ecSignature = parseECSignature(signature);
-            return isValidECSignature(msgHash, ecSignature, signerAddress);
+            return isValidECSignature(prefixedMessageHex, ecSignature, signerAddress);
         }
 
         // TODO: Rename Contract -> Wallet
@@ -78,13 +75,14 @@ export async function isValidSignatureAsync(
     }
 }
 
-export function getVRSHexString(ecSignature: ECSignature): string {
-    const vrs = `0x${intToHex(ecSignature.v)}${ethUtil.stripHexPrefix(ecSignature.r)}${ethUtil.stripHexPrefix(
-        ecSignature.s,
-    )}`;
-    return vrs;
-}
-
+/**
+ * Checks if the supplied elliptic curve signature corresponds to signing `data` with
+ * the private key corresponding to `signerAddress`
+ * @param   data          The hex encoded data signed by the supplied signature.
+ * @param   signature     An object containing the elliptic curve signature parameters.
+ * @param   signerAddress The hex encoded address that signed the data, producing the supplied signature.
+ * @return Whether the ECSignature is valid.
+ */
 export function isValidECSignature(data: string, signature: ECSignature, signerAddress: string): boolean {
     assert.isHexString('data', data);
     assert.doesConformToSchema('signature', signature, schemas.ecSignatureSchema);
@@ -112,17 +110,16 @@ export function isValidECSignature(data: string, signature: ECSignature, signerA
  * @param   orderHash       Hex encoded orderHash to sign.
  * @param   signerAddress   The hex encoded Ethereum address you wish to sign it with. This address
  *          must be available via the Provider supplied to 0x.js.
- * @param   shouldAddPersonalMessagePrefix  Some signers add the personal message prefix `\x19Ethereum Signed Message`
- *          themselves (e.g Parity Signer, Ledger, TestRPC) and others expect it to already be done by the client
- *          (e.g Metamask). Depending on which signer this request is going to, decide on whether to add the prefix
- *          before sending the request.
+ * @param   hashPrefixOpts Different signers add/require different prefixes be appended to the message being signed.
+ *          Since we cannot know ahead of time which signer you are using, you must supply both a prefixType and
+ *          whether it must be added before calling `eth_sign` (some signers add it themselves)
  * @return  An object containing the Elliptic curve signature parameters generated by signing the orderHash.
  */
 export async function ecSignOrderHashAsync(
     provider: Provider,
     orderHash: string,
     signerAddress: string,
-    shouldAddPersonalMessagePrefix: boolean,
+    messagePrefixOpts: MessagePrefixOpts,
 ): Promise<ECSignature> {
     assert.isHexString('orderHash', orderHash);
     const web3Wrapper = new Web3Wrapper(provider);
@@ -130,12 +127,10 @@ export async function ecSignOrderHashAsync(
     const normalizedSignerAddress = signerAddress.toLowerCase();
 
     let msgHashHex = orderHash;
-    if (shouldAddPersonalMessagePrefix) {
-        const orderHashBuff = ethUtil.toBuffer(orderHash);
-        const msgHashBuff = ethUtil.hashPersonalMessage(orderHashBuff);
-        msgHashHex = ethUtil.bufferToHex(msgHashBuff);
+    const prefixedMsgHashHex = addSignedMessagePrefix(orderHash, messagePrefixOpts.prefixType);
+    if (messagePrefixOpts.shouldAddPrefixBeforeCallingEthSign) {
+        msgHashHex = prefixedMsgHashHex;
     }
-
     const signature = await web3Wrapper.signMessageAsync(normalizedSignerAddress, msgHashHex);
 
     // HACK: There is no consensus on whether the signatureHex string should be formatted as
@@ -146,7 +141,7 @@ export async function ecSignOrderHashAsync(
     const validVParamValues = [27, 28];
     const ecSignatureVRS = parseSignatureHexAsVRS(signature);
     if (_.includes(validVParamValues, ecSignatureVRS.v)) {
-        const isValidVRSSignature = isValidECSignature(orderHash, ecSignatureVRS, normalizedSignerAddress);
+        const isValidVRSSignature = isValidECSignature(prefixedMsgHashHex, ecSignatureVRS, normalizedSignerAddress);
         if (isValidVRSSignature) {
             return ecSignatureVRS;
         }
@@ -154,7 +149,7 @@ export async function ecSignOrderHashAsync(
 
     const ecSignatureRSV = parseSignatureHexAsRSV(signature);
     if (_.includes(validVParamValues, ecSignatureRSV.v)) {
-        const isValidRSVSignature = isValidECSignature(orderHash, ecSignatureRSV, normalizedSignerAddress);
+        const isValidRSVSignature = isValidECSignature(prefixedMsgHashHex, ecSignatureRSV, normalizedSignerAddress);
         if (isValidRSVSignature) {
             return ecSignatureRSV;
         }
@@ -163,6 +158,30 @@ export async function ecSignOrderHashAsync(
     throw new Error(OrderError.InvalidSignature);
 }
 
+export function addSignedMessagePrefix(message: string, messagePrefixType: MessagePrefixType): string {
+    switch (messagePrefixType) {
+        case MessagePrefixType.None:
+            return message;
+
+        case MessagePrefixType.EthSign: {
+            const msgBuff = ethUtil.toBuffer(message);
+            const prefixedMsgBuff = ethUtil.hashPersonalMessage(msgBuff);
+            const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff);
+            return prefixedMsgHex;
+        }
+
+        case MessagePrefixType.Trezor: {
+            const msgBuff = ethUtil.toBuffer(message);
+            const prefixedMsgBuff = hashTrezorPersonalMessage(msgBuff);
+            const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff);
+            return prefixedMsgHex;
+        }
+
+        default:
+            throw new Error(`Unrecognized MessagePrefixType: ${messagePrefixType}`);
+    }
+}
+
 function hashTrezorPersonalMessage(message: Buffer): Buffer {
     const prefix = ethUtil.toBuffer('\x19Ethereum Signed Message:\n' + String.fromCharCode(message.length));
     return ethUtil.sha3(Buffer.concat([prefix, message]));
@@ -183,11 +202,6 @@ function parseECSignature(signature: string): ECSignature {
     return ecSignature;
 }
 
-function intToHex(i: number): string {
-    const hex = ethUtil.bufferToHex(ethUtil.toBuffer(i));
-    return hex;
-}
-
 function getSignatureTypeIndexIfExists(signature: string): number {
     const unprefixedSignature = ethUtil.stripHexPrefix(signature);
     const signatureTypeHex = unprefixedSignature.substr(0, 2);
diff --git a/packages/order-utils/src/types.ts b/packages/order-utils/src/types.ts
index f79d52359..db0bfb249 100644
--- a/packages/order-utils/src/types.ts
+++ b/packages/order-utils/src/types.ts
@@ -1,3 +1,25 @@
 export enum OrderError {
     InvalidSignature = 'INVALID_SIGNATURE',
 }
+
+/**
+ * The requisite message prefix (is any) to add to an `eth_sign` request.
+ */
+export enum MessagePrefixType {
+    None = 'NONE',
+    EthSign = 'ETH_SIGN',
+    Trezor = 'TREZOR',
+}
+
+/**
+ * Options related to message prefixing of messages sent to `eth_sign`
+ * Some signers prepend a message prefix (e.g Parity Signer, Ledger, TestRPC), while
+ * others require it already be prepended (e.g Metamask). In addition, different signers
+ * expect slightly different prefixes (See: https://github.com/ethereum/go-ethereum/issues/14794).
+ * Depending on the signer that will receive your signing request, you must specify the
+ * desired prefix and whether it should be added before making the `eth_sign` request.
+ */
+export interface MessagePrefixOpts {
+    prefixType: MessagePrefixType;
+    shouldAddPrefixBeforeCallingEthSign: boolean;
+}
diff --git a/packages/order-utils/test/order_hash_test.ts b/packages/order-utils/test/order_hash_test.ts
index 7cb87c8ab..21ae22589 100644
--- a/packages/order-utils/test/order_hash_test.ts
+++ b/packages/order-utils/test/order_hash_test.ts
@@ -39,7 +39,7 @@ describe('Order hashing', () => {
         it('throws a readable error message if taker format is invalid', async () => {
             const orderWithInvalidtakerFormat = {
                 ...order,
-                taker: (null as any) as string,
+                takerAddress: (null as any) as string,
             };
             const expectedErrorMessage =
                 'Order taker must be of type string. If you want anyone to be able to fill an order - pass ZeroEx.NULL_ADDRESS';
diff --git a/packages/order-utils/test/signature_utils_test.ts b/packages/order-utils/test/signature_utils_test.ts
index 4babd5582..773a32a56 100644
--- a/packages/order-utils/test/signature_utils_test.ts
+++ b/packages/order-utils/test/signature_utils_test.ts
@@ -7,7 +7,7 @@ import 'make-promises-safe';
 import 'mocha';
 import * as Sinon from 'sinon';
 
-import { ecSignOrderHashAsync, generatePseudoRandomSalt, orderHashUtils } from '../src';
+import { ecSignOrderHashAsync, generatePseudoRandomSalt, MessagePrefixType, orderHashUtils } from '../src';
 import { isValidECSignature, isValidSignatureAsync } from '../src/signature_utils';
 
 import { chaiSetup } from './utils/chai_setup';
@@ -16,8 +16,6 @@ import { provider, web3Wrapper } from './utils/web3_wrapper';
 chaiSetup.configure();
 const expect = chai.expect;
 
-const SHOULD_ADD_PERSONAL_MESSAGE_PREFIX = false;
-
 describe('Signature utils', () => {
     describe('#isValidSignature', () => {
         let dataHex = '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0';
@@ -75,28 +73,28 @@ describe('Signature utils', () => {
         // TODO: remaining sigs
     });
     describe('#isValidECSignature', () => {
-        // The Exchange smart contract `isValidECSignature` method only validates orderHashes and assumes
-        // the length of the data is exactly 32 bytes. Thus for these tests, we use data of this size.
-        const dataHex = '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0';
+        // TODO: Replace this with a vanilla signature without ANY prefix or modification
         const signature = {
             v: 27,
-            r: '0x61a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33',
-            s: '0x40349190569279751135161d22529dc25add4f6069af05be04cacbda2ace2254',
+            r: '0xaca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d64393',
+            s: '0x46b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf2',
         };
-        const address = '0x5409ed021d9299bf6814279a6a1411a7e866a631';
+        const data = '0x47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad';
+        const address = '0x0e5cb767cce09a7f3ca594df118aa519be5e2b5a';
+
         it("should return false if the data doesn't pertain to the signature & address", async () => {
             expect(isValidECSignature('0x0', signature, address)).to.be.false();
         });
         it("should return false if the address doesn't pertain to the signature & data", async () => {
             const validUnrelatedAddress = '0x8b0292b11a196601ed2ce54b665cafeca0347d42';
-            expect(isValidECSignature(dataHex, signature, validUnrelatedAddress)).to.be.false();
+            expect(isValidECSignature(data, signature, validUnrelatedAddress)).to.be.false();
         });
-        it("should return false if the signature doesn't pertain to the dataHex & address", async () => {
+        it("should return false if the signature doesn't pertain to the data & address", async () => {
             const wrongSignature = _.assign({}, signature, { v: 28 });
-            expect(isValidECSignature(dataHex, wrongSignature, address)).to.be.false();
+            expect(isValidECSignature(data, wrongSignature, address)).to.be.false();
         });
-        it('should return true if the signature does pertain to the dataHex & address', async () => {
-            const isValidSignatureLocal = isValidECSignature(dataHex, signature, address);
+        it('should return true if the signature does pertain to the data & address', async () => {
+            const isValidSignatureLocal = isValidECSignature(data, signature, address);
             expect(isValidSignatureLocal).to.be.true();
         });
     });
@@ -147,12 +145,11 @@ describe('Signature utils', () => {
                 r: '0x61a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33',
                 s: '0x40349190569279751135161d22529dc25add4f6069af05be04cacbda2ace2254',
             };
-            const ecSignature = await ecSignOrderHashAsync(
-                provider,
-                orderHash,
-                makerAddress,
-                SHOULD_ADD_PERSONAL_MESSAGE_PREFIX,
-            );
+            const messagePrefixOpts = {
+                prefixType: MessagePrefixType.EthSign,
+                shouldAddPrefixBeforeCallingEthSign: false,
+            };
+            const ecSignature = await ecSignOrderHashAsync(provider, orderHash, makerAddress, messagePrefixOpts);
             expect(ecSignature).to.deep.equal(expectedECSignature);
         });
         it('should return the correct ECSignature for signatureHex concatenated as R + S + V', async () => {
@@ -181,12 +178,11 @@ describe('Signature utils', () => {
                 },
             };
 
-            const ecSignature = await ecSignOrderHashAsync(
-                fakeProvider,
-                orderHash,
-                makerAddress,
-                SHOULD_ADD_PERSONAL_MESSAGE_PREFIX,
-            );
+            const messagePrefixOpts = {
+                prefixType: MessagePrefixType.EthSign,
+                shouldAddPrefixBeforeCallingEthSign: false,
+            };
+            const ecSignature = await ecSignOrderHashAsync(fakeProvider, orderHash, makerAddress, messagePrefixOpts);
             expect(ecSignature).to.deep.equal(expectedECSignature);
         });
         it('should return the correct ECSignature for signatureHex concatenated as V + R + S', async () => {
@@ -212,12 +208,11 @@ describe('Signature utils', () => {
                 },
             };
 
-            const ecSignature = await ecSignOrderHashAsync(
-                fakeProvider,
-                orderHash,
-                makerAddress,
-                SHOULD_ADD_PERSONAL_MESSAGE_PREFIX,
-            );
+            const messagePrefixOpts = {
+                prefixType: MessagePrefixType.EthSign,
+                shouldAddPrefixBeforeCallingEthSign: false,
+            };
+            const ecSignature = await ecSignOrderHashAsync(fakeProvider, orderHash, makerAddress, messagePrefixOpts);
             expect(ecSignature).to.deep.equal(expectedECSignature);
         });
     });