Make add new account idempotent operation (#15566)

app/scripts/metamask-controller.actions.test.js

@@ -0,0 +1,110 @@
+import { strict as assert } from 'assert';
+import sinon from 'sinon';
+import proxyquire from 'proxyquire';
+
+const Ganache = require('../../test/e2e/ganache');
+
+const ganacheServer = new Ganache();
+
+const browserPolyfillMock = {
+  runtime: {
+    id: 'fake-extension-id',
+    onInstalled: {
+      addListener: () => undefined,
+    },
+    onMessageExternal: {
+      addListener: () => undefined,
+    },
+    getPlatformInfo: async () => 'mac',
+  },
+};
+
+let loggerMiddlewareMock;
+const createLoggerMiddlewareMock = () => (req, res, next) => {
+  if (loggerMiddlewareMock) {
+    loggerMiddlewareMock.requests.push(req);
+    next((cb) => {
+      loggerMiddlewareMock.responses.push(res);
+      cb();
+    });
+    return;
+  }
+  next();
+};
+
+const MetaMaskController = proxyquire('./metamask-controller', {
+  './lib/createLoggerMiddleware': { default: createLoggerMiddlewareMock },
+}).default;
+
+describe('MetaMaskController', function () {
+  let metamaskController;
+  const sandbox = sinon.createSandbox();
+  const noop = () => undefined;
+
+  before(async function () {
+    await ganacheServer.start();
+  });
+
+  beforeEach(function () {
+    metamaskController = new MetaMaskController({
+      showUserConfirmation: noop,
+      encryptor: {
+        encrypt(_, object) {
+          this.object = object;
+          return Promise.resolve('mock-encrypted');
+        },
+        decrypt() {
+          return Promise.resolve(this.object);
+        },
+      },
+      initLangCode: 'en_US',
+      platform: {
+        showTransactionNotification: () => undefined,
+        getVersion: () => 'foo',
+      },
+      browser: browserPolyfillMock,
+      infuraProjectId: 'foo',
+    });
+  });
+
+  afterEach(function () {
+    sandbox.restore();
+  });
+
+  after(async function () {
+    await ganacheServer.quit();
+  });
+
+  describe('#addNewAccount', function () {
+    it('two parallel calls with same accountCount give same result', async function () {
+      await metamaskController.createNewVaultAndKeychain('test@123');
+      const [addNewAccountResult1, addNewAccountResult2] = await Promise.all([
+        metamaskController.addNewAccount(1),
+        metamaskController.addNewAccount(1),
+      ]);
+      assert.deepEqual(
+        Object.keys(addNewAccountResult1.identities),
+        Object.keys(addNewAccountResult2.identities),
+      );
+    });
+
+    it('two successive calls with same accountCount give same result', async function () {
+      await metamaskController.createNewVaultAndKeychain('test@123');
+      const [addNewAccountResult1, addNewAccountResult2] = await Promise.all([
+        metamaskController.addNewAccount(1),
+        Promise.resolve(1).then(() => metamaskController.addNewAccount(1)),
+      ]);
+      assert.deepEqual(
+        Object.keys(addNewAccountResult1.identities),
+        Object.keys(addNewAccountResult2.identities),
+      );
+    });
+
+    it('two successive calls with different accountCount give different results', async function () {
+      await metamaskController.createNewVaultAndKeychain('test@123');
+      const addNewAccountResult1 = await metamaskController.addNewAccount(1);
+      const addNewAccountResult2 = await metamaskController.addNewAccount(2);
+      assert.notDeepEqual(addNewAccountResult1, addNewAccountResult2);
+    });
+  });
+});

app/scripts/metamask-controller.js

@@ -2634,30 +2634,41 @@ export default class MetamaskController extends EventEmitter {
   /**
    * Adds a new account to the default (first) HD seed phrase Keyring.
    *
+   * @param accountCount
    * @returns {} keyState
    */
-  async addNewAccount() {
+  async addNewAccount(accountCount) {
     const primaryKeyring =
       this.keyringController.getKeyringsByType('HD Key Tree')[0];
     if (!primaryKeyring) {
       throw new Error('MetamaskController - No HD Key Tree found');
     }
     const { keyringController } = this;
-    const oldAccounts = await keyringController.getAccounts();
-    const keyState = await keyringController.addNewAccount(primaryKeyring);
-    const newAccounts = await keyringController.getAccounts();
+    const { identities: oldIdentities } =
+      this.preferencesController.store.getState();
 
-    await this.verifySeedPhrase();
+    if (Object.keys(oldIdentities).length === accountCount) {
+      const oldAccounts = await keyringController.getAccounts();
+      const keyState = await keyringController.addNewAccount(primaryKeyring);
+      const newAccounts = await keyringController.getAccounts();
 
-    this.preferencesController.setAddresses(newAccounts);
-    newAccounts.forEach((address) => {
-      if (!oldAccounts.includes(address)) {
-        this.preferencesController.setSelectedAddress(address);
-      }
-    });
+      await this.verifySeedPhrase();
 
-    const { identities } = this.preferencesController.store.getState();
-    return { ...keyState, identities };
+      this.preferencesController.setAddresses(newAccounts);
+      newAccounts.forEach((address) => {
+        if (!oldAccounts.includes(address)) {
+          this.preferencesController.setSelectedAddress(address);
+        }
+      });
+
+      const { identities } = this.preferencesController.store.getState();
+      return { ...keyState, identities };
+    }
+
+    return {
+      ...keyringController.memStore.getState(),
+      identities: oldIdentities,
+    };
   }
 
   /**

app/scripts/metamask-controller.test.js

@@ -734,7 +734,7 @@ describe('MetaMaskController', function () {
     });
 
     it('#addNewAccount', async function () {
-      await metamaskController.addNewAccount();
+      await metamaskController.addNewAccount(1);
       const getAccounts =
         await metamaskController.keyringController.getAccounts();
       assert.equal(getAccounts.length, 2);

ui/store/actions.js

@@ -353,7 +353,9 @@ export function addNewAccount() {
 
     let newIdentities;
     try {
-      const { identities } = await promisifiedBackground.addNewAccount();
+      const { identities } = await promisifiedBackground.addNewAccount(
+        Object.keys(oldIdentities).length,
+      );
       newIdentities = identities;
     } catch (error) {
       dispatch(displayWarning(error.message));

ui/store/actions.test.js

@@ -19,6 +19,9 @@ const defaultState = {
         balance: '0x0',
       },
     },
+    identities: {
+      '0xFirstAddress': {},
+    },
     cachedBalances: {
       '0x1': {
         '0xFirstAddress': '0x0',
@@ -387,7 +390,7 @@ describe('Actions', () => {
         metamask: { identities: {}, ...defaultState.metamask },
       });
 
-      const addNewAccount = background.addNewAccount.callsFake((cb) =>
+      const addNewAccount = background.addNewAccount.callsFake((_, cb) =>
         cb(null, {
           identities: {},
         }),
@@ -395,14 +398,14 @@ describe('Actions', () => {
 
       actions._setBackgroundConnection(background);
 
-      await store.dispatch(actions.addNewAccount());
+      await store.dispatch(actions.addNewAccount(1));
       expect(addNewAccount.callCount).toStrictEqual(1);
     });
 
     it('displays warning error message when addNewAccount in background callback errors', async () => {
       const store = mockStore();
 
-      background.addNewAccount.callsFake((cb) => {
+      background.addNewAccount.callsFake((_, cb) => {
         cb(new Error('error'));
       });
 
@@ -414,7 +417,7 @@ describe('Actions', () => {
         { type: 'HIDE_LOADING_INDICATION' },
       ];
 
-      await expect(store.dispatch(actions.addNewAccount())).rejects.toThrow(
+      await expect(store.dispatch(actions.addNewAccount(1))).rejects.toThrow(
         'error',
       );