From 410570de1a99a9b317f9a3c5bf9ba9041dd00572 Mon Sep 17 00:00:00 2001 From: ricky Date: Wed, 11 Aug 2021 11:18:37 -0400 Subject: [PATCH] Allow for speedUp and stop based on provided gasValues from consumer (#535) * Allow for speedUp and stop based on provided gasValues from consumer * make this optional * validate gas values are hex string * use util * rename * update wording * Update tests * remove return true * Use type predicate functions * Move type predicate functions to util * Update unit tests * Add validation ensure tx is increased by at least the minimum required amount * Add validateMinimumIncrease method * Address nit * Add validation in stopTransaction * fix spelling * remove unnecessary optional chaining * test --- src/transaction/TransactionController.ts | 121 ++++++++++++++++++----- src/util.test.ts | 85 +++++++++++++++- src/util.ts | 36 +++++++ 3 files changed, 219 insertions(+), 23 deletions(-) diff --git a/src/transaction/TransactionController.ts b/src/transaction/TransactionController.ts index 2fa46888397..0f25e344ca6 100644 --- a/src/transaction/TransactionController.ts +++ b/src/transaction/TransactionController.ts @@ -24,6 +24,10 @@ import { query, getIncreasedPriceFromExisting, isEIP1559Transaction, + isGasPriceValue, + isFeeMarketEIP1559Values, + validateGasValues, + validateMinimumIncrease, } from '../util'; import { MAINNET, RPC } from '../constants'; @@ -79,6 +83,15 @@ export interface Transaction { estimatedBaseFee?: string; } +export interface GasPriceValue { + gasPrice: string; +} + +export interface FeeMarketEIP1559Values { + maxFeePerGas: string; + maxPriorityFeePerGas: string; +} + /** * The status of the transaction. Each status represents the state of the transaction internally * in the wallet. Some of these correspond with the state of the transaction on the network, but @@ -701,7 +714,13 @@ export class TransactionController extends BaseController< * * @param transactionID - ID of the transaction to cancel */ - async stopTransaction(transactionID: string) { + async stopTransaction( + transactionID: string, + gasValues?: GasPriceValue | FeeMarketEIP1559Values, + ) { + if (gasValues) { + validateGasValues(gasValues); + } const transactionMeta = this.state.transactions.find( ({ id }) => id === transactionID, ); @@ -713,21 +732,48 @@ export class TransactionController extends BaseController< throw new Error('No sign method defined.'); } - const gasPrice = getIncreasedPriceFromExisting( + // gasPrice (legacy non EIP1559) + const minGasPrice = getIncreasedPriceFromExisting( transactionMeta.transaction.gasPrice, CANCEL_RATE, ); + const gasPriceFromValues = isGasPriceValue(gasValues) && gasValues.gasPrice; + + const newGasPrice = + (gasPriceFromValues && + validateMinimumIncrease(gasPriceFromValues, minGasPrice)) || + minGasPrice; + + // maxFeePerGas (EIP1559) const existingMaxFeePerGas = transactionMeta.transaction?.maxFeePerGas; + const minMaxFeePerGas = getIncreasedPriceFromExisting( + existingMaxFeePerGas, + CANCEL_RATE, + ); + const maxFeePerGasValues = + isFeeMarketEIP1559Values(gasValues) && gasValues.maxFeePerGas; + const newMaxFeePerGas = + (maxFeePerGasValues && + validateMinimumIncrease(maxFeePerGasValues, minMaxFeePerGas)) || + (existingMaxFeePerGas && minMaxFeePerGas); + + // maxPriorityFeePerGas (EIP1559) const existingMaxPriorityFeePerGas = transactionMeta.transaction?.maxPriorityFeePerGas; - - const newMaxFeePerGas = - existingMaxFeePerGas && - getIncreasedPriceFromExisting(existingMaxFeePerGas, CANCEL_RATE); + const minMaxPriorityFeePerGas = getIncreasedPriceFromExisting( + existingMaxPriorityFeePerGas, + CANCEL_RATE, + ); + const maxPriorityFeePerGasValues = + isFeeMarketEIP1559Values(gasValues) && gasValues.maxPriorityFeePerGas; const newMaxPriorityFeePerGas = - existingMaxPriorityFeePerGas && - getIncreasedPriceFromExisting(existingMaxPriorityFeePerGas, CANCEL_RATE); + (maxPriorityFeePerGasValues && + validateMinimumIncrease( + maxPriorityFeePerGasValues, + minMaxPriorityFeePerGas, + )) || + (existingMaxPriorityFeePerGas && minMaxPriorityFeePerGas); const txParams = newMaxFeePerGas && newMaxPriorityFeePerGas @@ -744,7 +790,7 @@ export class TransactionController extends BaseController< : { from: transactionMeta.transaction.from, gasLimit: transactionMeta.transaction.gas, - gasPrice, + gasPrice: newGasPrice, nonce: transactionMeta.transaction.nonce, to: transactionMeta.transaction.from, value: '0x0', @@ -767,7 +813,13 @@ export class TransactionController extends BaseController< * * @param transactionID - ID of the transaction to speed up */ - async speedUpTransaction(transactionID: string) { + async speedUpTransaction( + transactionID: string, + gasValues?: GasPriceValue | FeeMarketEIP1559Values, + ) { + if (gasValues) { + validateGasValues(gasValues); + } const transactionMeta = this.state.transactions.find( ({ id }) => id === transactionID, ); @@ -782,24 +834,49 @@ export class TransactionController extends BaseController< } const { transactions } = this.state; - const gasPrice = getIncreasedPriceFromExisting( + + // gasPrice (legacy non EIP1559) + const minGasPrice = getIncreasedPriceFromExisting( transactionMeta.transaction.gasPrice, SPEED_UP_RATE, ); + const gasPriceFromValues = isGasPriceValue(gasValues) && gasValues.gasPrice; + + const newGasPrice = + (gasPriceFromValues && + validateMinimumIncrease(gasPriceFromValues, minGasPrice)) || + minGasPrice; + + // maxFeePerGas (EIP1559) const existingMaxFeePerGas = transactionMeta.transaction?.maxFeePerGas; + const minMaxFeePerGas = getIncreasedPriceFromExisting( + existingMaxFeePerGas, + SPEED_UP_RATE, + ); + const maxFeePerGasValues = + isFeeMarketEIP1559Values(gasValues) && gasValues.maxFeePerGas; + const newMaxFeePerGas = + (maxFeePerGasValues && + validateMinimumIncrease(maxFeePerGasValues, minMaxFeePerGas)) || + (existingMaxFeePerGas && minMaxFeePerGas); + + // maxPriorityFeePerGas (EIP1559) const existingMaxPriorityFeePerGas = transactionMeta.transaction?.maxPriorityFeePerGas; - - const newMaxFeePerGas = - existingMaxFeePerGas && - getIncreasedPriceFromExisting(existingMaxFeePerGas, SPEED_UP_RATE); + const minMaxPriorityFeePerGas = getIncreasedPriceFromExisting( + existingMaxPriorityFeePerGas, + SPEED_UP_RATE, + ); + const maxPriorityFeePerGasValues = + isFeeMarketEIP1559Values(gasValues) && gasValues.maxPriorityFeePerGas; const newMaxPriorityFeePerGas = - existingMaxPriorityFeePerGas && - getIncreasedPriceFromExisting( - existingMaxPriorityFeePerGas, - SPEED_UP_RATE, - ); + (maxPriorityFeePerGasValues && + validateMinimumIncrease( + maxPriorityFeePerGasValues, + minMaxPriorityFeePerGas, + )) || + (existingMaxPriorityFeePerGas && minMaxPriorityFeePerGas); const txParams = newMaxFeePerGas && newMaxPriorityFeePerGas @@ -813,7 +890,7 @@ export class TransactionController extends BaseController< : { ...transactionMeta.transaction, gasLimit: transactionMeta.transaction.gas, - gasPrice, + gasPrice: newGasPrice, }; const unsignedEthTx = this.prepareUnsignedEthTx(txParams); @@ -846,7 +923,7 @@ export class TransactionController extends BaseController< ...baseTransactionMeta, transaction: { ...transactionMeta.transaction, - gasPrice, + gasPrice: newGasPrice, }, }; transactions.push(newTransactionMeta); diff --git a/src/util.test.ts b/src/util.test.ts index b413f8edfa3..c0a343a4a91 100644 --- a/src/util.test.ts +++ b/src/util.test.ts @@ -4,12 +4,22 @@ import nock from 'nock'; import HttpProvider from 'ethjs-provider-http'; import EthQuery from 'eth-query'; import * as util from './util'; -import { Transaction } from './transaction/TransactionController'; +import { + Transaction, + GasPriceValue, + FeeMarketEIP1559Values, +} from './transaction/TransactionController'; const VALID = '4e1fF7229BDdAf0A73DF183a88d9c3a04cc975e0'; const SOME_API = 'https://someapi.com'; const SOME_FAILING_API = 'https://somefailingapi.com'; +const MAX_FEE_PER_GAS = 'maxFeePerGas'; +const MAX_PRIORITY_FEE_PER_GAS = 'maxPriorityFeePerGas'; +const GAS_PRICE = 'gasPrice'; +const FAIL = 'lol'; +const PASS = '0x1'; + const mockFlags: { [key: string]: any } = { estimateGas: null, gasPrice: null, @@ -960,4 +970,77 @@ describe('util', () => { expect(util.isEIP1559Transaction(tx)).toBe(false); }); }); + + describe('validateGasValues', () => { + it('should throw when provided invalid gas values', () => { + const gasValues: GasPriceValue = { + [GAS_PRICE]: FAIL, + }; + expect(() => util.validateGasValues(gasValues)).toThrow(TypeError); + expect(() => util.validateGasValues(gasValues)).toThrow( + `expected hex string for ${GAS_PRICE} but received: ${FAIL}`, + ); + }); + it('should throw when any provided gas values are invalid', () => { + const gasValues: FeeMarketEIP1559Values = { + [MAX_PRIORITY_FEE_PER_GAS]: PASS, + [MAX_FEE_PER_GAS]: FAIL, + }; + expect(() => util.validateGasValues(gasValues)).toThrow(TypeError); + expect(() => util.validateGasValues(gasValues)).toThrow( + `expected hex string for ${MAX_FEE_PER_GAS} but received: ${FAIL}`, + ); + }); + it('should return true when provided valid gas values', () => { + const gasValues: FeeMarketEIP1559Values = { + [MAX_FEE_PER_GAS]: PASS, + [MAX_PRIORITY_FEE_PER_GAS]: PASS, + }; + expect(() => util.validateGasValues(gasValues)).not.toThrow(TypeError); + }); + }); + + describe('isFeeMarketEIP1559Values', () => { + it('should detect if isFeeMarketEIP1559Values', () => { + const gasValues = { + [MAX_PRIORITY_FEE_PER_GAS]: PASS, + [MAX_FEE_PER_GAS]: FAIL, + }; + expect(util.isFeeMarketEIP1559Values(gasValues)).toBe(true); + expect(util.isGasPriceValue(gasValues)).toBe(false); + }); + }); + + describe('isGasPriceValue', () => { + it('should detect if isGasPriceValue', () => { + const gasValues: GasPriceValue = { + [GAS_PRICE]: PASS, + }; + expect(util.isGasPriceValue(gasValues)).toBe(true); + expect(util.isFeeMarketEIP1559Values(gasValues)).toBe(false); + }); + }); + + describe('validateMinimumIncrease', () => { + it('should throw if increase does not meet minimum requirement', () => { + expect(() => + util.validateMinimumIncrease('0x50fd51da', '0x5916a6d6'), + ).toThrow(Error); + expect(() => + util.validateMinimumIncrease('0x50fd51da', '0x5916a6d6'), + ).toThrow( + 'The proposed value: 1358778842 should meet or exceed the minimum value: 1494656726', + ); + }); + it('should not throw if increase meets minimum requirement', () => { + expect(() => + util.validateMinimumIncrease('0x5916a6d6', '0x5916a6d6'), + ).not.toThrow(Error); + }); + it('should not throw if increase exceeds minimum requirement', () => { + expect(() => + util.validateMinimumIncrease('0x7162a5ca', '0x5916a6d6'), + ).not.toThrow(Error); + }); + }); }); diff --git a/src/util.ts b/src/util.ts index b596a57914a..d1bbdb8c3e9 100644 --- a/src/util.ts +++ b/src/util.ts @@ -15,6 +15,8 @@ import { validate } from 'jsonschema'; import { Transaction, FetchAllOptions, + GasPriceValue, + FeeMarketEIP1559Values, } from './transaction/TransactionController'; import { MessageParams } from './message-manager/MessageManager'; import { PersonalMessageParams } from './message-manager/PersonalMessageManager'; @@ -710,6 +712,40 @@ export const getIncreasedPriceFromExisting = ( return getIncreasedPriceHex(convertPriceToDecimal(value), rate); }; +export const validateGasValues = ( + gasValues: GasPriceValue | FeeMarketEIP1559Values, +) => { + Object.keys(gasValues).forEach((key) => { + const value = (gasValues as any)[key]; + if (typeof value !== 'string' || !isHexString(value)) { + throw new TypeError( + `expected hex string for ${key} but received: ${value}`, + ); + } + }); +}; + +export const isFeeMarketEIP1559Values = ( + gasValues?: GasPriceValue | FeeMarketEIP1559Values, +): gasValues is FeeMarketEIP1559Values => + (gasValues as FeeMarketEIP1559Values)?.maxFeePerGas !== undefined || + (gasValues as FeeMarketEIP1559Values)?.maxPriorityFeePerGas !== undefined; + +export const isGasPriceValue = ( + gasValues?: GasPriceValue | FeeMarketEIP1559Values, +): gasValues is GasPriceValue => + (gasValues as GasPriceValue)?.gasPrice !== undefined; + +export function validateMinimumIncrease(proposed: string, min: string) { + const proposedDecimal = convertPriceToDecimal(proposed); + const minDecimal = convertPriceToDecimal(min); + if (proposedDecimal >= minDecimal) { + return proposed; + } + const errorMsg = `The proposed value: ${proposedDecimal} should meet or exceed the minimum value: ${minDecimal}`; + throw new Error(errorMsg); +} + export default { BNToHex, fractionBN,