feat(keyring-controller): allow non-evm addresses for non-evm methods/functions (#4282)

ccharly · web-flow · commit 3be5fcc7e884 · 2024-05-20T14:09:50.000+02:00
## Explanation

This allows support of non-EVM addresses for most keyring methods.

Right now, our addresses are just plain `string`, meaning we have not
easy way to detect if an address is an ethereum one or not. So we use
runtime check to verify this, and based on this we adapt some logic
(mostly the address normalization right now).

Also, some `Hex` were being dropped in favor of `string` type to make
addresses a bit more "generic".

## References

&lt;!--
Are there any issues that this pull request is tied to? Are there other
links that reviewers should consult to understand these changes better?

For example:

* Fixes #12345
* Related to #67890
--&gt;

## Changelog

&lt;!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
--&gt;


### `@metamask/keyring-controller`

- **ADDED**: Basic support for non-EVM addresses

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
@@ -657,7 +657,7 @@ describe('KeyringController', () => {
               await expect(
                 controller.exportAccount(password, ''),
               ).rejects.toThrow(
-                'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
+                'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
               );
             });
           });
@@ -688,7 +688,7 @@ describe('KeyringController', () => {
 
     describe('when the keyring for the given address does not support exportAccount', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -737,7 +737,7 @@ describe('KeyringController', () => {
 
     describe('when the keyring for the given address does not support getEncryptionPublicKey', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -799,15 +799,15 @@ describe('KeyringController', () => {
           await expect(
             controller.decryptMessage(messageParams),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
     });
 
     describe('when the keyring for the given address does not support decryptMessage', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -1181,15 +1181,15 @@ describe('KeyringController', () => {
           await expect(
             controller.removeAccount('0xDUMMY_INPUT'),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
     });
 
     describe('when the keyring for the given address does not support removeAccount', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -1239,15 +1239,15 @@ describe('KeyringController', () => {
               from: '',
             }),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
     });
 
     describe('when the keyring for the given address does not support signMessage', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -1307,15 +1307,15 @@ describe('KeyringController', () => {
               from: '',
             }),
           ).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
     });
 
     describe('when the keyring for the given address does not support signPersonalMessage', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -1577,7 +1577,7 @@ describe('KeyringController', () => {
 
     describe('when the keyring for the given address does not support signTypedMessage', () => {
       it('should throw error', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
@@ -1659,7 +1659,7 @@ describe('KeyringController', () => {
             expect(unsignedEthTx.v).toBeUndefined();
             await controller.signTransaction(unsignedEthTx, '');
           }).rejects.toThrow(
-            'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
+            'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
           );
         });
       });
@@ -1681,7 +1681,7 @@ describe('KeyringController', () => {
 
     describe('when the keyring for the given address does not support signTransaction', () => {
       it('should throw if the keyring for the given address does not support signTransaction', async () => {
-        const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19';
+        const address = '0x5AC6D462f054690a373FABF8CC28e161003aEB19';
         stubKeyringClassWithAccount(MockKeyring, address);
         await withController(
           { keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
@@ -8,7 +8,7 @@ import type { RestrictedControllerMessenger } from '@metamask/base-controller';
 import { BaseController } from '@metamask/base-controller';
 import * as encryptorUtils from '@metamask/browser-passworder';
 import HDKeyring from '@metamask/eth-hd-keyring';
-import { normalize } from '@metamask/eth-sig-util';
+import { normalize as ethNormalize } from '@metamask/eth-sig-util';
 import SimpleKeyring from '@metamask/eth-simple-keyring';
 import type {
   EthBaseTransaction,
@@ -34,6 +34,7 @@ import {
   bytesToHex,
   hasProperty,
   isObject,
+  isStrictHexString,
   isValidHexAddress,
   isValidJson,
   remove0x,
@@ -503,12 +504,44 @@ async function displayForKeyring(
 
   return {
     type: keyring.type,
-    // Cast to `Hex[]` here is safe here because `accounts` has no nullish
-    // values, and `normalize` returns `Hex` unless given a nullish value
-    accounts: accounts.map(normalize) as Hex[],
+    // Cast to `string[]` here is safe here because `accounts` has no nullish
+    // values, and `normalize` returns `string` unless given a nullish value
+    accounts: accounts.map(normalize) as string[],
   };
 }
 
+/**
+ * Check if address is an ethereum address
+ *
+ * @param address - An address.
+ * @returns Returns true if the address is an ethereum one, false otherwise.
+ */
+function isEthAddress(address: string): boolean {
+  // We first check if it's a matching `Hex` string, so that is narrows down
+  // `address` as an `Hex` type, allowing us to use `isValidHexAddress`
+  return (
+    // NOTE: This function only checks for lowercased strings
+    isStrictHexString(address.toLowerCase()) &&
+    // This checks for lowercased addresses and checksum addresses too
+    isValidHexAddress(address as Hex)
+  );
+}
+
+/**
+ * Normalize ethereum or non-EVM address.
+ *
+ * @param address - Ethereum or non-EVM address.
+ * @returns The normalized address.
+ */
+function normalize(address: string): string | undefined {
+  // Since the `KeyringController` is only dealing with address, we have
+  // no other way to get the associated account type with this address. So we
+  // are down to check the actual address format for now
+  // TODO: Find a better way to not have those runtime checks based on the
+  //       address value!
+  return isEthAddress(address) ? ethNormalize(address) : address;
+}
+
 /**
  * Controller responsible for establishing and managing user identity.
  *
@@ -644,6 +677,10 @@ export class KeyringController extends BaseController<
     keyring: EthKeyring<Json>,
     accountCount?: number,
   ): Promise<Hex> {
+    // READ THIS CAREFULLY:
+    // We still uses `Hex` here, since we are not using this method when creating
+    // and account using a "Snap Keyring". This function assume the `keyring` is
+    // ethereum compatible, but "Snap Keyring" might not be.
     return this.#persistOrRollback(async () => {
       const oldAccounts = await this.#getAccountsFromKeyrings();
 
@@ -828,15 +865,15 @@ export class KeyringController extends BaseController<
     account: string,
     opts?: Record<string, unknown>,
   ): Promise<string> {
-    const normalizedAddress = normalize(account) as Hex;
+    const address = ethNormalize(account) as Hex;
     const keyring = (await this.getKeyringForAccount(
       account,
     )) as EthKeyring<Json>;
     if (!keyring.getEncryptionPublicKey) {
       throw new Error(KeyringControllerError.UnsupportedGetEncryptionPublicKey);
     }
 
-    return await keyring.getEncryptionPublicKey(normalizedAddress, opts);
+    return await keyring.getEncryptionPublicKey(address, opts);
   }
 
   /**
@@ -851,7 +888,7 @@ export class KeyringController extends BaseController<
     from: string;
     data: Eip1024EncryptedData;
   }): Promise<string> {
-    const address = normalize(messageParams.from) as Hex;
+    const address = ethNormalize(messageParams.from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -873,9 +910,7 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to keyring of the `account` if one exists.
    */
   async getKeyringForAccount(account: string): Promise<unknown> {
-    // Cast to `Hex` here is safe here because `address` is not nullish.
-    // `normalizeToHex` returns `Hex` unless given a nullish value.
-    const hexed = normalize(account) as Hex;
+    const address = normalize(account);
 
     const candidates = await Promise.all(
       this.#keyrings.map(async (keyring) => {
@@ -885,7 +920,7 @@ export class KeyringController extends BaseController<
 
     const winners = candidates.filter((candidate) => {
       const accounts = candidate[1].map(normalize);
-      return accounts.includes(hexed);
+      return accounts.includes(address);
     });
 
     if (winners.length && winners[0]?.length) {
@@ -894,9 +929,7 @@ export class KeyringController extends BaseController<
 
     // Adding more info to the error
     let errorInfo = '';
-    if (!isValidHexAddress(hexed)) {
-      errorInfo = 'The address passed in is invalid/empty';
-    } else if (!candidates.length) {
+    if (!candidates.length) {
       errorInfo = 'There are no keyrings';
     } else if (!winners.length) {
       errorInfo = 'There are keyrings, but none match the address';
@@ -999,7 +1032,7 @@ export class KeyringController extends BaseController<
    * @fires KeyringController:accountRemoved
    * @returns Promise resolving when the account is removed.
    */
-  async removeAccount(address: Hex): Promise<void> {
+  async removeAccount(address: string): Promise<void> {
     await this.#persistOrRollback(async () => {
       const keyring = (await this.getKeyringForAccount(
         address,
@@ -1013,7 +1046,10 @@ export class KeyringController extends BaseController<
       // The `removeAccount` method of snaps keyring is async. We have to update
       // the interface of the other keyrings to be async as well.
       // eslint-disable-next-line @typescript-eslint/await-thenable
-      await keyring.removeAccount(address);
+      // FIXME: We do cast to `Hex` to makes the type checker happy here, and
+      // because `Keyring<State>.removeAccount` requires address to be `Hex`. Those
+      // type would need to be updated for a full non-EVM support.
+      await keyring.removeAccount(address as Hex);
 
       const accounts = await keyring.getAccounts();
       // Check if this was the last/only account
@@ -1057,7 +1093,7 @@ export class KeyringController extends BaseController<
       throw new Error("Can't sign an empty message");
     }
 
-    const address = normalize(messageParams.from) as Hex;
+    const address = ethNormalize(messageParams.from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -1075,7 +1111,7 @@ export class KeyringController extends BaseController<
    * @returns Promise resolving to a signed message string.
    */
   async signPersonalMessage(messageParams: PersonalMessageParams) {
-    const address = normalize(messageParams.from) as Hex;
+    const address = ethNormalize(messageParams.from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -1113,7 +1149,7 @@ export class KeyringController extends BaseController<
 
       // Cast to `Hex` here is safe here because `messageParams.from` is not nullish.
       // `normalize` returns `Hex` unless given a nullish value.
-      const address = normalize(messageParams.from) as Hex;
+      const address = ethNormalize(messageParams.from) as Hex;
       const keyring = (await this.getKeyringForAccount(
         address,
       )) as EthKeyring<Json>;
@@ -1147,7 +1183,7 @@ export class KeyringController extends BaseController<
     from: string,
     opts?: Record<string, unknown>,
   ): Promise<TxData> {
-    const address = normalize(from) as Hex;
+    const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -1171,7 +1207,7 @@ export class KeyringController extends BaseController<
     transactions: EthBaseTransaction[],
     executionContext: KeyringExecutionContext,
   ): Promise<EthBaseUserOperation> {
-    const address = normalize(from) as Hex;
+    const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -1201,7 +1237,7 @@ export class KeyringController extends BaseController<
     userOp: EthUserOperation,
     executionContext: KeyringExecutionContext,
   ): Promise<EthUserOperationPatch> {
-    const address = normalize(from) as Hex;
+    const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -1226,7 +1262,7 @@ export class KeyringController extends BaseController<
     userOp: EthUserOperation,
     executionContext: KeyringExecutionContext,
   ): Promise<string> {
-    const address = normalize(from) as Hex;
+    const address = ethNormalize(from) as Hex;
     const keyring = (await this.getKeyringForAccount(
       address,
     )) as EthKeyring<Json>;
@@ -1979,7 +2015,7 @@ export class KeyringController extends BaseController<
    *
    * @returns A promise resolving to an array of accounts.
    */
-  async #getAccountsFromKeyrings(): Promise<Hex[]> {
+  async #getAccountsFromKeyrings(): Promise<string[]> {
     const keyrings = this.#keyrings;
 
     const keyringArrays = await Promise.all(
@@ -1989,9 +2025,9 @@ export class KeyringController extends BaseController<
       return res.concat(arr);
     }, []);
 
-    // Cast to `Hex[]` here is safe here because `addresses` has no nullish
-    // values, and `normalize` returns `Hex` unless given a nullish value
-    return addresses.map(normalize) as Hex[];
+    // Cast to `string[]` here is safe here because `addresses` has no nullish
+    // values, and `normalize` returns `string` unless given a nullish value
+    return addresses.map(normalize) as string[];
   }
 
   /**
