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.

Sign up for GitHub

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

Jump to bottom

feat: add utility function to get supported chains from the Security Alerts API #10267

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+121 −9
Open

feat: add utility function to get supported chains from the Security Alerts API #10267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c752ab6
feat: add supported chains utils
vinistevam Jul 9, 2024
fec9d5c
Merge branch 'main' into feat/add-utility-function-supported-chains
vinistevam Jul 10, 2024
a5129ef
fix unit test
vinistevam Jul 10, 2024
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
59 changes: 53 additions & 6 deletions app/lib/ppom/ppom-util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@ import * as SignatureRequestActions from '../../actions/signatureRequest'; // es
import * as TransactionActions from '../../actions/transaction'; // eslint-disable-line import/no-namespace
import Engine from '../../core/Engine';
import PPOMUtil from './ppom-util';
import {
isSecurityAlertsAPIEnabled,
validateWithSecurityAlertsAPI,
} from './security-alerts-api';
// eslint-disable-next-line import/no-namespace
import * as securityAlertAPI from './security-alerts-api';

const CHAIN_ID_MOCK = '0x1';

Expand Down Expand Up @@ -76,10 +74,12 @@ const mockSignatureRequest = {

describe('PPOM Utils', () => {
const validateWithSecurityAlertsAPIMock = jest.mocked(
validateWithSecurityAlertsAPI,
securityAlertAPI.validateWithSecurityAlertsAPI,
);

const isSecurityAlertsEnabledMock = jest.mocked(isSecurityAlertsAPIEnabled);
const isSecurityAlertsEnabledMock = jest.mocked(
securityAlertAPI.isSecurityAlertsAPIEnabled,
);

const normalizeTransactionParamsMock = jest.mocked(
normalizeTransactionParams,
Expand Down Expand Up @@ -238,6 +238,9 @@ describe('PPOM Utils', () => {

it('uses security alerts API if enabled', async () => {
isSecurityAlertsEnabledMock.mockReturnValue(true);
jest
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but could we create a persistent variable for this such as getSupportedChainsMock like we do for isSecurityAlertsEnabledMock?

.spyOn(securityAlertAPI, 'getSupportedChains')
.mockImplementation(async () => [CHAIN_ID_MOCK]);

await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);

Expand All @@ -252,6 +255,9 @@ describe('PPOM Utils', () => {

it('uses controller if security alerts API throws', async () => {
isSecurityAlertsEnabledMock.mockReturnValue(true);
jest
.spyOn(securityAlertAPI, 'getSupportedChains')
.mockImplementation(async () => [CHAIN_ID_MOCK]);

validateWithSecurityAlertsAPIMock.mockRejectedValue(
new Error('Test Error'),
Expand All @@ -270,4 +276,45 @@ describe('PPOM Utils', () => {
);
});
});

describe('isChainSupported', () => {
describe('when security alerts API is enabled', () => {
beforeEach(async () => {
isSecurityAlertsEnabledMock.mockReturnValue(true);
jest
.spyOn(securityAlertAPI, 'getSupportedChains')
.mockImplementation(async () => [CHAIN_ID_MOCK]);
});
it('returns true if chain is supported', async () => {
expect(await PPOMUtil.isChainSupported(CHAIN_ID_MOCK)).toStrictEqual(
true,
);
});

it('returns false if chain is not supported', async () => {
expect(await PPOMUtil.isChainSupported('0x2')).toStrictEqual(false);
});

it('returns correctly if security alerts API throws', async () => {
jest
.spyOn(securityAlertAPI, 'getSupportedChains')
.mockRejectedValue(new Error('Test Error'));
expect(await PPOMUtil.isChainSupported(CHAIN_ID_MOCK)).toStrictEqual(
true,
);
});
});

describe('when security alerts API is disabled', () => {
it('returns true if chain is supported', async () => {
expect(await PPOMUtil.isChainSupported(CHAIN_ID_MOCK)).toStrictEqual(
true,
);
});

it('returns false if chain is not supported', async () => {
expect(await PPOMUtil.isChainSupported('0x2')).toStrictEqual(false);
});
});
});
});
20 changes: 18 additions & 2 deletions app/lib/ppom/ppom-util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ import {
import { WALLET_CONNECT_ORIGIN } from '../../util/walletconnect';
import AppConstants from '../../core/AppConstants';
import {
getSupportedChains,
isSecurityAlertsAPIEnabled,
validateWithSecurityAlertsAPI,
} from './security-alerts-api';
import { PPOMController } from '@metamask/ppom-validator';
import { Hex } from '@metamask/utils';

export interface PPOMRequest {
method: string;
Expand Down Expand Up @@ -65,7 +67,7 @@ async function validateRequest(req: PPOMRequest, transactionId?: string) {

const chainId = NetworkController.state.providerConfig.chainId;
const isConfirmationMethod = CONFIRMATION_METHODS.includes(req.method);
const isSupportedChain = BLOCKAID_SUPPORTED_CHAIN_IDS.includes(chainId);
const isSupportedChain = await isChainSupported(chainId);

const isSecurityAlertsEnabled =
PreferencesController.state.securityAlertsEnabled;
Expand Down Expand Up @@ -119,6 +121,20 @@ async function validateRequest(req: PPOMRequest, transactionId?: string) {
}
}

async function isChainSupported(chainId: Hex): Promise<boolean> {
try {
const supportedChains = isSecurityAlertsAPIEnabled()
? await getSupportedChains()
: BLOCKAID_SUPPORTED_CHAIN_IDS;
return supportedChains.includes(chainId);
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but could we remove some duplication with something like:

let supportedChainIds = BLOCKAID_SUPPORTED_CHAIN_IDS;

try {
    if(isSecurityAlertsAPIEnabled()) {
        supportedChainIds = getSupportedChainds();
    }
} catch (e) {
    Logger.log(
      `Error fetching supported chains from security alerts API: ${e}`,
    );
}

return supportedChainIds.includes(chainId);

} catch (e) {
Logger.log(
`Error fetching supported chains from security alerts API: ${e}`,
);
return BLOCKAID_SUPPORTED_CHAIN_IDS.includes(chainId);
}
}

async function validateWithController(
ppomController: PPOMController,
request: PPOMRequest,
Expand Down Expand Up @@ -193,4 +209,4 @@ function normalizeRequest(request: PPOMRequest): PPOMRequest {
};
}

export default { validateRequest };
export default { validateRequest, isChainSupported };
31 changes: 30 additions & 1 deletion app/lib/ppom/security-alerts-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import {
Reason,
ResultType,
} from '../../components/Views/confirmations/components/BlockaidBanner/BlockaidBanner.types';
import { validateWithSecurityAlertsAPI } from './security-alerts-api';
import {
getSupportedChains,
validateWithSecurityAlertsAPI,
} from './security-alerts-api';

const CHAIN_ID_MOCK = '0x1';

Expand Down Expand Up @@ -66,4 +69,30 @@ describe('Security Alerts API', () => {
);
});
});

describe('getSupportedChains', () => {
it('sends GET request', async () => {
const SUPPORTED_CHAIN_IDS_MOCK = ['0x1', '0x2'];
fetchMock.mockResolvedValue({
ok: true,
json: async () => SUPPORTED_CHAIN_IDS_MOCK,
});
const response = await getSupportedChains();

expect(response).toEqual(SUPPORTED_CHAIN_IDS_MOCK);

expect(fetchMock).toHaveBeenCalledTimes(1);
expect(fetchMock).toHaveBeenCalledWith(
`https://example.com/supportedChains`,
);
});

it('throws an error if response is not ok', async () => {
fetchMock.mockResolvedValue({ ok: false, status: 404 });

await expect(getSupportedChains()).rejects.toThrow(
'Security alerts API request failed with status: 404',
);
});
});
});
20 changes: 20 additions & 0 deletions app/lib/ppom/security-alerts-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { Hex } from '@metamask/utils';
import { SecurityAlertResponse } from '../../components/Views/confirmations/components/BlockaidBanner/BlockaidBanner.types';

const ENDPOINT_VALIDATE = 'validate';
const ENDPOINT_SUPPORTED_CHAINS = 'supportedChains';

export interface SecurityAlertsAPIRequest {
method: string;
Expand All @@ -19,6 +21,10 @@ export async function validateWithSecurityAlertsAPI(
return postRequest(endpoint, request);
}

export async function getSupportedChains(): Promise<Hex[]> {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe getSecurityAlertsAPISupportedChainIds to be explicit?

return getRequest(ENDPOINT_SUPPORTED_CHAINS);
}

async function postRequest(endpoint: string, body: unknown) {
const url = getUrl(endpoint);

Expand All @@ -39,6 +45,20 @@ async function postRequest(endpoint: string, body: unknown) {
return response.json();
}

async function getRequest(endpoint: string) {
const url = getUrl(endpoint);

const response = await fetch(url);

if (!response.ok) {
throw new Error(
`Security alerts API request failed with status: ${response.status}`,
);
}

return response.json();
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the duplication and create a request function for example that accepts the method and some extra properties?

}

function getUrl(endpoint: string) {
const host = process.env.SECURITY_ALERTS_API_URL;

Expand Down
Loading