Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add type hints #1118

Closed
wants to merge 10 commits into from
Closed

Conversation

johnsBeharry
Copy link

Work In Progress: Do not merge


What was wrong?

Related to Issue #1112

How was it fixed?

Due to the size of the codebase I'm automating some of the type hinting with monkeytype but the results still require a lot of tweaking.

Cute Animal Picture

cute-hyena

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a partial review of some things that stood out to me.

BTW, this will absolutely have to be done in smaller pieces (we can't type the whole library in one pull request as reviewing that PR would be too difficult). I'd recommend doing that sooner than later.

ens/main.py Outdated
@@ -188,7 +190,7 @@ def resolve(self, name, get='addr'):
else:
return None

def resolver(self, normal_name):
def resolver(self, normal_name: str): # mt: Optional[ConciseContract]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carver with ConciseContract on the way out this should probably be updated to just be a normal web3.contract.Contract type right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but... it will very likely the ens module, which is internally expecting the ConciseContract API. So maybe not here.

Also, I think the same concept will still exist, just not as a factory, with somewhat fewer features, and by a different name: ContractCaller. Updating ENS to use the new class can be a part of resolving #1025

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked ContractReader and exposing it via Contract.reader automatically as part of the core contract API.

ens/main.py Outdated
@@ -254,15 +256,15 @@ def setup_owner(self, name, new_owner=default, transact={}):
self._claim_ownership(new_owner, unowned, owned, super_owner, transact=transact)
return new_owner

def _assert_control(self, account, name, parent_owned=None):
def _assert_control(self, account: str, name: str, parent_owned: Optional[str] = None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

account should probably be eth_typing.ChecksumAddress

ens/main.py Outdated
@@ -279,7 +281,7 @@ def _first_owner(self, name):
return (owner, unowned, name)

@dict_copy
def _claim_ownership(self, owner, unowned, owned, old_owner=None, transact={}):
def _claim_ownership(self, owner: str, unowned: List[str], owned: str, old_owner: Optional[str] = None, transact: Dict[str, str] = {}) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owner should probably be eth_typing.ChecksumAddress (though there's some question here about supporting canonical (bytes) addresses too... but maybe we can gloss over that for now)

The transact can be made more specific using mypy_extensions.TypedDict

https://github.com/python/mypy/blob/a7f0263f29317985e0a2ca6f85ba5b7666401347/extensions/mypy_extensions.py#L73-L99

ens/utils.py Outdated
@@ -32,7 +36,7 @@ def Web3():
return Web3


def dict_copy(func):
def dict_copy(func: Callable) -> Callable:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to use a generic type here: https://docs.python.org/3.5/library/typing.html#typing.Generic so that the return type of the returned function is preserved.

@@ -31,11 +31,12 @@
)


def filter_by_type(_type, contract_abi):
from typing import Any, Dict, Iterator, List, Optional, Tuple, Type, Union
def filter_by_type(_type: str, contract_abi: Any) -> Union[List[Dict[str, Union[bool, str]]], List[Dict[str, Union[bool, List[Dict[str, Union[bool, str]]], str]]], List[Union[Dict[str, Union[bool, List[Dict[str, str]], str]], Dict[str, Union[bool, str]]]], List[Dict[str, Union[bool, List[Dict[str, str]], str]]], List[Union[Dict[str, Union[bool, str]], Dict[str, Union[bool, List[Dict[str, Union[bool, str]]], str]]]]]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need to define some typing.NewType and mypy_extensions.TypedDict for the overall shape of a contract ABI if we want the typing here to be readable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, there's some even more crazy outputs from monkeytype which makes things very unreadable that I haven't committed as yet.

I'll see how I can start offloading some of these to typing.NewType and mypy_extensions.TypedDict in the morning.

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more drive-by comments. didn't mark everywhere that eth_typing.Hash32 should be used, or all of the locations where things should be loosened to typing.Sequence instead of List.

ens/main.py Outdated
@@ -48,7 +65,7 @@ class ENS:
is_valid_name = staticmethod(is_valid_name)
reverse_domain = staticmethod(address_to_reverse_domain)

def __init__(self, providers=default, addr=None):
def __init__(self, providers: Any = default, addr: Optional[ChecksumAddress] = None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the type for providers should be typing.Sequence[web3.providers.base.BaseProvider]

ens/main.py Outdated
@@ -291,7 +308,7 @@ def _claim_ownership(self, owner, unowned, owned, old_owner=None, transact={}):
owned = "%s.%s" % (label, owned)

@dict_copy
def _set_resolver(self, name, resolver_addr=None, transact={}):
def _set_resolver(self, name: str, resolver_addr: None = None, transact: Dict[str, str] = {}): # mt: return was set as ConciseContract
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that this (and multiple other functions) use a mutable default argument. This isn't causing us problems because of the dict_copy decorator, but it should be fixed. All of these methods that have a transact parameter with the default being a dictionary need to be updated as follows:

def the_method(self, ..., transact=None):
    if transact is None:
        transact = {}
    ...

Do you mind making those changes along with the current work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of-course not a problem. I'll also place it in a separate branch for a PR so it can be integrated into master on it's own.

ens/main.py Outdated
@@ -279,7 +296,7 @@ def _first_owner(self, name):
return (owner, unowned, name)

@dict_copy
def _claim_ownership(self, owner, unowned, owned, old_owner=None, transact={}):
def _claim_ownership(self, owner: ChecksumAddress, unowned: List[str], owned: str, old_owner: Optional[ChecksumAddress] = None, transact: Dict[str, str] = {}) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unowned should be updated to be typing.Sequence[str]

see this document for why: https://github.com/ethereum/snake-charmers-tactical-manual/blob/master/style-guide.md#choosing-the-right-type

ens/main.py Outdated
@@ -279,7 +296,7 @@ def _first_owner(self, name):
return (owner, unowned, name)

@dict_copy
def _claim_ownership(self, owner, unowned, owned, old_owner=None, transact={}):
def _claim_ownership(self, owner: ChecksumAddress, unowned: List[str], owned: str, old_owner: Optional[ChecksumAddress] = None, transact: Dict[str, str] = {}) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been using mypy_extensions.TypedDict for dictionaries with known key/value types.

See https://github.com/python/mypy/blob/a7f0263f29317985e0a2ca6f85ba5b7666401347/extensions/mypy_extensions.py#L74-L99 for how to use it.

ens/utils.py Outdated
if not isinstance(data, str):
return Web3().toHex(data)
return data


def init_web3(providers=default):
def init_web3(provider=default) -> Web3:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's missing a type for provider

ens/utils.py Outdated
if timestamp:
return datetime.datetime.fromtimestamp(timestamp, datetime.timezone.utc)
else:
return None


def sha3_text(val):
def sha3_text(val: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the output type here should be eth_typing.Hash32 which is a special type of bytes

ens/utils.py Outdated
if isinstance(val, str):
val = val.encode('utf-8')
return Web3().keccak(val)


def label_to_hash(label):
def label_to_hash(label: str) -> HexBytes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the output type here should be eth_typing.Hash32

ens/utils.py Outdated
label = normalize_name(label)
if '.' in label:
raise ValueError("Cannot generate hash for label %r with a '.'" % label)
return Web3().keccak(text=label)


def name_to_hash(name):
def name_to_hash(name: str) -> bytes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same: eth_typing.Hash32

@wolovim
Copy link
Member

wolovim commented Feb 10, 2020

Hate to throw out the work, but types appear to have already been implemented across the lib. Okay to close? cc/ @kclowes

@kclowes
Copy link
Collaborator

kclowes commented Feb 10, 2020

Yep, thanks @marcgarreau! And thanks for the time spent @johnsBeharry!

@kclowes kclowes closed this Feb 10, 2020
@johnsBeharry
Copy link
Author

@marcgarreau the branch got stale a long time ago. it's no problem at all, had to take a step away from it for a while but glad its been implemented. apologies for for not sending an update sooner though 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants