Merge pull request #100 from 0xProject/improveSignOrder

Improve signOrderHashAsync

3 files changed, 51 insertions, 38 deletions

diff --git a/src/0x.ts b/src/0x.ts
index 95935c258..92a892336 100644
--- a/src/0x.ts
+++ b/src/0x.ts
@@ -9,6 +9,7 @@ import compareVersions = require('compare-versions');
 import {Web3Wrapper} from './web3_wrapper';
 import {constants} from './utils/constants';
 import {utils} from './utils/utils';
+import {signatureUtils} from './utils/signature_utils';
 import {assert} from './utils/assert';
 import {ExchangeWrapper} from './contract_wrappers/exchange_wrapper';
 import {TokenRegistryWrapper} from './contract_wrappers/token_registry_wrapper';
@@ -221,39 +222,28 @@ export class ZeroEx {
 
         const signature = await this._web3Wrapper.signTransactionAsync(signerAddress, msgHashHex);
 
-        let signatureData;
-        const [nodeVersionNumber] = findVersions(nodeVersion);
-        // Parity v1.6.6 and earlier returns the signatureData as vrs instead of rsv as Geth does
-        // Later versions return rsv but for the time being we still want to support version < 1.6.6
-        // Date: May 23rd 2017
-        const latestParityVersionWithVRS = '1.6.6';
-        const isVersionBeforeParityFix = compareVersions(nodeVersionNumber, latestParityVersionWithVRS) <= 0;
-        if (isParityNode && isVersionBeforeParityFix) {
-            const signatureBuffer = ethUtil.toBuffer(signature);
-            let v = signatureBuffer[0];
-            if (v < 27) {
-                v += 27;
+        // HACK: There is no consensus on whether the signatureHex string should be formatted as
+        // v + r + s OR r + s + v, and different clients (even different versions of the same client)
+        // return the signature params in different orders. In order to support all client implementations,
+        // we parse the signature in both ways, and evaluate if either one is a valid signature.
+        const validVParamValues = [27, 28];
+        const ecSignatureVRS = signatureUtils.parseSignatureHexAsVRS(signature);
+        if (_.includes(validVParamValues, ecSignatureVRS.v)) {
+            const isValidVRSSignature = ZeroEx.isValidSignature(orderHash, ecSignatureVRS, signerAddress);
+            if (isValidVRSSignature) {
+                return ecSignatureVRS;
             }
-            signatureData = {
-                v,
-                r: signatureBuffer.slice(1, 33),
-                s: signatureBuffer.slice(33, 65),
-            };
-        } else {
-            signatureData = ethUtil.fromRpcSig(signature);
         }
 
-        const {v, r, s} = signatureData;
-        const ecSignature: ECSignature = {
-            v,
-            r: ethUtil.bufferToHex(r),
-            s: ethUtil.bufferToHex(s),
-        };
-        const isValidSignature = ZeroEx.isValidSignature(orderHash, ecSignature, signerAddress);
-        if (!isValidSignature) {
-            throw new Error(ZeroExError.INVALID_SIGNATURE);
+        const ecSignatureRSV = signatureUtils.parseSignatureHexAsRSV(signature);
+        if (_.includes(validVParamValues, ecSignatureRSV.v)) {
+            const isValidRSVSignature = ZeroEx.isValidSignature(orderHash, ecSignatureRSV, signerAddress);
+            if (isValidRSVSignature) {
+                return ecSignatureRSV;
+            }
         }
-        return ecSignature;
+
+        throw new Error(ZeroExError.INVALID_SIGNATURE);
     }
     /**
      * Returns the ethereum addresses of all available exchange contracts
diff --git a/src/utils/signature_utils.ts b/src/utils/signature_utils.ts
new file mode 100644
index 000000000..b312b5554
--- /dev/null
+++ b/src/utils/signature_utils.ts
@@ -0,0 +1,29 @@
+import * as ethUtil from 'ethereumjs-util';
+import {ECSignature} from '../types';
+
+export const signatureUtils = {
+    parseSignatureHexAsVRS(signatureHex: string): ECSignature {
+        const signatureBuffer = ethUtil.toBuffer(signatureHex);
+        let v = signatureBuffer[0];
+        if (v < 27) {
+            v += 27;
+        }
+        const r = signatureBuffer.slice(1, 33);
+        const s = signatureBuffer.slice(33, 65);
+        const ecSignature: ECSignature = {
+            v,
+            r: ethUtil.bufferToHex(r),
+            s: ethUtil.bufferToHex(s),
+        };
+        return ecSignature;
+    },
+    parseSignatureHexAsRSV(signatureHex: string): ECSignature {
+        const {v, r, s} = ethUtil.fromRpcSig(signatureHex);
+        const ecSignature: ECSignature = {
+            v,
+            r: ethUtil.bufferToHex(r),
+            s: ethUtil.bufferToHex(s),
+        };
+        return ecSignature;
+    },
+};
diff --git a/test/0x.js_test.ts b/test/0x.js_test.ts
index f25f104bd..0c82c803d 100644
--- a/test/0x.js_test.ts
+++ b/test/0x.js_test.ts
@@ -163,7 +163,7 @@ describe('ZeroEx library', () => {
             _.each(stubs, s => s.restore());
             stubs = [];
         });
-        it('Should return the correct ECSignature on TestPRC nodeVersion', async () => {
+        it('Should return the correct ECSignature', async () => {
             const orderHash = '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0';
             const expectedECSignature = {
                 v: 27,
@@ -173,8 +173,7 @@ describe('ZeroEx library', () => {
             const ecSignature = await zeroEx.signOrderHashAsync(orderHash, makerAddress);
             expect(ecSignature).to.deep.equal(expectedECSignature);
         });
-        it('should return the correct ECSignature on Parity > V1.6.6', async () => {
-            const newParityNodeVersion = 'Parity//v1.6.7-beta-e128418-20170518/x86_64-macos/rustc1.17.0';
+        it('should return the correct ECSignature for signatureHex concatenated as R + S + V', async () => {
             const orderHash = '0x34decbedc118904df65f379a175bb39ca18209d6ce41d5ed549d54e6e0a95004';
             // tslint:disable-next-line: max-line-length
             const signature = '0x22109d11d79cb8bf96ed88625e1cd9558800c4073332a9a02857499883ee5ce3050aa3cc1f2c435e67e114cdce54b9527b4f50548342401bc5d2b77adbdacb021b';
@@ -184,8 +183,6 @@ describe('ZeroEx library', () => {
                 s: '0x050aa3cc1f2c435e67e114cdce54b9527b4f50548342401bc5d2b77adbdacb02',
             };
             stubs = [
-                Sinon.stub((zeroEx as any)._web3Wrapper, 'getNodeVersionAsync')
-                    .returns(Promise.resolve(newParityNodeVersion)),
                 Sinon.stub((zeroEx as any)._web3Wrapper, 'signTransactionAsync')
                     .returns(Promise.resolve(signature)),
                 Sinon.stub(ZeroEx, 'isValidSignature').returns(true),
@@ -194,8 +191,7 @@ describe('ZeroEx library', () => {
             const ecSignature = await zeroEx.signOrderHashAsync(orderHash, makerAddress);
             expect(ecSignature).to.deep.equal(expectedECSignature);
         });
-        it('should return the correct ECSignature on Parity < V1.6.6', async () => {
-            const newParityNodeVersion = 'Parity//v1.6.6-beta-8c6e3f3-20170411/x86_64-macos/rustc1.16.0';
+        it('should return the correct ECSignature for signatureHex concatenated as V + R + S', async () => {
             const orderHash = '0xc793e33ffded933b76f2f48d9aa3339fc090399d5e7f5dec8d3660f5480793f7';
             // tslint:disable-next-line: max-line-length
             const signature = '0x1bc80bedc6756722672753413efdd749b5adbd4fd552595f59c13427407ee9aee02dea66f25a608bbae457e020fb6decb763deb8b7192abab624997242da248960';
@@ -205,8 +201,6 @@ describe('ZeroEx library', () => {
                 s: '0x2dea66f25a608bbae457e020fb6decb763deb8b7192abab624997242da248960',
             };
             stubs = [
-                Sinon.stub((zeroEx as any)._web3Wrapper, 'getNodeVersionAsync')
-                    .returns(Promise.resolve(newParityNodeVersion)),
                 Sinon.stub((zeroEx as any)._web3Wrapper, 'signTransactionAsync')
                     .returns(Promise.resolve(signature)),
                 Sinon.stub(ZeroEx, 'isValidSignature').returns(true),