Skip to content

Commit

Permalink
Ensure that eth_estimateGas is called to estimate gas limit for simpl…
Browse files Browse the repository at this point in the history
…e sends on custom networks (#11418)

* Ensure that eth_estimateGas is called to estimate gas limit for simple sends on custom networks

* getIsNonStandardEthChain returns false when in test

* Add comment explaining gas limit buffer multipliers in estimateGasLimitForSend
  • Loading branch information
danjm authored Jul 1, 2021
1 parent 9889d07 commit a603016
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 14 deletions.
55 changes: 41 additions & 14 deletions ui/ducks/send/send.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
getIsMainnet,
getSelectedAddress,
getTargetAccount,
getIsNonStandardEthChain,
} from '../../selectors';
import {
displayWarning,
Expand Down Expand Up @@ -174,8 +175,11 @@ async function estimateGasLimitForSend({
sendToken,
to,
data,
isNonStandardEthChain,
...options
}) {
let isSimpleSendOnNonStandardNetwork = false;

// blockGasLimit may be a falsy, but defined, value when we receive it from
// state, so we use logical or to fall back to MIN_GAS_LIMIT_HEX.
const blockGasLimit = options.blockGasLimit || MIN_GAS_LIMIT_HEX;
Expand Down Expand Up @@ -211,8 +215,10 @@ async function estimateGasLimitForSend({
// Geth will return '0x', and ganache-core v2.2.1 will return '0x0'
const contractCodeIsEmpty =
!contractCode || contractCode === '0x' || contractCode === '0x0';
if (contractCodeIsEmpty) {
if (contractCodeIsEmpty && !isNonStandardEthChain) {
return GAS_LIMITS.SIMPLE;
} else if (contractCodeIsEmpty && isNonStandardEthChain) {
isSimpleSendOnNonStandardNetwork = true;
}
}

Expand All @@ -231,25 +237,41 @@ async function estimateGasLimitForSend({
}
}

// If we do not yet have a gasLimit, we must call into our background
// process to get an estimate for gasLimit based on known parameters.

paramsForGasEstimate.gas = addHexPrefix(
multiplyCurrencies(blockGasLimit, 0.95, {
multiplicandBase: 16,
multiplierBase: 10,
roundDown: '0',
toNumericBase: 'hex',
}),
);
if (!isSimpleSendOnNonStandardNetwork) {
// If we do not yet have a gasLimit, we must call into our background
// process to get an estimate for gasLimit based on known parameters.

paramsForGasEstimate.gas = addHexPrefix(
multiplyCurrencies(blockGasLimit, 0.95, {
multiplicandBase: 16,
multiplierBase: 10,
roundDown: '0',
toNumericBase: 'hex',
}),
);
}

// The buffer multipler reduces transaction failures by ensuring that the
// estimated gas is always sufficient. Without the multiplier, estimates
// for contract interactions can become inaccurate over time. This is because
// gas estimation is non-deterministic. The gas required for the exact same
// transaction call can change based on state of a contract or changes in the
// contracts environment (blockchain data or contracts it interacts with).
// Applying the 1.5 buffer has proven to be a useful guard against this non-
// deterministic behaviour.
//
// Gas estimation of simple sends should, however, be deterministic. As such
// no buffer is needed in those cases.
const bufferMultiplier = isSimpleSendOnNonStandardNetwork ? 1 : 1.5;

try {
// call into the background process that will simulate transaction
// execution on the node and return an estimate of gasLimit
const estimatedGasLimit = await estimateGas(paramsForGasEstimate);
const estimateWithBuffer = addGasBuffer(
estimatedGasLimit,
blockGasLimit,
1.5,
bufferMultiplier,
);
return addHexPrefix(estimateWithBuffer);
} catch (error) {
Expand Down Expand Up @@ -303,7 +325,9 @@ export async function getERC20Balance(token, accountAddress) {
export const computeEstimatedGasLimit = createAsyncThunk(
'send/computeEstimatedGasLimit',
async (_, thunkApi) => {
const { send, metamask } = thunkApi.getState();
const state = thunkApi.getState();
const { send, metamask } = state;
const isNonStandardEthChain = getIsNonStandardEthChain(state);
if (send.stage !== SEND_STAGES.EDIT) {
const gasLimit = await estimateGasLimitForSend({
gasPrice: send.gas.gasPrice,
Expand All @@ -313,6 +337,7 @@ export const computeEstimatedGasLimit = createAsyncThunk(
to: send.recipient.address?.toLowerCase(),
value: send.amount.value,
data: send.draftTransaction.userInputHexData,
isNonStandardEthChain,
});
await thunkApi.dispatch(setCustomGasLimit(gasLimit));
return {
Expand All @@ -337,6 +362,7 @@ export const initializeSendState = createAsyncThunk(
'send/initializeSendState',
async (_, thunkApi) => {
const state = thunkApi.getState();
const isNonStandardEthChain = getIsNonStandardEthChain(state);
const {
send: { asset, stage, recipient, amount, draftTransaction },
metamask,
Expand Down Expand Up @@ -383,6 +409,7 @@ export const initializeSendState = createAsyncThunk(
to: recipient.address.toLowerCase(),
value: amount.value,
data: draftTransaction.userInputHexData,
isNonStandardEthChain,
});
gasLimit = estimatedGasLimit || gasLimit;
}
Expand Down
9 changes: 9 additions & 0 deletions ui/ducks/send/send.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,9 @@ describe('Send Slice', () => {
metamask: {
blockGasLimit: '',
selectedAddress: '',
provider: {
chainId: '0x1',
},
},
...defaultSendAmountState.send,
send: {
Expand Down Expand Up @@ -1160,6 +1163,9 @@ describe('Send Slice', () => {
metamask: {
blockGasLimit: '',
selectedAddress: '',
provider: {
chainId: '0x1',
},
},
send: {
account: {
Expand Down Expand Up @@ -1372,6 +1378,9 @@ describe('Send Slice', () => {
metamask: {
blockGasLimit: '',
selectedAddress: '',
provider: {
chainId: '0x1',
},
},
send: {
account: {
Expand Down
4 changes: 4 additions & 0 deletions ui/selectors/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,10 @@ export function getIsTestnet(state) {
return TEST_CHAINS.includes(chainId);
}

export function getIsNonStandardEthChain(state) {
return !(getIsMainnet(state) || getIsTestnet(state) || process.env.IN_TEST);
}

export function getPreferences({ metamask }) {
return metamask.preferences;
}
Expand Down

0 comments on commit a603016

Please sign in to comment.