From e36be911bfbe8a3e6e009d2b3f52f40290878c78 Mon Sep 17 00:00:00 2001 From: httpjunkie Date: Mon, 24 Mar 2025 16:36:39 -0400 Subject: [PATCH 1/5] feat: add transaction hash to Smart Transaction analytics events - Update Smart Transactions Controller to support tx hash in analytics when feature flag is enabled - Add tx hash to sensitiveProperties with proper type definitions - Only include hash when both remote feature flag is enabled and user has opted into MetaMetrics - Update tests to support new "required" parameters --- src/SmartTransactionsController.test.ts | 4 ++++ src/SmartTransactionsController.ts | 19 +++++++++++++++-- src/index.test.ts | 2 ++ src/utils.ts | 28 +++++++++++++++++++++++-- 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/SmartTransactionsController.test.ts b/src/SmartTransactionsController.test.ts index bcb2b7c9..d006f5be 100644 --- a/src/SmartTransactionsController.test.ts +++ b/src/SmartTransactionsController.test.ts @@ -2222,6 +2222,10 @@ async function withController( getFeatureFlags: jest.fn(), updateTransaction: jest.fn(), ...options, + getRemoteFeatureFlags: + options?.getRemoteFeatureFlags ?? + (() => ({ transactionsTxHashInAnalytics: false })), + getParticipateInMetrics: options?.getParticipateInMetrics ?? (() => false), }); function triggerNetworStateChange(state: NetworkState) { diff --git a/src/SmartTransactionsController.ts b/src/SmartTransactionsController.ts index ac3aa45f..a9b2d7ce 100644 --- a/src/SmartTransactionsController.ts +++ b/src/SmartTransactionsController.ts @@ -206,6 +206,8 @@ type SmartTransactionsControllerOptions = { getMetaMetricsProps: () => Promise; getFeatureFlags: () => FeatureFlags; updateTransaction: (transaction: TransactionMeta, note: string) => void; + getRemoteFeatureFlags: () => { transactionsTxHashInAnalytics?: boolean }; + getParticipateInMetrics: () => boolean; }; export type SmartTransactionsControllerPollingInput = { @@ -245,6 +247,10 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo #updateTransaction: SmartTransactionsControllerOptions['updateTransaction']; + #getRemoteFeatureFlags: () => { transactionsTxHashInAnalytics?: boolean }; + + #getParticipateInMetrics: () => boolean; + /* istanbul ignore next */ async #fetch(request: string, options?: RequestInit) { const fetchOptions = { @@ -272,6 +278,8 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo getMetaMetricsProps, getFeatureFlags, updateTransaction, + getRemoteFeatureFlags, + getParticipateInMetrics, }: SmartTransactionsControllerOptions) { super({ name: controllerName, @@ -319,6 +327,8 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo `${controllerName}:stateChange`, (currentState) => this.checkPoll(currentState), ); + this.#getRemoteFeatureFlags = getRemoteFeatureFlags; + this.#getParticipateInMetrics = getParticipateInMetrics; } async _executePoll({ @@ -411,6 +421,8 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo properties: getSmartTransactionMetricsProperties(updatedSmartTransaction), sensitiveProperties: getSmartTransactionMetricsSensitiveProperties( updatedSmartTransaction, + this.#getRemoteFeatureFlags, + this.#getParticipateInMetrics, ), }); } @@ -733,8 +745,11 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo event: MetaMetricsEventName.StxConfirmed, category: MetaMetricsEventCategory.Transactions, properties: getSmartTransactionMetricsProperties(smartTransaction), - sensitiveProperties: - getSmartTransactionMetricsSensitiveProperties(smartTransaction), + sensitiveProperties: getSmartTransactionMetricsSensitiveProperties( + smartTransaction, + this.#getRemoteFeatureFlags, + this.#getParticipateInMetrics, + ), }); this.#updateSmartTransaction( { ...smartTransaction, confirmed: true }, diff --git a/src/index.test.ts b/src/index.test.ts index f04e5557..a2ab40d7 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -38,6 +38,8 @@ describe('default export', () => { getFeatureFlags: jest.fn(), updateTransaction: jest.fn(), clientId: ClientId.Extension, + getRemoteFeatureFlags: () => ({ transactionsTxHashInAnalytics: false }), + getParticipateInMetrics: () => false, }); expect(controller).toBeInstanceOf(SmartTransactionsController); jest.clearAllTimers(); diff --git a/src/utils.ts b/src/utils.ts index 1fd34882..6efda6be 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -256,19 +256,43 @@ export const getSmartTransactionMetricsProperties = ( }; }; +type SmartTransactionSensitiveProperties = { + token_from_symbol?: string; + token_to_symbol?: string; + account_hardware_type?: string; + account_type?: string; + device_model?: string; + transaction_hash?: string; +}; + export const getSmartTransactionMetricsSensitiveProperties = ( smartTransaction: SmartTransaction, -) => { + getRemoteFeatureFlags?: () => { transactionsTxHashInAnalytics?: boolean }, + getParticipateInMetrics?: () => boolean, +): SmartTransactionSensitiveProperties => { if (!smartTransaction) { return {}; } - return { + + const sensitiveProperties: SmartTransactionSensitiveProperties = { token_from_symbol: smartTransaction.sourceTokenSymbol, token_to_symbol: smartTransaction.destinationTokenSymbol, account_hardware_type: smartTransaction.accountHardwareType, account_type: smartTransaction.accountType, device_model: smartTransaction.deviceModel, }; + + // Add transaction hash if feature flag is enabled and user has opted in + if ( + getRemoteFeatureFlags?.()?.transactionsTxHashInAnalytics && + getParticipateInMetrics?.() && + smartTransaction.statusMetadata?.minedHash + ) { + sensitiveProperties.transaction_hash = + smartTransaction.statusMetadata.minedHash; + } + + return sensitiveProperties; }; export const getReturnTxHashAsap = ( From 55654a223ac933f23962325557b2f5a1da2e62bb Mon Sep 17 00:00:00 2001 From: httpjunkie Date: Tue, 25 Mar 2025 13:37:37 -0400 Subject: [PATCH 2/5] fix: removing getParticipateInMetrics from PR - The second check for getParticipateInMetrics is redundant since MetaMetrics events would only be sent if the user has already opted in - The MetaMetrics system handles this permission check at a higher level before any events are even tracked - Our implementation didn't need to check it --- src/SmartTransactionsController.test.ts | 1 - src/SmartTransactionsController.ts | 7 ------- src/index.test.ts | 1 - src/utils.ts | 2 -- 4 files changed, 11 deletions(-) diff --git a/src/SmartTransactionsController.test.ts b/src/SmartTransactionsController.test.ts index d006f5be..e148ee31 100644 --- a/src/SmartTransactionsController.test.ts +++ b/src/SmartTransactionsController.test.ts @@ -2225,7 +2225,6 @@ async function withController( getRemoteFeatureFlags: options?.getRemoteFeatureFlags ?? (() => ({ transactionsTxHashInAnalytics: false })), - getParticipateInMetrics: options?.getParticipateInMetrics ?? (() => false), }); function triggerNetworStateChange(state: NetworkState) { diff --git a/src/SmartTransactionsController.ts b/src/SmartTransactionsController.ts index a9b2d7ce..3996d2b4 100644 --- a/src/SmartTransactionsController.ts +++ b/src/SmartTransactionsController.ts @@ -207,7 +207,6 @@ type SmartTransactionsControllerOptions = { getFeatureFlags: () => FeatureFlags; updateTransaction: (transaction: TransactionMeta, note: string) => void; getRemoteFeatureFlags: () => { transactionsTxHashInAnalytics?: boolean }; - getParticipateInMetrics: () => boolean; }; export type SmartTransactionsControllerPollingInput = { @@ -249,8 +248,6 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo #getRemoteFeatureFlags: () => { transactionsTxHashInAnalytics?: boolean }; - #getParticipateInMetrics: () => boolean; - /* istanbul ignore next */ async #fetch(request: string, options?: RequestInit) { const fetchOptions = { @@ -279,7 +276,6 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo getFeatureFlags, updateTransaction, getRemoteFeatureFlags, - getParticipateInMetrics, }: SmartTransactionsControllerOptions) { super({ name: controllerName, @@ -328,7 +324,6 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo (currentState) => this.checkPoll(currentState), ); this.#getRemoteFeatureFlags = getRemoteFeatureFlags; - this.#getParticipateInMetrics = getParticipateInMetrics; } async _executePoll({ @@ -422,7 +417,6 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo sensitiveProperties: getSmartTransactionMetricsSensitiveProperties( updatedSmartTransaction, this.#getRemoteFeatureFlags, - this.#getParticipateInMetrics, ), }); } @@ -748,7 +742,6 @@ export default class SmartTransactionsController extends StaticIntervalPollingCo sensitiveProperties: getSmartTransactionMetricsSensitiveProperties( smartTransaction, this.#getRemoteFeatureFlags, - this.#getParticipateInMetrics, ), }); this.#updateSmartTransaction( diff --git a/src/index.test.ts b/src/index.test.ts index a2ab40d7..011b14cb 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -39,7 +39,6 @@ describe('default export', () => { updateTransaction: jest.fn(), clientId: ClientId.Extension, getRemoteFeatureFlags: () => ({ transactionsTxHashInAnalytics: false }), - getParticipateInMetrics: () => false, }); expect(controller).toBeInstanceOf(SmartTransactionsController); jest.clearAllTimers(); diff --git a/src/utils.ts b/src/utils.ts index 6efda6be..fdd6e13c 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -268,7 +268,6 @@ type SmartTransactionSensitiveProperties = { export const getSmartTransactionMetricsSensitiveProperties = ( smartTransaction: SmartTransaction, getRemoteFeatureFlags?: () => { transactionsTxHashInAnalytics?: boolean }, - getParticipateInMetrics?: () => boolean, ): SmartTransactionSensitiveProperties => { if (!smartTransaction) { return {}; @@ -285,7 +284,6 @@ export const getSmartTransactionMetricsSensitiveProperties = ( // Add transaction hash if feature flag is enabled and user has opted in if ( getRemoteFeatureFlags?.()?.transactionsTxHashInAnalytics && - getParticipateInMetrics?.() && smartTransaction.statusMetadata?.minedHash ) { sensitiveProperties.transaction_hash = From 13f2dc271bcb242235ba21922777a5035be4a4d1 Mon Sep 17 00:00:00 2001 From: httpjunkie Date: Tue, 25 Mar 2025 20:16:43 -0400 Subject: [PATCH 3/5] fix: correct Uint8Array.from usage in getTxHash Fix TypeScript error by properly converting hex string to `Uint8Array` for `TransactionFactory.fromSerializedData`. This maintains the same functionality while providing the correct type expected by the function. The change converts the hex string to bytes using `Buffer.from()` first, then creates a `Uint8Array` from those bytes, ensuring type safety without changing the underlying data. --- src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils.ts b/src/utils.ts index fdd6e13c..f9a920e0 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -230,7 +230,7 @@ export const getTxHash = (signedTxHex: any) => { } const txHashBytes = TransactionFactory.fromSerializedData( // eslint-disable-next-line no-restricted-globals - Buffer.from(signedTxHex.slice(2), 'hex'), + Uint8Array.from(Buffer.from(signedTxHex.slice(2), 'hex')), ).hash(); return bytesToHex(txHashBytes); }; From 6670949819d69112c9686cb10c5590c19e610988 Mon Sep 17 00:00:00 2001 From: httpjunkie Date: Tue, 25 Mar 2025 20:43:37 -0400 Subject: [PATCH 4/5] refactor: improve type safety and hex handling in getTxHash Replaces unsafe `Buffer` usage with `hexToBytes` utility and adds proper string typing, ensuring reliable transaction hash generation for MetaMetrics events. --- src/utils.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index f9a920e0..e4841014 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,5 +1,5 @@ import { TransactionFactory } from '@ethereumjs/tx'; -import { bytesToHex } from '@ethereumjs/util'; +import { bytesToHex, hexToBytes } from '@ethereumjs/util'; import { hexlify } from '@ethersproject/bytes'; import type { TransactionMeta } from '@metamask/transaction-controller'; import { TransactionStatus } from '@metamask/transaction-controller'; @@ -224,13 +224,15 @@ export const incrementNonceInHex = (nonceInHex: string): string => { return hexlify(Number(nonceInDec) + 1); }; -export const getTxHash = (signedTxHex: any) => { +export const getTxHash = (signedTxHex: string) => { if (!signedTxHex) { return ''; } + const hexWithoutPrefix = signedTxHex.startsWith('0x') + ? signedTxHex.slice(2) + : signedTxHex; const txHashBytes = TransactionFactory.fromSerializedData( - // eslint-disable-next-line no-restricted-globals - Uint8Array.from(Buffer.from(signedTxHex.slice(2), 'hex')), + hexToBytes(`0x${hexWithoutPrefix}`), ).hash(); return bytesToHex(txHashBytes); }; From 39351a2577c371d3d956d5b203a60d4de784599e Mon Sep 17 00:00:00 2001 From: httpjunkie Date: Tue, 25 Mar 2025 21:19:50 -0400 Subject: [PATCH 5/5] fix(utils): improve getTxHash hex string handling The getTxHash function has been enhanced to handle hex strings more robustly: - Normalizes case (upper/lower) before processing - Properly handles 0x-prefixed and unprefixed hex strings - Prevents double-prefix issues that could cause transaction failures This change makes the function more resilient to varying hex string formats that may come from different parts of the system, while maintaining consistent hash generation regardless of input format. Tests added to verify: - Prefix handling (with/without 0x) - Case consistency (upper/lower/mixed) - Hash determinism across different string representations --- src/utils.test.ts | 57 +++++++++++++++++++++++++++++++++++++++++++++++ src/utils.ts | 13 +++++++---- 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/src/utils.test.ts b/src/utils.test.ts index 5d06b53f..149e8f60 100644 --- a/src/utils.test.ts +++ b/src/utils.test.ts @@ -356,6 +356,11 @@ describe('src/utils.js', () => { }); describe('getTxHash', () => { + /** + * Core functionality test - verifies that the function correctly generates + * a deterministic hash from a valid signed transaction. This is the primary + * use case for transaction tracking in MetaMetrics. + */ it('returns a transaction hash from a signed transaction', () => { const expectedTxHash = '0x0302b75dfb9fd9eb34056af031efcaee2a8cbd799ea054a85966165cd82a7356'; @@ -363,17 +368,69 @@ describe('src/utils.js', () => { expect(txHash).toBe(expectedTxHash); }); + /** + * Safety check - ensures the function handles empty input gracefully + * instead of throwing errors. Important for defensive programming and + * preventing crashes when tracking transaction hashes. + */ it('returns an empty string if there is no signed transaction', () => { const expectedTxHash = ''; const txHash = utils.getTxHash(''); expect(txHash).toBe(expectedTxHash); }); + /** + * Error handling test - verifies that malformed transaction data + * results in appropriate error messaging rather than silent failures + * or incorrect hash generation. + */ it('throws an error with an incorrect signed transaction', () => { expect(() => { utils.getTxHash('0x0302b75dfb9fd9eb34056af0'); }).toThrow('kzg instance required to instantiate blob tx'); }); + + /** + * Format handling test - ensures the function works correctly with both + * prefixed (0x) and unprefixed hex strings. This is crucial as transaction + * data might come in either format from different parts of the system. + */ + it('handles transaction hex without 0x prefix', () => { + const withPrefix = createSignedTransaction(); + const withoutPrefix = createSignedTransaction().slice(2); + expect(utils.getTxHash(withPrefix)).toBe(utils.getTxHash(withoutPrefix)); + }); + + /** + * Consistency test - verifies that the function generates the same hash + * regardless of hex string casing. This ensures reliable transaction + * tracking even if the hex representation varies across the system. + */ + it('maintains consistent hash with different string representations', () => { + const original = createSignedTransaction(); + // Convert to upper/lower case but preserve the 0x prefix + const upperCase = `0x${original.slice(2).toUpperCase()}`; + const lowerCase = `0x${original.slice(2).toLowerCase()}`; + const hash = utils.getTxHash(original); + + expect(utils.getTxHash(upperCase)).toBe(hash); + expect(utils.getTxHash(lowerCase)).toBe(hash); + }); + + /** + * Case insensitivity test - verifies that the function handles both uppercase + * and lowercase hex characters correctly by normalizing the input. This is + * important because hex strings might be represented in different cases + * across the system, but should still produce the same hash. + */ + it('maintains consistent hash regardless of hex string case', () => { + const original = createSignedTransaction(); + // Convert the middle portion to uppercase, keeping 0x prefix lowercase + const mixedCase = `0x${original.slice(2).toUpperCase()}`; + const hash = utils.getTxHash(original); + + expect(utils.getTxHash(mixedCase)).toBe(hash); + }); }); describe('getReturnTxHashAsap', () => { diff --git a/src/utils.ts b/src/utils.ts index e4841014..92726352 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -228,11 +228,16 @@ export const getTxHash = (signedTxHex: string) => { if (!signedTxHex) { return ''; } - const hexWithoutPrefix = signedTxHex.startsWith('0x') - ? signedTxHex.slice(2) - : signedTxHex; + // First normalize the entire string to lowercase + const normalizedInput = signedTxHex.toLowerCase(); + // Then remove 0x prefix if it exists + const hexWithoutPrefix = normalizedInput.startsWith('0x') + ? normalizedInput.slice(2) + : normalizedInput; + // Add single 0x prefix + const prefixedHex = `0x${hexWithoutPrefix}`; const txHashBytes = TransactionFactory.fromSerializedData( - hexToBytes(`0x${hexWithoutPrefix}`), + hexToBytes(prefixedHex), ).hash(); return bytesToHex(txHashBytes); };