Skip to content

accounts/abi: fix event tupleUnpack errors + cosmetic cleaning #15452

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

Closed
robert-zaremba wants to merge 6 commits into from
Closed

accounts/abi: fix event tupleUnpack errors + cosmetic cleaning #15452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ebb68bd
accounts/abi: adding event unpacker tests
robert-zaremba Nov 10, 2017
6d604a8
accounts/abi: fix event tupleUnpack
robert-zaremba Nov 9, 2017
f1e9e83
accounts/abi: fix event unpack into slice
robert-zaremba Nov 10, 2017
f0fc23b
accounts/abi: satisfy most of the linter warnings
robert-zaremba Nov 10, 2017
882a32a
accounts/abi: add Method Unpack tests
robert-zaremba Nov 23, 2017
2aeb255
accounts/abi: add unpack into array test
robert-zaremba Dec 1, 2017
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
3 changes: 2 additions & 1 deletion accounts/abi/abi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (abi ABI) Unpack(v interface{}, name string, output []byte) (err error) {
} else if event, ok := abi.Events[name]; ok {
unpack = event
} else {
return fmt.Errorf("abi: could not locate named method or event.")
return fmt.Errorf("abi: could not locate named method or event")
Copy link
Copy Markdown
Member

@gballet gballet Nov 10, 2017

Choose a reason for hiding this comment

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

Is that change really necessary? I can see the point (pun intended) of not displaying two dots if that error is subsequently printed as a %v followed by another dot. However, it would be preferable not to add that dot over there, and not clutter the diff with unnecessary entries (more on that later)

Copy link
Copy Markdown
Contributor Author

@robert-zaremba robert-zaremba Nov 10, 2017

Choose a reason for hiding this comment

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

This was reported by golinit

}

// requires a struct to unpack into for a tuple return...
Expand All @@ -99,6 +99,7 @@ func (abi ABI) Unpack(v interface{}, name string, output []byte) (err error) {
return unpack.singleUnpack(v, output)
}

// UnmarshalJSON implements json.Unmarshaler interface
func (abi *ABI) UnmarshalJSON(data []byte) error {
var fields []struct {
Type string
Expand Down
11 changes: 11 additions & 0 deletions accounts/abi/argument.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type Argument struct {
Indexed bool // indexed is only used by events
}

// UnmarshalJSON implements json.Unmarshaler interface
func (a *Argument) UnmarshalJSON(data []byte) error {
var extarg struct {
Name string
Expand All @@ -49,3 +50,13 @@ func (a *Argument) UnmarshalJSON(data []byte) error {

return nil
}

func countNonIndexedArguments(args []Argument) int {
out := 0
for i := range args {
if !args[i].Indexed {
out++
}
}
return out
}
42 changes: 18 additions & 24 deletions accounts/abi/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,22 @@ func (e Event) tupleUnpack(v interface{}, output []byte) error {
var (
value = valueOf.Elem()
typ = value.Type()
kind = value.Kind()
)

if value.Kind() != reflect.Struct {
return fmt.Errorf("abi: cannot unmarshal tuple in to %v", typ)
if err := requireUnpackKind(value, typ, kind, e.Inputs, true); err != nil {
return err
}

j := 0
for i := 0; i < len(e.Inputs); i++ {
input := e.Inputs[i]
// `i` counts the nonindexed arguments.
// `j` counts the number of complex types.
// both `i` and `j` are used to to correctly compute `data` offset.
i, j := -1, 0
for _, input := range e.Inputs {
if input.Indexed {
// can't read, continue
// Indexed arguments are not packed into data
continue
}
i++
marshalledValue, err := toGoType((i+j)*32, input.Type, output)
if err != nil {
return err
Expand All @@ -83,31 +86,25 @@ func (e Event) tupleUnpack(v interface{}, output []byte) error {
}
reflectValue := reflect.ValueOf(marshalledValue)

switch value.Kind() {
switch kind {
case reflect.Struct:
for j := 0; j < typ.NumField(); j++ {
field := typ.Field(j)
// TODO read tags: `abi:"fieldName"`
if field.Name == strings.ToUpper(e.Inputs[i].Name[:1])+e.Inputs[i].Name[1:] {
if err := set(value.Field(j), reflectValue, e.Inputs[i]); err != nil {
if field.Name == strings.ToUpper(input.Name[:1])+input.Name[1:] {
if err := set(value.Field(j), reflectValue, input); err != nil {
return err
}
}
}
case reflect.Slice, reflect.Array:
if value.Len() < i {
return fmt.Errorf("abi: insufficient number of arguments for unpack, want %d, got %d", len(e.Inputs), value.Len())
}
v := value.Index(i)
if v.Kind() != reflect.Ptr && v.Kind() != reflect.Interface {
return fmt.Errorf("abi: cannot unmarshal %v in to %v", v.Type(), reflectValue.Type())
if err := requireAssignable(v, reflectValue); err != nil {
return err
}
reflectValue := reflect.ValueOf(marshalledValue)
if err := set(v.Elem(), reflectValue, e.Inputs[i]); err != nil {
if err := set(v.Elem(), reflectValue, input); err != nil {
return err
}
default:
return fmt.Errorf("abi: cannot unmarshal tuple in to %v", typ)
}
}
return nil
Expand All @@ -123,7 +120,7 @@ func (e Event) singleUnpack(v interface{}, output []byte) error {
}

if e.Inputs[0].Indexed {
return fmt.Errorf("abi: attempting to unpack indexed variable into element.")
return fmt.Errorf("abi: attempting to unpack indexed variable into element")
}

value := valueOf.Elem()
Expand All @@ -132,8 +129,5 @@ func (e Event) singleUnpack(v interface{}, output []byte) error {
if err != nil {
return err
}
if err := set(value, reflect.ValueOf(marshalledValue), e.Inputs[0]); err != nil {
return err
}
return nil
return set(value, reflect.ValueOf(marshalledValue), e.Inputs[0])
}
164 changes: 164 additions & 0 deletions accounts/abi/event_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,52 @@ package abi

import (
"bytes"
"encoding/hex"
"encoding/json"
"math/big"
"reflect"
"strings"
"testing"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"github.com/stretchr/testify/assert"
Copy link
Copy Markdown
Member

@gballet gballet Nov 10, 2017

Choose a reason for hiding this comment

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

You're pulling in two libraries that produce a big diff (and code) size, and you're the only one using it. I suggest that you create a different PR to start using those two libraries, so that the content of this PR is only about tupleUnpack. If the community is interested in using testify, you will get upvotes and we'll then include it. This PR does too many different things and we can't include it whole.

Copy link
Copy Markdown
Contributor Author

@robert-zaremba robert-zaremba Nov 10, 2017

Choose a reason for hiding this comment

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

OK, will do that.

Copy link
Copy Markdown
Contributor Author

@robert-zaremba robert-zaremba Nov 10, 2017

Choose a reason for hiding this comment

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

Testify is a really convenient library when doing tests. I will extract it from this pull request and do another one then.

"github.com/stretchr/testify/require"
)

var jsonEventTransfer = []byte(`{
"anonymous": false,
"inputs": [
{
"indexed": true, "name": "from", "type": "address"
}, {
"indexed": true, "name": "to", "type": "address"
}, {
"indexed": false, "name": "value", "type": "uint256"
}],
"name": "Transfer",
"type": "event"
}`)

var jsonEventPledge = []byte(`{
"anonymous": false,
"inputs": [{
"indexed": false, "name": "who", "type": "address"
}, {
"indexed": false, "name": "wad", "type": "uint128"
}, {
"indexed": false, "name": "currency", "type": "bytes3"
}],
"name": "Pledge",
"type": "event"
Copy link
Copy Markdown
Member

@VoR0220 VoR0220 Dec 1, 2017

Choose a reason for hiding this comment

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

Needs a test to take into account static array types.

Copy link
Copy Markdown
Contributor Author

@robert-zaremba robert-zaremba Dec 1, 2017

Choose a reason for hiding this comment

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

Done

}`)

// 1000000
var transferData1 = "00000000000000000000000000000000000000000000000000000000000f4240"

// "0x00Ce0d46d924CC8437c806721496599FC3FFA268", 2218516807680, "usd"
var pledgeData1 = "00000000000000000000000000ce0d46d924cc8437c806721496599fc3ffa2680000000000000000000000000000000000000000000000000000020489e800007573640000000000000000000000000000000000000000000000000000000000"

func TestEventId(t *testing.T) {
var table = []struct {
definition string
Expand Down Expand Up @@ -77,3 +114,130 @@ func TestEventMultiValueWithArrayUnpack(t *testing.T) {
require.Equal(t, [2]uint8{1, 2}, rst.Value1)
require.Equal(t, uint8(3), rst.Value2)
}

func TestEventTupleUnpack(t *testing.T) {

type EventTransfer struct {
Value *big.Int
}

type EventPledge struct {
Who common.Address
Wad *big.Int
Currency [3]byte
}

type BadEventPledge struct {
Who string
Wad int
Currency [3]byte
}

bigint := new(big.Int)
bigintExpected := big.NewInt(1000000)
bigintExpected2 := big.NewInt(2218516807680)
addr := common.HexToAddress("0x00Ce0d46d924CC8437c806721496599FC3FFA268")
var testCases = []struct {
data string
dest interface{}
expected interface{}
jsonLog []byte
error string
name string
}{{
transferData1,
&EventTransfer{},
&EventTransfer{Value: bigintExpected},
jsonEventTransfer,
"",
"Can unpack ERC20 Transfer event into structure",
}, {
transferData1,
&[]interface{}{&bigint},
&[]interface{}{&bigintExpected},
jsonEventTransfer,
"",
"Can unpack ERC20 Transfer event into slice",
}, {
pledgeData1,
&EventPledge{},
&EventPledge{
addr,
bigintExpected2,
[3]byte{'u', 's', 'd'}},
jsonEventPledge,
"",
"Can unpack Pledge event into structure",
}, {
pledgeData1,
&[]interface{}{&common.Address{}, &bigint, &[3]byte{}},
&[]interface{}{
&addr,
&bigintExpected2,
&[3]byte{'u', 's', 'd'}},
jsonEventPledge,
"",
"Can unpack Pledge event into slice",
}, {
pledgeData1,
&[3]interface{}{&common.Address{}, &bigint, &[3]byte{}},
&[3]interface{}{
&addr,
&bigintExpected2,
&[3]byte{'u', 's', 'd'}},
jsonEventPledge,
"",
"Can unpack Pledge event into an array",
Copy link
Copy Markdown
Member

@VoR0220 VoR0220 Dec 1, 2017

Choose a reason for hiding this comment

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

I think I wasn't clear. This is a good test to have but it isn't exactly what I meant when I said add a test for a static array. Relook at what you have written and consider the following problem.

Let's assume we create an event of the signature event A(uint[3] indexed a, uint[3] b, string c). Currently it is my belief that your current implementation will fail this test and overflow.

}, {
pledgeData1,
&[]interface{}{new(int), 0, 0},
&[]interface{}{},
jsonEventPledge,
"abi: cannot unmarshal common.Address in to int",
"Can not unpack Pledge event into slice with wrong types",
}, {
pledgeData1,
&BadEventPledge{},
&BadEventPledge{},
jsonEventPledge,
"abi: cannot unmarshal common.Address in to string",
"Can not unpack Pledge event into struct with wrong filed types",
}, {
pledgeData1,
&[]interface{}{common.Address{}, new(big.Int)},
&[]interface{}{},
jsonEventPledge,
"abi: insufficient number of elements in the list/array for unpack, want 3, got 2",
"Can not unpack Pledge event into too short slice",
}, {
pledgeData1,
new(map[string]interface{}),
&[]interface{}{},
jsonEventPledge,
"abi: cannot unmarshal tuple into map[string]interface {}",
"Can not unpack Pledge event into map",
}}

for _, tc := range testCases {
assert := assert.New(t)
tc := tc
t.Run(tc.name, func(t *testing.T) {
err := unpackTestEventData(tc.dest, tc.data, tc.jsonLog, assert)
if tc.error == "" {
assert.Nil(err, "Should be able to unpack event data.")
assert.Equal(tc.expected, tc.dest)
} else {
assert.EqualError(err, tc.error)
}
})
}
}

func unpackTestEventData(dest interface{}, hexData string, jsonEvent []byte, assert *assert.Assertions) error {
data, err := hex.DecodeString(hexData)
assert.NoError(err, "Hex data should be a correct hex-string")
var e Event
assert.NoError(json.Unmarshal(jsonEvent, &e), "Should be able to unmarshal event ABI")
a := ABI{Events: map[string]Event{"e": e}}
return a.Unpack(dest, "e", data)
}
Loading