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: fix ConnectPage when a non-EVM account is selected #28436

Merged
merged 6 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions test/jest/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import {
initialState,
} from '../../ui/ducks/send';
import { MetaMaskReduxState } from '../../ui/store/store';
import mockState from '../data/mock-state.json';

export type MockState = typeof mockState;

export const MOCK_DEFAULT_ADDRESS =
'0xd5e099c71b797516c10ed0f0d895f429c2781111';
Expand Down Expand Up @@ -245,3 +248,53 @@ export const getSelectedInternalAccountFromMockState = (
state.metamask.internalAccounts.selectedAccount
];
};

export function overrideAccountsFromMockState<
MockMetaMaskState extends MockState['metamask'],
>(
state: { metamask: MockMetaMaskState },
accounts: InternalAccount[],
selectedAccountId?: string,
): { metamask: MockMetaMaskState } {
// First, re-create the accounts mapping and the currently selected account.
const [{ id: newFirstAccountId }] = accounts;
const newSelectedAccount = selectedAccountId ?? newFirstAccountId ?? '';
const newInternalAccounts = accounts.reduce(
(
acc: MetaMaskReduxState['metamask']['internalAccounts']['accounts'],
account,
) => {
acc[account.id] = account;
return acc;
},
{},
);

// Re-create the keyring mapping too, since some selectors are using their internal
// account list.
const newKeyrings: MetaMaskReduxState['metamask']['keyrings'] = [];
for (const keyring of state.metamask.keyrings) {
const newAccountsForKeyring = [];
for (const account of accounts) {
if (account.metadata.keyring.type === keyring.type) {
newAccountsForKeyring.push(account.address);
}
}
newKeyrings.push({
type: keyring.type,
accounts: newAccountsForKeyring,
});
}

return {
...state,
metamask: {
...state.metamask,
internalAccounts: {
accounts: newInternalAccounts,
selectedAccount: newSelectedAccount,
},
keyrings: newKeyrings,
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
},
};
}
138 changes: 95 additions & 43 deletions ui/pages/permissions-connect/connect-page/connect-page.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,63 @@ import {
EndowmentTypes,
RestrictedMethods,
} from '../../../../shared/constants/permissions';
import { ConnectPage, ConnectPageRequest } from './connect-page';
import { overrideAccountsFromMockState } from '../../../../test/jest/mocks';
import {
MOCK_ACCOUNT_BIP122_P2WPKH,
MOCK_ACCOUNT_EOA,
} from '../../../../test/data/mock-accounts';
import { ConnectPage, ConnectPageProps } from './connect-page';

const mockTestDappUrl = 'https://test.dapp';

const render = (
props: {
request: ConnectPageRequest;
permissionsRequestId: string;
rejectPermissionsRequest: (id: string) => void;
approveConnection: (request: ConnectPageRequest) => void;
activeTabOrigin: string;
} = {
request: {
id: '1',
origin: 'https://test.dapp',
},
permissionsRequestId: '1',
rejectPermissionsRequest: jest.fn(),
approveConnection: jest.fn(),
activeTabOrigin: 'https://test.dapp',
},
state = {},
options: {
props?: ConnectPageProps;
state?: object;
} = {},
) => {
const {
props = {
request: {
id: '1',
origin: mockTestDappUrl,
},
permissionsRequestId: '1',
rejectPermissionsRequest: jest.fn(),
approveConnection: jest.fn(),
activeTabOrigin: mockTestDappUrl,
},
state,
} = options;

const state2 = {
...mockState,
metamask: {
...mockState.metamask,
...state,
permissionHistory: {
mockTestDappUrl: {
eth_accounts: {
accounts: {
'0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc': 1709225290848,
},
},
},
},
},
activeTab: {
origin: mockTestDappUrl,
},
};
console.log(JSON.stringify(state2.metamask.internalAccounts, null, 2));
console.log(JSON.stringify(state2.metamask.keyrings, null, 2));
ccharly marked this conversation as resolved.
Show resolved Hide resolved
const store = configureStore({
...mockState,
metamask: {
...mockState.metamask,
...state,
permissionHistory: {
'https://test.dapp': {
mockTestDappUrl: {
eth_accounts: {
accounts: {
'0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc': 1709225290848,
Expand All @@ -44,7 +73,7 @@ const render = (
},
},
activeTab: {
origin: 'https://test.dapp',
origin: mockTestDappUrl,
},
});
return renderWithProvider(<ConnectPage {...props} />, store);
Expand Down Expand Up @@ -82,33 +111,56 @@ describe('ConnectPage', () => {

it('should render with defaults from the requested permissions', () => {
const { container } = render({
request: {
id: '1',
origin: 'https://test.dapp',
permissions: {
[RestrictedMethods.eth_accounts]: {
caveats: [
{
type: CaveatTypes.restrictReturnedAccounts,
value: ['0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc'],
},
],
},
[EndowmentTypes.permittedChains]: {
caveats: [
{
type: CaveatTypes.restrictNetworkSwitching,
value: ['0x1'],
},
],
props: {
request: {
id: '1',
origin: mockTestDappUrl,
permissions: {
[RestrictedMethods.eth_accounts]: {
caveats: [
{
type: CaveatTypes.restrictReturnedAccounts,
value: ['0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc'],
},
],
},
[EndowmentTypes.permittedChains]: {
caveats: [
{
type: CaveatTypes.restrictNetworkSwitching,
value: ['0x1'],
},
],
},
},
},
permissionsRequestId: '1',
rejectPermissionsRequest: jest.fn(),
approveConnection: jest.fn(),
activeTabOrigin: mockTestDappUrl,
},
permissionsRequestId: '1',
rejectPermissionsRequest: jest.fn(),
approveConnection: jest.fn(),
activeTabOrigin: 'https://test.dapp',
});
expect(container).toMatchSnapshot();
});

it('should render a disabled confirm if current account is a non-EVM account', () => {
// NOTE: We select the non-EVM account by default here!
const mockSelectedAccount = MOCK_ACCOUNT_BIP122_P2WPKH.id;
ccharly marked this conversation as resolved.
Show resolved Hide resolved
const mockAccounts = [MOCK_ACCOUNT_EOA, MOCK_ACCOUNT_BIP122_P2WPKH];
const mockAccountsState = overrideAccountsFromMockState(
mockState,
mockAccounts,
mockSelectedAccount,
);

const { getByText } = render({
state: mockAccountsState.metamask,
});
const confirmButton = getByText('Connect');
const cancelButton = getByText('Cancel');
// The currently selected account is a Bitcoin account, the "connecting account list" would be
// empty by default and thus, we cannot confirm without explictly select an EVM account.
expect(confirmButton).toBeDisabled();
expect(cancelButton).toBeDefined();
});
});
9 changes: 5 additions & 4 deletions ui/pages/permissions-connect/connect-page/connect-page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export type ConnectPageRequest = {
>;
};

type ConnectPageProps = {
export type ConnectPageProps = {
request: ConnectPageRequest;
permissionsRequestId: string;
rejectPermissionsRequest: (id: string) => void;
Expand Down Expand Up @@ -124,10 +124,11 @@ export const ConnectPage: React.FC<ConnectPageProps> = ({
}, [accounts, internalAccounts]);

const currentAccount = useSelector(getSelectedInternalAccount);
const currentAccountAddress = isEvmAccountType(currentAccount.type)
? [currentAccount.address]
: []; // We do not support non-EVM accounts connections
const defaultAccountsAddresses =
requestedAccounts.length > 0
? requestedAccounts
: [currentAccount?.address];
requestedAccounts.length > 0 ? requestedAccounts : currentAccountAddress;
const [selectedAccountAddresses, setSelectedAccountAddresses] = useState(
defaultAccountsAddresses,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ function getDefaultSelectedAccounts(currentAddress, permissionsRequest) {
return new Set(
requestedAccounts
.map((address) => address.toLowerCase())
// We only consider EVM accounts here (used for `eth_requestAccounts` or `eth_accounts`)
.filter(isEthAddress),
);
}

// We only consider EVM accounts here (used for `eth_requestAccounts` or `eth_accounts`)
return new Set(isEthAddress(currentAddress) ? [currentAddress] : []);
}

Expand Down