ethereum / eth-abi

Ethereum ABI utilities for python
MIT License
242 stars 269 forks source link

Convert library to use a registry pattern for encoders and decoders. #36

Closed pipermerriam closed 6 years ago

pipermerriam commented 6 years ago

What was wrong?

The way eth-abi is structured, it cannot be extended without changes to the core library. This is problematic because new ABI types are likely to be created (as seen here).

How can it be fixed?

We'll create a new ABIRegistry class with the following functionality.

registry = ABIRegistry()

# register an encoder and a decoder for a static type.
registry.register("string", string_encoder, string_decoder)

# register only an encoder or decoder
registry.register_encoder("string", string_encoder)
registry.register_decoder("string", string_decoder)

# register a dynamic encoder/decoder
registry.register(lambda base, sub, arrlist: bool(arrlist), array_encoder, array_decoder)

# register an exact static type.
registry.register(("uint", "256", "[]", uint_encoder, uint_decoder)

# the main encode/decode API
encode_abi = registry.encode_abi
decode_abi = registry.decode_abi
encode_single = registry.encode_single
decode_single = registry.decode_single

Then, within the codebase, we make a default registry to expose all of the built-in encoders and decoders.

Implementation details.

validation

The register method should raise an exception if the same static type is registered twice.

determining which encoder/decoder to use

Internally, the ABIRegistry will need to implement a dynamic version of the eth_abi.encoding.get_single_encoder and eth_abi.decoding.get_single_decoder. The logic for these functions should be as follows.

  1. check for an exact match against static abi types.
  2. evaluate all callbacks. If exactly one (1) returns true, use that type. Otherwise raise an appropriate exception.
pipermerriam commented 6 years ago

My suggestion for the callbacks to error if more than one returns True needs to be evaluated. My inclination was to go with a strict rule set with no ambiguity, otherwise, we'd need to do something like giving priority based on the order that things were registered and that feels wrong to me.

davesque commented 6 years ago

@pipermerriam I suppose we'll need to include some registration callbacks by default to handle types which must instantiate an encoder/decoder with parameters from the "sub" and "array" parts. Given this, and assuming multiple matching callbacks would cause an error, how would we handle the case in which someone wants to override a default callback?

pipermerriam commented 6 years ago

@davesque probably either:

davesque commented 6 years ago

@pipermerriam @carver

I talked briefly about the need to settle on some API by which encoder/decoder classes can accept a type string that was matched to them by the registry (and, for tuple types, probably also a reference to the registry itself). Here's one possible way I can imagine doing that:

# base.py:

class FromTypeStr:
    """
    A mixin for any class which should be able to create an appropriate
    instance of itself for the given type string and type registry.
    """
    @classmethod
    def from_type_str(cls, type_str, registry):
        raise NotImplementedError('Must implement `from_type_str`')

# registry.py:

class ABIRegistry:
    ...
    def _get_encoder(self, type_str: str):
        encoder = self._encoders.find(type_str)

        if issubclass(encoder, base.FromTypeStr):
            return encoder.from_type_str(type_str, self)

        return encoder

# encoding.py:

class UnsignedIntegerEncoder(FromTypeStr, NumberEncoder):
    ...
    @classmethod
    def from_type_str(cls, type_str, registry):
        result = parser.parse(type_str)

        if not something_this_parser_can_handle(result):
            raise ValueError('...')

        sub = ...

        return cls.as_encoder(value_bit_size=int(sub))

Above, we have a mixin class which can be used to determine if a value stored in the registry is an encoder/decoder class that requires some extra steps to be made into a callable which will work for a type string.

Thoughts?

carver commented 6 years ago

Yup, this looks about how I imagined it. :+1:

davesque commented 6 years ago

@carver Do we ever bother using the abc module? In this case, I'm not sure it gets us much since we're never really going to try instantiating an encoder or decoder class until we call from_type_str. So we'd get a NotImplementedError before the abc machinery caused a TypeError.

davesque commented 6 years ago

Actually, never mind. There's effectively no reason as I see it to use an abstract base class in this case.

pipermerriam commented 6 years ago

@davesque should we close this? maybe close it and open up specific issues for anything remaining if there are remaining tasks.

davesque commented 6 years ago

Yep, I guess I forgot there was an issue ticket for it.