diff --git a/.github/workflows/e2e-dapp-interactions-stress.yml b/.github/workflows/e2e-dapp-interactions-stress.yml new file mode 100644 index 000000000000..3950d8a36fde --- /dev/null +++ b/.github/workflows/e2e-dapp-interactions-stress.yml @@ -0,0 +1,90 @@ +name: E2E Dapp Interactions Stress + +on: + pull_request: + paths: + - '.github/workflows/e2e-dapp-interactions-stress.yml' + - 'test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts' + - 'test/e2e/run-e2e-test.js' + workflow_dispatch: + inputs: + attempts: + description: Number of repeated attempts for this spec + required: false + default: '30' + +jobs: + test-e2e-dapp-interactions-stress: + name: test-e2e-dapp-interactions-stress + runs-on: ubuntu-latest + timeout-minutes: 80 + container: + image: ghcr.io/metamask/metamask-extension-e2e-image:v24.13.0 + credentials: + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + INFURA_PROJECT_ID: ${{ secrets.INFURA_PROJECT_ID }} + SENTRY_DSN_PERFORMANCE: ${{ vars.SENTRY_DSN_PERFORMANCE }} + FORCE_COLOR: '1' + ATTEMPTS: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.attempts || '30' }} + RUN_ID: ${{ github.run_id }} + PR_NUMBER: ${{ github.event.pull_request.number || '' }} + + steps: + - name: Checkout and setup environment + uses: MetaMask/action-checkout-and-setup@v3 + with: + is-high-risk-environment: false + skip-allow-scripts: true + + - name: Restore .metamask folder + id: restore-metamask + uses: actions/cache/restore@v5 + with: + path: .metamask + key: .metamask-${{ hashFiles('yarn.lock') }} + fail-on-cache-miss: false + + - name: Install anvil if cache missed + if: ${{ steps.restore-metamask.outputs.cache-hit != 'true' }} + run: yarn mm-foundryup + + - name: Build test artifacts + run: yarn build:test + + - name: Configure Xvfb + run: Xvfb -ac :99 -screen 0 1280x1024x16 & + + - name: Run dapp interactions stress test + env: + PROPERTIES: JOB_NAME:test-e2e-dapp-interactions-stress,RUN_ID:${{ env.RUN_ID }},PR_NUMBER:${{ env.PR_NUMBER }} + shell: bash + run: | + set -euo pipefail + + if ! [[ "${ATTEMPTS}" =~ ^[0-9]+$ ]] || [ "${ATTEMPTS}" -lt 1 ]; then + echo "Invalid ATTEMPTS value: ${ATTEMPTS}" + exit 1 + fi + + RETRIES=$((ATTEMPTS - 1)) + + echo "Running test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts ${ATTEMPTS} times" + + yarn test:e2e:single \ + test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts \ + --browser chrome \ + --stop-after-one-failure \ + --retries "${RETRIES}" \ + --debug=false + + - name: Upload test artifacts + if: ${{ !cancelled() }} + uses: actions/upload-artifact@v6 + with: + name: test-e2e-dapp-interactions-stress + path: | + ./test-artifacts + ./test/test-results/e2e diff --git a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts index 4e63ad34f5c5..6afc0523d8a5 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts +++ b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts @@ -3,6 +3,10 @@ import type { JsonRpcRequest, PendingJsonRpcResponse, } from '@metamask/utils'; +import { + Caip25CaveatType, + Caip25EndowmentPermissionName, +} from '@metamask/chain-agnostic-permission'; import * as Util from '../../util'; import requestEthereumAccounts from './request-accounts'; @@ -50,7 +54,7 @@ const createMockedHandler = () => { const getCaip25PermissionFromLegacyPermissionsForOrigin = jest .fn() .mockResolvedValue({}); - const requestPermissionsForOrigin = jest.fn().mockReturnValue({}); + const requestPermissionsForOrigin = jest.fn().mockResolvedValue([{}]); const response: PendingJsonRpcResponse = { jsonrpc: '2.0' as const, id: 0, @@ -150,6 +154,123 @@ describe('requestEthereumAccountsHandler', () => { expect(getAccounts).toHaveBeenCalledTimes(2); }); + it('briefly retries account lookup if approval result has not propagated yet', async () => { + const { handler, getAccounts, response } = createMockedHandler(); + getAccounts + .mockReturnValueOnce([]) + .mockReturnValueOnce([]) + .mockReturnValueOnce(['0xdead']); + + await handler(baseRequest); + + expect(response.result).toStrictEqual(['0xdead']); + expect(getAccounts).toHaveBeenCalledTimes(3); + }); + + it('falls back to granted CAIP-25 accounts when account lookup remains empty', async () => { + const { handler, getAccounts, response, requestPermissionsForOrigin } = + createMockedHandler(); + + getAccounts.mockReturnValue([]); + requestPermissionsForOrigin.mockResolvedValue([ + { + [Caip25EndowmentPermissionName]: { + caveats: [ + { + type: Caip25CaveatType, + value: { + isMultichainOrigin: true, + requiredScopes: {}, + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], + }, + }, + sessionProperties: {}, + }, + }, + ], + }, + }, + ]); + + await handler(baseRequest); + + expect(response.result).toStrictEqual(['0xdead']); + }); + + it('shares the same in-flight request for concurrent requests from the same origin', async () => { + const { + next, + getAccounts, + sendMetrics, + metamaskState, + getCaip25PermissionFromLegacyPermissionsForOrigin, + requestPermissionsForOrigin, + } = createMockedHandler(); + + let resolveApprovalRequest: (() => void) | undefined; + const approvalRequestPromise = new Promise((resolve) => { + resolveApprovalRequest = resolve; + }); + requestPermissionsForOrigin.mockImplementation(async () => { + await approvalRequestPromise; + return [{}]; + }); + getAccounts.mockReturnValueOnce([]).mockReturnValueOnce(['0xdead']); + + const firstResponse: PendingJsonRpcResponse = { + jsonrpc: '2.0', + id: 1, + result: undefined, + }; + const secondResponse: PendingJsonRpcResponse = { + jsonrpc: '2.0', + id: 2, + result: undefined, + }; + const firstEnd = jest.fn(); + const secondEnd = jest.fn(); + + const firstRequestPromise = requestEthereumAccounts.implementation( + { ...baseRequest, id: 1 }, + firstResponse, + next, + firstEnd, + { + getAccounts, + sendMetrics, + metamaskState, + getCaip25PermissionFromLegacyPermissionsForOrigin, + requestPermissionsForOrigin, + }, + ); + const secondRequestPromise = requestEthereumAccounts.implementation( + { ...baseRequest, id: 2 }, + secondResponse, + next, + secondEnd, + { + getAccounts, + sendMetrics, + metamaskState, + getCaip25PermissionFromLegacyPermissionsForOrigin, + requestPermissionsForOrigin, + }, + ); + + expect(requestPermissionsForOrigin).toHaveBeenCalledTimes(1); + + resolveApprovalRequest?.(); + await Promise.all([firstRequestPromise, secondRequestPromise]); + + expect(firstResponse.result).toStrictEqual(['0xdead']); + expect(secondResponse.result).toStrictEqual(['0xdead']); + expect(getAccounts).toHaveBeenCalledTimes(2); + expect(firstEnd).toHaveBeenCalled(); + expect(secondEnd).toHaveBeenCalled(); + }); + it('emits the dapp viewed metrics event when shouldEmitDappViewedEvent returns true', async () => { const { handler, getAccounts, sendMetrics } = createMockedHandler(); getAccounts diff --git a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts index 7f5750bef170..f68989dcbad6 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts +++ b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts @@ -8,7 +8,11 @@ import type { JsonRpcEngineNextCallback, } from '@metamask/json-rpc-engine'; import type { OriginString } from '@metamask/permission-controller'; -import { rpcErrors } from '@metamask/rpc-errors'; +import { + Caip25CaveatType, + Caip25EndowmentPermissionName, + getEthAccounts, +} from '@metamask/chain-agnostic-permission'; import { MESSAGE_TYPE } from '../../../../../shared/constants/app'; import type { FlattenedBackgroundStateProxy } from '../../../../../shared/types'; @@ -19,6 +23,7 @@ import { import { shouldEmitDappViewedEvent } from '../../util'; import type { GetAccounts, + GrantedPermissions, HandlerWrapper, SendMetrics, GetCaip25PermissionFromLegacyPermissionsForOrigin, @@ -67,8 +72,44 @@ const requestEthereumAccounts = { } satisfies RequestEthereumAccountsConstraint; export default requestEthereumAccounts; -// Used to rate-limit pending requests to one per origin -const locks = new Set(); +// Tracks active eth_requestAccounts requests by origin so concurrent calls +// share the same in-flight request instead of competing for approvals. +const pendingRequests = new Map>(); +const POST_APPROVAL_ACCOUNT_POLL_INTERVAL_MS = 50; +const POST_APPROVAL_ACCOUNT_POLL_TIMEOUT_MS = 1000; + +function hasCaip25Scopes( + value: unknown, +): value is Parameters[0] { + return ( + value !== null && + typeof value === 'object' && + ('requiredScopes' in value || 'optionalScopes' in value) + ); +} + +function getEthAccountsFromGrantedPermissions( + grantedPermissions: GrantedPermissions, +): string[] { + const caip25Permission = grantedPermissions[Caip25EndowmentPermissionName]; + if (!caip25Permission) { + return []; + } + + const caip25CaveatValue = caip25Permission.caveats?.find( + ({ type }) => type === Caip25CaveatType, + )?.value; + + if (!caip25CaveatValue) { + return []; + } + + if (!hasCaip25Scopes(caip25CaveatValue)) { + return []; + } + + return getEthAccounts(caip25CaveatValue); +} /** * This method attempts to retrieve the Ethereum accounts available to the @@ -104,71 +145,90 @@ async function requestEthereumAccountsHandler< }: RequestEthereumAccountsOptions, ): Promise { const { origin } = req ?? {}; - if (locks.has(origin)) { - res.error = rpcErrors.resourceUnavailable( - `Already processing ${MESSAGE_TYPE.ETH_REQUEST_ACCOUNTS}. Please wait.`, - ); - return end(); - } - let ethAccounts = getAccounts(origin); - if (ethAccounts.length > 0) { - // We wait for the extension to unlock in this case only, because permission - // requests are handled when the extension is unlocked, regardless of the - // lock state when they were received. + const inFlightRequest = pendingRequests.get(origin); + if (inFlightRequest) { try { - locks.add(origin); - res.result = ethAccounts; - end(); + res.result = await inFlightRequest; + return end(); } catch (error) { - end(error); - } finally { - locks.delete(origin); + return end(error); } - return undefined; } - try { - const caip25Permission = - getCaip25PermissionFromLegacyPermissionsForOrigin(); - await requestPermissionsForOrigin(caip25Permission); - } catch (error) { - return end(error); - } + const requestPromise = (async () => { + let ethAccounts = getAccounts(origin); + if (ethAccounts.length === 0) { + const caip25Permission = + getCaip25PermissionFromLegacyPermissionsForOrigin(); + const [grantedPermissions] = + await requestPermissionsForOrigin(caip25Permission); + + // We cannot derive ethAccounts directly from the CAIP-25 permission + // because the accounts will not be in order of lastSelected + ethAccounts = getAccounts(origin); + + // In some flows, approval resolves before account permissions are + // observable via getAccounts(origin). Retry briefly to avoid returning + // a transient empty result to the requesting dapp. + if (ethAccounts.length === 0) { + const timeoutAt = Date.now() + POST_APPROVAL_ACCOUNT_POLL_TIMEOUT_MS; + while (ethAccounts.length === 0 && Date.now() < timeoutAt) { + await new Promise((resolve) => + setTimeout(resolve, POST_APPROVAL_ACCOUNT_POLL_INTERVAL_MS), + ); + ethAccounts = getAccounts(origin); + } + } - // We cannot derive ethAccounts directly from the CAIP-25 permission - // because the accounts will not be in order of lastSelected - ethAccounts = getAccounts(origin); - - // first time connection to dapp will lead to no log in the permissionHistory - // and if user has connected to dapp before, the dapp origin will be included in the permissionHistory state - // we will leverage that to identify `is_first_visit` for metrics - if (shouldEmitDappViewedEvent(metamaskState.metaMetricsId)) { - const isFirstVisit = !Object.keys(metamaskState.permissionHistory).includes( - origin, - ); - sendMetrics( - { - event: MetaMetricsEventName.DappViewed, - category: MetaMetricsEventCategory.InpageProvider, - referrer: { - url: origin, + // If state propagation still has not caught up, fall back to the + // accounts in the granted CAIP-25 permission so eth_requestAccounts + // does not return a transient empty result. + if (ethAccounts.length === 0) { + ethAccounts = getEthAccountsFromGrantedPermissions(grantedPermissions); + } + } + + // first time connection to dapp will lead to no log in the permissionHistory + // and if user has connected to dapp before, the dapp origin will be included in the permissionHistory state + // we will leverage that to identify `is_first_visit` for metrics + if (shouldEmitDappViewedEvent(metamaskState.metaMetricsId)) { + const isFirstVisit = !Object.keys( + metamaskState.permissionHistory, + ).includes(origin); + sendMetrics( + { + event: MetaMetricsEventName.DappViewed, + category: MetaMetricsEventCategory.InpageProvider, + referrer: { + url: origin, + }, + properties: { + // eslint-disable-next-line @typescript-eslint/naming-convention + is_first_visit: isFirstVisit, + // eslint-disable-next-line @typescript-eslint/naming-convention + number_of_accounts: Object.keys(metamaskState.identities).length, + // eslint-disable-next-line @typescript-eslint/naming-convention + number_of_accounts_connected: ethAccounts.length, + }, }, - properties: { - // eslint-disable-next-line @typescript-eslint/naming-convention - is_first_visit: isFirstVisit, - // eslint-disable-next-line @typescript-eslint/naming-convention - number_of_accounts: Object.keys(metamaskState.identities).length, - // eslint-disable-next-line @typescript-eslint/naming-convention - number_of_accounts_connected: ethAccounts.length, + { + excludeMetaMetricsId: true, }, - }, - { - excludeMetaMetricsId: true, - }, - ); - } + ); + } - res.result = ethAccounts; - return end(); + return ethAccounts; + })(); + + pendingRequests.set(origin, requestPromise); + + try { + res.result = await requestPromise; + return end(); + } catch (error) { + return end(error); + } finally { + pendingRequests.delete(origin); + } } diff --git a/test/e2e/page-objects/pages/header-navbar.ts b/test/e2e/page-objects/pages/header-navbar.ts index 08aafd783cb6..68eb42e9ca0a 100644 --- a/test/e2e/page-objects/pages/header-navbar.ts +++ b/test/e2e/page-objects/pages/header-navbar.ts @@ -187,10 +187,25 @@ class HeaderNavbar { async checkNotificationCountInMenuOption(count: number): Promise { await this.mouseClickOnThreeDotMenu(); - await this.driver.findElement({ - css: this.notificationCountOption, - text: count.toString(), - }); + await this.driver.waitForSelector(this.notificationCountOption); + const notificationCountElement = await this.driver.findElement( + this.notificationCountOption, + ); + const notificationCountText = + (await notificationCountElement.getAttribute('textContent'))?.trim() ?? + ''; + const notificationCountValue = Number.parseInt(notificationCountText, 10); + + assert.equal( + Number.isNaN(notificationCountValue), + false, + `Expected numeric notification count, got "${notificationCountText}"`, + ); + assert.equal( + notificationCountValue >= count, + true, + `Expected notification count to be at least ${count}, got ${notificationCountValue}`, + ); } async checkIfNetworkPickerClickable(clickable: boolean): Promise { diff --git a/test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts b/test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts index 3b87401897e7..4a53d3bd95ea 100644 --- a/test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts +++ b/test/e2e/tests/dapp-interactions/dapp-interactions.spec.ts @@ -82,7 +82,11 @@ describe('Dapp interactions', function () { ); await connectAccountConfirmation.checkPageIsLoaded(); await connectAccountConfirmation.confirmConnect(); - await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); + try { + await driver.switchToWindowWithUrl(DAPP_ONE_URL); + } catch { + await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); + } await testDapp.checkConnectedAccounts(DEFAULT_FIXTURE_ACCOUNT); // Login to homepage 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 index 3aab09c896cc..64600bcc9c63 100644 --- a/ui/components/app/permission-page-container/permission-page-container.component.js +++ b/ui/components/app/permission-page-container/permission-page-container.component.js @@ -154,7 +154,7 @@ export default class PermissionPageContainer extends Component { rejectPermissionsRequest(request?.metadata?.id); }; - onSubmit = () => { + onSubmit = async () => { const { request: _request, approvePermissionsRequest, @@ -221,9 +221,9 @@ export default class PermissionPageContainer extends Component { }; if (Object.keys(request.permissions).length > 0) { - approvePermissionsRequest(request); + await approvePermissionsRequest(request); } else { - rejectPermissionsRequest(request?.metadata?.id); + await rejectPermissionsRequest(request?.metadata?.id); } }; diff --git a/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx b/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx index acc8d60ed843..ef51d13f04e4 100644 --- a/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx +++ b/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx @@ -31,7 +31,8 @@ export const NotificationsTagCounter = ({ className="notification-list-item__unread-dot__wrapper" style={{ position: 'absolute', - cursor: 'pointer', + cursor: 'default', + pointerEvents: 'none', top: 0, left: '50%', zIndex: 1, diff --git a/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.test.tsx b/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.test.tsx index 7643e32c47ee..885e738ffd09 100644 --- a/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.test.tsx +++ b/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.test.tsx @@ -94,7 +94,7 @@ const mockUseAccountGroupsForPermissions = jest.fn((..._args: unknown[]) => ({ ], }, ], - caipAccountIdsOfConnectedAccountGroupWithRequested: ['eip155:1:0x123'], + caipAccountIdsOfConnectedAndRequestedAccountGroups: ['eip155:1:0x123'], selectedAndRequestedAccountGroups: [ { id: 'entropy:01JKAF3DSGM3AB87EM9N0K41AJ/0', @@ -170,7 +170,7 @@ jest.mock('../../../hooks/useAccountGroupsForPermissions', () => ({ ], }, ], - caipAccountIdsOfConnectedAccountGroupWithRequested: ['eip155:1:0x123'], + caipAccountIdsOfConnectedAndRequestedAccountGroups: ['eip155:1:0x123'], selectedAndRequestedAccountGroups: [ { id: 'entropy:01JKAF3DSGM3AB87EM9N0K41AJ/0', @@ -288,7 +288,9 @@ jest.mock('../../permissions-connect/connect-page/utils', () => ({ })); jest.mock('../../../../shared/lib/multichain/scope-utils', () => ({ - getCaip25AccountFromAccountGroupAndScope: jest.fn(() => ['eip155:1:0x123']), + getCaip25AccountIdsFromAccountGroupAndScope: jest.fn(() => [ + 'eip155:1:0x123', + ]), })); const mockGetCaip25CaveatValueFromPermissions = jest.requireMock( diff --git a/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.tsx b/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.tsx index 6362efac6c26..ec6fedd5eb9b 100644 --- a/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.tsx +++ b/ui/pages/multichain-accounts/multichain-accounts-connect-page/multichain-accounts-connect-page.tsx @@ -105,7 +105,9 @@ export type MultichainConnectPageProps = { request: MultichainAccountsConnectPageRequest; permissionsRequestId: string; rejectPermissionsRequest: (id: string) => void; - approveConnection: (request: MultichainAccountsConnectPageRequest) => void; + approveConnection: ( + request: MultichainAccountsConnectPageRequest, + ) => Promise | void; targetSubjectMetadata: { extensionId: string | null; iconUrl: string | null; @@ -335,6 +337,8 @@ export const MultichainAccountsConnectPage: React.FC< testNetworkConfigurations, requestedCaipChainIds, isEip1193Request, + isSolanaWalletStandardRequest, + isTronWalletAdapterRequest, currentlySelectedNetwork.chainId, requestedNamespaces, requestedNamespacesWithoutWallet, @@ -374,9 +378,9 @@ export const MultichainAccountsConnectPage: React.FC< const { suggestedAccountGroups, suggestedCaipAccountIds } = useMemo(() => { if (connectedAccountGroups.length > 0) { return { - suggestedAccountGroups: connectedAccountGroupWithRequested, + suggestedAccountGroups: connectedAccountGroupWithRequested ?? [], suggestedCaipAccountIds: - caipAccountIdsOfConnectedAndRequestedAccountGroups, + caipAccountIdsOfConnectedAndRequestedAccountGroups ?? [], }; } @@ -400,9 +404,9 @@ export const MultichainAccountsConnectPage: React.FC< } return { - suggestedAccountGroups: selectedAndRequestedAccountGroups, + suggestedAccountGroups: selectedAndRequestedAccountGroups ?? [], suggestedCaipAccountIds: getCaip25AccountIdsFromAccountGroupAndScope( - selectedAndRequestedAccountGroups, + selectedAndRequestedAccountGroups ?? [], requestedAndAlreadyConnectedCaipChainIdsOrDefault, ), }; @@ -417,12 +421,12 @@ export const MultichainAccountsConnectPage: React.FC< ]); const [selectedAccountGroupIds, setSelectedAccountGroupIds] = useState( - suggestedAccountGroups.map((group) => group.id), + (suggestedAccountGroups ?? []).map((group) => group.id), ); const [selectedCaipAccountIds, setSelectedCaipAccountIds] = useState< CaipAccountId[] - >(suggestedCaipAccountIds); + >(suggestedCaipAccountIds ?? []); const handleAccountGroupIdsSelected = useCallback( ( @@ -432,29 +436,73 @@ export const MultichainAccountsConnectPage: React.FC< if (isUserModified) { setUserHasModifiedSelection(true); } - const updatedSelectedChains = [...selectedChainIds]; - - // Create lookup sets for selected account group IDs - const selectedGroupIds = new Set(accountGroupIds); + setSelectedAccountGroupIds(accountGroupIds); + setPageMode(MultichainAccountsConnectPageMode.Summary); + }, + [], + ); - // Filter to only selected account groups - const selectedAccountGroups = supportedAccountGroups.filter( - (group: AccountGroupWithInternalAccounts) => - selectedGroupIds.has(group.id), + // Keep selected chains in sync with defaults until the user explicitly edits. + useEffect(() => { + if ( + !userHasModifiedSelection && + !isEqual( + requestedAndAlreadyConnectedCaipChainIdsOrDefault, + selectedChainIds, + ) + ) { + handleChainIdsSelected( + requestedAndAlreadyConnectedCaipChainIdsOrDefault, + { + isUserModified: false, + }, ); + } + }, [ + userHasModifiedSelection, + requestedAndAlreadyConnectedCaipChainIdsOrDefault, + selectedChainIds, + handleChainIdsSelected, + ]); + + // Keep CAIP account IDs derived from the current selected groups and chains. + useEffect(() => { + if (typeof getCaip25AccountIdsFromAccountGroupAndScope !== 'function') { + return; + } - const caip25AccountIds = getCaip25AccountIdsFromAccountGroupAndScope( + const selectedGroupIds = new Set(selectedAccountGroupIds); + const selectedAccountGroups = supportedAccountGroups.filter( + (group: AccountGroupWithInternalAccounts) => + selectedGroupIds.has(group.id), + ); + + const nextSelectedCaipAccountIds = + getCaip25AccountIdsFromAccountGroupAndScope( selectedAccountGroups, - updatedSelectedChains, + selectedChainIds, ); - handleChainIdsSelected(updatedSelectedChains, { isUserModified }); - setSelectedAccountGroupIds(accountGroupIds); - setSelectedCaipAccountIds(caip25AccountIds); - setPageMode(MultichainAccountsConnectPageMode.Summary); - }, - [selectedChainIds, supportedAccountGroups, handleChainIdsSelected], - ); + // During initial hydration, preserve a precomputed non-empty selection + // instead of replacing it with a transient empty derivation. + if ( + !userHasModifiedSelection && + nextSelectedCaipAccountIds.length === 0 && + selectedCaipAccountIds.length > 0 + ) { + return; + } + + if (!isEqual(nextSelectedCaipAccountIds, selectedCaipAccountIds)) { + setSelectedCaipAccountIds(nextSelectedCaipAccountIds); + } + }, [ + selectedAccountGroupIds, + supportedAccountGroups, + selectedChainIds, + selectedCaipAccountIds, + userHasModifiedSelection, + ]); // Ensures the selected account state is kept in sync with the default selected account value // until the user makes modifications to the selected account/network values. @@ -493,25 +541,67 @@ export const MultichainAccountsConnectPage: React.FC< rejectPermissionsRequest(permissionsRequestId); }, [permissionsRequestId, rejectPermissionsRequest]); - const onConfirm = useCallback(() => { - const _request = { + const onConfirm = useCallback(async () => { + const requestMetadata = request.metadata; + const nextRequestMetadata: NonNullable< + MultichainAccountsConnectPageRequest['metadata'] + > = { + id: requestMetadata?.id ?? permissionsRequestId, + origin: targetSubjectMetadata.origin ?? requestMetadata?.origin, + }; + if (requestMetadata?.isEip1193Request !== undefined) { + nextRequestMetadata.isEip1193Request = requestMetadata.isEip1193Request; + } + + const selectedGroupIds = new Set(selectedAccountGroupIds); + const selectedAccountGroups = supportedAccountGroups.filter( + (group: AccountGroupWithInternalAccounts) => + selectedGroupIds.has(group.id), + ); + + const selectedCaipAccountIdsForRequest = + getCaip25AccountIdsFromAccountGroupAndScope( + selectedAccountGroups, + selectedChainIds, + ); + + // Compute submit payload from source selections at confirm time so we do + // not depend on asynchronously derived local state. + const finalSelectedCaipAccountIds = + selectedCaipAccountIdsForRequest.length > 0 + ? selectedCaipAccountIdsForRequest + : selectedCaipAccountIds; + + if ( + finalSelectedCaipAccountIds.length === 0 || + selectedChainIds.length === 0 + ) { + return; + } + + const _request: MultichainAccountsConnectPageRequest = { ...request, + metadata: nextRequestMetadata, permissions: { ...request.permissions, ...generateCaip25Caveat( requestedCaip25CaveatValueWithExistingPermissions, - selectedCaipAccountIds, + finalSelectedCaipAccountIds, selectedChainIds, ), }, }; - approveConnection(_request); + await approveConnection(_request); }, [ request, + permissionsRequestId, requestedCaip25CaveatValueWithExistingPermissions, selectedCaipAccountIds, selectedChainIds, + selectedAccountGroupIds, + supportedAccountGroups, approveConnection, + targetSubjectMetadata.origin, ]); const title = transformOriginToTitle(targetSubjectMetadata.origin); @@ -728,7 +818,8 @@ export const MultichainAccountsConnectPage: React.FC< onClick={onConfirm} disabled={ selectedAccountGroupIds.length === 0 || - selectedChainIds.length === 0 + selectedChainIds.length === 0 || + selectedCaipAccountIds.length === 0 } > {t('connect')} diff --git a/ui/pages/permissions-connect/connect-page/connect-page.tsx b/ui/pages/permissions-connect/connect-page/connect-page.tsx index ad9a1d4cbfc2..fc4d5d9982a9 100644 --- a/ui/pages/permissions-connect/connect-page/connect-page.tsx +++ b/ui/pages/permissions-connect/connect-page/connect-page.tsx @@ -110,7 +110,7 @@ export type ConnectPageProps = { request: ConnectPageRequest; permissionsRequestId: string; rejectPermissionsRequest: (id: string) => void; - approveConnection: (request: ConnectPageRequest) => void; + approveConnection: (request: ConnectPageRequest) => Promise | void; activeTabOrigin: string; targetSubjectMetadata: { extensionId: string | null; @@ -428,9 +428,23 @@ export const ConnectPage: React.FC = ({ rejectPermissionsRequest(permissionsRequestId); }, [permissionsRequestId, rejectPermissionsRequest]); - const onConfirm = useCallback(() => { - const _request = { + const onConfirm = useCallback(async () => { + const requestMetadata = request.metadata; + const nextRequestMetadata: NonNullable = { + id: requestMetadata?.id ?? permissionsRequestId, + origin: targetSubjectMetadata.origin ?? requestMetadata?.origin, + }; + if (requestMetadata?.isEip1193Request !== undefined) { + nextRequestMetadata.isEip1193Request = requestMetadata.isEip1193Request; + } + if (requestMetadata?.promptToCreateSolanaAccount !== undefined) { + nextRequestMetadata.promptToCreateSolanaAccount = + requestMetadata.promptToCreateSolanaAccount; + } + + const _request: ConnectPageRequest = { ...request, + metadata: nextRequestMetadata, permissions: { ...request.permissions, ...generateCaip25Caveat( @@ -440,13 +454,15 @@ export const ConnectPage: React.FC = ({ ), }, }; - approveConnection(_request); + await approveConnection(_request); }, [ request, + permissionsRequestId, requestedCaip25CaveatValueWithExistingPermissions, selectedCaipAccountAddresses, selectedChainIds, approveConnection, + targetSubjectMetadata.origin, ]); const title = transformOriginToTitle(targetSubjectMetadata.origin); diff --git a/ui/pages/permissions-connect/permissions-connect.tsx b/ui/pages/permissions-connect/permissions-connect.tsx index 39ba48338bd6..2bfefeef7b1e 100644 --- a/ui/pages/permissions-connect/permissions-connect.tsx +++ b/ui/pages/permissions-connect/permissions-connect.tsx @@ -336,6 +336,11 @@ function PermissionsConnect() { const prevLastConnectedInfoRef = useRef( null, ); + const isApprovingPermissionsRef = useRef(false); + const redirectTimeoutRef = useRef | undefined>( + undefined, + ); + const permissionsRequestRef = useRef(permissionsRequest); // Define redirect function before it's used in effects const redirect = useCallback( @@ -347,6 +352,11 @@ function PermissionsConnect() { shouldRedirect = !isRequestingSnap; + if (redirectTimeoutRef.current) { + clearTimeout(redirectTimeoutRef.current); + redirectTimeoutRef.current = undefined; + } + setRedirecting(shouldRedirect); setPermissionsApproved(approved); @@ -356,7 +366,14 @@ function PermissionsConnect() { } if (approved) { - setTimeout(() => navigate(DEFAULT_ROUTE), APPROVE_TIMEOUT); + redirectTimeoutRef.current = setTimeout(() => { + // If another permissions request appears before the timeout expires, + // keep the user on the approval flow instead of navigating home. + if (!permissionsRequestRef.current) { + navigate(DEFAULT_ROUTE); + } + redirectTimeoutRef.current = undefined; + }, APPROVE_TIMEOUT); return; } navigate(DEFAULT_ROUTE); @@ -364,34 +381,78 @@ function PermissionsConnect() { [permissions, navigate], ); - // Handle initial navigation on mount + useEffect(() => { + permissionsRequestRef.current = permissionsRequest; + + if (permissionsRequest && redirecting) { + if (redirectTimeoutRef.current) { + clearTimeout(redirectTimeoutRef.current); + redirectTimeoutRef.current = undefined; + } + setRedirecting(false); + setPermissionsApproved(null); + } + }, [permissionsRequest, redirecting]); + + useEffect( + () => () => { + if (redirectTimeoutRef.current) { + clearTimeout(redirectTimeoutRef.current); + } + }, + [], + ); + + // Handle initial setup on mount. useEffect(() => { dispatch(getRequestAccountTabIdsAction()); + }, [dispatch]); + // Keep the approval route aligned with the current request type. + useEffect(() => { if (!permissionsRequest) { navigate(DEFAULT_ROUTE, { replace: true }); return; } - if (location.pathname === connectPath && !isRequestingAccounts) { - switch (requestType) { - case 'wallet_installSnap': - navigate(snapInstallPath, { replace: true }); - break; - case 'wallet_updateSnap': - navigate(snapUpdatePath, { replace: true }); - break; - case 'wallet_installSnapResult': - navigate(snapResultPath, { replace: true }); - break; - case 'wallet_connectSnaps': - navigate(snapsConnectPath, { replace: true }); - break; - default: - navigate(confirmPermissionPath, { replace: true }); - } + if (isRequestingAccounts) { + return; + } + + let targetPath = confirmPermissionPath; + switch (requestType) { + case 'wallet_installSnap': + targetPath = snapInstallPath; + break; + case 'wallet_updateSnap': + targetPath = snapUpdatePath; + break; + case 'wallet_installSnapResult': + targetPath = snapResultPath; + break; + case 'wallet_connectSnaps': + targetPath = snapsConnectPath; + break; + default: + targetPath = confirmPermissionPath; + break; } - }, []); // eslint-disable-line react-hooks/exhaustive-deps + + if (location.pathname !== targetPath) { + navigate(targetPath, { replace: true }); + } + }, [ + permissionsRequest, + isRequestingAccounts, + requestType, + location.pathname, + confirmPermissionPath, + snapInstallPath, + snapUpdatePath, + snapResultPath, + snapsConnectPath, + navigate, + ]); // Cache targetSubjectMetadata when it changes useEffect(() => { @@ -412,6 +473,14 @@ function PermissionsConnect() { prevPermissionsRequestRef.current && !redirecting ) { + // While approval submission is in flight, wait for approveConnection + // to resolve and call redirect(true/false) explicitly. + if (isApprovingPermissionsRef.current) { + prevPermissionsRequestRef.current = permissionsRequest; + prevLastConnectedInfoRef.current = lastConnectedInfo; + return; + } + const lastConnectedForOrigin = lastConnectedInfo[origin] as | { lastApproved?: number; accounts?: Record } | undefined; @@ -479,16 +548,53 @@ function PermissionsConnect() { ); const approveConnection = useCallback( - (request: Record) => { - // Cast through unknown to satisfy both local and controller types - dispatch( - approvePermissionsRequestAction( - request as unknown as ControllerPermissionsRequest, - ), - ); - redirect(true); + async (request: Record) => { + isApprovingPermissionsRef.current = true; + try { + const requestMetadata = + (request.metadata as Record | undefined) ?? {}; + const pendingRequestMetadata = + (permissionsRequest?.metadata as + | Record + | undefined) ?? {}; + + // Normalize approval metadata from the active pending request first to + // avoid submitting mixed (id/origin) pairs during lock/unlock races. + const normalizedRequest = { + ...request, + metadata: { + ...requestMetadata, + id: + (pendingRequestMetadata.id as string | undefined) ?? + (requestMetadata.id as string | undefined) ?? + permissionsRequestId, + origin: + (pendingRequestMetadata.origin as string | undefined) ?? + (requestMetadata.origin as string | undefined) ?? + originFromRequest, + }, + }; + + // Cast through unknown to satisfy both local and controller types + await dispatch( + approvePermissionsRequestAction( + normalizedRequest as unknown as ControllerPermissionsRequest, + ), + ); + redirect(true); + } catch { + // Keep the user on the connect page if approval submission fails. + } finally { + isApprovingPermissionsRef.current = false; + } }, - [dispatch, redirect], + [ + dispatch, + permissionsRequest, + permissionsRequestId, + originFromRequest, + redirect, + ], ); const setSnapsInstallPrivacyWarningShownStatus = useCallback( @@ -661,13 +767,8 @@ function PermissionsConnect() { element={ { - dispatch( - approvePermissionsRequestAction( - request as unknown as ControllerPermissionsRequest, - ), - ); - redirect(true); + approvePermissionsRequest={async (request: unknown) => { + await approveConnection(request as Record); }} rejectPermissionsRequest={(requestId: string) => cancelPermissionsRequest(requestId) diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 57e108590023..7c9f86d90831 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -5442,6 +5442,7 @@ export function approvePermissionsRequest( await submitRequestToBackground('approvePermissionsRequest', [request]); } catch (err) { dispatch(displayWarning(err)); + throw err; } await forceUpdateMetamaskState(dispatch); };