Return events from blocks skipped over during Finalization, too #473
Conversation
|
@paulormart I wonder whether you could give this change a go; I haven't given it much of a workout yet but it'd be great to get some initial impression of whether it seems to be doing the right thing :) |
subxt/src/rpc.rs
Outdated
| #[doc(hidden)] | ||
| pub async fn block_hash_internal( | ||
| &self, | ||
| block_number: T::BlockNumber, | ||
| ) -> Result<Option<T::Hash>, BasicError> { | ||
| let block_number = ListOrValue::Value(block_number); | ||
| let params = rpc_params![block_number]; | ||
| let list_or_value = self.client.request("chain_getBlockHash", params).await?; | ||
| match list_or_value { | ||
| ListOrValue::Value(hash) => Ok(hash), | ||
| ListOrValue::List(_) => Err("Expected a Value, got a List".into()), | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not a fan of duplicating this block_hash function. The problem is that Config::BlockNumber isn't something that we can easily work with.
I could also require that Config::BlockNumberimplements something like Into<u128>, or I could remove the "other" copy of this function, but it sounds like some consideration went into it, so I was hesitant. What do you think @ascjones?
There was a problem hiding this comment.
The nice thing abour requiring Into<u128> or similar would be that I can get rid of num_traits again, and the other required traits I added to BlockNumber, which I was just using to add a minimal amount of information about the BlockNumber so that I could iterate over them.
Is it reasonable to assume that nobody would use something more esoteric than a rust unsigned int to represent block numbers, and so Into<u128> shouldn't cause any issues?
There was a problem hiding this comment.
I think it is reasonable to use Into<u128>
There was a problem hiding this comment.
I went for Into<u64> in the end because u128's don't serialize to JSON so easily; I'm hoping u64 is also a safe target here (I assume it's basically a choice between u32 and u64 that people would tend to make?)
There was a problem hiding this comment.
Into<u64> should also be fine.
|
Awesome stuff @jsdw. I have just run my initial test against this branch and there is no skipped blocks. Seems to work fine, thank you. |
|
Fantastic; thank you for doing that! |
| #[doc(hidden)] | ||
| pub fn subscribe_to_block_headers_filling_in_gaps<'a, S, E, T: Config>( | ||
| client: &'a Client<T>, | ||
| mut last_block_num: Option<u64>, |
There was a problem hiding this comment.
Is it ok to assume that the block number is u64 here? Or would Option<Into<u64>> work?
There was a problem hiding this comment.
since block numbers can always be converted into u64s now, it's just easier to do the conversion first and then pass in the u64's here (this function isn't "public" anyway).
If you subscribe to receive finalized block headers, blocks may be skipped over whenever a bunch of them are finalized at the same time. This PR makes sure to return events from all of the skipped-over blocks too.
Closes #468