Add confirmations for creation/removal of snap accounts (#20684)

## Explanation Progresses MetaMask/accounts-planning#14 Designs: https://www.figma.com/file/XzaR4r04pMXIE8ft1T2hmJ/Add%2Fremove-snap-accounts%2Fkeyring-snaps-%2313-and-%2314?type=design&node-id=604-2586&mode=design - this PR works in conjunction with changes in the [eth-snap-keyring](MetaMask/eth-snap-keyring#99) to intercept account additions from a snap and get approval from the user. - This PR heavily leverages existing patterns with the approval controller/confirmations flow and for that reason, differs slightly from the original designs. - Since we are waiting on the [accounts controller PR](#20344) to support custom names, I have removed the TextField from the creation screen. - I have also opted to use the out of the box approval success/error screens with custom messages instead of the designs that @raulmono provided above. These standardized success/error screens are almost the same as the designs but lack the description and translations from the design. It is not currently possible to have custom success/error screens without a massive overhaul to the confirmations flow which would effect many teams. If changes are needed we should work with @matthewwalsh0 to make those changes upstream at a later date. ## Screenshots/Screencaps  <img width="384" alt="Screenshot 2023-09-15 at 2 46 30 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7d119dc5-6a7c-4e83-ae9d-52390217a52d"> <img width="363" alt="Screenshot 2023-09-15 at 4 57 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/f20a63a6-9e16-40f7-b269-d697424454f0"> <img width="371" alt="Screenshot 2023-09-15 at 4 57 17 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/c00c2baa-8aa4-403f-aec1-7164a103792c"> <img width="370" alt="Screenshot 2023-09-20 at 3 54 27 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/8adc01ed-6138-43e7-aa93-7514d35f8a5e"> <img width="367" alt="Screenshot 2023-09-20 at 3 54 34 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/b246f7a6-05b1-4f5c-a2e5-d90b99310588"> ### Before https://github.com/MetaMask/metamask-extension/assets/22918444/465e7833-6ed1-4c38-b618-81723ed54928 ### After #### Happy Path https://github.com/MetaMask/metamask-extension/assets/22918444/049566e1-a121-4803-9166-1f26de2eba6f #### Error case https://github.com/MetaMask/metamask-extension/assets/22918444/fb628db4-4781-4bab-9efd-b755e011ff26 ## Manual Testing Steps This change requires the manual installing of the snaps rpc methods handlers and building metamask flask. Here are the steps. #### Setup 1. checkout this branch and run `yarn start --build-type flask` 2. in your browser, load the extension by clicking `Load unpacked` in the extension settings and using the `dist/chrome/` (or whatever browser you are using) as the source 3. create your account and login #### Testing the snap account creation 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 4. connect the dapp 5. click create account. Custom account names are not supported at this time <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 6. click create 7. a new account should be created on the snap 8. open metamask and click the accounts dropdown, a new snap account should be added #### Testing the denial flow 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 2. connect the dapp 3. click create account <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 4. A modal should open on extension 5. click cancel 9. the modal should close and the dapp should throw an error `User denied account creation` <img width="1158" alt="Screenshot 2023-09-20 at 4 14 25 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7eee4396-a762-4c97-a680-2e03b358596d"> #### Testing the error case - luckily for us, the snap simple keyring had a bug in version 0.2.1 that we can use to test this flow. To test the error flow, use the following steps 1. open this test snap: https://metamask.github.io/snap-simple-keyring/0.2.1/ 2. connect dapp (uninstall and reinstall if you were using a different version before) 3. click create account 4. the account creation modal should pop up 5. click create, an error should be shown with the message `Method not supported: event:accountCreated` 6. create another account 10. click cancel, the modal should close and the dapp should throw an error `User denied account addition` #### Testing account removal 1. follow the previous mentioned steps to create 2 or more snap accounts with this [snap](https://metamask.github.io/snap-simple-keyring/latest/) 2. copy the account `id` from the side bar and paste it into the remove account section of the snap 3. click remove account <img width="1184" alt="Screenshot 2023-09-20 at 4 17 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e8fb4891-a59d-436f-995c-02d145e1b3e9"> 4. A modal should show up indicating what you are trying to do 5. click the `share` icon beside the public address and it should open that account on etherscan. It should work with different networks as well. 6. click `remove` 7. a success screen should be shown and the snap should update with the state and the account no longer visible in the side bar 8. open the extension and verify that the account is no longer in the account list 9. repeat steps 1 and 2 10. this time click cancel 11. the extension should close and an error on the snap should appear with the text `User denied account removal` <img width="1168" alt="Screenshot 2023-09-20 at 4 20 03 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e85e8ab1-aec0-4ac7-8a56-5253c82af53b"> 12. verify that the account was not removed from the extension or the snap ## Pre-merge author checklist - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added ## Pre-merge reviewer checklist - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] PR is linked to the appropriate GitHub issue - [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. In this case, a QA Engineer approval will be be required. --------- Co-authored-by: Monte Lai <[email protected]> Co-authored-by: Matthew Walsh <[email protected]> Co-authored-by: Daniel Rocha <[email protected]> Co-authored-by: Howard Braham <[email protected]> Co-authored-by: MetaMask Bot <[email protected]>
MetaMask · Sep 29, 2023 · 57b14a8 · 57b14a8
1 parent 12c9d97
commit 57b14a8
Show file tree

Hide file tree

Showing 48 changed files with 1,673 additions and 210 deletions.
diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json
diff --git a/app/scripts/background.js b/app/scripts/background.js
@@ -28,6 +28,9 @@ import {
   ENVIRONMENT_TYPE_FULLSCREEN,
   EXTENSION_MESSAGES,
   PLATFORM_FIREFOX,
+  ///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
+  SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES,
+  ///: END:ONLY_INCLUDE_IN
 } from '../../shared/constants/app';
 import {
   REJECT_NOTIFICATION_CLOSE,
@@ -802,6 +805,14 @@ export function setupController(
             controller.approvalController.accept(id, false);
             break;
           ///: END:ONLY_INCLUDE_IN
+          ///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
+          case SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.confirmAccountCreation:
+            controller.approvalController.accept(id, false);
+            break;
+          case SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.confirmAccountRemoval:
+            controller.approvalController.accept(id, false);
+            break;
+          ///: END:ONLY_INCLUDE_IN
           default:
             controller.approvalController.reject(
               id,

diff --git a/app/scripts/lib/snap-keyring/index.ts b/app/scripts/lib/snap-keyring/index.ts
@@ -0,0 +1 @@
+export { default as snapKeyringBuilder } from './snap-keyring';
diff --git a/app/scripts/lib/snap-keyring/snap-keyring.ts b/app/scripts/lib/snap-keyring/snap-keyring.ts
@@ -0,0 +1,147 @@
+import { SnapKeyring } from '@metamask/eth-snap-keyring';
+import type { SnapController } from '@metamask/snaps-controllers';
+import type {
+  ApprovalController,
+  ResultComponent,
+} from '@metamask/approval-controller';
+import type { KeyringController } from '@metamask/keyring-controller';
+import { SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES } from '../../../../shared/constants/app';
+import { t } from '../../translate';
+/**
+ * Constructs a SnapKeyring builder with specified handlers for managing snap accounts.
+ *
+ * @param getSnapController - A function that retrieves the Snap Controller instance.
+ * @param getApprovalController - A function that retrieves the Approval Controller instance.
+ * @param getKeyringController - A function that retrieves the Keyring Controller instance.
+ * @param getCoreKeyringController - A function that retrieves the Core Keyring Controller instance.
+ * @param removeAccountHelper - A function to help remove an account based on its address.
+ * @returns The constructed SnapKeyring builder instance with the following methods:
+ * - `saveState`: Persists all keyrings in the keyring controller.
+ * - `addAccount`: Initiates the process of adding an account with user confirmation and handling the user input.
+ * - `removeAccount`: Initiates the process of removing an account with user confirmation and handling the user input.
+ */
+const snapKeyringBuilder = (
+  getSnapController: () => SnapController,
+  getApprovalController: () => ApprovalController,
+  getKeyringController: () => KeyringController,
+  getCoreKeyringController: () => KeyringController,
+  removeAccountHelper: (address: string) => Promise<any>,
+) => {
+  const builder = (() => {
+    return new SnapKeyring(getSnapController() as any, {
+      addressExists: async (address) => {
+        const addresses = await getCoreKeyringController().getAccounts();
+        return addresses.includes(address.toLowerCase());
+      },
+      saveState: async () => {
+        await getKeyringController().persistAllKeyrings();
+      },
+      addAccount: async (
+        _address: string,
+        origin: string,
+        handleUserInput: (accepted: boolean) => Promise<void>,
+      ) => {
+        const { id: addAccountApprovalId } =
+          getApprovalController().startFlow();
+
+        const snapAuthorshipHeader: ResultComponent = {
+          name: 'SnapAuthorshipHeader',
+          key: 'snapHeader',
+          properties: { snapId: origin },
+        };
+
+        try {
+          const confirmationResult: boolean =
+            (await getApprovalController().addAndShowApprovalRequest({
+              origin,
+              type: SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.confirmAccountCreation,
+            })) as boolean;
+
+          if (confirmationResult) {
+            try {
+              await handleUserInput(confirmationResult);
+              await getKeyringController().persistAllKeyrings();
+              await getApprovalController().success({
+                message: t('snapAccountCreated') ?? 'Your account is ready!',
+                header: [snapAuthorshipHeader],
+              });
+            } catch (error) {
+              await getApprovalController().error({
+                error: (error as Error).message,
+                header: [snapAuthorshipHeader],
+              });
+              throw new Error(
+                `Error occurred while creating snap account: ${
+                  (error as Error).message
+                }`,
+              );
+            }
+          } else {
+            await handleUserInput(confirmationResult);
+            throw new Error('User denied account creation');
+          }
+        } finally {
+          getApprovalController().endFlow({
+            id: addAccountApprovalId,
+          });
+        }
+      },
+      removeAccount: async (
+        address: string,
+        snapId: string,
+        handleUserInput: (accepted: boolean) => Promise<void>,
+      ) => {
+        const { id: removeAccountApprovalId } =
+          getApprovalController().startFlow();
+
+        const snapAuthorshipHeader: ResultComponent = {
+          name: 'SnapAuthorshipHeader',
+          key: 'snapHeader',
+          properties: { snapId },
+        };
+
+        try {
+          const confirmationResult: boolean =
+            (await getApprovalController().addAndShowApprovalRequest({
+              origin: snapId,
+              type: SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.confirmAccountRemoval,
+              requestData: { publicAddress: address },
+            })) as boolean;
+
+          if (confirmationResult) {
+            try {
+              await removeAccountHelper(address);
+              await handleUserInput(confirmationResult);
+              await getKeyringController().persistAllKeyrings();
+              await getApprovalController().success({
+                message: t('snapAccountRemoved') ?? 'Account removed',
+                header: [snapAuthorshipHeader],
+              });
+            } catch (error) {
+              await getApprovalController().error({
+                error: (error as Error).message,
+                header: [snapAuthorshipHeader],
+              });
+              throw new Error(
+                `Error occurred while removing snap account: ${
+                  (error as Error).message
+                }`,
+              );
+            }
+          } else {
+            await handleUserInput(confirmationResult);
+            throw new Error('User denied account removal');
+          }
+        } finally {
+          getApprovalController().endFlow({
+            id: removeAccountApprovalId,
+          });
+        }
+      },
+    });
+  }) as any;
+  builder.type = SnapKeyring.type;
+  return builder;
+};
+
+export default snapKeyringBuilder;
diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
@@ -70,9 +70,6 @@ import {
   IframeExecutionService,
 } from '@metamask/snaps-controllers';
 ///: END:ONLY_INCLUDE_IN
-///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
-import { SnapKeyring } from '@metamask/eth-snap-keyring';
-///: END:ONLY_INCLUDE_IN
 
 ///: BEGIN:ONLY_INCLUDE_IN(build-mmi)
 import {
@@ -242,6 +239,9 @@ import { securityProviderCheck } from './lib/security-provider-helpers';
 import { IndexedDBPPOMStorage } from './lib/ppom/indexed-db-backend';
 ///: END:ONLY_INCLUDE_IN
 import { updateCurrentLocale } from './translate';
+///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
+import { snapKeyringBuilder } from './lib/snap-keyring';
+///: END:ONLY_INCLUDE_IN
 
 export const METAMASK_CONTROLLER_EVENTS = {
   // Fired after state changes that impact the extension badge (unapproved msg count)
@@ -855,21 +855,21 @@ export default class MetamaskController extends EventEmitter {
     }
 
     ///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
+    const getSnapController = () => this.snapController;
+    const getApprovalController = () => this.approvalController;
+    const getKeyringController = () => this.keyringController;
+    const getCoreKeyringController = () => this.coreKeyringController;
+
     additionalKeyrings.push(
-      (() => {
-        const builder = () =>
-          new SnapKeyring(this.snapController, {
-            saveState: async () => {
-              await this.keyringController.persistAllKeyrings();
-            },
-            removeAccount: async (address) => {
-              await this.removeAccount(address);
-            },
-          });
-        builder.type = SnapKeyring.type;
-        return builder;
-      })(),
+      snapKeyringBuilder(
+        getSnapController,
+        getApprovalController,
+        getKeyringController,
+        getCoreKeyringController,
+        (address) => this.removeAccount(address),
+      ),
     );
+
     ///: END:ONLY_INCLUDE_IN
 
     const keyringControllerMessenger = this.controllerMessenger.getRestricted({

diff --git a/development/verify-locale-strings.js b/development/verify-locale-strings.js
@@ -187,6 +187,8 @@ async function verifyEnglishLocale() {
       'shared/**/*.js',
       'shared/**/*.ts',
       'shared/**/*.tsx',
+      'app/scripts/lib/**/*.js',
+      'app/scripts/lib/**/*.ts',
       'app/scripts/constants/**/*.js',
       'app/scripts/constants/**/*.ts',
       'app/scripts/platforms/**/*.js',

diff --git a/lavamoat/browserify/desktop/policy.json b/lavamoat/browserify/desktop/policy.json
@@ -1188,7 +1188,8 @@
     },
     "@metamask/eth-snap-keyring": {
       "globals": {
-        "console.error": true
+        "console.error": true,
+        "console.log": true
       },
       "packages": {
         "@ethereumjs/tx": true,
@@ -2248,7 +2249,6 @@
         "@metamask/providers>@metamask/object-multiplex": true,
         "@metamask/rpc-methods": true,
         "@metamask/snaps-controllers>@metamask/post-message-stream": true,
-        "@metamask/snaps-controllers>@metamask/snaps-registry": true,
         "@metamask/snaps-controllers>@metamask/utils": true,
         "@metamask/snaps-controllers>@xstate/fsm": true,
         "@metamask/snaps-controllers>concat-stream": true,
@@ -2257,6 +2257,7 @@
         "@metamask/snaps-controllers>readable-web-to-node-stream": true,
         "@metamask/snaps-controllers>tar-stream": true,
         "@metamask/snaps-utils": true,
+        "@metamask/snaps-utils>@metamask/snaps-registry": true,
         "eth-rpc-errors": true,
         "json-rpc-engine": true,
         "json-rpc-middleware-stream": true,
@@ -2296,13 +2297,6 @@
         "superstruct": true
       }
     },
-    "@metamask/snaps-controllers>@metamask/snaps-registry": {
-      "packages": {
-        "@metamask/key-tree>@noble/secp256k1": true,
-        "@metamask/snaps-controllers>@metamask/utils": true,
-        "superstruct": true
-      }
-    },
     "@metamask/snaps-controllers>@metamask/utils": {
       "globals": {
         "TextDecoder": true,
@@ -2511,6 +2505,26 @@
         "superstruct": true
       }
     },
+    "@metamask/snaps-utils>@metamask/snaps-registry": {
+      "packages": {
+        "@metamask/key-tree>@noble/secp256k1": true,
+        "@metamask/snaps-utils>@metamask/snaps-registry>@metamask/utils": true,
+        "superstruct": true
+      }
+    },
+    "@metamask/snaps-utils>@metamask/snaps-registry>@metamask/utils": {
+      "globals": {
+        "TextDecoder": true,
+        "TextEncoder": true
+      },
+      "packages": {
+        "@metamask/key-tree>@noble/hashes": true,
+        "browserify>buffer": true,
+        "nock>debug": true,
+        "semver": true,
+        "superstruct": true
+      }
+    },
     "@metamask/snaps-utils>@metamask/utils": {
       "globals": {
         "TextDecoder": true,
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		export { default as snapKeyringBuilder } from './snap-keyring';