From 11f687b7f86a33006245799741bc0958bd1acec0 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 30 Aug 2021 19:44:05 -0230 Subject: [PATCH] Base rlp encoded logic in signTransaction on return type of getMessageToSign (#99) * Base rlp encoded logic in signTransaction on return type of getMessageToSign * Update ethereumjs common library version to support eip1559 txes in tests --- index.js | 9 ++-- package.json | 2 +- test/test-eth-ledger-bridge-keyring.js | 61 ++++++++++++++++++++------ yarn.lock | 8 ---- 4 files changed, 54 insertions(+), 26 deletions(-) diff --git a/index.js b/index.js index e071b8e9..17fa2e52 100644 --- 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/package.json b/package.json index e13c7afb..4016c6b1 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "hdkey": "0.8.0" }, "devDependencies": { - "@ethereumjs/common": "^2.2.0", + "@ethereumjs/common": "^2.4.0", "@metamask/eslint-config": "^3.0.0", "babel-eslint": "^10.1.0", "chai": "^4.1.2", diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js index b742a91f..a9fc63ee 100644 --- 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 }) }) }) }) diff --git a/yarn.lock b/yarn.lock index 67132898..5877466f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -109,14 +109,6 @@ lodash "^4.17.19" to-fast-properties "^2.0.0" -"@ethereumjs/common@^2.2.0": - version "2.2.0" - resolved "https://registry.yarnpkg.com/@ethereumjs/common/-/common-2.2.0.tgz#850a3e3e594ee707ad8d44a11e8152fb62450535" - integrity sha512-PyQiTG00MJtBRkJmv46ChZL8u2XWxNBeAthznAUIUiefxPAXjbkuiCZOuncgJS34/XkMbNc9zMt/PlgKRBElig== - dependencies: - crc-32 "^1.2.0" - ethereumjs-util "^7.0.9" - "@ethereumjs/common@^2.4.0": version "2.4.0" resolved "https://registry.yarnpkg.com/@ethereumjs/common/-/common-2.4.0.tgz#2d67f6e6ba22246c5c89104e6b9a119fb3039766"