Refactor to handle new keyrings with bridge dependencies #163

bergarces · 2022-11-15T16:34:05Z

The PR was initially going to add code to handle an asynchronous init method for keyrings. But now it has more changes, many of them breaking changes.

Related PR for eth-trezor-keyring.
MetaMask/eth-trezor-keyring#143

Summary

New `init` method

Adds support for asynchronous `init` method present in some keyrings. It'll only be called if it exists to preserve compatibility.

Keyring Builder Functions (BREAKING CHANGE)

Adds support to inject bridge logic into keyrings. This is a breaking change as it means that we no longer have an array of classes that can be instantiated, but an array of keyring builder functions that return an instance.

This is done so that MetaMaskController can provide the KeyringController a builder function for a keyring that can contain MV2, MV3 or Node logic to communicate with a hardware device. It will also allow us to, eventually, change the bridge logic dynamically.

This means no longer passing an array of classes, but an array of builder functions. A keyringBuilderFactory function is exported in order to assist with this task.

It also means the internal property called keyringTypes is now replaced with keyringBuilders.

Moved keyring instantiation logic to a single private method

`_restoreKeyring` and `addNewKeyring` were both creating new instances. The logic has been moved to a single place so that we can make sure that, after instantiation, we call `deserialize` and `init`.

Replace `getKeyringClassForType` with `getKeyringBuilderForType` (BREAKING CHANGE)

This method is used mostly internally, but also by `seed-phrase-verifier` in the extension.

.eslintrc.js

bergarces · 2022-12-02T16:36:03Z

index.js

    if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) {
      keyring.generateRandomMnemonic();
-      keyring.addAccounts();
+      await keyring.addAccounts();


This method is asynchronous. I've taken the liberty to add an await.

Nit: could we move this to a separate PR? We could merge this in isolation, since it's not directly tied to the other changes. That would make it easier to understand the commit history and prepare the changelog and so on.

It would also be great to have a test for this, to demonstrate that when addAccounts is doing asynchronous work, this function guarantees that the accounts have been added by the time it resolves.

Yes, better on a different PR. I've removed it from here and will open another one.

bergarces · 2022-12-02T16:40:32Z

index.js

+    return keyring;
+  }
+}
+


There are many instances in which we try to access the type from the static class. "Luckily", javascript allows a function to also have properties.

By exporting this helper method we can make sure that whenever we create a keyring builder function, the type is added as well.

Also, since we are always calling deserialize after instantiating the keyring, I'm just passing the bridge to the constructor.

If we're changing the "Keyring" API in a non-backwards-compatible way, I'd prefer that we start using an object for constructor options rather than parameters (i.e. the "options bag" pattern, passing in { bridge } rather than bridge). This pattern is easier to read, and makes it easy to add additional options without breaking changes.

Fair enough. I've made the change to the options bag pattern directly in eth-trezor-keyring PR. There's no longer any reference here as the new simple builder factory does not concern itself with bridges anymore.

The only argument I have in favour of passing dependencies without wrapping them in an object is that, if this ever moves to TypeScript, we could benefit from the syntactic sugar to declare them as class properties in the constructor, but that's a bit far-fetched.

constructor( private readonly bridge: KeyringBridge, )

bergarces · 2022-12-02T16:49:39Z

index.js

@@ -873,6 +875,36 @@ class KeyringController extends EventEmitter {
      );
    }
  }
+


There are two methods in which a new keyring is instantiated: _restoreKeyring and addNewKeyring.

I've extracted the logic into a _newKeyring method so that we always instantiate, initialise (if available) and deserialize.

On the topic of deserialization, after looking at every single keyring I'm convinced that we don't have to pass the data at construction time as long as we call deserialize afterwards. deserialize is an asynchronous function, and we probably should stop calling it within the constructor (I have done so in the Trezor keyring refactor).

bergarces · 2022-12-05T10:30:47Z

index.js

-      : keyringTypes;
+    this.keyringBuilders = opts.keyringBuilders
+      ? defaultKeyringBuilders.concat(opts.keyringBuilders)
+      : defaultKeyringBuilders;


I thought it would be wise to rename this property as it will no longer hold class types, but builder functions.

I have checked within the extension code and it does not appear to be used, but worth thinking about any possible side effects as this can be a breaking change if external code tries to access this property.

keyringTypes in the store is left unchanged and it still stores the types as strings.

index.js

Gudahtt · 2022-12-07T16:18:26Z

index.js

-module.exports = KeyringController;
+function keyringBuilderFactory(KeyringClass, BridgeClass) {
+  const builder = () => {
+    const constructorDependencies = BridgeClass ? new BridgeClass() : undefined;


I'm torn on whether this is a good idea. I see the advantage of including a "builder" function here directly, but maybe it would be better to encourage using custom builder functions instead.

Most bridges will require initialization, for example, but this offers no way to do that directly (presumably we'd need to invoke it in the keyring's init function instead). There is no simple way to pass in constructor parameters either.

Maybe we could instead offer a simpler "builder" function just for the simple keyrings, and recommend a custom builder function for keyrings using bridges?

I was at odds with doing this as well, probably best to leave a simple one as you suggest though.

What tilted me towards creating this method was the odd situation in which we have to make sure that the builder function returned must have a type property, which is not something particularly easy to document.

I've replaced it with the simpler version, so the one with the bridge will probably have to be added to metamask-controller in order to instantiate some of the keyrings (the QR one probably won't need a bridge and can use this function too).

add test

Gudahtt

LGTM!

bergarces · 2022-12-12T15:11:30Z

Thanks for reviewing and merging @Gudahtt . I was going to ask if there was something else needed before merging.

I have a local branch of the extension in which I've applied and tested the KeyringController changes (only the controller, without using the new Trezor implementation). So if a PR is eventually needed when a new version is released I'm happy to assist (changes are only needed to metamask-controller and seed-phrase-verifier).

Gudahtt · 2022-12-12T15:15:50Z

Thanks @bergarces, that would be great! If we can get this into the extension and update the keyrings to use this builder API one-at-a-time that would be ideal.

bergarces requested a review from a team as a code owner November 15, 2022 16:34

bergarces force-pushed the add-init-method branch from 449a7ad to 1dfa4de Compare December 2, 2022 16:33

bergarces commented Dec 2, 2022

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

bergarces commented Dec 2, 2022

View reviewed changes

bergarces force-pushed the add-init-method branch from 1dfa4de to 9e176db Compare December 2, 2022 16:36

bergarces commented Dec 2, 2022

View reviewed changes

bergarces force-pushed the add-init-method branch from 9e176db to 035d1e0 Compare December 2, 2022 16:48

bergarces commented Dec 2, 2022

View reviewed changes

bergarces force-pushed the add-init-method branch 3 times, most recently from e32258f to 4079eb4 Compare December 2, 2022 20:29

bergarces commented Dec 5, 2022

View reviewed changes

bergarces changed the title ~~Adds async init method to keyrings~~ Refactor to handle new keyrings with bridge dependencies Dec 5, 2022

bergarces commented Dec 5, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

Gudahtt reviewed Dec 7, 2022

View reviewed changes

index.js Show resolved Hide resolved

Gudahtt reviewed Dec 7, 2022

View reviewed changes

bergarces added 6 commits December 9, 2022 11:36

Adds async init method to keyrings

6ef7f15

instantiate keyrings with builder functions

f1c9718

rename properties and method

6593deb

add test

remove eslintrc changes

78a3376

make KeyringController a named export

500e1a3

changes from review

a486f5b

bergarces force-pushed the add-init-method branch from 56f4b2a to a486f5b Compare December 9, 2022 11:37

remove await from addAccounts

53d8263

Gudahtt approved these changes Dec 9, 2022

View reviewed changes

Merge branch 'main' into add-init-method

eeeee36

Gudahtt merged commit e130876 into MetaMask:main Dec 12, 2022

bergarces mentioned this pull request Dec 13, 2022

await for addAccounts for HD Keyring #176

Merged

adonesky1 mentioned this pull request Jan 9, 2023

Integrate KeyringController v10 MetaMask/metamask-extension#17056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to handle new keyrings with bridge dependencies #163

Refactor to handle new keyrings with bridge dependencies #163

bergarces commented Nov 15, 2022 •

edited by Gudahtt

Loading

bergarces Dec 2, 2022

Gudahtt Dec 7, 2022

Gudahtt Dec 7, 2022

bergarces Dec 9, 2022

bergarces Dec 2, 2022

bergarces Dec 2, 2022 •

edited

Loading

Gudahtt Dec 7, 2022

bergarces Dec 9, 2022 •

edited

Loading

bergarces Dec 2, 2022 •

edited

Loading

bergarces Dec 5, 2022 •

edited

Loading

Gudahtt Dec 7, 2022 •

edited

Loading

bergarces Dec 9, 2022

Gudahtt left a comment

bergarces commented Dec 12, 2022

Gudahtt commented Dec 12, 2022

@@ @@ -873,6 +875,36 @@ class KeyringController extends EventEmitter { @@
                     );
                   }
                 }

Refactor to handle new keyrings with bridge dependencies #163

Refactor to handle new keyrings with bridge dependencies #163

Conversation

bergarces commented Nov 15, 2022 • edited by Gudahtt Loading

Summary

New `init` method

Keyring Builder Functions (BREAKING CHANGE)

Moved keyring instantiation logic to a single private method

Replace `getKeyringClassForType` with `getKeyringBuilderForType` (BREAKING CHANGE)

bergarces Dec 2, 2022

Choose a reason for hiding this comment

Gudahtt Dec 7, 2022

Choose a reason for hiding this comment

Gudahtt Dec 7, 2022

Choose a reason for hiding this comment

bergarces Dec 9, 2022

Choose a reason for hiding this comment

bergarces Dec 2, 2022

Choose a reason for hiding this comment

bergarces Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Gudahtt Dec 7, 2022

Choose a reason for hiding this comment

bergarces Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

bergarces Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

bergarces Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Gudahtt Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

bergarces Dec 9, 2022

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

bergarces commented Dec 12, 2022

Gudahtt commented Dec 12, 2022

bergarces commented Nov 15, 2022 •

edited by Gudahtt

Loading

bergarces Dec 2, 2022 •

edited

Loading

bergarces Dec 9, 2022 •

edited

Loading

bergarces Dec 2, 2022 •

edited

Loading

bergarces Dec 5, 2022 •

edited

Loading

Gudahtt Dec 7, 2022 •

edited

Loading