-
Notifications
You must be signed in to change notification settings - Fork 242
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
#671
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
BlocksClient
for working with blocks
Co-authored-by: James Wilson <james@jsdw.me>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Looks awesome, good job! Just a couple of minor nits :) |
pub fn subscribe_headers( | ||
&self, | ||
) -> impl Future<Output = Result<impl Stream<Item = Result<T::Header, Error>>, Error>> | ||
+ Send | ||
+ 'static { | ||
let client = self.client.clone(); | ||
async move { client.rpc().subscribe_blocks().await } | ||
} |
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.
Just thinking about that other PR; does this return all new blocks or all new "best" blocks?
This does subscribe to all new "best" blocks indeed.
Might be worth clarifying this in the docs either way?
It's a good idea to document this properly.
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).
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.
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.
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 :)
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
subxt/src/blocks/blocks_client.rs
Outdated
E: Into<Error> + Send + 'static, | ||
{ | ||
sub.flat_map(move |s| { | ||
let rpc = client.clone(); |
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 don't understand why this is clone is needed, but I guess it the stream::iter
below, can't you remove the "move stuff" there to avoid this clone?
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 don't recall off the top of my head why there is a clone here. My guess is removing the move stuff would introduce a lifetime on client
which wouldn't work out since we want to return that thing. Perhaps we can avoid a little cloning somewhere though?
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.
Couldn't remove the clone from here, but removed one clone from above client.rpc().clone()
when calling this function. The clone was circumvented by passing in a Client
trait directly and then using the .rpc()
inside the fn.
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
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
The filling in of missing finalized blocks can be applied to other use cases than that of finding events.
Move the core functionality of filling to the RPC client to make it more generic.
Blocked by #673.
Closes #630.