-
Notifications
You must be signed in to change notification settings - Fork 648
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
Tighten type support and fix typing issues #1948
Tighten type support and fix typing issues #1948
Conversation
d348683
to
8625942
Compare
267dcb8
to
bed2942
Compare
e7759d7
to
e711160
Compare
e711160
to
be5583b
Compare
be5583b
to
3c3102d
Compare
eth/abc.py
Outdated
... | ||
|
||
@abstractmethod | ||
def copy(self, *args: Any, **kwargs: Any) -> THeader: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just 'MiningHeaderAPI'
, so we can drop THeader
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because BlockHeaderAPI
inherits this API and so we don't want to hardcode the return type to MiningHeaderAPI
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But actually, we don't really need it to be on MiningHeaderAPI
and can move it down to BlockHeaderAPI
and then hardcode it. This makes the usage slightly nicer because otherwise mypy is asking us for a type annotation at every place where we use copy
like so:
unused_header: BlockHeaderAPI = header.copy(gas_used=0)
If I move it down to BlockHeaderAPI
we can simply do:
unused_header = header.copy(gas_used=0)
... | ||
|
||
@abstractmethod | ||
def as_dict(self) -> Dict[Hashable, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a quick comment on each of the methods that are copied over from rlp.Serializeable
, saying that they can be deleted from this API after pyrlp
properly exposes the types? (I think just these last three methods)
Edit: and the methods in the other classes, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comments on all methods. Btw, I've also tried grouping all of them in a RLPSerializableAPI(Generic[T])
so that we don't have to manually add all these methods to all these different types and while this worked great for >= py37 it yielded an obscure meta class MRO error that I couldn't resolve.
@@ -136,6 +136,6 @@ def from_header(cls, header: BlockHeaderAPI, chaindb: ChainDatabaseAPI) -> "Fron | |||
# Execution API | |||
# | |||
def add_uncle(self, uncle: BlockHeaderAPI) -> "FrontierBlock": | |||
self.uncles.append(uncle) | |||
self.uncles += (uncle,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, hooray for bugfixes. :) I guess worthy of a news fragment, and surprising and problematic that there's no test coverage on this. Maybe add a test that covers it (or at least an issue to do it as part of beta)? Looks like adding uncles was totally broken. (Is this overwritten in different forks, and that's why we didn't notice?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I have to investigate that once more tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the reason seems to be that this is essentially dead code. At least there's no code in Py-EVM or Trinity calling add_uncle
. Since this is a public API there's a non-zero chance it is used at another place that I might not be aware of but it seems unlikely. Anyway, I'm not dropping it pre-merge in case I'm overseeing something but I'm happy to send another PR to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a public API there's a non-zero chance it is used at another place that I might not be aware of but it seems unlikely.
Yeah, very unlikely, since every call to it would immediately fail 😆
I think we can safely remove an API that has been broken for 3 years:
3b0bcc219 eth/vm/forks/frontier/blocks.py (2019-08-01 11:43:24 -0600 138) def add_uncle(self, uncle: BlockHeaderAPI) -> "FrontierBlock":
26173f6ed evm/vm/flavors/frontier/blocks.py (2017-06-28 14:50:03 -0600 139) self.uncles.append(uncle)
26173f6ed evm/vm/flavors/frontier/blocks.py (2017-06-28 14:50:03 -0600 140) self.header.uncles_hash = keccak(rlp.encode(self.uncles))
26173f6ed evm/vm/flavors/frontier/blocks.py (2017-06-28 14:50:03 -0600 141) return self
I'm happy to send another PR to remove it.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, now I'm confused again. The init method adds uncles as []
. So I guess maybe it was working before, and we have to think a little more carefully. It's a mess that it's created as a tuple in one place and a list in another.
e50d9ae
to
c2ed99c
Compare
c2ed99c
to
4fd6088
Compare
What was wrong?
Currently, any method that expects any of
BaseHeaderAPI
,BaseBlockAPI
,UnsignedTransactionAPI
,ReceiptAPI
does not properly enforce type safe. In practice that means that a method such as:Can be called as
do_something_with_block(accidential_is_transaction)
ordo_something_with_block(accidential_is_header)
and mypy does not complain about it.This has allowed serious bugs to fly under the radar: ethereum/trinity#1830
The root cause for this is that anything derived from
rlp.Serializable
will be seen asAny
by mypy. We currently do not enforce this to be invalid but if so, mypy would rightfully reject a series of types:How was it fixed?
I believe the best fix would be to do a serious overhaul of py-rlp to enforce strong type support. As I tried to add type support to py-rlp in a minimalistic fashion I noticed another way of approaching this which gets us halfway there without even touching py-rlp (and hence being faster to deliver).
Basically, what this does is rewrite all
eth.abc
types to not derive fromrlp.Serializable
and move therlp.Serializable
a level deeper. That means that things such asBaseBlock
will still be seen asAny
(just like before) but at least we have full type safety at the top level e.g.BlockAPI
.This does already a great job at exposing tons of bugs without opening a huge time sink such as refactoring py-rlp.
To-Do
Cute Animal Picture