Upgrading the Import Account modal

app/scripts/account-import-strategies/index.js

@@ -1,9 +1,15 @@
-import log from 'loglevel';
+import { isValidMnemonic } from '@ethersproject/hdnode';
+import {
+  bufferToHex,
+  getBinarySize,
+  isValidPrivate,
+  toBuffer,
+} from 'ethereumjs-util';
 import Wallet from 'ethereumjs-wallet';
 import importers from 'ethereumjs-wallet/thirdparty';
-import { toBuffer, isValidPrivate, bufferToHex } from 'ethereumjs-util';
-import { addHexPrefix } from '../lib/util';
+import log from 'loglevel';
 import { stripHexPrefix } from '../../../shared/modules/hexstring-utils';
+import { addHexPrefix } from '../lib/util';
 
 const accountImporter = {
   async importAccount(strategy, args) {
@@ -15,18 +21,37 @@ const accountImporter = {
   strategies: {
     'Private Key': (privateKey) => {
       if (!privateKey) {
-        throw new Error('Cannot import an empty key.');
+        throw new Error('Cannot import an empty key.'); // It should never get here, because this should be stopped in the UI
+      }
+
+      // Check if the user has entered an SRP by mistake instead of a private key
+      if (isValidMnemonic(privateKey.trim())) {
+        throw new Error(`t('importAccountErrorIsSRP')`);
       }
 
-      const prefixed = addHexPrefix(privateKey);
-      const buffer = toBuffer(prefixed);
+      const trimmedPrivateKey = privateKey.replace(/\s+/gu, ''); // Remove all whitespace
 
-      if (!isValidPrivate(buffer)) {
-        throw new Error('Cannot import invalid private key.');
+      const prefixedPrivateKey = addHexPrefix(trimmedPrivateKey);
+      let buffer;
+      try {
+        buffer = toBuffer(prefixedPrivateKey);
+      } catch (e) {
+        throw new Error(`t('importAccountErrorNotHexadecimal')`);
+      }
+
+      try {
+        if (
+          !isValidPrivate(buffer) ||
+          getBinarySize(prefixedPrivateKey) !== 64 + '0x'.length // Fixes issue #17719 -- isValidPrivate() will let a key of 63 hex digits through without complaining, this line ensures 64 hex digits + '0x' = 66 digits
+        ) {
+          throw new Error(`t('importAccountErrorNotAValidPrivateKey')`);
+        }
+      } catch (e) {
+        throw new Error(`t('importAccountErrorNotAValidPrivateKey')`);
       }
 
-      const stripped = stripHexPrefix(prefixed);
-      return stripped;
+      const strippedPrivateKey = stripHexPrefix(prefixedPrivateKey);
+      return strippedPrivateKey;
     },
     'JSON File': (input, password) => {
       let wallet;

app/scripts/lib/account-tracker.test.js

@@ -1,4 +1,3 @@
-import { strict as assert } from 'assert';
 import EventEmitter from 'events';
 
 import { SINGLE_CALL_BALANCES_ADDRESS } from '../constants/contracts';
@@ -94,7 +93,7 @@ describe('Account Tracker', () => {
         currentBlockGasLimit: '',
       };
 
-      assert.deepEqual(newAccounts, expectedAccounts);
+      expect(newAccounts).toStrictEqual(expectedAccounts);
     });
 
     it('should not change accounts if the passed address is not in accounts', async () => {
@@ -118,7 +117,7 @@ describe('Account Tracker', () => {
         currentBlockGasLimit: '',
       };
 
-      assert.deepEqual(newAccounts, expectedAccounts);
+      expect(newAccounts).toStrictEqual(expectedAccounts);
     });
 
     it('should update the passed address account balance, and set other balances to null, if useMultiAccountBalanceChecker is false', async () => {
@@ -137,7 +136,7 @@ describe('Account Tracker', () => {
         currentBlockGasLimit: '',
       };
 
-      assert.deepEqual(newAccounts, expectedAccounts);
+      expect(newAccounts).toStrictEqual(expectedAccounts);
     });
   });
 
@@ -164,7 +163,7 @@ describe('Account Tracker', () => {
         currentBlockGasLimit: '',
       };
 
-      assert.deepEqual(newAccounts, expectedAccounts);
+      expect(newAccounts).toStrictEqual(expectedAccounts);
     });
 
     it('should update all balances if useMultiAccountBalanceChecker is true', async () => {
@@ -192,7 +191,7 @@ describe('Account Tracker', () => {
         currentBlockGasLimit: '',
       };
 
-      assert.deepEqual(newAccounts, expectedAccounts);
+      expect(newAccounts).toStrictEqual(expectedAccounts);
     });
   });
 });

test/e2e/tests/add-account.spec.js

@@ -242,7 +242,7 @@ describe('Add account', function () {
 
         // enter private key',
         await driver.fill('#private-key-box', testPrivateKey);
-        await driver.clickElement({ text: 'Import', tag: 'button' });
+        await driver.clickElement({ text: 'Import', tag: 'span' });
 
         // should show the correct account name
         const importedAccountName = await driver.findElement(

test/e2e/tests/from-import-ui.spec.js

@@ -212,7 +212,7 @@ describe('MetaMask Import UI', function () {
 
         // enter private key',
         await driver.fill('#private-key-box', testPrivateKey1);
-        await driver.clickElement({ text: 'Import', tag: 'button' });
+        await driver.clickElement({ text: 'Import', tag: 'span' });
 
         // should show the correct account name
         const importedAccountName = await driver.findElement(
@@ -239,7 +239,7 @@ describe('MetaMask Import UI', function () {
         await driver.clickElement({ text: 'Import account', tag: 'div' });
         // enter private key
         await driver.fill('#private-key-box', testPrivateKey2);
-        await driver.clickElement({ text: 'Import', tag: 'button' });
+        await driver.clickElement({ text: 'Import', tag: 'span' });
 
         // should see new account in account menu
         const importedAccount2Name = await driver.findElement(
@@ -315,7 +315,7 @@ describe('MetaMask Import UI', function () {
         await driver.clickElement('.account-menu__icon');
         await driver.clickElement({ text: 'Import account', tag: 'div' });
 
-        await driver.clickElement('.new-account-import-form__select');
+        await driver.clickElement('.dropdown__select');
         await driver.clickElement({ text: 'JSON File', tag: 'option' });
 
         const fileInput = await driver.findElement('input[type="file"]');
@@ -330,7 +330,7 @@ describe('MetaMask Import UI', function () {
 
         await driver.fill('#json-password-box', 'foobarbazqux');
 
-        await driver.clickElement({ text: 'Import', tag: 'button' });
+        await driver.clickElement({ text: 'Import', tag: 'span' });
 
         // should show the correct account name
         const importedAccountName = await driver.findElement(
@@ -392,11 +392,11 @@ describe('MetaMask Import UI', function () {
 
         // enter private key',
         await driver.fill('#private-key-box', testPrivateKey);
-        await driver.clickElement({ text: 'Import', tag: 'button' });
+        await driver.clickElement({ text: 'Import', tag: 'span' });
 
         // error should occur
         await driver.waitForSelector({
-          css: '.error',
+          css: '.mm-help-text',
           text: 'The account you are trying to import is a duplicate',
         });
       },

ui/components/app/asset-list-item/asset-list-item.js

@@ -144,7 +144,7 @@ const AssetListItem = ({
               name={ICON_NAMES.ARROW_RIGHT}
               color={Color.iconDefault}
               size={ICON_SIZES.SM}
-              style={{ 'vertical-align': 'middle' }}
+              style={{ verticalAlign: 'middle' }}
             />
             {sendTokenButton}
           </Box>

ui/components/component-library/form-text-field/form-text-field.js

@@ -103,7 +103,6 @@ export const FormTextField = ({
     />
     {helpText && (
       <HelpText
-        error={error}
         severity={error && SEVERITIES.DANGER}
         marginTop={1}
         {...helpTextProps}

ui/components/component-library/help-text/help-text.js

@@ -47,7 +47,7 @@ export const HelpText = ({
 };
 HelpText.propTypes = {
   /**
-   * The color of the HelpText will be overridden if error is true
+   * The color of the HelpText will be overridden if there is a severity passed
    * Defaults to Color.textDefault
    */
   color: PropTypes.oneOf(Object.values(TextColor)),

ui/components/component-library/index.js

@@ -26,7 +26,7 @@ export { Label } from './label';
 export { PickerNetwork } from './picker-network';
 export { Tag } from './tag';
 export { TagUrl } from './tag-url';
-export { Text, TEXT_DIRECTIONS } from './text';
+export { Text, TEXT_DIRECTIONS, INVISIBLE_CHARACTER } from './text';
 export { Input, INPUT_TYPES } from './input';
 export { TextField, TEXT_FIELD_TYPES, TEXT_FIELD_SIZES } from './text-field';
 export { TextFieldSearch } from './text-field-search';

ui/components/component-library/text/index.js

@@ -1,2 +1,2 @@
 export { Text } from './text';
-export { TEXT_DIRECTIONS } from './text.constants';
+export { TEXT_DIRECTIONS, INVISIBLE_CHARACTER } from './text.constants';

ui/components/component-library/text/text.constants.js

@@ -3,3 +3,9 @@ export const TEXT_DIRECTIONS = {
   RIGHT_TO_LEFT: 'rtl',
   AUTO: 'auto',
 };
+
+/**
+ * The INVISIBLE_CHARACTER is a very useful tool if you want to make sure a line of text
+ * takes up vertical space even if it's empty.
+ */
+export const INVISIBLE_CHARACTER = '\u200B';

ui/helpers/utils/accounts.js

@@ -1,3 +1,5 @@
+import { INVISIBLE_CHARACTER } from '../../components/component-library';
+
 export function getAccountNameErrorMessage(
   accounts,
   context,
@@ -26,7 +28,7 @@ export function getAccountNameErrorMessage(
 
   let errorMessage;
   if (isValidAccountName) {
-    errorMessage = '\u200d'; // This is Unicode for an invisible character, so the spacing stays constant
+    errorMessage = INVISIBLE_CHARACTER; // Using an invisible character, so the spacing stays constant
   } else if (isDuplicateAccountName) {
     errorMessage = context.t('accountNameDuplicate');
   } else if (isReservedAccountName) {

ui/pages/create-account/create-account.component.js

@@ -1,39 +1,36 @@
-import React, { Component } from 'react';
-import { Switch, Route } from 'react-router-dom';
+import React from 'react';
+import { Route, Switch } from 'react-router-dom';
+import Box from '../../components/ui/box';
 
 import {
-  NEW_ACCOUNT_ROUTE,
-  IMPORT_ACCOUNT_ROUTE,
   CONNECT_HARDWARE_ROUTE,
+  IMPORT_ACCOUNT_ROUTE,
+  NEW_ACCOUNT_ROUTE,
 } from '../../helpers/constants/routes';
-import NewAccountCreateForm from './new-account.container';
-import NewAccountImportForm from './import-account';
 import ConnectHardwareForm from './connect-hardware';
+import NewAccountImportForm from './import-account';
+import NewAccountCreateForm from './new-account.container';
 
-export default class CreateAccountPage extends Component {
-  render() {
-    return (
-      <div className="new-account">
-        <div className="new-account__form">
-          <Switch>
-            <Route
-              exact
-              path={NEW_ACCOUNT_ROUTE}
-              component={NewAccountCreateForm}
-            />
-            <Route
-              exact
-              path={IMPORT_ACCOUNT_ROUTE}
-              component={NewAccountImportForm}
-            />
-            <Route
-              exact
-              path={CONNECT_HARDWARE_ROUTE}
-              component={ConnectHardwareForm}
-            />
-          </Switch>
-        </div>
-      </div>
-    );
-  }
+export default function CreateAccountPage() {
+  return (
+    <Box className="new-account">
+      <Switch>
+        <Route
+          exact
+          path={NEW_ACCOUNT_ROUTE}
+          component={NewAccountCreateForm}
+        />
+        <Route
+          exact
+          path={IMPORT_ACCOUNT_ROUTE}
+          component={NewAccountImportForm}
+        />
+        <Route
+          exact
+          path={CONNECT_HARDWARE_ROUTE}
+          component={ConnectHardwareForm}
+        />
+      </Switch>
+    </Box>
+  );
 }

ui/pages/create-account/import-account/bottom-buttons.js

@@ -0,0 +1,48 @@
+import PropTypes from 'prop-types';
+import React from 'react';
+import { useDispatch } from 'react-redux';
+import {
+  ButtonPrimary,
+  ButtonSecondary,
+  BUTTON_SECONDARY_SIZES,
+} from '../../../components/component-library';
+import Box from '../../../components/ui/box/box';
+import { DISPLAY } from '../../../helpers/constants/design-system';
+import { useI18nContext } from '../../../hooks/useI18nContext';
+import * as actions from '../../../store/actions';
+
+BottomButtons.propTypes = {
+  importAccountFunc: PropTypes.func.isRequired,
+  isPrimaryDisabled: PropTypes.bool.isRequired,
+};
+
+export default function BottomButtons({
+  importAccountFunc,
+  isPrimaryDisabled,
+}) {
+  const t = useI18nContext();
+  const dispatch = useDispatch();
+
+  return (
+    <Box display={DISPLAY.FLEX} gap={4}>
+      <ButtonSecondary
+        onClick={() => {
+          dispatch(actions.hideWarning());
+          window.history.back();
+        }}
+        size={BUTTON_SECONDARY_SIZES.LG}
+        block
+      >
+        {t('cancel')}
+      </ButtonSecondary>
+      <ButtonPrimary
+        onClick={importAccountFunc}
+        disabled={isPrimaryDisabled}
+        size={BUTTON_SECONDARY_SIZES.LG}
+        block
+      >
+        {t('import')}
+      </ButtonPrimary>
+    </Box>
+  );
+}

ui/pages/create-account/import-account/bottom-buttons.stories.js

@@ -0,0 +1,18 @@
+import React from 'react';
+import BottomButtons from './bottom-buttons';
+
+export default {
+  title: 'Pages/CreateAccount/ImportAccount/BottomButtons',
+  component: BottomButtons,
+  argTypes: {
+    isPrimaryDisabled: {
+      control: {
+        type: 'boolean',
+      },
+    },
+  },
+};
+
+export const DefaultStory = (args) => <BottomButtons {...args} />;
+
+DefaultStory.storyName = 'Default';