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

@@ -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()
+	dst := reflect.ValueOf(v).Elem()
-	if len(nonIndexedArgs) == 0 {
+	src := reflect.ValueOf(marshalledValues)
-		return nil
-	}
-	argument := nonIndexedArgs[0]
-	elem := reflect.ValueOf(v).Elem()
 
-	if elem.Kind() == reflect.Struct && argument.Type.T != TupleTy {
+	if dst.Kind() == reflect.Struct && src.Kind() != reflect.Struct {
-		fieldmap, err := mapArgNamesToStructFields([]string{argument.Name}, elem)
+		return set(dst.Field(0), src)
-		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)
 	}
-	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()
-		value          = reflect.ValueOf(v).Elem()
+	nonIndexedArgs := arguments.NonIndexed()
-		typ            = value.Type()
-		kind           = value.Kind()
-		nonIndexedArgs = arguments.NonIndexed()
-	)
-	if err := requireUnpackKind(value, len(nonIndexedArgs), arguments); err != nil {
-		return err
-	}
 
-	// If the interface is a struct, get of abi->struct_field mapping
+	switch value.Kind() {
-	var abi2struct map[string]string
+	case reflect.Struct:
-	if kind == 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 {
-	for i, arg := range nonIndexedArgs {
-		switch kind {
-		case reflect.Struct:
 			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,

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,

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))
+		if src.Index(i).Kind() == reflect.Struct {
-	}
+			if err := set(slice.Index(i), src.Index(i)); err != nil {
-	dst.Set(slice)
+				return err
-}
+			}
-
+		} else {
-// requireAssignable assures that `dest` is a pointer and it's not an interface.
+			// e.g. [][32]uint8 to []common.Hash
-func requireAssignable(dst, src reflect.Value) error {
+			if err := set(slice.Index(i), src.Index(i)); err != nil {
-	if dst.Kind() != reflect.Ptr && dst.Kind() != reflect.Interface {
+				return err
-		return fmt.Errorf("abi: cannot unmarshal %v into %v", src.Type(), dst.Type())
+			}
-	}
+		}
-	return nil
+	}
-}
+	if dst.CanSet() {
-
+		dst.Set(slice)
-// requireUnpackKind verifies preconditions for unpacking `args` into `kind`
+		return nil
-func requireUnpackKind(v reflect.Value, minLength int, args Arguments) error {
+	}
-	switch v.Kind() {
+	return errors.New("Cannot set slice, destination not settable")
-	case reflect.Struct:
+}
-	case reflect.Slice, reflect.Array:
+
-		if v.Len() < minLength {
+func setArray(dst, src reflect.Value) error {
-			return fmt.Errorf("abi: insufficient number of elements in the list/array for unpack, want %d, got %d",
+	array := reflect.New(dst.Type()).Elem()
-				minLength, v.Len())
+	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
 }

accounts/abi/unpack_test.go

@@ -120,8 +120,7 @@ var unpackTests = []unpackTest{
 	{
 		def:  `[{"type": "bytes"}]`,
 		enc:  "000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000200100000000000000000000000000000000000000000000000000000000000000",
-		want: [32]byte{},
+		want: [32]byte{1},
-		err:  "abi: cannot unmarshal []uint8 in to [32]uint8",
 	},
 	{
 		def:  `[{"type": "bytes32"}]`,
@@ -135,8 +134,7 @@ var unpackTests = []unpackTest{
 		want: struct {
 			IntOne *big.Int
 			Intone *big.Int
-		}{},
+		}{IntOne: big.NewInt(1)},
-		err: "abi: purely underscored output cannot unpack to struct",
 	},
 	{
 		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 {