-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Change eth_getBalance and eth_getCode to use Method #1733
Conversation
a77cb01
to
40a64c8
Compare
@@ -222,6 +214,17 @@ def block_identifier_munger( | |||
block_identifier = self.defaultBlock | |||
return [*args, block_identifier] | |||
|
|||
def address_resolver_munger(self, account, block_identifier=None): | |||
resolved_addr = abi_ens_resolver(self.web3, 'address', account)[1] |
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'm fairly sure that having this web3
here is going to cause problems when we actually implement async, but I think it's the best solution for the moment. I believe the issue is that the middleware used to resolve an ENS address before it hit validate_address
, but now validate_address
is called before an ENS address is resolved. This results in an InvalidAddress
error in the case where an ENS name is passed because validate_address
expects a hexstring. For posterity's sake, the approach I took earlier was to add a conditional in validate_address
to check that the domain was valid according to the idna spec, without actually resolving it, and then allowing the middleware to resolve it after the validation.
@marcgarreau let me know if you have a preference on approach!
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'm not too concerned about the added "cost" to the async work at this point, but this context may be long forgotten by the time its needed. Maybe a note on this issue is the way to address that?
Conceptually this is easy enough to reason about, though: if you pass an ENS name into one of these methods, the first thing that happens is a resolution check. 👍
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'm fine kicking the can down the road till async hits.
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.
This will also fail as soon as the method is used on a ModuleV2
since the current architecture there removes self.web3
on the module class.
d5349a4
to
fbcc75c
Compare
What was wrong?
The Eth module needs to be moved to use Method. Part 2 includes changes to eth_getBalance and eth_getCode.
Related to Issue #1568
How was it fixed?
Updated eth.py to use Method for selected methods.
Todo:
Cute Animal Picture