From 6ef7f15e8594e7f25975601bf3b5672ec823ccc3 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Mon, 14 Nov 2022 15:20:30 +0000 Subject: [PATCH 1/7] Adds async init method to keyrings --- index.js | 28 ++++++++++++++++++++++------ test/index.js | 19 +++++++++++++++++++ test/lib/mock-keyring.js | 19 +++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 test/lib/mock-keyring.js diff --git a/index.js b/index.js index 17f641d6..6b6116bc 100644 --- a/index.js +++ b/index.js @@ -237,11 +237,11 @@ class KeyringController extends EventEmitter { * @returns {Promise} The new keyring. */ async addNewKeyring(type, opts) { - const Keyring = this.getKeyringClassForType(type); - const keyring = new Keyring(opts); + const keyring = await this._newKeyring(type, opts); + if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { keyring.generateRandomMnemonic(); - keyring.addAccounts(); + await keyring.addAccounts(); } const accounts = await keyring.getAccounts(); @@ -700,12 +700,12 @@ class KeyringController extends EventEmitter { async _restoreKeyring(serialized) { const { type, data } = serialized; - const Keyring = this.getKeyringClassForType(type); - if (!Keyring) { + const keyring = await this._newKeyring(type); + if (!keyring) { this._unsupportedKeyrings.push(serialized); return undefined; } - const keyring = new Keyring(); + await keyring.deserialize(data); // getAccounts also validates the accounts for some keyrings await keyring.getAccounts(); @@ -873,6 +873,22 @@ class KeyringController extends EventEmitter { ); } } + + async _newKeyring(type, data) { + const Keyring = this.getKeyringClassForType(type); + + if (!Keyring) { + return undefined; + } + + const keyring = new Keyring(data); + + if (keyring.init) { + await keyring.init(); + } + + return keyring; + } } module.exports = KeyringController; diff --git a/test/index.js b/test/index.js index 0656724d..4055d079 100644 --- a/test/index.js +++ b/test/index.js @@ -6,6 +6,7 @@ const sinon = require('sinon'); const Wallet = require('ethereumjs-wallet').default; const KeyringController = require('..'); +const { KeyringMockWithInit } = require('./lib/mock-keyring'); const mockEncryptor = require('./lib/mock-encryptor'); const password = 'password123'; @@ -33,6 +34,7 @@ describe('KeyringController', function () { beforeEach(async function () { keyringController = new KeyringController({ encryptor: mockEncryptor, + keyringTypes: [KeyringMockWithInit], }); await keyringController.createNewVaultAndKeychain(password); @@ -240,6 +242,23 @@ describe('KeyringController', function () { const allAccounts = await keyringController.getAccounts(); expect(allAccounts).toHaveLength(3); }); + + it('should call init method if available', async function () { + const Keyring = keyringController.getKeyringClassForType( + 'Keyring Mock With Init', + ); + + const initSpy = sinon.spy(Keyring.prototype, 'init'); + + const keyring = await keyringController.addNewKeyring( + 'Keyring Mock With Init', + ); + + const keyringAccounts = await keyring.getAccounts(); + expect(keyringAccounts).toHaveLength(0); + + expect(initSpy.calledOnce).toBe(true); + }); }); describe('restoreKeyring', function () { diff --git a/test/lib/mock-keyring.js b/test/lib/mock-keyring.js new file mode 100644 index 00000000..919eb07e --- /dev/null +++ b/test/lib/mock-keyring.js @@ -0,0 +1,19 @@ +class KeyringMockWithInit { + init() { + return Promise.resolve(); + } + + getAccounts() { + return []; + } + + serialize() { + return Promise.resolve({}); + } +} + +KeyringMockWithInit.type = 'Keyring Mock With Init'; + +module.exports = { + KeyringMockWithInit, +}; From f1c971858a812dd8a782645fc6620e9681e2acd0 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 2 Dec 2022 16:33:33 +0000 Subject: [PATCH 2/7] instantiate keyrings with builder functions --- .eslintrc.js | 3 +++ index.js | 28 ++++++++++++++++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index f371d0a7..534d285c 100644 --- 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'], env: { commonjs: true, diff --git a/index.js b/index.js index 6b6116bc..ef99ad55 100644 --- a/index.js +++ b/index.js @@ -8,7 +8,10 @@ const { normalize: normalizeAddress } = require('eth-sig-util'); const SimpleKeyring = require('@metamask/eth-simple-keyring'); const HdKeyring = require('@metamask/eth-hd-keyring'); -const keyringTypes = [SimpleKeyring, HdKeyring]; +const keyringTypes = [ + keyringBuilderFactory(SimpleKeyring), + keyringBuilderFactory(HdKeyring), +]; const KEYRINGS_TYPE_MAP = { HD_KEYRING: 'HD Key Tree', @@ -700,13 +703,12 @@ class KeyringController extends EventEmitter { async _restoreKeyring(serialized) { const { type, data } = serialized; - const keyring = await this._newKeyring(type); + const keyring = await this._newKeyring(type, data); if (!keyring) { this._unsupportedKeyrings.push(serialized); return undefined; } - await keyring.deserialize(data); // getAccounts also validates the accounts for some keyrings await keyring.getAccounts(); this.keyrings.push(keyring); @@ -875,13 +877,15 @@ class KeyringController extends EventEmitter { } async _newKeyring(type, data) { - const Keyring = this.getKeyringClassForType(type); + const keyringBuilder = this.getKeyringClassForType(type); - if (!Keyring) { + if (!keyringBuilder) { return undefined; } - const keyring = new Keyring(data); + const keyring = keyringBuilder(); + + await keyring.deserialize(data); if (keyring.init) { await keyring.init(); @@ -891,4 +895,16 @@ class KeyringController extends EventEmitter { } } +function keyringBuilderFactory(KeyringClass, BridgeClass) { + const builder = () => { + const constructorDependencies = BridgeClass ? new BridgeClass() : undefined; + return new KeyringClass(constructorDependencies); + }; + + builder.type = KeyringClass.type; + + return builder; +} + module.exports = KeyringController; +module.exports.keyringBuilderFactory = keyringBuilderFactory; From 6593deb49d98a7c5ff7bc137259bbec77d4340a2 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Mon, 5 Dec 2022 10:27:38 +0000 Subject: [PATCH 3/7] rename properties and method add test --- index.js | 26 +++++++++++++++----------- test/index.js | 14 +++++--------- test/lib/mock-keyring.js | 4 ++++ 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/index.js b/index.js index ef99ad55..712b555c 100644 --- a/index.js +++ b/index.js @@ -8,7 +8,7 @@ const { normalize: normalizeAddress } = require('eth-sig-util'); const SimpleKeyring = require('@metamask/eth-simple-keyring'); const HdKeyring = require('@metamask/eth-hd-keyring'); -const keyringTypes = [ +const defaultKeyringBuilders = [ keyringBuilderFactory(SimpleKeyring), keyringBuilderFactory(HdKeyring), ]; @@ -38,13 +38,15 @@ class KeyringController extends EventEmitter { constructor(opts) { super(); const initState = opts.initState || {}; - this.keyringTypes = opts.keyringTypes - ? keyringTypes.concat(opts.keyringTypes) - : keyringTypes; + this.keyringBuilders = opts.keyringBuilders + ? defaultKeyringBuilders.concat(opts.keyringBuilders) + : defaultKeyringBuilders; this.store = new ObservableStore(initState); this.memStore = new ObservableStore({ isUnlocked: false, - keyringTypes: this.keyringTypes.map((keyringType) => keyringType.type), + keyringTypes: this.keyringBuilders.map( + (keyringBuilder) => keyringBuilder.type, + ), keyrings: [], encryptionKey: null, }); @@ -233,7 +235,7 @@ class KeyringController extends EventEmitter { * and the current decrypted Keyrings array. * * All Keyring classes implement a unique `type` string, - * and this is used to retrieve them from the keyringTypes array. + * and this is used to retrieve them from the keyringBuilders array. * * @param {string} type - The type of keyring to add. * @param {Object} opts - The constructor options for the keyring. @@ -718,16 +720,18 @@ class KeyringController extends EventEmitter { /** * Get Keyring Class For Type * - * Searches the current `keyringTypes` array - * for a Keyring class whose unique `type` property + * Searches the current `keyringBuilders` array + * for a Keyring builder whose unique `type` property * matches the provided `type`, * returning it if it exists. * * @param {string} type - The type whose class to get. * @returns {Keyring|undefined} The class, if it exists. */ - getKeyringClassForType(type) { - return this.keyringTypes.find((keyring) => keyring.type === type); + getKeyringBuilderForType(type) { + return this.keyringBuilders.find( + (keyringBuilder) => keyringBuilder.type === type, + ); } /** @@ -877,7 +881,7 @@ class KeyringController extends EventEmitter { } async _newKeyring(type, data) { - const keyringBuilder = this.getKeyringClassForType(type); + const keyringBuilder = this.getKeyringBuilderForType(type); if (!keyringBuilder) { return undefined; diff --git a/test/index.js b/test/index.js index 4055d079..fb5adc1c 100644 --- a/test/index.js +++ b/test/index.js @@ -6,6 +6,7 @@ const sinon = require('sinon'); const Wallet = require('ethereumjs-wallet').default; const KeyringController = require('..'); +const { keyringBuilderFactory } = require('..'); const { KeyringMockWithInit } = require('./lib/mock-keyring'); const mockEncryptor = require('./lib/mock-encryptor'); @@ -34,7 +35,7 @@ describe('KeyringController', function () { beforeEach(async function () { keyringController = new KeyringController({ encryptor: mockEncryptor, - keyringTypes: [KeyringMockWithInit], + keyringBuilders: [keyringBuilderFactory(KeyringMockWithInit)], }); await keyringController.createNewVaultAndKeychain(password); @@ -244,20 +245,15 @@ describe('KeyringController', function () { }); it('should call init method if available', async function () { - const Keyring = keyringController.getKeyringClassForType( - 'Keyring Mock With Init', - ); - - const initSpy = sinon.spy(Keyring.prototype, 'init'); + const initSpy = sinon.spy(KeyringMockWithInit.prototype, 'init'); const keyring = await keyringController.addNewKeyring( 'Keyring Mock With Init', ); - const keyringAccounts = await keyring.getAccounts(); - expect(keyringAccounts).toHaveLength(0); + expect(keyring).toBeInstanceOf(KeyringMockWithInit); - expect(initSpy.calledOnce).toBe(true); + sinon.assert.calledOnce(initSpy); }); }); diff --git a/test/lib/mock-keyring.js b/test/lib/mock-keyring.js index 919eb07e..cd57b9ae 100644 --- a/test/lib/mock-keyring.js +++ b/test/lib/mock-keyring.js @@ -10,6 +10,10 @@ class KeyringMockWithInit { serialize() { return Promise.resolve({}); } + + deserialize(_) { + return Promise.resolve(); + } } KeyringMockWithInit.type = 'Keyring Mock With Init'; From 78a33760f4bbc11dc359967d58562c069209fb49 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Mon, 5 Dec 2022 11:24:44 +0000 Subject: [PATCH 4/7] remove eslintrc changes --- .eslintrc.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 534d285c..f371d0a7 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -1,8 +1,5 @@ module.exports = { root: true, - parserOptions: { - ecmaVersion: 2018, // to support object rest spread, e.g. {...x, ...y} - }, extends: ['@metamask/eslint-config'], env: { commonjs: true, From 500e1a33dce3f035c9fa1c1525b7d6f6a1cd8180 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Wed, 7 Dec 2022 11:20:23 +0000 Subject: [PATCH 5/7] make KeyringController a named export --- index.js | 6 ++++-- test/index.js | 3 +-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 712b555c..6b6ad529 100644 --- a/index.js +++ b/index.js @@ -910,5 +910,7 @@ function keyringBuilderFactory(KeyringClass, BridgeClass) { return builder; } -module.exports = KeyringController; -module.exports.keyringBuilderFactory = keyringBuilderFactory; +module.exports = { + KeyringController, + keyringBuilderFactory, +}; diff --git a/test/index.js b/test/index.js index fb5adc1c..e4b4b749 100644 --- a/test/index.js +++ b/test/index.js @@ -5,8 +5,7 @@ const normalizeAddress = sigUtil.normalize; const sinon = require('sinon'); const Wallet = require('ethereumjs-wallet').default; -const KeyringController = require('..'); -const { keyringBuilderFactory } = require('..'); +const { KeyringController, keyringBuilderFactory } = require('..'); const { KeyringMockWithInit } = require('./lib/mock-keyring'); const mockEncryptor = require('./lib/mock-encryptor'); From a486f5b0eb5659c81faca5b2499081368f319868 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 9 Dec 2022 11:28:10 +0000 Subject: [PATCH 6/7] changes from review --- index.js | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index 6b6ad529..da793009 100644 --- a/index.js +++ b/index.js @@ -880,6 +880,15 @@ class KeyringController extends EventEmitter { } } + /** + * Instantiate, initialize and return a new keyring + * + * The keyring instantiated is of the given `type`. + * + * @param {string} type - The type of keyring to add. + * @param {Object} data - The data to restore a previously serialized keyring. + * @returns {Promise} The new keyring. + */ async _newKeyring(type, data) { const keyringBuilder = this.getKeyringBuilderForType(type); @@ -899,13 +908,18 @@ class KeyringController extends EventEmitter { } } -function keyringBuilderFactory(KeyringClass, BridgeClass) { - const builder = () => { - const constructorDependencies = BridgeClass ? new BridgeClass() : undefined; - return new KeyringClass(constructorDependencies); - }; +/** + * Get builder function for `Keyring` + * + * Returns a builder function for `Keyring` with a `type` property. + * + * @param {typeof Keyring} Keyring - The Keyring class for the builder. + * @returns {function: Keyring} A builder function for the given Keyring. + */ +function keyringBuilderFactory(Keyring) { + const builder = () => new Keyring(); - builder.type = KeyringClass.type; + builder.type = Keyring.type; return builder; } From 53d8263163b8f16584f56ca48971cd198f5ba930 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 9 Dec 2022 11:49:53 +0000 Subject: [PATCH 7/7] remove await from addAccounts --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index da793009..2a8f0039 100644 --- a/index.js +++ b/index.js @@ -246,7 +246,7 @@ class KeyringController extends EventEmitter { if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { keyring.generateRandomMnemonic(); - await keyring.addAccounts(); + keyring.addAccounts(); } const accounts = await keyring.getAccounts();