Only try to resolve ENS names on mainnet #1037
Conversation
ankitchiplunkar
changed the title
|
@carver @pipermerriam would love any inputs on this PR. |
web3/utils/normalizers.py
Outdated
| @@ -137,6 +138,8 @@ def abi_ens_resolver(w3, abi_type, val): | |||
| raise InvalidAddress("Could not look up name, because no web3 connection available") | |||
| elif w3.ens is None: | |||
| raise InvalidAddress("Could not look up name, because ENS is set to None") | |||
| elif int(w3.net.version) is not 1 and not isinstance(w3.ens, StaticENS): | |||
| raise InvalidAddress("Could not look up name, because web3 is not connected to mainnet") | |||
There was a problem hiding this comment.
Would you add the name val to this message and the ones above it? Something like:
"Could not look up name %r, because web3 is not connected to mainnet" % val
| def test_pass_name_resolver_mainnet(): | ||
| url = "https://mainnet.infura.io/" | ||
| w3 = w3_name_resolver(url) | ||
| w3.eth.getBalance("ethereum.eth") |
There was a problem hiding this comment.
This looks like it's actually hitting Infura for the test, right? Tests should be able to run while disconnected from the Internet.
One option is to use construct_fixture_middleware to return the network id (probably injected at layer 0).
There was a problem hiding this comment.
Pushed the test offline but the BaseProvider was missing several methods which had to be implemented eg eth_getBlockByNumber, eth_call and eth_getBalance.
Tried to enable these methods using construct_fixture_middleware at layer 0 but the command w3.eth.getBalance(name) would call eth_getBalance --> eth_call --> eth_getBlockByNumber (which would again throw method not implemented error).
Maybe after 2-3 recusrive calls the method construct_fixture_middleware fails.
Have bypassed this issue by letting pytest catch the NotImplementedError do tell me if this is not an acceptable solution.
There was a problem hiding this comment.
@carver drive-by-idea.
fixture_recorder_middleware
Lets you connect whatever provider you like, make some calls, and then export the request/response data in a format that could then be passed into construct_fixture_middleware to emulate the request/responses.
| url = "https://kovan.infura.io/" | ||
| w3 = w3_name_resolver(url) | ||
| with pytest.raises(InvalidAddress): | ||
| w3.eth.getBalance("ethereum.eth") |
| }) | ||
| w3.middleware_stack.inject(return_chain_on_mainnet, layer=0) | ||
| with pytest.raises(NotImplementedError): | ||
| w3.eth.getBalance('ethereum.eth') |
There was a problem hiding this comment.
I think this could end up as a passing test even if the implementation is broken, too easily. It's expected to struggle with the right solution here. Part of the reason this change wasn't made yet is that testing it well is tricky. :)
StaticENS seems like an obvious choice, but decide whether to look up the name with an isinstance on that class, it's problematic to use in the test. Maybe reimplement a new special StaticENS in the test module and use that instead.
There was a problem hiding this comment.
Have created a new ENS class in the test for resolving this middleware
web3/utils/normalizers.py
Outdated
| because ENS is set to None" % val) | ||
| elif int(w3.net.version) is not 1 and not isinstance(w3.ens, StaticENS): | ||
| raise InvalidAddress("Could not look up name %r, \ | ||
| because web3 is not connected to mainnet" % val) |
There was a problem hiding this comment.
nitpick
We've been avoiding line continuations like that in these projects. Our style guide is here: https://github.com/pipermerriam/ethereum-dev-tactical-manual/blob/master/style-guide.md#multi-line-statements
The preferred style is something like:
raise InvalidAddress(
"Could not look up name %r, "
"because web3 is not connected to mainnet" % (
val,
)
)(I broke up the lines a lot more than you probably need to, I was just demonstrating the preferred newline locations on a really long line)
There was a problem hiding this comment.
Accepted the changes
| 'net_version': '2', | ||
| }) | ||
| w3.middleware_stack.inject(return_chain_on_mainnet, layer=0) | ||
| with pytest.raises(InvalidAddress): |
There was a problem hiding this comment.
This test can now confirm that the name shows up in the exception with something like:
with pytest.raises(InvalidAddress, match='.*ethereum\.eth.*'):
carver
force-pushed
the
ankit/ens_mainnet_middleware
branch
from
11798b5 to
606ee3a
Compare
What was wrong?
Related to Issue # #887
How was it fixed?
abi_name_resolverto check if the chain is mainnet.StaticENSto enable passing of old tests.Cute Animal Picture