From f3f1e59eea922e5fb0d78d55aba471c35c20b022 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 13 May 2020 17:50:18 +0200 Subject: [PATCH] accounts/abi: simplify reflection logic (#21058) * accounts/abi: simplified reflection logic * accounts/abi: simplified reflection logic * accounts/abi: removed unpack * accounts/abi: removed comments * accounts/abi: removed uneccessary complications * accounts/abi: minor changes in error messages * accounts/abi: removed unnused code * accounts/abi: fixed indexed argument unpacking * accounts/abi: removed superfluous test cases This commit removes two test cases. The first one is trivially invalid as we have the same test cases as passing in packing_test.go L375. The second one passes now, because we don't need the mapArgNamesToStructFields in unpack_atomic anymore. Checking for purely underscored arg names generally should not be something we do as the abi/contract is generally out of the control of the user. * accounts/abi: removed comments, debug println * accounts/abi: added commented out code * accounts/abi: addressed comments * accounts/abi: remove unnecessary dst.CanSet check * accounts/abi: added dst.CanSet checks --- accounts/abi/argument.go | 142 ++++++------------------------------ accounts/abi/event_test.go | 4 +- accounts/abi/reflect.go | 85 ++++++++++++--------- accounts/abi/unpack_test.go | 8 +- 4 files changed, 81 insertions(+), 158 deletions(-) diff --git a/accounts/abi/argument.go b/accounts/abi/argument.go index 5c1e391f5..81151ef0e 100644 --- a/accounts/abi/argument.go +++ b/accounts/abi/argument.go @@ -122,149 +122,55 @@ func (arguments Arguments) UnpackIntoMap(v map[string]interface{}, data []byte) return nil } -// unpack sets the unmarshalled value to go format. -// Note the dst here must be settable. -func unpack(t *Type, dst interface{}, src interface{}) error { - var ( - dstVal = reflect.ValueOf(dst).Elem() - srcVal = reflect.ValueOf(src) - ) - tuple, typ := false, t - for { - if typ.T == SliceTy || typ.T == ArrayTy { - typ = typ.Elem - continue - } - tuple = typ.T == TupleTy - break - } - if !tuple { - return set(dstVal, srcVal) - } - - // Dereferences interface or pointer wrapper - dstVal = indirectInterfaceOrPtr(dstVal) - - switch t.T { - case TupleTy: - if dstVal.Kind() != reflect.Struct { - return fmt.Errorf("abi: invalid dst value for unpack, want struct, got %s", dstVal.Kind()) - } - fieldmap, err := mapArgNamesToStructFields(t.TupleRawNames, dstVal) - if err != nil { - return err - } - for i, elem := range t.TupleElems { - fname := fieldmap[t.TupleRawNames[i]] - field := dstVal.FieldByName(fname) - if !field.IsValid() { - return fmt.Errorf("abi: field %s can't found in the given value", t.TupleRawNames[i]) - } - if err := unpack(elem, field.Addr().Interface(), srcVal.Field(i).Interface()); err != nil { - return err - } - } - return nil - case SliceTy: - if dstVal.Kind() != reflect.Slice { - return fmt.Errorf("abi: invalid dst value for unpack, want slice, got %s", dstVal.Kind()) - } - slice := reflect.MakeSlice(dstVal.Type(), srcVal.Len(), srcVal.Len()) - for i := 0; i < slice.Len(); i++ { - if err := unpack(t.Elem, slice.Index(i).Addr().Interface(), srcVal.Index(i).Interface()); err != nil { - return err - } - } - dstVal.Set(slice) - case ArrayTy: - if dstVal.Kind() != reflect.Array { - return fmt.Errorf("abi: invalid dst value for unpack, want array, got %s", dstVal.Kind()) - } - array := reflect.New(dstVal.Type()).Elem() - for i := 0; i < array.Len(); i++ { - if err := unpack(t.Elem, array.Index(i).Addr().Interface(), srcVal.Index(i).Interface()); err != nil { - return err - } - } - dstVal.Set(array) - } - return nil -} - // unpackAtomic unpacks ( hexdata -> go ) a single value func (arguments Arguments) unpackAtomic(v interface{}, marshalledValues interface{}) error { - nonIndexedArgs := arguments.NonIndexed() - if len(nonIndexedArgs) == 0 { - return nil - } - argument := nonIndexedArgs[0] - elem := reflect.ValueOf(v).Elem() + dst := reflect.ValueOf(v).Elem() + src := reflect.ValueOf(marshalledValues) - if elem.Kind() == reflect.Struct && argument.Type.T != TupleTy { - fieldmap, err := mapArgNamesToStructFields([]string{argument.Name}, elem) - if err != nil { - return err - } - field := elem.FieldByName(fieldmap[argument.Name]) - if !field.IsValid() { - return fmt.Errorf("abi: field %s can't be found in the given value", argument.Name) - } - return unpack(&argument.Type, field.Addr().Interface(), marshalledValues) + if dst.Kind() == reflect.Struct && src.Kind() != reflect.Struct { + return set(dst.Field(0), src) } - return unpack(&argument.Type, elem.Addr().Interface(), marshalledValues) + return set(dst, src) } // unpackTuple unpacks ( hexdata -> go ) a batch of values. func (arguments Arguments) unpackTuple(v interface{}, marshalledValues []interface{}) error { - var ( - value = reflect.ValueOf(v).Elem() - typ = value.Type() - kind = value.Kind() - nonIndexedArgs = arguments.NonIndexed() - ) - if err := requireUnpackKind(value, len(nonIndexedArgs), arguments); err != nil { - return err - } + value := reflect.ValueOf(v).Elem() + nonIndexedArgs := arguments.NonIndexed() - // If the interface is a struct, get of abi->struct_field mapping - var abi2struct map[string]string - if kind == reflect.Struct { + switch value.Kind() { + case reflect.Struct: argNames := make([]string, len(nonIndexedArgs)) for i, arg := range nonIndexedArgs { argNames[i] = arg.Name } var err error - if abi2struct, err = mapArgNamesToStructFields(argNames, value); err != nil { + abi2struct, err := mapArgNamesToStructFields(argNames, value) + if err != nil { return err } - } - for i, arg := range nonIndexedArgs { - switch kind { - case reflect.Struct: + for i, arg := range nonIndexedArgs { field := value.FieldByName(abi2struct[arg.Name]) if !field.IsValid() { return fmt.Errorf("abi: field %s can't be found in the given value", arg.Name) } - if err := unpack(&arg.Type, field.Addr().Interface(), marshalledValues[i]); err != nil { + if err := set(field, reflect.ValueOf(marshalledValues[i])); 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(arguments), value.Len()) - } - v := value.Index(i) - if err := requireAssignable(v, reflect.ValueOf(marshalledValues[i])); err != nil { - return err - } - if err := unpack(&arg.Type, v.Addr().Interface(), marshalledValues[i]); err != nil { - return err - } - default: - return fmt.Errorf("abi:[2] cannot unmarshal tuple in to %v", typ) } + case reflect.Slice, reflect.Array: + if value.Len() < len(marshalledValues) { + return fmt.Errorf("abi: insufficient number of arguments for unpack, want %d, got %d", len(arguments), value.Len()) + } + for i := range nonIndexedArgs { + if err := set(value.Index(i), reflect.ValueOf(marshalledValues[i])); err != nil { + return err + } + } + default: + return fmt.Errorf("abi:[2] cannot unmarshal tuple in to %v", value.Type()) } return nil - } // UnpackValues can be used to unpack ABI-encoded hexdata according to the ABI-specification, diff --git a/accounts/abi/event_test.go b/accounts/abi/event_test.go index 28da4c502..1f19a2741 100644 --- a/accounts/abi/event_test.go +++ b/accounts/abi/event_test.go @@ -312,14 +312,14 @@ func TestEventTupleUnpack(t *testing.T) { &[]interface{}{common.Address{}, new(big.Int)}, &[]interface{}{}, jsonEventPledge, - "abi: insufficient number of elements in the list/array for unpack, want 3, got 2", + "abi: insufficient number of arguments 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 {}", + "abi:[2] cannot unmarshal tuple in to map[string]interface {}", "Can not unpack Pledge event into map", }, { mixedCaseData1, diff --git a/accounts/abi/reflect.go b/accounts/abi/reflect.go index 4bb6c4fa0..75f1a00a5 100644 --- a/accounts/abi/reflect.go +++ b/accounts/abi/reflect.go @@ -17,6 +17,7 @@ package abi import ( + "errors" "fmt" "math/big" "reflect" @@ -32,14 +33,6 @@ func indirect(v reflect.Value) reflect.Value { return v } -// indirectInterfaceOrPtr recursively dereferences the value until value is not interface. -func indirectInterfaceOrPtr(v reflect.Value) reflect.Value { - if (v.Kind() == reflect.Interface || v.Kind() == reflect.Ptr) && v.Elem().IsValid() { - return indirect(v.Elem()) - } - return v -} - // reflectIntType returns the reflect using the given size and // unsignedness. func reflectIntType(unsigned bool, size int) reflect.Type { @@ -90,7 +83,11 @@ func set(dst, src reflect.Value) error { case srcType.AssignableTo(dstType) && dst.CanSet(): dst.Set(src) case dstType.Kind() == reflect.Slice && srcType.Kind() == reflect.Slice && dst.CanSet(): - setSlice(dst, src) + return setSlice(dst, src) + case dstType.Kind() == reflect.Array: + return setArray(dst, src) + case dstType.Kind() == reflect.Struct: + return setStruct(dst, src) default: return fmt.Errorf("abi: cannot unmarshal %v in to %v", src.Type(), dst.Type()) } @@ -100,33 +97,55 @@ func set(dst, src reflect.Value) error { // setSlice attempts to assign src to dst when slices are not assignable by default // e.g. src: [][]byte -> dst: [][15]byte // setSlice ignores if we cannot copy all of src' elements. -func setSlice(dst, src reflect.Value) { +func setSlice(dst, src reflect.Value) error { slice := reflect.MakeSlice(dst.Type(), src.Len(), src.Len()) for i := 0; i < src.Len(); i++ { - reflect.Copy(slice.Index(i), src.Index(i)) - } - dst.Set(slice) -} - -// requireAssignable assures that `dest` is a pointer and it's not an interface. -func requireAssignable(dst, src reflect.Value) error { - if dst.Kind() != reflect.Ptr && dst.Kind() != reflect.Interface { - return fmt.Errorf("abi: cannot unmarshal %v into %v", src.Type(), dst.Type()) - } - return nil -} - -// requireUnpackKind verifies preconditions for unpacking `args` into `kind` -func requireUnpackKind(v reflect.Value, minLength int, args Arguments) error { - switch v.Kind() { - case reflect.Struct: - case reflect.Slice, reflect.Array: - if v.Len() < minLength { - return fmt.Errorf("abi: insufficient number of elements in the list/array for unpack, want %d, got %d", - minLength, v.Len()) + if src.Index(i).Kind() == reflect.Struct { + if err := set(slice.Index(i), src.Index(i)); err != nil { + return err + } + } else { + // e.g. [][32]uint8 to []common.Hash + if err := set(slice.Index(i), src.Index(i)); err != nil { + return err + } + } + } + if dst.CanSet() { + dst.Set(slice) + return nil + } + return errors.New("Cannot set slice, destination not settable") +} + +func setArray(dst, src reflect.Value) error { + array := reflect.New(dst.Type()).Elem() + min := src.Len() + if src.Len() > dst.Len() { + min = dst.Len() + } + for i := 0; i < min; i++ { + if err := set(array.Index(i), src.Index(i)); err != nil { + return err + } + } + if dst.CanSet() { + dst.Set(array) + return nil + } + return errors.New("Cannot set array, destination not settable") +} + +func setStruct(dst, src reflect.Value) error { + for i := 0; i < src.NumField(); i++ { + srcField := src.Field(i) + dstField := dst.Field(i) + if !dstField.IsValid() || !srcField.IsValid() { + return fmt.Errorf("Could not find src field: %v value: %v in destination", srcField.Type().Name(), srcField) + } + if err := set(dstField, srcField); err != nil { + return err } - default: - return fmt.Errorf("abi: cannot unmarshal tuple into %v", v.Type()) } return nil } diff --git a/accounts/abi/unpack_test.go b/accounts/abi/unpack_test.go index 7a590b4b4..767d1540e 100644 --- a/accounts/abi/unpack_test.go +++ b/accounts/abi/unpack_test.go @@ -120,8 +120,7 @@ var unpackTests = []unpackTest{ { def: `[{"type": "bytes"}]`, enc: "000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000200100000000000000000000000000000000000000000000000000000000000000", - want: [32]byte{}, - err: "abi: cannot unmarshal []uint8 in to [32]uint8", + want: [32]byte{1}, }, { def: `[{"type": "bytes32"}]`, @@ -135,8 +134,7 @@ var unpackTests = []unpackTest{ want: struct { IntOne *big.Int Intone *big.Int - }{}, - err: "abi: purely underscored output cannot unpack to struct", + }{IntOne: big.NewInt(1)}, }, { def: `[{"name":"int_one","type":"int256"},{"name":"IntOne","type":"int256"}]`, @@ -362,7 +360,7 @@ func TestMethodMultiReturn(t *testing.T) { }, { &[]interface{}{new(int)}, &[]interface{}{}, - "abi: insufficient number of elements in the list/array for unpack, want 2, got 1", + "abi: insufficient number of arguments for unpack, want 2, got 1", "Can not unpack into a slice with wrong types", }} for _, tc := range testCases {