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

Number Overflow Issue #3976

Merged
merged 16 commits into from
Apr 9, 2021
Merged

Number Overflow Issue #3976

merged 16 commits into from
Apr 9, 2021

Conversation

spacesailor24
Copy link
Contributor

@spacesailor24 spacesailor24 commented Mar 25, 2021

Updates the use of hexToNumber to outputBigNumberFormatter (which returns a number string instead of a number)

This currently doesn't update other potentially problematic places such as here and here

Fixes #3745, #3912, #3829, and #3936
Supersedes #3789 and #3948

Notes

@render
Copy link

render bot commented Mar 25, 2021

@spacesailor24 spacesailor24 added the P1 High severity bugs label Mar 26, 2021
@spacesailor24 spacesailor24 changed the base branch from 1.x to 3.x March 26, 2021 06:22
@spacesailor24 spacesailor24 added the work-in-progress Prevents stalebot from closing a pr label Mar 26, 2021
@alstjd0921
Copy link

Currently, no errors are being seen anywhere other than the process of formatting the gas value. I think it's too radical a change.

@spacesailor24
Copy link
Contributor Author

@alstjd0921 Thank you for commenting/reviewing the code, this was actually a point of discussion I wanted to have:

I've tested your #3948 against the test repo I made, and it does pass using your branch, so presumably it fixes all the mentioned issues.

However, even though changing one field is more desirable that forcing users to update multiple fields, I want to avoid having to make another breaking change in the future where some field related to gas overflows. So, I would like to keep any updates I've made to fields pertaining to gas, but revert other changes like block.number or transactionIndex, as they probably won't come near exceeding JavaScript's max integer.

To be specific, the fields I want to remain returning number strings are: gas, cumulativeGasUsed, gasUsed, and gasLimit (I will be updating this PR accordingly after this comment)

How do you (and anyone else reading this) feel about this?

P.s. in the rewrite we've started, we will be using the native BigInt, so this won't be a problem going forward

@spacesailor24
Copy link
Contributor Author

spacesailor24 commented Mar 29, 2021

Blocked by #3982

@alstjd0921
Copy link

alstjd0921 commented Mar 30, 2021

@spacesailor24 As you said, it seems desirable to limit the problem to gas-related things.
I think it makes sense now. I was just concerned about the change of unrelated with gas overflow (e.g., timestamp, index, or other things) in commit a687dde.

@spacesailor24
Copy link
Contributor Author

@alstjd0921 if you want to merge this, then we could merge your #3948

@coveralls
Copy link

coveralls commented Mar 31, 2021

Pull Request Test Coverage Report for Build 706496265

  • 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
  • 247 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+2.5%) to 75.95%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/web3-core-helpers/src/formatters.js 5 6 83.33%
Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 70.0%
packages/web3-core-helpers/src/formatters.js 10 78.88%
packages/web3-core-helpers/lib/formatters.js 19 81.52%
packages/web3-utils/src/utils.js 27 10.74%
packages/web3-core-helpers/src/errors.js 29 1.56%
packages/web3-utils/src/soliditySha3.js 34 3.92%
packages/web3-utils/src/index.js 42 32.12%
packages/web3-eth-accounts/src/index.js 85 33.06%
Totals Coverage Status
Change from base Build 706495893: 2.5%
Covered Lines: 3108
Relevant Lines: 3877

💛 - Coveralls

@alstjd0921
Copy link

@spacesailor24
I merged this but seems like some checks were broken. Should i handle it?

@spacesailor24
Copy link
Contributor Author

@alstjd0921 Thought I fixed all the tests, but missed the CI ones. I'm going to work on getting those fixed today

@spacesailor24
Copy link
Contributor Author

Am now just realizing that the failing tests are a result of pulling the ganache-core repo and running their tests, so I'm trying to figure out what to do about that because of the breaking change

@spacesailor24
Copy link
Contributor Author

So the plan will be to merge this, branch off develop, update the tests, then target the branch for failing check

@spacesailor24 spacesailor24 marked this pull request as ready for review April 9, 2021 00:35
Copy link
Contributor

@frankiebee frankiebee left a comment

Choose a reason for hiding this comment

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

Wool woop

@spacesailor24
Copy link
Contributor Author

The was merged with failing ganache-core tests. I updated most of the failing tests in this, but there were 3 remaining that seem to need actual core code to be updated

@spacesailor24 spacesailor24 mentioned this pull request Apr 9, 2021
@sebsobseb
Copy link

Am I correct that the actual hexToNumber util method remains the same? It will still return a number?

For example, this check with triple equals will not be impacted? :
web3.util.hexToNumber("0x4") === 4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High severity bugs work-in-progress Prevents stalebot from closing a pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error while decode transaction gas: "Number can only safely store up to 53 bits"
5 participants