Conversation
- remove redundant req.securityAlertResponse
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #23480 +/- ##
========================================
Coverage 68.44% 68.45%
========================================
Files 1141 1141
Lines 43770 43767 -3
Branches 11729 11728 -1
========================================
Hits 29957 29957
+ Misses 13813 13810 -3 ☔ View full report in Codecov by Sentry. |
Builds ready [3da4686]
Page Load Metrics (1363 ± 417 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| .catch((error: any) => { | ||
| const errorObject = error as unknown as Error; |
There was a problem hiding this comment.
How would you feel about a) typing error with unknown instead of any, and b) using a runtime type check rather than a compile time type assertion to express assumptions about the shape of error (e.g. it should have name and message property)?
The following diff only affirms the constraints already in place and adds handling for the exception case without adding new restrictions:
diff --git a/app/scripts/lib/ppom/ppom-middleware.ts b/app/scripts/lib/ppom/ppom-middleware.ts
index 47a1be0a98..32fda30fdc 100644
--- a/app/scripts/lib/ppom/ppom-middleware.ts
+++ b/app/scripts/lib/ppom/ppom-middleware.ts
@@ -81,17 +95,21 @@ export function createPPOMMiddleware(
securityAlertResponse = await ppom.validateJsonRpc(req);
securityAlertResponse.securityAlertId = securityAlertId;
})
- .catch((error: any) => {
- const errorObject = error as unknown as Error;
+ .catch((error: unknown) => {
+ if (error instanceof Error) {
+ const errorObject = error;
- sentry?.captureException(error);
- console.error('Error validating JSON RPC using PPOM: ', error);
+ sentry?.captureException(error);
+ console.error('Error validating JSON RPC using PPOM: ', error);
- securityAlertResponse = {
- result_type: BlockaidResultType.Errored,
- reason: BlockaidReason.errored,
- description: `${errorObject.name}: ${errorObject.message}`,
- };
+ securityAlertResponse = {
+ result_type: BlockaidResultType.Errored,
+ reason: BlockaidReason.errored,
+ description: `${errorObject.name}: ${errorObject.message}`,
+ };
+ } else {
+ throw error;
+ }
})
.finally(() => {
updateSecurityAlertResponseByTxId(req, {There was a problem hiding this comment.
I like these suggestions! Thanks @MajorLift
one thing is, I think we'd still like to capture the error in Sentry no matter what form it arrives in. What if, in the case we receive a non-Error type, we coerce the value into a string to turn it into an Error?
example:
.catch((error: unknown) => {
const errorObject =
error instanceof Error ? error : new Error(error as string);
sentry?.captureException(errorObject);
console.error(
'Error validating JSON RPC using PPOM: ',
errorObject,
);
securityAlertResponse = {
result_type: BlockaidResultType.Errored,
reason: BlockaidReason.errored,
description: `${errorObject.name}: ${errorObject.message}`,
};
})
There was a problem hiding this comment.
In that case, I would suggest the following:
.catch((error: unknown) => {
sentry?.captureException(error);
console.error(
'Error validating JSON RPC using PPOM: ',
typeof error === 'object' || typeof error === 'string'
? error
: JSON.stringify(error),
);
securityAlertResponse = {
result_type: BlockaidResultType.Errored,
reason: BlockaidReason.errored,
description:
error instanceof Error
? `${error.name}: ${error.message}`
: JSON.stringify(error),
};
})new Error(error as string)doesn't actually converterrorinto a string at runtime (TypeScript has type assertions not type casts). This means information inerrorwould be lost if it's just passed into anErrorconstructor.String(error)would be one way to do runtime conversion, but the result is likely to be[object Object], which is why we needJSON.stringify.
There was a problem hiding this comment.
re: new Error(error as string)
oops. of course. not sure why I wrote it like this. I was thinking new Error(String(error)),, but you have a great point with it might turn out to be [object Object] as well. Good call with the JSON.stringify(error)
thanks @MajorLift! lgtm and updated 06530bc
|
Some additional typing suggestions on this file that fall outside the scope of the current diffs. Feel free to apply or disregard! diff --git a/app/scripts/lib/ppom/ppom-middleware.ts b/app/scripts/lib/ppom/ppom-middleware.ts
index 47a1be0a98..977c49cdcf 100644
--- a/app/scripts/lib/ppom/ppom-middleware.ts
+++ b/app/scripts/lib/ppom/ppom-middleware.ts
@@ -1,6 +1,8 @@
+import { JsonRpcRequest, JsonRpcResponse } from 'json-rpc-engine';
import { PPOM } from '@blockaid/ppom_release';
import { PPOMController } from '@metamask/ppom-validator';
import { NetworkController } from '@metamask/network-controller';
+import { Hex, Json, JsonRpcParams } from '@metamask/utils';
import { v4 as uuid } from 'uuid';
import {
@@ -12,7 +14,7 @@ import { SIGNING_METHODS } from '../../../../shared/constants/transaction';
import { PreferencesController } from '../../controllers/preferences';
import { SecurityAlertResponse } from '../transaction/util';
-const { sentry } = global as any;
+const { sentry } = global;
const CONFIRMATION_METHODS = Object.freeze([
'eth_sendRawTransaction',
@@ -20,7 +22,7 @@ const CONFIRMATION_METHODS = Object.freeze([
...SIGNING_METHODS,
]);
-export const SUPPORTED_CHAIN_IDS: string[] = [
+export const SUPPORTED_CHAIN_IDS: Hex[] = [
CHAIN_IDS.ARBITRUM,
CHAIN_IDS.AVALANCHE,
CHAIN_IDS.BASE,
@@ -48,17 +50,28 @@ export const SUPPORTED_CHAIN_IDS: string[] = [
* @param updateSecurityAlertResponseByTxId
* @returns PPOMMiddleware function.
*/
-export function createPPOMMiddleware(
+export function createPPOMMiddleware<
+ Params extends JsonRpcParams = JsonRpcParams,
+ Result extends Json = Json,
+>(
ppomController: PPOMController,
preferencesController: PreferencesController,
networkController: NetworkController,
appStateController: any,
updateSecurityAlertResponseByTxId: (
- req: any,
+ req: JsonRpcRequest & {
+ securityAlertResponse: SecurityAlertResponse;
+ },
securityAlertResponse: SecurityAlertResponse,
) => void,
) {
- return async (req: any, _res: any, next: () => void) => {
+ return async (
+ req: JsonRpcRequest<Params> & {
+ securityAlertResponse: SecurityAlertResponse;
+ },
+ _res: JsonRpcResponse<Result>,
+ next: () => void,
+ ) => {
try {
const securityAlertsEnabled =
preferencesController.store.getState()?.securityAlertsEnabled; |
- see #23480 (comment) Co-authored-by: Jongsun Suh <34228073+MajorLift@users.noreply.github.com>
|
@MajorLift nice! updated #23480 (comment) (sorry missed finalizing co-author on this one but comment is linked) |
|
This should probably fix the test errors. Also no worries on the co-author thing. I believe one co-authored commit is enough to be added as a co-author to the eventual squashed commit. diff --git a/app/scripts/lib/ppom/ppom-middleware.test.ts b/app/scripts/lib/ppom/ppom-middleware.test.ts
index de3cff83f9..be38298295 100644
--- a/app/scripts/lib/ppom/ppom-middleware.test.ts
+++ b/app/scripts/lib/ppom/ppom-middleware.test.ts
@@ -1,3 +1,8 @@
+import {
+ type Hex,
+ JsonRpcRequestStruct,
+ JsonRpcResponseStruct,
+} from '@metamask/utils';
import { CHAIN_IDS } from '../../../../shared/constants/network';
import {
BlockaidReason,
@@ -21,7 +26,7 @@ Object.defineProperty(globalThis, 'performance', {
const createMiddleWare = (
usePPOM?: any,
securityAlertsEnabled?: boolean,
- chainId?: string,
+ chainId?: Hex,
) => {
const usePPOMMock = jest.fn();
const ppomController = {
@@ -64,8 +69,8 @@ describe('PPOMMiddleware', () => {
const usePPOMMock = jest.fn();
const middlewareFunction = createMiddleWare(usePPOMMock);
await middlewareFunction(
- { method: 'eth_sendTransaction' },
- undefined,
+ { ...JsonRpcRequestStruct, method: 'eth_sendTransaction' },
+ { ...JsonRpcResponseStruct },
() => undefined,
);
expect(usePPOMMock).toHaveBeenCalledTimes(1);
@@ -75,10 +80,15 @@ describe('PPOMMiddleware', () => {
const usePPOM = async () => Promise.resolve('VALIDATION_RESULT');
const middlewareFunction = createMiddleWare(usePPOM);
const req = {
+ ...JsonRpcRequestStruct,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};
- await middlewareFunction(req, undefined, () => undefined);
+ await middlewareFunction(
+ req,
+ { ...JsonRpcResponseStruct },
+ () => undefined,
+ );
expect(req.securityAlertResponse).toBeDefined();
});
@@ -86,10 +96,15 @@ describe('PPOMMiddleware', () => {
const usePPOM = async () => Promise.resolve('VALIDATION_RESULT');
const middlewareFunction = createMiddleWare(usePPOM, false);
const req = {
+ ...JsonRpcRequestStruct,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};
- await middlewareFunction(req, undefined, () => undefined);
+ await middlewareFunction(
+ req,
+ { ...JsonRpcResponseStruct },
+ () => undefined,
+ );
expect(req.securityAlertResponse).toBeUndefined();
});
@@ -97,10 +112,15 @@ describe('PPOMMiddleware', () => {
const usePPOM = async () => Promise.resolve('VALIDATION_RESULT');
const middlewareFunction = createMiddleWare(usePPOM, false, '0x2');
const req = {
+ ...JsonRpcRequestStruct,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};
- await middlewareFunction(req, undefined, () => undefined);
+ await middlewareFunction(
+ req,
+ { ...JsonRpcResponseStruct },
+ () => undefined,
+ );
expect(req.securityAlertResponse).toBeUndefined();
});
@@ -110,10 +130,15 @@ describe('PPOMMiddleware', () => {
};
const middlewareFunction = createMiddleWare({ usePPOM });
const req = {
+ ...JsonRpcRequestStruct,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};
- await middlewareFunction(req, undefined, () => undefined);
+ await middlewareFunction(
+ req,
+ { ...JsonRpcResponseStruct },
+ () => undefined,
+ );
expect((req.securityAlertResponse as any)?.result_type).toBe(
BlockaidResultType.Errored,
);
@@ -132,8 +157,8 @@ describe('PPOMMiddleware', () => {
const middlewareFunction = createMiddleWare(usePPOM);
const nextMock = jest.fn();
await middlewareFunction(
- { method: 'eth_sendTransaction' },
- undefined,
+ { ...JsonRpcRequestStruct, method: 'eth_sendTransaction' },
+ { ...JsonRpcResponseStruct },
nextMock,
);
expect(nextMock).toHaveBeenCalledTimes(1);
@@ -146,8 +171,8 @@ describe('PPOMMiddleware', () => {
const middlewareFunction = createMiddleWare(usePPOM);
const nextMock = jest.fn();
await middlewareFunction(
- { method: 'eth_sendTransaction' },
- undefined,
+ { ...JsonRpcRequestStruct, method: 'eth_sendTransaction' },
+ { ...JsonRpcResponseStruct },
nextMock,
);
expect(nextMock).toHaveBeenCalledTimes(1);
@@ -163,8 +188,8 @@ describe('PPOMMiddleware', () => {
};
const middlewareFunction = createMiddleWare(usePPOM);
await middlewareFunction(
- { method: 'eth_sendTransaction' },
- undefined,
+ { ...JsonRpcRequestStruct, method: 'eth_sendTransaction' },
+ { ...JsonRpcResponseStruct },
() => undefined,
);
expect(validateMock).toHaveBeenCalledTimes(1);
@@ -180,8 +205,8 @@ describe('PPOMMiddleware', () => {
};
const middlewareFunction = createMiddleWare(usePPOM);
await middlewareFunction(
- { method: 'eth_someRequest' },
- undefined,
+ { ...JsonRpcRequestStruct, method: 'eth_someRequest' },
+ { ...JsonRpcResponseStruct },
() => undefined,
);
expect(validateMock).toHaveBeenCalledTimes(0);
@@ -189,6 +214,7 @@ describe('PPOMMiddleware', () => {
it('normalizes transaction requests before validation', async () => {
const requestMock1 = {
+ ...JsonRpcRequestStruct,
method: 'eth_sendTransaction',
params: [{ data: '0x1' }],
};
@@ -212,7 +238,11 @@ describe('PPOMMiddleware', () => {
const middlewareFunction = createMiddleWare(usePPOM);
- await middlewareFunction(requestMock1, undefined, () => undefined);
+ await middlewareFunction(
+ requestMock1,
+ { ...JsonRpcResponseStruct },
+ () => undefined,
+ );
expect(normalizePPOMRequestMock).toHaveBeenCalledTimes(1);
expect(normalizePPOMRequestMock).toHaveBeenCalledWith(requestMock1);
diff --git a/app/scripts/lib/transaction/util.ts b/app/scripts/lib/transaction/util.ts
index ad22e7a4f7..1a06a50a99 100644
--- a/app/scripts/lib/transaction/util.ts
+++ b/app/scripts/lib/transaction/util.ts
@@ -14,6 +14,7 @@ import {
} from '@metamask/user-operation-controller';
///: BEGIN:ONLY_INCLUDE_IF(blockaid)
import { PPOMController } from '@metamask/ppom-validator';
+import type { Hex } from '@metamask/utils';
import { captureException } from '@sentry/browser';
import { addHexPrefix } from 'ethereumjs-util';
import { v4 as uuid } from 'uuid';
@@ -60,7 +61,7 @@ export type AddTransactionOptions = NonNullable<
>;
type BaseAddTransactionRequest = {
- chainId: string;
+ chainId: Hex;
networkClientId: string;
ppomController: PPOMController;
securityAlertsEnabled: boolean; |
| import { PPOMController } from '@metamask/ppom-validator'; | ||
| import { NetworkController } from '@metamask/network-controller'; | ||
| import { Hex, Json, JsonRpcParams } from '@metamask/utils'; | ||
| import { JsonRpcRequest, JsonRpcResponse } from 'json-rpc-engine'; |
There was a problem hiding this comment.
JsonRpcRequest and JsonRpcResponse should be imported from @metamask/utils. json-rpc-engine is actualy re-exporting those two types from @metamask/utils and the repo has been archived.
| .catch((error: unknown) => { | ||
| sentry?.captureException(error); | ||
| console.error( | ||
| 'Error validating JSON RPC using PPOM: ', | ||
| typeof error === 'object' || typeof error === 'string' | ||
| ? error | ||
| : JSON.stringify(error), | ||
| ); | ||
|
|
||
| return securityAlertResponse; | ||
| } | ||
| securityAlertResponse = { | ||
| result_type: BlockaidResultType.Errored, | ||
| reason: BlockaidReason.errored, | ||
| description: | ||
| error instanceof Error | ||
| ? `${error.name}: ${error.message}` | ||
| : JSON.stringify(error), | ||
| }; |
There was a problem hiding this comment.
This is going to result in capturing errors that are not directly related with the ppom validation... usePPOM might throw an error that is completely unrelated with validating request. For example any of these can throw an unrelated error. And I might be wrong, but I am assuming that 'User has securityAlertsEnabled set to false' should not be a blockaid error description and captured within the securityAlertResponse.
Nevertheless those errors are not being caught anywhere (usePPOM returns a promise and we are not awaiting for it in order to be able to catch it here). So I do agree that we need to either await for usePPOM to resolve or if we do not need to await for it then we need to add the chained catch. But we need to distinguish between errors being thrown within the validation callback and errors being thrown by usePPOM.
JsonRpcResponse from @metamask/util
Builds ready [dc37c65]
Page Load Metrics (959 ± 466 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [0818c8a]
Page Load Metrics (915 ± 546 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| typeof error === 'object' || typeof error === 'string' | ||
| ? error | ||
| : JSON.stringify(error), |
There was a problem hiding this comment.
Does not need to be solved in this PR, but why are we doing this serialization?
I get that JSON.stringify(error) when error is an instanceof Error will result in an empty object (as Error properties are not enumerable). But why typeof 'object'?
Wouldn't this be a better approach?
error instanceof Error || typeof error === 'string'For example the above option would allow us to properly log objects with nested arrays of objects.
Also this pattern is repeated a couple of times in this file. Should we at least have a method to consoleLogSerializedError? It would just have two args: the error (unknown) and the message to be logged.
Anyways, serliazing an unknown variable properly can be complex, as I am sure I am missing a couple of edge cases.
There was a problem hiding this comment.
console.error outputs interactively expandable representations of input objects. This applies to nested objects/arrays as well, so to my understanding, serialization isn't necessary here unless we're dealing with very large, deeply nested objects.
typeof error === 'object' is because I assumed that we'd want the console.error non-serialized representation for thrown object literals in general, not just for Error types.
That said, I'm not familiar with the logging and monitoring pipeline in the extension, so my assumptions about use cases might be off here.
I think a consoleLogSerializedError helper method makes a lot of sense. We have serializeError and related methods in @metamask/rpc-errors for json-rpc errors. Something similar for browser console and sentry errors would definitely be useful.
There was a problem hiding this comment.
Oh you are 100% right.. I was thinking about node console and not the browser (which is the env where the extension is executed). My bad 🤦♂️
@digiwand feel free to ignore my comment about typeof x === 'object' vs instanceof error.
Yes we should have a common way to not just serialize errors, but also to stack them up (where the stack trace and message of one error does not get lost in the hierarchical error handling). Anyways this out of scope of this PR.
|
thanks for the reviews and discussions! Merging and fixing/updating tests in a new PR to get this fix in sooner |
|
PR for related tests: |
Description
Related issues
Fixes: #23571
Tests Related to: #23745
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist