Base rlp encoded logic in signTransaction on return type of getMessag…

…eToSign
MetaMask · Aug 27, 2021 · e7be828 · e7be828
1 parent 3c22af7
commit e7be828
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 17 deletions.
diff --git a/index.js b/index.js
@@ -252,9 +252,11 @@ class LedgerBridgeKeyring extends EventEmitter {
     // Note also that `getMessageToSign` will return valid RLP for all transaction types, whereas the
     // `serialize` method will not for any transaction type except legacy. This is because `serialize` includes
     // empty r, s and v values in the encoded rlp. This is why we use `getMessageToSign` here instead of `serialize`.
-    rawTxHex = tx.type === 0 || !tx.type
-      ? ethUtil.rlp.encode(tx.getMessageToSign(false)).toString('hex')
-      : tx.getMessageToSign(false).toString('hex')
+    const messageToSign = tx.getMessageToSign(false)
+
+    rawTxHex = Buffer.isBuffer(messageToSign)
+      ? messageToSign.toString('hex')
+      : ethUtil.rlp.encode(messageToSign).toString('hex')
 
     return this._signTransaction(address, rawTxHex, (payload) => {
       // Because tx will be immutable, first get a plain javascript object that
@@ -274,7 +276,6 @@ class LedgerBridgeKeyring extends EventEmitter {
   }
 
   _signTransaction (address, rawTxHex, handleSigning) {
-
     return new Promise((resolve, reject) => {
       this.unlockAccountByAddress(address)
         .then((hdPath) => {

diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js
@@ -48,6 +48,7 @@ const fakeTx = new EthereumTx({
 })
 
 const common = new Common({ chain: 'mainnet' })
+const commonEIP1559 = Common.forCustomChain('mainnet', {}, 'london')
 const newFakeTx = TransactionFactory.fromTxData({
   nonce: '0x00',
   gasPrice: '0x09184e72a000',
@@ -57,6 +58,18 @@ const newFakeTx = TransactionFactory.fromTxData({
   data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057',
 }, { common, freeze: false })
 
+const fakeTypeTwoTx = TransactionFactory.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('LedgerBridgeKeyring', function () {
@@ -456,30 +469,52 @@ describe('LedgerBridgeKeyring', function () {
     })
 
     describe('using new versions of ethereumjs/tx', function () {
-      it('should pass serialized transaction to ledger and return signed tx', async function () {
+      it('should pass correctly encoded legacy transaction to ledger and return signed tx', async function () {
+        // Generated by signing newFakeTx with private key eee0290acfa88cf7f97be7525437db1624293f829b8a2cba380390618d62662b
+        const expectedRSV = {
+          v: '0x26',
+          r: '0xf3a7718999d1b87beda810b25cc025153e74df0745279826b9b2f3d1d1b6318',
+          s: '0x7e33bdfbf5272dc4f55649e9ba729849670171a68ef8c0fbeed3b879b90b8954',
+        }
+
         await basicSetupToUnlockOneAccount()
-        // Signature will be invalid due to not having private key / public key
-        // pair for testing.
+
         sandbox.on(newFakeTx, 'verifySignature', () => true)
-        sandbox.on(TransactionFactory, 'fromTxData', () => {
-          // 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
-        })
         sandbox.on(keyring, '_sendMessage', (msg, cb) => {
           assert.deepStrictEqual(msg.params, {
             hdPath: "m/44'/60'/0'/0",
             tx: ethUtil.rlp.encode(newFakeTx.getMessageToSign(false)).toString('hex'),
           })
-          cb({ success: true, payload: { v: '0x25', r: '0x0', s: '0x0' } })
+          cb({ success: true, payload: expectedRSV })
         })
 
         const returnedTx = await keyring.signTransaction(fakeAccounts[0], newFakeTx, common)
         expect(keyring._sendMessage).to.have.been.called()
-        expect(returnedTx).to.have.property('v')
-        expect(returnedTx).to.have.property('r')
-        expect(returnedTx).to.have.property('s')
+        expect(returnedTx.toJSON()).to.deep.equal({ ...newFakeTx.toJSON(), ...expectedRSV })
+      })
+
+      it('should pass correctly encoded EIP1559 transaction to ledger and return signed tx', async function () {
+        // Generated by signing fakeTypeTwoTx with private key eee0290acfa88cf7f97be7525437db1624293f829b8a2cba380390618d62662b
+        const expectedRSV = {
+          v: '0x0',
+          r: '0x5ffb3adeaec80e430e7a7b02d95c5108b6f09a0bdf3cf69869dc1b38d0fb8d3a',
+          s: '0x28b234a5403d31564e18258df84c51a62683e3f54fa2b106fdc1a9058006a112',
+        }
+
+        await basicSetupToUnlockOneAccount()
+
+        sandbox.on(fakeTypeTwoTx, 'verifySignature', () => true)
+        sandbox.on(keyring, '_sendMessage', (msg, cb) => {
+          assert.deepStrictEqual(msg.params, {
+            hdPath: "m/44'/60'/0'/0",
+            tx: fakeTypeTwoTx.getMessageToSign(false).toString('hex'),
+          })
+          cb({ success: true, payload: expectedRSV })
+        })
+
+        const returnedTx = await keyring.signTransaction(fakeAccounts[0], fakeTypeTwoTx, commonEIP1559)
+        expect(keyring._sendMessage).to.have.been.called()
+        expect(returnedTx.toJSON()).to.deep.equal({ ...fakeTypeTwoTx.toJSON(), ...expectedRSV })
       })
     })
   })