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

Support for arrays of variable length types is broken #93

Closed
Arachnid opened this issue Aug 18, 2018 · 3 comments · Fixed by #92
Closed

Support for arrays of variable length types is broken #93

Arachnid opened this issue Aug 18, 2018 · 3 comments · Fixed by #92

Comments

@Arachnid
Copy link

Here's the canonical way (per Solidity, at least) to encode (bytes[]("0xf8a8fd6d", "0xf8a8fd6d")):

0000000000000000000000000000000000000000000000000000000000000020
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000080
0000000000000000000000000000000000000000000000000000000000000004
f8a8fd6d00000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000004
f8a8fd6d00000000000000000000000000000000000000000000000000000000

Broken down:

  • 0x00: A head component, consisting of an offset for the tail data (0x20)
  • 0x20: The tail of the array, specifying its length (2)
  • 0x40: The head component of the first element of the array: the offset for the tail data, relative to the start of the current tail data (0x40 + 0x40 = 0x80)
  • 0x60: The head component of the first element of the array: the offset for the tail data, relative to the start of the current tail data (0x80 + 0x40 = 0xC0)
  • 0x80: The length of the first array element (4)
  • 0xA0: The first array element (0xf8a8fd6d)
  • 0xC0: The length of the second array element (4)
  • 0xE0: The second array element (0xf8a8fd6d)

This is most easily thought of as a recursive encoding: The tail data for the first (and in this case, only) element of the ABI is the entire ABI encoding of that element.

In contrast, this library presently produces the current output:

0000000000000000000000000000000000000000000000000000000000000020
0000000000000000000000000000000000000000000000000000000000000002
0000000000000000000000000000000000000000000000000000000000000004
f8a8fd6d00000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000004
f8a8fd6d00000000000000000000000000000000000000000000000000000000

The head element, and the length of the array are correct, but then array elements are packed tightly, without the correct head/tail recursive encoding.

@carver
Copy link
Collaborator

carver commented Aug 19, 2018

Thanks @Arachnid - I wish all bug reports were this detailed! 😀 The good news is that this came up already, so it looks like the change is nearly merged in #92.

I marked that PR as closing this issue, so there will be an update when it's merged. If you'd like, I can also ping you when web3.py gets deployed with the eth-abi version that includes #92.

@pipermerriam
Copy link
Member

Also, I believe that @davesque is going to be submitting a PR to ethereum/tests to increase the coverage of the testing examples (I think there are current a whopping total of like 5). We could definitely use some help validating that the examples we submit are truly correct but I suspect that will shake out quickly once other implementations start testing against them.

@carver
Copy link
Collaborator

carver commented Aug 22, 2018

Unfortunately, I just realized that this was built into the master (v2) branch which isn't stable, or ready for inclusion into the stable Web3.py release. I'll see if I can backport it.

pacrob added a commit to pacrob/eth-abi that referenced this issue Dec 9, 2023
…sion

bump sphinx version and set py version rtd uses to 3.8
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 a pull request may close this issue.

3 participants