Tuple ABI support

[carver]

What was wrong?

Tuple types are not supported yet in Web3.py

How can it be fixed?

• Add tests for passing in & returning tuple values with contract functions
• Figure out how to handle web3.utils.abi.is_encodable when elements of the tuple are handled as special cases, like what if the function accepts this tuple as an argument: (address, string, (bytes, bytes32)) and someone passes in ('myname.eth', b'already-encoded', ('0x0123', '0x4567'))

One option to consider: write custom encoders for address, bytes*, and string, replacing the default ones. Then is_encodable would be replaced entirely with eth_abi.is_encodable

[pipermerriam]

We can probably special case the tuple case for address and string so that our is_encodable function knows how to walk tuple types. Any 3rd party types may just be out of luck for now if they want ENS names or fancy string handling.

[pipermerriam]

Lets define this as two phases.

1. support passing in the values only as actual tuples like your example.
2. support passing in richer objects like a dict or any object with all of the necessary attributes for the tuple so that fields of the tuple can have names.
3. support for returning richer versions of tuple return types, maybe via namedtuple or dict

[carver]

so that fields of the tuple can have names.

TIL that tuple components are named in the ABI. Nice

[stefanmendoza]

I'd be interested in tackling this! May need some guidance, but I'll start digging into the contract ABI spec to get some context (not super familiar with ABIs):
https://solidity.readthedocs.io/en/latest/abi-spec.html

Let me know if there's any other docs that you think would help give me more context!

[stefanmendoza]

@carver following up on this - was on vacation, going to start looking into this again this week. 👍

[fabioberger]

Any update on this?

[stefanmendoza]

@fabioberger still working on this! Was finishing looking over the contract ABI spec last night to get a better understanding how the tuples / structs work. Was gonna post out any questions I have tonight or tomorrow to clarify what exactly we need / fill in any gaps in my understanding and then get to digging into the implementation tomorrow. 👍

[stefanmendoza]

@fabioberger just a heads up, I'm getting pulled into some critical stuff at work so most likely not gonna have a chance to dig into this again until the weekend. If you all needed this right now, feel free to pick it up. If not, I'll just keep chugging along with this. 👍🏽

[stefanmendoza]

@carver / @pipermerriam okay, after messing around with the code in web3._utils.abi and eth_abi.abi with some debugging, I think I see the issue... So the first thing we'd need to do is to expand on the encoder classes in eth_abi, right? So that the tuple parsing can handle the "complex" types like address = ENS name, string = bytes, bytes = hexstr, etc.?

[davesque]

@stefanmendoza Encoder classes in eth_abi should already handle any kind of tuple type. I'm not sure about all the details of this issue, but I'm guessing it's just a matter of making the proper modifications to web3.py's use of eth_abi. Also, I can't remember off the top of my head but it might also take cutting a new release of eth_abi with the latest master and maybe upgrading web3.py's version dependency.

[stefanmendoza]

@davesque

Encoder classes in eth_abi should already handle any kind of tuple type.

I was more referring to Jason's suggestion that we update eth_abi#is_encodable and the relevant encoders so that we can remove is_encodable from web3. The types Jason provided fail:

(venv) [11:38:47 PM] @stefanmendoza ➜  eth-abi git:(master) python
Python 3.7.1 (default, Nov  6 2018, 18:46:03) 
[Clang 10.0.0 (clang-1000.11.45.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from eth_abi import abi
>>> abi.is_encodable('string', b'already-encoded')
False
>>> abi.is_encodable('address', 'myname.eth')
False
>>> abi.is_encodable('bytes', '0x0123')
False

I know @pipermerriam suggested we just have conditionals for them in the is_encodable function, but would it be possible to just add these to the encoders or potentially even eth-utils for the case of the ENS name for an address? The scope of that may be larger than my understanding makes it seem though.

Also, I can't remember off the top of my head but it might also take cutting a new release of eth_abi with the latest master and maybe upgrading web3.py's version dependency.

Yeah, I had ran into a similar issue before... The dependency tree is pretty nasty. If we want to use master (eth-abi 2.x), we'll have to figure out what all needs to be bumped up.

For reference: #1056 (comment)

[carver]

So the idea would roughly be to create your own registry of eth-abi encoders that have custom implementations, and then call eth-abi's is_encodable().

For example, you could subclass AddressEncoder and modify validate_value() to accept ENS names as valid (falling back to the default validation, otherwise). The tuple encoder doesn't have to be modified, it should JustWork:tm: by internally looking up the other encoders in the custom registry.

The best solution probably modifies is_encodable() to accept registry as an optional argument.

[pipermerriam]

The best solution probably modifies is_encodable() to accept registry as an optional argument.

I'd love to clean this up so that we only use public APIs from eth-abi. It's the only real way I see to make it possible to support new types or to inject your own support for different formats for a specific type. We'd need to have a formal API somewhere (probably globally on Web3 and locally on Contract) to allow setting your own ABI registry.

cc @kclowes as I think you're working on this.

[stefanmendoza]

@carver / @pipermerriam cool, thanks guys! I'm thinking of splitting up the first chunk of work like this:

• Add support for custom registries in eth_abi
• Dependency upgrade for web3.py to support pulling in eth_abi 2.x and it's related dependencies
• Add custom encoders to web3.py and add a custom registry (thinking of pulling in the default registry from eth_abi and deregistering the ones we're overriding then registering the custom ones?)
• Remove custom encoding methods in web3.py and enable usage of the custom registry to support basic tuple types

Sound like a reasonable approach? We'll probably need to discuss more about what you meant in regards to the formal API in Web3, @pipermerriam.

Also @pipermerriam - I looked and @kclowes doesn't seem to have cloned eth_abi or web3.py and pushed them up, so I'm gonna just start digging into these, but @kclowes please let me know if you already have some work done and we can tackle this together if you want!

[kclowes]

👋 Hi Stefan! I have just been poking around with this over the last couple days and trying to get my head wrapped around the problem. I don't have any "work" done to speak of. I'm happy to pick it up though if it looks like you won't have time over the next few days, just let me know!

[carver]

I'd love to clean this up so that we only use public APIs from eth-abi. It's the only real way I see to make it possible to support new types or to inject your own support for different formats for a specific type. We'd need to have a formal API somewhere (probably globally on Web3 and locally on Contract) to allow setting your own ABI registry.

Just to clarify, there are two public APIs under discussion here: eth-abi's and web3's.

For web3, allowing you override the default eth-abi registry seems pretty reasonable, but this PR won't need to depend on it.

For eth-abi, this PR may depend on a new eth-abi API. Maybe a better API than is_encodable(type, val, registry=...) would be registry.is_encodable(typ, val). It seems pretty reasonable to ask the registry itself (aka, the set of all possible types) whether a type is encodable. I think I'd be happy with either of those, though. Someone went out of their way to make is_encodable public, by making from eth_abi import is_encodable a thing.

thinking of pulling in the default registry from eth_abi and deregistering the ones we're overriding then registering the custom ones?

Seems like a reasonable first pass 👍

[stefanmendoza]

For eth-abi, this PR may depend on a new eth-abi API. Maybe a better API than is_encodable(type, val, registry=...) would be registry.is_encodable(typ, val). It seems pretty reasonable to ask the registry itself (aka, the set of all possible types) whether a type is encodable. I think I'd be happy with either of those, though. Someone went out of their way to make is_encodable public, by making from eth_abi import is_encodable a thing.

@carver - So one of the reasons why I was thinking of just making the passive change to the eth-abi.abi module instead of extending on registry was because I was under the assumption that we'd want to add this support across all encoding / decoding methods in the module (see here). It doesn't make sense to me to have registry.encode_abi(...), registry.decode_abi(...), etc. I think it makes more sense to pull from the registry to get the proper encoder / decoder and call the relevant methods on those (through the abi module) as having those methods on the registry seems like it's overloading the purpose of the registry. Thoughts? 🤷‍♂️

EDIT: On the flip side though, I do think it could get unwieldy to have to pass a registry around to all those methods... 🤔 May need to give it more thought. Potentially have another class that takes a registry and then that has the methods? Maybe an ABIEncoder and ABIDecoder, with the methods from eth_abi.abi on the applicable classes?

class ABIEncoder():
    def __init__(self, registry):
        self._registry = registry

   def encode():
        ...

   def encode_single():
       ...

class ABIDecoder():
    def __init__(self, registry):
        self._registry = registry

   def decode():
        ...

   def decode_single():
       ...

registry = ABIRegistry()
encoder = ABIEncoder(registry)
encoder.encode_single('string', 'abc')

[carver]

I think it makes more sense to pull from the registry to get the proper encoder / decoder and call the relevant methods on those (through the abi module) as having those methods on the registry seems like it's overloading the purpose of the registry. Thoughts?

Yeah, it's a reasonable argument, and probably why is_encoding was written in the first place.

Ok, to iterate on the ABIEncoder idea. We could tie together the encoding/decoding methods:

class ABICodec:
  def __init__(self, registry):
    # If you want a different registry, create a new ABICodec instance
    # part of me wants to be able to "freeze" the registry at this point, but that's a whole other convo
    self._registry = registry
  def is_encodable():
  def encode_abi():
  def encode_single():
  def decode_abi():
  def decode_single():

The w3 instance would keep a w3._abi_codec instance to be used in any ABI encoding or decoding. This also lays the groundwork for a straightforward way to have people customize the eth-abi.

I do think it could get unwieldy to have to pass a registry around to all those methods...

Someone, somewhere has to keep a reference to the custom registry. So there may be no way around referencing that codec instance a lot.

[stefanmendoza]

@carver Codec was the word I was looking for lol I was thinking of something like a SerDe but couldn't remember the equivalent for an encoder / decoder. 😅 I think this approach makes sense. 👍

Someone, somewhere has to keep a reference to the custom registry. So there may be no way around referencing that codec instance a lot.

Agreed with this, but instead of passing a registry instance to every method call, I think it's a little nicer to instantiate a Codec and pass that around just and call methods on that instead. That's more of what I was thinking.

[davesque]

Am I correct that basically the main issue here is that there's no way to say you have a custom registry and that everyone should use it? If so, then the codec class makes sense but only really as a wrapper around an updatable reference to the current active registry. As such, I'd expect to see something like this on ABICodec:

class ABICodec:
    ...

    def set_registry(self, registry):
        self._registry = registry

    ...

Also, were we thinking that this ABICodec would live in eth-abi or web3.py?

[carver]

Am I correct that basically the main issue here is that there's no way to say you have a custom registry and that everyone should use it?

Yup, sounds about right.

If so, then the codec class makes sense but only really as a wrapper around an updatable reference to the current active registry.

Yes and no. Because the client has to keep this codec instance around anyway, it's the same to change the registry in these two ways:

# the mutable registry way
w3._abi_codec.set_registry(new_registry)

# the immutable registry way
w3._abi_codec = ABICodec(new_registry)

I tend to like the immutable approach, but in this particular case I wouldn't cry if we did the mutable one.

Also, were we thinking that this ABICodec would live in eth-abi or web3.py?

I think eth-abi. It seems like something that other eth-abi users could reasonably want.

[davesque]

Here's a related question that seems important to me: is it eth-abi that tracks the current registry or codec, and transparently updates the functionality of the methods it exposes as part of its API, or is tracking the current registry or codec left to consumers of eth-abi? I think you're suggesting it should be eth-abi consumers that do this, correct? Namely, the current Web3 instance provided by web3.py would do it. I don't really feel strongly either way. Just trying to get a better idea of everyone's thinking.

[carver]

Here's a related question that seems important to me: is it eth-abi that tracks the current registry or codec, and transparently updates the functionality of the methods it exposes as part of its API, or is tracking the current registry or codec left to consumers of eth-abi? I think you're suggesting it should be eth-abi consumers that do this, correct?

Right, the consumer would track the codec, and the codec would track the registry.

Namely, the current Web3 instance provided by web3.py would do it. I don't really feel strongly either way. Just trying to get a better idea of everyone's thinking.

Yup, that's what I'm suggesting.

[feuGeneA]

Hey guys, I wanted to let you know that I'm working on a partial solution to this issue.

I've got a very specific use case that I need to get working. It's a call to an existing contract method, which both accepts and returns a struct, which of course is encoded as a tuple in the ABI.

Thankfully, since my requirements don't demand support for ENS names or any other fancy string handling in the struct components, I'm taking @pipermerriam 's comment as license to make just the changes I need in order to get my use case working. And, since I personally don't have any use for the fancy ENS/string handling, I'll leave the eth_abi registry stuff to you all. 😄

@pipermerriam , FYI, it's looking like I'll be covering all of your "phases" in order to support my use case.

I've already got changes working to support is_encodable for my struct, but I've still got plenty more to drill through, and then some polish/cleanup (I've already got some redundant logic...), and then of course I'll need to get some good tests in place. I should be have a PR up next week.

[stefanmendoza]

@feuGeneA cool! So are you essentially just going to house all the logic for tuples here in web3.py?

[feuGeneA]

@stefanmendoza that's what I'm doing right now. But to be honest I'm not sure whether it belongs in web3.py or eth_abi, because it does seem like maybe it's more suited to eth_abi? Would appreciate feedback on that.

Here's the gist of what I'm doing:

The problem is that, while the current code just blindly copies the type names from the ABI and passes them to eth_abi, the type name 'tuple' isn't supported by eth_abi, which raises an exception ValueError: No matching entries for 'tuple' in encoder registry. Instead, eth_abi expects you to describe the tuple; that is, indicate the type not just as 'tuple' but rather as a parenthesized list of the types of the tuple components. So if you had a function that accepts a Solidity struct { uint256 a; uint256 b; } as one of its arguments, then you would need to use eth_abi like encode_abi('(uint256, uint256)', [0, 1])

So my solution is simple: In web3.py, before calling eth_abi methods, look in the ABI for arguments of type tuple, and if found walk through them to compose type and argument lists as eth_abi expects them.

Again, this logic does seem (to my naive mind) like it could be useful to other users of eth_abi (not just web3.py), so maybe it belongs there instead? But eth_abi did already add support for tuples, and this is how they chose to build the API, so that's what I'm working with.

[pipermerriam]

@feuGeneA it'd be good if you could post a pull request with the code you've already got for this as that is often more illustrative than any description can be. It doesn't have to be something that's ready to merge or even something that we plan on merging.

[feuGeneA]

@pipermerriam Here's the current status of my work in progress: 77d2ed1

I hope you don't cringe too much 😄 I'm definitely just hacking my way through the errors, trying to get my data to flow through. Hopefully the experience will teach us what the solution should look like.

[pipermerriam]

@feuGeneA Thanks for opening the PR. I'm fine with your approach. Sometimes just throwing code at things till it works and then refining is a totally acceptable way to figure stuff out.

[carver]

Note that @stefanmendoza is making progress on the approach for custom ABI registries. I suspect we only need one of those approaches to eventually be merged. Both are exploratory enough that it probably makes sense to continue on both.

[stefanmendoza]

@carver @davesque thanks for the help, guys! Much appreciated. So based on my comment here, do you all agree that the next step would be trying to fix any dependency conflicts with web3.py so we can pull in eth-abi 2.x? Would we want it on master or a separate branch? Or do we want to finish up adding support to eth-abi (dicts, etc.) and have it "feature complete" and then bring 2.x out of beta and uplift web3? I'm leaning more in favor of the latter.

cc @pipermerriam

[feuGeneA]

Okay, my PR #1147 is "done" and ready for review. I've consolidated all the duplicate logic, added quite a few tests, verified that it works for my contract method which accepts and returns a tuple, and generally polished it to my own satisfaction. Please let me know what you think.

[carver]

do you all agree that the next step would be trying to fix any dependency conflicts with web3.py so we can pull in eth-abi 2.x?

Yeah, that sounds good.

Would we want it on master or a separate branch?

Go ahead and do master, which is already forked from v4 (it is the web3 v5 home, now). The good news is that this means we don't need to simultaneously support eth-abi v1 and v2.

Or do we want to finish up adding support to eth-abi (dicts, etc.) and have it "feature complete" and then bring 2.x out of beta and uplift web3?

Hm, that would be great, but... it seems like several people are itching for tuple support now, and since web3 is already on a v5 pre-release I think it's okay to build on the eth-abi pre-release for now. Definitely add issues to eth-abi for things that you think need to happen before v2 goes stable, though!

[carver]

Okay, my PR #1147 is "done" and ready for review.

@feuGeneA can I ask you and @stefanmendoza to work together on a unified solution? (specifically by commenting on this issue, so that we can all be a part of the conversation)

The approach of upgrading to eth-abi v2 with tuple support continues to look promising to me. It would be great if you two could figure out how #1147 might fit into that direction (maybe it's mostly the same, or maybe it is dramatically different, it's not obvious to me yet).

[stefanmendoza]

@carver sure!

@feuGeneA I'll look over your PR to get an idea of what your approach is. 👍

[feuGeneA]

I must admit that I don't yet fully comprehend how and where the utilization of a custom eth_abi registry will be incorporated into web3.py, but I feel like the scope of changes in #1147 is small enough that it's unlikely to have much impact on whatever approach would have been taken anyways.

Despite the +488 -10 change stats on #1147, the changes are actually quite simple. Outside of tests and wholly new helper functions, there are really only about 45 lines of changed/added code, confined to these specific areas:

• in _utils/abi.py
  • in get_abi_input_types(), 1 line changed
  • in get_abi_output_types(), 1 line changed
  • in is_encodable(), 19 lines added
  • in check_if_arguments_can_be_encoded(), 1 line changed
  • in data_tree_map(), 7 new lines replaced 1 old line
  • in abi_sub_tree(), 7 lines added
• in _utils/contracts.py
  • in get_function_info(), 1 line added

And all of those new helper functions, listed below, are completely independent, so they could be easily transplanted to wherever it is that they should live (eth_abi? eth_utils?).

• get_tuple_component_types(), used by _utils.abi.is_encodable().
• get_abi_inputs(), used by _utils.abi.check_if_arguments_can_be_encoded() and _utils.contracts.get_function_info().
• collapse_if_tuple(), used by _utils.abi.get_abi_input_types() and _utils.abi.get_abi_output_types().

And, actually, we can shave collapse_if_tuple() off of this list, and use the identical one that was just merged into eth_utils (though it hasn't yet been released). [Edit: collapse_if_tuple() has been released in eth-utils in v1.4.0.]

Finally, to help us all reason about these changes, I drew up a crude call graph, just below, to show what uses the functions that have changed. Each bullet is directly used by its direct sub-bullets.

• _utils.abi.check_if_arguments_can_be_encoded()
  • _utils.abi.filter_by_encodability()
    • find_matching_fn_abi()
      • contract.Contract._find_matching_event_abi()
      • contract.ImplicitMethod.__call_by_default()
      • contract.ContractFunction._set_function_info()
      • contract.call_contract_function()
  • _utils.contracts.encode_abi()
    • _utils.contracts.encode_transaction_data()
    • contract.Contract.encodeABI()
    • contract.Contract._encode_constructor_data()
    • contract.ContractConstructor._encode_data_in_transaction()
    • contract.ContractFunction._encode_transaction_data()
  • contract.Contract.find_functions_by_args()
    • contract.Contract.get_function_by_args()
• _utils.abi.get_abi_input_types()
  • _utils.contracts.encode_abi() [REPEATED FROM ABOVE]
• _utils.abi.get_abi_output_types()
  • contract.call_contract_function() [REPEATED FROM ABOVE]
• _utils.abi.is_encodable()
  • _utils.filters.match_fn()
  • _utils.abi.check_if_arguments_can_be_encoded() [REPEATED FROM ABOVE]
• _utils.data_tree_map()
  • _utils.map_abi_data()
    • main.Web3.soliditySha3()
    • _utils.rpc_abi.apply_abi_formatters_to_dict()
    • _utils.rpc_abi.abi_request_formatters()
    • _utils.contracts.encode_abi() [REPEATED FROM ABOVE]
    • _utils.events.get_event_data()
    • contract.Contract.decode_function_input()
• _utils.abi.abi_sub_tree()
  • _utils.abi.abi_data_tree()
    • _utils.abi.map_abi_data() [REPEATED FROM ABOVE]

Lots of content here... I hope it's helpful! Please do let me know how you all feel about my assertion that the changes in #1147 aren't significant enough to alter the approach of utilizing a custom eth_abi registry.

Edit: Added some more changes, to support arrays of tuples, and updated links above appropriately, and upped the count of changed/added non-test/non-helper lines to 45. 😄

[stefanmendoza]

Despite the +546 -10 change stats on #1147, the changes are actually quite simple. Outside of tests and wholly new helper functions, there are really only about 35 lines of changed/added code, confined to these specific areas:

Sweet! I'll check this out tomorrow during the day if I get time; if not, in the evening. Then we can sync up on how we may use the custom registries from eth_abi.

[stefanmendoza]

@feuGeneA / @carver / @davesque

Will be digging back into this over the next two weeks while I'm on holiday!

[stefanmendoza]

Looks like the following dependency warnings are showing up on master of web3.py. Saw them in the pip install, so I ran pipdeptree (you can download this by using pip install pipdeptree).

Warning!!! Possibly conflicting dependencies found:
* py-evm==0.2.0a33
 - eth-typing [required: >=1.1.0,<2.0.0, installed: 2.0.0]
* ethtoken==0.0.1a4
 - web3 [required: >=4.0.0,<5.0.0, installed: 5.0.0a2]
* eth-tester==0.1.0b33
 - eth-abi [required: >=1.0.0-beta.1,<2, installed: 2.0.0b4]

@carver some PRs to fix this:

• Bump web3 to 5.x carver/ethtoken.py#4
• Bump eth-tester to 0.1.0b35, pytest to >= 3.6.0 #1175

Will need a release of eth-tester and ethtoken.py. I'm going to work on integrating my eth-abi and
@feuGeneA's web3.py changes next. We may need to make another PR to eth-abi if anything from there should be pulled out, but I'm thinking of just getting the custom registries integrated and working first and then figuring out how to merge Gene's code with that and / or pull some of it back out into eth-abi and consume it in web3.

[davesque]

@stefanmendoza Just FYI, I've also been looking at getting this PR ready to merge. I think that might involve adding a "cloning" feature to the registry in eth-abi. Then, we can just clone the existing registry and swap out a couple of coder classes to get the value-munging behavior we want. Also, I might have some fixes and refactors here and there to add to this PR.

[Amxx]

Any news ?

[carver]

I seem to recall reviewing a PR by @davesque that added tuple support to v5. So it's probably good to close, thoughts @davesque?

[Amxx]

Just tried, it's not supported by v5.0.0a4

[davesque]

@Amxx Just made a new PR ( #1235 ) which rebuilds PR #1147 . Currently a work in progress but almost done. The bulk of the work, which I believe was to harden the get_abi_inputs function, is done.

[carver]

Basic tuple API support was added in #1235, so closing...

There remains a bug, detailed in #1629, that seems to affect events. That will be the tracking place for any event-related issues.