Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Internal dependencies organization #1380

Open
lemunozm opened this issue Jun 2, 2023 · 15 comments
Open

Internal dependencies organization #1380

lemunozm opened this issue Jun 2, 2023 · 15 comments
Labels
crcl-protocol Circle protocol related. dependencies Pull requests that update a dependency file. epic Bigger issue, needs to be broken down. I6-refactoring Code needs refactoring. P2-nice-to-have Issue is worth doing. Q7-very-hard Can be done by an expert coder with a deep knowledge of the codebase.

Comments

@lemunozm
Copy link
Contributor

lemunozm commented Jun 2, 2023

Description

We don't have well-defined rules about where things should be. Here I propose some changes and rules that I think can improve the repo organization and get rid of dependency problems in the future.

Feel free to discuss!

Link to the diagram sources

Current state

Our repo has the following dependencies, and we have 2 main levels:

Note: These arrows are transitive. It means i.e. that a runtime-common could make use of cfg-traits, or that the pallet level has access to cfg-primitives

image

Ideally, we would want to remove those red upward dependencies that exist now.
Down/right directions in this diagram mean "more generic", so those upper lines involve a lot of knowledge from a part that should not be known about. We only want to remain dependencies to "more generic" and avoid generic things knowing about specific things.

Desired

By removing the red lines, and redefining the purpose of each crate, we can get a down/right diagram:

image

Purpose of each crate:

From more generic (widely purpose) to less generic (centrifuge-related purpose):

  • cfg-primitives: Common utilities without any specific behavior associated. All constant with knowledge about the usage on runtime should be moved to cfg-types, and we only left the generic ones:
    • i.e. Adjustement type or SECONDS_PER_MINUTE, DAYS constants..
  • cfg-traits: Common interfaces to all runtimes. It implies traits and also could imply types tied to those traits. But it should not use cfg-types, because they are specific types for configuring in the runtimes.
    • i.e. Permission, InterestAccrual, or PoolInvest.
  • cfg-types: Should know about centrifuge-related types. These are types used in runtimes to configure pallets, but should not be used direclty in the pallets. They make sense for our use cases but not for others who can use our pallets. These types, therefore, should not know about pallets or runtimes. And should not be types used by cfg-traits.
    • i.e. complex types: CurrencyId, FeeKey, Location
    • i.e. simple types: PoolId, AccountId, Balance.
    • i.e. constants: MAXIMUM_BLOCK_WEIGHT, CFG(?), SLOT_DURATION, TREASURY_FEE_RATIO
    • If there are different types in each runtime, as AIR and CFG, I think each should be into some runtime-dependent module as runtime/altair/src/types.rs.
  • runtime-common: Common utilities to all runtimes. It knows about how runtimes work and has hard dependencies on the pallet used. It's simply a common place where to store code common to all runtimes.
@lemunozm lemunozm added Q5-hard Can be done by an experienced coder with a good knowledge of the codebase. P2-nice-to-have Issue is worth doing. I6-refactoring Code needs refactoring. crcl-protocol Circle protocol related. dependencies Pull requests that update a dependency file. labels Jun 2, 2023
@wischli
Copy link
Contributor

wischli commented Jun 6, 2023

Thanks for bringing this up and supporting your argument with nifty diagrams!

Say I have a custom type in cfg-types for which I want to implement a trait from cfg-types, e.g. some trait over our CurrencyId enum. Do I understand correctly, that this could only live in runtime-common if it is relevant on the runtime level?

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2023

Good question!

Probably, cfg-types, which lives in the centrifuge domain, can also access cfg-traits (and transitively to cfg-primitives), which is at a lower level.

Something like the blue line:
image

@wischli
Copy link
Contributor

wischli commented Jun 6, 2023

So by applying this proposal, we would have to provide all cfg-types used on the pallet level via the pallet::Config trait instead, right? E.g. pallet-connectors uses DomainAddress (among others) which needs to be added to the Config trait then.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2023

Yes (that is what we are doing all the time). Or if the type is very generic, then it can be place in cfg-primitives, and let all pallets to use them directly.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2023

I added another graph to express the idea better of going from more specific (related to our centrifuge domain, top) to more generic (easier for everybody to use, bottom):

image

Arrows are transitive, and a module must not reach an upper module.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 8, 2023

@wischli @mustermeiszer I was thinking about if we would really need to differentiate between runtime-common and cfg-types in this schema, maybe both can be the same crate, WDYT? 🤔

@wischli
Copy link
Contributor

wischli commented Jun 8, 2023

@wischli @mustermeiszer I was thinking about if we would really need to differentiate between runtime-common and cfg-types in this schema, maybe both can be the same crate, WDYT? 🤔

IMO, cfg-types could indeed be moved to a module inside runtime-common but I am sure @mustermeiszer has a stronger opinion.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 9, 2023

I like what you say in your last comment @wischli, I think after this, the structure would be as follows:

image

Quite simple / straightforward

I think the main idea is that:

  • RuntimeX and runtime-common are only for our mission and for us.
  • PalletX, cfg-traits, and cfg-primitives ideally, any project could use it.

@mustermeiszer
Copy link
Collaborator

No opinion here from my side 😃

@mustermeiszer
Copy link
Collaborator

cfg-primitives are mostly primitives that are really specific to us. Mostly, all teams use the same setup -- e.g. type Balance = u128 but no trait or pallet should depend on that IMO. But I agree with the above otherwise.

@lemunozm
Copy link
Contributor Author

Yeah, actually, I agree with you.- cfg-primitives should be a module of runtime-common then or be at the same level. It should live in the runtime level.

The drawn box now called cfg-primitives should then be called cfg-utils or similar and contains things like Adjustment type. Utilities for everybody to use.

@lemunozm
Copy link
Contributor Author

lemunozm commented Oct 9, 2023

Updated diagram with the above:

image

@mustermeiszer
Copy link
Collaborator

So cfg-traits MUST NOT contain any types from cfg-types?

@lemunozm
Copy link
Contributor Author

Yes if those types are intended to be used to configure the runtime.

Right now, cfg_types contains a mix of general purpose types and centrifuge-specific runtime types. The first one could be used by cfg_traits (as Adjustement type) but the second set of types should not. The first set of types as Adjustement could be placed in cfg-utils.

@lemunozm
Copy link
Contributor Author

So, as an overview, we should divide cfg-types in:

  • general purpose types, in cfg_utils.
  • centrifuge-related types, in runtime-common/types

@lemunozm lemunozm added epic Bigger issue, needs to be broken down. Q7-very-hard Can be done by an expert coder with a deep knowledge of the codebase. and removed Q5-hard Can be done by an experienced coder with a good knowledge of the codebase. labels Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. dependencies Pull requests that update a dependency file. epic Bigger issue, needs to be broken down. I6-refactoring Code needs refactoring. P2-nice-to-have Issue is worth doing. Q7-very-hard Can be done by an expert coder with a deep knowledge of the codebase.
Projects
None yet
Development

No branches or pull requests

3 participants