Service factory refactor, part 1 #3384
Conversation
In follow-up commits, we want to be able to directly call maintain_transaction_pool, offchain_workers, and start_rpc, without having to implement the Components trait. This commit is a preliminary step: we extract the code to freestanding functions.
Instead of implementing AbstractService, Future, and Executor on Service, we implement them on NewService instead. The implementations of AbstractService, Future, and Executor on Service still exist, but they just wrap to the respective implementations for NewService.
Instead of having multiple $build_ parameters passed to the macro, let's group them all into one. This change is necessary for the follow-up commits, because we are going to call new_impl! only after all the components have already been built.
This makes it possible to be explicit as what the generic parameter of the NewServiceis, without relying on type inference.
Introduces a new builder-like ServiceBuilder struct that creates a NewService.
Similar to the introduction of new_impl!, we extract the actual code into a macro, letting us get rid of the Components and Factory traits
…uilder Can be used as a replacement for the chain_ops::* methods
Instead of just run, adds run_with_builder to ParseAndPrepareExport/Import/Revert. This lets you run these operations with a ServiceBuilder instead of a ServiceFactory.
This is technically a breaking change, but the transaction-factory crate is only ever used from within substrate-node, which this commit updates as well.
We slightly change the trait bounds in order to make all the methods usable.
|
Any good reason to turn |
I did that in order to share the same code between the old and new APIs, and to make solving conflicts while rebasing easier. It's intended to be temporary, and switched back to regular functions afterwards. (to be honest I would have gone a different route if I had had the guarantee to not encounter any conflict) |
|
By conflict you mean merge conflict? Or something in polkadot? |
Yes, merge conflicts. |
|
There's a lot of dirt remaining after the refactor (#3382), and where to stop adding commits and when to submit the PR is a bit arbitrary. For example there are lots of items to move around, we can remove the |
|
I still don't understand why we not merge everything at once? If you are afraid of porting the stuff to polkadot, why not start porting directly while we review the new interfaces? And when we merge the other one, you can directly open a pr in polkadot? |
I actually tried doing that, but Polkadot never seems to be up to date with Substrate master, and I don't really want to fix the breaking changes in the SRML stuff myself. But to clarify, the idea is not to merge one then wait for the other. The idea is to merge one then immediately the other, so that they are squashed in two different commits. This way, in case of an urgency, we can revert one and not the other. Alternatively, in case of urgency, we could also cherry-pick the urgent commits on a Substrate branch that doesn't have this refactoring. |
|
I looked a bit more at Polkadot's code, and I think it should be fine be port. What I was worried about are the fact that I removed Feel free to merge only the entire refactor if you prefer. |
|
Ty. |
3 participants
As described in #3382, this is the first part that doesn't break anything and that we should merge before #3382.