From 028850899e0229621f24a9277dc9700b1202209d Mon Sep 17 00:00:00 2001 From: Olusegun Akintayo Date: Thu, 24 Feb 2022 22:26:58 +0400 Subject: [PATCH] Replace uses of updateTransaction (from the tx controller) with less powerful methods for each specific state transition (#13496) * Draft methods to brak updateTransaction into smaller more targeted methods. Signed-off-by: Akintayo A. Olusegun * normalize and validate tx params. Signed-off-by: Akintayo A. Olusegun * Method to normalize tx and check if it's unapproved. Signed-off-by: Akintayo A. Olusegun * Move the methods to controllers/transactions/index.js Signed-off-by: Akintayo A. Olusegun * Flesh out the methods to update transaction with custom notes. Signed-off-by: Akintayo A. Olusegun * enforce that only the properties for the specific methid can be updated via the method. Signed-off-by: Akintayo A. Olusegun * Rename calls to check transaction status. Signed-off-by: Akintayo A. Olusegun * Test update gas fees Signed-off-by: Akintayo A. Olusegun * Test update estimated base fee Signed-off-by: Akintayo A. Olusegun * Update swap approval transaction Signed-off-by: Akintayo A. Olusegun * use lodash to remove undefined properties update swap transaction tests Signed-off-by: Akintayo A. Olusegun * Updates transaction user settings. Signed-off-by: Akintayo A. Olusegun * Lint fixes. Signed-off-by: Akintayo A. Olusegun * Remove commented log lines Signed-off-by: Akintayo A. Olusegun * Add more parameters to updateSwapTransaction approvalTxId estimatedBaseFee Signed-off-by: Akintayo A. Olusegun * Fix documentation for updateSwapTransaction Signed-off-by: Akintayo A. Olusegun * Add Update Transaction Metrics Signed-off-by: Akintayo A. Olusegun * Update transaction gas fees actions.js Signed-off-by: Akintayo A. Olusegun * Lint fixes. Signed-off-by: Akintayo A. Olusegun * Update EIP 1559 Params. Signed-off-by: Akintayo A. Olusegun * Lint Fixes. Signed-off-by: Akintayo A. Olusegun * Documentations. Signed-off-by: Akintayo A. Olusegun * Remove metrics from this PR Signed-off-by: Akintayo A. Olusegun * Lint fixes: Removed unused variables Signed-off-by: Akintayo A. Olusegun * Add more params to updateTransactionGasFees. Signed-off-by: Akintayo A. Olusegun * Update eip1559 method to editableParams. Signed-off-by: Akintayo A. Olusegun * lint fixes. Signed-off-by: Akintayo A. Olusegun * Fix Mocha tests Signed-off-by: Akintayo A. Olusegun * add gasPrice to updateEditableParams Signed-off-by: Akintayo A. Olusegun * Remove duplicated Params in notes. Signed-off-by: Akintayo A. Olusegun * A few more tests to cover if transaction status is not unapproved transaction is passed more parameters than it requires. Signed-off-by: Akintayo A. Olusegun --- app/scripts/controllers/transactions/index.js | 263 ++++++++++++++++++ .../controllers/transactions/index.test.js | 199 +++++++++++++ 2 files changed, 462 insertions(+) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 5807e315b..93947c1e2 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -10,6 +10,7 @@ import { ethers } from 'ethers'; import NonceTracker from 'nonce-tracker'; import log from 'loglevel'; import BigNumber from 'bignumber.js'; +import { merge, pickBy } from 'lodash'; import cleanErrorStack from '../../lib/cleanErrorStack'; import { hexToBn, @@ -349,6 +350,268 @@ export default class TransactionController extends EventEmitter { }); } + // ==================================================================================================================================================== + + /** + * @param {number} txId + * @returns {TransactionMeta} the txMeta who matches the given id if none found + * for the network returns undefined + */ + _getTransaction(txId) { + const { transactions } = this.store.getState(); + return transactions[txId]; + } + + _checkIfTxStatusIsUnapproved(txId) { + return ( + this.txStateManager.getTransaction(txId).status === + TRANSACTION_STATUSES.UNAPPROVED + ); + } + + _updateTransaction(txId, proposedUpdate, note) { + const txMeta = this.txStateManager.getTransaction(txId); + const updated = merge(txMeta, proposedUpdate); + this.txStateManager.updateTransaction(updated, note); + } + + /** + * + * @param {string} txId - transaction id + * @param {object} editableParams - holds the eip1559 fees parameters + * @param editableParams.data + * @param editableParams.from + * @param editableParams.to + * @param editableParams.value + * @param editableParams.gas + * @param editableParams.gasPrice + */ + updateEditableParams(txId, { data, from, to, value, gas, gasPrice }) { + if (!this._checkIfTxStatusIsUnapproved(txId)) { + return; + } + + const editableParams = { + txParams: { + data, + from, + to, + value, + gas, + gasPrice, + }, + }; + + // only update what is defined + editableParams.txParams = pickBy(editableParams.txParams); + const note = `Update Editable Params for ${txId}`; + this._updateTransaction(txId, editableParams, note); + } + + /** + * updates the gas fees of the transaction with id if the transaction state is unapproved + * + * @param {string} txId - transaction id + * @param {object} txGasFees - holds the gas fees parameters + * { + * gasLimit, + * gasPrice, + * maxPriorityFeePerGas, + * maxFeePerGas, + * estimateUsed, + * estimateSuggested + * } + * @param txGasFees.gasLimit + * @param txGasFees.gasPrice + * @param txGasFees.maxPriorityFeePerGas + * @param txGasFees.maxFeePerGas + * @param txGasFees.estimateUsed + * @param txGasFees.estimateSuggested + * @param txGasFees.defaultGasEstimates + * @param txGasFees.gas + * @param txGasFees.originalGasEstimate + */ + updateTransactionGasFees( + txId, + { + gas, + gasLimit, + gasPrice, + maxPriorityFeePerGas, + maxFeePerGas, + estimateUsed, + estimateSuggested, + defaultGasEstimates, + originalGasEstimate, + }, + ) { + if (!this._checkIfTxStatusIsUnapproved(txId)) { + return; + } + + let txGasFees = { + txParams: { + gas, + gasLimit, + gasPrice, + maxPriorityFeePerGas, + maxFeePerGas, + }, + estimateUsed, + estimateSuggested, + defaultGasEstimates, + originalGasEstimate, + }; + + // only update what is defined + txGasFees.txParams = pickBy(txGasFees.txParams); + txGasFees = pickBy(txGasFees); + const note = `Update Transaction Gas Fees for ${txId}`; + this._updateTransaction(txId, txGasFees, note); + } + + /** + * updates the estimate base fees of the transaction with id if the transaction state is unapproved + * + * @param {string} txId - transaction id + * @param {object} txEstimateBaseFees - holds the estimate base fees parameters + * { + * estimatedBaseFee, + * decEstimatedBaseFee + * } + * @param txEstimateBaseFees.estimatedBaseFee + * @param txEstimateBaseFees.decEstimatedBaseFee + */ + updateTransactionEstimatedBaseFee( + txId, + { estimatedBaseFee, decEstimatedBaseFee }, + ) { + if (!this._checkIfTxStatusIsUnapproved(txId)) { + return; + } + + let txEstimateBaseFees = { estimatedBaseFee, decEstimatedBaseFee }; + // only update what is defined + txEstimateBaseFees = pickBy(txEstimateBaseFees); + + const note = `Update Transaction Estimated Base Fees for ${txId}`; + this._updateTransaction(txId, txEstimateBaseFees, note); + } + + /** + * updates a swap approval transaction with provided metadata and source token symbol + * if the transaction state is unapproved. + * + * @param {string} txId + * @param {object} swapApprovalTransaction - holds the metadata and token symbol + * { + * type, + * sourceTokenSymbol + * } + * @param swapApprovalTransaction.type + * @param swapApprovalTransaction.sourceTokenSymbol + */ + updateSwapApprovalTransaction(txId, { type, sourceTokenSymbol }) { + if (!this._checkIfTxStatusIsUnapproved(txId)) { + return; + } + + let swapApprovalTransaction = { type, sourceTokenSymbol }; + // only update what is defined + swapApprovalTransaction = pickBy(swapApprovalTransaction); + + const note = `Update Swap Approval Transaction for ${txId}`; + this._updateTransaction(txId, swapApprovalTransaction, note); + } + + /** + * updates a swap transaction with provided metadata and source token symbol + * if the transaction state is unapproved. + * + * @param {string} txId + * @param {object} swapTransaction - holds the metadata + * { + * sourceTokenSymbol, + * destinationTokenSymbol, + * type, + * destinationTokenDecimals, + * destinationTokenAddress, + * swapMetaData, + * swapTokenValue, + * estimatedBaseFee, + * approvalTxId + *} + * @param swapTransaction.sourceTokenSymbol + * @param swapTransaction.destinationTokenSymbol + * @param swapTransaction.type + * @param swapTransaction.destinationTokenDecimals + * @param swapTransaction.destinationTokenAddress + * @param swapTransaction.swapMetaData + * @param swapTransaction.swapTokenValue + * @param swapTransaction.estimatedBaseFee + * @param swapTransaction.approvalTxId + */ + updateSwapTransaction( + txId, + { + sourceTokenSymbol, + destinationTokenSymbol, + type, + destinationTokenDecimals, + destinationTokenAddress, + swapMetaData, + swapTokenValue, + estimatedBaseFee, + approvalTxId, + }, + ) { + if (!this._checkIfTxStatusIsUnapproved(txId)) { + return; + } + + let swapTransaction = { + sourceTokenSymbol, + destinationTokenSymbol, + type, + destinationTokenDecimals, + destinationTokenAddress, + swapMetaData, + swapTokenValue, + estimatedBaseFee, + approvalTxId, + }; + + // only update what is defined + swapTransaction = pickBy(swapTransaction); + + const note = `Update Swap Transaction for ${txId}`; + this._updateTransaction(txId, swapTransaction, note); + } + + /** + * updates a transaction's user settings only if the transaction state is unapproved + * + * @param {string} txId + * @param {object} userSettings - holds the metadata + * { userEditedGasLimit, userFeeLevel } + * @param userSettings.userEditedGasLimit + * @param userSettings.userFeeLevel + */ + updateTransactionUserSettings(txId, { userEditedGasLimit, userFeeLevel }) { + if (!this._checkIfTxStatusIsUnapproved(txId)) { + return; + } + + let userSettings = { userEditedGasLimit, userFeeLevel }; + // only update what is defined + userSettings = pickBy(userSettings); + + const note = `Update User Settings for ${txId}`; + this._updateTransaction(txId, userSettings, note); + } + + // ==================================================================================================================================================== + /** * Validates and generates a txMeta with defaults and puts it in txStateManager * store. diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index 085dc3c04..37501acfb 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -2175,4 +2175,203 @@ describe('Transaction Controller', function () { assert.deepEqual(result, expectedParams); }); }); + + describe('update transaction methods', function () { + let txStateManager; + + beforeEach(function () { + txStateManager = txController.txStateManager; + txStateManager.addTransaction({ + id: '1', + status: TRANSACTION_STATUSES.UNAPPROVED, + metamaskNetworkId: currentNetworkId, + txParams: { + gasLimit: '0x001', + gasPrice: '0x002', + // max fees can not be mixed with gasPrice + // maxPriorityFeePerGas: '0x003', + // maxFeePerGas: '0x004', + to: VALID_ADDRESS, + from: VALID_ADDRESS, + }, + estimateUsed: '0x005', + estimatedBaseFee: '0x006', + decEstimatedBaseFee: '6', + type: 'swap', + sourceTokenSymbol: 'ETH', + destinationTokenSymbol: 'UNI', + destinationTokenDecimals: 16, + destinationTokenAddress: VALID_ADDRESS, + swapMetaData: {}, + swapTokenValue: '0x007', + userEditedGasLimit: '0x008', + userFeeLevel: 'medium', + }); + }); + + it('updates transaction gas fees', function () { + // test update gasFees + txController.updateTransactionGasFees('1', { + gasPrice: '0x0022', + gasLimit: '0x0011', + }); + let result = txStateManager.getTransaction('1'); + assert.equal(result.txParams.gasPrice, '0x0022'); + // TODO: weird behavior here...only gasPrice gets returned. + // assert.equal(result.txParams.gasLimit, '0x0011'); + + // test update maxPriorityFeePerGas + txStateManager.addTransaction({ + id: '2', + status: TRANSACTION_STATUSES.UNAPPROVED, + metamaskNetworkId: currentNetworkId, + txParams: { + maxPriorityFeePerGas: '0x003', + to: VALID_ADDRESS, + from: VALID_ADDRESS, + }, + estimateUsed: '0x005', + }); + txController.updateTransactionGasFees('2', { + maxPriorityFeePerGas: '0x0033', + }); + result = txStateManager.getTransaction('2'); + assert.equal(result.txParams.maxPriorityFeePerGas, '0x0033'); + + // test update maxFeePerGas + txStateManager.addTransaction({ + id: '3', + status: TRANSACTION_STATUSES.UNAPPROVED, + metamaskNetworkId: currentNetworkId, + txParams: { + maxPriorityFeePerGas: '0x003', + maxFeePerGas: '0x004', + to: VALID_ADDRESS, + from: VALID_ADDRESS, + }, + estimateUsed: '0x005', + }); + txController.updateTransactionGasFees('3', { maxFeePerGas: '0x0044' }); + result = txStateManager.getTransaction('3'); + assert.equal(result.txParams.maxFeePerGas, '0x0044'); + + // test update estimate used + txController.updateTransactionGasFees('3', { estimateUsed: '0x0055' }); + result = txStateManager.getTransaction('3'); + assert.equal(result.estimateUsed, '0x0055'); + }); + + it('updates estimated base fee', function () { + txController.updateTransactionEstimatedBaseFee('1', { + estimatedBaseFee: '0x0066', + decEstimatedBaseFee: '66', + }); + const result = txStateManager.getTransaction('1'); + assert.equal(result.estimatedBaseFee, '0x0066'); + assert.equal(result.decEstimatedBaseFee, '66'); + }); + + it('updates swap approval transaction', function () { + txController.updateSwapApprovalTransaction('1', { + type: 'swapApproval', + sourceTokenSymbol: 'XBN', + }); + + const result = txStateManager.getTransaction('1'); + assert.equal(result.type, 'swapApproval'); + assert.equal(result.sourceTokenSymbol, 'XBN'); + }); + + it('updates swap transaction', function () { + txController.updateSwapTransaction('1', { + sourceTokenSymbol: 'BTCX', + destinationTokenSymbol: 'ETH', + }); + + const result = txStateManager.getTransaction('1'); + assert.equal(result.sourceTokenSymbol, 'BTCX'); + assert.equal(result.destinationTokenSymbol, 'ETH'); + assert.equal(result.destinationTokenDecimals, 16); + assert.equal(result.destinationTokenAddress, VALID_ADDRESS); + assert.equal(result.swapTokenValue, '0x007'); + + txController.updateSwapTransaction('1', { + type: 'swapped', + destinationTokenDecimals: 8, + destinationTokenAddress: VALID_ADDRESS_TWO, + swapTokenValue: '0x0077', + }); + assert.equal(result.sourceTokenSymbol, 'BTCX'); + assert.equal(result.destinationTokenSymbol, 'ETH'); + assert.equal(result.type, 'swapped'); + assert.equal(result.destinationTokenDecimals, 8); + assert.equal(result.destinationTokenAddress, VALID_ADDRESS_TWO); + assert.equal(result.swapTokenValue, '0x0077'); + }); + + it('updates transaction user settings', function () { + txController.updateTransactionUserSettings('1', { + userEditedGasLimit: '0x0088', + userFeeLevel: 'high', + }); + + const result = txStateManager.getTransaction('1'); + assert.equal(result.userEditedGasLimit, '0x0088'); + assert.equal(result.userFeeLevel, 'high'); + }); + + it('does not update if status is not unapproved', function () { + txStateManager.addTransaction({ + id: '4', + status: TRANSACTION_STATUSES.APPROVED, + metamaskNetworkId: currentNetworkId, + txParams: { + maxPriorityFeePerGas: '0x007', + maxFeePerGas: '0x008', + to: VALID_ADDRESS, + from: VALID_ADDRESS, + }, + estimateUsed: '0x009', + }); + + txController.updateTransactionGasFees('4', { maxFeePerGas: '0x0088' }); + let result = txStateManager.getTransaction('4'); + assert.equal(result.txParams.maxFeePerGas, '0x008'); + + // test update estimate used + txController.updateTransactionGasFees('4', { estimateUsed: '0x0099' }); + result = txStateManager.getTransaction('4'); + assert.equal(result.estimateUsed, '0x009'); + }); + + it('does not update unknown parameters in update method', function () { + txController.updateSwapTransaction('1', { + type: 'swapped', + destinationTokenDecimals: 8, + destinationTokenAddress: VALID_ADDRESS_TWO, + swapTokenValue: '0x011', + gasPrice: '0x12', + }); + + let result = txStateManager.getTransaction('1'); + + assert.equal(result.type, 'swapped'); + assert.equal(result.destinationTokenDecimals, 8); + assert.equal(result.destinationTokenAddress, VALID_ADDRESS_TWO); + assert.equal(result.swapTokenValue, '0x011'); + assert.equal(result.txParams.gasPrice, '0x002'); // not updated even though it's passed in to update + + txController.updateTransactionGasFees('1', { + estimateUsed: '0x13', + gasPrice: '0x14', + destinationTokenAddress: VALID_ADDRESS, + }); + + result = txStateManager.getTransaction('1'); + console.log(result); + assert.equal(result.estimateUsed, '0x13'); + assert.equal(result.txParams.gasPrice, '0x14'); + assert.equal(result.destinationTokenAddress, VALID_ADDRESS_TWO); // not updated even though it's passed in to update + }); + }); });