Skip to content

Add a minimumGasLimit to the original gas customization modal#9623

Merged
danjm merged 7 commits intodevelopfrom
swaps-minimum-gas-limit-2
Oct 19, 2020
Merged

Add a minimumGasLimit to the original gas customization modal#9623
danjm merged 7 commits intodevelopfrom
swaps-minimum-gas-limit-2

Conversation

@danjm
Copy link
Copy Markdown
Contributor

@danjm danjm commented Oct 16, 2020

An alternative to #9600 that directly updates the existing gas customization modal, as opposed to the new swaps customization modal added in #9599

We will still want to do #9600, but doing it this way allows us to get this fix into prod faster, which will help reduce failed swaps.

@danjm danjm requested a review from a team as a code owner October 16, 2020 17:35
@danjm danjm requested a review from brad-decker October 16, 2020 17:35
@github-actions
Copy link
Copy Markdown
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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [94d0734]
Page Load Metrics (427 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint308644168
domContentLoaded30568142611455
load30668342711455
domInteractive30468042511455

Copy link
Copy Markdown
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

This PR does not account for the usage of the gasLimitTooLow locale message as the constant GAS_LIMIT_TOO_LOW_ERROR_KEY in error-keys.js, which is used to produce error messages in the ConfirmPageContainerContent component via the getErrorKey method of ConfirmTransactionBase.

Perhaps the simplest solution is to preserve the original gasLimitTooLow locale message and create a new one that accepts a substitution?

customPriceIsSafe: PropTypes.bool,
isSpeedUp: PropTypes.bool,
customGasLimitMessage: PropTypes.string,
minimumGasLimit: PropTypes.number.isRequired,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we still need a default of 21000 at the level at this component. This is kinda in conflict with the comment I made on the other PR (sorry 😅 ) but I just noticed that this component is used in 2 other distinct places, outside of the component where you're passing this through.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 466e4de

@danjm
Copy link
Copy Markdown
Contributor Author

danjm commented Oct 16, 2020

@rekmarks I've addressed your comment in 42cd1091c

@danjm danjm force-pushed the swaps-minimum-gas-limit-2 branch from 42cd109 to 779638d Compare October 16, 2020 22:00
Copy link
Copy Markdown
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [22dcebc]
Page Load Metrics (432 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30104472311
domContentLoaded30169043011053
load30269243211053
domInteractive30069043011053

Copy link
Copy Markdown
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!

@danjm danjm merged commit 7925a76 into develop Oct 19, 2020
@danjm danjm deleted the swaps-minimum-gas-limit-2 branch October 19, 2020 13:11
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants