Consensys / ethql

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

RFC: Initial attempt at supporting contract introspection for ERC20 #85

Closed pcardune closed 4 years ago

pcardune commented 6 years ago

I imagine you all have thought about how to do this more than me, so hopefully this code is on the right track. The basic idea is that you want clients to be able to manually specify how a particular account address should be interpreted. So when a contract implements multiple standards, the ethql server doesn't need to guess which one to use.

If this looks good, the next step would be to implement a way to customize ethql with additional ABI's/contract types. One idea that would be pretty cool is to make an ABI -> graphql schema converter, so you could just hand ethql a directory of ABI files and it would automatically start up with a schema that supported all of those ABIs.

raulk commented 6 years ago

@pcardune – I was in DappCon last week presenting EthQL and on holiday this week. I want to make sure I study the proposal carefully. Does it impact you guys if we put this on hold until next Monday?

pcardune commented 6 years ago

not at all. Take your time.

pcardune commented 6 years ago

It would be nice for this to get looked at eventually though :)

raulk commented 6 years ago

@pcardune Sorry, my bad! I'm working introducing the notion of capabilities and plugins which will bring in a new design that makes it easier to introduce top-level queries, such as a hypothetical:

{
  tokenContract(address: "0x1234...") {
    ...
  }
}

Using a single entrypoint (Account->contract field) to dive into any kind of contract supported by the decoding engine is powerful. However, the standard string parameter makes me a bit nervous, because the developer does not know what values are supported by the EthQL deployment they're hitting.

We could consider an enum instead of a string, but because GraphQL doesn't support extend enum in the schema, all possible values would have to be inlined in the definition, which does not play well with how I see plugins working.

Note that I use the term EthQL deployment because soon each user of EthQL will be able to instantiate EthQLServer with the set of plugins they want (ERC20, ERC777, ERC821, ENS, ERC165...), including custom plugins for their own contracts (uPort, Nori, Gitcoin, etc.)

Also there is repetition between the standard and the type assertion (ERC20).

I'm trending towards the approach briefly explained here: #97, where an ethql-plugin-erc20 module would contain:

The subschema could contain the following extend, which adds a field to the root query. Like this, we avoid using polymorphism, and we have the benefit of an introspectable and fully typed schema.

extend type Query {
  tokenContract(address: Address!): TokenContract
}

I've introduced the first batch of changes to support modularisation in one of my last commits (ca6aedd7a9aa71b7f6c5af834f798f810ee32f27) – this one separates the model from the resolvers. More changes coming in the next days.

WDYT?

raulk commented 6 years ago

@pcardune – just to provide a follow-up. This is how the plugin data structure is looking like in my current tree:

export interface EthqlPlugin {
  name: string;
  priority: number;
  schema?: string[];
  resolvers?: IResolvers<any, any>;
  services?: Partial<EthqlServiceRegistry>;
  dependsOn?: {
    services: string[];
  };
  order?: {
    before?: string[];
    after?: string[];
  };
  init?: (result: EthqlBootstrapResult) => void;
}

This will allow the ERC20 plugin to add top-level queries such as tokenContract that don't require polymorphism and do not break modularity.

jaycenhorton commented 6 years ago

just an FYI @pcardune is offline for the next couple weeks :smile:

I can't speak entirely on his behalf, but the plugins sound scalable :+1:

kshinn commented 5 years ago

@pcardune @jaycenhorton It's been a while since this thread was updated. So many changes in the repo have occurred since and what from what I understand support for what this PR was proofing has been added. Is there anything missing that should be considered? or can this PR be closed?