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.

Sign up for GitHub

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

Jump to bottom

Bug fix for Int256 decode range [2^248, type(int256).max] and [ type(int256.min), -(2^248) ) #2070

Merged
merged 3 commits into from
Jun 26, 2024
Merged

Bug fix for Int256 decode range [2^248, type(int256).max] and [ type(int256.min), -(2^248) ) #2070

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2b49a5
Bug fix for Int256 decode
tonykwok1992 Jun 22, 2024
5947e77
Change log
tonykwok1992 Jun 26, 2024
d183957
Test int16/Uint16 min max only
tonykwok1992 Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions abi/src/main/java/org/web3j/abi/TypeDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,17 +141,14 @@ public static <T extends NumericType> T decodeNumeric(String input, Class<T> typ
try {
byte[] inputByteArray = Numeric.hexStringToByteArray(input);
int typeLengthAsBytes = getTypeLengthInBytes(type);

byte[] resultByteArray = new byte[typeLengthAsBytes + 1];

if (Int.class.isAssignableFrom(type) || Fixed.class.isAssignableFrom(type)) {
resultByteArray[0] = inputByteArray[0]; // take MSB as sign bit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These contains bug for decoding int256 range mentioned in PR

}

int valueOffset = Type.MAX_BYTE_LENGTH - typeLengthAsBytes;
System.arraycopy(inputByteArray, valueOffset, resultByteArray, 1, typeLengthAsBytes);

BigInteger numericValue = new BigInteger(resultByteArray);
BigInteger numericValue;
if (Uint.class.isAssignableFrom(type) || Ufixed.class.isAssignableFrom(type)) {
numericValue = new BigInteger(1, inputByteArray, valueOffset, typeLengthAsBytes);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BigInteger constructor already support reading offset from input byte array, do not need the array copying here

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

} else {
numericValue = new BigInteger(inputByteArray, valueOffset, typeLengthAsBytes);
}
return type.getConstructor(BigInteger.class).newInstance(numericValue);

} catch (NoSuchMethodException
Expand Down
53 changes: 53 additions & 0 deletions abi/src/test/java/org/web3j/abi/TypeDecoderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@
import org.web3j.abi.datatypes.generated.Uint80;
import org.web3j.abi.datatypes.generated.Uint88;
import org.web3j.abi.datatypes.generated.Uint96;
import org.web3j.utils.Numeric;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
Expand Down Expand Up @@ -858,11 +859,63 @@ public void testIntDecode() throws Exception {
Int256.class),
(new Int256(BigInteger.valueOf(-1))));




assertEquals(
TypeDecoder.decodeNumeric(
TypeEncoder.encodeNumeric(new Int256(BigInteger.TWO.pow(248))),
Int256.class),
new Int256(BigInteger.TWO.pow(248)));

assertEquals(
TypeDecoder.decodeNumeric(
TypeEncoder.encodeNumeric(new Int256(BigInteger.TWO.pow(248).negate().subtract(BigInteger.ONE))),
Int256.class),
new Int256(BigInteger.TWO.pow(248).negate().subtract(BigInteger.ONE)));



assertEquals(
TypeDecoder.decodeNumeric(
"0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
Int256.class),
new Int256(new BigInteger("57896044618658097711785492504343953926634992332820282019728792003956564819967")));

assertEquals(
TypeDecoder.decodeNumeric(
"0x8000000000000000000000000000000000000000000000000000000000000000",
Int256.class),
new Int256(new BigInteger("-57896044618658097711785492504343953926634992332820282019728792003956564819968")));

assertEquals(TypeDecoder.instantiateType("int", 123), (new Int(BigInteger.valueOf(123))));

assertEquals(TypeDecoder.instantiateType("int", -123), (new Int(BigInteger.valueOf(-123))));
}

@Test
public void testInt16All() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pick one type that won't take too long to run test to test the entire range for sanity

Copy link
Contributor

Choose a reason for hiding this comment

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

So 2^255 - 1 and -2^255 used to fail in decoding?

Copy link
Contributor

@NickSneo NickSneo Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

testing all Int16 and uint16 can be a overkill, adding tests for boundary cases should be enough.
What do you think @gtebrean ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testing all Int16 and uint16 can be a overkill, adding tests for boundary cases should be enough. What do you think @gtebrean ?

it is a loop with 2^16 = 65536 entries, I think it runs at a reasonable time. But if there is concern I can change it to boundary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So 2^255 - 1 and -2^255 used to fail in decoding?

yes, that's the case I'm using, later on found that the bug is in a large range mentioned in PR title

Copy link
Contributor

Choose a reason for hiding this comment

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

Nischal is right, test only min and max here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

int boundary = (int) Math.pow(2, 16 - 1);
for (int i = -boundary; i < boundary; i++) {
assertEquals(
TypeDecoder.decodeNumeric(
TypeEncoder.encodeNumeric(new Int16(BigInteger.valueOf(i))),
Int16.class),
new Int16(BigInteger.valueOf(i)));
}
}

@Test
public void testUint16All() throws Exception {
int boundary = (int) Math.pow(2, 16);
for (int i = 0; i < boundary; i++) {
assertEquals(
TypeDecoder.decodeNumeric(
TypeEncoder.encodeNumeric(new Uint16(BigInteger.valueOf(i))),
Uint16.class),
new Uint16(BigInteger.valueOf(i)));
}
}
/*
TODO: Enable once Solidity supports fixed types - see
https://github.com/ethereum/solidity/issues/409
Expand Down
Loading