Merge `EthKeyringController` into `KeyringController` #3830

mikesposito · 2024-01-24T12:20:37Z

Explanation

This PR moves the logic implemented in EthKeyringController to KeyringController, leaving KeyringController's API unchanged

References

Fixes Merge EthKeyringController into KeyringController #3766

Changelog

`@metamask/keyring-controller`

ADDED: Added keyringBuilderFactory util
ADDED: Added GenericEncryptor, ExportableKeyEncryptor and SerializedKeyring types
CHANGED: Removed @metamask/eth-keyring-controller dep
CHANGED: Added dependencies from @metamask/eth-keyring-controller
- @metamask/eth-hd-keyring
- @metamask/eth-simple-keyring
- @metamask/eth-sig-util
- @metamask/browser-passworder

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

mikesposito · 2024-01-24T16:55:12Z

packages/keyring-controller/src/KeyringController.ts

+  /**
+   * Update the controller state with its current keyrings.
+   */
+  async #updateKeyringsInState(): Promise<void> {
+    const keyrings = await Promise.all(this.#keyrings.map(displayForKeyring));
+    this.update((state) => {
+      state.keyrings = keyrings;
+    });
  }


This is the equivalent of EthKeyringController.updateMemStoreKeyrings: https://github.com/MetaMask/KeyringController/blob/0134de248cc16126b9dcb880a4d96776f8dbc043/src/KeyringController.ts#L358

Perhaps #updateStateKeyrings would reflect the old name better, but I'm not sure what would be the best naming for this

Suggested change

/**

* Update the controller state with its current keyrings.

*/

async #updateKeyringsInState(): Promise<void> {

const keyrings = await Promise.all(this.#keyrings.map(displayForKeyring));

this.update((state) => {

state.keyrings = keyrings;

});

}

/**

* Update the controller state with its current keyrings.

*/

async #updateStateKeyrings(): Promise<void> {

const keyrings = await Promise.all(this.#keyrings.map(displayForKeyring));

this.update((state) => {

state.keyrings = keyrings;

});

}

I like updateKeyringsInState over updateStateKeyrings as it reads better. However it might be nice to convey that it's not taking a set of keyrings but rather it's persisting the keyrings that have already been saved. So perhaps persistKeyrings? I don't know if we already use "persist" elsewhere to mean "write to state", but it seems to fit.

Unfortunately we already have a method called persistAllKeyrings, which saves the contents of the #keyrings array to the encrypted (and persisted) storage - the vault

This function instead, syncs the Controller state with the contents of the #keyrings array, so we are not persisting keyrings anywhere, but we are updating their representation in the controller state

Ah, gotcha. Okay, updateKeyringsInState seems fine then.

mikesposito · 2024-01-25T11:42:20Z

packages/keyring-controller/tests/mocks/mockEncryptor.ts

@@ -35,13 +34,13 @@ export default class MockEncryptor implements ExportableKeyEncryptor {
      throw new Error(INVALID_PASSWORD_ERROR);
    }

-    return cacheVal ?? {};
+    return JSON.parse(cacheVal) ?? {};


This is due to an interesting discovery regarding the QRKeyring. Apparently, the serialize method of the keyring does not return an object compatible with what we internally call "serializable" (i.e. Json type), as it contains keys with undefined as value

The only reason why this does not break production is that the encryptor calls JSON.stringify, which removes any key with undefined as value.

Without this, the unlock operation would be broken after adding the QRKeyring, as the isSerializedKeyringArray assertion would fail

Due to this, I'm also realising that we make sure that what is unlocked from the vault is a correct serializable object, but we don't do that when locking and saving the keyrings to the vault (the other way around): this means that:

we are not sure that we are saving a proper object in the vault, but we expect it to be when reading from it.

we are effectively relying on the fact that the object that we retrieve from the vault is not the same that we saved in it

Perhaps we could add the same check in persistAllKeyrings, even if it is not required type-wise. This would ensure that each keyring implements serialize properly (given that this is our requirement)

mikesposito · 2024-01-25T12:08:54Z

packages/keyring-controller/src/KeyringController.ts

+   * Checks for duplicate keypairs, using the the first account in the given
+   * array. Rejects if a duplicate is found.
+   *
+   * Only supports 'Simple Key Pair'.


Uhmm, we probably want to avoid account duplication with any keyring type, but I'm wondering if we want to change this behavior in this PR

Ah perhaps not, but good eye. I wonder if this was done intentionally for simplicity or was added before the other keyring types were added and hasn't been updated since.

mikesposito · 2024-01-25T12:55:03Z

packages/keyring-controller/src/KeyringController.ts

+    if (type === KeyringTypes.hd && (!isObject(opts) || !opts.mnemonic)) {
+      if (!keyring.generateRandomMnemonic) {
+        throw new Error(
+          KeyringControllerError.UnsupportedGenerateRandomMnemonic,
+        );
+      }
+
+      keyring.generateRandomMnemonic();
+      await keyring.addAccounts(1);
+    }


Most likely unrelated to this PR: I feel like this should be the responsibility of a hypothetical init method to be added on HDKeyring

packages/keyring-controller/src/KeyringController.ts

## Explanation  This PR refactors tests related to user operations methods, to align them with the structure used for other methods. It also adds test coverage for keyrings that do not support user operations ## References  * Related to #3830 ## Changelog  No functional changes ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate

## Explanation  This PR adds additional tests for edge cases, useful for #3830. Some `istanbul ignore if` statements are also being removed ## References  * Fixes #3767 * See #3830 ## Changelog  No functional changes ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate

mikesposito · 2024-01-26T12:35:07Z

packages/keyring-controller/src/KeyringController.ts

  /**
-   * Create keyring.
+   * Create a new keyring, ensuring that the first account is
+   * also created.
   *
-   * - Creates a new vault.
-   * - Creates a new keyring with at least one account.
-   * - Makes the first account the selected account.
   * @param type - Keyring type to instantiate.
   * @param opts - Optional parameters required to instantiate the keyring.
-   * @returns A promise that resolves if the operation was successful.
+   * @returns A promise that resolves if the operation is successful.
   */
-  async #createKeyring(type: string, opts?: unknown) {
+  async #createKeyringWithFirstAccount(type: string, opts?: unknown) {
    const keyring = (await this.addNewKeyring(type, opts)) as EthKeyring<Json>;

    const [firstAccount] = await keyring.getAccounts();


This method has been renamed to better match what it is used for

I'm now realizing the new naming might also be incorrect since the first account is not explicitly created here, but only expected to be created - and this is a case only for HD keyring because we explicitly check for that in the addNewAccount method.

I think there might be a better way to do all this (see also my previous comment: #3830 (comment))

mikesposito · 2024-01-26T12:41:12Z

packages/keyring-controller/src/KeyringController.ts

-    if (!keyring) {
-      throw new Error(KeyringControllerError.NoKeyring);
-    }


This condition is not possible, even in case the keyring builder is somehow broken and returns undefined, as #newKeyring would throw an error.

mcmire

I checked every method in eth-keyring-controller and everything seems to be represented here. Nice! All of my comments are either thoughts or minor suggestions.

packages/keyring-controller/src/KeyringController.ts

mcmire · 2024-01-26T20:02:14Z

types/@metamask/eth-hd-keyring.d.ts

@@ -0,0 +1 @@
+declare module '@metamask/eth-hd-keyring';


Note that adding these files will make it possible to build @metamask/keyring-controller. But these type augmentations won't get included in the compiled version of the package, so in order to use the package, consumers may have to add these to their project as well.

However... I see that we copied these from eth-keyring-controller. And obviously we haven't had the problem I described above, because otherwise we would have had to handle it already. So maybe we don't need to do anything right now, but we should probably make a note to add type definition files to these packages (even if they're empty) so we don't have to do this.

Uhm good point. We should tackle the typescript migration for these keyring packages, and that might be part of the effort to align them all with the Keyring API - but in the meantime, adding blank type definition files to these packages sounds like a good short-term solution.

mcmire · 2024-01-26T20:20:10Z

packages/keyring-controller/src/KeyringController.ts

+      return res.concat(arr);
+    }, []);
+
+    // Cast to `Hex[]` here is safe here because `addresses` has no nullish


Just a thought for later: it would be nice if we could add an overload to normalize so it returns Hex if given something non-nullish, so we don't have to cast here.

mcmire · 2024-01-26T20:43:23Z

packages/keyring-controller/src/KeyringController.ts

+  /**
+   * Update the controller state with its current keyrings.
+   */
+  async #updateKeyringsInState(): Promise<void> {
+    const keyrings = await Promise.all(this.#keyrings.map(displayForKeyring));
+    this.update((state) => {
+      state.keyrings = keyrings;
+    });
  }


I like updateKeyringsInState over updateStateKeyrings as it reads better. However it might be nice to convey that it's not taking a set of keyrings but rather it's persisting the keyrings that have already been saved. So perhaps persistKeyrings? I don't know if we already use "persist" elsewhere to mean "write to state", but it seems to fit.

mcmire · 2024-01-26T20:47:48Z

packages/keyring-controller/src/KeyringController.ts

+   * Checks for duplicate keypairs, using the the first account in the given
+   * array. Rejects if a duplicate is found.
+   *
+   * Only supports 'Simple Key Pair'.


Ah perhaps not, but good eye. I wonder if this was done intentionally for simplicity or was added before the other keyring types were added and hasn't been updated since.

mcmire · 2024-01-26T20:49:24Z

packages/keyring-controller/src/utils.ts

+ *
+ * @param error - Error message or Error object to throw.
+ */
+export function throwError(error: string | Error): never {


This seems like such a simple function that I'm curious if we need it, but if it was copied from somewhere else, then I suppose that's fine.

We copied this one from eth-keyring-controller, but it is used in only three occurrences, and all of them just pass a string - so I think this is just adding cognitive overload.

removed in a9712ca

mcmire

Looks good to me!

mikesposito force-pushed the refactor/merge-keyring-controllers branch from 49d7b61 to bd80fe1 Compare January 24, 2024 12:30

This was referenced Jan 24, 2024

Add coverage for unsupported keyring methods #3831

Merged

Refactor user operations tests #3832

Merged

mikesposito force-pushed the refactor/merge-keyring-controllers branch 2 times, most recently from e087e79 to d11a19a Compare January 24, 2024 16:48

mikesposito commented Jan 24, 2024

View reviewed changes

mikesposito force-pushed the refactor/merge-keyring-controllers branch 2 times, most recently from 88ab32e to a5ed982 Compare January 24, 2024 20:26

mikesposito commented Jan 25, 2024

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

mikesposito mentioned this pull request Jan 25, 2024

Add tests for KeyringController edge cases #3847

Merged

3 tasks

mikesposito force-pushed the refactor/merge-keyring-controllers branch 2 times, most recently from 18c9f9a to 84d9cf4 Compare January 26, 2024 10:42

mikesposito marked this pull request as ready for review January 26, 2024 10:47

mikesposito requested a review from a team as a code owner January 26, 2024 10:47

mikesposito requested review from gantunesr, mcmire, danroc, Gudahtt, kanthesha and MajorLift January 26, 2024 11:02

mikesposito force-pushed the refactor/merge-keyring-controllers branch from c26aba4 to 83a94cd Compare January 26, 2024 12:28

mikesposito commented Jan 26, 2024

View reviewed changes

mikesposito force-pushed the refactor/merge-keyring-controllers branch from b2f6d4e to a9d1f1a Compare January 26, 2024 14:21

mcmire reviewed Jan 26, 2024

View reviewed changes

mikesposito force-pushed the refactor/merge-keyring-controllers branch from a9712ca to 9985f69 Compare January 29, 2024 13:21

mcmire approved these changes Jan 29, 2024

View reviewed changes

mikesposito added 15 commits January 29, 2024 20:18

merge eth-keyring-controller logic

1319d5d

refactor: rename updateMemStoreKeyrings

dd08f83

chore: use eth-sig-util ^7.0.1

0950f20

refactor: make createNewVaultWithKeyring private

f5e4632

refactor: drop eth-keyring-controller dependency

57116b9

fix: port isSerializedKeyringsArray function

a375ad5

test: adjust coverage

d38c7f8

refactor: #restoreKeyring function

0856709

refactor: remove unreachable branches

a4b7eab

test: add hdkeyring test case

29e7e47

refactor: remove addNewAccount code repetition

ee61a04

docs: update jsdoc

2139fbe

refactor: remove useless eslint-disable

8022c16

docs: fix jsdoc

03dde44

refactor: remove throwError util

dd28464

mikesposito force-pushed the refactor/merge-keyring-controllers branch from 9985f69 to dd28464 Compare January 29, 2024 19:18

mikesposito merged commit 50ee1ee into main Jan 30, 2024
136 checks passed

mikesposito deleted the refactor/merge-keyring-controllers branch January 30, 2024 09:03

MajorLift mentioned this pull request Mar 7, 2024

Merge ETHKeyringController and KeyringController #3836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `EthKeyringController` into `KeyringController` #3830

Merge `EthKeyringController` into `KeyringController` #3830

mikesposito commented Jan 24, 2024 •

edited

Loading

mikesposito Jan 24, 2024 •

edited

Loading

mikesposito Jan 25, 2024

mcmire Jan 26, 2024

mikesposito Jan 29, 2024

mcmire Jan 29, 2024

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 25, 2024 •

edited

Loading

mcmire Jan 26, 2024

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 26, 2024

mikesposito Jan 26, 2024 •

edited

Loading

mikesposito Jan 26, 2024

mcmire left a comment

mcmire Jan 26, 2024

mikesposito Jan 29, 2024

mcmire Jan 26, 2024

mcmire Jan 26, 2024

mcmire Jan 26, 2024

mcmire Jan 26, 2024

mikesposito Jan 29, 2024

mcmire left a comment

Merge EthKeyringController into KeyringController #3830

Merge EthKeyringController into KeyringController #3830

Conversation

mikesposito commented Jan 24, 2024 • edited Loading

Explanation

References

Changelog

@metamask/keyring-controller

Checklist

mikesposito Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

mikesposito Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

mikesposito Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Merge `EthKeyringController` into `KeyringController` #3830

Merge `EthKeyringController` into `KeyringController` #3830

mikesposito commented Jan 24, 2024 •

edited

Loading

`@metamask/keyring-controller`

mikesposito Jan 24, 2024 •

edited

Loading

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 25, 2024 •

edited

Loading

mikesposito Jan 26, 2024 •

edited

Loading