Christoph/fix/typehints #1412

cburgdorf · 2018-10-19T14:25:48Z

This is mostly #1404 by @Bhargavasomu with some minimal fine tuning on top.

What was wrong?

The eth module was not type hinted till now. This needs to be fixed and also the tox.ini file needs to be updated.
Issue : #1398

How was it fixed?

Adding type hints to the arguments and the function. The phase of adding the type hints is as follows

eth.utils (Started previously and hence wanted to finish it)
eth.tools
eth.vm
Everything else which is leftover

Cute Animal Picture

pipermerriam · 2018-10-19T15:31:55Z

eth/tools/builder/chain/builders.py


+VMConfigurationType = Iterable[Tuple[int, Type[BaseVM]]]
+GeneralStateType = Union[AccountState, List[Tuple[Address, Dict[str, Union[int, bytes, Dict[int, int]]]]]]  # noqa: E501


I think the inner Dict should be updated to be an AccountDetals.

pipermerriam · 2018-10-19T15:32:08Z

eth/typing.py

+
+# TODO: Move into eth_typing
+
+AccountDetails = TypedDict('AccountDetails',


Ooooh! TypedDict is cool.

Yep, they let us avoid sprinkling the code with assert statements #1404 (comment)

pipermerriam · 2018-10-19T15:34:22Z

eth/utils/module_loading.py


-def import_string(dotted_path):
+
+def import_string(dotted_path: str) -> Any:


can you use types.ModuleType here instead of Any?

pipermerriam · 2018-10-19T15:34:47Z

eth/utils/numeric.py

@@ -12,7 +15,7 @@
 )


-def int_to_bytes32(value):
+def int_to_bytes32(value: Union[int, bool]) -> bytes:


I think Hash32 is more appropriate than bytes

pipermerriam · 2018-10-19T15:37:10Z

eth/utils/transactions.py

-def create_transaction_signature(unsigned_txn, private_key, chain_id=None):
+def create_transaction_signature(unsigned_txn: BaseTransaction,
+                                 private_key: datatypes.PrivateKey,
+                                 chain_id: int=None) -> Tuple[int, int, int]:


candidate for eth-typing VRS = NewType("VRS", Tuple[int, int, int])

also, UINT256 = NewType("UINT256", int) for these values that are specifically bounded within the 256 bit integer space.

So VRS would then be a VRS = NewType("VRS", Tuple[UINT256, UINT256, UINT256])?

Yes, though v might have a lower ceiling than the others (not sure what it is, might be a single byte).

Don't talk low level math with me! What does v even stand for? Vitalik? ;)

https://bitcoin.stackexchange.com/questions/38351/ecdsa-v-r-s-what-is-v/38909

v is the recovery id and looks to be reasonably bounded within a single byte (2**8 or UINT8)

cburgdorf · 2018-10-19T16:20:40Z

@Bhargavasomu thanks again for doing the bulk of the work! You may want to check out the few extra commits on top just to get a feel what kind of changes were made.

carver · 2018-10-19T18:53:02Z

tox.ini

@@ -45,7 +45,7 @@ commands=
    native-state-metropolis: pytest {posargs:tests/json-fixtures/test_state.py -k Metropolis}
    lightchain_integration: pytest --integration {posargs:tests/trinity/integration/test_lightchain_integration.py}

-deps = .[p2p,trinity,eth-extra,test]
+deps = .[eth,p2p,trinity,eth-extra,test]


This is confusing to me: . should already be installing those packages:
https://github.com/cburgdorf/py-evm/blob/db8bf2e1003bb18542a3e36ff46fbffabfddc808/setup.py#L102

Thanks for pointing that out, now I'm confused as well. Let's see #1417

carver · 2018-10-19T18:53:40Z

setup.py

@@ -12,6 +12,7 @@
        "eth-typing>=2.0.0,<3.0.0",
        "eth-utils>=1.3.0b0,<2.0.0",
        "lru-dict>=1.1.6",
+        "mypy_extensions>=0.4.1,<1.0.0",


Feels a little weird, like including flake8 in the distribution. Don't feel strongly about it though.

At the end of the day, mypy_extensions is a library as any other and we are using it now for things like TypedDict. Notice that this is not just a type hint per se. We have to have it in order for code such as the following to work.

py-evm/eth/typing.py

Lines 20 to 25 in 7ad4259

AccountDetails = TypedDict('AccountDetails',

{'balance': int,

'nonce': int,

'code': bytes,

'storage': Dict[int, int]

})

Bhargavasomu · 2018-10-20T03:14:34Z

@cburgdorf I noticed that you have moved some type definitions to eth.typing and importing from there, which is fancy BTW. I haven't known we could do this, definitely will use this method for frequently invoking type definitions

cburgdorf · 2018-10-20T06:03:35Z

Yep, and in the long run we might move some if not all to https://github.com/ethereum/eth-typing

cburgdorf force-pushed the christoph/fix/typehints branch from bf9dfb0 to 590668a Compare October 19, 2018 14:28

cburgdorf mentioned this pull request Oct 19, 2018

Add type hints For eth/utils module #1404

Closed

pipermerriam approved these changes Oct 19, 2018

View reviewed changes

cburgdorf force-pushed the christoph/fix/typehints branch from a649738 to d75a4ff Compare October 19, 2018 15:38

Bhargavasomu and others added 4 commits October 19, 2018 17:56

Add type hints For eth/utils module

70ece90

Tighten type of gas estimator

39fccc3

Dedupe some types into eth.typing

2a38a41

Tighten int_to_bytes32

4e21aae

cburgdorf force-pushed the christoph/fix/typehints branch from 7d5e5ad to 141a415 Compare October 19, 2018 15:56

cburgdorf added 2 commits October 19, 2018 18:00

Tighten type of import_string

dd0fd42

Introduce VRS type

e27638a

cburgdorf force-pushed the christoph/fix/typehints branch from 141a415 to e27638a Compare October 19, 2018 16:05

Rename VMConfigurationType -> VMConfiguration

db8bf2e

cburgdorf merged commit e4a203a into ethereum:master Oct 19, 2018

carver reviewed Oct 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Christoph/fix/typehints #1412

Christoph/fix/typehints #1412

cburgdorf commented Oct 19, 2018 •

edited

Loading

pipermerriam Oct 19, 2018

pipermerriam Oct 19, 2018

cburgdorf Oct 19, 2018

pipermerriam Oct 19, 2018

pipermerriam Oct 19, 2018

pipermerriam Oct 19, 2018

cburgdorf Oct 19, 2018

pipermerriam Oct 19, 2018

cburgdorf Oct 19, 2018

pipermerriam Oct 19, 2018

cburgdorf commented Oct 19, 2018

carver Oct 19, 2018

cburgdorf Oct 19, 2018

carver Oct 19, 2018

cburgdorf Oct 19, 2018

Bhargavasomu commented Oct 20, 2018

cburgdorf commented Oct 20, 2018


		VMConfigurationType = Iterable[Tuple[int, Type[BaseVM]]]
		GeneralStateType = Union[AccountState, List[Tuple[Address, Dict[str, Union[int, bytes, Dict[int, int]]]]]] # noqa: E501


		# TODO: Move into eth_typing

		AccountDetails = TypedDict('AccountDetails',


		def import_string(dotted_path):

		def import_string(dotted_path: str) -> Any:

	AccountDetails = TypedDict('AccountDetails',
	{'balance': int,
	'nonce': int,
	'code': bytes,
	'storage': Dict[int, int]
	})

Christoph/fix/typehints #1412

Christoph/fix/typehints #1412

Conversation

cburgdorf commented Oct 19, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

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

cburgdorf commented Oct 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

Bhargavasomu commented Oct 20, 2018

cburgdorf commented Oct 20, 2018

cburgdorf commented Oct 19, 2018 •

edited

Loading