Bug fix for Int256 decode range [2^248, type(int256).max] and [ type(int256.min), -(2^248) ) #2070
Conversation
Signed-off-by: tonykwok1992 <[email protected]>
tonykwok1992
requested review from
conor10,
gtebrean and
NickSneo
as code owners
| 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 { |
There was a problem hiding this comment.
pick one type that won't take too long to run test to test the entire range for sanity
There was a problem hiding this comment.
So 2^255 - 1 and -2^255 used to fail in decoding?
There was a problem hiding this comment.
testing all Int16 and uint16 can be a overkill, adding tests for boundary cases should be enough.
What do you think @gtebrean ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
So
2^255 - 1and-2^255used 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
There was a problem hiding this comment.
Nischal is right, test only min and max here
| byte[] resultByteArray = new byte[typeLengthAsBytes + 1]; | ||
|
|
||
| if (Int.class.isAssignableFrom(type) || Fixed.class.isAssignableFrom(type)) { | ||
| resultByteArray[0] = inputByteArray[0]; // take MSB as sign bit |
There was a problem hiding this comment.
These contains bug for decoding int256 range mentioned in PR
| BigInteger numericValue = new BigInteger(resultByteArray); | ||
| BigInteger numericValue; | ||
| if (Uint.class.isAssignableFrom(type) || Ufixed.class.isAssignableFrom(type)) { | ||
| numericValue = new BigInteger(1, inputByteArray, valueOffset, typeLengthAsBytes); |
There was a problem hiding this comment.
The BigInteger constructor already support reading offset from input byte array, do not need the array copying here
|
@tonykwok1992 looks good from my side, thanks for bringing this up. Please update also the Changelog.md and I will merge this. |
added |
Signed-off-by: tonykwok1992 <[email protected]>
tonykwok1992
force-pushed
the
bug-fix-decodeNumeric
branch
from
f020865
to
5947e77
Compare
Signed-off-by: tonykwok1992 <[email protected]>
What does this PR do?
Bug fix for decoding Int256 range [2^248, type(int256).max] and [ type(int256.min), -(2^248) )
Where should the reviewer start?
All files
Why is it needed?
The decoding of int256 number range [2^248, type(int256).max] and [ type(int256.min), -(2^248) ) will throw exception,
type(int256.min) and type(int256).max is the particular case that we found
Checklist