search

paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 643 forks source link

refactor polkadot-runtime-common #2186

Open xlc opened 10 months ago

xlc commented 10 months ago

It is like an unloved child received all the garbage gift from people don't know where to put their stuff.

Yet I am forced to inherent all those via transitive dependency.

cumulus-pallet-xcmp-queue -> polkadot-runtime-common -> pallet-asset-rate and https://github.com/paritytech/polkadot-sdk/issues/1136#issuecomment-1797063298

xlc commented 10 months ago

why trait PriceForMessageDelivery lives in polkadot-runtime-common?

https://github.com/paritytech/polkadot-sdk/blob/6f1b33befef5c164ccca53595d8f03dda203fae4/polkadot/runtime/common/src/xcm_sender.rs#L34

and there is zero reason to make the Id type generic. it only makes life of the dev doing upgrades miserable

xlc commented 10 months ago

polkadot-runtime-common received another gift https://github.com/paritytech/polkadot-sdk/pull/1814

asiniscalchi commented 5 months ago

I'm totally behind the idea of taking the refactoring of polkadot-runtime-common one step at a time. Breaking it down into manageable pieces not only makes it simpler to oversee and evaluate each change, but it also allows us to adapt our strategy based on real-time feedback and unforeseen challenges.

In line with this, could you give us an example of a particular component or functionality within polkadot-runtime-common that you believe should be relocated?

xlc commented 5 months ago

We need to figure out what is polkadot-runtime-common exactly first. Maybe someone can write a single sentence description? And everything that doesn't fit within that description should be relocated.

If you really want an answer from me, I will just say everything should be relocated.

But we can start with pallets. Every crate can only contain a single pallet. Let's enforce this policy.

asiniscalchi commented 5 months ago

Taking pallet_para_sudo_wrapper as an example, if we were to extract it, where do you think it should be relocated? https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/runtime/common/src/paras_sudo_wrapper.rs

This could serve as our starting point. By tackling this step by step, the polkadot-runtime-common crate will gradually slim down, perhaps even finding its true identity through this process.

Relocating the pallets from the polkadot-runtime-common crate can be tackled in parallel, making it a straightforward task for the community. This approach will allow us to efficiently distribute the work and make significant progress quickly.

I can draft a couple of user stories (issues) to kickstart this if you're interested.

bkontur commented 5 months ago

We need to figure out what is polkadot-runtime-common exactly first. Maybe someone can write a single sentence description?

https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/runtime/common/Cargo.toml#L4

name = "polkadot-runtime-common"
description = "Pallets and constants used in Relay Chain networks."

There is also some stuff which is used or shared between relaychain and parachain runtimes. E.g. mentioned PriceForMessageDelivery, previously we had two traits PriceForChildMessageDelivery and in the other module PriceForSiblingMessageDelivery, I joined them to one PriceForMessageDelivery, so that we don't duplicate other stuff e.g. ExponentialPrice, ConstantPrice, NoPriceForMessageDelivery ... which can be now used by relay and para chains.

Where would be the best place for this stuff? Do we want several small "common" or specific crates? It is specific runtime stuff, so probably xcm modules are not a good fit. Yes, we have polkadot-runtime-common and parachains-common, but what about stuff which is relevant for both relaychain and parachain? Where to place them? So, yes, if we intend to refactor that out, we need to consider a better place and the rationale behind the structure. Actual base dir structure:

asiniscalchi commented 5 months ago 
graph TD;
    polkadot-->substrate;
    relaychain_n -->substrate
    cumulus-->substrate;
    parachain_m-->substrate;
    parachain_m-->cumulus;

If this dependency graph mirrors reality, it'll show us where components best fit.

Let's focus on pallet_para_sudo_wrapper for now, aiming to house it in its own crate within the Polkadot package. This first step towards slimming polkadot-runtime-common down allows for easy future relocations.

asiniscalchi commented 5 months ago

@xlc @bkontur I propose you this: https://github.com/paritytech/polkadot-sdk/issues/3877

asiniscalchi commented 5 months ago

@xlc @bkontur I propose you this: #3877

I've implemented the user story in a way that's completely transparent to any code utilizing polkadot-runtime-common. This approach allows us to refactor without causing any issues externally. Once we're satisfied with the results, we might consider relocating the outcome, potentially removing the common crate that seems very risky.

https://github.com/paritytech/polkadot-sdk/pull/3882

xlc commented 5 months ago

We need to figure out what is polkadot-runtime-common exactly first. Maybe someone can write a single sentence description?

https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/runtime/common/Cargo.toml#L4

name = "polkadot-runtime-common"
description = "Pallets and constants used in Relay Chain networks."

There is also some stuff which is used or shared between relaychain and parachain runtimes. E.g. mentioned PriceForMessageDelivery, previously we had two traits PriceForChildMessageDelivery and in the other module PriceForSiblingMessageDelivery, I joined them to one PriceForMessageDelivery, so that we don't duplicate other stuff e.g. ExponentialPrice, ConstantPrice, NoPriceForMessageDelivery ... which can be now used by relay and para chains.

Where would be the best place for this stuff? Do we want several small "common" or specific crates? It is specific runtime stuff, so probably xcm modules are not a good fit. Yes, we have polkadot-runtime-common and parachains-common, but what about stuff which is relevant for both relaychain and parachain? Where to place them? So, yes, if we intend to refactor that out, we need to consider a better place and the rationale behind the structure. Actual base dir structure:

  • cumulus
  • polkadot
  • substrate

for constants have a constant crate for utility types, have a utility crate for xcm utility types, have a xcm utility crate

we should not only think about relaychain and system parachains. many types are also used/useful for downstream projects and we should allow downstream projects to not include bunch useless dependencies and code.

moving all the pallets out is a good starting point.

asiniscalchi commented 5 months ago

Let's go then ... it might be useful to add a checkbox list in this user story description.

Additionally, I kindly request that the PR be merged. The advantageous aspect of this refactor strategy is that it enhances code quality incrementally with each piece completed, eliminating the need to delay until the entirety of the work is finalized.

asiniscalchi commented 5 months ago

@kianenigma some feedback here. I was able to extract 4 pallets without touching any logic ... but it's sensitive code as it is audited

asiniscalchi commented 5 months ago

hi there ... is anybody giving feedback on this ? @xlc @bkchr ?