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

Remove default sendToken value in send.component.js #8764

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

tmashuang
Copy link
Contributor

Fixes #8763

@tmashuang tmashuang requested a review from a team as a code owner June 8, 2020 17:38
@metamaskbot
Copy link
Collaborator

Builds ready [ee0acd4]
Page Load Metrics (585 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30493653
domContentLoaded35378758312660
load35578858512660
domInteractive35378758312660

whymarrh
whymarrh previously approved these changes Jun 9, 2020
return BASE_TOKEN_GAS_COST
}

if (sendToken) {
if (Object.keys(sendToken).length !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Though if sendToken is undefined or null this will throw now 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I suspect that this PR would fix the bug, but only because estimateGas is never called with sendToken set to null or undefined.

It's only called by updateGasData in actions.js, which itself is only called by updateAndSetGasLimit in send.container.js, which is only called in one spot in send.component.js, where sendToken is given a default value of {}:

state.metamask.send.token is always set to undefined when there is no sendToken, so that default should always be set. But only for a very tenuous reason.

It would seem better to remove the = {} instead.

@tmashuang tmashuang changed the title Checks for empty sendToken object in estimateGas Remove default sendToken value in send.component.js Jun 9, 2020
@metamaskbot
Copy link
Collaborator

Builds ready [fc6bc43]
Page Load Metrics (695 ± 28 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33103472010
domContentLoaded4978096935928
load4998106955928
domInteractive4968086935928

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@tmashuang tmashuang merged commit e124a9b into develop Jun 10, 2020
@tmashuang tmashuang deleted the estimateGas-fix branch June 10, 2020 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal Txs' gas limit default overpriced for eth transfers
4 participants