-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
GraphQL API #17903
GraphQL API #17903
Conversation
…a is reported properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nitpicks, otherwise I think this will make a great addition!
common/hexutil/json.go
Outdated
case int32: | ||
*b = Uint64(input) | ||
default: | ||
err = fmt.Errorf("Unexpected type for BigInt: %v", input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = fmt.Errorf("Unexpected type for BigInt: %v", input) | |
err = fmt.Errorf("Unexpected type for Uint64: %v", input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or Long
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it should be Long
. Fixed.
Co-Authored-By: Arachnid <arachnid@notdot.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about a Pagination model in the context of EthQL. The complete connection model approach has one too many levels of depth for my taste. And we don't need to interoperate with Relay. But it serves as an inspiration.
Paged and non-paged query scenarios can co-exist, as both are desirable depending on the situation. User-driven introspective queries, analytics, diagnostics, etc. will prefer one-shot queries. Whereas Dapps, scripts, explorers, etc. should use pagination. We rely on the developer/user to decide responsibly (and infrastructure providers could block non-paged queries).
The fields that would benefit from pagination are:
- Query->blocks
- Query->logs
- Block->transactions
- Block->logs
Opaque cursors are useful. It allows clients like Geth to encode the tracking data in the cursor itself (e.g. base64 json/protobuf) and thus avoid storing server-side state.
Schema-wise, it could look like this:
input Selector {
offset: Int
first: Int
after: String
}
input PageInfo {
cursor: String
more: Boolean
}
type PagedBlocks {
data: [Block!]!
pageInfo: PageInfo!
}
type PagedLogs {
data: [Log!]!
pageInfo: PageInfo!
}
type NonPagedQueries {
blocks(from: Long!, to: Long): [Block!]!
logs(filter: FilterCriteria!): [Log!]!
}
type Query {
blocks(from: Long!, to: Long, selector: Selector): PagedBlocks!
logs(filter: FilterCriteria!, selector: Selector): PagedLogs!
"Namespace for non-paged queries"
nonPaged: NonPagedQueries!
}
Just some initial thoughts.
ethgraphql/schema.go
Outdated
ommerHash: Bytes32! | ||
# Transactions is a list of transactions associated with this block. If | ||
# transactions are unavailable for this block, this field will be null. | ||
transactions: [Transaction!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for filtering transactions in different forms is something that EthQL users appreciate. Currently we have a split brain, where some filters take the form of fields (e.g. transactionsInvolving
, transactionsRoles
) and others are in an input type:
"""
A filter for transactions. Setting multiple criteria is equivalent to combining them with an AND operator.
"""
input TransactionFilter {
"Only selects transactions that emit logs."
withLogs: Boolean
"Only selects transactions that received input data."
withInput: Boolean
"Only selects transactions that created a contract."
contractCreation: Boolean
}
Conflating all filters into an input type that can be passed as an arg to the transactions
field would be welcome, e.g. (some docs omitted for brevity; a catch-all type that can be structured for better semantics; simple on purpose):
input TransactionFilter {
"Accounts from which a transaction was sent."
from: [Address]
"Accounts to which the transaction was sent."
to: [Address]
"Accounts that can appear in any side of the transaction."
actor: [Address]
withLogs: Boolean
withInput: Boolean
contractCreation: Boolean
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'd like to leave that to a second PR as well, in order to simplify matters.
@raulk Regarding pagination, I don't think it's necessary for blocks -> transactions, blocks -> logs, or query -> blocks - all of these are either self-limited by gas limit or by the user parameters (in the case of query -> blocks). I agree we should implement pagination for querying logs - I left it out of this PR because it would involve a deeper change in Geth's log filtering internals, and wanted to give that its own PR. |
cmd/utils/flags.go
Outdated
cfg.GraphQLHost = ctx.GlobalString(GraphQLListenAddrFlag.Name) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this empty line, make code more compact and doesn't really provide any value.
Name: "graphqlport", | ||
Usage: "GraphQL server listening port", | ||
Value: node.DefaultGraphQLPort, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been working on reformatting the flags a bit for a while now, making them more namespace-y. E.g.
--mine Enable mining
--miner.threads value Number of CPU threads to use for mining (default: 0)
--miner.notify value Comma separated HTTP URL list to notify of new work packages
--miner.gasprice "1000000000" Minimum gas price for mining a transaction
--miner.gastarget value Target gas floor for mined blocks (default: 8000000)
Please use the same format for any new flag: graphql.addr
, graphql.port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, instead of piggiebacking on RPCCors and RPCVhosts, please define a separate set. Since we're talking about two different TCP endpoints, it's imho necessary to be able to configure them separately.
@@ -72,6 +72,23 @@ func (b Bytes) String() string { | |||
return Encode(b) | |||
} | |||
|
|||
func (b Bytes) ImplementsGraphQLType(name string) bool { return name == "Bytes" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public method. Please add a method doc for it.
@@ -72,6 +72,23 @@ func (b Bytes) String() string { | |||
return Encode(b) | |||
} | |||
|
|||
func (b Bytes) ImplementsGraphQLType(name string) bool { return name == "Bytes" } | |||
|
|||
func (b *Bytes) UnmarshalGraphQL(input interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public method. Please add a method doc for it.
@@ -187,6 +204,23 @@ func (b *Big) String() string { | |||
return EncodeBig(b.ToInt()) | |||
} | |||
|
|||
func (b Big) ImplementsGraphQLType(name string) bool { return name == "BigInt" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public method. Please add a method doc for it.
ethgraphql/graphiql.go
Outdated
@@ -0,0 +1,73 @@ | |||
package ethgraphql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the copyright preamble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in fact copied from elsewhere, MIT licensed. I've added the MIT license preamble, which I believe is compatible with the GPL.
ethgraphql/main.go
Outdated
// You should have received a copy of the GNU Lesser General Public License | ||
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
package ethgraphql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being the main file, please add a package doc. I'd also prefer to call this file something other than main
. Perhaps simply graphql
?
ethgraphql/schema.go
Outdated
@@ -0,0 +1,289 @@ | |||
package ethgraphql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the copyright preamble.
ethgraphql/main_test.go
Outdated
@@ -0,0 +1,13 @@ | |||
package ethgraphql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the copyright preamble. Please rename the file to graphql_test.go
.
cmd/geth/config.go
Outdated
@@ -30,6 +30,7 @@ import ( | |||
"github.com/ethereum/go-ethereum/cmd/utils" | |||
"github.com/ethereum/go-ethereum/dashboard" | |||
"github.com/ethereum/go-ethereum/eth" | |||
"github.com/ethereum/go-ethereum/ethgraphql" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name the package simply graphql
? I mean it's not like Geth is ever going to have some other graphql thing added to it any time soon.
@karalabe PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few extra stylistic comments and your branch PR also needs to be rebased. Otherwise, looks good to me!
receipts []*types.Receipt | ||
} | ||
|
||
// resolve returns the internal Block object represennting this block, fetching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: representing
Closing this PR and using #18445 instead |
I've written up a draft EIP for standardising this functionality here: https://github.com/Arachnid/EIPs/blob/graphql/EIPS/eip-draft-graphql.md |
@Arachnid is the repo for your draft EIP private? The link seems broken. |
@timbeiko Sorry; it's since been merged as a draft, and can be found at https://eips.ethereum.org/EIPS/eip-1767 |
This PR defines a GraphQL API that implements the bulk of the functionality of the
eth_
namespace in the JSON-RPC API, but with more friendly semantics and more efficient execution. In a test on my laptop, fetching all receipts for a recent block took 500 milliseconds with 32 threads with the existing JSON-RPC based API, but only 18ms with this GraphQL implementation.I'm hoping to define this graphql schema as an ERC standard and get it adopted across a broad range of clients as a new standard API. This PR is intended to be a PoC and eventually a reference implementation.
After patching this PR in, you can try it out by running geth with the
--graphql
flag, and visiting localhost:8547 in your browser, which will give you an interactive GraphiQL query explorer, with inline help. For instance, try this query to get information on the latest synced block:The following table maps existing RPC calls under the
eth_
namespace to their GraphQL equivalents:{ block { number } }
{ call(data: { to: "0x...", data: "0x..." }) { data status gasUsed } }
{ estimateGas(data: { to: "0x...", data: "0x..." }) }
{ gasPrice }
{ account(address: "0x...") { balance } }
{ block(hash: "0x...") { ... } }
{ block(number: 123) { ... } }
{ block(hash: "0x...") { transactionCount } }
{ block(number: x) { transactionCounnt } }
{ account(address: "0x...") { code } }
{ logs(filter: { ... }) { ... } }
or{ block(...) { logs(filter: { ... }) { ... } } }
{ account(address: "0x...") { storage(slot: "0x...") } }
{ block(hash: "0x...") { transactionAt(index: x) { ... } } }
{ block(number: n) { transactionAt(index: x) { ... } } }
{ transaction(hash: "0x...") { ... } }
{ account(address: "0x...") { transactionCount } }
{ transaction(hash: "0x...") { ... } }
{ block(hash: "0x...") { ommerAt(index: x) { ... } } }
{ block(number: n) { ommerAt(index: x) { ... } } }
{ block(hash: "0x...") { ommerCount } }
{ block(number: x) { ommerCount } }
{ protocolVersion }
mutation { sendRawTransaction(data: data) }
{ syncing { ... } }