Detect ENS registry's address for correct network

[filips123]

What was wrong?

ENS in Web3py only supports mainnet. Even if supported testnet is used, users have to manually specify the registry's address.

How was it fixed?

The class now detects the current network and uses the correct registry's address.

Cute Animal Picture

[carver]

No objection to the concept. 👍 It looks like it breaks some tests right now (which might not support self.web3.net.version)

[filips123]

@carver How could this be fixed? Can tests be fixed to support self.web3.net.version?

[filips123]

@carver Some tests failed because self.web3.net.version is a string and I compared it with an integer. I now convert str to int and then compare it.

Some tests are now fixed but many of them are still failing.

[filips123]

@pipermerriam @carver Do you know how to fix tests? Also, are you sure that this failure is related to my changes? The failed tests don't seem to be related to ENS and ENS test also passed.

I also tried to merge upstream into my PR to possible fix some tests. But this still doesn't fix tests.

[davesque]

Actually, sorry to keep nitpicking here but it would be nice if there was something that made it clear what chain id belonged to which network. Here are some options:

NET_VERSION_TO_ENS_ADDR = {
    1: ..., # Mainnet
    ...
}

-or-

MAINNET_NET_VERSION = 1
...
NET_VERSION_TO_ENS_ADDR = {
    MAINNET_NET_VERSION: ...,
}

The second option seems a bit nicer since it gives us other constants that could be useful. It's just as verbose as the code before my original suggestion, but it still has the advantage of using dict methods instead of re-implementing dict-like functionality in a custom function.

Incidentally, it looks like there's already an ens.constants module that might be a better place to stick these values. Then you'd just import them from that module.

I want to CC @pipermerriam and @carver here and ask what's the preferred terminology we should be using: "net version" or "chain id"? Would "chain id" actually be a better term to represent in the names of these constants?

[filips123]

I also updated this.

Regarding "net version" or "chain id" I don't know what to use. Some documentation says net version and some chain id. If needed, I can also rename this.

[carver]

Actually, sorry to keep nitpicking here

Yeah, I would say let's get this PR into a basic working state, make sure the high-level architecture is okay, and then get into the nits.

I want to CC @pipermerriam and @carver here and ask what's the preferred terminology we should be using: "net version" or "chain id"? Would "chain id" actually be a better term to represent in the names of these constants?

Presumably, it could even be the combination that's important: a pair of network and chain ID might actually be the right index.

[filips123]

@pipermerriam @carver I investigated CI failures a bit more and this is what I found:

• Test docs failed because of File "web3.eth.account.rst", line 279, in default.
  Code in that docs triggers ENS initialization and this causes ENS to get net_version. But at the time of checking net_version, no Web3 providers are detected with AutoProvider, which then raises an exception and causes the test to fail.

• Other tests failed because of some The provided value did not satisfy any of the formatter conditions exception. However, I don't know what could cause it.

Note that I also added net_version to methods that are skipped in stalecheck middleware. This is because some tests will fail when net_version is called and blockchain is not synced.

How to fix those errors?

[carver]

I'm not convinced that swallowing StaleBlockchain is right here, but won't have a chance to dig in. I would at least recommend raise from exc here so that we can see the underlying exception.

[filips123]

StaleBlockchain is again not handled in my latest commits. It was one of my (failed) attempts to fix the tests.

You can see exception traceback in CI tests of my other commits in this PR.

[filips123]

Here I also resolved conflicts and added typings, and tests now also fail. Can you also check this?

[kclowes]

Yep, I'll take a look @filips123

[kclowes]

@filips123 I'm not sure what was going on before, but I re-ran the workflow and it looks like the failures are expected failures. Looks like the parity tests are hitting your UnknownNetwork error. Doc tests are passing locally for me, so might be worth rebuilding those and trying that again. If that doesn't work, I can take a deeper look.

[filips123]

Because of ENS resolver migration, addresses on all networks are now the same, and this was achieved in 802c6f5. However, Rinkeby requires PoA middleware so this PR now only adds it. I also renamed ENS_MAINNET_ADDR to ENS_PUBLIC_ADDR because the address is the same for all public networks.

[fselmo]

Thanks for the contribution here but looks like this is quite stale at this point.