From 7796d3521d0e8bc958b30484823a847b6716a401 Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Mon, 12 Aug 2024 10:29:05 -0400 Subject: [PATCH] fix(TXL-207): fixes gaps in transaction validation and async error logging (#4596) ## Explanation This PR repairs some gaps in our validation of transaction parameters: * validate that if a transaction envelope type is specified that it is a valid type * validate transaction parameters before right before adding transaction to state * catch and log async errors Notes * The steps to reproduce in https://github.com/MetaMask/metamask-extension/issues/23180 revealed an async error that was being swallowed. ## References This PR will fix these bugs once clients are updated: * Related: https://github.com/MetaMask/MetaMask-planning/issues/2341 * Related: https://github.com/MetaMask/metamask-extension/issues/23180 ## Changelog ### `@metamask/transaction-controller` - **FIXED**: Fixes gaps in transaction parameter validation and async error logging ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- .../transaction-controller/jest.config.js | 6 ++-- .../src/TransactionController.ts | 29 ++++++++++++------- .../src/utils/validation.test.ts | 9 ++++++ .../src/utils/validation.ts | 22 ++++++++++++++ 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index 5b59faf5b8..c228664b80 100644 --- a/packages/transaction-controller/jest.config.js +++ b/packages/transaction-controller/jest.config.js @@ -18,9 +18,9 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 93.44, - functions: 98.4, - lines: 98.72, - statements: 98.73, + functions: 97.39, + lines: 98.4, + statements: 98.42, }, }, diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 5a76be847e..478e93a8cd 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -1110,8 +1110,10 @@ export class TransactionController extends BaseController< this.addMetadata(addedTransactionMeta); if (requireApproval !== false) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.#updateSimulationData(addedTransactionMeta); + this.#updateSimulationData(addedTransactionMeta).catch((error) => { + log('Error while updating simulation data', error); + throw error; + }); } else { log('Skipping simulation as approval not required'); } @@ -1694,8 +1696,10 @@ export class TransactionController extends BaseController< this.onTransactionStatusChange(updatedTransactionMeta); // Intentional given potential duration of process. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.updatePostBalance(updatedTransactionMeta); + this.updatePostBalance(updatedTransactionMeta).catch((error) => { + log('Error while updating post balance', error); + throw error; + }); this.messagingSystem.publish( `${controllerName}:transactionConfirmed`, @@ -2429,6 +2433,7 @@ export class TransactionController extends BaseController< } private addMetadata(transactionMeta: TransactionMeta) { + validateTxParams(transactionMeta.txParams); this.update((state) => { state.transactions = this.trimTransactionsForState([ ...state.transactions, @@ -2439,8 +2444,8 @@ export class TransactionController extends BaseController< private async updateGasProperties(transactionMeta: TransactionMeta) { const isEIP1559Compatible = - (await this.getEIP1559Compatibility(transactionMeta.networkClientId)) && - transactionMeta.txParams.type !== TransactionEnvelopeType.legacy; + transactionMeta.txParams.type !== TransactionEnvelopeType.legacy && + (await this.getEIP1559Compatibility(transactionMeta.networkClientId)); const { networkClientId, chainId } = transactionMeta; @@ -3358,8 +3363,10 @@ export class TransactionController extends BaseController< this.onTransactionStatusChange(transactionMeta); // Intentional given potential duration of process. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.updatePostBalance(transactionMeta); + this.updatePostBalance(transactionMeta).catch((error) => { + log('Error while updating post balance', error); + throw error; + }); } private async updatePostBalance(transactionMeta: TransactionMeta) { @@ -3710,8 +3717,10 @@ export class TransactionController extends BaseController< ) ) { log('Updating simulation data due to transaction parameter update'); - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.#updateSimulationData(transactionMeta); + this.#updateSimulationData(transactionMeta).catch((error) => { + log('Error updating simulation data', error); + throw error; + }); } } diff --git a/packages/transaction-controller/src/utils/validation.test.ts b/packages/transaction-controller/src/utils/validation.test.ts index e26175022b..d5127ba50d 100644 --- a/packages/transaction-controller/src/utils/validation.test.ts +++ b/packages/transaction-controller/src/utils/validation.test.ts @@ -5,6 +5,15 @@ import { validateTxParams } from './validation'; describe('validation', () => { describe('validateTxParams', () => { + it('should throw if unknown transaction envelope type is specified', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(() => validateTxParams({ type: '0x3' } as any)).toThrow( + rpcErrors.invalidParams( + 'Invalid transaction envelope type: "0x3". Must be one of: 0x0, 0x1, 0x2', + ), + ); + }); + it('should throw if no from address', () => { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/packages/transaction-controller/src/utils/validation.ts b/packages/transaction-controller/src/utils/validation.ts index 7cf5e389f8..4752502017 100644 --- a/packages/transaction-controller/src/utils/validation.ts +++ b/packages/transaction-controller/src/utils/validation.ts @@ -55,6 +55,7 @@ export function validateTxParams( txParams: TransactionParams, isEIP1559Compatible = true, ) { + validateEnvelopeType(txParams.type); validateEIP1559Compatibility(txParams, isEIP1559Compatible); validateParamFrom(txParams.from); validateParamRecipient(txParams); @@ -64,6 +65,27 @@ export function validateTxParams( validateGasFeeParams(txParams); } +/** + * Validates the `type` property, ensuring that if it is specified, it is a valid transaction envelope type. + * + * @param type - The transaction envelope type to validate. + * @throws Throws invalid params if the type is not a valid transaction envelope type. + */ +function validateEnvelopeType(type: string | undefined) { + if ( + type && + !Object.values(TransactionEnvelopeType).includes( + type as TransactionEnvelopeType, + ) + ) { + throw rpcErrors.invalidParams( + `Invalid transaction envelope type: "${type}". Must be one of: ${Object.values( + TransactionEnvelopeType, + ).join(', ')}`, + ); + } +} + /** * Validates EIP-1559 compatibility for transaction creation. *