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

Change eth_getBalance and eth_getCode to use Method #1733

Merged
merged 3 commits into from Sep 11, 2020

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented Sep 9, 2020

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

Put a link to a cute animal picture inside the parenthesis-->

ens/utils.py Outdated Show resolved Hide resolved
@@ -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]
Copy link
Collaborator Author

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!

Copy link
Member

@wolovim wolovim Sep 10, 2020

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. 👍

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 fine kicking the can down the road till async hits.

Copy link
Member

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.

@kclowes kclowes changed the title [WIP] Change eth_getBalance and eth_getCode to use Method Change eth_getBalance and eth_getCode to use Method Sep 10, 2020
@kclowes kclowes merged commit e2b5bcb into ethereum:master Sep 11, 2020
@kclowes kclowes deleted the eth-to-method-2 branch September 11, 2020 16:41
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.

None yet

3 participants