migrate pallet-mixnet to umbrella crate #6986
Conversation
UtkarshBhardwaj007
added
R0-silent
|
Part of #6504 |
| @@ -203,29 +203,40 @@ pub mod prelude { | |||
| /// Dispatch types from `frame-support`, other fundamental traits | |||
| #[doc(no_inline)] | |||
| pub use frame_support::dispatch::{GetDispatchInfo, PostDispatchInfo}; | |||
| pub use frame_support::traits::{Contains, IsSubType, OnRuntimeUpgrade}; | |||
| pub use frame_support::traits::{ | |||
| Contains, EstimateNextSessionRotation, IsSubType, OnRuntimeUpgrade, OneSessionHandler, | |||
There was a problem hiding this comment.
Which traits to include in the prelude is subjective I suppose, but adding these two here instead of just use frame::traits::OneSessionHandler might be adding too much if they're not used that often. In general the prelude should be sparing. I'll leave that decision to @kianenigma and @re-gius though since I haven't really been involved in this so far
There was a problem hiding this comment.
I agree with you that we should only include things that are used a lot in the prelude. Both OneSessionHandler and OnRuntimeUpgrade are used in various other pallets, hence I included them in the prelude. So my idea was to only import things from frame::prelude or frame::deps for dependencies. We can create modules to group similar ideas like traits, arithmetic or cryptography and re-export them in the prelude. This way all imports used in multiple pallets would be directly imported from the prelude and other imports from dependencies would be from frame::deps.
But given that I made this decision only based on the usage count of the 2 types (above) and there might be other considerations involved, happy to hear your thoughts on this @kianenigma and @re-gius.
There was a problem hiding this comment.
In general the prelude should be sparing
My thinking so far has been to make preludes expansive: it will likely over-import, but hardly under-import.
To this particular case, we can defer the judgment to a bit of domain specific knowledge:
OnRuntimeUpgrade is a no-brainer very commonly used trait, should be in prelude.
OneSessionHandler is only for pallets that interact with pallet-session. All consensus/staking related pallets do so, but there are not so many of them. I'd be happy to go either way in this one if someone has a strong opinion.
If not, my opinion is to keep Session related traits like EstimateNextSessionRotation and OneSessionHandler in prelude as pallet-session is somewhat integral to all runtimes built with FRAME.
There was a problem hiding this comment.
Thanks for adding context to this. Based on what you said, I do agree with your approach in this case.
I'd be happy to go either way in this one if someone has a strong opinion.
I just think that we should somehow concretely define what should and shouldn't be in the prelude. This would speed up the migration as there would be no ambiguity. For example, I was going about this based on usage count. If something is used in more than (let's say) 3 pallets, it goes in the prelude. But like you pointed out, in this case, it might make more sense to not have it in the prelude. Unfortunately, I don't have a suggestion on how we can define this other than the usage count itself.
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
| }; | ||
| let Some(block_in_phase) = | ||
| block_in_phase.checked_sub(&T::NumRequestsToCurrentBlocks::get()) | ||
| else { | ||
| return SessionPhase::RequestsToCurrent | ||
| return SessionPhase::RequestsToCurrent; |
There was a problem hiding this comment.
This is likely because your formatter is different than what we use in CI.
You might want to ask in the CI helpdepsk to once and for all document where one should find the proper rust versions for all operations (test, fmt), if you have a hard time finding it.
There was a problem hiding this comment.
I asked in the CI helpdesk and they pointed me to .github/env which only has this configuration:
IMAGE="docker.io/paritytech/ci-unified:bullseye-1.81.0-2024-11-19-v202411281558"I checked the fmt workflow as well and that has this configuration -
- name: Cargo fmt
id: required
run: cargo +nightly fmt --all -- --checkI am using the nightly toolchain as well for fmt and given that the GitHub workflow is succeeding, I think this formatting is correct.
Here is my configuration:
rustc --version
rustc 1.81.0 (eeb90cda1 2024-09-04)
cargo --version
cargo 1.81.0 (2dbb1af80 2024-08-20)
rustc +nightly --version
rustc 1.85.0-nightly (c26db435b 2024-12-15)
cargo +nightly --version
cargo 1.85.0-nightly (769f622e1 2024-12-14)
umbrella/Cargo.toml
Outdated
| @@ -290,7 +290,6 @@ runtime-benchmarks = [ | |||
| "pallet-membership?/runtime-benchmarks", | |||
| "pallet-message-queue?/runtime-benchmarks", | |||
| "pallet-migrations?/runtime-benchmarks", | |||
| "pallet-mixnet?/runtime-benchmarks", | |||
There was a problem hiding this comment.
This should not be removed from what I can say -- This pallet will still have runtime-benchmarks feature.
There was a problem hiding this comment.
There is no bechmarking.rs for pallet-mixnet, hence I remove this. It builds without any issue. Could you please explain a bit as to why you think that this should be still included?
There was a problem hiding this comment.
Added it back for now. Will understand the context for this offline.
Description
Migrate pallet-mixnet to use umbrella crate whilst adding a few types and traits in the frame prelude that are used by other pallets as well.
Review Notes
pallet-mixnetto use the umbrella crate.use sp_application_crypto::RuntimeAppPublic;and imports fromsp_mixnet::types::have not been migrated to the umbrella crate as they are not used in any / many other places and are relevant only to thepallet-mixnet.frame-systemhave been added to the mainpreludeas they have usage across various pallets.arithmeticmodule in the mainpreludesince this is used a lot throughout various pallets.cargo fmtpalet-mixnetas there is no benchmarking.rs present forpallet-mixnet. For the same reason,"pallet-mixnet?/runtime-benchmarks"has been removed fromumbrella/Cargo.toml.