Migrating salary pallet to use umbrella crate #7048

seemantaggarwal · 2025-01-06T07:17:38Z

Description

Migrating salary pallet to use umbrella crate. It is a follow-up from #7025
Why did I create this new branch?
I did this, so that the unnecessary cargo fmt changes from the previous branch are discarded and hence opened this new PR.

Review Notes

This PR migrates pallet-salary to use the umbrella crate.

Added change: Explanation requested for why TestExternalities was replaced by TestState as testing_prelude already includes it
pub use sp_io::TestExternalities as TestState;

I have also modified the defensive! macro to be compatible with umbrella crate as it was being used in the salary pallet

substrate/frame/salary/src/benchmarking.rs

substrate/frame/salary/src/lib.rs

substrate/frame/salary/src/tests/integration.rs

substrate/frame/support/src/traits.rs

substrate/frame/salary/src/benchmarking.rs

substrate/frame/salary/src/lib.rs

substrate/frame/salary/src/tests/integration.rs

kianenigma

A couple of things are imported manually that are likely in the prelude or else must be added, otherwise looks good!

substrate/frame/salary/src/lib.rs

substrate/frame/salary/src/tests/integration.rs

substrate/frame/salary/src/tests/unit.rs

UtkarshBhardwaj007 · 2025-01-06T16:24:18Z

substrate/frame/salary/src/weights.rs

@@ -46,8 +46,10 @@
 #![allow(unused_imports)]
 #![allow(missing_docs)]

-use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
+use crate::*;


Why do you need this?

Could you add some explanation as to why you think this is needed before resolving the thread again? You can add a commit link if you want or any explanation.

UtkarshBhardwaj007 · 2025-01-06T16:25:27Z

substrate/frame/salary/src/weights.rs

@@ -46,8 +46,10 @@
 #![allow(unused_imports)]
 #![allow(missing_docs)]

-use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
+use crate::*;
+use frame::prelude::*;


This again is not needed. All the imports are already in scope.

- use frame::prelude::*;

Could you add some reasoning as to why you think this is needed before resolving the thread? I believe you shouldn't need this.

substrate/frame/src/lib.rs

gui1117

Looks good to me, some comments about prelude.

gui1117 · 2025-01-07T09:23:29Z

substrate/frame/salary/src/lib.rs

-	defensive,
-	dispatch::DispatchResultWithPostInfo,
-	ensure,
+


personal nits, separating imports in categories is a not needed overhead IMHO.

Suggested change

Automatic change by fmt

The trick is that if you don't have any extra newlines between use statements, they will indeed be grouped together and rustfmt will sort them for you. I think @gui1117 's comment is towards removing newlines in between use statements.

gui1117 · 2025-01-07T09:24:37Z

substrate/frame/salary/src/tests/integration.rs

-	pallet_prelude::Weight,
-	parameter_types,
-	traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll},
+


cargo fmt will put crate above other import by default

Suggested change

gui1117 · 2025-01-07T09:25:06Z

substrate/frame/salary/src/tests/unit.rs

@@ -20,20 +20,14 @@
 use std::collections::BTreeMap;



Suggested change

gui1117 · 2025-01-07T09:31:50Z

substrate/frame/src/lib.rs

+		defensive, defensive_assert,
+		traits::{
+			tokens::ConvertRank, Contains, EitherOf, EstimateNextSessionRotation, IsSubType,
+			MapSuccess, NoOpPoll, OnRuntimeUpgrade, OneSessionHandler,


I am not sure ConvertRank is really prelude but it can be ok.

Our rule here from other reviews was that if a trait is used in 3 or more pallets, it can be in prelude, if not directly imported in the migration.

gui1117 · 2025-01-07T09:34:12Z

substrate/frame/src/lib.rs

@@ -505,7 +508,7 @@ pub mod runtime {
 	#[cfg(feature = "std")]
 	pub mod testing_prelude {
 		pub use sp_core::storage::Storage;
-		pub use sp_runtime::BuildStorage;
+		pub use sp_runtime::{BuildStorage, DispatchError::Unavailable};


I am not sure Unavailable fits into prelude, DispatchError should be enough I think.

thanks for the comments @gui1117 I added a new commit for PRdoc, can you re review? additionally, for the current pallet only Unavailable was needed, I can have a separate PR for modifying this

Yeah, I do think it is a bit too much to import enum variants in any prelude -- the Enum itself makes a lot of sense. DispatchError is super popular, and 100% should be in prelude.

If a code wants to use Unavailable, they would just use DispatchError::Unavailable and that is fine.

Please add this (among other guidelines) to substrate/frame/src/lib.rs, in the top of the file, there is some high level doc, and a section exactly explaining the rules of the umbrella crate.

github-actions · 2025-01-07T09:47:08Z

Review required! Latest push from author must always be reviewed

kianenigma · 2025-01-07T11:00:23Z

substrate/frame/src/lib.rs

@@ -319,7 +322,7 @@ pub mod testing_prelude {
 	/// Other helper macros from `frame_support` that help with asserting in tests.
 	pub use frame_support::{
 		assert_err, assert_err_ignore_postinfo, assert_error_encoded_size, assert_noop, assert_ok,
-		assert_storage_noop, storage_alias,
+		assert_storage_noop, construct_runtime, storage_alias,


Are you sure construct_runtime is not already in runtime::prelude::*? I would 99% be sure it already is.

+1, it is also there in testing_prelude.

kianenigma · 2025-01-07T11:02:42Z

substrate/frame/support/src/traits/misc.rs

@@ -66,15 +66,15 @@ impl<T: VariantCount> Get<u32> for VariantCountOf<T> {
 #[macro_export]
 macro_rules! defensive {
 	() => {
-		frame_support::__private::log::error!(
+		$crate::__private::log::error!(


kianenigma

LGTM after current comments are addressed.

substrate/frame/salary/src/benchmarking.rs

UtkarshBhardwaj007 · 2025-01-08T11:33:41Z

substrate/frame/salary/src/tests/integration.rs

-	parameter_types,
-	traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll},
+use crate::{
+	tests::integration::sp_api_hidden_includes_construct_runtime::hidden_include::sp_runtime, *,


Why not import sp_runtime from the umbrella pallet directly?

let me crosscheck this, this was not a conscious change for sure

yes hidden_includes are generated by the macro for the macro usage, it can break any time.

substrate/frame/salary/src/tests/unit.rs

UtkarshBhardwaj007 · 2025-01-08T11:42:00Z

substrate/frame/salary/src/tests/unit.rs

 use crate as pallet_salary;
 use crate::*;
+use core::cell::RefCell;
+use frame::{deps::sp_runtime::traits::Identity, testing_prelude::*, traits::tokens::ConvertRank};
+use std::collections::BTreeMap;


Removing the line space between two import blocks causes them to be sorted alphabetically by the fmt. I don't have a strong preference but to be consistent, we should keep imports from the standard library in a separate block (having a line space at the end) at the top.

Personally I would prefer to not put in a separate block. If it is meant to be in a particular order then fmt should do it, same as they put crate and super out of the alphabetic order.

other comments addressed, leaving this as is

UtkarshBhardwaj007 · 2025-01-08T11:47:26Z

substrate/frame/src/lib.rs

@@ -262,7 +265,7 @@ pub mod benchmarking {
 		pub use frame_benchmarking::benchmarking::*;
 		// The system origin, which is very often needed in benchmarking code. Might be tricky only


Please update the comments with descriptions when you add new re-exports to the umbrella crate.

Raworigin is still intacte

I meant could you also add a description for frame_system::Pallet like there exists for RawOrigin.

kianenigma

Pushing one last comment with a few fixes:

Identity works fine if imported from sp_runtime
RankedMembers, RankedMembersSwapHandler are both used in 3 pallets, should go to prelude (as per the conventions of the migration)
StateVersion is used in the testing setup of the migration, same.

diff:

diff --git a/substrate/frame/salary/src/lib.rs b/substrate/frame/salary/src/lib.rs
index 256d1ad0cd..6a843625f4 100644
--- a/substrate/frame/salary/src/lib.rs
+++ b/substrate/frame/salary/src/lib.rs
@@ -22,10 +22,7 @@
 use core::marker::PhantomData;
 use frame::{
 	prelude::*,
-	traits::{
-		tokens::{GetSalary, Pay, PaymentStatus},
-		RankedMembers, RankedMembersSwapHandler,
-	},
+	traits::tokens::{GetSalary, Pay, PaymentStatus},
 };
 
 #[cfg(test)]
diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs
index 6d38a1c9b0..e4e9c8f6a3 100644
--- a/substrate/frame/salary/src/tests/integration.rs
+++ b/substrate/frame/salary/src/tests/integration.rs
@@ -19,10 +19,7 @@
 
 use crate as pallet_salary;
 use crate::*;
-use frame::{
-	deps::{sp_io, sp_runtime::StateVersion},
-	testing_prelude::*,
-};
+use frame::{deps::sp_io, testing_prelude::*};
 use pallet_ranked_collective::{EnsureRanked, Geometric};
 
 type Rank = u16;
diff --git a/substrate/frame/salary/src/tests/unit.rs b/substrate/frame/salary/src/tests/unit.rs
index 3bb7bc4adf..6bdd86ddef 100644
--- a/substrate/frame/salary/src/tests/unit.rs
+++ b/substrate/frame/salary/src/tests/unit.rs
@@ -20,7 +20,7 @@
 use crate as pallet_salary;
 use crate::*;
 use core::cell::RefCell;
-use frame::{deps::sp_runtime::traits::Identity, testing_prelude::*, traits::tokens::ConvertRank};
+use frame::{testing_prelude::*, traits::tokens::ConvertRank};
 use std::collections::BTreeMap;
 
 type Block = MockBlock<Test>;
diff --git a/substrate/frame/src/lib.rs b/substrate/frame/src/lib.rs
index dba039bee2..1795dfeb81 100644
--- a/substrate/frame/src/lib.rs
+++ b/substrate/frame/src/lib.rs
@@ -207,7 +207,7 @@ pub mod prelude {
 		defensive, defensive_assert,
 		traits::{
 			Contains, EitherOf, EstimateNextSessionRotation, IsSubType, MapSuccess, NoOpPoll,
-			OnRuntimeUpgrade, OneSessionHandler,
+			OnRuntimeUpgrade, OneSessionHandler, RankedMembers, RankedMembersSwapHandler,
 		},
 	};
 
@@ -232,7 +232,7 @@ pub mod prelude {
 	/// Runtime traits
 	#[doc(no_inline)]
 	pub use sp_runtime::traits::{
-		BlockNumberProvider, Bounded, Convert, DispatchInfoOf, Dispatchable, ReduceBy,
+		BlockNumberProvider, Bounded, Convert, DispatchInfoOf, Dispatchable, Identity, ReduceBy,
 		ReplaceWithDefault, SaturatedConversion, Saturating, StaticLookup, TrailingZeroInput,
 	};
 	/// Other error/result types for runtime
@@ -333,7 +333,7 @@ pub mod testing_prelude {
 	pub use sp_io::TestExternalities as TestState;
 
 	/// Commonly used runtime traits for testing.
-	pub use sp_runtime::traits::BadOrigin;
+	pub use sp_runtime::{traits::BadOrigin, StateVersion};
 }
 
 /// All of the types and tools needed to build FRAME-based runtimes.

LGTM after applying the above patch

paritytech-workflow-stopper · 2025-01-08T16:06:06Z

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12674245840
Failed job name: test-linux-stable

UtkarshBhardwaj007

LGTM. 2 points:

25 CI workflows are failing. Could be a red-herring but the number is suspiciously large so we should check to ensure that the failures are unrelated and raise them in the CI help desk.
Add one small comment about adding description for new re-exports that are added to any of the preludes

seemantaggarwal added 3 commits January 2, 2025 13:23

salary pallet migration to umbrella crate

1bf72c4

selectiv changes for salary pallet from cargo fmt

81ef8c3

optimisations to salary pallet to use umbrella crate

12b5035

seemantaggarwal added R0-silent Changes should not be mentioned in any release notes T2-pallets This PR/Issue is related to a particular pallet. labels Jan 6, 2025

seemantaggarwal requested a review from a team as a code owner January 6, 2025 07:17

changes to unit testing for migration to umbrella crate

9b5c229

re-gius reviewed Jan 6, 2025

View reviewed changes

Revision 5 addressing comments for umbrella crate migration

6a6a009

paritytech-review-bot bot requested a review from a team January 6, 2025 15:01

remove formatting from traits

b015e26

kianenigma reviewed Jan 6, 2025

View reviewed changes

substrate/frame/salary/src/benchmarking.rs Outdated Show resolved Hide resolved

Merge branch 'master' into new-salary-pallet

53675b3

kianenigma reviewed Jan 6, 2025

View reviewed changes

substrate/frame/salary/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 6, 2025

View reviewed changes

substrate/frame/salary/src/tests/integration.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 6, 2025

View reviewed changes

UtkarshBhardwaj007 reviewed Jan 6, 2025

View reviewed changes

using defensive as part of prelude instead of frame_support

3291d1e

paritytech-review-bot bot requested a review from a team January 6, 2025 16:51

seemantaggarwal and others added 3 commits January 7, 2025 12:40

updating salary pallet migration to umbrella crate

3dc4d8b

run taplo and night fmt

bf3302b

Merge branch 'master' into new-salary-pallet

5397c4c

gui1117 approved these changes Jan 7, 2025

View reviewed changes

add prdoc

8db080b

github-actions bot requested a review from gui1117 January 7, 2025 09:47

Merge branch 'master' into new-salary-pallet

c0b15a1

kianenigma reviewed Jan 7, 2025

View reviewed changes

kianenigma approved these changes Jan 7, 2025

View reviewed changes

seemantaggarwal and others added 4 commits January 8, 2025 15:17

re importing convert and convertrank to run successful tests

b427d2a

Moving convert to prelude

557bc71

removing unused import

de1b02f

Merge branch 'master' into new-salary-pallet

4399f35

UtkarshBhardwaj007 reviewed Jan 8, 2025

View reviewed changes

excluding sp_runtime outside of umbrella crate

ae9b6e2

re-gius approved these changes Jan 8, 2025

View reviewed changes

kianenigma approved these changes Jan 8, 2025

View reviewed changes

patch applied from @kianenigma

22ff507

UtkarshBhardwaj007 approved these changes Jan 8, 2025

View reviewed changes

reverting identity to not be included in the umbrella crate

9302fdc

iulianbarbu removed the request for review from AKJUS January 9, 2025 08:17

iulianbarbu removed the R0-silent Changes should not be mentioned in any release notes label Jan 9, 2025

Update pr_7048.prdoc

e472261

github-actions bot requested review from AKJUS and kianenigma January 9, 2025 11:56

seemantaggarwal enabled auto-merge January 9, 2025 13:19

Merge branch 'master' into new-salary-pallet

9116f5a

seemantaggarwal disabled auto-merge January 9, 2025 13:20

seemantaggarwal enabled auto-merge January 9, 2025 13:20

seemantaggarwal closed this Jan 9, 2025

auto-merge was automatically disabled January 9, 2025 13:41
Pull request was closed

seemantaggarwal reopened this Jan 9, 2025

seemantaggarwal enabled auto-merge January 9, 2025 13:44

seemantaggarwal removed the request for review from AKJUS January 9, 2025 13:45

seemantaggarwal added this pull request to the merge queue Jan 9, 2025

Merged via the queue into master with commit 2f17958 Jan 9, 2025
320 of 347 checks passed

seemantaggarwal deleted the new-salary-pallet branch January 9, 2025 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating salary pallet to use umbrella crate #7048

Migrating salary pallet to use umbrella crate #7048

seemantaggarwal commented Jan 6, 2025 •

edited by kianenigma

Loading

kianenigma left a comment

UtkarshBhardwaj007 Jan 6, 2025

UtkarshBhardwaj007 Jan 7, 2025

UtkarshBhardwaj007 Jan 6, 2025

UtkarshBhardwaj007 Jan 7, 2025

gui1117 left a comment

gui1117 Jan 7, 2025

seemantaggarwal Jan 7, 2025

kianenigma Jan 7, 2025

gui1117 Jan 7, 2025

gui1117 Jan 7, 2025

gui1117 Jan 7, 2025

kianenigma Jan 7, 2025

gui1117 Jan 7, 2025

seemantaggarwal Jan 7, 2025

kianenigma Jan 7, 2025

github-actions bot commented Jan 7, 2025

kianenigma Jan 7, 2025

UtkarshBhardwaj007 Jan 7, 2025

kianenigma Jan 7, 2025

kianenigma left a comment

UtkarshBhardwaj007 Jan 8, 2025

seemantaggarwal Jan 8, 2025

gui1117 Jan 8, 2025

UtkarshBhardwaj007 Jan 8, 2025

gui1117 Jan 8, 2025

seemantaggarwal Jan 8, 2025

UtkarshBhardwaj007 Jan 8, 2025 •

edited

Loading

seemantaggarwal Jan 8, 2025

UtkarshBhardwaj007 Jan 8, 2025

kianenigma left a comment

paritytech-workflow-stopper bot commented Jan 8, 2025

UtkarshBhardwaj007 left a comment

		@@ -262,7 +265,7 @@ pub mod benchmarking {
		pub use frame_benchmarking::benchmarking::*;
		// The system origin, which is very often needed in benchmarking code. Might be tricky only

Migrating salary pallet to use umbrella crate #7048

Migrating salary pallet to use umbrella crate #7048

Conversation

seemantaggarwal commented Jan 6, 2025 • edited by kianenigma Loading

Description

Review Notes

kianenigma 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

Choose a reason for hiding this comment

gui1117 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

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

github-actions bot commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UtkarshBhardwaj007 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

paritytech-workflow-stopper bot commented Jan 8, 2025

UtkarshBhardwaj007 left a comment

Choose a reason for hiding this comment

seemantaggarwal commented Jan 6, 2025 •

edited by kianenigma

Loading

UtkarshBhardwaj007 Jan 8, 2025 •

edited

Loading