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

Add support for ENS contenthash methods #3428

Merged
merged 22 commits into from
May 7, 2020
Merged

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Mar 22, 2020

Description

Adds getContenthash and setContenthash methods to the ENS module.

Example

await web3.eth.ens.getContenthash('ethereum.eth');

> {
  "protocolType": "ipfs",
  "decoded": "QmaEBknbGT4bTQiQoe2VNgBJbRfygQGktnaW5TbuKixjYL"
}

await web3.eth.ens.setContenthash(
  'ethereum.eth', 
  'ipfs://QmaEBknbGT4bTQiQoe2VNgBJbRfygQGktnaW5TbuKixjYL,
);

As discussed in #3392, there are now two widely used resolver contracts for ENS with different interfaces.

PR adds support for the newer PublicResolver ABI, toggling between the two candidates by making an EIP 165 supportsInterface call for the contenthash method before selecting which ABI to use. If the Resolver address associated with the ENS name supports contenthash, the PublicResolver ABI is used, otherwise module defaults to the legacy Resolver ABI.

PR updates the ENS Resolver ABI to include contenthash interface as well as deprecated content. Makes EIP 165 supportsInterface calls for a subset of resolver methods to verify that the target contract implements them.

ENS has a feature that allows nodes to register ABIs on-chain for contract addresses but this does not appear to be implemented for the publicly deployed Resolver contracts themselves.

Running a reverse address check on Etherscan for the mainnet PublicResolver at: 0x4976fb03C32e5B8cfe2b6cCB31c09Ba78EBaBa41 does not discover an ENS node which might supply an ABI.

Fixes #3392

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@cgewecke cgewecke mentioned this pull request Mar 22, 2020
13 tasks
@coveralls
Copy link

coveralls commented Mar 22, 2020

Coverage Status

Coverage increased (+0.8%) to 89.43% when pulling 7b8d535 on issue/3392-ens-contenthash into 2a5c5cb on 1.x.

@cgewecke cgewecke added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Mar 22, 2020
@cgewecke cgewecke marked this pull request as ready for review March 23, 2020 02:45
@cgewecke cgewecke changed the base branch from issue/3392-ens-tests to 1.x March 25, 2020 22:42
@ricmoo
Copy link
Contributor

ricmoo commented Mar 26, 2020

ABI is supported in the public resolver (resolver.eth) See line 72: https://etherscan.io/address/0x4976fb03c32e5b8cfe2b6ccb31c09ba78ebaba41#code

@cgewecke
Copy link
Collaborator Author

@ricmoo I might misunderstand this but isn't that ABI method similar to contentHash, e.g it lets you associate an ABI with a name?

In this case we're looking for the ABI of the resolver itself and I wasn't able to see a way of retrieving it.

If it is possible it would be the best solution by far though...

@ricmoo
Copy link
Contributor

ricmoo commented Mar 27, 2020

Oh! I see what you mean.

We could talk to @Arachnid about adding the ABI to resolver.eth, but I think it would be fine to just hardcode the getters (and maybe the setters) into it; each has an associated EIP.

Also, maybe it should be contenthash, not contentHash (and setContenthash, not setContentHash) to keep it inline with the EIP-1577 standard?

@cgewecke
Copy link
Collaborator Author

Also, maybe it should be contenthash, not contentHash (and setContenthash, not setContentHash) to keep it inline with the standard?

@ricmoo Ah yes, that's a good point.

@cgewecke cgewecke force-pushed the issue/3392-ens-contenthash branch 2 times, most recently from 62591f4 to b8a0b14 Compare April 7, 2020 01:12
@cgewecke
Copy link
Collaborator Author

cgewecke commented Apr 7, 2020

Have revised per initial review - think this is moving in a better direction...

  • Removed ABI toggling
  • Added logic to query supportsInterface for each resolver method call Web3 wraps and ENS advertises an interface ID for in their docs. This is tolerant - e.g it will error if the Resolver doesn't support content but ignore methods it doesn't know about.
  • Replaced ABI with the Resolver generic from @ensdomain/[email protected]. (This includes definitions for deprecated methods like content).
  • Added an E2E test for pubkey set/get - its interface is also getting validated now.
  • Removed camel-casing of contenthash

@cgewecke cgewecke changed the title Add support for ENS content hash methods Add support for ENS contenthash methods Apr 7, 2020
package.json Outdated Show resolved Hide resolved
@@ -41,14 +55,15 @@ function ResolverMethodHandler(registry) {
* @param {function} callback
* @returns {Object}
*/
ResolverMethodHandler.prototype.method = function (ensName, methodName, methodArguments, callback) {
ResolverMethodHandler.prototype.method = function (ensName, methodName, methodArguments, outputFormatter, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outputFormatter addition wouldn't be required.. you can just wrap the promise as on other places :-)

Copy link
Contributor

@nivida nivida Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This ugly handler has to get refactored/removed anyways.. just ping me.. I do have a solution somewhere on a handwritten paper)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow, but yes this could be refactored at some point.

this.parent.registry.getResolver(this.ensName).then(function (resolver) {
self.parent.handleCall(promiEvent, resolver.methods[self.methodName], preparedArguments, callback);
this.parent.registry.getResolver(this.ensName).then(async function (resolver) {
await self.parent.checkInterfaceSupport(resolver, self.methodName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember a discussion I had once with @cgewecke about additional network requests and their impact. Shouldn't this checkInterface call not also be optional with a strictEns flag or so? Otherwise, the arguments in the discussion we had are false:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The origin of this design is the discussion beginning in #3392 comment. In this case there are two widely deployed resolvers with different APIs, so there's a strong case for validating the request before-hand.

I think you're alluding here to a debate in #3428 about whether or not to gate revert-with-reason behind an option flag. The ENS module is not subject to the similar considerations AFAIK, personally don't think this will impact people in the same way 🤷

CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of just copy it should we probably create an EF package for it with working together with ENS @Arachnid

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code comes out of @ensdomains/ui which is relatively large. This seemed like the lightest approach.

@@ -22,6 +22,12 @@ import { TransactionRevertInstructionError } from 'web3-core-helpers';
import { Eth } from 'web3-eth';
import { Contract } from 'web3-eth-contract';

export interface ContentHash {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice! 🎊

test/eth.ens.js Outdated Show resolved Hide resolved
@cgewecke
Copy link
Collaborator Author

Rebased on bcf248f

@cgewecke
Copy link
Collaborator Author

Update: Have made suggested improvements per @nivida's review:

  • interfaceIds moved to config
  • preserved the legacy ABI and only added contentHash methods as necessary
  • deleted a stray mocha timeout

@andrewheadricke
Copy link

Just stumbled across this issue, looking forward to getting this incorporated into web3

Copy link
Collaborator

@ryanio ryanio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice comprehensive work @cgewecke.

New tests are passing successfully in CI:

    ✓ should error when calling "getContent" if resolver does not support it
    ✓ should error when calling "setContent" if resolver does not support it
    ✓ getContenthash return object keys are null if no contentHash is set
    ✓ should get/set an IPFS contenthash (ipfs://)
    ✓ should get/set an IPFS contenthash (/ipfs/)
    ✓ should get/set a bzz contenthash
    ✓ should get/set an onion contenthash
    ✓ should get/set an onion3 contenthash
    ✓ setContenthash errors when encoding an invalid contenthash (promise)
    ✓ setContentHash errors when encoding an invalid contenthash (callback)

@ryanio ryanio merged commit 31459e9 into 1.x May 7, 2020
@holgerd77 holgerd77 deleted the issue/3392-ens-contenthash branch May 7, 2020 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Review Needed Maintainer(s) need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

web3.eth.ens.getContent seems to do not work properly
6 participants