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

Add type hints to web3.providers.ethtester #1518

Merged

Conversation

njgheorghita
Copy link
Contributor

What was wrong?

Type hints missing from web3.providers.ethtester

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the type-hints-providers-ethtester branch 2 times, most recently from 9648037 to 48fcf7a Compare November 20, 2019 13:25
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.

Would like to re-review once you've made another pass at tightening some of the types (assuming they can actually be tightened)

@@ -190,7 +191,7 @@ def address_in(address: ChecksumAddress, addresses: Collection[ChecksumAddress])


def address_to_reverse_domain(address: ChecksumAddress) -> str:
lower_unprefixed_address = remove_0x_prefix(to_normalized_address(address))
lower_unprefixed_address = remove_0x_prefix(HexStr(to_normalized_address(address)))
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR but it would be ideal if we got our typing right so that we didn't need to wrap this in HexStr.

if fn_kwargs is None:
fn_kwargs = {}
return getattr(eth_tester, fn_name)(*fn_args, **fn_kwargs)


def without_eth_tester(fn):
def without_eth_tester(fn: Callable[..., Any]) -> Callable[..., RPCResponse]:
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be more accurately typed as:

TReturn = TypeVar("TReturn")
TParams = TypeVar("TParams")

def without_eth_tester(fn: Callable[[TParams], TReturn]) -> Callable[[EThereumTester, TParams], TReturn]:
    ...

return fn(params)
return inner


def without_params(fn):
def without_params(fn: Callable[..., Any]) -> Callable[..., RPCResponse]:
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Typing can be more precicesly defined using TypeVar types.

return eth_tester, preprocessor_fn(params)


def static_return(value):
def inner(*args, **kwargs):
def static_return(value: Any) -> Callable[..., Any]:
Copy link
Member

Choose a reason for hiding this comment

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

This one should be doable as def static_return(value: TValue) -> Callable[..., TValue]:

@@ -73,7 +101,7 @@ def client_version(eth_tester, params):


@curry
def null_if_excepts(exc_type, fn):
def null_if_excepts(exc_type: Type[BaseException], fn: Callable[..., Any]) -> Callable[..., Any]:
Copy link
Member

Choose a reason for hiding this comment

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

Same here, types can be tightened using TypeVar for callable return type.

filter_params = params[0]
filter_id = eth_tester.create_log_filter(**filter_params)
return filter_id


def get_logs(eth_tester, params):
def get_logs(eth_tester: "EthereumTester", params: Any) -> List[LogReceipt]:
Copy link
Member

Choose a reason for hiding this comment

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

For each of these that is a defined API we should know what the params value looks like and thus be able to use stricture types than Any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Will take care of tightening up the params value in a different pr to avoid bloat.

return eth_tester.add_account(_generate_random_private_key())


def personal_send_transaction(eth_tester, params):
def personal_send_transaction(eth_tester: "EthereumTester", params: Any) -> HexStr:
Copy link
Member

Choose a reason for hiding this comment

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

params here should be someting like Optional[TxParams] I think.

async def make_request(self, method, params):
async def make_request(
self, method: RPCEndpoint, params: Any
) -> Coroutine[Any, Any, RPCResponse]:
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this returning a Coroutine type. I would have expected just RPCResponse...

@@ -55,7 +81,7 @@ def __init__(self, ethereum_tester=None, api_endpoints=None):
else:
self.api_endpoints = api_endpoints

def make_request(self, method, params):
def make_request(self, method: RPCEndpoint, params: Any) -> RPCResponse:
Copy link
Member

Choose a reason for hiding this comment

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

At minimum do we know that params is a List[Any]? I'm not 100% sure but I think that we have some knowledge that params is a list of values.

Copy link
Contributor Author

@njgheorghita njgheorghita Nov 21, 2019

Choose a reason for hiding this comment

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

params This will be a list or other iterable of the parameters for the JSON-RPC method being called.

Yup! - but it from some testing it seems that it requires a List type not Sequence / Iterable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this update touches a lot of files, so to avoid from bloating this pr, i'll follow it up with another that tightens this up

@njgheorghita njgheorghita merged commit d0ae459 into ethereum:master Nov 22, 2019
@njgheorghita njgheorghita deleted the type-hints-providers-ethtester branch November 22, 2019 11:18
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.

2 participants