Use frame umbrella crate in `pallet-proxy` and `pallet-multisig` #5995

kianenigma · 2024-10-09T13:26:05Z

A step towards #4782

In order to nail down the right preludes in polkadot-sdk-frame, we need to migrate a number of pallets to be written with it. Moreover, migrating our pallets to this simpler patter will encourage the ecosystem to also follow along.

If this PR is approved and has no unwanted negative consequences, I will make a tracking issue to migrate all pallets to this umbrella crate.

TODO:

fix frame benchmarking template. Can we detect the umbrella crate in there and have an if else? cc @ggwpez
Migrate benchmarking to v2 @re-gius a good candidate for you, you can open a PR against my branch.
tracking issue with follow-ups

…frame-umbrella-in-pallets

ggwpez · 2024-10-09T15:24:55Z

substrate/frame/multisig/src/weights.rs

-use core::marker::PhantomData;
-
+// TODO update this in frame-weight-template.hbs
+use frame::weights_prelude::*;


Huh honestly not sure what to best do here. The long-term solution is to have a project config file for the benchmarker, like for Zepter, so that it gets all the flags from there.
Then we could add an argument to use the frame crate instead.

Until then we either have to add a --use-frame-crate flag manually, or cheat by re-exporting the weight type from the crate and use that.

This would break both for frame and polkadot-sdk, basically meaning pallet development always needs frame_support directly in Cargo.toml? unfortunate.

How about providing two frame-weight-template.hbs ? One for old school, one for new school?

Discussed about moving all our pallets to include frame as dependency and moving the template to assume that exists.

maybe we could also assume that the pallet weight is a direct module inside the module with pallet definition.

So we can use super::pallet::reexport_of_frame?

But assuming frame as a dependency sounds better, people can always modify the .hbs to their flavor.

the handlebar templates should be able to allow whatever decision you all pick, but the change needs to happen at that level, not here.

I see two ways forward with this:

We currently use substrate/.maintain/frame-weight-template.hbs in most places. We create a new substrate/.maintain/frame-umbrella-weight-template.hbs. Users are free to choose. Our CI should also adapt, and based on the presence of frame-support in the deps, make a decision.

We make a mega PR that will move all pallets to have a #[doc(hidden)] pub(crate) mod __private_weights { /* re export all the stuff that might be needed in the weight.rs file */ }. We also update substrate/.maintain/frame-weight-template.hbs to start with use crate::__private_weights::*. Our CI should pass once we merge this. Then, we resume this PR.

Solution 1 is clearly better, and more extensible. Solution 2 is only a reasonable hack if 1 is too hard to pull off.

@mordamax this is mainly a question from you; how hard will it be to have our weight generation CI (the fellowship's) be able to handle two templates? or perhaps we already support it?

I talked to @mordamax and we should explore option, it should not have any major blockers and just needs more updates to our CI.

It is not the cleanest thing in the world, but this is how it would look like:

2d8323f

substrate/frame/Cargo.toml

gui1117 · 2024-10-12T15:51:15Z

substrate/frame/multisig/Cargo.toml

-frame-system = { workspace = true }
-sp-io = { workspace = true }
-sp-runtime = { workspace = true }
+frame = { workspace = true, features = ["experimental", "runtime"] }


do we actually need experimental? I don't see it used in the past AFAICT

I will remove it as soon as I am a bit more confident in the readiness of the frame crate 🙈

gui1117 · 2024-10-12T15:54:57Z

substrate/frame/multisig/src/migrations.rs

@@ -42,15 +36,14 @@ pub mod v1 {
 	pub struct MigrateToV1<T>(core::marker::PhantomData<T>);
 	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
+		fn pre_upgrade() -> Result<Vec<u8>, frame::deps::sp_runtime::TryRuntimeError> {


Not related to the PR, but maybe we could introduce: TryRuntimeResult<T> = Result<T, TryRuntimeError>

substrate/frame/multisig/src/migrations.rs

gui1117 · 2024-10-12T15:59:12Z

substrate/frame/multisig/src/lib.rs

 };
-pub use weights::WeightInfo;
+use weights::WeightInfo;


Note that it can be used by the weights templates like this line:

impl<T: frame_system::Config> {{pallet}}::WeightInfo for WeightInfo<T> {

gui1117 · 2024-10-12T16:01:58Z

substrate/frame/proxy/Cargo.toml

-frame-system = { workspace = true }
-sp-io = { workspace = true }
-sp-runtime = { workspace = true }
+frame = { workspace = true, features = ["experimental", "runtime"] }


note experimental wasn't used before

gui1117 · 2024-10-12T16:07:45Z

substrate/frame/src/lib.rs

 //! pub mod runtime {
 //! 	# use polkadot_sdk_frame as frame;
 //! 	use frame::runtime::prelude::*;
 //! }
 //! ```
 //!
-//! See: [`prelude`], [`testing_prelude`] and [`runtime::prelude`].
+//! See: .


what is this line?

gui1117 · 2024-10-12T16:13:55Z

templates/minimal/runtime/Cargo.toml

 	"scale-info/std",
 	"serde_json/std",
+
+	"pallet-minimal-template/std",
+	"polkadot-sdk/std",


personally I find keeping it in one block better.

then it can also be sorted by toml linters

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

shawntabrizi · 2024-10-15T15:05:28Z

templates/minimal/pallets/template/src/lib.rs

+#[allow(unused_imports)]
+use frame::prelude::*;


why though? just for the template to be extended? perhaps you are better to find some way to just use it?

shawntabrizi · 2024-10-15T15:07:39Z

substrate/frame/proxy/src/weights.rs

-use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
-use core::marker::PhantomData;
+use frame::weights_prelude::*;


this change doesn't make sense. it needs to be done at the template level, or handled later.

shawntabrizi

went through all the files, and changes look good except the benchmarks which need template level changes

Sub-task of PR #5995 ## Integration These changes should **not** require any integration effort. --------- Co-authored-by: Dónal Murray <donal.murray@parity.io> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

re-gius · 2024-10-17T10:43:29Z

substrate/frame/src/lib.rs

 //!
 //! ```
-//! # use polkadot_sdk_frame as frame;
+//! use polkadot_sdk_frame as frame;
 //! #[frame::pallet]
 //! pub mod pallet {
 //! 	# use polkadot_sdk_frame as frame;


We can remove this line since it's a duplicate

kianenigma · 2024-10-17T12:36:33Z

substrate/frame/support/src/lib.rs

@@ -886,7 +886,10 @@ pub mod testing_prelude {
 pub mod pallet_prelude {
 	pub use crate::{
 		defensive, defensive_assert,
-		dispatch::{DispatchClass, DispatchResult, DispatchResultWithPostInfo, Parameter, Pays},
+		dispatch::{


I now realize that it is actually best if we don't alter re-exports that are made here, and instead bring more and more things into frame::prelude. So I would resume by reverting all changes here, add the items added to frame::prelude manually as new pub use frame_support::{this, that}.

This is because adding dependencies here would also mean that we will need to cause a breaking change in frame_support, as we are altering the public API.

Also, in action we are seeing that it in this PR, adding items in this prelude is causing random other pallets to break, because of ambiguous imports.

So, the guideline that I have written so far:

When doing so, prefer adding the relevant item to frame_support::pallet_prelude and frame_system::pallet_prelude.

is not a good advice.

kianenigma added 3 commits October 9, 2024 13:59

pallet-proxy

27df8ea

pallet-multisig

8eefc00

Merge branch 'master' of github.com:paritytech/polkadot-sdk into kiz-…

c151681

…frame-umbrella-in-pallets

kianenigma requested a review from a team as a code owner October 9, 2024 13:26

kianenigma added R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed R0-silent Changes should not be mentioned in any release notes labels Oct 9, 2024

kianenigma mentioned this pull request Oct 9, 2024

Use the umbrella crate for the parachain template #5991

Merged

kianenigma added 3 commits October 9, 2024 15:55

better docs

78b52f2

Merge branch 'master' of github.com:paritytech/polkadot-sdk into kiz-…

0305c59

…frame-umbrella-in-pallets

also improve the minimal template

d253bc5

ggwpez reviewed Oct 9, 2024

View reviewed changes

fix

a4b1fd2

paritytech-review-bot bot requested a review from a team October 9, 2024 15:41

re-gius mentioned this pull request Oct 10, 2024

Upgrade Proxy and Multisig pallets to Benchmarking V2 #6018

Merged

gui1117 reviewed Oct 12, 2024

View reviewed changes

Update substrate/frame/multisig/src/migrations.rs

e0293d1

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

paritytech-review-bot bot requested a review from a team October 15, 2024 14:58

shawntabrizi reviewed Oct 15, 2024

View reviewed changes

shawntabrizi approved these changes Oct 15, 2024

View reviewed changes

re-gius and others added 3 commits October 15, 2024 17:32

Upgrade Proxy and Multisig pallets to Benchmarking V2 (#6018)

253ce3f

Sub-task of PR #5995 ## Integration These changes should **not** require any integration effort. --------- Co-authored-by: Dónal Murray <donal.murray@parity.io> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

cleanup

ff36c53

possibly how CI will look like

2d8323f

kianenigma requested review from a team as code owners October 15, 2024 17:13

Master.into()

687b61f

paritytech-review-bot bot requested a review from a team October 16, 2024 13:35

re-gius reviewed Oct 17, 2024

View reviewed changes

kianenigma commented Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use frame umbrella crate in `pallet-proxy` and `pallet-multisig` #5995

Use frame umbrella crate in `pallet-proxy` and `pallet-multisig` #5995

kianenigma commented Oct 9, 2024 •

edited

Loading

ggwpez Oct 9, 2024

kianenigma Oct 9, 2024 •

edited

Loading

ggwpez Oct 11, 2024

gui1117 Oct 12, 2024

shawntabrizi Oct 15, 2024

kianenigma Oct 15, 2024

kianenigma Oct 15, 2024

kianenigma Oct 15, 2024

gui1117 Oct 12, 2024

kianenigma Oct 15, 2024

gui1117 Oct 12, 2024

gui1117 Oct 12, 2024

gui1117 Oct 12, 2024

gui1117 Oct 12, 2024

gui1117 Oct 12, 2024

shawntabrizi Oct 15, 2024

shawntabrizi Oct 15, 2024

shawntabrizi Oct 15, 2024

shawntabrizi left a comment

re-gius Oct 17, 2024

kianenigma Oct 17, 2024

kianenigma Oct 17, 2024

Use frame umbrella crate in pallet-proxy and pallet-multisig #5995

Are you sure you want to change the base?

Use frame umbrella crate in pallet-proxy and pallet-multisig #5995

Conversation

kianenigma commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

kianenigma Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawntabrizi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use frame umbrella crate in `pallet-proxy` and `pallet-multisig` #5995

Use frame umbrella crate in `pallet-proxy` and `pallet-multisig` #5995

kianenigma commented Oct 9, 2024 •

edited

Loading

kianenigma Oct 9, 2024 •

edited

Loading