ens plugin #111

StevenJNPearce · 2018-10-03T06:14:51Z

Fixes #106

Requirements

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.

Solution must be developed as an EthQL plugin called ens. This plugin:

* Depends on the 'web3' service, and is ordered after core.

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

* 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:

* top-level account

* transactionsInvolving

* transactionsRoles

Definition of done

Test cases with full coverage.

PR accepted and merged, introducing a new top-level directory ens with the specified plugin.

TODO Code commented, documented and linted.

TODO README and wiki updated listing and showcasing this functionality.

raulk

Some initial feedback to unblock you ;-)

src/__tests__/ens/ens.test.ts

raulk · 2018-10-06T13:02:02Z

src/ens/services/ens/index.ts

@@ -0,0 +1,11 @@
+import ENS = require('ez-ens');


Putting this file in src/ens/services/ens.ts should be fine. It's simple enough to not need a directory.

raulk · 2018-10-06T13:07:11Z

src/ens/resolvers/index.ts

@@ -0,0 +1,57 @@
+import { GraphQLScalarType, Kind } from 'graphql';


Commit 5ba9227 introduced a change that allows the resolver map to be a function, taking in the previous resolvers and returning new ones that will be merged. Using this approach, ens wouldn't need to hard-reference the resolvers from core, it would just assume they are there (given the ordering sequence, as core is applied before), and would wrap around them accordingly. The way I see it, ens resolvers resolve the address promise, and call the original resolver that comes in the prev argument.

raulk · 2018-10-06T13:08:16Z

src/ens/resolvers/index.ts

+}
+
+// tslint:disable-next-line
+const Address = new GraphQLScalarType({


We should separate the Address scalar type from the rest into different files, e.g. scalars.ts and resolvers.ts (look at how it's done in core).

raulk · 2018-10-06T13:09:56Z

src/ens/resolvers/index.ts

+  serialize: String,
+  parseValue: input => (Web3.utils.isAddress(input) || isEnsDomain(input) ? input : undefined),
+  parseLiteral: ast => {
+    if (ast.kind !== Kind.STRING || (!Web3.utils.isAddress(ast.value) && !isEnsDomain(ast.value))) {


We actually want to return a Promise<String> in all cases, whether it is an ENS domain or not. If it's an ETH address, we'd return Promise.resolve(ast.value), whereas if it's an ENS domain, we'd return the promise from the ENS lib.

Then the wrapper resolvers (see above) would resolve the promise before delegating onto the original resolver.

@raulk I'm not sure of how to implement it this way, as context/services are not accessible as they are when declaring a resolver.

You're right. I'm thinking the scalar parsers can return a 1-arg thunk, e.g.

type AddressThunk = (context: EthqlContext) => Promise<string>;

Then the resolver could do something like:

const addr = await params.address(context);

raulk · 2018-10-06T13:10:35Z

package.json

@@ -60,6 +60,7 @@
    "dataloader": "^1.4.0",
    "deepmerge": "^2.1.1",
    "express": "^4.16.3",
+    "ez-ens": "^1.0.4",


Curious as to why this choice of library?

compact + it works. Seemed like a good option.

StevenJNPearce · 2018-10-10T23:24:35Z

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

raulk · 2018-10-15T22:56:29Z

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

StevenJNPearce · 2018-10-17T00:34:04Z

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

raulk · 2018-10-18T11:04:43Z

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

StevenJNPearce · 2018-10-19T23:06:08Z

@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 #111 (comment) . 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 · 2018-10-24T11:51:26Z

@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.

raulk · 2018-10-24T11:57:44Z

src/ens/resolvers/index.ts

+import resolvers from './resolvers';
+import scalars from './scalars';
+
+export default [resolvers, scalars].reduce((prev, curr) => _.merge({}, curr, prev), {});


Take for example the account resolver which now needs to be wrapped in the ENS resolution logic.

Instead of returning a resolvers map here, we would return a function like the following:

export default (prev) => { const wrappedResolvers = { Query: { account: (obj, args, context: EthqlContext) => { args.address = await args.address(context); return prev.Query.account(obj, args, context); } }, // same for the other affected resolvers // Block: { transactionsInvolving, transactionsRoles }, }; return _.mergeDeep(prev, wrappedResolvers); }

@raulk looks like i forgot to comment back here after fixing this last week. You can check it now.

ens plugin ens plugin

StevenJNPearce · 2018-11-08T18:17:56Z

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

StevenJNPearce · 2018-11-08T20:40:49Z

@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 · 2018-11-12T10:26:26Z

@StevenJNPearce looking, thanks!

raulk · 2018-11-12T11:58:40Z

@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 4f6cd81.

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.

raulk reviewed Oct 6, 2018

View reviewed changes

raulk mentioned this pull request Oct 15, 2018

Lerna build #107

Closed

9 tasks

StevenJNPearce force-pushed the master branch from e84e331 to 369da6d Compare October 19, 2018 22:44

raulk reviewed Oct 24, 2018

View reviewed changes

ens plugin

88ac02e

ens plugin ens plugin

StevenJNPearce force-pushed the master branch from 1f568fe to 88ac02e Compare October 28, 2018 02:28

gitcoinbot mentioned this pull request Nov 7, 2018

Support resolving ENS names in queries (as a plugin) #106

Closed

7 tasks

Merge branch 'master' into master

25517ee

ens plugin

3003576

raulk and others added 3 commits November 12, 2018 10:27

Update README.md

70dacbd

refresh yarn.lock.

3c00d04

ens: fix regressions introduced in PR.

4f6cd81

raulk approved these changes Nov 12, 2018

View reviewed changes

raulk merged commit 9270831 into Consensys:master Nov 12, 2018

kshinn pushed a commit to kshinn/ethql that referenced this pull request Jan 10, 2019

ens plugin (Consensys#111)

99cdb3f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ens plugin #111

ens plugin #111

StevenJNPearce commented Oct 3, 2018 •

edited

Loading

raulk left a comment

raulk Oct 6, 2018

raulk Oct 6, 2018

raulk Oct 6, 2018

raulk Oct 6, 2018

StevenJNPearce Oct 6, 2018 •

edited

Loading

raulk Oct 6, 2018

raulk Oct 6, 2018

StevenJNPearce Oct 6, 2018

StevenJNPearce commented Oct 10, 2018

raulk commented Oct 15, 2018

StevenJNPearce commented Oct 17, 2018

raulk commented Oct 18, 2018

StevenJNPearce commented Oct 19, 2018

raulk commented Oct 24, 2018

raulk Oct 24, 2018

StevenJNPearce Nov 3, 2018

StevenJNPearce commented Nov 8, 2018

StevenJNPearce commented Nov 8, 2018

raulk commented Nov 12, 2018

raulk commented Nov 12, 2018

		@@ -0,0 +1,57 @@
		import { GraphQLScalarType, Kind } from 'graphql';

ens plugin #111

ens plugin #111

Conversation

StevenJNPearce commented Oct 3, 2018 • edited Loading

Requirements

Definition of done

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenJNPearce Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenJNPearce commented Oct 10, 2018

raulk commented Oct 15, 2018

StevenJNPearce commented Oct 17, 2018

raulk commented Oct 18, 2018

StevenJNPearce commented Oct 19, 2018

raulk commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenJNPearce commented Nov 8, 2018

StevenJNPearce commented Nov 8, 2018

raulk commented Nov 12, 2018

raulk commented Nov 12, 2018

StevenJNPearce commented Oct 3, 2018 •

edited

Loading

StevenJNPearce Oct 6, 2018 •

edited

Loading