search

Consensys / ethql

A GraphQL interface to Ethereum :fire:
Apache License 2.0
622 stars 85 forks source link

ens plugin #111

Closed StevenJNPearce closed 5 years ago

StevenJNPearce commented 5 years ago

Fixes #106

Requirements

  • [x] Enhance the Address scalar to identify .eth names. The parseLiteral and parseValue functions should return a promise, which immediately resolves to the original value if it's not an ENS name, and to the resolved address if it's an ENS name.

  • [ ] Perform resolution through this library: https://github.com/ensdomains/ensjs - This library is incompatible with web3-1.0.0 - I've used the 'ez-ens' library, it works great.

  • [x] Solution must be developed as an EthQL plugin called ens. This plugin:

  • [x] * Depends on the 'web3' service, and is ordered after core.

  • [x] * Adds a new service ens: a singleton instance of the ENS object instantiated with the ensjs library above.

  • [x] * Adds resolvers that wrap core resolvers, awaiting on the promise and delegating to the underlying resolver. These fields introduced by the core schema, and resolved there, are to be wrapped:

  • [x] * top-level account

  • [x] * transactionsInvolving

  • [x] * transactionsRoles

Definition of done

  • [X] Test cases with full coverage.
  • [ ] PR accepted and merged, introducing a new top-level directory ens with the specified plugin.
  • [x] TODO Code commented, documented and linted.
  • [ ] TODO README and wiki updated listing and showcasing this functionality.
StevenJNPearce commented 5 years ago

@raulk I was a little busy these last couple of days. I should have plenty of time to work on this tomorrow.

raulk commented 5 years ago

@StevenJNPearce just checking in to see if you've got any follow-up questions?

StevenJNPearce commented 5 years ago

I was just busy, sorry about the wait. Will let you know if I have any questions.

raulk commented 5 years ago

@StevenJNPearce any chance we can get an ETC here? We have other things inflight which depend on the ENS integration now.

StevenJNPearce commented 5 years ago

@raulk I took another look at this today and have resolved most of the issues that came up in the review expect one. The one I've not dealt with is https://github.com/ConsenSys/ethql/pull/111#discussion_r223181936 . Whilst I could understand the exmaples in the tests with the commit referenced in your comment, I couldn't get my head round how to implement it - I found the combination of the reduce, merge and functions with (prev, next) arguments a bit confusing.

raulk commented 5 years ago

@StevenJNPearce yeah, that's a tricky part. I'm happy to accept suggestions regarding a clearer API.

The idea here is that some plugins are "decorator" plugins. This is the case of ens. It should not explicitly refer to other resolvers, but it should take any existing ones and "wrap" them in custom logic.

Let me have a look and propose a change on your PR.

StevenJNPearce commented 5 years ago

@raulk Commenting here as you may have missed my comment on having fixed the last change requested above.

StevenJNPearce commented 5 years ago

@raulk I don't see a way to make a pull request against the wiki repository. Maybe you can take the contents of README.md from my last commit and add them to the wiki.

raulk commented 5 years ago

@StevenJNPearce looking, thanks!

raulk commented 5 years ago

@StevenJNPearce Good job here. Unfortunately, this PR did introduce a serious regression: non-ENS addresses stopped working. And there were no test cases covering queries with normal addresses with the ENS plugin, so it went unnoticed. Luckily I noticed this, and fixed it for you in https://github.com/ConsenSys/ethql/pull/111/commits/4f6cd81acfbd6a1ecca53fa0eb8718d45003f7c3.

Strangely, the ConsenSys/ethql CircleCI account hasn't picked up this PR. Looking into that, as I'd like to see a green CircleCI before I merge.