Skip to content

Commit

Permalink
Improve comments, tests and edge case handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@danjm
danjm committed Jul 28, 2021
1 parent 982aa55 commit 0ce50e8
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 13 deletions.
61 changes: 49 additions & 12 deletions app/scripts/controllers/transactions/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,6 @@ export default class TransactionController extends EventEmitter {
maxFeePerGas: defaultMaxFeePerGas,
maxPriorityFeePerGas: defaultMaxPriorityFeePerGas,
} = await this._getDefaultGasFees(txMeta, eip1559Compatibility);

const {
gasLimit: defaultGasLimit,
simulationFails,
Expand All @@ -414,32 +413,65 @@ export default class TransactionController extends EventEmitter {
}

if (eip1559Compatibility) {
// If the dapp has suggested a gas price, but no maxFeePerGas or maxPriorityFeePerGas
// then we set maxFeePerGas and maxPriorityFeePerGas to the suggested gasPrice.
if (
txMeta.txParams.gasPrice &&
!txMeta.txParams.maxFeePerGas &&
!txMeta.txParams.maxPriorityFeePerGas
) {
txMeta.txParams.maxFeePerGas = txMeta.txParams.gasPrice;
txMeta.txParams.maxPriorityFeePerGas = txMeta.txParams.gasPrice;
}
} else {
if (defaultMaxFeePerGas && !txMeta.txParams.maxFeePerGas) {
// If the dapp has not set the gasPrice or the maxFeePerGas, then we set maxFeePerGas
// with the one returned by the gasFeeController, if that is available.
txMeta.txParams.maxFeePerGas = defaultMaxFeePerGas;
}

if (defaultMaxFeePerGas && !txMeta.txParams.maxFeePerGas) {
txMeta.txParams.maxFeePerGas = defaultMaxFeePerGas;
}
if (
defaultMaxPriorityFeePerGas &&
!txMeta.txParams.maxPriorityFeePerGas
) {
// If the dapp has not set the gasPrice or the maxPriorityFeePerGas, then we set maxPriorityFeePerGas
// with the one returned by the gasFeeController, if that is available.
txMeta.txParams.maxPriorityFeePerGas = defaultMaxPriorityFeePerGas;
}

if (
defaultMaxPriorityFeePerGas &&
!txMeta.txParams.maxPriorityFeePerGas
) {
txMeta.txParams.maxPriorityFeePerGas = defaultMaxPriorityFeePerGas;
if (defaultGasPrice && !txMeta.txParams.maxFeePerGas) {
// If the dapp has not set the gasPrice or the maxFeePerGas, and no maxFeePerGas is available
// from the gasFeeController, then we set maxFeePerGas to the defaultGasPrice, assuming it is
// available.
txMeta.txParams.maxFeePerGas = defaultGasPrice;
}

if (
txMeta.txParams.maxFeePerGas &&
!txMeta.txParams.maxPriorityFeePerGas
) {
// If the dapp has not set the gasPrice or the maxPriorityFeePerGas, and no maxPriorityFeePerGas is
// available from the gasFeeController, then we set maxPriorityFeePerGas to
// txMeta.txParams.maxFeePerGas, which will either be the gasPrice from the controller, the maxFeePerGas
// set by the dapp, or the maxFeePerGas from the controller.
txMeta.txParams.maxPriorityFeePerGas = txMeta.txParams.maxFeePerGas;
}
}

// We remove the gasPrice param entirely when on an eip1559 compatible network

delete txMeta.txParams.gasPrice;
} else {
// We ensure that maxFeePerGas and maxPriorityFeePerGas are not in the transaction params
// when not on a EIP1559 compatible network

delete txMeta.txParams.maxPriorityFeePerGas;
delete txMeta.txParams.maxFeePerGas;
}

// If we have gotten to this point, and none of gasPrice, maxPriorityFeePerGas or maxFeePerGas are
// set on txParams, it means that either we are on a non-EIP1559 network and the dapp didn't suggest
// a gas price, or we are on an EIP1559 network, and none of gasPrice, maxPriorityFeePerGas or maxFeePerGas
// were available from either the dapp or the network.
if (
defaultGasPrice &&
!txMeta.txParams.gasPrice &&
Expand All @@ -448,14 +480,15 @@ export default class TransactionController extends EventEmitter {
) {
txMeta.txParams.gasPrice = defaultGasPrice;
}

if (defaultGasLimit && !txMeta.txParams.gas) {
txMeta.txParams.gas = defaultGasLimit;
}
return txMeta;
}

/**
* Gets default gas price, or returns `undefined` if gas price is already set
* Gets default gas fees, or returns `undefined` if gas fees are already set
* @param {Object} txMeta - The txMeta object
* @returns {Promise<string|undefined>} The default gas price
*/
Expand Down Expand Up @@ -489,10 +522,14 @@ export default class TransactionController extends EventEmitter {
};
}
} else if (gasEstimateType === GAS_ESTIMATE_TYPES.LEGACY) {
// The LEGACY type includes low, medium and high estimates of
// gas price values.
return {
gasPrice: decGWEIToHexWEI(gasFeeEstimates.medium),
};
} else if (gasEstimateType === GAS_ESTIMATE_TYPES.ETH_GASPRICE) {
// The ETH_GASPRICE type just includes a single gas price property,
// which we can assume was retrieved from eth_gasPrice
return {
gasPrice: decGWEIToHexWEI(gasFeeEstimates.gasPrice),
};
Expand All @@ -503,7 +540,7 @@ export default class TransactionController extends EventEmitter {

const gasPrice = await this.query.gasPrice();

return { gasPrice: addHexPrefix(gasPrice.toString(16)) };
return { gasPrice: gasPrice && addHexPrefix(gasPrice.toString(16)) };
}

/**
Expand Down
110 changes: 109 additions & 1 deletion app/scripts/controllers/transactions/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('Transaction Controller', function () {
return '0xee6b2800';
},
networkStore: new ObservableStore(currentNetworkId),
getEIP1559Compatibility: () => Promise.resolve(true),
getEIP1559Compatibility: () => Promise.resolve(false),
txHistoryLimit: 10,
blockTracker: blockTrackerStub,
signTransaction: (ethTx) =>
Expand Down Expand Up @@ -473,6 +473,110 @@ describe('Transaction Controller', function () {
stub1.restore();
stub2.restore();
});

it('should add gasPrice as maxFeePerGas and maxPriorityFeePerGas if there are no sources of other fee data available', async function () {
const TEST_GASPRICE = '0x12a05f200';

const stub1 = sinon
.stub(txController, 'getEIP1559Compatibility')
.returns(true);

const stub2 = sinon
.stub(txController, '_getDefaultGasFees')
.callsFake(() => ({ gasPrice: TEST_GASPRICE }));

txController.txStateManager._addTransactionsToState([
{
id: 1,
status: TRANSACTION_STATUSES.UNAPPROVED,
metamaskNetworkId: currentNetworkId,
txParams: {
to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
},
history: [{}],
},
]);
const txMeta = {
id: 1,
txParams: {
from: '0xc684832530fcbddae4b4230a47e991ddcec2831d',
to: '0xc684832530fcbddae4b4230a47e991ddcec2831d',
},
history: [{}],
};
providerResultStub.eth_getBlockByNumber = { gasLimit: '47b784' };
providerResultStub.eth_estimateGas = '5209';

const txMetaWithDefaults = await txController.addTxGasDefaults(txMeta);

assert.equal(
txMetaWithDefaults.txParams.maxFeePerGas,
TEST_GASPRICE,
'should have added the correct max fee per gas',
);
assert.equal(
txMetaWithDefaults.txParams.maxPriorityFeePerGas,
TEST_GASPRICE,
'should have added the correct max priority fee per gas',
);
stub1.restore();
stub2.restore();
});

it('should not add gasPrice if the fee data is available from the dapp', async function () {
const TEST_GASPRICE = '0x12a05f200';
const TEST_MAX_FEE_PER_GAS = '0x12a05f200';
const TEST_MAX_PRIORITY_FEE_PER_GAS = '0x77359400';

const stub1 = sinon
.stub(txController, 'getEIP1559Compatibility')
.returns(true);

const stub2 = sinon
.stub(txController, '_getDefaultGasFees')
.callsFake(() => ({ gasPrice: TEST_GASPRICE }));

txController.txStateManager._addTransactionsToState([
{
id: 1,
status: TRANSACTION_STATUSES.UNAPPROVED,
metamaskNetworkId: currentNetworkId,
txParams: {
to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
maxFeePerGas: TEST_MAX_FEE_PER_GAS,
maxPriorityFeePerGas: TEST_MAX_PRIORITY_FEE_PER_GAS,
},
history: [{}],
},
]);
const txMeta = {
id: 1,
txParams: {
from: '0xc684832530fcbddae4b4230a47e991ddcec2831d',
to: '0xc684832530fcbddae4b4230a47e991ddcec2831d',
},
history: [{}],
};
providerResultStub.eth_getBlockByNumber = { gasLimit: '47b784' };
providerResultStub.eth_estimateGas = '5209';

const txMetaWithDefaults = await txController.addTxGasDefaults(txMeta);

assert.equal(
txMetaWithDefaults.txParams.maxFeePerGas,
TEST_MAX_FEE_PER_GAS,
'should have added the correct max fee per gas',
);
assert.equal(
txMetaWithDefaults.txParams.maxPriorityFeePerGas,
TEST_MAX_PRIORITY_FEE_PER_GAS,
'should have added the correct max priority fee per gas',
);
stub1.restore();
stub2.restore();
});
});

describe('_getDefaultGasFees', function () {
Expand Down Expand Up @@ -934,6 +1038,9 @@ describe('Transaction Controller', function () {
});

it('sets txParams.type to 0x2 (EIP-1559)', async function () {
const eip1559CompatibilityStub = sinon
.stub(txController, 'getEIP1559Compatibility')
.returns(true);
txController.txStateManager._addTransactionsToState([
{
status: TRANSACTION_STATUSES.UNAPPROVED,
Expand All @@ -952,6 +1059,7 @@ describe('Transaction Controller', function () {
]);
await txController.signTransaction('2');
assert.equal(fromTxDataSpy.getCall(0).args[0].type, '0x2');
eip1559CompatibilityStub.restore();
});
});

Expand Down

0 comments on commit 0ce50e8

Please sign in to comment.