BlockInterval support for hash ranges #728
Conversation
github-actions
bot
added
crate: zero_bin
sergerad
force-pushed
the
serge/block-hash-interval
branch
from
b5ef272
to
7a75048
Compare
|
@atanmarko I have reimplemented the existing unit tests with mocks and added new tests for hash-based inputs. The functionality is all in place, just need to clean things up. But I'll hold off for now until you get a chance to do a review pass. Thanks! |
sergerad
changed the title
zero/src/block_interval.rs
Outdated
| /// end_block is always treated as inclusive because it may have been | ||
| /// specified as a block hash. | ||
| pub async fn new( | ||
| provider: Arc<impl ZeroBlockProvider>, |
There was a problem hiding this comment.
smell: doesn't feel right to need an Arc here
There was a problem hiding this comment.
No smell here. We use one provider instance in the whole program, we pass it from the top level using Arc.
There was a problem hiding this comment.
I think this is a case of needless pass by value
- ZeroBlockProvider only has
&selfreceivers - So the function only needs
[&] impl ZeroBlockProvider(note thatalloy_provider::Provideralready does theimpl<T: Provider> Provider for &T { .. }
The function should require as little from its callers as possible
There was a problem hiding this comment.
@0xaatif In the leader we have couple of threads using provider. We only instantiate one instance at the start of the program and pass it from the top and multiple async tasks use this provider in parallel. I agree that in this particular case (in particular where the block interval is created in the leader) pass by value (or with Arc smart pointer) is not needed, but it was easier and cleaner to by convention maintain the same Provider argument through the whole application than to create Provider, pass it by reference and then wrap it in Arc to pass it further to other tasks. Cloning the Arc is just increasing the reference counter so it is not really an overhead.
There was a problem hiding this comment.
P.S. Earlier, Provider was indeed created and handled as an common reference. But during refactors it often happened that functions were called from other tasks, so the only way they could get the provider is through Arc pointer. Hence the Arc convention was born.
| /// assert_eq!(BlockInterval::new("32141").unwrap(), BlockInterval::SingleBlockId(BlockId::Number(32141.into()))); | ||
| /// assert_eq!(BlockInterval::new("100..").unwrap(), BlockInterval::FollowFrom{start_block: 100}); | ||
| /// ``` | ||
| pub fn new(s: &str) -> anyhow::Result<BlockInterval> { |
There was a problem hiding this comment.
we should close #284 if we're dropping all the parsing :)
zero/src/block_interval.rs
Outdated
| use crate::parsing; | ||
| use crate::provider::CachedProvider; | ||
| #[cfg_attr(test, automock)] | ||
| pub trait BlockIntervalProvider { |
There was a problem hiding this comment.
@0xaatif note that auto_impl(&, Box) caused conflicting implementations to impl<T: Provider> BlockIntervalProvider for T { so that its why its not here. My understanding is that the impl block below would cover refs and Boxes. LMK if I'm mistaken
zero/src/block_interval.rs
Outdated
| fn latest_block_number(&self) -> impl Future<Output = anyhow::Result<u64>> + Send; | ||
| } | ||
|
|
||
| impl<T: Transport + Clone, P: Provider<T>> BlockIntervalProvider<T> for P { |
There was a problem hiding this comment.
@0xaatif apologies the approach was actually more involved than I had realized - the previous commit was not compiling, my IDE must have been seriously lagging for me to not notice before pushing up.
This does appear to work for our purposes. The problem is that it causes a conflict with the mocked type. I don't know how to get around that conflict yet unfortunately. LMK if you have any ideas.
There was a problem hiding this comment.
Pushed another commit that manages the conflict
sergerad
force-pushed
the
serge/block-hash-interval
branch
from
e1852c8
to
c47e2a6
Compare
sergerad
force-pushed
the
serge/block-hash-interval
branch
from
00061a3
to
99dbbc2
Compare
Closes #287.
Hash-based Block Intervals
Update scripts, CLI, and leader stack to allow block ranges to be specified by block hashes, not just block numbers.
Changes
Refactored
BlockInterval::new()to allow for the simplest support for block hash ranges in the runtime and associated scripts.Changed
BlockIntervalto only containu64ranges (invariant of valid numerical range).Update CLI to provide flags:
--start-block [num|hash]and--end-block [num|hash]