Skip to content

Commit

Permalink
fix: get supportedChains to avoid blocking the confirmation process (…
Browse files Browse the repository at this point in the history 
…#28313)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
This PR addresses the blocking behaviour in the confirmation process
caused by the synchronous `supportedChains` check. Previously, the Send
flow was halted until a response from the
`security-alerts.api.cx.metamask.io` API was received. This created
delays and negatively impacted the user experience, especially when the
API response was slow.

**Key Changes:**
**Asynchronous Chain Validation:** The `supportedChains` check is now
performed asynchronously within the non-awaited
`validateRequestWithPPOM` function. This ensures that the Send flow is
not blocked while waiting for the API response.
**Introduction of CheckingChain Result Type:** A new result type,
`CheckingChain`, is introduced to represent the intermediate state
before a definitive result is obtained. This state is used before the
`loading` status in both middleware and transaction utility functions.

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28313?quickstart=1)

## **Related issues**

Fixes: #28255
#28257

## **Manual testing steps**

1. Use a proxy to intercept requests
2. Start a Send flow
3. Intercept the request to `security-alerts.api.cx.metamask.io`
4. See send flow cannot be initiated until we have a response to this
request

## **Screenshots/Recordings**


[supported-chains-.webm](https://github.com/user-attachments/assets/4e9e495a-10f3-4bb1-8d05-8045a735b655)


[unsupported-chains.webm](https://github.com/user-attachments/assets/e5767bc1-2eab-44bd-83c3-777d34c23ff6)

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension 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
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
  • Loading branch information
@vinistevam
vinistevam authored Nov 12, 2024
1 parent 68ba635 commit e54f555
Show file tree
Hide file tree
Showing 12 changed files with 271 additions and 293 deletions.
78 changes: 4 additions & 74 deletions app/scripts/lib/ppom/ppom-middleware.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
BlockaidReason,
BlockaidResultType,
} from '../../../../shared/constants/security-provider';
import { flushPromises } from '../../../../test/lib/timer-helpers';
import { mockNetworkState } from '../../../../test/stub/networks';
import { createPPOMMiddleware, PPOMMiddlewareRequest } from './ppom-middleware';
import {
Expand Down Expand Up @@ -105,7 +104,6 @@ const createMiddleware = (
};

describe('PPOMMiddleware', () => {
const validateRequestWithPPOMMock = jest.mocked(validateRequestWithPPOM);
const generateSecurityAlertIdMock = jest.mocked(generateSecurityAlertId);
const handlePPOMErrorMock = jest.mocked(handlePPOMError);
const isChainSupportedMock = jest.mocked(isChainSupported);
Expand All @@ -114,7 +112,6 @@ describe('PPOMMiddleware', () => {
beforeEach(() => {
jest.resetAllMocks();

validateRequestWithPPOMMock.mockResolvedValue(SECURITY_ALERT_RESPONSE_MOCK);
generateSecurityAlertIdMock.mockReturnValue(SECURITY_ALERT_ID_MOCK);
handlePPOMErrorMock.mockReturnValue(SECURITY_ALERT_RESPONSE_MOCK);
isChainSupportedMock.mockResolvedValue(true);
Expand All @@ -129,38 +126,13 @@ describe('PPOMMiddleware', () => {
};
});

it('updates alert response after validating request', async () => {
it('adds checking chain response to confirmation requests while validation is in progress', async () => {
const updateSecurityAlertResponse = jest.fn();

const middlewareFunction = createMiddleware({
updateSecurityAlertResponse,
});

const req = {
...REQUEST_MOCK,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};

await middlewareFunction(
req,
{ ...JsonRpcResponseStruct.TYPE },
() => undefined,
);

await flushPromises();

expect(updateSecurityAlertResponse).toHaveBeenCalledTimes(1);
expect(updateSecurityAlertResponse).toHaveBeenCalledWith(
req.method,
SECURITY_ALERT_ID_MOCK,
SECURITY_ALERT_RESPONSE_MOCK,
);
});

it('adds loading response to confirmation requests while validation is in progress', async () => {
const middlewareFunction = createMiddleware();

const req: PPOMMiddlewareRequest<(string | { to: string })[]> = {
...REQUEST_MOCK,
method: 'eth_sendTransaction',
Expand All @@ -173,7 +145,9 @@ describe('PPOMMiddleware', () => {
() => undefined,
);

expect(req.securityAlertResponse?.reason).toBe(BlockaidReason.inProgress);
expect(req.securityAlertResponse?.reason).toBe(
BlockaidReason.checkingChain,
);
expect(req.securityAlertResponse?.result_type).toBe(
BlockaidResultType.Loading,
);
Expand All @@ -197,50 +171,6 @@ describe('PPOMMiddleware', () => {
expect(validateRequestWithPPOM).not.toHaveBeenCalled();
});

it('does not do validation if unable to get the chainId from the network provider config', async () => {
isChainSupportedMock.mockResolvedValue(false);
const middlewareFunction = createMiddleware({
chainId: null,
});

const req = {
...REQUEST_MOCK,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};

await middlewareFunction(
req,
{ ...JsonRpcResponseStruct.TYPE },
() => undefined,
);

expect(req.securityAlertResponse).toBeUndefined();
expect(validateRequestWithPPOM).not.toHaveBeenCalled();
});

it('does not do validation if user is not on a supported network', async () => {
isChainSupportedMock.mockResolvedValue(false);
const middlewareFunction = createMiddleware({
chainId: '0x2',
});

const req = {
...REQUEST_MOCK,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};

await middlewareFunction(
req,
{ ...JsonRpcResponseStruct.TYPE },
() => undefined,
);

expect(req.securityAlertResponse).toBeUndefined();
expect(validateRequestWithPPOM).not.toHaveBeenCalled();
});

it('does not do validation when request is not for confirmation method', async () => {
const middlewareFunction = createMiddleware();

Expand Down
30 changes: 9 additions & 21 deletions app/scripts/lib/ppom/ppom-middleware.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,16 @@ import { MESSAGE_TYPE } from '../../../../shared/constants/app';
import { SIGNING_METHODS } from '../../../../shared/constants/transaction';
import { PreferencesController } from '../../controllers/preferences-controller';
import { AppStateController } from '../../controllers/app-state-controller';
import { LOADING_SECURITY_ALERT_RESPONSE } from '../../../../shared/constants/security-provider';
import { SECURITY_ALERT_RESPONSE_CHECKING_CHAIN } from '../../../../shared/constants/security-provider';
// eslint-disable-next-line import/no-restricted-paths
import { getProviderConfig } from '../../../../ui/ducks/metamask/metamask';
import { trace, TraceContext, TraceName } from '../../../../shared/lib/trace';
import {
generateSecurityAlertId,
handlePPOMError,
isChainSupported,
validateRequestWithPPOM,
} from './ppom-util';
import { SecurityAlertResponse } from './types';
import { SecurityAlertResponse, UpdateSecurityAlertResponse } from './types';

const CONFIRMATION_METHODS = Object.freeze([
'eth_sendRawTransaction',
Expand Down Expand Up @@ -64,11 +63,7 @@ export function createPPOMMiddleware<
networkController: NetworkController,
appStateController: AppStateController,
accountsController: AccountsController,
updateSecurityAlertResponse: (
method: string,
signatureAlertId: string,
securityAlertResponse: SecurityAlertResponse,
) => void,
updateSecurityAlertResponse: UpdateSecurityAlertResponse,
) {
return async (
req: PPOMMiddlewareRequest<Params>,
Expand All @@ -88,9 +83,7 @@ export function createPPOMMiddleware<

if (
!securityAlertsEnabled ||
!CONFIRMATION_METHODS.includes(req.method) ||
// Do not move this call above this check because it will result in unnecessary calls
!(await isChainSupported(chainId))
!CONFIRMATION_METHODS.includes(req.method)
) {
return;
}
Expand Down Expand Up @@ -122,27 +115,22 @@ export function createPPOMMiddleware<
request: req,
securityAlertId,
chainId,
}).then((securityAlertResponse) => {
updateSecurityAlertResponse(
req.method,
securityAlertId,
securityAlertResponse,
);
updateSecurityAlertResponse,
}),
);

const loadingSecurityAlertResponse: SecurityAlertResponse = {
...LOADING_SECURITY_ALERT_RESPONSE,
const securityAlertResponseCheckingChain: SecurityAlertResponse = {
...SECURITY_ALERT_RESPONSE_CHECKING_CHAIN,
securityAlertId,
};

if (SIGNING_METHODS.includes(req.method)) {
appStateController.addSignatureSecurityAlertResponse(
loadingSecurityAlertResponse,
securityAlertResponseCheckingChain,
);
}

req.securityAlertResponse = loadingSecurityAlertResponse;
req.securityAlertResponse = securityAlertResponseCheckingChain;
} catch (error) {
req.securityAlertResponse = handlePPOMError(
error,
Expand Down
Loading

0 comments on commit e54f555

Please sign in to comment.