fix: refactor sign.js and related tests (#1243)

* fix: refactor sign.js and related tests

* fix: remove unused dep

* fix: update package.json correctly

package-lock.json

@@ -4655,23 +4655,21 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "delegates": "1.0.0",
-            "readable-stream": "2.3.6"
+            "delegates": "^1.0.0",
+            "readable-stream": "^2.0.6"
           }
         },
         "balanced-match": {
           "version": "1.0.0",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "brace-expansion": {
           "version": "1.1.11",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
-            "balanced-match": "1.0.0",
+            "balanced-match": "^1.0.0",
             "concat-map": "0.0.1"
           }
         },
@@ -4684,20 +4682,17 @@
         "code-point-at": {
           "version": "1.1.0",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "concat-map": {
           "version": "0.0.1",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "console-control-strings": {
           "version": "1.1.0",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "core-util-is": {
           "version": "1.0.2",
@@ -4738,7 +4733,7 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "minipass": "2.2.4"
+            "minipass": "^2.2.1"
           }
         },
         "fs.realpath": {
@@ -4753,14 +4748,14 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "aproba": "1.2.0",
-            "console-control-strings": "1.1.0",
-            "has-unicode": "2.0.1",
-            "object-assign": "4.1.1",
-            "signal-exit": "3.0.2",
-            "string-width": "1.0.2",
-            "strip-ansi": "3.0.1",
-            "wide-align": "1.1.2"
+            "aproba": "^1.0.3",
+            "console-control-strings": "^1.0.0",
+            "has-unicode": "^2.0.0",
+            "object-assign": "^4.1.0",
+            "signal-exit": "^3.0.0",
+            "string-width": "^1.0.1",
+            "strip-ansi": "^3.0.1",
+            "wide-align": "^1.1.0"
           }
         },
         "glob": {
@@ -4769,12 +4764,12 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "fs.realpath": "1.0.0",
-            "inflight": "1.0.6",
-            "inherits": "2.0.3",
-            "minimatch": "3.0.4",
-            "once": "1.4.0",
-            "path-is-absolute": "1.0.1"
+            "fs.realpath": "^1.0.0",
+            "inflight": "^1.0.4",
+            "inherits": "2",
+            "minimatch": "^3.0.4",
+            "once": "^1.3.0",
+            "path-is-absolute": "^1.0.0"
           }
         },
         "has-unicode": {
@@ -4789,7 +4784,7 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "safer-buffer": "2.1.2"
+            "safer-buffer": "^2.1.0"
           }
         },
         "ignore-walk": {
@@ -4798,7 +4793,7 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "minimatch": "3.0.4"
+            "minimatch": "^3.0.4"
           }
         },
         "inflight": {
@@ -4807,15 +4802,14 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "once": "1.4.0",
-            "wrappy": "1.0.2"
+            "once": "^1.3.0",
+            "wrappy": "1"
           }
         },
         "inherits": {
           "version": "2.0.3",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "ini": {
           "version": "1.3.5",
@@ -4827,9 +4821,8 @@
           "version": "1.0.0",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
-            "number-is-nan": "1.0.1"
+            "number-is-nan": "^1.0.0"
           }
         },
         "isarray": {
@@ -4842,25 +4835,22 @@
           "version": "3.0.4",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
-            "brace-expansion": "1.1.11"
+            "brace-expansion": "^1.1.7"
           }
         },
         "minimist": {
           "version": "0.0.8",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "minipass": {
           "version": "2.2.4",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
-            "safe-buffer": "5.1.1",
-            "yallist": "3.0.2"
+            "safe-buffer": "^5.1.1",
+            "yallist": "^3.0.0"
           }
         },
         "minizlib": {
@@ -4869,14 +4859,13 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "minipass": "2.2.4"
+            "minipass": "^2.2.1"
           }
         },
         "mkdirp": {
           "version": "0.5.1",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "minimist": "0.0.8"
           }
@@ -4893,9 +4882,9 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "debug": "2.6.9",
-            "iconv-lite": "0.4.21",
-            "sax": "1.2.4"
+            "debug": "^2.1.2",
+            "iconv-lite": "^0.4.4",
+            "sax": "^1.2.4"
           }
         },
         "node-pre-gyp": {
@@ -4904,16 +4893,16 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "detect-libc": "1.0.3",
-            "mkdirp": "0.5.1",
-            "needle": "2.2.0",
-            "nopt": "4.0.1",
-            "npm-packlist": "1.1.10",
-            "npmlog": "4.1.2",
-            "rc": "1.2.7",
-            "rimraf": "2.6.2",
-            "semver": "5.5.0",
-            "tar": "4.4.1"
+            "detect-libc": "^1.0.2",
+            "mkdirp": "^0.5.1",
+            "needle": "^2.2.0",
+            "nopt": "^4.0.1",
+            "npm-packlist": "^1.1.6",
+            "npmlog": "^4.0.2",
+            "rc": "^1.1.7",
+            "rimraf": "^2.6.1",
+            "semver": "^5.3.0",
+            "tar": "^4"
           }
         },
         "nopt": {
@@ -4922,8 +4911,8 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "abbrev": "1.1.1",
-            "osenv": "0.1.5"
+            "abbrev": "1",
+            "osenv": "^0.1.4"
           }
         },
         "npm-bundled": {
@@ -4938,8 +4927,8 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "ignore-walk": "3.0.1",
-            "npm-bundled": "1.0.3"
+            "ignore-walk": "^3.0.1",
+            "npm-bundled": "^1.0.1"
           }
         },
         "npmlog": {
@@ -4948,17 +4937,16 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "are-we-there-yet": "1.1.4",
-            "console-control-strings": "1.1.0",
-            "gauge": "2.7.4",
-            "set-blocking": "2.0.0"
+            "are-we-there-yet": "~1.1.2",
+            "console-control-strings": "~1.1.0",
+            "gauge": "~2.7.3",
+            "set-blocking": "~2.0.0"
           }
         },
         "number-is-nan": {
           "version": "1.0.1",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "object-assign": {
           "version": "4.1.1",
@@ -4970,9 +4958,8 @@
           "version": "1.4.0",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
-            "wrappy": "1.0.2"
+            "wrappy": "1"
           }
         },
         "os-homedir": {
@@ -4993,8 +4980,8 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "os-homedir": "1.0.2",
-            "os-tmpdir": "1.0.2"
+            "os-homedir": "^1.0.0",
+            "os-tmpdir": "^1.0.0"
           }
         },
         "path-is-absolute": {
@@ -5015,10 +5002,10 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "deep-extend": "0.5.1",
-            "ini": "1.3.5",
-            "minimist": "1.2.0",
-            "strip-json-comments": "2.0.1"
+            "deep-extend": "^0.5.1",
+            "ini": "~1.3.0",
+            "minimist": "^1.2.0",
+            "strip-json-comments": "~2.0.1"
           },
           "dependencies": {
             "minimist": {
@@ -5035,13 +5022,13 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "core-util-is": "1.0.2",
-            "inherits": "2.0.3",
-            "isarray": "1.0.0",
-            "process-nextick-args": "2.0.0",
-            "safe-buffer": "5.1.1",
-            "string_decoder": "1.1.1",
-            "util-deprecate": "1.0.2"
+            "core-util-is": "~1.0.0",
+            "inherits": "~2.0.3",
+            "isarray": "~1.0.0",
+            "process-nextick-args": "~2.0.0",
+            "safe-buffer": "~5.1.1",
+            "string_decoder": "~1.1.1",
+            "util-deprecate": "~1.0.1"
           }
         },
         "rimraf": {
@@ -5050,7 +5037,7 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "glob": "7.1.2"
+            "glob": "^7.0.5"
           }
         },
         "safe-buffer": {
@@ -5092,11 +5079,10 @@
           "version": "1.0.2",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
-            "code-point-at": "1.1.0",
-            "is-fullwidth-code-point": "1.0.0",
-            "strip-ansi": "3.0.1"
+            "code-point-at": "^1.0.0",
+            "is-fullwidth-code-point": "^1.0.0",
+            "strip-ansi": "^3.0.0"
           }
         },
         "string_decoder": {
@@ -5105,7 +5091,7 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "safe-buffer": "5.1.1"
+            "safe-buffer": "~5.1.0"
           }
         },
         "strip-ansi": {
@@ -5113,7 +5099,7 @@
           "bundled": true,
           "dev": true,
           "requires": {
-            "ansi-regex": "2.1.1"
+            "ansi-regex": "^2.0.0"
           }
         },
         "strip-json-comments": {
@@ -5128,13 +5114,13 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "chownr": "1.0.1",
-            "fs-minipass": "1.2.5",
-            "minipass": "2.2.4",
-            "minizlib": "1.1.0",
-            "mkdirp": "0.5.1",
-            "safe-buffer": "5.1.1",
-            "yallist": "3.0.2"
+            "chownr": "^1.0.1",
+            "fs-minipass": "^1.2.5",
+            "minipass": "^2.2.4",
+            "minizlib": "^1.1.0",
+            "mkdirp": "^0.5.0",
+            "safe-buffer": "^5.1.1",
+            "yallist": "^3.0.2"
           }
         },
         "util-deprecate": {
@@ -5149,7 +5135,7 @@
           "dev": true,
           "optional": true,
           "requires": {
-            "string-width": "1.0.2"
+            "string-width": "^1.0.2"
           }
         },
         "wrappy": {

package.json

@@ -47,7 +47,6 @@
     "eslint-plugin-node": "^5.2.1",
     "eslint-plugin-promise": "^3.8.0",
     "eslint-plugin-standard": "^3.1.0",
-    "ethereumjs-util": "^5.2.0",
     "ethjs-abi": "^0.2.1",
     "ganache-cli": "6.1.0",
     "solidity-coverage": "^0.5.4",

test/AutoIncrementing.test.js

@@ -1,5 +1,3 @@
-const { hashMessage } = require('./helpers/sign');
-
 const AutoIncrementing = artifacts.require('AutoIncrementingImpl');
 
 require('chai')
@@ -7,8 +5,8 @@ require('chai')
   .should();
 
 const EXPECTED = [1, 2, 3, 4];
-const KEY1 = hashMessage('key1');
-const KEY2 = hashMessage('key2');
+const KEY1 = web3.sha3('key1');
+const KEY2 = web3.sha3('key2');
 
 contract('AutoIncrementing', function ([_, owner]) {
   beforeEach(async function () {

test/helpers/sign.js

@@ -1,26 +1,21 @@
-const utils = require('ethereumjs-util');
-const { soliditySha3 } = require('web3-utils');
+const { sha3, soliditySha3 } = require('web3-utils');
 
 const REAL_SIGNATURE_SIZE = 2 * 65; // 65 bytes in hexadecimal string legnth
 const PADDED_SIGNATURE_SIZE = 2 * 96; // 96 bytes in hexadecimal string length
 
 const DUMMY_SIGNATURE = `0x${web3.padLeft('', REAL_SIGNATURE_SIZE)}`;
 
-/**
- * Hash and add same prefix to the hash that ganache use.
- * @param {string} message the plaintext/ascii/original message
- * @return {string} the hash of the message, prefixed, and then hashed again
- */
-function hashMessage (message) {
-  const messageHex = Buffer.from(utils.sha3(message).toString('hex'), 'hex');
-  const prefix = utils.toBuffer('\u0019Ethereum Signed Message:\n' + messageHex.length.toString());
-  return utils.bufferToHex(utils.sha3(Buffer.concat([prefix, messageHex])));
+// messageHex = '0xdeadbeef'
+function toEthSignedMessageHash (messageHex) {
+  const messageBuffer = Buffer.from(messageHex.substring(2), 'hex');
+  const prefix = Buffer.from(`\u0019Ethereum Signed Message:\n${messageBuffer.length}`);
+  return sha3(Buffer.concat([prefix, messageBuffer]));
 }
 
-// signs message in node (auto-applies prefix)
-// message must be in hex already! will not be autoconverted!
-const signMessage = (signer, message = '') => {
-  return web3.eth.sign(signer, message);
+// signs message in node (ganache auto-applies "Ethereum Signed Message" prefix)
+// messageHex = '0xdeadbeef'
+const signMessage = (signer, messageHex = '0x') => {
+  return web3.eth.sign(signer, messageHex); // actually personal_sign
 };
 
 // @TODO - remove this when we migrate to web3-1.0.0
@@ -62,18 +57,18 @@ const getBouncerSigner = (contract, signer) => (redeemer, methodName, methodArgs
     } else {
       const abi = contract.abi.find(abi => abi.name === methodName);
       const name = transformToFullName(abi);
-      const signature = web3.sha3(name).slice(0, 10);
+      const signature = sha3(name).slice(0, 10);
       parts.push(signature);
     }
   }
 
-  // ^ substr to remove `0x` because in solidity the address is a set of byes, not a string `0xabcd`
-  const hashOfMessage = soliditySha3(...parts);
-  return signMessage(signer, hashOfMessage);
+  // return the signature of the "Ethereum Signed Message" hash of the hash of `parts`
+  const messageHex = soliditySha3(...parts);
+  return signMessage(signer, messageHex);
 };
 
 module.exports = {
-  hashMessage,
   signMessage,
+  toEthSignedMessageHash,
   getBouncerSigner,
 };

test/library/ECRecovery.test.js

@@ -1,4 +1,4 @@
-const { hashMessage, signMessage } = require('../helpers/sign');
+const { signMessage, toEthSignedMessageHash } = require('../helpers/sign');
 const { expectThrow } = require('../helpers/expectThrow');
 
 const ECRecoveryMock = artifacts.require('ECRecoveryMock');
@@ -6,67 +6,61 @@ const ECRecoveryMock = artifacts.require('ECRecoveryMock');
 require('chai')
   .should();
 
-contract('ECRecovery', function ([_, anyone]) {
-  let ecrecovery;
-  const TEST_MESSAGE = 'OpenZeppelin';
+const TEST_MESSAGE = web3.sha3('OpenZeppelin');
+const WRONG_MESSAGE = web3.sha3('Nope');
 
+contract('ECRecovery', function ([_, anyone]) {
   beforeEach(async function () {
-    ecrecovery = await ECRecoveryMock.new();
+    this.mock = await ECRecoveryMock.new();
   });
 
   it('recover v0', async function () {
     // Signature generated outside ganache with method web3.eth.sign(signer, message)
     const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
-    const message = web3.sha3(TEST_MESSAGE);
     // eslint-disable-next-line max-len
     const signature = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be89200';
-    (await ecrecovery.recover(message, signature)).should.equal(signer);
+    (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
   });
 
   it('recover v1', async function () {
     // Signature generated outside ganache with method web3.eth.sign(signer, message)
     const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
-    const message = web3.sha3(TEST_MESSAGE);
     // eslint-disable-next-line max-len
     const signature = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e001';
-    (await ecrecovery.recover(message, signature)).should.equal(signer);
+    (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
   });
 
   it('recover using web3.eth.sign()', async function () {
-    // Create the signature using account[0]
-    const signature = signMessage(anyone, web3.sha3(TEST_MESSAGE));
+    // Create the signature
+    const signature = signMessage(anyone, TEST_MESSAGE);
 
     // Recover the signer address from the generated message and signature.
-    (await ecrecovery.recover(
-      hashMessage(TEST_MESSAGE),
+    (await this.mock.recover(
+      toEthSignedMessageHash(TEST_MESSAGE),
       signature
     )).should.equal(anyone);
   });
 
   it('recover using web3.eth.sign() should return wrong signer', async function () {
-    // Create the signature using account[0]
-    const signature = signMessage(anyone, web3.sha3(TEST_MESSAGE));
+    // Create the signature
+    const signature = signMessage(anyone, TEST_MESSAGE);
 
     // Recover the signer address from the generated message and wrong signature.
-    (await ecrecovery.recover(hashMessage('Nope'), signature)).should.not.equal(anyone);
+    (await this.mock.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone);
   });
 
-  it('recover should revert when a small hash is sent', async function () {
-    // Create the signature using account[0]
+  // @TODO - remove `skip` once we upgrade to solc^0.5
+  it.skip('recover should revert when a small hash is sent', async function () {
+    // Create the signature
     const signature = signMessage(anyone, TEST_MESSAGE);
-    try {
-      await expectThrow(
-        ecrecovery.recover(hashMessage(TEST_MESSAGE).substring(2), signature)
-      );
-    } catch (error) {
-      // @TODO(shrugs) - remove this once we upgrade to solc^0.5
-    }
+    await expectThrow(
+      this.mock.recover(TEST_MESSAGE.substring(2), signature)
+    );
   });
 
   context('toEthSignedMessage', function () {
     it('should prefix hashes correctly', async function () {
-      const hashedMessage = web3.sha3(TEST_MESSAGE);
-      (await ecrecovery.toEthSignedMessageHash(hashedMessage)).should.equal(hashMessage(TEST_MESSAGE));
+      (await this.mock.toEthSignedMessageHash(TEST_MESSAGE)).should.equal(toEthSignedMessageHash(TEST_MESSAGE));
     });
   });
 });