Add support for content hash (EIP1577) for ENS domains #1411
Conversation
| @@ -959,6 +959,43 @@ | |||
| ] | |||
|
|
|||
| RESOLVER = [ | |||
There was a problem hiding this comment.
@njgheorghita another place that should get updated eventually to use an EthPM package. I was under the impression you might already have the ENS contracts packaged up?
ens/main.py
Outdated
| transact['from'] = owner | ||
| resolver = self._set_resolver(name, transact=transact) | ||
|
|
||
| is_eip1577 = resolver.functions.supportsInterface('0xbc1c58d1').call() |
There was a problem hiding this comment.
These hex strings could benefit from being made into constants and potentially linking to where/how they are defined.
There was a problem hiding this comment.
@pipermerriam Ok, I moved them into constants.
I will also link where are they defined but I have a little problem here. Interface for EIP 1577 is defined in ENS resolver docs. But I don't know where a legacy interface is defined. It is 100% correct because it was used in old resolver contract but I haven't saw and EIP about it? What should I do here?
ens/utils.py
Outdated
|
|
||
|
|
||
| def resolve_content_record(resolver, name): | ||
| is_eip1577 = resolver.functions.supportsInterface('0xbc1c58d1').call() |
There was a problem hiding this comment.
we don't do any automatic interpolation of hex strings. I'm not sure what the contract interface is but this might need to be converted to it's bytes representation.
There was a problem hiding this comment.
Are you sure? All tests pass and I also tested this manually and all should work.
| content = resolver.functions.content(namehash).call() | ||
|
|
||
| if is_none_or_zero_address(content): | ||
| return None |
There was a problem hiding this comment.
I'd like to get away from this None return in favor of using an exception raising pattern. cc @kclowes as this might need to go through a deprecation cycle in the other functions if this pattern is used there.
For here I think we'd probably want a new exception class for this.
There was a problem hiding this comment.
Other methods (address) also use None return.
|
@pipermerriam Any update? Can you please check this again? I fixed some of the requested changes and some currently unresolved ones aren't probably so important. |
|
I ran out of time to look at this today, but I’ll take a look either tomorrow or Wednesday morning at the latest @filips123! |
| @@ -65,6 +65,7 @@ | |||
| url='https://github.com/ethereum/web3.py', | |||
| include_package_data=True, | |||
| install_requires=[ | |||
| "content-hash>=1.0.0,<2.0.0", | |||
There was a problem hiding this comment.
This needs some discussion. Just leaving comment here so it doesn't get merged before resolving this.
There was a problem hiding this comment.
@pipermerriam I understand concerns of including additional third-party package into Web3.py. However, I can update the requirement to lock content-hash to a specific version (currently 1.1.0) so you would be able to manually review it before updating.
I think that content hash support for ENS should be as important as current ENS functionality. The content hash will really expend possibilities of ENS as ENS domains are not anymore bound only to ETH addresses.
|
@filips123 Thanks for raising this PR! We are hesitant to add any more functionality to the ENS module than is strictly necessary for the core web3 library. But, it would be awesome if you took this and built an independent ENS library on top of web3 that implements this EIP (and any other ENS functionality that is missing from web3) instead! |
|
@kclowes I don't think that support for content hash for ENS should be considered as extra functionality which is not necessary for the core. I think that it is as important as whole ENS support with current support for getting addresses. |
|
We'll look into this after Devcon and figure out a path forward. Thanks for getting the PR started and for engaging in helping us figure this out. |
|
@pipermerriam Have you checked anything yet? |
|
I resolved conflicts and added typings. However, all tests fail. Can you check this? |
filips123
force-pushed
the
support-eip-1577
branch
from
1b825b9 to
9525883
Compare
|
I re-merged code and fixed typings, and it seems that tests are now working again. Update: Except lint test because of "incompatible return value type" and one other test which looks unrelated to this PR. |
|
Moving forward more and more site will be using IPFS so this has become a high priority issue for me - I hope it gets merged soon. |
|
In 2022, several new projects were introduced that enabled people to host their own websites using ENS domains, such as nimi.io and planetable.xyz. The ENS DAO recognized and rewarded these projects for their contributions to the growth of the ecosystem. https://discuss.ens.domains/t/term-2-grants-summary/14115 I am looking forward to the support for content hash in web3.py. |
|
closed by #2700 The basic functionality for interacting with set/get contenthash has been introduced by updating the default ABI on the ENS resolver to use ENS's Public Resolver 2. The |
What was wrong?
ENS does not support content hash (EIP1577) or content (legacy). See #1397.
How was it fixed?
This adds support for content hash and content and fixes #1397. It updates resolver's ABI and bytecode to support EIP1577. It adds
contentandsetup_contentmethods to ENS class.Method
contentfirst tries to get the content hash. If resolver supports it, it uses mycontent-hashlibrary to decode content hash and return it. If it doesn't support it, it uses legacy content.Method
setup_contenttries to add content hash which is encoded with mycontent-hashlibrary. But if the resolver doesn't support it, it ignorestypeof content and setshashas HEX encoded value.Tests and docs are also updated.
Cute Animal Picture