Add eip 1559 support for Trezor Model T (#108)

* Upgrade trezor-connect to 8.2.1 This adds support for EIP-1559 for the Trezor T, once we get all this library to work with it as well. We use version "8.2.1-extended" exactly, as we need non-browser support for local tests. * Update @ethereumjs/common to 2.4.0 for 1559 tests * Upgrade @ethereumjs/tx dependency to ^3.2.1 Although 3.3.0 is out, I've selected ^3.2.1 just to match the current version of MetaMask's package.json file. Version 3.2.0 adds support for EIP-1559 transactions. * Test EIP-1559 transaction support Test case copied over from MetaMask/eth-ledger-bridge-keyring#99 * Set ESLint ecmaVersion to 2018 This adds support for object-rest-spread (e.g. {...x, ...y}), without using the babel parser, which we use in tests. This is different from what the Metamask extension has, which is 2017, but they're using the babel parser. * Add EIP-1559 transaction support Creating an unfrozen transaction (added in #88) seems to be a change that was only required in eth-ledger-keyring, not in Trezor, and is fixed by @ethereumjs/tx: v3.1.4 anyway. I've removed this part, since it was causing issues with EIP-1559 transactions, and does not seem necessary for non-EIP-1559 transactions either. * Document EIP1559 support for Model T on README.md * Add method to expose trezor model * Lint fix * Update index.js Co-authored-by: Alois Klink <[email protected]> * Use event listener instead of getFeatures call to set model * Lint fix * Revert getEIP1559Support to getModel * Move event listener first and trigger event via init * Lint fix * Remove error handle in getModel method Co-authored-by: Alois Klink <[email protected]> Co-authored-by: David Walsh <[email protected]>
MetaMask · Nov 18, 2021 · b41303b · b41303b
1 parent c6cb014
commit b41303b
Show file tree

Hide file tree

Showing 6 changed files with 355 additions and 273 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -1,5 +1,8 @@
 module.exports = {
   root: true,
+  parserOptions: {
+    ecmaVersion: 2018, // to support object rest spread, e.g. {...x, ...y}
+  },
 
   extends: ['@metamask/eslint-config', '@metamask/eslint-config-nodejs'],
 

diff --git a/README.md b/README.md
@@ -13,6 +13,8 @@ device. However there are a number of differences:
 - It does not support the `signMessage`, `signTypedData` or `exportAccount`
   methods, because TREZOR devices do not support these operations.
 - The method `signPersonalMessage` requires the firmware version 2.0.7+ for TREZOR Model T and 1.6.2+ on TREZOR ONE
+- As of `trezor-connect`: `8.2.1`, passing an EIP-1559 transaction to `signTransaction`
+  requires the firmware version 2.4.2+ for TREZOR Model T, and is unsupported on all firmwares for TREZOR ONE.
 
 ## Using
 

diff --git a/index.js b/index.js
@@ -25,6 +25,28 @@ function wait(ms) {
   return new Promise((resolve) => setTimeout(resolve, ms));
 }
 
+/**
+ * @typedef {import('@ethereumjs/tx').TypedTransaction} TypedTransaction
+ * @typedef {InstanceType<import("ethereumjs-tx")>} OldEthJsTransaction
+ */
+
+/**
+ * Check if the given transaction is made with ethereumjs-tx or @ethereumjs/tx
+ *
+ * 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)).
+ *
+ * @param {TypedTransaction | OldEthJsTransaction} tx
+ * @returns {tx is OldEthJsTransaction} Returns `true` if tx is an old-style ethereumjs-tx transaction.
+ */
+function isOldStyleEthereumjsTx(tx) {
+  return typeof tx.getChainId === 'function';
+}
+
 class TrezorKeyring extends EventEmitter {
   constructor(opts = {}) {
     super();
@@ -36,7 +58,17 @@ class TrezorKeyring extends EventEmitter {
     this.unlockedAccount = 0;
     this.paths = {};
     this.deserialize(opts);
-    TrezorConnect.manifest(TREZOR_CONNECT_MANIFEST);
+
+    TrezorConnect.on('DEVICE_EVENT', (event) => {
+      if (event && event.payload && event.payload.features) {
+        this.model = event.payload.features.model;
+      }
+    });
+    TrezorConnect.init({ manifest: TREZOR_CONNECT_MANIFEST });
+  }
+
+  getModel() {
+    return this.model;
   }
 
   serialize() {
@@ -177,13 +209,20 @@ class TrezorKeyring extends EventEmitter {
     );
   }
 
-  // tx is an instance of the ethereumjs-transaction class.
+  /**
+   * Signs a transaction using Trezor.
+   *
+   * Accepts either an ethereumjs-tx or @ethereumjs/tx transaction, and returns
+   * the same type.
+   *
+   * @template {TypedTransaction | OldEthJsTransaction} Transaction
+   * @param {string} address - Hex string address.
+   * @param {Transaction} tx - Instance of either new-style or old-style ethereumjs transaction.
+   * @returns {Promise<Transaction>} The signed transaction, an instance of either new-style or old-style
+   * ethereumjs transaction.
+   */
   signTransaction(address, tx) {
-    // 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') {
+    if (isOldStyleEthereumjsTx(tx)) {
       // 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
@@ -196,32 +235,17 @@ class TrezorKeyring extends EventEmitter {
         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,
+      tx,
       (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 a type to support transactions with a type other than 0
+        txData.type = tx.type;
         // The fromTxData utility expects v,r and s to be hex prefixed
         txData.v = ethUtil.addHexPrefix(payload.v);
         txData.r = ethUtil.addHexPrefix(payload.r);
@@ -236,22 +260,43 @@ class TrezorKeyring extends EventEmitter {
     );
   }
 
-  // tx is an instance of the ethereumjs-transaction class.
+  /**
+   *
+   * @template {TypedTransaction | OldEthJsTransaction} Transaction
+   * @param {string} address - Hex string address.
+   * @param {number} chainId - Chain ID
+   * @param {Transaction} tx - Instance of either new-style or old-style ethereumjs transaction.
+   * @param {(import('trezor-connect').EthereumSignedTx) => Transaction} handleSigning - Converts signed transaction
+   * to the same new-style or old-style ethereumjs-tx.
+   * @returns {Promise<Transaction>} The signed transaction, an instance of either new-style or old-style
+   * ethereumjs transaction.
+   */
   async _signTransaction(address, chainId, tx, handleSigning) {
+    let transaction;
+    if (isOldStyleEthereumjsTx(tx)) {
+      // legacy transaction from ethereumjs-tx package has no .toJSON() function,
+      // so we need to convert to hex-strings manually manually
+      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),
+      };
+    } else {
+      // new-style transaction from @ethereumjs/tx package
+      // we can just copy tx.toJSON() for everything except chainId, which must be a number
+      transaction = { ...tx.toJSON(), chainId };
+    }
+
     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),
-        },
+        transaction,
       });
       if (response.success) {
         const newOrMutatedTx = handleSigning(response.payload);

diff --git a/package.json b/package.json
@@ -34,13 +34,13 @@
     "mocha/mkdirp": "^0.5.1"
   },
   "dependencies": {
-    "@ethereumjs/tx": "^3.1.4",
+    "@ethereumjs/tx": "^3.2.1",
     "ethereumjs-util": "^7.0.9",
     "hdkey": "0.8.0",
-    "trezor-connect": "8.1.23-extended"
+    "trezor-connect": "8.2.1-extended"
   },
   "devDependencies": {
-    "@ethereumjs/common": "^2.2.0",
+    "@ethereumjs/common": "^2.4.0",
     "@lavamoat/allow-scripts": "^1.0.6",
     "@metamask/auto-changelog": "^2.3.0",
     "@metamask/eslint-config": "^8.0.0",

diff --git a/test/test-eth-trezor-keyring.js b/test/test-eth-trezor-keyring.js
@@ -10,8 +10,11 @@ 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 {
+  TransactionFactory,
+  FeeMarketEIP1559Transaction,
+} = require('@ethereumjs/tx');
+const { default: Common, Chain, Hardfork } = require('@ethereumjs/common');
 
 const TrezorKeyring = require('..');
 
@@ -52,6 +55,10 @@ const fakeTx = new EthereumTx({
 });
 
 const common = new Common({ chain: 'mainnet' });
+const commonEIP1559 = new Common({
+  chain: Chain.Mainnet,
+  hardfork: Hardfork.London,
+});
 const newFakeTx = TransactionFactory.fromTxData(
   {
     nonce: '0x00',
@@ -64,6 +71,21 @@ const newFakeTx = TransactionFactory.fromTxData(
   { common, freeze: false },
 );
 
+const fakeTypeTwoTx = FeeMarketEIP1559Transaction.fromTxData(
+  {
+    nonce: '0x00',
+    maxFeePerGas: '0x19184e72a000',
+    maxPriorityFeePerGas: '0x09184e72a000',
+    gasLimit: '0x2710',
+    to: '0x0000000000000000000000000000000000000000',
+    value: '0x00',
+    data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057',
+    type: 2,
+    v: '0x01',
+  },
+  { common: commonEIP1559, freeze: false },
+);
+
 chai.use(spies);
 
 describe('TrezorKeyring', function () {
@@ -393,6 +415,51 @@ describe('TrezorKeyring', function () {
       assert.equal(returnedTx.common.chainIdBN().toString('hex'), '1');
       assert(TrezorConnect.ethereumSignTransaction.calledOnce);
     });
+
+    it('should pass correctly encoded EIP1559 transaction to trezor and return signed tx', async function () {
+      // Copied from @MetaMask/eth-ledger-bridge-keyring
+      // Generated by signing fakeTypeTwoTx with an unknown private key
+      const expectedRSV = {
+        v: '0x0',
+        r: '0x5ffb3adeaec80e430e7a7b02d95c5108b6f09a0bdf3cf69869dc1b38d0fb8d3a',
+        s: '0x28b234a5403d31564e18258df84c51a62683e3f54fa2b106fdc1a9058006a112',
+      };
+      // Override actual address of 0x391535104b6e0Ea6dDC2AD0158aB3Fbd7F04ed1B
+      sinon.stub(TransactionFactory, 'fromTxData').callsFake((...args) => {
+        const tx = TransactionFactory.fromTxData.wrappedMethod(...args);
+        sinon.stub(tx, 'getSenderAddress').returns(fakeAccounts[0]);
+        return tx;
+      });
+
+      sinon
+        .stub(TrezorConnect, 'ethereumSignTransaction')
+        .callsFake((params) => {
+          expect(params.transaction).to.be.an('object');
+          // chainId must be a number, unlike other variables which can be hex-strings
+          expect(params.transaction)
+            .to.have.property('chainId')
+            .to.satisfy(Number.isInteger);
+          expect(params.transaction).to.have.property('maxFeePerGas');
+          expect(params.transaction).to.have.property('maxPriorityFeePerGas');
+          expect(params.transaction).to.not.have.property('gasPrice');
+          return Promise.resolve({
+            success: true,
+            payload: expectedRSV,
+          });
+        });
+
+      const returnedTx = await keyring.signTransaction(
+        fakeAccounts[0],
+        fakeTypeTwoTx,
+        commonEIP1559,
+      );
+
+      assert(TrezorConnect.ethereumSignTransaction.calledOnce);
+      expect(returnedTx.toJSON()).to.deep.equal({
+        ...fakeTypeTwoTx.toJSON(),
+        ...expectedRSV,
+      });
+    });
   });
 
   describe('signMessage', function () {