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

Use hexToDecimal to parse gasLimit in send footer #10969

Closed
wants to merge 2 commits into from
Closed

Use hexToDecimal to parse gasLimit in send footer #10969

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 2, 2021

Fixes: #10919

Explanation:
The gasLimit is being represented as an unprefixed hexadecimal string every render after the first. This means the comparison to 5208 is just comparing to NaN in a lot of cases which are incidentally passing.

In the issue 300000 and 300001 are flagged as numbers that fail when they should not, but there are also quite a few that pass when they shouldn't too, like 1000.

To fix, gasLimit is now parsed to a decimal and compared to 21000.

Manual testing steps:

  • On develop branch, send a transaction with the advanced gas controls
    • Verify the "Next" button is disabled for gas limits of 300000 and 300001
    • Verify the "Next" button is not disabled for a gas limit of 1000
  • On this branch do the same and verify the opposite for each one

@ghost ghost self-requested a review as a code owner May 2, 2021 21:58
@ghost ghost requested a review from danjm May 2, 2021 21:58
@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2021

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.

@ghost
Copy link
Author

ghost commented May 2, 2021

I have read the CLA Document and I hereby sign the CLA

@brad-decker
Copy link
Contributor

@JackAldridge thanks for the contribution. Would you be interested in testing your findings with the work done in #10965 ? Regardless i'd love to get your work landed ahead of that change as I definitely encountered this problem during said refactor and you should get your 'contributor' tag :D

@brad-decker brad-decker requested review from brad-decker and removed request for danjm May 12, 2021 22:31
@brad-decker
Copy link
Contributor

Go ahead and rebase on develop and push those changes up so we can get a build going.

@ghost
Copy link
Author

ghost commented May 13, 2021

Okay just rebased, and yep I can take a look at that refactor later today

@ghost
Copy link
Author

ghost commented May 20, 2021

@brad-decker just got round to looking at that other PR and can confirm it fixes all the issues I looked at.

Unless it's urgent maybe we should just wait for that one to land? Getting unit test failures here and not familiar enough with the test suite to tell if it's actually related to these changes.

@brad-decker
Copy link
Contributor

The unit tests are failing legitimately due to our tests using "mockGasLimit" 😬 it's entirely up to you. I can jump in here tomorrow and clean up those tests and get your commit landed ahead of my changes. I'm also okay if you want to close this. In any event thanks for taking a look at the refactor! Appreciate all the eyes I can get on it 😊

@ghost
Copy link
Author

ghost commented May 20, 2021

Cool, I'm going to close this one then. Thanks for taking a look

@ghost ghost closed this May 20, 2021
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2021
This pull request was closed.
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.

Can't proceed with gas limit set to 300000 or 300001
1 participant