From 6d1454515f0980d10043061755e03925b50a1134 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 5 Sep 2024 12:41:24 +0200 Subject: [PATCH 1/7] Add initial tracing to SignatureController --- packages/controller-utils/src/types.ts | 37 ++++++++++++++ .../src/SignatureController.ts | 49 +++++++++++++++---- packages/signature-controller/src/types.ts | 37 ++++++++++++++ packages/transaction-controller/src/types.ts | 1 + 4 files changed, 115 insertions(+), 9 deletions(-) create mode 100644 packages/signature-controller/src/types.ts diff --git a/packages/controller-utils/src/types.ts b/packages/controller-utils/src/types.ts index b71139791b8..83b5fb5977a 100644 --- a/packages/controller-utils/src/types.ts +++ b/packages/controller-utils/src/types.ts @@ -127,3 +127,40 @@ export type NetworkNickname = // TODO: Move to @metamask/utils export type Partialize = Omit & Partial>; + +/** A context in which to execute a trace, in order to generate nested timings. */ +export type TraceContext = unknown; + +/** Request to trace an operation. */ +export type TraceRequest = { + /** Additional data to include in the trace. */ + data?: Record; + + /** Name of the operation. */ + name: string; + + /** + * Unique identifier for the trace. + * Required if starting a trace and not providing a callback. + */ + id?: string; + + /** Trace context in which to execute the operation. */ + parentContext?: TraceContext; + + /** Additional tags to include in the trace to filter results. */ + tags?: Record; +}; + +/** Callback that traces the performance of an operation. */ +export type TraceCallback = ( + /** Request to trace the performance of an operation. */ + request: TraceRequest, + + /** + * Callback to trace. + * Thrown errors will not be caught, but the trace will still be recorded. + * @param context - The context in which the operation is running. + */ + fn?: (context?: TraceContext) => ReturnType, +) => Promise; diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index 24a7abab10c..56f3bcee7a6 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -44,6 +44,8 @@ import type { Hex, Json } from '@metamask/utils'; import EventEmitter from 'events'; import { cloneDeep } from 'lodash'; +import type { TraceCallback, TraceContext } from './types'; + const controllerName = 'SignatureController'; const stateMetadata = { @@ -109,9 +111,10 @@ export type SignatureControllerMessenger = RestrictedControllerMessenger< >; export type SignatureControllerOptions = { - messenger: SignatureControllerMessenger; - isEthSignEnabled: () => boolean; getAllState: () => unknown; + getCurrentChainId: () => Hex; + isEthSignEnabled: () => boolean; + messenger: SignatureControllerMessenger; securityProviderRequest?: ( // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -120,7 +123,7 @@ export type SignatureControllerOptions = { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any ) => Promise; - getCurrentChainId: () => Hex; + trace?: TraceCallback; }; /** @@ -141,6 +144,8 @@ export class SignatureController extends BaseController< #typedMessageManager: TypedMessageManager; + #trace: TraceCallback; + /** * Construct a Sign controller. * @@ -149,12 +154,14 @@ export class SignatureController extends BaseController< * @param options.getAllState - Callback to retrieve all user state. * @param options.securityProviderRequest - A function for verifying a message, whether it is malicious or not. * @param options.getCurrentChainId - A function for retrieving the current chainId. + * @param options.trace - Callback to generate trace information. */ constructor({ - messenger, getAllState, - securityProviderRequest, getCurrentChainId, + messenger, + securityProviderRequest, + trace, }: SignatureControllerOptions) { super({ name: controllerName, @@ -178,6 +185,7 @@ export class SignatureController extends BaseController< undefined, getCurrentChainId, ); + this.#trace = trace ?? (((_request, fn) => fn?.()) as TraceCallback); this.#handleMessageManagerEvents( this.#personalMessageManager, @@ -276,11 +284,13 @@ export class SignatureController extends BaseController< * * @param messageParams - The params of the message to sign & return to the Dapp. * @param req - The original request, containing the origin. + * @param traceContext - The parent context for any new traces. * @returns Promise resolving to the raw data of the signature request. */ async newUnsignedPersonalMessage( messageParams: PersonalMessageParams, req: OriginalRequest, + traceContext?: TraceContext, ): Promise { return this.#newUnsignedAbstractMessage( this.#personalMessageManager, @@ -292,6 +302,9 @@ export class SignatureController extends BaseController< this.#signPersonalMessage.bind(this), messageParams, req, + undefined, + undefined, + traceContext, ); } @@ -303,6 +316,7 @@ export class SignatureController extends BaseController< * @param version - The version indicating the format of the typed data. * @param signingOpts - An options bag for signing. * @param signingOpts.parseJsonData - Whether to parse the JSON before signing. + * @param traceContext - The parent context for any new traces. * @returns Promise resolving to the raw data of the signature request. */ async newUnsignedTypedMessage( @@ -310,6 +324,7 @@ export class SignatureController extends BaseController< req: OriginalRequest, version: string, signingOpts: TypedMessageSigningOptions, + traceContext?: TraceContext, ): Promise { const signTypeForLogger = this.#getSignTypeForLogger(version); return this.#newUnsignedAbstractMessage( @@ -324,6 +339,7 @@ export class SignatureController extends BaseController< req, version, signingOpts, + traceContext, ); } @@ -397,6 +413,7 @@ export class SignatureController extends BaseController< req: OriginalRequest, version?: string, signingOpts?: SO, + traceContext?: TraceContext, ) { let resultCallbacks: AcceptResultCallbacks | undefined; try { @@ -425,9 +442,12 @@ export class SignatureController extends BaseController< messageParamsWithId, ); - const acceptResult = await this.#requestApproval( - messageParamsWithId, - approvalType, + const acceptResult = await this.#trace( + { name: 'Await Approval', parentContext: traceContext }, + (context) => + this.#requestApproval(messageParamsWithId, approvalType, { + traceContext: context, + }), ); resultCallbacks = acceptResult.resultCallbacks; @@ -445,7 +465,10 @@ export class SignatureController extends BaseController< // TODO: Either fix this lint violation or explain why it's necessary to ignore. // eslint-disable-next-line @typescript-eslint/await-thenable - await signMessage(messageParamsWithId, signingOpts); + + await this.#trace({ name: 'Sign', parentContext: traceContext }, () => + signMessage(messageParamsWithId, signingOpts), + ); const signatureResult = await signaturePromise; @@ -807,6 +830,7 @@ export class SignatureController extends BaseController< async #requestApproval( msgParams: AbstractMessageParamsMetamask, type: ApprovalType, + { traceContext }: { traceContext?: TraceContext }, ): Promise { const id = msgParams.metamaskId as string; const origin = msgParams.origin || ORIGIN_METAMASK; @@ -814,6 +838,13 @@ export class SignatureController extends BaseController< // We are explicitly cloning the message params here to prevent the mutation errors on development mode // Because sending it through the messaging system will make the object read only const clonedMsgParams = cloneDeep(msgParams); + + await this.#trace({ + name: 'Notification Display', + id, + parentContext: traceContext, + }); + return (await this.messagingSystem.call( 'ApprovalController:addRequest', { diff --git a/packages/signature-controller/src/types.ts b/packages/signature-controller/src/types.ts new file mode 100644 index 00000000000..7946ca4b3fd --- /dev/null +++ b/packages/signature-controller/src/types.ts @@ -0,0 +1,37 @@ +// Trace types later will be imported from '@metamask/controller-utils' +/** A context in which to execute a trace, in order to generate nested timings. */ +export type TraceContext = unknown; + +/** Request to trace an operation. */ +export type TraceRequest = { + /** Additional data to include in the trace. */ + data?: Record; + + /** Name of the operation. */ + name: string; + + /** + * Unique identifier for the trace. + * Required if starting a trace and not providing a callback. + */ + id?: string; + + /** Trace context in which to execute the operation. */ + parentContext?: TraceContext; + + /** Additional tags to include in the trace to filter results. */ + tags?: Record; +}; + +/** Callback that traces the performance of an operation. */ +export type TraceCallback = ( + /** Request to trace the performance of an operation. */ + request: TraceRequest, + + /** + * Callback to trace. + * Thrown errors will not be caught, but the trace will still be recorded. + * @param context - The context in which the operation is running. + */ + fn?: (context?: TraceContext) => ReturnType, +) => Promise; diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index 39565cecce8..c98343e413b 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -1300,6 +1300,7 @@ export type SimulationData = { tokenBalanceChanges: SimulationTokenBalanceChange[]; }; +// Trace types later will be imported from '@metamask/controller-utils' /** A context in which to execute a trace, in order to generate nested timings. */ export type TraceContext = unknown; From 473880a15fd262e9d57058e99d7272cfe529dbc7 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 5 Sep 2024 12:46:56 +0200 Subject: [PATCH 2/7] Remove unused option --- packages/signature-controller/src/SignatureController.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index 56f3bcee7a6..ae0bbf31d1d 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -113,7 +113,6 @@ export type SignatureControllerMessenger = RestrictedControllerMessenger< export type SignatureControllerOptions = { getAllState: () => unknown; getCurrentChainId: () => Hex; - isEthSignEnabled: () => boolean; messenger: SignatureControllerMessenger; securityProviderRequest?: ( // TODO: Replace `any` with type From 4389c98462520921f1bed7afd2a351ed02965164 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 5 Sep 2024 15:13:54 +0200 Subject: [PATCH 3/7] Add options bag --- .../src/SignatureController.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index ae0bbf31d1d..aeab6022345 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -283,13 +283,14 @@ export class SignatureController extends BaseController< * * @param messageParams - The params of the message to sign & return to the Dapp. * @param req - The original request, containing the origin. - * @param traceContext - The parent context for any new traces. + * @param options - An options bag for the method. + * @param options.traceContext - The parent context for any new traces. * @returns Promise resolving to the raw data of the signature request. */ async newUnsignedPersonalMessage( messageParams: PersonalMessageParams, req: OriginalRequest, - traceContext?: TraceContext, + options: { traceContext?: TraceContext } = {}, ): Promise { return this.#newUnsignedAbstractMessage( this.#personalMessageManager, @@ -303,7 +304,7 @@ export class SignatureController extends BaseController< req, undefined, undefined, - traceContext, + options.traceContext, ); } @@ -315,7 +316,8 @@ export class SignatureController extends BaseController< * @param version - The version indicating the format of the typed data. * @param signingOpts - An options bag for signing. * @param signingOpts.parseJsonData - Whether to parse the JSON before signing. - * @param traceContext - The parent context for any new traces. + * @param options - An options bag for the method. + * @param options.traceContext - The parent context for any new traces. * @returns Promise resolving to the raw data of the signature request. */ async newUnsignedTypedMessage( @@ -323,7 +325,7 @@ export class SignatureController extends BaseController< req: OriginalRequest, version: string, signingOpts: TypedMessageSigningOptions, - traceContext?: TraceContext, + options: { traceContext?: TraceContext } = {}, ): Promise { const signTypeForLogger = this.#getSignTypeForLogger(version); return this.#newUnsignedAbstractMessage( @@ -338,7 +340,7 @@ export class SignatureController extends BaseController< req, version, signingOpts, - traceContext, + options.traceContext, ); } From a8e4e15da998d7f38c4692642f38fc094b5cfc8f Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 5 Sep 2024 15:16:36 +0200 Subject: [PATCH 4/7] Import types from controller-utils --- .../src/SignatureController.ts | 3 +- packages/signature-controller/src/types.ts | 37 ------------------- 2 files changed, 1 insertion(+), 39 deletions(-) delete mode 100644 packages/signature-controller/src/types.ts diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index aeab6022345..f83a71b4e4f 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -10,6 +10,7 @@ import type { } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; import { ApprovalType, ORIGIN_METAMASK } from '@metamask/controller-utils'; +import type { TraceCallback, TraceContext } from '@metamask/controller-utils'; import type { KeyringControllerSignMessageAction, KeyringControllerSignPersonalMessageAction, @@ -44,8 +45,6 @@ import type { Hex, Json } from '@metamask/utils'; import EventEmitter from 'events'; import { cloneDeep } from 'lodash'; -import type { TraceCallback, TraceContext } from './types'; - const controllerName = 'SignatureController'; const stateMetadata = { diff --git a/packages/signature-controller/src/types.ts b/packages/signature-controller/src/types.ts deleted file mode 100644 index 7946ca4b3fd..00000000000 --- a/packages/signature-controller/src/types.ts +++ /dev/null @@ -1,37 +0,0 @@ -// Trace types later will be imported from '@metamask/controller-utils' -/** A context in which to execute a trace, in order to generate nested timings. */ -export type TraceContext = unknown; - -/** Request to trace an operation. */ -export type TraceRequest = { - /** Additional data to include in the trace. */ - data?: Record; - - /** Name of the operation. */ - name: string; - - /** - * Unique identifier for the trace. - * Required if starting a trace and not providing a callback. - */ - id?: string; - - /** Trace context in which to execute the operation. */ - parentContext?: TraceContext; - - /** Additional tags to include in the trace to filter results. */ - tags?: Record; -}; - -/** Callback that traces the performance of an operation. */ -export type TraceCallback = ( - /** Request to trace the performance of an operation. */ - request: TraceRequest, - - /** - * Callback to trace. - * Thrown errors will not be caught, but the trace will still be recorded. - * @param context - The context in which the operation is running. - */ - fn?: (context?: TraceContext) => ReturnType, -) => Promise; From bd89147f3a5ecc7b67f7b564f0a8187f36876b6a Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 5 Sep 2024 15:23:41 +0200 Subject: [PATCH 5/7] Remove Trace types from transaction-controller --- .../src/TransactionController.ts | 3 +- packages/transaction-controller/src/types.ts | 53 ------------------- 2 files changed, 1 insertion(+), 55 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 88a7ba4ad1d..3b120e07346 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -21,6 +21,7 @@ import { convertHexToDecimal, isInfuraNetworkType, } from '@metamask/controller-utils'; +import type { TraceCallback, TraceContext } from '@metamask/controller-utils'; import EthQuery from '@metamask/eth-query'; import type { FetchGasFeeEstimateOptions, @@ -78,8 +79,6 @@ import type { SimulationData, GasFeeEstimates, GasFeeFlowResponse, - TraceCallback, - TraceContext, GasPriceValue, FeeMarketEIP1559Values, } from './types'; diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index c98343e413b..9cbc4e0cd72 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -1299,56 +1299,3 @@ export type SimulationData = { /** Data concerning a change to the user's token balances. */ tokenBalanceChanges: SimulationTokenBalanceChange[]; }; - -// Trace types later will be imported from '@metamask/controller-utils' -/** A context in which to execute a trace, in order to generate nested timings. */ -export type TraceContext = unknown; - -/** Request to trace an operation. */ -export type TraceRequest = { - /** Additional data to include in the trace. */ - data?: Record; - - /** Name of the operation. */ - name: string; - - /** - * Unique identifier for the trace. - * Required if starting a trace and not providing a callback. - */ - id?: string; - - /** Trace context in which to execute the operation. */ - parentContext?: TraceContext; - - /** Additional tags to include in the trace to filter results. */ - tags?: Record; -}; - -/** Callback that traces the performance of an operation. */ -export type TraceCallback = ( - /** Request to trace the performance of an operation. */ - request: TraceRequest, - - /** - * Callback to trace. - * Thrown errors will not be caught, but the trace will still be recorded. - * @param context - The context in which the operation is running. - */ - fn?: (context?: TraceContext) => ReturnType, -) => Promise; - -/** Gas fee properties for a legacy transaction. */ -export type GasPriceValue = { - /** Price per gas for legacy transactions. */ - gasPrice: string; -}; - -/** Gas fee properties for an EIP-1559 transaction. */ -export type FeeMarketEIP1559Values = { - /** Maximum amount to pay per gas. */ - maxFeePerGas: string; - - /** Maximum amount per gas to give to the validator as an incentive. */ - maxPriorityFeePerGas: string; -}; From 6ea311ffcf0e7ba07f646783da19d447dfe01d10 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 5 Sep 2024 15:30:00 +0200 Subject: [PATCH 6/7] Revert unwanted type changes --- packages/transaction-controller/src/types.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index 9cbc4e0cd72..143c8295407 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -1299,3 +1299,18 @@ export type SimulationData = { /** Data concerning a change to the user's token balances. */ tokenBalanceChanges: SimulationTokenBalanceChange[]; }; + +/** Gas fee properties for a legacy transaction. */ +export type GasPriceValue = { + /** Price per gas for legacy transactions. */ + gasPrice: string; +}; + +/** Gas fee properties for an EIP-1559 transaction. */ +export type FeeMarketEIP1559Values = { + /** Maximum amount to pay per gas. */ + maxFeePerGas: string; + + /** Maximum amount per gas to give to the validator as an incentive. */ + maxPriorityFeePerGas: string; +}; From 39802fe258615497998dee5f206dbd25a6f07678 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Fri, 6 Sep 2024 12:39:58 +0200 Subject: [PATCH 7/7] Add actionId while sending trace --- packages/signature-controller/src/SignatureController.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index f83a71b4e4f..4ae73a8523b 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -447,6 +447,7 @@ export class SignatureController extends BaseController< (context) => this.#requestApproval(messageParamsWithId, approvalType, { traceContext: context, + actionId: req?.id?.toString(), }), ); @@ -830,7 +831,10 @@ export class SignatureController extends BaseController< async #requestApproval( msgParams: AbstractMessageParamsMetamask, type: ApprovalType, - { traceContext }: { traceContext?: TraceContext }, + { + traceContext, + actionId, + }: { traceContext?: TraceContext; actionId?: string }, ): Promise { const id = msgParams.metamaskId as string; const origin = msgParams.origin || ORIGIN_METAMASK; @@ -841,7 +845,7 @@ export class SignatureController extends BaseController< await this.#trace({ name: 'Notification Display', - id, + id: actionId, parentContext: traceContext, });