graph-node icon indicating copy to clipboard operation
graph-node copied to clipboard

Bug: BigInt.toString produces incorrect decimal string for large uint256 numbers

Open JonathanAmenechi opened this issue 3 years ago • 9 comments

Do you want to request a feature or report a bug? Bug

What is the current behavior? Calling BigInt.toString in graph-ts execution context produces an incorrect results for large numbers

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

Given this hex: 0x344435891c9a299443ad1ff6f97829fcf3aff343795f7eb86730d1d0f968cb54, various popular js libraries successfully convert the hex to the following decimal string: 23640783215816018536649662874651199072550547213372823297679392888119521495892

In the graph-ts execution context, BigInt.toString should return the same decimal string. However BigInt.fromByteArray(ByteArray.fromHexString(hex)).toString() returns the following: 38353673751782187045679142956196807757628143702081891553022150933500130575412, which is incorrect.

JonathanAmenechi avatar Oct 04 '22 15:10 JonathanAmenechi

Hi @JonathanAmenechi thanks for the report - @evaporei one thing that occurs to me is whether the conversion from hex to byte array to bigint will work as illustrated?

azf20 avatar Oct 09 '22 19:10 azf20

Looks like this issue has been open for 6 months with no activity. Is it still relevant? If not, please remember to close it.

github-actions[bot] avatar Apr 08 '23 00:04 github-actions[bot]

This should be moved to https://github.com/graphprotocol/graph-tooling repo instead since the bug is there.

maoueh avatar Apr 10 '23 14:04 maoueh

Does seem like that may be the case @maoueh - @leoyvens wondering if you have any ideas at a quick glance?

azf20 avatar Apr 11 '23 21:04 azf20

The issue is probably big endian vs little endian, and the result would be correct for BigInt.fromByteArray(ByteArray.fromHexString(hex).reverse()). We should have a nicer way of doing this such as a BigInt.fromHex which assumes the hex string is big endian.

leoyvens avatar Apr 12 '23 15:04 leoyvens

0x344435891c9a299443ad1ff6f97829fcf3aff343795f7eb86730d1d0f968cb54 seems to be encoded in big endian, I'm using our Go tool to decode the hex to decimal (which expects the hex to be big endian) and it works:

to_dec 0x344435891c9a299443ad1ff6f97829fcf3aff343795f7eb86730d1d0f968cb54
23640783215816018536649662874651199072550547213372823297679392888119521495892

Now, I tried adding a unit test in graph-ts code but I realized the big_int_to_string is actually an intrinsics implemented in Rust, so I need to add a unit test in graph-node to see how it looks like.

maoueh avatar Apr 12 '23 15:04 maoueh

Ok so actually @leoyvens is right that is an endianess problem, but the expected input side for fromHexString is little endian and not big endian like mentioned in his comment. Hexadecimal number are in the majority of library interpreted as big-endian hence why the difference.

const bytes = ByteArray.fromHexString("344435891c9a299443ad1ff6f97829fcf3aff343795f7eb86730d1d0f968cb54").reverse()
const result = BigInt.fromByteArray(changetype<ByteArray>(bytes));

assert(typeConversion.bigIntToString(result) == "33640783215816018536649662874651199072550547213372823297679392888119521495892");

Pass as a unit test on graph-runtime-test.

The fromHexString documentation should be enhanced to discuss this common pitfall where hex number are decoded they were little endian which is "unexpected" but changing the current behavior might prove too big of a breaking change.

maoueh avatar Apr 12 '23 17:04 maoueh

if you arrived here trying to convert an Address to a BigInt then this is how you can do it:

BigInt.fromUnsignedBytes(
      changetype<ByteArray>(
        ByteArray.fromHexString(
          fooBarAddress.toHexString()
        ).reverse()
      )
    )

outdoteth avatar Jun 07 '23 22:06 outdoteth

Question to me is when is a little endian read of from a Bytes or ByteArray useful when reading data from chain? I don't recall any data being little endian except possibly the serialization of the U256 internal data which is even chunked into u64's, hmmm. Internally the U256 type has read and write functions for big and little endian into slices. Maybe there should just be a BigInt.fromUnsignedBytesBE() as an option. I added a decode_number to do that.

richtera avatar Apr 26 '24 17:04 richtera