Non-substantive typing clean-up #2036
Conversation
There was a problem hiding this comment.
- Hesitant about introducing
PyListin spec - Agree with
uint64/other types for constants - Some type casts in the code seem unnecessary, since they should automatically follow from the arithmetic typing rules of the ssz code.
PR looks good, but the balance between typing and readability/simplicity is getting blurry
| @@ -773,7 +773,7 @@ def compute_committee(indices: Sequence[ValidatorIndex], | |||
| Return the committee corresponding to ``indices``, ``seed``, ``index``, and committee ``count``. | |||
| """ | |||
| start = (len(indices) * index) // count | |||
| end = (len(indices) * (index + 1)) // count | |||
| end = (len(indices) * uint64(index + 1)) // count | |||
There was a problem hiding this comment.
I thought this wouldn't be necessary anymore with the changes to remerkleable? uint64 + int -> uint64?
There was a problem hiding this comment.
The error message (#1707) was error: Need type annotation for 'end'. I think mypy couldn't determine the type here with static checks.
I can change it to end: uint64 = (len(indices) * (index + 1)) // count alternatively.
There was a problem hiding this comment.
Maybe it works when you uint64(len(indices)) instead? I find it weird to coerce the result, instead of the inputs.
| shard_block_lengths = [] # type: PyList[uint64] | ||
| shard_data_roots = [] # type: PyList[Root] | ||
| shard_states = [] # type: PyList[ShardState] |
There was a problem hiding this comment.
PyList was not meant to make it into the spec. More type safety is nice, but I am hesitant to introduce this.
There was a problem hiding this comment.
I was hesitant too, and intentionally used type comments instead of shard_block_lengths: PyList[uint64] = []. 😅 But I understand your concerns.
A "PyList is okay" argument is that we already have Python Set, Dict, and Tuple in the specs.
Would renaming PyList to Array be better?
There was a problem hiding this comment.
PyList is okay then I guess. And sorry for nitpicking, it's just a style thing that we tried to avoid previously, but typing is more important.
uint64()castings to phase 1 constants and configurations.