Skip to content

Commit

Permalink
Upgrading the Import Account modal (#17763)
Browse files Browse the repository at this point in the history
Co-authored-by: georgewrmarshall <george.marshall@consensys.net>
Co-authored-by: NidhiKJha <nidhi.kumari@consensys.net>
Co-authored-by: montelaidev <monte.lai@consensys.net>
  • Loading branch information
4 people authored Mar 6, 2023
1 parent 01abfb3 commit 694773f
Show file tree
Hide file tree
Showing 27 changed files with 652 additions and 747 deletions.
17 changes: 16 additions & 1 deletion app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 35 additions & 10 deletions app/scripts/account-import-strategies/index.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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;
Expand Down
11 changes: 5 additions & 6 deletions app/scripts/lib/account-tracker.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { strict as assert } from 'assert';
import EventEmitter from 'events';

import { SINGLE_CALL_BALANCES_ADDRESS } from '../constants/contracts';
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -137,7 +136,7 @@ describe('Account Tracker', () => {
currentBlockGasLimit: '',
};

assert.deepEqual(newAccounts, expectedAccounts);
expect(newAccounts).toStrictEqual(expectedAccounts);
});
});

Expand All @@ -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 () => {
Expand Down Expand Up @@ -192,7 +191,7 @@ describe('Account Tracker', () => {
currentBlockGasLimit: '',
};

assert.deepEqual(newAccounts, expectedAccounts);
expect(newAccounts).toStrictEqual(expectedAccounts);
});
});
});
2 changes: 1 addition & 1 deletion test/e2e/tests/add-account.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
12 changes: 6 additions & 6 deletions test/e2e/tests/from-import-ui.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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"]');
Expand All @@ -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(
Expand Down Expand Up @@ -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',
});
},
Expand Down
2 changes: 1 addition & 1 deletion ui/components/app/asset-list-item/asset-list-item.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ export const FormTextField = ({
/>
{helpText && (
<HelpText
error={error}
severity={error && SEVERITIES.DANGER}
marginTop={1}
{...helpTextProps}
Expand Down
2 changes: 1 addition & 1 deletion ui/components/component-library/help-text/help-text.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
2 changes: 1 addition & 1 deletion ui/components/component-library/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
2 changes: 1 addition & 1 deletion ui/components/component-library/text/index.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export { Text } from './text';
export { TEXT_DIRECTIONS } from './text.constants';
export { TEXT_DIRECTIONS, INVISIBLE_CHARACTER } from './text.constants';
6 changes: 6 additions & 0 deletions ui/components/component-library/text/text.constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
4 changes: 3 additions & 1 deletion ui/helpers/utils/accounts.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { INVISIBLE_CHARACTER } from '../../components/component-library';

export function getAccountNameErrorMessage(
accounts,
context,
Expand Down Expand Up @@ -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) {
Expand Down
61 changes: 29 additions & 32 deletions ui/pages/create-account/create-account.component.js
Original file line number Diff line number Diff line change
@@ -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>
);
}
48 changes: 48 additions & 0 deletions ui/pages/create-account/import-account/bottom-buttons.js
Original file line number Diff line number Diff line change
@@ -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>
);
}
18 changes: 18 additions & 0 deletions ui/pages/create-account/import-account/bottom-buttons.stories.js
Original file line number Diff line number Diff line change
@@ -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';
Loading

0 comments on commit 694773f

Please sign in to comment.