Merge pull request #43 from sbp-parity/sbp-m2-review

SBP-M2 review comments

.github/workflows/build.yml

@@ -26,6 +26,7 @@ jobs:
 
       - uses: actions/checkout@v3
 
+      # SBP-M2 review: use stable toolchain
       - name: Rust Setup
         uses: dtolnay/rust-toolchain@master
         with:
@@ -35,6 +36,7 @@ jobs:
       - name: Rustfmt
         uses: actions-rs/cargo@v1
         with:
+          # SBP-M2 review: use stable toolchain
           command: fmt
           args: --all --check
 

Cargo.toml

@@ -35,6 +35,7 @@ mainnet-runtime = { path = "runtime/mainnet" }
 runtime-common = { path = "runtime/common", default-features = false }
 
 # Substrate
+# SBP-M2 review: update to recent version of Polkadot SDK - v1.5 current release
 frame-benchmarking = { version = "24.0.0", default-features = false }
 frame-benchmarking-cli = { version = "28.0.0" }
 frame-executive = { version = "24.0.0", default-features = false }
@@ -139,4 +140,5 @@ xcm-builder = { package = "staging-xcm-builder", version = "3.0.0", default-feat
 xcm-executor = { package = "staging-xcm-executor", version = "3.0.0", default-features = false }
 
 # SBP-M1 review: add integration tests to ensure runtime functionality works as expected. Suggested tools include zombienet, parachains-integration-tests, chopsticks as applicable.
+# SBP-M2 review: some integration tests have been added in the upstream extended-parachain-template, consider adding along with some specific to your use cases
 # TODO (@khssnv): zombienet added, consider parachains-integration-tests and chopsticks

docs/rust-setup.md

@@ -9,6 +9,7 @@ source ~/.cargo/env
 Ubuntu: 
 ```sh
 sudo apt update
+# SBP-M2 review: missing protobuf-compiler
 sudo apt install -y cmake pkg-config libssl-dev git gcc build-essential git clang libclang-dev
 ```
 Arch Linux:
@@ -23,8 +24,10 @@ brew update
 brew install openssl cmake llvm
 ```
 
+# SBP-M2 review: use stable
 - ### Install the `wasm` target and the `nightly` toolchain for rust
 
+# SBP-M2 review: use stable
 ```sh
 rustup update nightly
 rustup target add wasm32-unknown-unknown --toolchain nightly

node/src/chain_spec.rs

@@ -312,6 +312,7 @@ pub mod mainnet {
 						get_account_id_from_seed::<sr25519::Public>("Eve//stash"),
 						get_account_id_from_seed::<sr25519::Public>("Ferdie//stash"),
 					],
+					// SBP-M2 review: consider using multisig for default mainnet config https://github.com/paritytech/extended-parachain-template/blob/2ef1fa882c5e1387ba581836c4899fc75080ef19/node/src/chain_spec.rs#L342
 					Some(get_account_id_from_seed::<sr25519::Public>("Alice")),
 					PARA_ID.into(),
 				)
@@ -368,6 +369,7 @@ pub mod mainnet {
 						get_account_id_from_seed::<sr25519::Public>("Eve//stash"),
 						get_account_id_from_seed::<sr25519::Public>("Ferdie//stash"),
 					],
+					// SBP-M2 review: consider using multisig for default mainnet config https://github.com/paritytech/extended-parachain-template/blob/2ef1fa882c5e1387ba581836c4899fc75080ef19/node/src/chain_spec.rs#L408
 					Some(get_account_id_from_seed::<sr25519::Public>("Alice")),
 					PARA_ID.into(),
 				)
@@ -408,6 +410,7 @@ pub mod mainnet {
 				..Default::default()
 			},
 			balances: mainnet_runtime::BalancesConfig {
+				// SBP-M1 review: ensure root_key funded
 				balances: endowed_accounts.iter().cloned().map(|k| (k, 1 << 60)).collect(),
 			},
 			// Configure two assets ALT1 & ALT2 with two owners, alice and bob respectively

runtime/devnet/src/lib.rs

@@ -66,6 +66,7 @@ use runtime_common::Nonce;
 use xcm::latest::prelude::BodyId;
 use xcm_executor::XcmExecutor;
 
+// SBP-M2 review: move type aliases to runtime/common to standardise across devnet/mainnet
 /// The address format for describing accounts.
 pub type Address = MultiAddress<AccountId, ()>;
 
@@ -109,6 +110,7 @@ pub type Executive = frame_executive::Executive<
 	AllPalletsWithSystem,
 >;
 
+// SBP-M2 review: duplicate module implementations, move to runtime/common to standardise across devnet/mainnet
 pub mod fee {
 	use super::{Balance, ExtrinsicBaseWeight, MILLIMQTY};
 	use frame_support::weights::{
@@ -218,12 +220,14 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	state_version: 1,
 };
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 pub const MICROMQTY: Balance = 1_000_000_000_000;
 pub const MILLIMQTY: Balance = 1_000 * MICROMQTY;
 pub const MQTY: Balance = 1_000 * MILLIMQTY;
 
 pub const EXISTENTIAL_DEPOSIT: Balance = MILLIMQTY;
 
+// SBP-M2 review: move to runtime/common to standardise across devnet/mainnet
 pub const fn deposit(items: u32, bytes: u32) -> Balance {
 	(items as Balance * 20 * MQTY + (bytes as Balance) * 100 * MICROMQTY) / 100
 }
@@ -234,6 +238,7 @@ pub fn native_version() -> NativeVersion {
 	NativeVersion { runtime_version: VERSION, can_author_with: Default::default() }
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const Version: RuntimeVersion = VERSION;
 
@@ -325,6 +330,7 @@ impl pallet_timestamp::Config for Runtime {
 	type WeightInfo = ();
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const UncleGenerations: u32 = 0;
 }
@@ -334,6 +340,7 @@ impl pallet_authorship::Config for Runtime {
 	type EventHandler = (CollatorSelection,);
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const ExistentialDeposit: Balance = EXISTENTIAL_DEPOSIT;
 }
@@ -356,6 +363,7 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ConstU32<0>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const AssetDeposit: Balance = 10 * MQTY;
 	pub const StringLimit: u32 = 50;
@@ -390,6 +398,7 @@ impl pallet_assets::Config for Runtime {
 	type BenchmarkHelper = ();
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	/// Relay Chain `TransactionByteFee` / 10
 	pub const TransactionByteFee: Balance = 10 * MICROMQTY;
@@ -413,11 +422,13 @@ impl pallet_utility::Config for Runtime {
 	type WeightInfo = pallet_utility::weights::SubstrateWeight<Runtime>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const ReservedXcmpWeight: Weight = MAXIMUM_BLOCK_WEIGHT.saturating_div(4);
 	pub const ReservedDmpWeight: Weight = MAXIMUM_BLOCK_WEIGHT.saturating_div(4);
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 /// Maximum number of blocks simultaneously accepted by the Runtime, not yet included
 /// into the relay chain.
 const UNINCLUDED_SEGMENT_CAPACITY: u32 = 1;
@@ -467,6 +478,7 @@ impl cumulus_pallet_dmp_queue::Config for Runtime {
 	type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub MaximumSchedulerWeight: Weight = Perbill::from_percent(80) *
 	RuntimeBlockWeights::get().max_block;
@@ -486,6 +498,7 @@ impl pallet_scheduler::Config for Runtime {
 	type Preimages = Preimage;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const Period: u32 = 6 * HOURS;
 	pub const Offset: u32 = 0;
@@ -511,6 +524,7 @@ impl pallet_sudo::Config for Runtime {
 	type WeightInfo = pallet_sudo::weights::SubstrateWeight<Runtime>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const CouncilMotionDuration: BlockNumber = 7 * DAYS;
 	pub const CouncilMaxProposals: u32 = 10;
@@ -531,6 +545,7 @@ impl pallet_collective::Config<CouncilCollective> for Runtime {
 	type MaxProposalWeight = MaxCollectivesProposalWeight;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	// One storage item; key size is 32; value is size 4+4+16+32 bytes = 56 bytes.
 	pub const DepositBase: Balance = deposit(1, 88);
@@ -548,6 +563,7 @@ impl pallet_multisig::Config for Runtime {
 	type WeightInfo = pallet_multisig::weights::SubstrateWeight<Runtime>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const PreimageBaseDeposit: Balance = deposit(2, 64);
 	pub const PreimageByteDeposit: Balance = deposit(0, 1);
@@ -574,6 +590,7 @@ impl pallet_aura::Config for Runtime {
 	type AllowMultipleBlocksPerSlot = ConstBool<false>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const PotId: PalletId = PalletId(*b"PotStake");
 	pub const MaxCandidates: u32 = 1000;
@@ -584,6 +601,7 @@ parameter_types! {
 	pub const StakingAdminBodyId: BodyId = BodyId::Defense;
 }
 
+// SBP-M2 review: move duplicate type alias to runtime/common to standardise across devnet/mainnet
 /// We allow root and the StakingAdmin to execute privileged collator selection operations.
 pub type CollatorSelectionUpdateOrigin = EitherOfDiverse<
 	EnsureRoot<AccountId>,
@@ -666,6 +684,7 @@ impl pallet_nfts::Config for Runtime {
 	type Locker = ();
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const NftFractionalizationPalletId: PalletId = PalletId(*b"fraction");
 	pub FractionalizedAssetSymbol: BoundedVec<u8, StringLimit> = (*b"FRAC").to_vec().try_into().expect("bad nft-fractionalization asset symbol");
@@ -699,6 +718,7 @@ impl pallet_nft_fractionalization::Config for Runtime {
 	type WeightInfo = pallet_nft_fractionalization::weights::SubstrateWeight<Runtime>;
 }
 
+// SBP-M2 review: move duplicate constants to runtime/common to standardise across devnet/mainnet
 parameter_types! {
 	pub const ProposalBond: Permill = Permill::from_percent(5);
 	pub const ProposalBondMinimum: Balance = 1 * MQTY;
@@ -813,6 +833,7 @@ mod benches {
 		[pallet_session, SessionBench::<Runtime>]
 		[pallet_scheduler, Scheduler]
 		[pallet_timestamp, Timestamp]
+		// SBP-M2 review: missing [pallet_treasury, Treasury] as per mainnet runtime
 		[pallet_collator_selection, CollatorSelection]
 		[pallet_multisig, Multisig]
 		[pallet_preimage, Preimage]
@@ -823,6 +844,7 @@ mod benches {
 		[cumulus_pallet_xcmp_queue, XcmpQueue]
 		// SBP-M1 review: add missing pallets: benchmarks should be re-run on reference hardware based on how they are configured/used by your runtime
 		// TODO (@khssnv): consider reference hardware and re-run benchmarks
+		// SBP-M2 review: add all pallets defined within construct_runtime! which have benchmarks defined - e.g. cumulus_pallet_parachain_system, treasury, sudo etc missing
 	);
 }