Skip to content
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

Block query in GraphQL #7548

Merged
merged 3 commits into from
Jan 21, 2021
Merged

Block query in GraphQL #7548

merged 3 commits into from
Jan 21, 2021

Conversation

psteckler
Copy link
Member

Add a query block in GraphQL that takes a stateHash string, and returns a block with that state hash, using the same format as the blocks returned by bestChain.

Closes #7404. Well, maybe, it doesn't support querying by height. Should we provide that as a separate query? I think we'd need to linear search through all breadcrumbs to support that, or add additional indexing to support such a query.

@psteckler psteckler requested a review from a team as a code owner January 20, 2021 01:53
Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do want height as another argument in my opinion: Yes it's a linear search on the best-chain but the best-chain size is capped at k=290 and walking the 290 chain won't be expensive. (feel free to add it in a follow-up PR if you want though)

List.map best_chain ~f:(block_of_breadcrumb coda) )

let block =
field "block"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are multiple places where a failure can be introduced in the processing of this query -- can you make this a result_field instead and provide proper error messages for each of the places where we (currently) are binding over the option? Then a consumer can understand why it's failing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkase Do you mean a distinct query, like blockByHeight, or that height should be an additional optional argument?

Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! But yes please make stateHash nullabke and height another nullable parameter. And just error if both are null (this is a convention in graphql apis).

the height you can follow up with, but could you make the stateHash parameter nullable now so we don’t need to break compatibility when you add height.

@psteckler psteckler added ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR labels Jan 21, 2021
@bkase bkase merged commit f45097e into compatible Jan 21, 2021
@bkase bkase deleted the feature/block-in-tf-graphql branch January 21, 2021 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants