hexToNumber: return BigInt if result is bigger than max integer #5157

nikoulai · 2022-06-22T12:21:40Z

Description

#5153

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

render · 2022-06-22T12:21:44Z

Your Render PR Server URL is https://web3-js-pr-5157.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-capgilirrk0fte5ckf4g.

coveralls · 2022-06-22T13:20:33Z

Pull Request Test Coverage Report for Build 2609049428

11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
282 unchanged lines in 9 files lost coverage.
Overall coverage increased (+2.3%) to 74.331%

Files with Coverage Reduction	New Missed Lines	%
packages/web3-core-requestmanager/src/jsonrpc.js	1	70.0%
packages/web3-core-method/lib/index.js	5	93.79%
packages/web3-core-helpers/src/formatters.js	8	84.3%
packages/web3-core-helpers/lib/formatters.js	10	81.58%
packages/web3-core-helpers/src/errors.js	29	1.56%
packages/web3-utils/src/soliditySha3.js	34	3.43%
packages/web3-utils/src/index.js	43	31.38%
packages/web3-utils/src/utils.js	45	10.67%
packages/web3-eth-accounts/src/index.js	107	23.67%

Totals
Change from base Build 2596319045:	2.3%
Covered Lines:	3261
Relevant Lines:	4137

💛 - Coveralls

nikoulai · 2022-06-22T15:58:52Z

I changed hexToNumber to return BigNumber if the value is greater than max_sage_in. The reason is not to introduce a breaking change to the application already using the functionality.

nikoulai · 2022-06-22T16:01:58Z

test/utils.toNumber.js

+    // { value: 'myString 34534!', expected: '0x6d79537472696e6720333435333421', error: true, errorMessage: 'Number can only safely store up to 53 bits'},
+    { value: new BN(15), expected: 15 },
+    { value: new BigNumber(15), expected: 15 },
+    // { value: 'Heeäööä👅D34ɝɣ24Єͽ-.,äü+#/', expected: '0x486565c3a4c3b6c3b6c3a4f09f9185443334c99dc9a33234d084cdbd2d2e2cc3a4c3bc2b232f', error: true, errorMessage: 'Number can only safely store up to 53 bits'},


These tests are no longer errors because toHex is called firstly and a valid input is formed. They were failing due to being too big. Are they valid inputs, though?

nazarhussain

Please remove the unnecessary and white space changes so we can focus on what the issue was.

packages/web3-utils/src/utils.js

nazarhussain · 2022-06-23T18:31:29Z

packages/web3-utils/src/utils.js

+    const [negative, hexValue] = String(value).startsWith("-")
+        ? [true, value.slice(1)]
+        : [false, value];
+    const num = toBN(hexValue);
+
+    if (num > Number.MAX_SAFE_INTEGER) {
+        return negative ? -num : num;
+    }
+
+    return negative ? -1 * num.toNumber() : num.toNumber();


This seems to be a problem with the .toNumber(), so we need to focus on it.

nazarhussain · 2022-06-23T18:31:46Z

packages/web3-utils/src/utils.js

+    const num = toBN(hexValue);
+
+    if (num > Number.MAX_SAFE_INTEGER) {
+        return negative ? -num : num;


It will return an integer if negative and a BN object if positive.

packages/web3-utils/src/utils.js

nazarhussain

@nikoulai Did you get a chance to look at the feedback.

nikoulai · 2022-06-28T08:13:26Z

@nazarhussain
I tried the suggested solution, but, in big value tests ("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" etc) the result is truncated (1.157920892373162e+77) due to the multiplication.

jdevcs

It will be breaking change in 1.x as discussed in team, so we should try to avoid that where possible for semver.
I think more good approach can be to use an optional output formatting param ( hex formatting ) in function and in case user explicitly wants to have hex it should not do formatting of that field to number like https://github.com/ChainSafe/web3.js/blob/aae9d4a9a4ab6d68296f0cc1a2a29540eb3ff433/packages/web3-core-helpers/src/formatters.js#L292 . If user wants to continue output formatted to number we can show a warning. It will be backward compatible in this way, and we will be in compliance with semver.

.prettierignore

nikoulai · 2022-07-01T09:19:23Z

Todo: I have to update docs if change is ok

nikoulai · 2022-07-01T09:20:32Z

Todo: I have to update docs if change is ok

jdevcs

LGTM, @nikoulai

yes we need to mention this in doc and
also could you remove new changes (space etc ) from changelog

nikoulai force-pushed the 5153/Number-can-only-safely-store-53-bits branch from c5925f2 to 2970891 Compare June 22, 2022 12:28

nikoulai changed the title ~~In hexToNumber return BigInt if result is bigger than max integer~~ hexToNumber: return BigInt if result is bigger than max integer Jun 22, 2022

nikoulai force-pushed the 5153/Number-can-only-safely-store-53-bits branch from 2970891 to 45534f7 Compare June 22, 2022 12:37

nikoulai force-pushed the 5153/Number-can-only-safely-store-53-bits branch from b78aa2e to 7bc0db2 Compare June 22, 2022 15:56

nikoulai commented Jun 22, 2022

View reviewed changes

nikoulai requested review from avkos, jdevcs, luu-alex, nazarhussain and spacesailor24 June 22, 2022 16:02

nikoulai self-assigned this Jun 22, 2022

nikoulai added the 1.x 1.0 related issues label Jun 22, 2022

nikoulai linked an issue Jun 22, 2022 that may be closed by this pull request

getTransactionReceipt: Number can only safely store up to 53 bits #5153

Closed

1 task

nazarhussain suggested changes Jun 22, 2022

View reviewed changes

packages/web3-utils/src/utils.js Outdated Show resolved Hide resolved

nikoulai added 2 commits June 23, 2022 12:20

In hexToNumber return BigInt if result is bigger than max integer

77cc64b

Add some BigInt test

7910356

nikoulai force-pushed the 5153/Number-can-only-safely-store-53-bits branch from 7bc0db2 to 7910356 Compare June 23, 2022 09:22

nikoulai added 2 commits June 23, 2022 13:53

Update CHANGELOG

6f5de24

Merge branch '1.x' into 5153/Number-can-only-safely-store-53-bits

26b98b2

nikoulai requested review from nazarhussain June 23, 2022 15:31

nazarhussain suggested changes Jun 23, 2022

View reviewed changes

nazarhussain suggested changes Jun 27, 2022

View reviewed changes

jdevcs reviewed Jun 29, 2022

View reviewed changes

Add hex format as option for output in transaction receipt

6347eb4

nikoulai added 3 commits June 30, 2022 20:06

Revert hexToNumber and it's tests to initial state

183eba8

Merge branch '1.x' into 5153/Number-can-only-safely-store-53-bits

d495dc5

Remove unecessary logs

573eba0

luu-alex reviewed Jun 30, 2022

View reviewed changes

.prettierignore Outdated Show resolved Hide resolved

nikoulai added 3 commits July 1, 2022 11:23

Add test for formatter

ce99587

Remove unecessary prettierignore

ce69ec2

Update CHANGELOG

d6d45aa

nikoulai requested review from jdevcs, luu-alex and nazarhussain July 1, 2022 09:18

nazarhussain approved these changes Jul 1, 2022

View reviewed changes

jdevcs reviewed Jul 1, 2022

View reviewed changes

Merge branch '1.x' into 5153/Number-can-only-safely-store-53-bits

5c0a7ce

nikoulai force-pushed the 5153/Number-can-only-safely-store-53-bits branch from b8b6ef6 to 5c0a7ce Compare July 4, 2022 09:15

Update docs

48134b5

jdevcs approved these changes Jul 4, 2022

View reviewed changes

nikoulai requested a review from jdevcs July 4, 2022 10:37

nikoulai merged commit 9e0d9d1 into 1.x Jul 4, 2022

nikoulai deleted the 5153/Number-can-only-safely-store-53-bits branch July 4, 2022 10:49

This was referenced Jul 6, 2022

update getTransactionReceipt return type #4872

Closed

effective gas price should be a big number. #4819

Closed

snyk-bot mentioned this pull request Aug 22, 2022

[Snyk] Upgrade web3-eth2-core from 1.7.4 to 1.7.5 Mertz22/web3.js#2

Open

hexToNumber: return BigInt if result is bigger than max integer #5157

hexToNumber: return BigInt if result is bigger than max integer #5157

Uh oh!

Conversation

nikoulai commented Jun 22, 2022

Description

Type of change

Checklist:

Uh oh!

render bot commented Jun 22, 2022

Uh oh!

coveralls commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2609049428

💛 - Coveralls

Uh oh!

nikoulai commented Jun 22, 2022

Uh oh!

nikoulai Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

nazarhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nazarhussain Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

nazarhussain Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nazarhussain left a comment

Choose a reason for hiding this comment

Uh oh!

nikoulai commented Jun 28, 2022

Uh oh!

jdevcs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikoulai commented Jul 1, 2022

Uh oh!

nikoulai commented Jul 1, 2022

Uh oh!

jdevcs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Jun 22, 2022 •

edited

Loading