accounts/abi: include fixed array size in offset for dynamic type

[yondonfu]

Currently, when ABI encoding arguments the offset in bytes to the start of the data area for dynamic types is calculated as len(method.Inputs)*32 + len(variableInput) where len(method.Inputs)*32 is the number of arguments multiplied by 32 bytes per argument and len(variableInput) is the byte length of the output appended to the end of packed output (used for dynamic types i.e strings and bytes). According to the ABI specification, each element of a fixed size array should take up 32 bytes i.e.uint256[0] takes up 32 bytes and uint256[1] takes up 32 bytes`. This not currently reflected in the calculation of the offset in bytes to the start of the data area for dynamic types.

This includes a fix for the offset calculation along with a test.

[VoR0220]

Barring a change that I have not noticed recently, I'm pretty sure that the ABI length of the static array is inferred from the type name, and thus does not require an offset. If it were a dynamic array however, then yes, it would need that in there. Dynamic array implementation is in this PR as well...lotta stuff here: #14743

[yondonfu]

To clarify I mean that the size of static arrays needs to be taken into account when calculating the offset used for dynamic types. An example:

Suppose we have the function function foo(string a, uint256[2] b) and we call foo("hello", [1, 2]). The ABI encoded arguments in the transaction data should look like:

9c6f5103 # Function selector
0000000000000000000000000000000000000000000000000000000000000060 # offset to "hello" = 96 bytes
0000000000000000000000000000000000000000000000000000000000000001 # 1 of [1,2]
0000000000000000000000000000000000000000000000000000000000000002 # 2 of [1,2]
0000000000000000000000000000000000000000000000000000000000000005 # size of "hello"
68656c6c6f000000000000000000000000000000000000000000000000000000 # "hello"

But, the way the offset for dynamic types is currently calculated would instead result in:

9c6f5103 # Function selector
0000000000000000000000000000000000000000000000000000000000000040 # offset to "hello" = 64 bytes
0000000000000000000000000000000000000000000000000000000000000001 # 1 of [1,2]
0000000000000000000000000000000000000000000000000000000000000002 # 2 of [1,2]
0000000000000000000000000000000000000000000000000000000000000005 # size of "hello"
68656c6c6f000000000000000000000000000000000000000000000000000000 # "hello"

The offset for "hello" should be 96 bytes (32 bytes for the offset, 2 * 32 bytes for the static array [1,2]), but it ends up being 64 bytes (32 bytes for the offset, 32 bytes for the static array).

[gballet]

Testing the output bytes is useful, but ultimately you should also check that it is possible to unpack what has been packed.

[yondonfu]

It appears that the unpacker interface currently supports unpacking method outputs, but not unpacking method inputs (feel free to correct me if I'm wrong here). Seems like adding support would be a bigger update and not sure if that should be a part of this PR or not?

[holiman]

Heh, did you see my comment here:#15702 (comment) ?

Yes, unpacking method inputs would be a very useful addition. I'm not sure what the ABI-package status is now, haven't kept track of the various (three?) PR:s that are in progress.

[holiman]

I agree that it doesn't have to be part of this PR, though. If we merge the current PR:s, I believe that making abi-unpacking more generic (supporting both input and output) can be added afterwards.

[yondonfu]

Sounds good to me, happy to help out with making abi unpacking more generic in a separate PR if needed as well

[gballet]

It would make sense to test more functions, accepting a mixed amount of arguments. For example, put the fixed array first, have several arrays (some dynamic, others not) and so on. You never know what kind of change will come after you.

[yondonfu]

Didn't have a chance to follow up on this PR until now, but I added additional test cases that include different method input combinations that use fixed and dynamic arrays. Also updated the code in method.go to adhere to the latest changes in type.go. Let me know if there any additional todos to get this merged!