add methods to abi that allow for the generic unpacking of event logs into map[string]interface{}

[i-norden]

This PR introduces an abi.UnpackIntoMap method which can be used to unpack an event without having had to first define a struct with matching field names to unpack it into. This allows for dynamic unpacking of event logs using a contract's ABI. Additionally, since the purpose of this method is to trade some type safety for flexibility, this method uses a bind.parseTopicsIntoMap function which allows for unpacking of event logs which have an indexed string.

While it makes sense to me that we want to exclude strings and other types of unbound size from topics, there is nothing stopping this from being implemented on the contract side of things (and it is) so I think it would be good to have a method to handle this case. Again, since this PR creates a new method for unpacking, we can introduce this capability without weakening the safety of the abi.Unpack method.

[i-norden]

Any interest in this PR? I can make the tests more thorough if that would help.

[gballet]

Thank you for your contribution. My comments are mostly style and requesting a more unit tests.

[gballet]

Thanks for those tests. I think there should be a couple more:

• test the "receive" function for inputs, check nothing ends up in the map
• add a function with only outputs, check its values end up in the map
• add one with both inputs and outputs, check only outputs values end up in the map
• add a test with a naming conflict

Could you also please break this function into several, each testing a different aspect? This way, when it fails we know exactly where the issue is.

[gballet]

is the String() conversion necessary?

[i-norden]

Yeah, as a pointer they aren't pointing to the same big.Int, so even though they have the same value they won't be equivalent.

[gballet]

Ah right. Then please use Cmp (see here) instead

[gballet]

Same request as in abi_test.go

[i-norden]

Thanks for the review @gballet!! I will make the requested changes shortly.

[gballet]

@i-norden I've made a partial review of your changes. There are some further issues with the lint build:

accounts/abi/abi_test.go:731:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/abi_test.go:792:5:warning: should use !bytes.Equal(getMap["hash"].([]byte), expectedBytes) instead (S1004) (gosimple)
accounts/abi/abi_test.go:878:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/bind/base_test.go:133:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/bind/base_test.go:186:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/bind/base_test.go:239:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/bind/base_test.go:294:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/bind/base_test.go:344:5:warning: should use !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead (S1004) (gosimple)
accounts/abi/bind/topics.go:234:11:warning: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (S1028) (gosimple)

Nothing terminal, just some coding style changes to satisfy the Most Enlightened Golang Linter:tm:

[mewwts]

Quick question/feedback: Why don't we fit this into the Unpack function? I was thinking you could do this by adding a

if reflect.ValueOf(v).Kind() == reflect.Map {
	return arguments.unpackMap(v, marshalledValues)
}

in Unpack.

Also; just making sure - this would work when unpacking arguments to / from function calls as well?

EDIT: forgot the reflect.ValueOf call in the example

[gballet]

To fix the linter error, you need to replace this with fmt.Errorf("unsupported...

[gballet]

to fix the linter issue, you need to write it as !bytes.Equal(receivedMap["memo"].([]byte), expectedReceivedMap["memo"].([]byte)) instead

[gballet]

@i-norden could you please fix the linter issues? I would do it myself but you have not allowed me to update your branch;

[i-norden]

@gballet sorry for the delay, I've been distracted! On it now. I am also trying to tease out a bug while using this on live data with https://github.com/vulcanize/account_transformers. I'm not yet sure if it is an issue specific to this new method but I am coming across TransferWithData events that cannot be unpacked (getting an abi: cannot marshal in to go slice: offset 900000000000032 would go over slice boundary (len=32) error).

@mewwts This method can only be used to unpack method outputs, similar to the Unpack method.

We could embed this within Unpack like

func (abi ABI) Unpack(v interface{}, name string, data []byte) (err error) {
	if len(data) == 0 {
		return fmt.Errorf("abi: unmarshalling empty output")
	}
	vMap, ok := v.(map[string]interface{})
	if ok {
		if method, ok := abi.Methods[name]; ok {
			if len(data)%32 != 0 {
				return fmt.Errorf("abi: improperly formatted output")
			}
			return method.Outputs.UnpackIntoMap(vMap, data)
		}
		if event, ok := abi.Events[name]; ok {
			return event.Inputs.UnpackIntoMap(vMap, data)
		}
	}
	// since there can't be naming collisions with contracts and events,
	// we need to decide whether we're calling a method or an event
	if method, ok := abi.Methods[name]; ok {
		if len(data)%32 != 0 {
			return fmt.Errorf("abi: improperly formatted output: %s - Bytes: [%+v]", string(data), data)
		}
		return method.Outputs.Unpack(v, data)
	}
	if event, ok := abi.Events[name]; ok {
		return event.Inputs.Unpack(v, data)
	}
	return fmt.Errorf("abi: could not locate named method or event")
}

but I'm not sure it is preferable to having the separate method, the behavior is different enough that I think it makes sense to keep them separate. And it's super minor but there is a cost to the type assertion. @gballet do you have a preference here?

Thanks!

[i-norden]

The error I was running into was caused by the event diverging from the standard and having the wrong indexed arguments, nothing wrong here. Made the linting changes and left as separate method for now.

[mewwts]

Sorry for the inaccuracy in my previous comment; my thinking was to expose this functionality in arguments Unpack: https://github.com/ethereum/go-ethereum/blob/fa6a4e45560e1968ae609c37000fe94887de2a28/accounts/abi/argument.go#L90

by checking reflect.ValueOf(v).Kind() before the final return.

It could look something like

func (arguments Arguments) Unpack(v interface{}, data []byte) error {
	// make sure the passed value is arguments pointer
	if reflect.Ptr != reflect.ValueOf(v).Kind() {
		return fmt.Errorf("abi: Unpack(non-pointer %T)", v)
	}
	marshalledValues, err := arguments.UnpackValues(data)
	if err != nil {
		return err
	}
	if arguments.isTuple() {
		return arguments.unpackTuple(v, marshalledValues)
	}
	if reflect.ValueOf(v).Kind() != reflect.Map {
		return arguments.unpackMap(v, marshalledValues)
	}
	return arguments.unpackAtomic(v, marshalledValues[0])
}

[gballet]

LGTM