Add method with_default_block_announcer to ServiceBuilder #5797
Conversation
Forked at: a516cf2 Parent branch: origin/master
|
It looks like @cecton signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
|
@cecton I assume this relevant for cumulus? |
Yes! Sorry I forgot to mention that. I will update the PR description |
| pub fn with_block_announce_validator( | ||
| self, | ||
| block_announce_validator_builder: | ||
| impl FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send> + 'static, |
There was a problem hiding this comment.
I think this should actually return a Result. That's how it's done in all the others and I guess it's for the same reasons than sc-cli's SubstrateCli having a lot of Result everywhere too
There was a problem hiding this comment.
The type is pretty complex here, maybe traitify this and then implement the trait for every FnOnce() -> ... ?
There was a problem hiding this comment.
Also why isn't this just a generic type on ServiceBuidler level with static new function instead of a builder? Is there some dynamic state we expect to be transferred with the closure?
fn with_block_announce_validator<TBAV: BlockAnnounceValidator<TCl, TBl>>(self) -> ServiceBuilder<TBAV, TBl, ...> {
... // copy over the fields.
}
trait BlockAnnounceValidator<TCl, B> {
fn new(client: Arc<TCl>) -> Self;
}There was a problem hiding this comment.
No. There are 2 reasons for that:
-
In my use case for cumulus (see issue in description) we want to use a custom validator JustifiedBlockAnnounceValidator and as you can see the arguments for
new()is actually something custom. I assume this can be personalized by the parachain as they want -
On substrate you can clearly see that this has been done with this intention because the function
newis not in the trait.
There was a problem hiding this comment.
To stay consistent with the existing API, I'd keep it this way for now.
There was a problem hiding this comment.
I thought that "staying consistent with the existing API" meant changing it to return a Result
There was a problem hiding this comment.
(Changing the closure to return a Result? Yes that's what I meant)
| ) -> Result<ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, | ||
| TExPool, TRpc, Backend>, Error> | ||
| where TSc: Clone, TFchr: Clone { | ||
| Ok(ServiceBuilder { |
There was a problem hiding this comment.
...and the Result here makes no sense if the closure in input is not called right away. @gnunicorn what do you think I should do?
There was a problem hiding this comment.
I think for consistency with the current implementation of the servicebuilder it is best to call the closure directly and use a Result in the closure.
There was a problem hiding this comment.
Yeah, this isn't pretty, but okay for now. And as we want to move to deferring calls (see #4587 ) we need to refactor this things alltogether soon anyways.
| pub fn with_block_announce_validator( | ||
| self, | ||
| block_announce_validator_builder: | ||
| impl FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send> + 'static, |
There was a problem hiding this comment.
The type is pretty complex here, maybe traitify this and then implement the trait for every FnOnce() -> ... ?
| pub fn with_block_announce_validator( | ||
| self, | ||
| block_announce_validator_builder: | ||
| impl FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send> + 'static, |
There was a problem hiding this comment.
Also why isn't this just a generic type on ServiceBuidler level with static new function instead of a builder? Is there some dynamic state we expect to be transferred with the closure?
fn with_block_announce_validator<TBAV: BlockAnnounceValidator<TCl, TBl>>(self) -> ServiceBuilder<TBAV, TBl, ...> {
... // copy over the fields.
}
trait BlockAnnounceValidator<TCl, B> {
fn new(client: Arc<TCl>) -> Self;
}
| ) -> Result<ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, | ||
| TExPool, TRpc, Backend>, Error> | ||
| where TSc: Clone, TFchr: Clone { | ||
| Ok(ServiceBuilder { |
There was a problem hiding this comment.
Yeah, this isn't pretty, but okay for now. And as we want to move to deferring calls (see #4587 ) we need to refactor this things alltogether soon anyways.
| pub fn with_block_announce_validator( | ||
| self, | ||
| block_announce_validator_builder: | ||
| impl FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send> + 'static, |
There was a problem hiding this comment.
To stay consistent with the existing API, I'd keep it this way for now.
|
bot merge |
ghost
merged commit
haha 😝 me neither. I wanted to change it but I had other things to do... 😞 |
Repeating it every time is probably the most efficient way to eventually have it changed. |
This is a small change that will allow changing the default block announcer when building a service.
This PR is relevant for the issue: paritytech/cumulus#7 as we need to set a custom default block announcer.