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

Migrating salary pallet to use umbrella crate #7048

Merged
merged 38 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
1bf72c4
salary pallet migration to umbrella crate
seemantaggarwal Jan 1, 2025
81ef8c3
selectiv changes for salary pallet from cargo fmt
seemantaggarwal Jan 6, 2025
12b5035
optimisations to salary pallet to use umbrella crate
seemantaggarwal Jan 5, 2025
9b5c229
changes to unit testing for migration to umbrella crate
seemantaggarwal Jan 6, 2025
6a6a009
Revision 5 addressing comments for umbrella crate migration
seemantaggarwal Jan 6, 2025
b015e26
remove formatting from traits
seemantaggarwal Jan 6, 2025
53675b3
Merge branch 'master' into new-salary-pallet
seemantaggarwal Jan 6, 2025
3291d1e
using defensive as part of prelude instead of frame_support
seemantaggarwal Jan 6, 2025
3dc4d8b
updating salary pallet migration to umbrella crate
seemantaggarwal Jan 7, 2025
bf3302b
run taplo and night fmt
seemantaggarwal Jan 7, 2025
5397c4c
Merge branch 'master' into new-salary-pallet
seemantaggarwal Jan 7, 2025
8db080b
add prdoc
seemantaggarwal Jan 7, 2025
c0b15a1
Merge branch 'master' into new-salary-pallet
seemantaggarwal Jan 7, 2025
b8c1985
cleaning unused imports
seemantaggarwal Jan 7, 2025
52a019b
cargo fmt
seemantaggarwal Jan 7, 2025
f1e1408
remove arithmetic from salary as it is already in the prelude
seemantaggarwal Jan 7, 2025
023c3fe
reverting taplo format changes from cargo.toml in salary pallet
seemantaggarwal Jan 7, 2025
6e83954
re formatting cargo.toml
seemantaggarwal Jan 7, 2025
eeb1fc5
re fixing cargo.toml spacing
seemantaggarwal Jan 7, 2025
e539109
Merge branch 'master' into new-salary-pallet
seemantaggarwal Jan 7, 2025
efeb7e6
running taplo format --config .config/taplo.toml substrate/frame/sala…
seemantaggarwal Jan 7, 2025
3f30ef6
changes for failing System and hypothetical import
seemantaggarwal Jan 7, 2025
4595acf
cargo fmt + applied patch on umbrella crate
seemantaggarwal Jan 7, 2025
49fb576
fixing stateversion and convertrank imports
seemantaggarwal Jan 7, 2025
63b251c
moving back convertrank to be handled in a separate issue
seemantaggarwal Jan 7, 2025
8da372e
fixing typo
seemantaggarwal Jan 7, 2025
a2cf833
importing back convertrank
seemantaggarwal Jan 8, 2025
dcf3995
cargo fmt nightly
seemantaggarwal Jan 8, 2025
5900660
extracting convert to implement separately later
seemantaggarwal Jan 8, 2025
b427d2a
re importing convert and convertrank to run successful tests
seemantaggarwal Jan 8, 2025
557bc71
Moving convert to prelude
seemantaggarwal Jan 8, 2025
de1b02f
removing unused import
seemantaggarwal Jan 8, 2025
4399f35
Merge branch 'master' into new-salary-pallet
seemantaggarwal Jan 8, 2025
ae9b6e2
excluding sp_runtime outside of umbrella crate
seemantaggarwal Jan 8, 2025
22ff507
patch applied from @kianenigma
seemantaggarwal Jan 8, 2025
9302fdc
reverting identity to not be included in the umbrella crate
seemantaggarwal Jan 9, 2025
e472261
Update pr_7048.prdoc
seemantaggarwal Jan 9, 2025
9116f5a
Merge branch 'master' into new-salary-pallet
seemantaggarwal Jan 9, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions prdoc/pr_7048.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: '[pallet-salary] Migrate to using frame umbrella crate'

doc:
- audience: Runtime Dev
description:
This PR migrates the pallet-salary to use the frame umbrella crate.
This is part of the ongoing effort to migrate all pallets to use the frame umbrella crate.
The effort is tracked [here](https://github.com/paritytech/polkadot-sdk/issues/6504).


crates:
- name: pallet-salary
bump: minor
- name: polkadot-sdk-frame
bump: minor
- name: traits-misc
bump: minor

39 changes: 9 additions & 30 deletions substrate/frame/salary/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,43 +17,22 @@ 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-io = { workspace = true }
sp-runtime = { workspace = true }
frame = { workspace = true, features = ["experimental", "runtime"] }

[features]
default = ["std"]
std = [
"codec/std",
"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",
"sp-io/std",
"sp-runtime/std",
"codec/std",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't change the indentation here, it also makes it harder to review. Probably it's a matter of the toml formatter you set up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup this is from taplo format

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The taplo workflow is failing indicating that the formatting is indeed not needed / incorrect here. You need to check all failed workflows before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taplo workflow is failing yes, but not for this file. let me revert it anyway, to a version before the taplo format

"log/std",
"pallet-ranked-collective/std",
"scale-info/std",
"frame/std",
]
runtime-benchmarks = [
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
"pallet-ranked-collective/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
]
try-runtime = [
"frame-support/try-runtime",
"frame-system/try-runtime",
"pallet-ranked-collective?/try-runtime",
"sp-runtime/try-runtime",
"pallet-ranked-collective/runtime-benchmarks",
"frame/runtime-benchmarks",
]
try-runtime = ["pallet-ranked-collective?/try-runtime", "frame/try-runtime"]
7 changes: 2 additions & 5 deletions substrate/frame/salary/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@
use super::*;
use crate::Pallet as Salary;

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

use frame::benchmarking::prelude::*;
seemantaggarwal marked this conversation as resolved.
Show resolved Hide resolved
const SEED: u32 = 0;

fn ensure_member_with_salary<T: Config<I>, I: 'static>(who: &T::AccountId) {
Expand All @@ -37,7 +34,7 @@ fn ensure_member_with_salary<T: Config<I>, I: 'static>(who: &T::AccountId) {
for _ in 0..255 {
let r = T::Members::rank_of(who).expect("prior guard ensures `who` is a member; qed");
if !T::Salary::get_salary(r, &who).is_zero() {
break
break;
seemantaggarwal marked this conversation as resolved.
Show resolved Hide resolved
}
T::Members::promote(who).unwrap();
}
Expand Down
23 changes: 8 additions & 15 deletions substrate/frame/salary/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,11 @@

#![cfg_attr(not(feature = "std"), no_std)]

use codec::{Decode, Encode, MaxEncodedLen};
use core::marker::PhantomData;
use scale_info::TypeInfo;
use sp_arithmetic::traits::{Saturating, Zero};
use sp_runtime::{Perbill, RuntimeDebug};

use frame_support::{
defensive,
dispatch::DispatchResultWithPostInfo,
ensure,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatic change by fmt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use frame::{
arithmetic::Perbill,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arithmetic is already in prelude.

- arithmetic::Perbill,

prelude::*,
traits::{
tokens::{GetSalary, Pay, PaymentStatus},
RankedMembers, RankedMembersSwapHandler,
Expand Down Expand Up @@ -85,11 +80,9 @@ pub struct ClaimantStatus<CycleIndex, Balance, Id> {
status: ClaimState<Balance, Id>,
}

#[frame_support::pallet]
#[frame::pallet]
pub mod pallet {
use super::*;
use frame_support::{dispatch::Pays, pallet_prelude::*};
use frame_system::pallet_prelude::*;

#[pallet::pallet]
pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
Expand Down Expand Up @@ -460,15 +453,15 @@ impl<T: Config<I>, I: 'static>
) {
if who == new_who {
defensive!("Should not try to swap with self");
return
return;
}
if Claimant::<T, I>::contains_key(new_who) {
defensive!("Should not try to overwrite existing claimant");
return
return;
}

let Some(claimant) = Claimant::<T, I>::take(who) else {
frame_support::defensive!("Claimant should exist when swapping");
defensive!("Claimant should exist when swapping");
return;
};

Expand Down
26 changes: 11 additions & 15 deletions substrate/frame/salary/src/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,21 @@

use crate as pallet_salary;
use crate::*;
use frame_support::{
assert_noop, assert_ok, derive_impl, hypothetically,
pallet_prelude::Weight,
parameter_types,
traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo fmt will put crate above other import by default

Suggested change

use frame::{
deps::{
sp_io::{self, MultiRemovalResults},
sp_runtime,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need sp_runtime?

sp_runtime::StateVersion should be enough

},
testing_prelude::*,
seemantaggarwal marked this conversation as resolved.
Show resolved Hide resolved
};
use pallet_ranked_collective::{EnsureRanked, Geometric};
use sp_core::{ConstU16, Get};
use sp_runtime::{
traits::{Convert, ReduceBy, ReplaceWithDefault},
BuildStorage,
};

type Rank = u16;
type Block = frame_system::mocking::MockBlock<Test>;

frame_support::construct_runtime!(
pub enum Test
{
construct_runtime!(
pub struct Test {
System: frame_system,
Salary: pallet_salary,
Club: pallet_ranked_collective,
Expand Down Expand Up @@ -145,9 +141,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 {
let t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();
let mut ext = sp_io::TestExternalities::new(t);
let mut ext = TestState::new(t);
ext.execute_with(|| System::set_block_number(1));
ext
}
Expand Down
18 changes: 6 additions & 12 deletions substrate/frame/salary/src/tests/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,14 @@
use std::collections::BTreeMap;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

use core::cell::RefCell;
use frame_support::{
assert_noop, assert_ok, derive_impl,
pallet_prelude::Weight,
parameter_types,
traits::{tokens::ConvertRank, ConstU64},
};
use sp_runtime::{traits::Identity, BuildStorage, DispatchResult};
use frame::testing_prelude::*;

use crate as pallet_salary;
use crate::*;

type Block = frame_system::mocking::MockBlock<Test>;
type Block = MockBlock<Test>;

frame_support::construct_runtime!(
construct_runtime!(
pub enum Test
{
System: frame_system,
Expand Down Expand Up @@ -124,7 +118,7 @@ impl RankedMembers for TestClub {
}
fn demote(who: &Self::AccountId) -> DispatchResult {
CLUB.with(|club| match club.borrow().get(who) {
None => Err(sp_runtime::DispatchError::Unavailable),
None => Err(DispatchError::Unavailable),
Some(&0) => {
club.borrow_mut().remove(&who);
Ok(())
Expand Down Expand Up @@ -156,9 +150,9 @@ impl Config for Test {
type Budget = Budget;
}

pub fn new_test_ext() -> sp_io::TestExternalities {
pub fn new_test_ext() -> TestState {
let t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();
let mut ext = sp_io::TestExternalities::new(t);
let mut ext = TestState::new(t);
ext.execute_with(|| System::set_block_number(1));
ext
}
Expand Down
4 changes: 3 additions & 1 deletion substrate/frame/salary/src/weights.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 11 additions & 8 deletions substrate/frame/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,12 @@ 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, EstimateNextSessionRotation, IsSubType, OnRuntimeUpgrade, OneSessionHandler,
pub use frame_support::{
defensive, defensive_assert,
traits::{
tokens::ConvertRank, Contains, EitherOf, EstimateNextSessionRotation, IsSubType,
MapSuccess, NoOpPoll, OnRuntimeUpgrade, OneSessionHandler,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
};

/// Pallet prelude of `frame-system`.
Expand All @@ -228,11 +232,10 @@ pub mod prelude {
/// Runtime traits
#[doc(no_inline)]
pub use sp_runtime::traits::{
BlockNumberProvider, Bounded, DispatchInfoOf, Dispatchable, SaturatedConversion,
Saturating, StaticLookup, TrailingZeroInput,
BlockNumberProvider, Bounded, Convert, DispatchInfoOf, Dispatchable, ReduceBy,
ReplaceWithDefault, SaturatedConversion, Saturating, StaticLookup, TrailingZeroInput,
};

/// Other runtime types and traits
/// Other error/result types for runtime
#[doc(no_inline)]
pub use sp_runtime::{
BoundToRuntimeAppPublic, DispatchErrorWithPostInfo, DispatchResultWithInfo, TokenError,
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, it is also there in testing_prelude.

};

pub use frame_system::{self, mocking::*};
Expand Down Expand Up @@ -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};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
6 changes: 3 additions & 3 deletions substrate/frame/support/src/traits/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good!

target: "runtime::defensive",
"{}",
$crate::traits::DEFENSIVE_OP_PUBLIC_ERROR
);
debug_assert!(false, "{}", $crate::traits::DEFENSIVE_OP_INTERNAL_ERROR);
};
($error:expr $(,)?) => {
frame_support::__private::log::error!(
$crate::__private::log::error!(
target: "runtime::defensive",
"{}: {:?}",
$crate::traits::DEFENSIVE_OP_PUBLIC_ERROR,
Expand All @@ -83,7 +83,7 @@ macro_rules! defensive {
debug_assert!(false, "{}: {:?}", $crate::traits::DEFENSIVE_OP_INTERNAL_ERROR, $error);
};
($error:expr, $proof:expr $(,)?) => {
frame_support::__private::log::error!(
$crate::__private::log::error!(
target: "runtime::defensive",
"{}: {:?}: {:?}",
$crate::traits::DEFENSIVE_OP_PUBLIC_ERROR,
Expand Down
Loading