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

fix: fromWei() parsing of large numbers > 10^20 #6882

Conversation

sgerodes
Copy link
Contributor

@sgerodes sgerodes commented Mar 11, 2024

Prevent automatic conversion to scientific notation for numbers >= 10^21 by using BigInt, ensuring accurate string representations and avoiding parsing errors in fromWei function.

Fixes #6880

Description

Current errornous behaviour examples:

Web3.utils.fromWei(19999990000000099000000, 'ether') === "1..99999900000001e+22"
Web3.utils.fromWei(19999990000000000000000, 'ether') === "0.0000001.999999e+22"
Web3.utils.fromWei(19000000000000000000000, 'ether') === "0.000000000001.9e+22"

The following code performs silently a wrong parsing because of this Issue

const wei = 19999999999999991611392;
const ethString = Web3.utils.fromWei(wei, 'ether');
const ethFloat = parseFloat(eth);
console.log(ethFloat) // equals 1.9 ETH, but should be > 19999 ETH

19999 ETH was parsed as 1.9 ETH with little code, that felt very intutive. Just using parseFloat. Which probably many of the devs use after they receive a string from "fromWei".

Explanation of the behaviour

When converting large numerical values from wei to ether using fromWei, we encounter an issue where numbers greater than or equal to 10^21 are automatically represented in scientific notation by JavaScript. This conversion introduces an additional decimal point into the string representation (1.9.99999999999999e+22 for 19999999999999990000000), leading to significant parsing errors and value distortion upon further processing (e.g., when using parseFloat, 19999 ETH incorrectly becomes 1.9 ETH).

JavaScript's default behavior of converting large numbers to scientific notation when stringified is the underlying cause. This automatic conversion impacts the string manipulation logic within the fromWei function, resulting in malformed outputs for large wei values.

Solution

I have implemented a conditional check to utilize BigInt for numbers that are at risk of being automatically converted to scientific notation (>= 10^21).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run lint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:coverage and my test cases cover all the lines and branches of the added code.
  • I ran npm run build and tested dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have linked Issue(s) with this PR in "Linked Issues" menu.

Prevent automatic conversion to scientific notation for numbers >= 10^21 by using BigInt, ensuring accurate string representations and avoiding parsing errors in fromWei function.
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Merging #6882 (aa1a376) into 4.x (1f81ff0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##              4.x    #6882   +/-   ##
=======================================
  Coverage   91.98%   91.98%           
=======================================
  Files         215      215           
  Lines        8174     8176    +2     
  Branches     2207     2208    +1     
=======================================
+ Hits         7519     7521    +2     
  Misses        655      655           
Flag Coverage Δ
UnitTests 91.98% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
web3 ∅ <ø> (∅)
web3-core ∅ <ø> (∅)
web3-errors ∅ <ø> (∅)
web3-eth ∅ <ø> (∅)
web3-eth-abi ∅ <ø> (∅)
web3-eth-accounts ∅ <ø> (∅)
web3-eth-contract ∅ <ø> (∅)
web3-eth-ens ∅ <ø> (∅)
web3-eth-iban ∅ <ø> (∅)
web3-eth-personal ∅ <ø> (∅)
web3-net ∅ <ø> (∅)
web3-providers-http ∅ <ø> (∅)
web3-providers-ipc ∅ <ø> (∅)
web3-providers-ws ∅ <ø> (∅)
web3-rpc-methods ∅ <ø> (∅)
web3-utils ∅ <ø> (∅)
web3-validator ∅ <ø> (∅)

@sgerodes sgerodes changed the title Fix scientific notation parsing for large numbers with BigInt Fix scientific notation parsing for large numbers for the fromWei() function Mar 12, 2024
Copy link
Contributor

@avkos avkos left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I found that this solution does not cover all cases because of JavaScript problem

@sgerodes
Copy link
Contributor Author

sgerodes commented Mar 14, 2024

@avkos Suddenly there is nothing the library, me or you can do about it.

The precision is lost alredy before the number enters any function of the web3 library. If the user choses to use javascript numbers (instead of bigints or strings) then he will deal with precision issues because the javascript numbers system itself is not precise.

Here is an example. You can see that the precision is already lost while initializing the number, not in the web3 library function.

const number = 100000000000000000010
console.log(number);
console.log(Web3.utils.toNumber(number));

logs:

100000000000000000000
100000000000000000000

Here is also a warning from my IDE that denotes exactly that issue.

Screenshot 2024-03-14 at 16 20 41

These issues you have mentioned were already there before my changes. My commits have not changed any behaviour related to that. If the user chooses to use the javascript 'number' type the value arrives in the function imprecise and there is no way the function can fix that.

@sgerodes sgerodes requested a review from avkos March 14, 2024 15:32
@sgerodes sgerodes changed the title Fix scientific notation parsing for large numbers for the fromWei() function fix: fromWei() parsing of large numbers > 10^20 Mar 14, 2024
@luu-alex
Copy link
Contributor

Hey there, thanks for submitting this PR :) we talked about this with the team and I think that theres limitations using number which is why this is headache dealing with large/decimal numbers.

My suggestion is that if a number is at the max that javascript will allow it (which i believe in this case its 100000000000000000000), an error should produce telling the user that their number is too large, and should use string/bigInts.

@sgerodes
Copy link
Contributor Author

@luu-alex This is indeed a viable approach to handle this situation. But this would probably break a lot of application that already use this library. There is really big amount of application already using this library. Backwards compatibility is a real concern here.

I can of course implement that, but are you sure that the error solution is good for the library?

@sgerodes
Copy link
Contributor Author

@luu-alex Anyways, I propose you to merge this request, because this PR is not about precision, it is about a real bug in parsing.

We can open a new ticket with a PR to discuss and potentially implement something for the precision problem. These two problems are linked but still different. And the initial Ticket was not aimed to solve the precision problem.

@luu-alex
Copy link
Contributor

@sgerodes I think you bring up some good points, we can move forward with this PR. I'll create another issue on precision for discussion

@luu-alex
Copy link
Contributor

luu-alex commented Mar 20, 2024

https://github.com/web3/web3.js/blob/4.x/packages/web3-utils/CHANGELOG.md include this fix in the changelog with issue #

@sgerodes
Copy link
Contributor Author

@luu-alex done. I have assumed this fix would be released in version 4.6.1

@luu-alex
Copy link
Contributor

@sgerodes we'll change that number when we plan to do next release, i changed it for you. :) thanks for this!

@luu-alex luu-alex merged commit a83e9d5 into web3:4.x Mar 25, 2024
51 of 54 checks passed
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.

Web3.utils.fromWei silently performs unsafe conversion for big numbers because of scientific notation
3 participants