Split polkadot parachains service file into multiple modules #4369

clangenb · 2024-05-03T12:32:28Z

I have been maintaining multiple parachains for the past years, and in every team, we try to stay close to upstream for the collator implementation.

Hence, staying up-to-date with upstream when updating polkadot versions was usually done by comparing the individual files. Comparing the service file to upstream was always a pain because it contains a lot of unnecessary code for parachain teams, i.e. implementations for irrelevant nodes like glutton, contracts_rococo etc. However, at the same time there are sometimes minor adjustments necessary for the parachain teams, e.g. RPC extensions and custom types. Having more than 1000 lines of code doesn't make it easier. I know that there is the parachains template, but it is outdated too often, and I don't have enough trust in it to be honest.

This PR aims to remove friction for parachain teams trying to stay up-to-date with the polkadot-sdk by:

Separating the service file into modules, such that it is easier to discern parachain relevant code from irrelevant code
The file separation has been done in a way such that in most cases one can just accept the upstream changes as there are usually no customizations downstream.
Do some re-export proxying of the parachain_commons types such that down_stream implementors don't have to replace the parachain_commons import in every file. They can just update the re-export at a single place.

I thought it will take me almost as long to formulate an issue with my suggestions as already implementing it, which is why I made this draft here now. So I am happy to receive feedback and re-adjust things. Spoileralert: I am thinking about a similar change for the commands.rs

NOTE: It is hard to spot the diff as the code has been put into new files. There was absolutely no change in the functions except for some public modifiers. The service module API stayed the same except for the type exports, which reside now in the new common_types module.

Polkadot address: 16YCL3UVpVWQLGW3p3Zx4k5WAEp9W1DwdDnxAbyAaPxVxnp3

clangenb · 2024-05-03T12:33:14Z

cumulus/polkadot-parachain/src/rpc.rs

-use parachains_common::{AccountId, Balance, Block, Nonce};
+use crate::service::common_types::{AccountId, Balance, Block, Nonce};


Hmm, this should maybe be at the crate root now that I look at it.

clangenb · 2024-05-03T12:39:04Z

cumulus/polkadot-parachain/src/service/mod.rs

+/// Module with common types.
+///
+/// The idea is to help downstream implementors who don't rely on `parachains_common` for types.
+/// Proxying the imports paths via this module makes it possible to only adjust imports here, as
+/// opposed to replacing `parachains_common` imports in every file.
+pub mod common_types {
+	pub use parachains_common::{AuraId, AccountId, Balance, Block, Hash, Nonce, Header};
+}
+
+use common_types::Block;


This is one of the simple but efficient core changes to help parachain teams.

kianenigma · 2024-05-21T14:13:53Z

I really this PR! this is very similar to the refactors that I also see necessary in #3597

That being said, I see some differences. We should break down a fully featured service.rs file into more individual components such as BuildConsensus and BuildRpc. I have used traits to standardize in the meantime how these are done although these traits are pretty ugly at the moment.

We should make sure these components are reused in polkadot-parachain and parachain-template. At the moment, almost 100% of parachain-template is copy-pasta from polkadot-parachain. It is in essence a subset of it.

Finally, we can then use these reusable components to build a nice configurable omni-node :)

@serban300 also wants to work on this. @clangenb what is your intentions? would you like to congtinue contributing?

clangenb · 2024-05-21T15:23:11Z

I am happy to have someone else building upon my work. However, I don't understand the following:

That being said, I see some differences. We should break down a fully featured service.rs file into more individual components such as BuildConsensus and BuildRpc. I have used traits to standardize in the meantime how these are done although these traits are pretty ugly at the moment.

Isn't this exactly what I have been doing? So maybe with some detailed review this PR is mergeable?

I absolutely agree that we should actually re-use these components in the template, it is the only way that the parachain-template will not be out of sync anymore.

If the work will only consist of incorporating a PR review and using these components in the template, I am happy to continue. If the expectations are higher, I will gladly pass the torch.

skunert

I agree with @kianenigma , this is a nice step.

skunert · 2024-05-21T15:00:11Z

cumulus/polkadot-parachain/src/service/rpc_extensions.rs

+		+ pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>
+		+ frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>,
+{
+	let deps = crate::rpc::FullDeps { client, pool, deny_unsafe };


IMO we could further simplify this. The FullDeps struct only has three members and this method here does nothing except create the struct and forward it to the rpc module.

My suggestion is to remove FullDeps alltogether and just pass the arguments to the rpc module methods. Then delete this file.

Yes, I agree. I just thought that this PR should maybe consist of file-splitting only, so that we have a better understandable diff when we want to change the code afterward. But I agree that this would be kind of a minor change.

skunert · 2024-05-21T16:43:54Z

cumulus/polkadot-parachain/src/service/mod.rs

+/// The idea is to help downstream implementors who don't rely on `parachains_common` for types.
+/// Proxying the imports paths via this module makes it possible to only adjust imports here, as
+/// opposed to replacing `parachains_common` imports in every file.
+pub mod common_types {


I don't think this makes sense. polkadot-parachain crate is just code that is necessary to build the polkadot-parachain binary. It has no external facing API, so there are no downstream implementors using this. We should continue using parachains_common here.

Maybe I badly formulated here. The reasoning here was that if a parachain team updates the polkadot version, and they want to integrate the changes by comparing the source files, they need to change the files in more places instead of just here.

Moonbeam for example would have to change the parachains_common::AccountId import in about four places now if I remember correctly, but I don't really like this re-export thing either, but I think it is better than the status quo.

Polkadot-Forum · 2024-05-23T08:25:29Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/2024-05-21-technical-fellowship-opendev-call/8264/1

serban300 · 2024-05-23T11:25:02Z

I am happy to have someone else building upon my work.

If the work will only consist of incorporating a PR review and using these components in the template, I am happy to continue. If the expectations are higher, I will gladly pass the torch.

Awesome and thanks for the contribution so far ! I would like to also take a look on the PR probably next week and let's discuss the way forward after that.

kianenigma · 2024-05-28T07:19:50Z

That being said, I see some differences. We should break down a fully featured service.rs file into more individual components such as BuildConsensus and BuildRpc. I have used traits to standardize in the meantime how these are done although these traits are pretty ugly at the moment.

Isn't this exactly what I have been doing? So maybe with some detailed review this PR is mergeable?

The main reason I said that, and the only disagreement I have with this PR is that these reusable-components should live outside of polkadot-parachain. I proposed cumulus/service, it could also be in a new omni-node/service or omni-node/common as in #2457.

clangenb · 2024-05-28T07:25:20Z

That being said, I see some differences. We should break down a fully featured service.rs file into more individual components such as BuildConsensus and BuildRpc. I have used traits to standardize in the meantime how these are done although these traits are pretty ugly at the moment.

Isn't this exactly what I have been doing? So maybe with some detailed review this PR is mergeable?

The main reason I said that, and the only disagreement I have with this PR is that these reusable-components should live outside of polkadot-parachain. I proposed cumulus/service, it could also be in a new omni-node/service or omni-node/common as in #2457.

ohhhh, got it! This makes absolute sense. I would probably go for an omni-node/service, but I will await @serban300's review before I continue doing anything.

serban300 · 2024-05-31T11:02:34Z

I looked on the PR. Thank you again for the contribution !

For more context, our intention is to make the entire node code more reusable and the end goal is indeed to remove friction for the parachain teams trying to stay up-to-date with the polkadot-sdk. There are some details here: #5 . Probably one of the steps would be to split the service.rs into multiple files, but we will also have to do bigger refactorings, for example:

move common logic to a separate crate
move some logic to reusable components
add some traits maybe
probably have some builder

The entire project is quite big and ambiguous, and there are some unknowns at this point.

The idea of splitting the service.rs file is good and aligned with our vision, but the problem is that at this point I'm not sure how the split should look like exactly, and I would leave it as a future step after some refactoring. Probably at some point, a particular split will become natural. So I would like to continue the work from here, do some refactoring and we can sync at that point about the split if that's ok with you.

Spoileralert: I am thinking about a similar change for the commands.rs

Please let's sync again at a later time about this as well since we'll probably touch the command.rs file in our refactorings.

kianenigma · 2024-05-31T13:49:31Z

@clangenb given the outcome above, please provide your DOT address in the PR description and I will submit an onchain DOT tip for your contributions. I do agree that this particular refactor might be best done by a full time contributor. Nonetheless, the exploration you have done here is super valuable.

clangenb · 2024-06-04T14:43:51Z

@clangenb given the outcome above, please provide your DOT address in the PR description and I will submit an onchain DOT tip for your contributions. I do agree that this particular refactor might be best done by a full time contributor. Nonetheless, the exploration you have done here is super valuable.

Thanks a lot, it is highly appreciated. However, I am a fairly new Rank 1 member of the fellowship and just became active. Hence, I would have assumed this to be part of the fellowship. Regardless, as a Rank 1 salary is not really that high, I will gladly accept the tip, I just don't know if this considered to be ok. Just in case, I will leave my address here: :)

My polkadot address would be: 16YCL3UVpVWQLGW3p3Zx4k5WAEp9W1DwdDnxAbyAaPxVxnp3

clangenb · 2024-06-04T14:50:44Z

I looked on the PR. Thank you again for the contribution !

For more context, our intention is to make the entire node code more reusable and the end goal is indeed to remove friction for the parachain teams trying to stay up-to-date with the polkadot-sdk. There are some details here: #5 . Probably one of the steps would be to split the service.rs into multiple files, but we will also have to do bigger refactorings, for example:
* move common logic to a separate crate

* move some logic to reusable components

* add some traits maybe

* probably have some builder
The entire project is quite big and ambiguous, and there are some unknowns at this point.

The idea of splitting the service.rs file is good and aligned with our vision, but the problem is that at this point I'm not sure how the split should look like exactly, and I would leave it as a future step after some refactoring. Probably at some point, a particular split will become natural. So I would like to continue the work from here, do some refactoring and we can sync at that point about the split if that's ok with you.

Spoileralert: I am thinking about a similar change for the commands.rs

Please let's sync again at a later time about this as well since we'll probably touch the command.rs file in our refactorings.

I absolutely align with what you have intended here. My idea was to have the PR as a first step to that refactoring. And merging this PR would immediately simplify the maintenance process for parachain teams. However, I also agree that we don't want to have too many refactoring PRs as every refactoring is an extra overhead for the parachain maintainers. I guess the decision of merging this PR more or less as-is should mainly depend on the timeline you have in mind for the overall project.

I am simply stating my thoughts here, if you think you want to build take it from here without merging the PR I am completely fine with it. 👍

serban300 · 2024-06-12T14:04:58Z

My idea was to have the PR as a first step to that refactoring. And merging this PR would immediately simplify the maintenance process for parachain teams. However, I also agree that we don't want to have too many refactoring PRs as every refactoring is an extra overhead for the parachain maintainers.

Yes, I also thought of that and I have the same concern about changing this too many times. And because of this to me it seems safer to postpone the split a bit.

I am simply stating my thoughts here, if you think you want to build take it from here without merging the PR I am completely fine with it. 👍

Thanks ! I'm not sure how the final split will look like, but I will use the split from this PR as a template while moving forward with the refactoring.

kianenigma · 2024-06-13T02:42:17Z

/tip medium

substrate-tip-bot · 2024-06-13T02:42:40Z

@kianenigma A referendum for a medium (80 DOT) tip was successfully submitted for @clangenb (16YCL3UVpVWQLGW3p3Zx4k5WAEp9W1DwdDnxAbyAaPxVxnp3 on polkadot).

Referendum number: 865.

substrate-tip-bot · 2024-06-13T02:43:44Z

The referendum has appeared on Polkassembly.

clangenb added 12 commits May 3, 2024 10:57

move service to service/mod.rs

5b175ae

refactor some stuff into service/core and service/start_node

6541297

refactor more stuff into service/core and service/start_node

9df9555

fix warnings with cargo fix -p polkadot-parachain-bin

58beadf

fix more warnings

978c0fd

rename core to consensus

d2694ff

improve some import paths and fmt

0fcf98d

add license headers

026cd3f

extract rpc_extensions

dfbcb9c

extract new_partial, and start_node_impl.rs to separate files

2430ed5

introduce common types module

180e1d5

cleanup

256fbe8

clangenb changed the title ~~Split polkadot-parachains service file into multiple modules~~ Split polkadot parachains service file into multiple modules May 3, 2024

clangenb commented May 3, 2024

View reviewed changes

fmt

c230b71

kianenigma mentioned this pull request May 21, 2024

parachain-omni-node binary #3597

Closed

skunert reviewed May 21, 2024

View reviewed changes

kianenigma closed this Jun 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split polkadot parachains service file into multiple modules #4369

Split polkadot parachains service file into multiple modules #4369

clangenb commented May 3, 2024 •

edited by kianenigma

Loading

clangenb May 3, 2024

clangenb May 3, 2024

kianenigma commented May 21, 2024

clangenb commented May 21, 2024

skunert left a comment

skunert May 21, 2024

clangenb May 22, 2024 •

edited

Loading

skunert May 21, 2024

clangenb May 22, 2024

Polkadot-Forum commented May 23, 2024

serban300 commented May 23, 2024

kianenigma commented May 28, 2024

clangenb commented May 28, 2024

serban300 commented May 31, 2024

kianenigma commented May 31, 2024

clangenb commented Jun 4, 2024

clangenb commented Jun 4, 2024

serban300 commented Jun 12, 2024 •

edited

Loading

kianenigma commented Jun 13, 2024

substrate-tip-bot bot commented Jun 13, 2024

substrate-tip-bot bot commented Jun 13, 2024

		use parachains_common::{AccountId, Balance, Block, Nonce};
		use crate::service::common_types::{AccountId, Balance, Block, Nonce};

Split polkadot parachains service file into multiple modules #4369

Split polkadot parachains service file into multiple modules #4369

Conversation

clangenb commented May 3, 2024 • edited by kianenigma Loading

clangenb May 3, 2024

Choose a reason for hiding this comment

clangenb May 3, 2024

Choose a reason for hiding this comment

kianenigma commented May 21, 2024

clangenb commented May 21, 2024

skunert left a comment

Choose a reason for hiding this comment

skunert May 21, 2024

Choose a reason for hiding this comment

clangenb May 22, 2024 • edited Loading

Choose a reason for hiding this comment

skunert May 21, 2024

Choose a reason for hiding this comment

clangenb May 22, 2024

Choose a reason for hiding this comment

Polkadot-Forum commented May 23, 2024

serban300 commented May 23, 2024

kianenigma commented May 28, 2024

clangenb commented May 28, 2024

serban300 commented May 31, 2024

kianenigma commented May 31, 2024

clangenb commented Jun 4, 2024

clangenb commented Jun 4, 2024

serban300 commented Jun 12, 2024 • edited Loading

kianenigma commented Jun 13, 2024

substrate-tip-bot bot commented Jun 13, 2024

substrate-tip-bot bot commented Jun 13, 2024

clangenb commented May 3, 2024 •

edited by kianenigma

Loading

clangenb May 22, 2024 •

edited

Loading

serban300 commented Jun 12, 2024 •

edited

Loading