From 3d5b3f9fe27730cdf64ad3efd29188aab975d100 Mon Sep 17 00:00:00 2001 From: jpuri Date: Mon, 9 May 2022 23:04:32 +0530 Subject: [PATCH 1/5] Fix in unlock function to always unlock ledger for current hdPath --- index.js | 4 ++-- test/test-eth-ledger-bridge-keyring.js | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index fb7d84dd..0185432b 100644 --- a/index.js +++ b/index.js @@ -146,7 +146,7 @@ class LedgerBridgeKeyring extends EventEmitter { addAccounts (n = 1) { return new Promise((resolve, reject) => { - this.unlock() + this.unlock(this.hdPath) .then(async (_) => { const from = this.unlockedAccount const to = from + n @@ -514,7 +514,7 @@ class LedgerBridgeKeyring extends EventEmitter { const from = (this.page - 1) * this.perPage const to = from + this.perPage - await this.unlock() + await this.unlock(this.hdPath) let accounts if (this._isLedgerLiveHdPath()) { accounts = await this._getAccountsBIP44(from, to) diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js index a9fc63ee..35831815 100644 --- a/test/test-eth-ledger-bridge-keyring.js +++ b/test/test-eth-ledger-bridge-keyring.js @@ -79,8 +79,8 @@ describe('LedgerBridgeKeyring', function () { async function basicSetupToUnlockOneAccount (accountIndex = 0) { keyring.setAccountToUnlock(accountIndex) - await keyring.addAccounts() sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[accountIndex])) + await keyring.addAccounts() } beforeEach(function () { @@ -242,6 +242,7 @@ describe('LedgerBridgeKeyring', function () { describe('addAccounts', function () { describe('with no arguments', function () { it('returns a single account', function (done) { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts() .then((accounts) => { @@ -253,6 +254,7 @@ describe('LedgerBridgeKeyring', function () { describe('with a numeric argument', function () { it('returns that number of accounts', function (done) { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts(5) .then((accounts) => { @@ -262,6 +264,7 @@ describe('LedgerBridgeKeyring', function () { }) it('returns the expected accounts', function (done) { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts(3) .then((accounts) => { @@ -287,6 +290,7 @@ describe('LedgerBridgeKeyring', function () { }) it('stores account details for non-bip44 accounts', function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setHdPath(`m/44'/60'/0'`) keyring.setAccountToUnlock(2) return keyring.addAccounts(1) @@ -300,6 +304,7 @@ describe('LedgerBridgeKeyring', function () { describe('when called multiple times', function () { it('should not remove existing accounts', function (done) { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts(1) .then(function () { @@ -319,6 +324,7 @@ describe('LedgerBridgeKeyring', function () { describe('removeAccount', function () { describe('if the account exists', function () { it('should remove that account', function (done) { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts() .then(async (accounts) => { @@ -343,12 +349,14 @@ describe('LedgerBridgeKeyring', function () { describe('getFirstPage', function () { it('should set the currentPage to 1', async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) await keyring.getFirstPage() assert.equal(keyring.page, 1) }) it('should return the list of accounts for current page', async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) const accounts = await keyring.getFirstPage() expect(accounts.length, keyring.perPage) @@ -363,6 +371,7 @@ describe('LedgerBridgeKeyring', function () { describe('getNextPage', function () { it('should return the list of accounts for current page', async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) const accounts = await keyring.getNextPage() expect(accounts.length, keyring.perPage) expect(accounts[0].address, fakeAccounts[0]) @@ -376,6 +385,7 @@ describe('LedgerBridgeKeyring', function () { describe('getPreviousPage', function () { it('should return the list of accounts for current page', async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) // manually advance 1 page await keyring.getNextPage() const accounts = await keyring.getPreviousPage() @@ -389,6 +399,7 @@ describe('LedgerBridgeKeyring', function () { }) it('should be able to go back to the previous page', async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) // manually advance 1 page await keyring.getNextPage() const accounts = await keyring.getPreviousPage() @@ -406,6 +417,7 @@ describe('LedgerBridgeKeyring', function () { const accountIndex = 5 let accounts = [] beforeEach(async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[accountIndex])) keyring.setAccountToUnlock(accountIndex) await keyring.addAccounts() accounts = await keyring.getAccounts() @@ -432,6 +444,7 @@ describe('LedgerBridgeKeyring', function () { describe('forgetDevice', function () { it('should clear the content of the keyring', async function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) // Add an account keyring.setAccountToUnlock(0) await keyring.addAccounts() @@ -565,9 +578,9 @@ describe('LedgerBridgeKeyring', function () { it('should reject if the account is not found on device', async function () { const requestedAccount = fakeAccounts[0] const incorrectAccount = fakeAccounts[1] + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(incorrectAccount)) keyring.setAccountToUnlock(0) await keyring.addAccounts() - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(incorrectAccount)) assert.rejects(() => keyring.unlockAccountByAddress(requestedAccount), new Error(`Ledger: Account ${fakeAccounts[0]} does not belong to the connected device`)) }) From 62ab6e670de56b7ee61e854f52b349545da137d0 Mon Sep 17 00:00:00 2001 From: jpuri Date: Thu, 12 May 2022 17:06:18 +0530 Subject: [PATCH 2/5] fixes --- index.js | 14 ++++++++------ test/test-eth-ledger-bridge-keyring.js | 19 ++----------------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/index.js b/index.js index 0185432b..fc87dc00 100644 --- a/index.js +++ b/index.js @@ -119,7 +119,7 @@ class LedgerBridgeKeyring extends EventEmitter { this.hdPath = hdPath } - unlock (hdPath) { + unlock (hdPath, updateHdk = true) { if (this.isUnlocked() && !hdPath) { return Promise.resolve('already unlocked') } @@ -133,8 +133,10 @@ class LedgerBridgeKeyring extends EventEmitter { }, ({ success, payload }) => { if (success) { - this.hdk.publicKey = Buffer.from(payload.publicKey, 'hex') - this.hdk.chainCode = Buffer.from(payload.chainCode, 'hex') + if (updateHdk) { + this.hdk.publicKey = Buffer.from(payload.publicKey, 'hex') + this.hdk.chainCode = Buffer.from(payload.chainCode, 'hex') + } resolve(payload.address) } else { reject(payload.error || new Error('Unknown error')) @@ -146,7 +148,7 @@ class LedgerBridgeKeyring extends EventEmitter { addAccounts (n = 1) { return new Promise((resolve, reject) => { - this.unlock(this.hdPath) + this.unlock() .then(async (_) => { const from = this.unlockedAccount const to = from + n @@ -373,7 +375,7 @@ class LedgerBridgeKeyring extends EventEmitter { throw new Error(`Ledger: Account for address '${checksummedAddress}' not found`) } const { hdPath } = this.accountDetails[checksummedAddress] - const unlockedAddress = await this.unlock(hdPath) + const unlockedAddress = await this.unlock(hdPath, false) // unlock resolves to the address for the given hdPath as reported by the ledger device // if that address is not the requested address, then this account belongs to a different device or seed @@ -514,7 +516,7 @@ class LedgerBridgeKeyring extends EventEmitter { const from = (this.page - 1) * this.perPage const to = from + this.perPage - await this.unlock(this.hdPath) + await this.unlock() let accounts if (this._isLedgerLiveHdPath()) { accounts = await this._getAccountsBIP44(from, to) diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js index 35831815..0fa04f08 100644 --- a/test/test-eth-ledger-bridge-keyring.js +++ b/test/test-eth-ledger-bridge-keyring.js @@ -79,8 +79,8 @@ describe('LedgerBridgeKeyring', function () { async function basicSetupToUnlockOneAccount (accountIndex = 0) { keyring.setAccountToUnlock(accountIndex) - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[accountIndex])) await keyring.addAccounts() + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[accountIndex])) } beforeEach(function () { @@ -242,7 +242,6 @@ describe('LedgerBridgeKeyring', function () { describe('addAccounts', function () { describe('with no arguments', function () { it('returns a single account', function (done) { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts() .then((accounts) => { @@ -254,7 +253,6 @@ describe('LedgerBridgeKeyring', function () { describe('with a numeric argument', function () { it('returns that number of accounts', function (done) { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts(5) .then((accounts) => { @@ -264,7 +262,6 @@ describe('LedgerBridgeKeyring', function () { }) it('returns the expected accounts', function (done) { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts(3) .then((accounts) => { @@ -277,9 +274,9 @@ describe('LedgerBridgeKeyring', function () { }) it('stores account details for bip44 accounts', function () { + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setHdPath(`m/44'/60'/0'/0/0`) keyring.setAccountToUnlock(1) - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[1])) return keyring.addAccounts(1) .then((accounts) => { assert.deepEqual(keyring.accountDetails[accounts[0]], { @@ -290,7 +287,6 @@ describe('LedgerBridgeKeyring', function () { }) it('stores account details for non-bip44 accounts', function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setHdPath(`m/44'/60'/0'`) keyring.setAccountToUnlock(2) return keyring.addAccounts(1) @@ -304,7 +300,6 @@ describe('LedgerBridgeKeyring', function () { describe('when called multiple times', function () { it('should not remove existing accounts', function (done) { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts(1) .then(function () { @@ -324,7 +319,6 @@ describe('LedgerBridgeKeyring', function () { describe('removeAccount', function () { describe('if the account exists', function () { it('should remove that account', function (done) { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setAccountToUnlock(0) keyring.addAccounts() .then(async (accounts) => { @@ -349,14 +343,12 @@ describe('LedgerBridgeKeyring', function () { describe('getFirstPage', function () { it('should set the currentPage to 1', async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) await keyring.getFirstPage() assert.equal(keyring.page, 1) }) it('should return the list of accounts for current page', async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) const accounts = await keyring.getFirstPage() expect(accounts.length, keyring.perPage) @@ -371,7 +363,6 @@ describe('LedgerBridgeKeyring', function () { describe('getNextPage', function () { it('should return the list of accounts for current page', async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) const accounts = await keyring.getNextPage() expect(accounts.length, keyring.perPage) expect(accounts[0].address, fakeAccounts[0]) @@ -385,7 +376,6 @@ describe('LedgerBridgeKeyring', function () { describe('getPreviousPage', function () { it('should return the list of accounts for current page', async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) // manually advance 1 page await keyring.getNextPage() const accounts = await keyring.getPreviousPage() @@ -399,7 +389,6 @@ describe('LedgerBridgeKeyring', function () { }) it('should be able to go back to the previous page', async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) // manually advance 1 page await keyring.getNextPage() const accounts = await keyring.getPreviousPage() @@ -417,7 +406,6 @@ describe('LedgerBridgeKeyring', function () { const accountIndex = 5 let accounts = [] beforeEach(async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[accountIndex])) keyring.setAccountToUnlock(accountIndex) await keyring.addAccounts() accounts = await keyring.getAccounts() @@ -444,7 +432,6 @@ describe('LedgerBridgeKeyring', function () { describe('forgetDevice', function () { it('should clear the content of the keyring', async function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) // Add an account keyring.setAccountToUnlock(0) await keyring.addAccounts() @@ -577,8 +564,6 @@ describe('LedgerBridgeKeyring', function () { it('should reject if the account is not found on device', async function () { const requestedAccount = fakeAccounts[0] - const incorrectAccount = fakeAccounts[1] - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(incorrectAccount)) keyring.setAccountToUnlock(0) await keyring.addAccounts() From b6070eb6abad4397a59d84caf160ac0a559ce775 Mon Sep 17 00:00:00 2001 From: jpuri Date: Thu, 12 May 2022 17:08:47 +0530 Subject: [PATCH 3/5] fixes --- test/test-eth-ledger-bridge-keyring.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js index 7fb0ab9f..b99297ba 100644 --- a/test/test-eth-ledger-bridge-keyring.js +++ b/test/test-eth-ledger-bridge-keyring.js @@ -274,9 +274,9 @@ describe('LedgerBridgeKeyring', function () { }) it('stores account details for bip44 accounts', function () { - sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) keyring.setHdPath(`m/44'/60'/0'/0/0`) keyring.setAccountToUnlock(1) + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(fakeAccounts[0])) return keyring.addAccounts(1) .then((accounts) => { assert.deepEqual(keyring.accountDetails[accounts[0]], { @@ -564,8 +564,10 @@ describe('LedgerBridgeKeyring', function () { it('should reject if the account is not found on device', async function () { const requestedAccount = fakeAccounts[0] + const incorrectAccount = fakeAccounts[1] keyring.setAccountToUnlock(0) await keyring.addAccounts() + sandbox.on(keyring, 'unlock', (_) => Promise.resolve(incorrectAccount)) assert.rejects(() => keyring.unlockAccountByAddress(requestedAccount), new Error(`Ledger: Account ${fakeAccounts[0]} does not belong to the connected device`)) }) From 5f9fa50a77657e3f652ef3329f8623a74139111a Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 18 May 2022 23:19:26 +0530 Subject: [PATCH 4/5] Adding unit test coverage --- test/test-eth-ledger-bridge-keyring.js | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js index b99297ba..1202f382 100644 --- a/test/test-eth-ledger-bridge-keyring.js +++ b/test/test-eth-ledger-bridge-keyring.js @@ -214,6 +214,47 @@ describe('LedgerBridgeKeyring', function () { done() }) }) + it('should update hdk.publicKey if updateHdk is true', function (done) { + const ledgerKeyring = new LedgerBridgeKeyring() + const newFakeHDkey = HDKey.fromExtendedKey(fakeXPubKey) + ledgerKeyring.hdk = newFakeHDkey + + sandbox.on(ledgerKeyring, '_sendMessage', (_, cb) => { + cb({ + success: true, + payload: { + publicKey: + '04197ced33b63059074b90ddecb9400c45cbc86210a20317b539b8cae84e573342149c3384ae45f27db68e75823323e97e03504b73ecbc47f5922b9b8144345e5a', + chainCode: + 'ba0fb16e01c463d1635ec36f5adeb93a838adcd1526656c55f828f1e34002a8b', + address: fakeAccounts[1], + }, + }) + }) + + ledgerKeyring.unlock(`m/44'/60'/0'/1`).then((_) => { + assert.notDeepEqual(ledgerKeyring.hdk.publicKey, newFakeHDkey.publicKey) + done() + }) + }) + it('should not update hdk.publicKey if updateHdk is false', function (done) { + sandbox.on(keyring, '_sendMessage', (_, cb) => { + cb({ + success: true, + payload: { + publicKey: + '04197ced33b63059074b90ddecb9400c45cbc86210a20317b539b8cae84e573342149c3384ae45f27db68e75823323e97e03504b73ecbc47f5922b9b8144345e5a', + chainCode: + 'ba0fb16e01c463d1635ec36f5adeb93a838adcd1526656c55f828f1e34002a8b', + address: fakeAccounts[0], + }, + }) + }) + keyring.unlock(`m/44'/60'/0'/1`, false).then((_) => { + assert.deepEqual(keyring.hdk.publicKey, fakeHdKey.publicKey) + done() + }) + }) }) describe('setHdPath', function () { From 96038ccb6c2971af9685fbdd988804815cf0f183 Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 18 May 2022 23:26:25 +0530 Subject: [PATCH 5/5] Adding unit test coverage --- test/test-eth-ledger-bridge-keyring.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/test-eth-ledger-bridge-keyring.js b/test/test-eth-ledger-bridge-keyring.js index 1202f382..6b05d4ba 100644 --- a/test/test-eth-ledger-bridge-keyring.js +++ b/test/test-eth-ledger-bridge-keyring.js @@ -216,8 +216,7 @@ describe('LedgerBridgeKeyring', function () { }) it('should update hdk.publicKey if updateHdk is true', function (done) { const ledgerKeyring = new LedgerBridgeKeyring() - const newFakeHDkey = HDKey.fromExtendedKey(fakeXPubKey) - ledgerKeyring.hdk = newFakeHDkey + ledgerKeyring.hdk = { publicKey: 'ABC' } sandbox.on(ledgerKeyring, '_sendMessage', (_, cb) => { cb({ @@ -233,12 +232,15 @@ describe('LedgerBridgeKeyring', function () { }) ledgerKeyring.unlock(`m/44'/60'/0'/1`).then((_) => { - assert.notDeepEqual(ledgerKeyring.hdk.publicKey, newFakeHDkey.publicKey) + assert.notDeepEqual(ledgerKeyring.hdk.publicKey, 'ABC') done() }) }) it('should not update hdk.publicKey if updateHdk is false', function (done) { - sandbox.on(keyring, '_sendMessage', (_, cb) => { + const ledgerKeyring = new LedgerBridgeKeyring() + ledgerKeyring.hdk = { publicKey: 'ABC' } + + sandbox.on(ledgerKeyring, '_sendMessage', (_, cb) => { cb({ success: true, payload: { @@ -246,12 +248,13 @@ describe('LedgerBridgeKeyring', function () { '04197ced33b63059074b90ddecb9400c45cbc86210a20317b539b8cae84e573342149c3384ae45f27db68e75823323e97e03504b73ecbc47f5922b9b8144345e5a', chainCode: 'ba0fb16e01c463d1635ec36f5adeb93a838adcd1526656c55f828f1e34002a8b', - address: fakeAccounts[0], + address: fakeAccounts[1], }, }) }) - keyring.unlock(`m/44'/60'/0'/1`, false).then((_) => { - assert.deepEqual(keyring.hdk.publicKey, fakeHdKey.publicKey) + + ledgerKeyring.unlock(`m/44'/60'/0'/1`, false).then((_) => { + assert.deepEqual(ledgerKeyring.hdk.publicKey, 'ABC') done() }) })