Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add array support for tuples #90

Merged
merged 8 commits into from
Aug 9, 2018

Conversation

tristan
Copy link
Contributor

@tristan tristan commented Aug 7, 2018

What was wrong?

eth-abi does not support encoding or decoding arrays of tuples
e.g. the example contract at http://solidity.readthedocs.io/en/latest/abi-spec.html#handling-tuple-types has method signature of:
function f(S memory s, T memory t, uint a) public { }
whose type string is represented as
(uint256,uint256[],(uint256,uint256)[]),(uint256,uint256),uint256

calling the function requires passing the data:

0x6f2be728000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000009000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000500000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000007

How was it fixed?

I updated the grammar code to support optional arrays on tuples, and modified the tuple encoder/decoder to support processing arrays.

Missing

still potentially missing is support for multidimensional arrays. e.g. the following tests should also pass, but even the simple case int[][] doesn't. it seems this might require a bit more architectural work, which might be better for someone more familiar with the code to handle.

    (
        '(int[][])',
        ((1, 2), (3, 4)),
        words('20', '2', '40', 'a0', '2', '1', '2', '2', '3', '4')
    ),
    (
        '((int,int)[][])',
        (((1, 2), (3, 4)), ((5, 6), (7, 8))),
        words('20', '2', '40', 'e0', '2' '1', '2', '3', '4', '2', '5', '6', '7', '8')
    ),
    (
        '((int,int)[][2])',
        (((1, 2), (3, 4)), ((5, 6), (7, 8))),
        words('20', '40', 'e0', '2' '1', '2', '3', '4', '2', '5', '6', '7', '8')
    ),

Cute Animal Picture

Cute animal picture

Copy link
Contributor

@davesque davesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thanks for doing all this work! I'll have to take a closer look at the multi-dimensional array issue and see if I can figure out what's going on. I'll do that before I merge this in.

@davesque davesque merged commit 2edf081 into ethereum:master Aug 9, 2018
@tristan tristan deleted the array_support_for_tuples branch August 10, 2018 14:28
@tristan
Copy link
Contributor Author

tristan commented Aug 10, 2018

👍 much cleaner than my attempt 😁

still some things missing with arrays (both normal and multidimensional) though. it works fine when the object is a tuple, but not when it is a base type.

https://solidity.readthedocs.io/en/develop/abi-spec.html#use-of-dynamic-types has some examples
and I would expect the following, when added to CORRECT_SINGLE_ENCODINGS, should pass

('uint256[]', (1, 2), words('20', '2', '1', '2')),
('uint256[][]', ((1, 2), (3, 4)), words('20', '2', '40', 'a0', '2', '1', '2', '2', '3', '4'))

but it seems the DynamicArrayEncoder sees the first (offset) byte as the size rather than an offset, and fails from there, and the DynamicArrayEncoder doesn't encode any offset information into the result.

@davesque
Copy link
Contributor

@tristan Neither of those examples that you gave are tuple types so their encodings won't have any offset words. For dynamic length arrays, the length is the first word in the encoding. From the solidity ABI spec here:
screen shot 2018-08-10 at 12 41 49 pm

@davesque
Copy link
Contributor

davesque commented Aug 10, 2018

@tristan I think the confusion is that, in the example you cite, all of the arguments to the function are encoded as a single tuple type. If you want to see a head-tail encoding, you need to do something like this:

In [1]: from eth_abi import encode_single

In [2]: from eth_utils import encode_hex

In [3]: out = encode_single('(uint,uint32[],bytes10,bytes)', (0x123, [0x456, 0x789], b"1234567890", b"Hello, world!"))

In [4]: out = encode_hex(out)[2:]

In [5]: for i in range(len(out) // 64):
   ...:     print(out[i * 64:(i + 1) * 64])
   ...:
0000000000000000000000000000000000000000000000000000000000000123
0000000000000000000000000000000000000000000000000000000000000080
3132333435363738393000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000000000000000000e0
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000456
0000000000000000000000000000000000000000000000000000000000000789
000000000000000000000000000000000000000000000000000000000000000d
48656c6c6f2c20776f726c642100000000000000000000000000000000000000

The important line above is line 3. The other ones after that are just to format the result nicely to match the documentation you cited.

@tristan
Copy link
Contributor Author

tristan commented Aug 11, 2018

hmm, my first example definitely works fine, my bad there I think i was confusing my output from encode_abi which gives the values i was expecting.

but i still can't figure out things for the 2nd multidimensional case.

if you take the 2nd example from the spec (yours uses the first):
g(uint[][],string[]) with input ([[1, 2], [3]], ["one", "two", "three"])

using your code above, wrapping it in a tuple, i end up with this:

In [1]: from eth_abi import encode_single

In [2]: from eth_utils import encode_hex

In [3]: out = encode_single('(uint[][],string[])', ([[1, 2], [3]], ["one", "two", "three"]))

In [4]: out = encode_hex(out)[2:]

In [5]: for i in range(len(out) // 64):
   ...:     print(out[i * 64:(i + 1) * 64])
   ...:     
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000100
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000001
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000001
0000000000000000000000000000000000000000000000000000000000000003
0000000000000000000000000000000000000000000000000000000000000003
0000000000000000000000000000000000000000000000000000000000000003
6f6e650000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000003
74776f0000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000005
7468726565000000000000000000000000000000000000000000000000000000

which is not the same as what is in the example in the spec.

what i'm actually trying to do is use decode_abi on the results I receive from parity when calling this contract method (and encode_abi on the reverse, but i'll focus on decode here):

pragma solidity ^0.4.24;
pragma experimental ABIEncoderV2;

contract ABIv2Test {
  function testArrayMultidimensionalOutput() public view returns (uint[][]) {
    uint[] memory a = new uint[](2);
    a[0] = 1;
    a[1] = 2;
    uint[] memory b = new uint[](2);
    b[0] = 3;
    b[1] = 4;
    uint[][] memory rval = new uint[][](2);
    rval[0] = a;
    rval[1] = b;
    return rval;
  }
}

which from parity (eth_call with data: 0x7590a36e) i get:

0000000000000000000000000000000000000000000000000000000000000020
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000040
00000000000000000000000000000000000000000000000000000000000000a0
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000001
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000003
0000000000000000000000000000000000000000000000000000000000000004

and calling
decode_abi(['uint256[][]'], decode_hex(rval)) raises eth_abi.exceptions.InsufficientDataBytes: Tried to read 32 bytes. Only got 0 bytes
and decode_single with (uint256[][]) throws the same.

perhaps i'm missing something usage wise here?

@davesque
Copy link
Contributor

@tristan Actually, you aren't missing anything. I just checked the spec and this is a bug in our ABI library which has been there for a while. It appears that lists are supposed to be encoded exactly as if they were tuples. Honestly, I'm not sure why dynamic list elements are supposed to be encoded with an offset since I think they could also be encoded in-place without any ambiguity. Oh well -- I didn't write the spec! :/ Thanks for pointing this out!

@davesque
Copy link
Contributor

Actually, dynamic array elements must be encoded that way to facilitate constant-time indexing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants