Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jul 1, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
972a46a
Ensure that eth_estimateGas is called to estimate gas limit for simpl…
danjm Jun 29, 2021
5fafef1
getIsNonStandardEthChain returns false when in test
danjm Jun 29, 2021
be48eda
Add comment explaining gas limit buffer multipliers in estimateGasLim…
danjm Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions ui/ducks/send/send.js
View file
Open in desktop
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,30 @@ 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',
}),
);
}

const bufferMultiplier = isSimpleSendOnNonStandardNetwork ? 1 : 1.5;
Copy link
Contributor

@adonesky1 adonesky1 Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a comment explaining the variance in the bufferMultiplier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adonesky1 added in the latest commit


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 +314,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 +326,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 +351,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 +398,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
View file
Open in desktop
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
View file
Open in desktop
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