Add support for providing custom registries

stefanmendoza commented 5 years ago

What was wrong?

https://github.com/ethereum/web3.py/issues/829

Specifically this comment and this comment. This in the first step towards supporting tuple types in web3.py - allow for providing customer registries to allow for customer encoders and decoders.

How was it fixed?

Added support for providing custom registries to the eth_abi.abi methods. The methods assume the eth_abi.registry.registry is the default.

Cute Animal Picture

Cute animal picture

davesque commented 5 years ago

Hey, Stefan. Thanks for your work on this. Some of our team members had a previous discussion about fixing this issue. In that discussion, we actually decided that the best path forward would be to convert encode_single, encode_abi, etc. into methods on the eth_abi.registry.ABIRegistry class. That way, there's no need to pass around the active registry as an argument and it would always be available as self in the context of registry operations.

stefanmendoza commented 5 years ago

@davesque okay, sounds good! Since that's a non-passive change, is the expectation that we would mark the eth_abi.abi methods as deprecated or just remove them since we're still on the beta and add a deprecation warning in the 1.x branch?

davesque commented 5 years ago

The methods could still exist in the same location. It's just that, instead of being stand-alone functions, they would be references to bound methods on a class instance. For example, you could end up doing something like this:

# eth_abi/abi.py

from .registry import registry

encode_single = registry.encode_single
encode_abi = registry.encode_abi
...

However, I'm realizing now that this doesn't really solve the issue of how web3.py or any other external consumer of eth_abi knows what registry to use. I'll have to spend some more time thinking about that one.

stefanmendoza commented 5 years ago

@davesque one thing @carver and I were discussing yesterday was potentially having a Codec class and passing that around as naming wise it makes more sense, you're not overloading the registry's purpose, and you're passing around the instance you're actually calling methods on (vs. this initial design I had in here where we were passing the registry to every method).

See my comment here and his comment below: https://github.com/ethereum/web3.py/issues/829#issuecomment-438784925

carver commented 5 years ago

Why did you decide to implement just the encoder instead of the full ABICodec concept from the other thread? It requires clients to track both an encoder and decoder pair AFAICT. So without more info, I prefer the paired approach as ABICodec.

stefanmendoza commented 5 years ago

@carver

Why did you decide to implement just the encoder instead of the full ABICodec concept from the other thread? It requires clients to track both an encoder and decoder pair AFAICT. So without more info, I prefer the paired approach as ABICodec.

Ugh, that's my bad. Typo. I moved the decode_single and decode_abi methods to that new class, just named it wrong. Need to rename to ABICodec and update the references accordingly.

https://github.com/ethereum/eth-abi/blob/90aba57138c40cc2672eb547b265be0d299a4095/eth_abi/abi.py#L99

https://github.com/ethereum/eth-abi/blob/90aba57138c40cc2672eb547b265be0d299a4095/eth_abi/abi.py#L119

stefanmendoza commented 5 years ago

@davesque sounds good! Will look into that this week.

@carver thanks for reviewing this. I'll look over your comments this week as well. 👍

stefanmendoza commented 5 years ago

Just an update, I think all that's left is adding tests for the Packed ABICodec's is_encodable method and updating the documentation to discuss using custom registries.

stefanmendoza commented 5 years ago

@carver / @davesque - I think this is ready for one last implementation / tests review before I make the final changes (adding the documentation for how to use the codecs with custom registries).

I responded to some of the remaining comments with fixes or asking for clarification. 👍

davesque commented 5 years ago

Yeah, I've got a few commits I'd like to add to this to tie a bow on things. Will hopefully have that all wrapped up before EOD.

davesque commented 5 years ago

@stefanmendoza @carver Alright, I made the PR to your fork of eth-abi. Let me know if you have any questions or concerns.

To briefly summarize what I did:

Made a new article in the docs describing how to use the ABICodec and related classes.
Changed the inheritance scheme and structure of classes in the codec module. Now, there is a BaseABICoder class which provides the constructor for two new classes: ABIEncoder and ABIDecoder. ABICodec is just a pass statement class which inherits from both of those new classes. This eliminates the need for a separate ABICodecPacked class and enclosing module.

davesque commented 5 years ago

Here's a link to that PR: https://github.com/stefanmendoza/eth-abi/pull/1

stefanmendoza commented 5 years ago

@davesque merged the PR! Thanks for the help!

carver commented 5 years ago

Cool, I'm really happy with this direction! When you write out the docs for a new API, and the examples look straightforward and concise, it's a great confirmation that some good choices were made.

davesque commented 5 years ago

I'm going to go ahead and merge this. Thanks for all your work, @stefanmendoza !

CC: @carver

ethereum / eth-abi