# Proposal to refactor package ABI

*Status: Draft
Author: Marius van der Wijden
Last Updated: 11 Mai 2020
Team: Geth*

## Motivation
The ABI package is very organically grown and provides several features that are not explicitly defined but implicitly assumed in tests. Generally the ABI package provides four core functionalities:

1. Creating new argument definitions from JSON
2. Packing structures that adhere to the argument definitions into byte slices
3. Unpacking given results into the structures defined by the arguments (currently called unmarshalling)
4. Copy the data from unmarshalled structures into provided structures
5. Handle indexed arguments (topics)

Especially the second two functionalities are not well defined.

## Differentiate between unmarshalling/unpacking and copying

Currently the abi package implicitly differentiates between unmarshalling and unpacking. The method `arguments.UnpackValues(data)` is used for unmarshalling. This method creates *new* data types based on the prototypes available in `arguments`. Since the datatypes are created strictly after the abi definition, unmarshalling should only fail if theres a missmatch between the provided data and the abi definition. Arguments are assigned based on structure alone.

Since the package implicitly (through tests) expects certain characteristics (e.g. unpacking into a datatype with overlapping names figures out the appropriate name), we need a method to assign the values that we unpacked to a provided struct. These provided structures do not have to abide by to the structure given in the abi definition. This job is currently handled in `arguments.unpackTuple(v, marshalledValues)` and `arguments.unpackAtomic(v, marshalledValues[0])`.
These functions recursively copy every value from the structures in `marshalledValues` to an appropriate value in `v`.

These distinctions is not made explicit. For example `arguments.Unpack(v, data)` employs both unmarshalling and copying while `arguments.UnpackIntoMap(v, data)` only sets the unmarshalled values into a map. I would like to make a bigger distinction, especially namewise between the two concepts.

## Only use unmarshalling in bind

Currently the autogenerated code from bind only uses `arguments.Unpack(v, data)` which in turn does both unmarshalling and copying. With the explicit differentiation between unmarshalling and copying in place we could only use the unmarshalling in the autogenerated methods as the . The unmarshalling + copying can still be used if a result struct is provided to `boundcontract.Call()`. If no interface is provided, `Call()` will only use unmarshalling.

Since we know the expected structure in advance (through the binding), we don't need to do the copy anymore. We can just cast the results into the autogenerated structs provided by bind.

## Move topics into abi

Indexed arguments are handled a bit differently than unindexed arguments (e.g. Solidity allows no tuple arguments to be indexed, for array types their keccak256 hash is stored instead). Currently handling of indexed arguments is done in the bind package, while unindexed arguments are handled in the abi package. This resulted in duplicated logic in the past (already fixed). However in order to group the responsibilites in a sensible way, we should move the topics to the abi package.

# New Design
Please note the change from `v interface{}` to `v []interface{}`.
As both events and functions can have multiple return arguments, it is easier and more sensible in my opinion to return a slice of return arguments than create a new structure that contains the return arguments.

Unpack should only handle unmarshalling, not copying:

// Unpack performs the operation hexdata -> Go format
func (arguments Arguments) Unpack(data []byte) (error, []interface{}) {
	/// ...
	// make sure the passed value is arguments pointer
	if reflect.Ptr != reflect.ValueOf(v).Kind() {
		return fmt.Errorf("abi: Unpack(non-pointer %T)", v)
	}
	return arguments.UnpackValues(data)
}

New methods for copying:

// Copy performs the operation Go format -> provided Struct 
func (arguments Arguments) Copy(v []interface{}, values []interface{}) error {
	/// ...
    if len(values) == 0 {
		return fmt.Errorf("abi: Copy(no-values  %T)", v)
	}
	if arguments.isTuple() {
		return arguments.copyTuple(v, marshalledValues)
	}
	return arguments.copyAtomic(v[0], marshalledValues[0])
}

// copyTuple copies a batch of values
// (used to be unpackTuple)
func (arguments Arguments) copyTuple(v []interface{}, values []interface{}) error
// copyAtomic copies a single value
// (used to be unpackAtomic)
func (arguments Arguments) copyAtomic(v interface{}, value interface{}) error

ABI unpack would then become something like the following:
Please note the change from `v interface{}` to `v []interface{}`.

// Unpack output in v according to the abi specification.
// If v is the empty interface, we return the unmarshalled values directly.
func (abi ABI) Unpack(v []interface{}, name string, data []byte) (err error) {
	// since there can't be naming collisions with contracts and events,
	// we need to decide whether we're calling a method or an event
    var args Arguments
    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)
        }
        args = method.Outputs
    }
    if event, ok := abi.Events[name]; ok {
        args = method.Inputs
    }
    if args == nil {
        return errors.New("abi: could not locate named method or event")
    }
    ifs, err := args.Unpack(v, data)
    if err != nil {
        return err
    }
    
    if len(v) != 0 {
        return args.Copy(v, ifs)
    } 
    v = ifs
    return nil
}

This allows us in the autogenerated code to do stuff like this

// G is a free data retrieval call binding the contract method 0xe2179b8e.
//
// Solidity: function g() pure returns(TupleTestS, T, uint256)
func (_TupleTest *TupleTestCaller) G(opts *bind.CallOpts) (TupleTestS, T, *big.Int, error) {
    var out []interface{}
	err := _TupleTest.contract.Call(opts, out, "g")
    return out[0].(TupleTestS), out[1].(T), out[2].(*big.Int), err
}

## Actionable Items

* Move topics from account/abi/bind to accounts/abi
* Make distinction between unmarshalling and copying
* Use unmarshalling in generated code (but allow for custom structs)