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

## **Description** This pr filters eth requests that goes to non evm accounts and also disables the selection of non evm accounts in the permission connect page. ## **Related issues** Fixes MetaMask/accounts-planning#462 ## **Manual testing steps** - Affects the accounts list during the dapp connection flow - Connection to dapps with an evm account stays the same. ## **Screenshots/Recordings** Not applicable ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
MetaMask · Jun 21, 2024 · 97edb9a · 97edb9a
1 parent 48bc2f0
commit 97edb9a
Show file tree

Hide file tree

Showing 9 changed files with 207 additions and 34 deletions.
diff --git a/ui/components/app/permission-page-container/permission-page-container.component.js b/ui/components/app/permission-page-container/permission-page-container.component.js
@@ -16,6 +16,7 @@ import { PermissionNames } from '../../../../app/scripts/controllers/permissions
 
 import SnapPrivacyWarning from '../snaps/snap-privacy-warning';
 import { getDedupedSnaps } from '../../../helpers/utils/util';
+import { containsEthPermissionsAndNonEvmAccount } from '../../../helpers/utils/permissions';
 ///: END:ONLY_INCLUDE_IF
 import {
   BackgroundColor,
@@ -242,6 +243,10 @@ export default class PermissionPageContainer extends Component {
             onSubmit={() => this.onSubmit()}
             submitText={this.context.t('confirm')}
             buttonSizeLarge={false}
+            disabled={containsEthPermissionsAndNonEvmAccount(
+              selectedAccounts,
+              requestedPermissions,
+            )}
           />
         </Box>
       </>

diff --git a/ui/components/ui/account-list/account-list.js b/ui/components/ui/account-list/account-list.js
@@ -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';
@@ -47,6 +48,13 @@ const AccountList = ({
 
   const [firstSelectedAccount] = selectedAccounts;
 
+  const handleEvmAccountClick = (account) => {
+    if (!isEvmAccountType(account.type)) {
+      return;
+    }
+    handleAccountClick(account.address);
+  };
+
   const Header = () => {
     let checked = false;
     let isIndeterminate = false;
@@ -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)}
                 className="choose-account-list__account"
                 ref={
                   isSelectedAccount && address === firstSelectedAccount
@@ -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>

diff --git a/ui/components/ui/account-list/account-list.test.js b/ui/components/ui/account-list/account-list.test.js
@@ -1,41 +1,60 @@
 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 defaultAddress = '0x64a845a5b02460acf8a3d84503b0d68d028b4bb4';
+
+const defaultArgs = {
+  accounts: [
+    {
+      address: defaultAddress,
+      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([defaultAddress]),
+  addressLastConnectedMap: {
+    [defaultAddress]: '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);
 };
 
@@ -54,4 +73,41 @@ describe('AccountList', () => {
     render();
     expect(screen.getByText('ETH')).toBeInTheDocument();
   });
+
+  it('disables the handleAccountClick action for non-EVM accounts', () => {
+    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('enables the handleAccountClick action for EVM accounts', () => {
+    const { container } = render();
+    const accountButton = container.querySelector(
+      '[data-testid="choose-account-list-0"]',
+    );
+
+    fireEvent.click(accountButton);
+    expect(mockHandleAccountClick).toHaveBeenCalled();
+  });
 });
diff --git a/ui/helpers/utils/permissions.test.ts b/ui/helpers/utils/permissions.test.ts
@@ -0,0 +1,66 @@
+import {
+  BtcAccountType,
+  BtcMethod,
+  InternalAccount,
+} from '@metamask/keyring-api';
+import { createMockInternalAccount } from '../../../test/jest/mocks';
+import { containsEthPermissionsAndNonEvmAccount } from './permissions';
+
+const mockAccount = createMockInternalAccount();
+const mockNonEvmAccount = {
+  ...mockAccount,
+  id: '4b94987c-165c-4287-bbc6-bee9c440e82a',
+  type: BtcAccountType.P2wpkh,
+  methods: [BtcMethod.SendMany],
+  address: 'bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq',
+};
+
+describe('containsEthPermissionsAndNonEvmAccount', () => {
+  it('return false if accounts array is empty', () => {
+    const accounts: InternalAccount[] = [];
+    const permissions = { eth_accounts: '' };
+
+    const result = containsEthPermissionsAndNonEvmAccount(
+      accounts,
+      permissions,
+    );
+
+    expect(result).toBe(false);
+  });
+
+  it('return false if accounts array contains only EVM accounts', () => {
+    const accounts: InternalAccount[] = [mockAccount, mockAccount];
+    const permissions = { eth_accounts: '' };
+
+    const result = containsEthPermissionsAndNonEvmAccount(
+      accounts,
+      permissions,
+    );
+
+    expect(result).toBe(false);
+  });
+
+  it('return false if permissions object does not contain eth_accounts permission', () => {
+    const accounts: InternalAccount[] = [mockAccount, mockNonEvmAccount];
+    const permissions = { some_other_permission: '' };
+
+    const result = containsEthPermissionsAndNonEvmAccount(
+      accounts,
+      permissions,
+    );
+
+    expect(result).toBe(false);
+  });
+
+  it('return true if accounts array contains non-EVM account and permissions object contains eth_accounts permission', () => {
+    const accounts: InternalAccount[] = [mockAccount, mockNonEvmAccount];
+    const permissions = { eth_accounts: '' };
+
+    const result = containsEthPermissionsAndNonEvmAccount(
+      accounts,
+      permissions,
+    );
+
+    expect(result).toBe(true);
+  });
+});
diff --git a/ui/helpers/utils/permissions.ts b/ui/helpers/utils/permissions.ts
@@ -0,0 +1,17 @@
+import { InternalAccount, isEvmAccountType } from '@metamask/keyring-api';
+import { RestrictedEthMethods } from '../../../shared/constants/permissions';
+
+export const containsEthPermissionsAndNonEvmAccount = (
+  accounts: InternalAccount[],
+  permissions: { [key: string]: string },
+) => {
+  const restrictedEthMethods = Object.keys(RestrictedEthMethods);
+  const containsEthPermissions = Object.keys(permissions).some((permission) =>
+    restrictedEthMethods.includes(permission),
+  );
+  const containsNonEvmAccount = accounts.some(
+    (account) => !isEvmAccountType(account.type),
+  );
+
+  return containsEthPermissions && containsNonEvmAccount;
+};
diff --git a/ui/pages/permissions-connect/choose-account/choose-account.js b/ui/pages/permissions-connect/choose-account/choose-account.js
@@ -1,6 +1,7 @@
 import PropTypes from 'prop-types';
 import React, { useState } from 'react';
 import { SubjectType } from '@metamask/permission-controller';
+import { isEvmAccountType } from '@metamask/keyring-api';
 import { useI18nContext } from '../../../hooks/useI18nContext';
 import PermissionsConnectFooter from '../../../components/app/permissions-connect-footer';
 import AccountList from '../../../components/ui/account-list';
@@ -30,6 +31,9 @@ const ChooseAccount = ({
   const [selectedAccounts, setSelectedAccounts] = useState(
     selectedAccountAddresses,
   );
+  const evmAccounts = accounts.filter((account) =>
+    isEvmAccountType(account.type),
+  );
   const t = useI18nContext();
 
   const handleAccountClick = (address) => {
@@ -44,7 +48,7 @@ const ChooseAccount = ({
 
   const selectAll = () => {
     const newSelectedAccounts = new Set(
-      accounts.map((account) => account.address),
+      evmAccounts.map((account) => account.address),
     );
     setSelectedAccounts(newSelectedAccounts);
   };
@@ -54,9 +58,13 @@ const ChooseAccount = ({
   };
 
   const allAreSelected = () => {
-    return accounts.length === selectedAccounts.size;
+    return evmAccounts.length === selectedAccounts.size;
   };
 
+  // If lengths are different, this means `accounts` holds some non-EVM accounts
+  const hasNonEvmAccounts =
+    Object.keys(selectedAccountAddresses).length > evmAccounts.length;
+
   const getHeaderText = () => {
     if (accounts.length === 0) {
       return t('connectAccountOrCreate');
@@ -123,7 +131,7 @@ const ChooseAccount = ({
           cancelText={t('cancel')}
           onSubmit={() => selectAccounts(selectedAccounts)}
           submitText={t('next')}
-          disabled={selectedAccounts.size === 0}
+          disabled={hasNonEvmAccounts || selectedAccounts.size === 0}
         />
       </Box>
     </>

diff --git a/ui/pages/permissions-connect/permissions-connect.component.js b/ui/pages/permissions-connect/permissions-connect.component.js
@@ -5,6 +5,7 @@ import { Switch, Route } from 'react-router-dom';
 import { ethErrors, serializeError } from 'eth-rpc-errors';
 import { SubjectType } from '@metamask/permission-controller';
 ///: END:ONLY_INCLUDE_IF
+import { isEthAddress } from '../../../app/scripts/lib/multichain/address';
 import { MILLISECOND } from '../../../shared/constants/time';
 import { DEFAULT_ROUTE } from '../../helpers/constants/routes';
 import PermissionPageContainer from '../../components/app/permission-page-container';
@@ -36,10 +37,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 {

diff --git a/ui/selectors/institutional/selectors.js b/ui/selectors/institutional/selectors.js
@@ -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;
@@ -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]

diff --git a/ui/selectors/permissions.js b/ui/selectors/permissions.js
@@ -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';
@@ -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: {
@@ -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: {