Complete Type Hinting for `eth.tools` #1420

Bhargavasomu · 2018-10-21T13:10:10Z

What was wrong?

Issue : #1398

How was it fixed?

Adding the type hints for eth.tools module completely.

Cute Animal Picture

Bhargavasomu · 2018-10-21T13:46:36Z

@cburgdorf I had to change the configurable function in eth.utils.database to Type[T].

For the eth.tools module, most of them were meant to be generic I guess, and hence I had to use Any a lot of times. I tried tightening the types instead of using Any, but it resulted in type error somewhere else. Please suggest me if we could tighten the types anywhere. Will make the changes.
I included some own defined types in eth.typing

pipermerriam · 2018-10-21T23:37:10Z

eth/tools/_utils/normalization.py


 #
 # Primitives
 #
 @functools.lru_cache(maxsize=1024)
-def normalize_int(value):
+def normalize_int(value: Any) -> int:


Will go through these more thoroughly tomorrow but wanted to drop a note that these input types can probably be tightened up to a Union[int, bytes, eth_typing.HexStr, str] (and so on for the other normalize_ functions as we should be able to define the corpus of allowed inputs.

Might even qualify for a type alias (not a NewType) to be put in eth.typing:

IntLike = Union[int, bytes, eth_typing.HexStr, str]

Alternative names: IntConvertible, IntRetrievable

cburgdorf · 2018-10-22T22:47:55Z

@Bhargavasomu I'll give this a proper review tomorrow!

cburgdorf

Solid work! Left a few minor notes inline.

cburgdorf · 2018-10-23T09:04:14Z

eth/tools/_utils/hashing.py

 from eth.rlp.logs import Log


-def hash_log_entries(log_entries):
+def hash_log_entries(log_entries: Iterable[Tuple[bytes, bytes, bytes]]) -> bytes:


As a rule of thumb, keccak will always return a hash of 32 bytes, hence everything that returns keccak(x) should always have the return type Hash32 rather than just arbitrary bytes.

cburgdorf · 2018-10-23T09:12:02Z

eth/tools/_utils/normalization.py


 #
 # Primitives
 #
 @functools.lru_cache(maxsize=1024)
-def normalize_int(value):
+def normalize_int(value: Any) -> int:


Might even qualify for a type alias (not a NewType) to be put in eth.typing:

IntLike = Union[int, bytes, eth_typing.HexStr, str]

Alternative names: IntConvertible, IntRetrievable

cburgdorf · 2018-10-23T09:26:04Z

eth/typing.py

@@ -32,4 +34,8 @@
    List[Tuple[Address, Dict[str, Union[int, bytes, Dict[int, int]]]]]
 ]

+TransactionType = Dict[str, Union[bytes, int, str]]


Did you try locking this down to a TypedDict? Also, in general I prefer not to use the suffix Type for any new types / aliases that we create. It feels redundant to me (we also say Chain, BaseComputation, BaseVM and not ChainType, BaseComputationType or BaseVMType.

As a name suggestion, maybe just TransactionDict?

def normalize_int(value: Union[int, bytes, str, ...]) -> int """ Robust to integer conversion, handling hex values, string representations, and special cases like `0x`. """ if is_integer(value): return value ... ...

@cburgdorf @pipermerriam if the type hinting was changed as above, then the return value would return type Union[int, bytes, str, ...] instead of int which would throw an error. I have tested those and tried tightening the types prior to this PR.

Inside the if is_integer block, it seems reasonable to cast(int, value).

If we need to cast it, then we would need to cast it to a new variable (which I feel is redundant). How do we overwrite the type of a variable, which is already type hinted? This is because, mypy is throwing error when trying to overwrite the type hinting of the variables which are already type hinted.

value = cast(int, value) return value

The following error occurs Incompatible return value type (got "Union[int, bytes, ...]", expected "int")

I think it's fine to: return cast(int, value)

I'm not sure why mypy doesn't like the cast. @cburgdorf is our resident mypy wrangler.

Bhargavasomu · 2018-10-23T15:57:43Z

In general how do I overwrite the Type hinting of a variable? This arises in the following scenario.

def dict_normalizer(formatters: Dict[Any, Callable[..., Any]],
                    required: Iterable[Any]=None,
                    optional: Iterable[Any]=None) -> Callable[[Dict[Any, Any]], Dict[str, Any]]:
    ...
    ...

This above function is generic and used by a lot of functions. But I need to tighten the functions which are using this function. Explained in below example.

normalize_main_transaction = dict_normalizer({
    "data": normalize_bytes,
    "gasLimit": normalize_int,
    "gasPrice": normalize_int,
    "nonce": normalize_int,
    "secretKey": normalize_bytes,
    "to": normalize_to_address,
    "value": normalize_int,
})

The normalize_main_transaction function returns object of the type
Callable[[Dict[Any, Any]], Dict[str, Any]]. But I need the type of normalize_main_transaction function to be of the type Callable[TransactionDict], TransactionDict].

How can the above be achieved?

Bhargavasomu · 2018-10-24T09:40:04Z

@cburgdorf I have done some searching regarding mypy and found that there are some issues which are still in deveopment by the mypy community. They are as follows.

Expected TypedDict key to be string literal when passing {str('foo'): str('bar')} python/mypy#4029
Overwriting the type hints of the variables is still not possible.

Because of this, in the code, I have used variable inline annotations wherever possible along with type: ignore. But circle.ci is showing the following error because of which the test cases are failing.

And by the variable annotation, it means the following.

How should I proceed with this?

cburgdorf · 2018-10-24T10:04:21Z

But circle.ci is showing the following error because of which the test cases are failing.

Since eth is still compatible with Python 3.5 you need to use the ugly comment notation for variable type annotations. Here's an example on how to do it.

py-evm/eth/chains/base.py

Lines 116 to 119 in e48b279

    
           chaindb = None  # type: BaseChainDB 
        
           chaindb_class = None  # type: Type[BaseChainDB] 
        
           vm_configuration = None  # type: Tuple[Tuple[int, Type[BaseVM]], ...] 
        
           chain_id = None  # type: int

Once we drop Python 3.5 support for the eth module (I expect that to happen within the next months) we can migrate those to the proper type annotation syntax.

Bhargavasomu · 2018-10-24T14:12:10Z

@cburgdorf I don't remember changing anything related to trinity-lightchain(went through the commit history too). The test cases are failing for that particularly. Is this because the module is to be rebased?

pipermerriam · 2018-10-24T15:52:06Z

@Bhargavasomu what you're seeing looks to be the same issue that I fixed in #1416

https://github.com/ethereum/py-evm/pull/1416/files#diff-1d37e48f9ceff6d8030570cd36286a61L269

Bhargavasomu · 2018-10-24T19:00:19Z

@pipermerriam , so maybe I should wait till #1416 gets merged?

pipermerriam · 2018-10-24T19:22:29Z

It has now been merged, try rebasing.

Bhargavasomu · 2018-10-24T19:29:51Z

@pipermerriam this is causing issues in the setup I guess. Something in installing the rocksdb is causing the error.

Bhargavasomu · 2018-10-25T06:58:23Z

@cburgdorf I have made the changes as required, the tests are failing because of some RocksDB installation, which I think is being taken care of. Could you please review it again and tell me whether anything else is to be added regarding the type hinting of this module?

cburgdorf · 2018-10-25T11:28:59Z

@Bhargavasomu I'll take a look today

cburgdorf

This looks good to me now. I applied some minor fine tuning on top. I think one could go crazy on generics to tighten some of the Any usage but honestly all these places are imho not worth going after. It's all "just" eth.tools stuff after all.

I'll leave this open just in case anyone else wants to give this another look. If I don't see anyone adding more comments by tomorrow, I'll merge ;)

@Bhargavasomu Feel free to jump to the next module (eth.vm, slicing PRs for submodules in whichever way works best for you)

As soon as this is merged, I'll trigger the payout for that achieved milestone. Great job again 👍

Bhargavasomu · 2018-10-25T18:32:38Z

@cburgdorf forgot about the NormalizerType, sorry about that. Will make the PR for the eth.vm as soon as possible. It's great working with you Christoph. Thankyou.

cburgdorf · 2018-10-26T09:39:05Z

@Bhargavasomu Likewise! You are doing great work and I'm happy to have you contributing 👍

I also just triggered the payout of the first 150 DAI for this milestone.

pipermerriam reviewed Oct 21, 2018

View reviewed changes

cburgdorf reviewed Oct 23, 2018

View reviewed changes

Bhargavasomu force-pushed the eth_tools branch from 1018e03 to f08b381 Compare October 24, 2018 09:29

Bhargavasomu force-pushed the eth_tools branch 3 times, most recently from ed7b1a8 to 4865d68 Compare October 24, 2018 13:44

Bhargavasomu force-pushed the eth_tools branch from 4865d68 to c616ac9 Compare October 24, 2018 18:56

Bhargavasomu force-pushed the eth_tools branch from c616ac9 to 0c14e29 Compare October 24, 2018 19:26

cburgdorf force-pushed the eth_tools branch from 0c14e29 to ce00f9e Compare October 25, 2018 17:00

cburgdorf approved these changes Oct 25, 2018

View reviewed changes

Enable Type Hinting for eth.tools._utils submodule

f7c300c

cburgdorf force-pushed the eth_tools branch from ce00f9e to ea241dd Compare October 26, 2018 09:19

Bhargavasomu and others added 5 commits October 26, 2018 11:23

Enable Type Hinting for eth.tools completely

b7e60a6

Cleanup Normalizer type hints

0f1ca1b

Remove superfluous cast

614b221

Tighten input of normalize_int

060f793

Tighten input of normalize_bytes

7101306

cburgdorf force-pushed the eth_tools branch from ea241dd to 7101306 Compare October 26, 2018 09:25

cburgdorf merged commit a59579b into ethereum:master Oct 26, 2018

cburgdorf mentioned this pull request Oct 26, 2018

Add type hints to eth module #1398

Closed

Bhargavasomu deleted the eth_tools branch November 8, 2018 06:27

Bhargavasomu mentioned this pull request Nov 12, 2018

Add type hints to eth.tools.builder modules #1366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete Type Hinting for `eth.tools` #1420

Complete Type Hinting for `eth.tools` #1420

Bhargavasomu commented Oct 21, 2018

Bhargavasomu commented Oct 21, 2018

pipermerriam Oct 21, 2018

cburgdorf Oct 23, 2018

cburgdorf commented Oct 22, 2018

cburgdorf left a comment

cburgdorf Oct 23, 2018

cburgdorf Oct 23, 2018

cburgdorf Oct 23, 2018

cburgdorf Oct 23, 2018

Bhargavasomu Oct 23, 2018

carver Oct 23, 2018

Bhargavasomu Oct 23, 2018 •

edited

Loading

carver Oct 23, 2018

Bhargavasomu commented Oct 23, 2018

Bhargavasomu commented Oct 24, 2018

cburgdorf commented Oct 24, 2018

Bhargavasomu commented Oct 24, 2018

pipermerriam commented Oct 24, 2018

Bhargavasomu commented Oct 24, 2018

pipermerriam commented Oct 24, 2018

Bhargavasomu commented Oct 24, 2018

Bhargavasomu commented Oct 25, 2018

cburgdorf commented Oct 25, 2018

cburgdorf left a comment

Bhargavasomu commented Oct 25, 2018

cburgdorf commented Oct 26, 2018

Complete Type Hinting for eth.tools #1420

Complete Type Hinting for eth.tools #1420

Conversation

Bhargavasomu commented Oct 21, 2018

What was wrong?

How was it fixed?

Cute Animal Picture

Bhargavasomu commented Oct 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Oct 22, 2018

cburgdorf left a 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

Bhargavasomu Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu commented Oct 23, 2018

Bhargavasomu commented Oct 24, 2018

cburgdorf commented Oct 24, 2018

Bhargavasomu commented Oct 24, 2018

pipermerriam commented Oct 24, 2018

Bhargavasomu commented Oct 24, 2018

pipermerriam commented Oct 24, 2018

Bhargavasomu commented Oct 24, 2018

Bhargavasomu commented Oct 25, 2018

cburgdorf commented Oct 25, 2018

cburgdorf left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Oct 25, 2018

cburgdorf commented Oct 26, 2018

Complete Type Hinting for `eth.tools` #1420

Complete Type Hinting for `eth.tools` #1420

Bhargavasomu Oct 23, 2018 •

edited

Loading