From e3bca640cad40c3dccc2f7f412ea564472b2366d Mon Sep 17 00:00:00 2001 From: Dmitry Shulyak Date: Sun, 26 Nov 2017 18:33:51 +0100 Subject: [PATCH 1/3] accounts/abi: Indexed inputs are not present in output --- accounts/abi/event.go | 2 ++ accounts/abi/event_test.go | 62 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/accounts/abi/event.go b/accounts/abi/event.go index 44ed7b8df256..c3a646a86204 100644 --- a/accounts/abi/event.go +++ b/accounts/abi/event.go @@ -69,6 +69,8 @@ func (e Event) tupleUnpack(v interface{}, output []byte) error { for i := 0; i < len(e.Inputs); i++ { input := e.Inputs[i] if input.Indexed { + // indexed inputs are not available in log output + j-- // can't read, continue continue } else if input.Type.T == ArrayTy { diff --git a/accounts/abi/event_test.go b/accounts/abi/event_test.go index 7e2f13f76320..ec5875417713 100644 --- a/accounts/abi/event_test.go +++ b/accounts/abi/event_test.go @@ -17,10 +17,14 @@ package abi import ( + "bytes" + "math/big" + "strconv" "strings" "testing" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/math" "github.com/ethereum/go-ethereum/crypto" ) @@ -54,3 +58,61 @@ func TestEventId(t *testing.T) { } } } + +type testResult struct { + Value1 *big.Int + Value2 *big.Int +} +type testCase struct { + definition string + want testResult +} + +func (tc testCase) encoded() []byte { + var b bytes.Buffer + if tc.want.Value1 != nil { + b.Write(math.PaddedBigBytes(math.U256(tc.want.Value1), 32)) + } + if tc.want.Value2 != nil { + b.Write(math.PaddedBigBytes(math.U256(tc.want.Value2), 32)) + } + return b.Bytes() +} + +func TestEventUnpack(t *testing.T) { + + table := []testCase{ + { + definition: `[{"anonymous":false,"inputs":[{"indexed":true,"name":"value1","type":"uint256"},{"indexed":false,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`, + want: testResult{Value2: big.NewInt(10)}, + }, + { + definition: `[{"anonymous":false,"inputs":[{"indexed":false,"name":"value1","type":"uint256"},{"indexed":false,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`, + want: testResult{Value1: big.NewInt(100), Value2: big.NewInt(1)}, + }, + { + definition: `[{"anonymous":false,"inputs":[{"indexed":false,"name":"value1","type":"uint256"},{"indexed":true,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`, + want: testResult{Value1: big.NewInt(100)}, + }, + } + for i, row := range table { + t.Run(strconv.Itoa(i+1), func(t *testing.T) { + t.Logf("unpacking %b with expected %v", row.encoded(), row.want) + abi, err := JSON(strings.NewReader(row.definition)) + if err != nil { + t.Fatal(err) + } + var rst testResult + if err := abi.Unpack(&rst, "transfer", row.encoded()); err != nil { + t.Fatalf("error unpacking %s: %v", row.definition, err) + } + if row.want.Value1 != nil && rst.Value1.Cmp(row.want.Value1) != 0 { + t.Errorf("result value1 %v is not equal to expected %v", rst.Value1, row.want.Value1) + } + + if row.want.Value2 != nil && rst.Value2.Cmp(row.want.Value2) != 0 { + t.Errorf("result value2 %v is not equal to expected %v", rst.Value2, row.want.Value2) + } + }) + } +} From 4fc2aa304ac160fd1c90d2a98254660c537f0872 Mon Sep 17 00:00:00 2001 From: Dmitry Shulyak Date: Thu, 30 Nov 2017 11:52:19 +0200 Subject: [PATCH 2/3] accounts/abi: count array fields after unpacking In current codebase array fields are counted before unpacking output. So, if we have array with 2 fields: toGoType function will try to unpack values from 64 till 64 + 2*32 which is not correct in my opinion. I shifted counting to be done after processing array, so in case array is the first field, and it has 2 elements: we will read it from 0 till 2*32. Additionally array doesnt require length prefix, so field that follows array should be read from 64 byte. This is why i made additional substitution in this change. --- accounts/abi/event.go | 7 +++-- accounts/abi/event_test.go | 57 ++++++++++++++++++++++++++++++-------- 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/accounts/abi/event.go b/accounts/abi/event.go index c3a646a86204..b1f6c2174425 100644 --- a/accounts/abi/event.go +++ b/accounts/abi/event.go @@ -73,14 +73,15 @@ func (e Event) tupleUnpack(v interface{}, output []byte) error { j-- // can't read, continue continue - } else if input.Type.T == ArrayTy { - // need to move this up because they read sequentially - j += input.Type.Size } marshalledValue, err := toGoType((i+j)*32, input.Type, output) if err != nil { return err } + if input.Type.T == ArrayTy { + // need to move this up because they read sequentially + j += input.Type.Size - 1 + } reflectValue := reflect.ValueOf(marshalledValue) switch value.Kind() { diff --git a/accounts/abi/event_test.go b/accounts/abi/event_test.go index ec5875417713..128d852bb853 100644 --- a/accounts/abi/event_test.go +++ b/accounts/abi/event_test.go @@ -18,13 +18,14 @@ package abi import ( "bytes" + "fmt" "math/big" + "reflect" "strconv" "strings" "testing" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/common/math" "github.com/ethereum/go-ethereum/crypto" ) @@ -60,59 +61,91 @@ func TestEventId(t *testing.T) { } type testResult struct { + Values [2]*big.Int Value1 *big.Int Value2 *big.Int } + type testCase struct { definition string want testResult } -func (tc testCase) encoded() []byte { +func (tc testCase) encoded(intType, arrayType Type) []byte { var b bytes.Buffer if tc.want.Value1 != nil { - b.Write(math.PaddedBigBytes(math.U256(tc.want.Value1), 32)) + val, _ := intType.pack(reflect.ValueOf(tc.want.Value1)) + b.Write(val) + } + + if !reflect.DeepEqual(tc.want.Values, [2]*big.Int{nil, nil}) { + val, _ := arrayType.pack(reflect.ValueOf(tc.want.Values)) + b.Write(val) } if tc.want.Value2 != nil { - b.Write(math.PaddedBigBytes(math.U256(tc.want.Value2), 32)) + val, _ := intType.pack(reflect.ValueOf(tc.want.Value2)) + b.Write(val) } return b.Bytes() } func TestEventUnpack(t *testing.T) { - + intType, _ := NewType("uint256") + arrayType, _ := NewType("uint256[2]") + definitionTemplate := `[{"anonymous":false,"inputs":[ +{"indexed":%t,"name":"value1","type":"%s"}, +{"indexed":%t,"name":"values","type":"%s"}, +{"indexed":%t,"name":"value2","type":"%s"}], +"name":"test","type":"event"}]` table := []testCase{ { - definition: `[{"anonymous":false,"inputs":[{"indexed":true,"name":"value1","type":"uint256"},{"indexed":false,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`, - want: testResult{Value2: big.NewInt(10)}, + // value1 is indexed + definition: fmt.Sprintf(definitionTemplate, true, intType, false, arrayType, false, intType), + want: testResult{Value2: big.NewInt(10), Values: [2]*big.Int{big.NewInt(10), big.NewInt(11)}}, }, { - definition: `[{"anonymous":false,"inputs":[{"indexed":false,"name":"value1","type":"uint256"},{"indexed":false,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`, + // only values field (array) is indexed + definition: fmt.Sprintf(definitionTemplate, false, intType, true, arrayType, false, intType), want: testResult{Value1: big.NewInt(100), Value2: big.NewInt(1)}, }, { - definition: `[{"anonymous":false,"inputs":[{"indexed":false,"name":"value1","type":"uint256"},{"indexed":true,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`, + // values and value2 are indexed + definition: fmt.Sprintf(definitionTemplate, false, intType, true, arrayType, true, intType), want: testResult{Value1: big.NewInt(100)}, }, + { + // value1 and values are not indexed + definition: fmt.Sprintf(definitionTemplate, false, intType, false, arrayType, true, intType), + want: testResult{Value1: big.NewInt(10), Values: [2]*big.Int{big.NewInt(10), big.NewInt(11)}}, + }, } for i, row := range table { t.Run(strconv.Itoa(i+1), func(t *testing.T) { - t.Logf("unpacking %b with expected %v", row.encoded(), row.want) + encoded := row.encoded(intType, arrayType) + t.Logf("unpacking definition %s, expected %v", row.definition, row.want) abi, err := JSON(strings.NewReader(row.definition)) if err != nil { t.Fatal(err) } var rst testResult - if err := abi.Unpack(&rst, "transfer", row.encoded()); err != nil { + if err := abi.Unpack(&rst, "test", encoded); err != nil { t.Fatalf("error unpacking %s: %v", row.definition, err) } if row.want.Value1 != nil && rst.Value1.Cmp(row.want.Value1) != 0 { t.Errorf("result value1 %v is not equal to expected %v", rst.Value1, row.want.Value1) } - if row.want.Value2 != nil && rst.Value2.Cmp(row.want.Value2) != 0 { t.Errorf("result value2 %v is not equal to expected %v", rst.Value2, row.want.Value2) } + if len(row.want.Values) != len(rst.Values) { + t.Errorf("result values %v are not equal to expected %v", rst.Values, row.want.Values) + } else { + for i, val := range rst.Values { + if exp := row.want.Values[i]; exp != nil && exp.Cmp(val) != 0 { + t.Errorf("value %d: %v is not equal to expected %v", i, val, exp) + } + } + } }) } } From 2191706805e9000791d6d2edb7a263024476b38b Mon Sep 17 00:00:00 2001 From: Dmitry Shulyak Date: Fri, 8 Dec 2017 12:39:00 +0200 Subject: [PATCH 3/3] accounts/abi: Improve tests for provided fixes --- accounts/abi/event.go | 3 +- accounts/abi/event_test.go | 114 ++++++++++++++++++------------------- 2 files changed, 58 insertions(+), 59 deletions(-) diff --git a/accounts/abi/event.go b/accounts/abi/event.go index b1f6c2174425..403eb088b2b4 100644 --- a/accounts/abi/event.go +++ b/accounts/abi/event.go @@ -79,7 +79,8 @@ func (e Event) tupleUnpack(v interface{}, output []byte) error { return err } if input.Type.T == ArrayTy { - // need to move this up because they read sequentially + // combined index ('i' + 'j') need to be adjusted only by size of array, thus + // we need to decrement 'j' because 'i' was incremented j += input.Type.Size - 1 } reflectValue := reflect.ValueOf(marshalledValue) diff --git a/accounts/abi/event_test.go b/accounts/abi/event_test.go index 128d852bb853..d0d074a0d30d 100644 --- a/accounts/abi/event_test.go +++ b/accounts/abi/event_test.go @@ -21,12 +21,12 @@ import ( "fmt" "math/big" "reflect" - "strconv" "strings" "testing" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" + "github.com/stretchr/testify/require" ) func TestEventId(t *testing.T) { @@ -89,63 +89,61 @@ func (tc testCase) encoded(intType, arrayType Type) []byte { return b.Bytes() } -func TestEventUnpack(t *testing.T) { - intType, _ := NewType("uint256") - arrayType, _ := NewType("uint256[2]") - definitionTemplate := `[{"anonymous":false,"inputs":[ -{"indexed":%t,"name":"value1","type":"%s"}, -{"indexed":%t,"name":"values","type":"%s"}, -{"indexed":%t,"name":"value2","type":"%s"}], -"name":"test","type":"event"}]` - table := []testCase{ - { - // value1 is indexed - definition: fmt.Sprintf(definitionTemplate, true, intType, false, arrayType, false, intType), - want: testResult{Value2: big.NewInt(10), Values: [2]*big.Int{big.NewInt(10), big.NewInt(11)}}, - }, - { - // only values field (array) is indexed - definition: fmt.Sprintf(definitionTemplate, false, intType, true, arrayType, false, intType), - want: testResult{Value1: big.NewInt(100), Value2: big.NewInt(1)}, - }, - { - // values and value2 are indexed - definition: fmt.Sprintf(definitionTemplate, false, intType, true, arrayType, true, intType), - want: testResult{Value1: big.NewInt(100)}, - }, - { - // value1 and values are not indexed - definition: fmt.Sprintf(definitionTemplate, false, intType, false, arrayType, true, intType), - want: testResult{Value1: big.NewInt(10), Values: [2]*big.Int{big.NewInt(10), big.NewInt(11)}}, - }, +// TestEventUnpackIndexed verifies that indexed field will be skipped by event decoder. +func TestEventUnpackIndexed(t *testing.T) { + definition := `[{"name": "test", "type": "event", "inputs": [{"indexed": true, "name":"value1", "type":"uint8"},{"indexed": false, "name":"value2", "type":"uint8"}]}]` + type testStruct struct { + Value1 uint8 + Value2 uint8 } - for i, row := range table { - t.Run(strconv.Itoa(i+1), func(t *testing.T) { - encoded := row.encoded(intType, arrayType) - t.Logf("unpacking definition %s, expected %v", row.definition, row.want) - abi, err := JSON(strings.NewReader(row.definition)) - if err != nil { - t.Fatal(err) - } - var rst testResult - if err := abi.Unpack(&rst, "test", encoded); err != nil { - t.Fatalf("error unpacking %s: %v", row.definition, err) - } - if row.want.Value1 != nil && rst.Value1.Cmp(row.want.Value1) != 0 { - t.Errorf("result value1 %v is not equal to expected %v", rst.Value1, row.want.Value1) - } - if row.want.Value2 != nil && rst.Value2.Cmp(row.want.Value2) != 0 { - t.Errorf("result value2 %v is not equal to expected %v", rst.Value2, row.want.Value2) - } - if len(row.want.Values) != len(rst.Values) { - t.Errorf("result values %v are not equal to expected %v", rst.Values, row.want.Values) - } else { - for i, val := range rst.Values { - if exp := row.want.Values[i]; exp != nil && exp.Cmp(val) != 0 { - t.Errorf("value %d: %v is not equal to expected %v", i, val, exp) - } - } - } - }) + abi, err := JSON(strings.NewReader(definition)) + require.NoError(t, err) + var b bytes.Buffer + b.Write(packNum(reflect.ValueOf(uint8(8)))) + var rst testStruct + require.NoError(t, abi.Unpack(&rst, "test", b.Bytes())) + require.Equal(t, uint8(0), rst.Value1) + require.Equal(t, uint8(8), rst.Value2) +} + +// TestEventMultiValueWithArrayUnpack verifies that array fields will be counted after parsing array. +func TestEventMultiValueWithArrayUnpack(t *testing.T) { + definition := `[{"name": "test", "type": "event", "inputs": [{"indexed": false, "name":"value1", "type":"uint8[2]"},{"indexed": false, "name":"value2", "type":"uint8"}]}]` + type testStruct struct { + Value1 [2]uint8 + Value2 uint8 } + abi, err := JSON(strings.NewReader(definition)) + require.NoError(t, err) + var b bytes.Buffer + var i uint8 = 1 + for ; i <= 3; i++ { + b.Write(packNum(reflect.ValueOf(i))) + } + var rst testStruct + require.NoError(t, abi.Unpack(&rst, "test", b.Bytes())) + require.Equal(t, [2]uint8{1, 2}, rst.Value1) + require.Equal(t, uint8(3), rst.Value2) +} + +// TestEventIndexedWithArrayUnpack verifies that decoder will not overlow when static array is indexed input. +func TestEventIndexedWithArrayUnpack(t *testing.T) { + definition := `[{"name": "test", "type": "event", "inputs": [{"indexed": true, "name":"value1", "type":"uint8[2]"},{"indexed": false, "name":"value2", "type":"string"}]}]` + type testStruct struct { + Value1 [2]uint8 + Value2 string + } + abi, err := JSON(strings.NewReader(definition)) + require.NoError(t, err) + var b bytes.Buffer + stringOut := "abc" + // number of fields that will be encoded * 32 + b.Write(packNum(reflect.ValueOf(32))) + b.Write(packNum(reflect.ValueOf(len(stringOut)))) + b.Write(common.RightPadBytes([]byte(stringOut), 32)) + fmt.Println(b.Bytes()) + var rst testStruct + require.NoError(t, abi.Unpack(&rst, "test", b.Bytes())) + require.Equal(t, [2]uint8{0, 0}, rst.Value1) + require.Equal(t, stringOut, rst.Value2) }