Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement
BlocksClient
for working with blocks #671Implement
BlocksClient
for working with blocks #671Changes from all commits
738cd82
453ca83
8e919f7
4985682
65a2960
bc8ec5c
e1892c7
be1a20c
2250701
a86237a
15b0478
af98f7f
cea372e
64dfd79
2b5066b
c069473
1a70b4a
dc9a435
bedc6a4
75bc024
bf5863f
ae7f5ff
afcf759
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just thinking about that other PR; does this return all new blocks or all new "best" blocks?
Might be worth clarifying this in the docs either way?
I wonder whether if we only subscribe to all new best blocks, we might miss some blocks?
But I also guess that the block number might be the same for more than one block we get back via this call, if the "best" chain changes (or if we are asking for all blocks anyway).
So maybe w can't fill things in, but we can document precisely what is returned either way :)
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 does subscribe to all new "best" blocks indeed.
It's a good idea to document this properly.
Yep, I think we can end up in a situation where we have two blocks with the same block number. In this case, we can't guarantee that we can fill in missing blocks.
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.
Yup, for that reason I don't think we should even try; as long as we document what will be returned it's all good.
I do wonder whether we should be subscribing to all new blocks too. Or whether just to add both "subscribe_best_headers" and "subscribe_all_headers" to go with "subscribe_finalized_headers". But as you said inthe other PR, let's just do what the new RPC API is going to support for now (I can't remember!)
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.
The new API will support all events: blocks, best blocks, and finalized blocks.
We don't support the simple block events (non best and non finalized) in subxt yet.
We can easily extend this to add all types of events, tho I think we can wait for a bit and implement it on top of the substrate's new API once it's released.
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.
As long as the API and naming here means that we can migrate easily enough and add whatever else we need, I'm happy with it! And the current choice of function is what we use in events, so it makes sense to me how it is!
Let's just make sure to clearly document it :)