Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter eth request to non-EVM accounts #25003

Merged
merged 12 commits into from
Jun 21, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@ import {
} from '@metamask/snaps-rpc-methods';
///: END:ONLY_INCLUDE_IF
import { SubjectType } from '@metamask/permission-controller';
import { isEvmAccountType } from '@metamask/keyring-api';
import { MetaMetricsEventCategory } from '../../../../shared/constants/metametrics';
import { PageContainerFooter } from '../../ui/page-container';
import PermissionsConnectFooter from '../permissions-connect-footer';
///: BEGIN:ONLY_INCLUDE_IF(snaps)
import { RestrictedMethods } from '../../../../shared/constants/permissions';
import {
RestrictedEthMethods,
RestrictedMethods,
} from '../../../../shared/constants/permissions';
import { PermissionNames } from '../../../../app/scripts/controllers/permissions';

import SnapPrivacyWarning from '../snaps/snap-privacy-warning';
Expand Down Expand Up @@ -199,6 +203,17 @@ export default class PermissionPageContainer extends Component {
};
///: END:ONLY_INCLUDE_IF

const containsEthPermissionsAndNonEVMAccount = (accounts, permissions) => {
const containsEthPermissions = Object.keys(permissions).some(
(permission) => Object.keys(RestrictedEthMethods).includes(permission),
);
const containsNonEVMAccount = accounts.some(
(account) => !isEvmAccountType(account.type),
);

return containsEthPermissions && containsNonEVMAccount;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an utility method, I don't see a reason for it to be inside a component unless I'm missing something. Also, unit tests.


const footerLeftActionText = requestedPermissions[
PermissionNames.permittedChains
]
Expand Down Expand Up @@ -242,6 +257,10 @@ export default class PermissionPageContainer extends Component {
onSubmit={() => this.onSubmit()}
submitText={this.context.t('confirm')}
buttonSizeLarge={false}
disabled={containsEthPermissionsAndNonEVMAccount(
selectedAccounts,
requestedPermissions,
)}
/>
</Box>
</>
Expand Down
16 changes: 14 additions & 2 deletions ui/components/ui/account-list/account-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { memo, useLayoutEffect, useRef } from 'react';
import PropTypes from 'prop-types';
import classnames from 'classnames';
import { isEqual } from 'lodash';
import { isEvmAccountType } from '@metamask/keyring-api';
import { useI18nContext } from '../../../hooks/useI18nContext';
import Identicon from '../identicon';
import UserPreferencedCurrencyDisplay from '../../app/user-preferenced-currency-display';
Expand Down Expand Up @@ -47,6 +48,13 @@ const AccountList = ({

const [firstSelectedAccount] = selectedAccounts;

const handleEVMAccountClick = (account) => {
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
if (!isEvmAccountType(account.type)) {
return;
}
handleAccountClick(account.address);
};

const Header = () => {
let checked = false;
let isIndeterminate = false;
Expand Down Expand Up @@ -128,7 +136,8 @@ const AccountList = ({
display={Display.Flex}
width={BlockSize.Full}
key={`choose-account-list-${index}`}
onClick={() => handleAccountClick(address)}
data-testid={`choose-account-list-${index}`}
onClick={() => handleEVMAccountClick(account)}
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
className="choose-account-list__account"
ref={
isSelectedAccount && address === firstSelectedAccount
Expand All @@ -146,7 +155,10 @@ const AccountList = ({
width={BlockSize.Full}
alignItems={AlignItems.center}
>
<Checkbox isChecked={isSelectedAccount} />
<Checkbox
isChecked={isSelectedAccount}
isDisabled={!isEvmAccountType(account.type)}
/>
<Box marginLeft={2}>
<Identicon diameter={34} address={address} />
</Box>
Expand Down
107 changes: 81 additions & 26 deletions ui/components/ui/account-list/account-list.test.js
Original file line number Diff line number Diff line change
@@ -1,41 +1,59 @@
import React from 'react';
import { screen } from '@testing-library/react';
import { screen, fireEvent } from '@testing-library/react';
import {
BtcAccountType,
BtcMethod,
EthAccountType,
} from '@metamask/keyring-api';
import { renderWithProvider } from '../../../../test/jest';
import configureStore from '../../../store/store';
import mockState from '../../../../test/data/mock-state.json';
import { ETH_EOA_METHODS } from '../../../../shared/constants/eth-methods';
import AccountList from './account-list';

const render = () => {
const mockHandleAccountClick = jest.fn();

const defaultArgs = {
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
accounts: [
{
address: '0x64a845a5b02460acf8a3d84503b0d68d028b4bb4',
addressLabel: 'Account 1',
label: 'Account 1',
balance: '87a73149c048545a3fe58',
id: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3',
metadata: {
name: 'Account 1',
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: ETH_EOA_METHODS,
type: EthAccountType.Eoa,
has: () => {
/** nothing to do */
},
},
],
selectedAccounts: new Set(['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4']),
addressLastConnectedMap: {
'0x64a845a5b02460acf8a3d84503b0d68d028b4bb4': 'Feb-22-2022',
},
allAreSelected: () => true,
nativeCurrency: 'USD',
selectNewAccountViaModal: jest.fn(),
deselectAll: jest.fn(),
selectAll: jest.fn(),
handleAccountClick: mockHandleAccountClick,
};

const render = (args = defaultArgs) => {
const store = configureStore({
metamask: {
...mockState.metamask,
},
});

const args = {
accounts: [
{
address: '0x64a845a5b02460acf8a3d84503b0d68d028b4bb4',
addressLabel: 'Account 1',
label: 'Account 1',
lastConnectedDate: 'Feb-22-2022',
balance: '87a73149c048545a3fe58',
has: () => {
/** nothing to do */
},
},
],
selectedAccounts: new Set(['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4']),
addressLastConnectedMap: {
'0x64a845a5b02460acf8a3d84503b0d68d028b4bb4': 'Feb-22-2022',
},
allAreSelected: () => true,
nativeCurrency: 'USD',
selectNewAccountViaModal: jest.fn(),
deselectAll: jest.fn(),
selectAll: jest.fn(),
handleAccountClick: jest.fn(),
};
return renderWithProvider(<AccountList {...args} />, store);
};

Expand All @@ -54,4 +72,41 @@ describe('AccountList', () => {
render();
expect(screen.getByText('ETH')).toBeInTheDocument();
});

it('handleAccountClick is disabled for non-EVM accounts', () => {
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
const { container } = render({
...defaultArgs,
accounts: [
{
id: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3',
metadata: {
name: 'Btc account',
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: [BtcMethod.SendMany],
type: BtcAccountType.P2wpkh,
address: 'bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq',
},
],
});
const accountButton = container.querySelector(
'[data-testid="choose-account-list-0"]',
);

fireEvent.click(accountButton);
expect(mockHandleAccountClick).not.toHaveBeenCalled();
});

it('handleAccountClick is enabled for EVM accounts', () => {
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
const { container } = render();
const accountButton = container.querySelector(
'[data-testid="choose-account-list-0"]',
);

fireEvent.click(accountButton);
expect(mockHandleAccountClick).toHaveBeenCalled();
});
});
19 changes: 16 additions & 3 deletions ui/pages/permissions-connect/choose-account/choose-account.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import PropTypes from 'prop-types';
import React, { useState } from 'react';
import { SubjectType } from '@metamask/permission-controller';
import { isEvmAccountType } from '@metamask/keyring-api';
import { isEthAddress } from '../../../../app/scripts/lib/multichain/address';
import { useI18nContext } from '../../../hooks/useI18nContext';
import PermissionsConnectFooter from '../../../components/app/permissions-connect-footer';
import AccountList from '../../../components/ui/account-list';
Expand Down Expand Up @@ -44,7 +46,9 @@ const ChooseAccount = ({

const selectAll = () => {
const newSelectedAccounts = new Set(
accounts.map((account) => account.address),
accounts
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
.filter((account) => isEvmAccountType(account.type))
.map((account) => account.address),
);
setSelectedAccounts(newSelectedAccounts);
};
Expand All @@ -54,7 +58,16 @@ const ChooseAccount = ({
};

const allAreSelected = () => {
return accounts.length === selectedAccounts.size;
return (
accounts.filter((account) => isEvmAccountType(account.type)).length ===
selectedAccounts.size
);
};

const hasNonEVMAccounts = () => {
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
return Object.keys(selectedAccountAddresses).some(
(address) => !isEthAddress(address),
);
};

const getHeaderText = () => {
Expand Down Expand Up @@ -123,7 +136,7 @@ const ChooseAccount = ({
cancelText={t('cancel')}
onSubmit={() => selectAccounts(selectedAccounts)}
submitText={t('next')}
disabled={selectedAccounts.size === 0}
disabled={hasNonEVMAccounts() || selectedAccounts.size === 0}
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
/>
</Box>
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ethErrors, serializeError } from 'eth-rpc-errors';
import { SubjectType } from '@metamask/permission-controller';
///: END:ONLY_INCLUDE_IF
import { getEnvironmentType } from '../../../app/scripts/lib/util';
import { isEthAddress } from '../../../app/scripts/lib/multichain/address';
import { ENVIRONMENT_TYPE_NOTIFICATION } from '../../../shared/constants/app';
import { MILLISECOND } from '../../../shared/constants/time';
import { DEFAULT_ROUTE } from '../../helpers/constants/routes';
Expand Down Expand Up @@ -38,10 +39,14 @@ function getDefaultSelectedAccounts(currentAddress, permissionsRequest) {
)?.value;

if (requestedAccounts) {
return new Set(requestedAccounts.map((address) => address.toLowerCase()));
return new Set(
requestedAccounts
.map((address) => address.toLowerCase())
.filter(isEthAddress),
);
}

return new Set([currentAddress]);
return new Set(isEthAddress(currentAddress) ? [currentAddress] : []);
}

export default class PermissionConnect extends Component {
Expand Down
3 changes: 2 additions & 1 deletion ui/selectors/institutional/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { toChecksumAddress } from 'ethereumjs-util';
import { getAccountType, getSelectedInternalAccount } from '../selectors';
import { getProviderConfig } from '../../ducks/metamask/metamask';
import { hexToDecimal } from '../../../shared/modules/conversion.utils';
import { normalizeSafeAddress } from '../../../app/scripts/lib/multichain/address';

export function getWaitForConfirmDeepLinkDialog(state) {
return state.metamask.waitForConfirmDeepLinkDialog;
Expand Down Expand Up @@ -36,7 +37,7 @@ export function getConfiguredCustodians(state) {
export function getCustodianIconForAddress(state, address) {
let custodianIcon;

const checksummedAddress = address && toChecksumAddress(address);
const checksummedAddress = address && normalizeSafeAddress(address);
if (
checksummedAddress &&
state.metamask.custodyAccountDetails?.[checksummedAddress]
Expand Down
3 changes: 3 additions & 0 deletions ui/selectors/permissions.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ApprovalType } from '@metamask/controller-utils';
///: BEGIN:ONLY_INCLUDE_IF(snaps)
import { WALLET_SNAP_PERMISSION_KEY } from '@metamask/snaps-rpc-methods';
///: END:ONLY_INCLUDE_IF
import { isEvmAccountType } from '@metamask/keyring-api';
import { CaveatTypes } from '../../shared/constants/permissions';
import { getApprovalRequestsByType } from './approvals';
import { createDeepEqualSelector } from './util';
Expand Down Expand Up @@ -311,6 +312,7 @@ export function getOrderedConnectedAccountsForActiveTab(state) {

return orderedAccounts
.filter((account) => connectedAccounts.includes(account.address))
.filter((account) => isEvmAccountType(account.type))
.map((account) => ({
...account,
metadata: {
Expand Down Expand Up @@ -349,6 +351,7 @@ export function getOrderedConnectedAccountsForConnectedDapp(state, activeTab) {

return orderedAccounts
.filter((account) => connectedAccounts.includes(account.address))
.filter((account) => isEvmAccountType(account.type))
.map((account) => ({
...account,
metadata: {
Expand Down