Negative numbers not properly converted in ABI encoding #2653
Conversation
Updated: Mon Jun 13 12:07:53 UTC 2016 |
|
Travis failed for go 1.4, but passed for 1.5, and 1.6. It fails at TestWSAttachWelcome-2. Not sure why and I don't think my changes broke it. |
|
@obscuren PTAL |
|
Thank you for your PR. Please format your commit according to our commit standard: |
When converting a negative number e.g., -2, the resulting ABI encoding should look as follows: fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe. However, since the check of the type is for an uint instead of an int, it results in the following ABI encoding: 0101010101010101010101010101010101010101010101010101010101010102. The Ethereum ABI (https://github.com/ethereum/wiki/wiki/Ethereum-Contract-ABI) says, that signed integers are stored in two's complement which should be of the form ffffff.... and not 01010101..... for e.g. -1. Thus, I removed the type check in numbers.go as well as the function S256 as I don't think they are correct. Or maybe I'm missing something?
|
👍 |
| } else { | ||
| return S2S256(int64(value.Uint())) | ||
| } | ||
| return U2U256(value.Uint()) |
There was a problem hiding this comment.
I think it would be cleaner to drop func U2U256(n uint64) altogether and just inline it here and below. Also it seems strange to me to go through an int64 for unsigned values (imho that's a bug, I'll check it with a test), so please use U256(new(big.Int).SetUint64(value.Uint())).
There was a problem hiding this comment.
Yup, the cast in both the original code and your code is wrong. My suggestion above fixes it. Please add this test to the testsuite for verification:
maxuint64 := []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 255, 255, 255, 255}
packed := packNum(reflect.ValueOf(uint64(math.MaxUint64)))
if !bytes.Equal(packed, maxuint64) {
t.Errorf("expected %x got %x", maxuint64, packed)
}
|
The issues you raised @karalabe have nothing to do with this PR. This PR is supposed to fix an outstanding encoding issue which is considered fixed. It would be cleaner to fix this up in a separate PR. |
|
@obscuren Fair enough. I can live with opening a fresh PR to address the using encoding issue. Otherwise LGTM 👍 |
|
Thanks, my testcases are now passing. Since I'm using small negative numbers, I did not run into int int64/uint64 issue. |
When converting a negative number e.g., -2, the resulting ABI encoding should look as follows:
fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe. However, since the check of the type is for an uint instead of an
int, it results in the following ABI encoding: 0101010101010101010101010101010101010101010101010101010101010102. The Ethereum ABI
(https://github.com/ethereum/wiki/wiki/Ethereum-Contract-ABI) says, that signed integers are stored
in two's complement which should be of the form ffffff.... and not 01010101..... for e.g. -1. Thus, I removed the type check in
numbers.go as well as the function S256 as I don't think they are correct. Or maybe I'm missing something?