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.

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

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Jun 29, 2021

This PR updates our gas estimation logic so that if we are doing a simple send on a network that is not one of the default/standard networks in metamask (e.g. eth mainnet, or the test networks supported by infura), then we set the gas estimate to the result of calling eth_gasEstimate

This will enable custom networks to set gas prices for simple sends that are different than 21000

@danjm danjm requested a review from a team as a code owner June 29, 2021 15:42
@danjm danjm requested a review from darkwing June 29, 2021 15:42
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@danjm danjm force-pushed the custom-networks-gas-limits branch from cce7d84 to 972a46a Compare June 29, 2021 15:55
brad-decker
brad-decker previously approved these changes Jun 29, 2021
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

looks good to me, minor fix that should fix e2e

@@ -376,6 +376,10 @@ export function getIsTestnet(state) {
return TEST_CHAINS.includes(chainId);
}

export function getIsNonStandardEthChain(state) {
return !(getIsMainnet(state) || getIsTestnet(state));
Copy link
Contributor

Choose a reason for hiding this comment

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

you should probably return false if IN_TEST to match some of our other logic to treat ganache as mainnet in tests.

@metamaskbot
Copy link
Collaborator

Builds ready [5fafef1]
Page Load Metrics (523 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45806194
domContentLoaded34268052210952
load34468152310953
domInteractive34268052110952

brad-decker
brad-decker previously approved these changes Jun 29, 2021
);
}

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

@adonesky1 adonesky1 Jun 30, 2021

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

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM! Code looks great and functions as described!

@metamaskbot
Copy link
Collaborator

Builds ready [be48eda]
Page Load Metrics (680 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint52916594
domContentLoaded43589767911857
load43689868011857
domInteractive43489767811857

@danjm danjm merged commit a603016 into develop Jul 1, 2021
@danjm danjm deleted the custom-networks-gas-limits branch July 1, 2021 01:54
@github-actions github-actions bot locked and limited conversation to collaborators Jul 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants