Fix big number accuracy

[KunZhou-at]

Previously, big numbers were sometimes returned as number instead of string even if the number is larger than Number.MAX_SAFE_INTEGER or smaller than Number.MIN_SAFE_INTEGER. The behavior was conditional on the equality of bigNumber.toString() === fullAccuracyNumberInString. However, the number may not be at full accuracy: for many big numbers BigInt(bigNumber).toString() !== bigNumber.toString(), e.g., 99000001013466380

[KunZhou-at]

dead code?

[KunZhou-at]

cc @wellwelwel @sidorares this was a pretty critical bug that caused data corruption for us, would love to contribute the fix to upstream

[wellwelwel]

@KunZhou-at, in fact the Number.MAX_SAFE_INTEGER is 9007199254740991.

this was a pretty critical bug that caused data corruption for us

I couldn't understand the issue, could you show an example where this would fail? It would be great if we could add a test to cover this possible failure as well 🙋🏻‍♂️

[KunZhou-at]

@wellwelwel thanks for taking a look! Basically my change is from

res = resNumber.toString() === resString ? resNumber : resString;

to

res = res.greaterThan(Number.MAX_SAFE_INTEGER) || res.lessThan(Number.MIN_SAFE_INTEGER) ? resString : res.toNumber();

There are numbers larger than Number.MAX_SAFE_INTEGER such that resNumber.toString() === resString, and the example I have provided is 99000001013466380. While the number's toString does print out the same number, it does not behave nicely with other native node facilities such as bigint. I think it's expected that the behavior of numbers outside of the safe range is undefined. I don't think toString is the correct way to determine if a number is at full accuracy.

I updated the test to only return strings for numbers above Number.MAX_SAFE_INTEGER, i.e. 9007199254740991.

[KunZhou-at]

@wellwelwel I still think this is a critical bug that can cause database corruption, but closing since it's not getting attention. Feel free to open and I can resolve the conflicts if that changes.