add support for new versions of ethereumjs/tx

.prettierrc.js

@@ -0,0 +1,10 @@
+/**
+ * This file just exists to enable devs that use vscode to benefit from auto
+ * formatting on save when using prettier plugin and the require config file
+ * setting. It grabs the config from the shared eslint-config and re-exports
+ * it to prevent any issues with mismatched settings
+ */
+const config = require('@metamask/eslint-config');
+const prettierConfig = config.rules[`prettier/prettier`][1];
+
+module.exports = prettierConfig;

index.js

@@ -1,8 +1,8 @@
 const { EventEmitter } = require('events');
 const ethUtil = require('ethereumjs-util');
-const Transaction = require('ethereumjs-tx');
 const HDKey = require('hdkey');
 const TrezorConnect = require('trezor-connect').default;
+const { TransactionFactory } = require('@ethereumjs/tx');
 
 const hdPathString = `m/44'/60'/0'/0`;
 const keyringType = 'Trezor Hardware';
@@ -14,6 +14,10 @@ const TREZOR_CONNECT_MANIFEST = {
   appUrl: 'https://metamask.io',
 };
 
+function wait(ms) {
+  return new Promise((resolve) => setTimeout(resolve, ms));
+}
+
 class TrezorKeyring extends EventEmitter {
   constructor(opts = {}) {
     super();
@@ -167,65 +171,101 @@ class TrezorKeyring extends EventEmitter {
 
   // tx is an instance of the ethereumjs-transaction class.
   signTransaction(address, tx) {
-    return new Promise((resolve, reject) => {
-      this.unlock()
-        .then((status) => {
-          setTimeout(
-            (_) => {
-              TrezorConnect.ethereumSignTransaction({
-                path: this._pathFromAddress(address),
-                transaction: {
-                  to: this._normalize(tx.to),
-                  value: this._normalize(tx.value),
-                  data: this._normalize(tx.data),
-                  chainId: tx._chainId,
-                  nonce: this._normalize(tx.nonce),
-                  gasLimit: this._normalize(tx.gasLimit),
-                  gasPrice: this._normalize(tx.gasPrice),
-                },
-              })
-                .then((response) => {
-                  if (response.success) {
-                    tx.v = response.payload.v;
-                    tx.r = response.payload.r;
-                    tx.s = response.payload.s;
-
-                    const signedTx = new Transaction(tx);
-
-                    const addressSignedWith = ethUtil.toChecksumAddress(
-                      `0x${signedTx.from.toString('hex')}`,
-                    );
-                    const correctAddress = ethUtil.toChecksumAddress(address);
-                    if (addressSignedWith !== correctAddress) {
-                      reject(
-                        new Error('signature doesnt match the right address'),
-                      );
-                    }
+    // transactions built with older versions of ethereumjs-tx have a
+    // getChainId method that newer versions do not. Older versions are mutable
+    // while newer versions default to being immutable. Expected shape and type
+    // of data for v, r and s differ (Buffer (old) vs BN (new))
+    if (typeof tx.getChainId === 'function') {
+      // In this version of ethereumjs-tx we must add the chainId in hex format
+      // to the initial v value. The chainId must be included in the serialized
+      // transaction which is only communicated to ethereumjs-tx in this
+      // value. In newer versions the chainId is communicated via the 'Common'
+      // object.
+      return this._signTransaction(address, tx.getChainId(), tx, (payload) => {
+        tx.v = Buffer.from(payload.v, 'hex');
+        tx.r = Buffer.from(payload.r, 'hex');
+        tx.s = Buffer.from(payload.s, 'hex');
+        return tx;
+      });
+    }
+    // For transactions created by newer versions of @ethereumjs/tx
+    // Note: https://github.com/ethereumjs/ethereumjs-monorepo/issues/1188
+    // It is not strictly necessary to do this additional setting of the v
+    // value. We should be able to get the correct v value in serialization
+    // if the above issue is resolved. Until then this must be set before
+    // calling .serialize(). Note we are creating a temporarily mutable object
+    // forfeiting the benefit of immutability until this happens. We do still
+    // return a Transaction that is frozen if the originally provided
+    // transaction was also frozen.
+    const unfrozenTx = TransactionFactory.fromTxData(tx.toJSON(), {
+      common: tx.common,
+      freeze: false,
+    });
+    unfrozenTx.v = new ethUtil.BN(
+      ethUtil.addHexPrefix(tx.common.chainId()),
+      'hex',
+    );
+    return this._signTransaction(
+      address,
+      tx.common.chainIdBN().toNumber(),
+      unfrozenTx,
+      (payload) => {
+        // Because tx will be immutable, first get a plain javascript object that
+        // represents the transaction. Using txData here as it aligns with the
+        // nomenclature of ethereumjs/tx.
+        const txData = tx.toJSON();
+        // The fromTxData utility expects v,r and s to be hex prefixed
+        txData.v = ethUtil.addHexPrefix(payload.v);
+        txData.r = ethUtil.addHexPrefix(payload.r);
+        txData.s = ethUtil.addHexPrefix(payload.s);
+        // Adopt the 'common' option from the original transaction and set the
+        // returned object to be frozen if the original is frozen.
+        return TransactionFactory.fromTxData(txData, {
+          common: tx.common,
+          freeze: Object.isFrozen(tx),
+        });
+      },
+    );
+  }
 
-                    resolve(signedTx);
-                  } else {
-                    reject(
-                      new Error(
-                        (response.payload && response.payload.error) ||
-                          'Unknown error',
-                      ),
-                    );
-                  }
-                })
-                .catch((e) => {
-                  reject(new Error((e && e.toString()) || 'Unknown error'));
-                });
+  // tx is an instance of the ethereumjs-transaction class.
+  async _signTransaction(address, chainId, tx, handleSigning) {
+    try {
+      const status = await this.unlock();
+      await wait(status === 'just unlocked' ? DELAY_BETWEEN_POPUPS : 0);
+      const response = await TrezorConnect.ethereumSignTransaction({
+        path: this._pathFromAddress(address),
+        transaction: {
+          to: this._normalize(tx.to),
+          value: this._normalize(tx.value),
+          data: this._normalize(tx.data),
+          chainId,
+          nonce: this._normalize(tx.nonce),
+          gasLimit: this._normalize(tx.gasLimit),
+          gasPrice: this._normalize(tx.gasPrice),
+        },
+      });
+      if (response.success) {
+        const newOrMutatedTx = handleSigning(response.payload);
+
+        const addressSignedWith = ethUtil.toChecksumAddress(
+          ethUtil.addHexPrefix(
+            newOrMutatedTx.getSenderAddress().toString('hex'),
+          ),
+        );
+        const correctAddress = ethUtil.toChecksumAddress(address);
+        if (addressSignedWith !== correctAddress) {
+          throw new Error("signature doesn't match the right address");
+        }
 
-              // This is necessary to avoid popup collision
-              // between the unlock & sign trezor popups
-            },
-            status === 'just unlocked' ? DELAY_BETWEEN_POPUPS : 0,
-          );
-        })
-        .catch((e) => {
-          reject(new Error((e && e.toString()) || 'Unknown error'));
-        });
-    });
+        return newOrMutatedTx;
+      }
+      throw new Error(
+        (response.payload && response.payload.error) || 'Unknown error',
+      );
+    } catch (e) {
+      throw new Error((e && e.toString()) || 'Unknown error');
+    }
   }
 
   signMessage(withAccount, data) {
@@ -266,7 +306,6 @@ class TrezorKeyring extends EventEmitter {
                   }
                 })
                 .catch((e) => {
-                  console.log('Error while trying to sign a message ', e);
                   reject(new Error((e && e.toString()) || 'Unknown error'));
                 });
               // This is necessary to avoid popup collision
@@ -276,7 +315,6 @@ class TrezorKeyring extends EventEmitter {
           );
         })
         .catch((e) => {
-          console.log('Error while trying to sign a message ', e);
           reject(new Error((e && e.toString()) || 'Unknown error'));
         });
     });

package.json

@@ -34,17 +34,18 @@
     "mocha/mkdirp": "^0.5.1"
   },
   "dependencies": {
+    "@ethereumjs/tx": "^3.1.4",
     "eth-sig-util": "^1.4.2",
     "ethereumjs-tx": "^1.3.4",
     "ethereumjs-util": "^7.0.9",
     "hdkey": "0.8.0",
     "trezor-connect": "8.1.23-extended"
   },
   "devDependencies": {
+    "@ethereumjs/common": "^2.2.0",
     "@metamask/eslint-config": "^6.0.0",
     "@metamask/eslint-config-mocha": "^6.0.0",
     "@metamask/eslint-config-nodejs": "^6.0.0",
-    "babel-eslint": "^10.1.0",
     "chai": "^4.1.2",
     "chai-spies": "^1.0.0",
     "eslint": "^7.26.0",

test/test-eth-trezor-keyring.js

@@ -10,6 +10,8 @@ const sinon = require('sinon');
 const EthereumTx = require('ethereumjs-tx');
 const HDKey = require('hdkey');
 const TrezorConnect = require('trezor-connect').default;
+const { TransactionFactory } = require('@ethereumjs/tx');
+const Common = require('@ethereumjs/common').default;
 
 const TrezorKeyring = require('..');
 
@@ -49,6 +51,19 @@ const fakeTx = new EthereumTx({
   chainId: 1,
 });
 
+const common = new Common({ chain: 'mainnet' });
+const newFakeTx = TransactionFactory.fromTxData(
+  {
+    nonce: '0x00',
+    gasPrice: '0x09184e72a000',
+    gasLimit: '0x2710',
+    to: '0x0000000000000000000000000000000000000000',
+    value: '0x00',
+    data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057',
+  },
+  { common, freeze: false },
+);
+
 chai.use(spies);
 
 describe('TrezorKeyring', function () {
@@ -328,19 +343,54 @@ describe('TrezorKeyring', function () {
   });
 
   describe('signTransaction', function () {
-    it('should call TrezorConnect.ethereumSignTransaction', function (done) {
-      sinon
-        .stub(TrezorConnect, 'ethereumSignTransaction')
-        .callsFake(() => Promise.resolve({}));
+    it('should pass serialized transaction to trezor and return signed tx', async function () {
+      sinon.stub(TrezorConnect, 'ethereumSignTransaction').callsFake(() => {
+        return Promise.resolve({
+          success: true,
+          payload: { v: '0x1', r: '0x0', s: '0x0' },
+        });
+      });
+      sinon.stub(fakeTx, 'verifySignature').callsFake(() => true);
+      sinon.stub(fakeTx, 'getSenderAddress').callsFake(() => fakeAccounts[0]);
+
+      const returnedTx = await keyring.signTransaction(fakeAccounts[0], fakeTx);
+      // assert that the v,r,s values got assigned to tx.
+      assert.ok(returnedTx.v);
+      assert.ok(returnedTx.r);
+      assert.ok(returnedTx.s);
+      // ensure we get a older version transaction back
+      assert.equal(returnedTx.getChainId(), 1);
+      assert.equal(returnedTx.common, undefined);
+      assert(TrezorConnect.ethereumSignTransaction.calledOnce);
+    });
 
-      keyring.signTransaction(fakeAccounts[0], fakeTx).catch((_) => {
-        // Since we only care about ensuring our function gets called,
-        // we want to ignore warnings due to stub data
+    it('should pass serialized newer transaction to trezor and return signed tx', async function () {
+      sinon.stub(TransactionFactory, 'fromTxData').callsFake(() => {
+        // without having a private key/public key pair in this test, we have
+        // mock out this method and return the original tx because we can't
+        // replicate r and s values without the private key.
+        return newFakeTx;
       });
-      setTimeout(() => {
-        assert(TrezorConnect.ethereumSignTransaction.calledOnce);
-        done();
-      }, SIGNING_DELAY);
+      sinon.stub(TrezorConnect, 'ethereumSignTransaction').callsFake(() => {
+        return Promise.resolve({
+          success: true,
+          payload: { v: '0x25', r: '0x0', s: '0x0' },
+        });
+      });
+      sinon
+        .stub(newFakeTx, 'getSenderAddress')
+        .callsFake(() => fakeAccounts[0]);
+      sinon.stub(newFakeTx, 'verifySignature').callsFake(() => true);
+      // sinon.stub(newFakeTx, 'from').get(() => fakeAccounts[0]);
+
+      const returnedTx = await keyring.signTransaction(
+        fakeAccounts[0],
+        newFakeTx,
+      );
+      // ensure we get a new version transaction back
+      assert.equal(returnedTx.getChainId, undefined);
+      assert.equal(returnedTx.common.chainIdBN().toString('hex'), '1');
+      assert(TrezorConnect.ethereumSignTransaction.calledOnce);
     });
   });