Skip to content

simulators/ethereum/graphql: change account queries to be on block, not account#426

Merged
holiman merged 2 commits intoethereum:masterfrom
renaynay:replace_accountQueries
Mar 12, 2021
Merged

simulators/ethereum/graphql: change account queries to be on block, not account#426
holiman merged 2 commits intoethereum:masterfrom
renaynay:replace_accountQueries

Conversation

@renaynay
Copy link
Copy Markdown
Contributor

This PR changes several testcases from querying on account to querying on block for account information. Queries on account are not required by the graphql spec. While Besu implements queries on account, geth does not as it is not required.

@renaynay renaynay force-pushed the replace_accountQueries branch from 99c44ec to 46b58ca Compare January 28, 2021 17:22
@fjl fjl changed the title simulators/ethereum/graphql/testcases: change account queries to be on block, not account simulators/ethereum/graphql: change account queries to be on block, not account Feb 1, 2021
@fjl
Copy link
Copy Markdown
Collaborator

fjl commented Feb 5, 2021

I don't fully understand which queries are unsupported. Do you mean account { ... } is not supported, but block { account { ... } } is supported?

The JSON-RPC compatibility table in EIP-1767 contains this row:

eth_getBalance | IMPLEMENTED | { account(address: "0x...") { balance } }

So I guess this query should be supported?

@renaynay
Copy link
Copy Markdown
Contributor Author

renaynay commented Feb 6, 2021

It was never required by the graphql spec though and we don't have it implemented. Should we implement support for queries on account then in geth?

@fjl
Copy link
Copy Markdown
Collaborator

fjl commented Feb 8, 2021

We talked about this in a call and decided couple things:

  • Basic change to add block { } around account query in tests is OK and besu guys also agreed with this change
  • Let's not add the multiple-status-code feature to tests. It will cause all tests to pass with all clients even though the clients disagree on basic things.
  • Account does not exist exception should be removed in besu

@renaynay
Copy link
Copy Markdown
Contributor Author

renaynay commented Mar 9, 2021

Account exception was removed in Besu and status code returned is now 200, after fixing merge conflicts and removing the option for multiple status codes, can we merge this?

@renaynay renaynay force-pushed the replace_accountQueries branch 2 times, most recently from 0b7e0c0 to d71de1a Compare March 9, 2021 12:08
@renaynay renaynay requested a review from fjl March 9, 2021 13:01
@holiman
Copy link
Copy Markdown
Contributor

holiman commented Mar 11, 2021

Oh, did I merge them in wrong order? Now there are conflicts (oops)

@renaynay
Copy link
Copy Markdown
Contributor Author

Yes, but it's okay :) I can fix them, thanks @holiman

@renaynay renaynay force-pushed the replace_accountQueries branch from 8689e2e to ccb1426 Compare March 11, 2021 16:26
@renaynay renaynay requested a review from holiman March 11, 2021 16:26
@holiman holiman merged commit fa21b1e into ethereum:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants