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

Implements all methods of the ENS registry #3325

Merged
merged 58 commits into from
Feb 21, 2020
Merged

Implements all methods of the ENS registry #3325

merged 58 commits into from
Feb 21, 2020

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Jan 21, 2020

Description

This PR does implement all the known methods of the ENS registry and brings more consistency to the ENS module. This as a good base clean up to forward the improvements listed in #3225.

Closed tasks from #3225:

  • Increase code style consistency
  • Support full registry API ( comments ENS reverse lookup #2683 )
  • Fix error handling for callbacks (UnhandledPromiseRejectionWarning)

Part of #3225

Things I've done

  • I've updated the funcDocs to increase the code style consistency
  • I've updated the type definitions (also for existing methods because they were misleading)
  • I've improved the naming of variables to increase the readability
  • I've added all missing Registry methods
  • I've added the missing but typed Resolver method supportsInterface
  • I've updated the callback signature to be aligned with the rest of the lib but with respecting of backward compatibility
  • I've improved the consistency of ENS module method names and deprecated the legacy ones

Findings

  • eth_call options can't get passed as expected and are required to for example display the history of owners of a name (You could define the defaultBlock property to achieve this but this is definitely not an intuitive way to interact with the API).

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 and extended the tests if necessary.
  • 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.

@nivida nivida added Feature Request 1.x 1.0 related issues labels Jan 21, 2020
@coveralls
Copy link

coveralls commented Jan 21, 2020

Coverage Status

Coverage increased (+0.6%) to 86.057% when pulling dea023f on feature/ENS-registry into 620c0ba on 1.x.

Copy link
Contributor Author

@nivida nivida left a comment

Choose a reason for hiding this comment

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

I have added some explanatory comments and a small note for a required improvement of the ENS "unit" tests.

packages/web3-eth-contract/types/index.d.ts Show resolved Hide resolved
docs/web3-eth-ens.rst Show resolved Hide resolved
docs/web3-eth-ens.rst Show resolved Hide resolved
docs/web3-eth-ens.rst Show resolved Hide resolved
packages/web3-eth-ens/src/ENS.js Show resolved Hide resolved
test/eth.ens.js Show resolved Hide resolved
test/eth.ens.js Outdated Show resolved Hide resolved
@nivida
Copy link
Contributor Author

nivida commented Jan 23, 2020

I've kept the ENS e2e tests as they are and will implement them within a separate PR after all the improvements from #3225 are finished.

@nivida nivida marked this pull request as ready for review January 23, 2020 10:16
@nivida nivida added the Review Needed Maintainer(s) need to review label Jan 23, 2020
@nivida nivida requested a review from cgewecke January 23, 2020 10:16
Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

This looks great.

There's a bit of a test gap - coverage is dropping almost a point. Do you think it would be possible to use the fixtures you wrote for #3301 to write some simple tests for this?

I think if you put scripts/e2e.ens.sh inside the unit_and_e2e block in ci.sh (maybe right after geth:auto?), it will get picked up by coveralls and we'd have a measurement for these files.

[EDIT - sorry I just saw this...]

I've kept the ENS e2e tests as they are and will implement them within a separate PR after all the improvements from #3225 are finished.

Is there a conflict with the other items? It seems normal to add the tests with the work.

@nivida
Copy link
Contributor Author

nivida commented Jan 28, 2020

There's a bit of a test gap - coverage is dropping almost a point. Do you think it would be possible to use the fixtures you wrote for #3301 to write some simple tests for this?
I think if you put scripts/e2e.ens.sh inside the unit_and_e2e block in ci.sh (maybe right after geth:auto?), it will get picked up by coveralls and we'd have a measurement for these files.

Yep! I will improve the coverage report! Thanks:)

Is there a conflict with the other items? It seems normal to add the tests with the work.

I've added the base e2e test initially to cover the custom registry case and only added here the unit tests and thought to create all the e2e tests as the last task of #3225. 🤔

@nivida nivida removed the Review Needed Maintainer(s) need to review label Feb 3, 2020
@nivida nivida added the Review Needed Maintainer(s) need to review label Feb 5, 2020
@nivida
Copy link
Contributor Author

nivida commented Feb 5, 2020

@cgewecke Ready for review ping 🎊 :)

Edit:
The reason for those followed commits after is a result of doing a review on my own. I will do such a review next time before I ping you.

Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

This looks great, really nice. Thanks so much for taking the the time to write all those tests - that file is huge.

Left a few rewording suggestions in the docs but otherwise LGTM 💯

docs/web3-eth-ens.rst Show resolved Hide resolved
docs/web3-eth-ens.rst Show resolved Hide resolved
docs/web3-eth-ens.rst Show resolved Hide resolved
docs/web3-eth-ens.rst Outdated Show resolved Hide resolved
docs/web3-eth-ens.rst Outdated Show resolved Hide resolved
docs/web3-eth-ens.rst Outdated Show resolved Hide resolved
docs/web3-eth-ens.rst Show resolved Hide resolved
@nivida
Copy link
Contributor Author

nivida commented Feb 6, 2020

@cgewecke I will wait on your answers on the un-resolved comments and will merge it after. Thanks for your review Chris much appreciated! :)

@nivida nivida requested a review from cgewecke February 20, 2020 10:33
@cgewecke
Copy link
Collaborator

@nivida Sorry - yes this has my approval already :) I closed the open questions and approval X 2!

@nivida
Copy link
Contributor Author

nivida commented Feb 21, 2020

@cgewecke

@nivida Sorry - yes this has my approval already :) I closed the open questions and approval X 2!

Oh sorry.. I thought a second approval will be shown in the PR overview. #shitHappens

@nivida nivida merged commit 93009e0 into 1.x Feb 21, 2020
@holgerd77 holgerd77 deleted the feature/ENS-registry branch February 25, 2020 11:57
@ryanio ryanio mentioned this pull request Apr 15, 2020
13 tasks
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 Feature Request Review Needed Maintainer(s) need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants