salary pallet migration to umbrella crate #7025

seemantaggarwal · 2025-01-02T08:09:09Z

✄ -----------------------------------------------------------------------------

Thank you for your Pull Request! 🙏 Please make sure it follows the contribution guidelines outlined in this
document and fill out the
sections below. Once you're ready to submit your PR for review, please delete this section and leave only the text under
the "Description" heading.

Description

Migrating salary pallet to use umbrella crates

Review Notes

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

✄ -----------------------------------------------------------------------------

re-gius · 2025-01-02T09:05:39Z

substrate/frame/salary/src/benchmarking.rs

 use sp_core::Get;
-
+#[allow(deprecated)]
+use frame::benchmarking::v1::*;


Suggested change

use frame::benchmarking::v1::*;

use frame::benchmarking::prelude::*;

Btw, notice that here we have the v2 benchmarking syntax, not the v1 (see here for more details)

re-gius · 2025-01-02T09:09:01Z

substrate/frame/salary/src/lib.rs

+
+};
+use frame::{
+	hashing::blake2_256,


the hashing module is already imported in the main prelude, so you don't need this

substrate/frame/salary/src/lib.rs

re-gius · 2025-01-02T09:11:44Z

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

-	assert_noop, assert_ok, derive_impl,
-	pallet_prelude::Weight,
-	parameter_types,
+use frame::{prelude::*, runtime::prelude::*, testing_prelude::*,


Are you sure you need them all? Ideally, we would like to have only the testing prelude here because it's a test file and the testing prelude should already contain the other two

re-gius · 2025-01-02T09:15:35Z

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

-};
+
+use frame::{prelude::*, runtime::prelude::*, testing_prelude::*,
+			traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll,


ConstU64 is already in the runtime::prelude::*, which is in the testing_prelude::*, so it's not needed. Try to double check the other traits and imports, they may already be imported through a prelude.

+1, to add to that - you might not need all these preludes. Try checking for each import and minimising the number of imports required.

UtkarshBhardwaj007 · 2025-01-02T13:14:09Z

substrate/frame/salary/Cargo.toml

@@ -17,43 +17,29 @@ targets = ["x86_64-unknown-linux-gnu"]

 [dependencies]
 codec = { features = ["derive"], workspace = true }
-frame-benchmarking = { optional = true, workspace = true }
-frame-support = { workspace = true }
-frame-system = { workspace = true }
 log = { workspace = true }
 pallet-ranked-collective = { optional = true, workspace = true }
 scale-info = { features = ["derive"], workspace = true }
 sp-arithmetic = { workspace = true }
 sp-core = { workspace = true }


sp-core should be imported from the deps module to further reduce dependencies outside the umbrella.

Suggested change

sp-core = { workspace = true }

UtkarshBhardwaj007 · 2025-01-02T13:15:14Z

substrate/frame/salary/Cargo.toml

@@ -17,43 +17,29 @@ targets = ["x86_64-unknown-linux-gnu"]

 [dependencies]
 codec = { features = ["derive"], workspace = true }
-frame-benchmarking = { optional = true, workspace = true }
-frame-support = { workspace = true }
-frame-system = { workspace = true }
 log = { workspace = true }
 pallet-ranked-collective = { optional = true, workspace = true }
 scale-info = { features = ["derive"], workspace = true }
 sp-arithmetic = { workspace = true }


sp-arithmetic is present in the main prelude and should be used directly from there instead of adding a dependency to this pallet.

Suggested change

sp-arithmetic = { workspace = true }

UtkarshBhardwaj007 · 2025-01-02T13:17:12Z

substrate/frame/salary/src/benchmarking.rs

@@ -22,10 +22,9 @@
 use super::*;
 use crate::Pallet as Salary;

-use frame_benchmarking::v2::*;
-use frame_system::{Pallet as System, RawOrigin};
 use sp_core::Get;


Suggested change

use sp_core::Get;

use frame::deps::sp_core::Get;

UtkarshBhardwaj007 · 2025-01-02T13:19:19Z

substrate/frame/salary/Cargo.toml

-	"frame-benchmarking?/std",
-	"frame-support/experimental",
-	"frame-support/std",
-	"frame-system/std",
 	"log/std",
 	"pallet-ranked-collective/std",
 	"scale-info/std",
 	"sp-arithmetic/std",
 	"sp-core/std",


Suggested change

"sp-core/std",

UtkarshBhardwaj007 · 2025-01-02T13:19:30Z

substrate/frame/salary/Cargo.toml

-	"frame-benchmarking?/std",
-	"frame-support/experimental",
-	"frame-support/std",
-	"frame-system/std",
 	"log/std",
 	"pallet-ranked-collective/std",
 	"scale-info/std",
 	"sp-arithmetic/std",


Suggested change

"sp-arithmetic/std",

UtkarshBhardwaj007 · 2025-01-02T13:25:37Z

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

-};
+
+use frame::{prelude::*, runtime::prelude::*, testing_prelude::*,
+			traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll,


+1, to add to that - you might not need all these preludes. Try checking for each import and minimising the number of imports required.

UtkarshBhardwaj007 · 2025-01-02T13:26:32Z

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

-	traits::{Convert, ReduceBy, ReplaceWithDefault},
-	BuildStorage,
-};
+use crate::tests::integration::sp_api_hidden_includes_construct_runtime::hidden_include::hypothetically;


This import was originally from frame_support. Why not -

Suggested change

use crate::tests::integration::sp_api_hidden_includes_construct_runtime::hidden_include::hypothetically;

use frame::deps::frame_support::hypothetically;

or add it to one of the preludes where it makes sense?

UtkarshBhardwaj007 · 2025-01-02T13:28:18Z

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

-frame_support::construct_runtime!(
-	pub enum Test
+construct_runtime!(
+	pub struct Test


Could you explain a bit as to why did we have to change this from an enum to a struct?

UtkarshBhardwaj007 · 2025-01-02T13:29:13Z

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

@@ -145,9 +144,9 @@ impl pallet_ranked_collective::Config for Test {
 	type BenchmarkSetup = Salary;
 }

-pub fn new_test_ext() -> sp_io::TestExternalities {
+pub fn new_test_ext() -> TestState{


Could you please add some explanation here as well?

UtkarshBhardwaj007 · 2025-01-02T13:29:52Z

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

@@ -194,7 +193,7 @@ fn swap_exhaustive_works() {

 			// The events mess up the storage root:
 			System::reset_events();
-			sp_io::storage::root(sp_runtime::StateVersion::V1)
+			sp_io::storage::root( frame::deps::sp_runtime::StateVersion::V1)


Instead of using the whole path here, let's import this at the top and then use it.

paritytech-workflow-stopper · 2025-01-02T18:37:05Z

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

salary pallet migration to umbrella crate

1bf72c4

seemantaggarwal requested a review from a team as a code owner January 2, 2025 08:09

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 2, 2025

re-gius reviewed Jan 2, 2025

View reviewed changes

UtkarshBhardwaj007 requested changes Jan 2, 2025

View reviewed changes

cargo fmt

86658a2

seemantaggarwal requested review from acatangiu, sorpaas, koute and a team as code owners January 2, 2025 18:31

paritytech-review-bot bot requested a review from a team January 2, 2025 18:32

optimisations to salary pallet to use umbrella crate

207cf71

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

salary pallet migration to umbrella crate #7025

salary pallet migration to umbrella crate #7025

seemantaggarwal commented Jan 2, 2025

re-gius Jan 2, 2025

re-gius Jan 2, 2025

re-gius Jan 2, 2025

re-gius Jan 2, 2025

re-gius Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

UtkarshBhardwaj007 Jan 2, 2025

paritytech-workflow-stopper bot commented Jan 2, 2025

	use frame::benchmarking::v1::*;
	use frame::benchmarking::prelude::*;

	use crate::tests::integration::sp_api_hidden_includes_construct_runtime::hidden_include::hypothetically;
	use frame::deps::frame_support::hypothetically;

salary pallet migration to umbrella crate #7025

Are you sure you want to change the base?

salary pallet migration to umbrella crate #7025

Conversation

seemantaggarwal commented Jan 2, 2025

Description

Review Notes

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

paritytech-workflow-stopper bot commented Jan 2, 2025