Add support for providing custom registries #109

stefanmendoza · 2018-11-14T04:27:14Z

What was wrong?

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

davesque · 2018-11-14T22:22:23Z

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 · 2018-11-14T23:48:30Z

@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 · 2018-11-15T05:02:47Z

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 · 2018-11-15T15:21:55Z

@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:
ethereum/web3.py#829 (comment)

carver · 2018-11-19T18:17:09Z

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 · 2018-11-19T18:24:53Z

@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.

eth-abi/eth_abi/abi.py

Line 99 in 90aba57

def decode_single(self, typ: TypeStr, data: Decodable) -> Any:

eth-abi/eth_abi/abi.py

Line 119 in 90aba57

    
           def decode_abi(self, types: Iterable[TypeStr], data: Decodable) -> Tuple[Any, ...]:

carver

Feel free to @ mention me again when ready for next review.

carver · 2018-11-19T18:47:42Z

eth_abi/__init__.py

+#     encode_single,
+#     encode_abi,
+#     is_encodable,
+# )


~~I think it's still okay to expose these as convenience methods for at-console quicky encoding/decoding with the default registry.~~

Nevermind, but it should import codec and the class (there are several docs to update, too). Something like:

from eth_abi.abi import ( ABICodec, default_codec as codec, )

This shortens to codec for super short clean examples in the docs, but default_codec is a more complete/accurate name for internal usage, like the tests.

Ended up defining the default_codec in the abi module as well as exposing the convenience methods per @davesque's comment here:
#109 (review)

Changes:
2f68d45#diff-8571b2a70a70cacaa83996b7cdcc5d4eR5

carver · 2018-11-19T18:57:48Z

eth_abi/abi.py

-        encoding e.g. ``'uint256'``, ``'bytes[]'``, ``'(int,int)'``, etc.
-    :param arg: The python value to be encoded.
+    def __init__(self, registry: ABIRegistry = default_registry):
+        self._registry = registry


So here are some examples how a mutable registry creates problems:

If someone modifies the default registry, it modifies how all copies of ABICodec() work, even after they have been created, which is surprising.

If someone does something a little naughty like codec = ABICodec(); codec._registry.register_encoder(...) then it will modify how the default registry works. A big surprise!

Since there's no way currently to lock down the registry from change and/or make a copy of a registry, here's an idea for resolving the problem:

change the registry import instance to a method: make_default_registry()

change the default value of the registry keyword arg to None

change this line to:

if registry is None: self._registry = make_default_registry() else: self._registry = registry

@carver wasn't exactly sure if you meant to make default_registry in eth_abi.registry into a method? This is how I ended up setting them up

BaseABICodecEncoder - abceb18#diff-840cea94757e5fecdafdc6c8bb6ba8e2R32

ABICodec - abceb18#diff-840cea94757e5fecdafdc6c8bb6ba8e2R100

ABICodecPacked - abceb18#diff-df4a437f066f81aabfd6125511b5e745R18

eth_abi/abi.py

davesque

There are a few things I'd like to see before merging this:

It would be nice to have ABICodec defined in a separate codec submodule. The issue is that there are two different submodules, abi and packed, that both include encoding/decoding functions which use the registry. It makes more sense to me to just import ABICodec into those two submodules and instantiate default instances for normal and packed coding.
The abi and packed submodules could expose a default_codec and default_packed_codec respectively. I think they should also expose references to the bound methods on those codec objects for backwards compatibility.
The tests might need to be restructured a bit in light of the encapsulation of coding function in an ABICodec class. I tried to structure the tests in correspondence with the particular modules that are included in the eth-abi package and also in correspondence with the particular resources offered by those modules. For example, there is a test_abi testing module which corresponds to the abi module and, within that, there are test_encode_single, test_encode_abi, etc. modules which correspond to the particular resources provided by the abi module. Those are unit testing modules which is the reason that structure was chosen. Now, I think we're going to need a test_codec module and, under that, a test_abi_codec module. You can probably just grab all of the tests from test_encode_single, test_encode_abi, test_decode_single, etc. and all the other testing modules which were previously unit testing the functions which were moved into the ABICodec class.

stefanmendoza · 2018-11-19T21:01:22Z

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

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

This reverts commit 90aba57.

eth_abi/codec.py

carver · 2018-11-20T04:32:02Z

eth_abi/codec_packed.py

+)
+
+
+class ABICodecPacked(ABICodec):


Looks like the inheritance actually works the other way, with some methods only implemented on the non-packed codec.

Yeah, it's weird. I feel like the assumption is that any subclass of the ABIEncoder would be expected to implement all five methods, but the packed implementation is experimental and isn't decodable. Not sure if it makes sense to have a completely different class for that one. Was trying to keep in line with the ABICodec. Thoughts? I could see both ways.

Another alternative is to make a third base codec class that implements the encoding methods. The packed codec would be like:

class ABICodecPacked(BaseABICodecEncoding): pass

The full codec would also subclass the Base codec and implement the decoding.

@carver I considered that as well, but would we want to make a BaseABICoderDecoding as well?

I think it's fine to just implement that in ABICodec for now.

Updated here: abceb18

Added null check for the registry provided to the base encoder: f939865

and a test for the null case ^ - d63273c

Ended up naming it BaseABICodecEncoder. Let me know if you think that doesn't make sense since we have separate *Encoder classes. If so, I can make it BaseABICodecEncoding like you suggested. Just thought the wording of that was kind of odd. 🤔

Yup, seems fine to me. Could also be BaseABIEncoder since it's only responsible for the encoding side of the codec. Whatever you like, since it's not intended to be part of the public API. :)

carver · 2018-11-20T04:32:36Z

eth_abi/codec_packed.py

+            ABICodec.__init__(self, registry)
+
+    def is_encodable(self, typ: TypeStr, arg: Any) -> bool:
+        raise NotImplementedError("'is_encodable' is not  supported in packed mode")


I haven't looked too deeply at the packed stuff. Why can't we test if a value is encodable?

Was just keeping with the methods that were previously implemented for packed encoding. I can dig into this some more and see. 🤔

@carver I think the lack of an is_encodable_packed function may have just been an oversight or for the sake of expedience. I just looked through the code and all of the Packed* encoder classes seem to inherit from base classes which implement validate_value. So they should all have what is needed to run an encodability check. Otherwise, I can't think of another reason that it wouldn't be possible. Now that we have the ABICodec class, we should get this "for free." We just need delete the NotImplementedError override and add something like is_encodable_packed = default_codec_packed.is_encodable.

Why rename the method with the _packed suffix? We don't rename any of the other methods like decode_abi_packed.

I'd say just implement it once in the base codec class and get it for free with the same name.

I think he's suggesting that because we're trying to stay passive by exporting the methods in eth_abi/packed. Like:

encode_abi_packed = default_codec_packed.encode_abi encode_single_packed = default_codec_packed.encode_single

Since those were previously exported. But this method didn't exist before.

Ah, thanks, I understand. That line was for the global module export 👍 sounds good

@carver What @stefanmendoza said. If we had had an is_encodable function for packed mode, it would have been named is_encodable_packed. Incidentally, I'm now realizing that this is possibly the reason I didn't add one: such a function, that calls out to all relevant validate_value methods, would not have actually ended up calling any different method than the regular is_encodable function. The criteria for encodability do not differ between normal and packed mode.

Added the method here:
abceb18#diff-f7c24c7b7a7b6405352611f3ee624887

stefanmendoza · 2018-11-24T17:37:33Z

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 · 2018-11-24T18:23:44Z

docs/releases.rst

@@ -32,7 +32,7 @@ v2.0.0-beta.1
    the parsing API more consistent with the new parsimonious parser.


There were warnings showing up when running make build-docs about the title underline being too short, so I fixed this up to clean up the warnings.

stefanmendoza · 2018-11-24T18:26:54Z

eth_abi/codec.py

    def __init__(self, registry: ABIRegistry=None):
+        """


@davesque / @carver - I noticed that the __init__ methods aren't showing up in the docs; also, I haven't really seen constructors be documented in the project. I think it could be valuable to have this documented as the API isn't self-documenting in regards to what registry is being used or even that there is a default registry being used. Not sure if this is something we'd move up to the class doc or just not have it at all?

It looks like we can get the __init__ docs added to the class docs by adding autoclass_content = 'both' to the docs conf. Can you experiment with it to see if that works as expected, looks okay, etc? https://stackoverflow.com/a/9772922/8412986

If so, we will probably add it to our project template so that it eventually gets filters to all our other projects.

stefanmendoza · 2018-11-24T18:27:49Z

docs/eth_abi.rst

+
+.. automodule:: eth_abi.codec
+    :members:
+    :undoc-members:


@davesque / @carver - Do we care to have this turned on or just display documented members?

Yeah, I think it's good to have it on. If we don't want something public, it should be _-prefixed.

stefanmendoza · 2018-11-24T18:29:49Z

@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. 👍

carver

Looks good to go, after the docs. (and @davesque approves, of course) 👍

davesque · 2018-11-27T20:46:27Z

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 · 2018-11-27T21:56:08Z

@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 · 2018-11-27T22:00:30Z

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

* Move some testing modules * Add "Codecs" article to docs Also, remove API docs for `codec_packed` module since it will be deleted. * Use common values for encodability tests * Import lint * Type annotations and docstring formatting * Remove explicit checking for `None` registries * Rearrange inheritance scheme in `codec` module This will save some code and hopefully add some utility to the classes provided by that module. * Add missing __init__.py file * Typo * Add note in releases article about codecs

stefanmendoza · 2018-11-30T05:26:13Z

@davesque merged the PR! Thanks for the help!

carver · 2018-11-30T15:36:25Z

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 · 2018-11-30T18:28:01Z

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

CC: @carver

Support custom registries with ABIEncoder

90aba57

carver reviewed Nov 19, 2018

View reviewed changes

davesque self-requested a review November 19, 2018 19:11

davesque suggested changes Nov 19, 2018

View reviewed changes

stefanmendoza added 3 commits November 19, 2018 20:55

Revert "Support custom registries with ABIEncoder"

1e4a102

This reverts commit 90aba57.

Create codec module and export methods in eth_abi.abi

2f68d45

Created packed codec in eth_abi/codec_packed

8ff02e5

davesque reviewed Nov 20, 2018

View reviewed changes

eth_abi/codec.py Show resolved Hide resolved

davesque reviewed Nov 20, 2018

View reviewed changes

eth_abi/codec.py Outdated Show resolved Hide resolved

stefanmendoza added 2 commits November 19, 2018 22:22

Addressing comments

9614f44

address nit about method defaults

ecece6d

carver reviewed Nov 20, 2018

View reviewed changes

Added support for is_encodable in packed codec

abceb18

stefanmendoza added 4 commits November 24, 2018 11:40

Added null check on registry in base encoder

f939865

Added test for null registry to basecodec

d63273c

Added tests for is_encodable with packed registry

588a373

started adding documentation, fixed sphinx warnings

49b78b1

stefanmendoza commented Nov 24, 2018

View reviewed changes

carver approved these changes Nov 27, 2018

View reviewed changes

carver mentioned this pull request Nov 29, 2018

Add support for tuples in ABI function signatures ethereum/eth-utils#141

Merged

stefanmendoza changed the title ~~[WIP] Add support for providing custom registries~~ Add support for providing custom registries Nov 30, 2018

davesque approved these changes Nov 30, 2018

View reviewed changes

davesque merged commit e6fbe02 into ethereum:master Nov 30, 2018

stefanmendoza deleted the web3_issue-829 branch November 30, 2018 18:28

		@@ -32,7 +32,7 @@ v2.0.0-beta.1
		the parsing API more consistent with the new parsimonious parser.

Add support for providing custom registries #109

Add support for providing custom registries #109

Conversation

stefanmendoza commented Nov 14, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

davesque commented Nov 14, 2018

stefanmendoza commented Nov 14, 2018

davesque commented Nov 15, 2018

stefanmendoza commented Nov 15, 2018 • edited Loading

carver commented Nov 19, 2018

stefanmendoza commented Nov 19, 2018 • edited Loading

carver left a comment

Choose a reason for hiding this comment

carver Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

stefanmendoza Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesque left a comment • edited Loading

Choose a reason for hiding this comment

stefanmendoza commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesque Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanmendoza Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesque Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

stefanmendoza Nov 24, 2018 • edited Loading

Choose a reason for hiding this comment

stefanmendoza commented Nov 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanmendoza commented Nov 24, 2018 • edited Loading

carver left a comment

Choose a reason for hiding this comment

davesque commented Nov 27, 2018

davesque commented Nov 27, 2018 • edited Loading

davesque commented Nov 27, 2018

stefanmendoza commented Nov 30, 2018

carver commented Nov 30, 2018

davesque commented Nov 30, 2018

stefanmendoza commented Nov 14, 2018 •

edited

Loading

stefanmendoza commented Nov 15, 2018 •

edited

Loading

stefanmendoza commented Nov 19, 2018 •

edited

Loading

carver Nov 19, 2018 •

edited

Loading

stefanmendoza Nov 20, 2018 •

edited

Loading

davesque left a comment •

edited

Loading

davesque Nov 20, 2018 •

edited

Loading

stefanmendoza Nov 21, 2018 •

edited

Loading

davesque Nov 22, 2018 •

edited

Loading

stefanmendoza Nov 24, 2018 •

edited

Loading

stefanmendoza commented Nov 24, 2018 •

edited

Loading

davesque commented Nov 27, 2018 •

edited

Loading