diff --git a/bin/subzero/runtime/src/lib.rs b/bin/subzero/runtime/src/lib.rs index b98a0e8283..a9b5c41a45 100644 --- a/bin/subzero/runtime/src/lib.rs +++ b/bin/subzero/runtime/src/lib.rs @@ -6,9 +6,9 @@ #[cfg(feature = "std")] include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs")); +pub mod constants; mod weights; pub mod xcm_config; -pub mod constants; use cumulus_pallet_parachain_system::RelayNumberStrictlyIncreases; use sp_api::impl_runtime_apis; @@ -28,18 +28,15 @@ use sp_version::RuntimeVersion; use frame_support::{ construct_runtime, parameter_types, traits::{ - tokens::nonfungibles::*, - AsEnsureOriginWithArg, ConstU32, Contains, EitherOfDiverse, EnsureOrigin, EnsureOriginWithArg - }, - weights::{ - constants::WEIGHT_PER_SECOND, ConstantMultiplier, DispatchClass, Weight, + tokens::nonfungibles::*, AsEnsureOriginWithArg, ConstU32, Contains, EitherOfDiverse, EnsureOrigin, + EnsureOriginWithArg, }, - BoundedVec, - PalletId, + weights::{constants::WEIGHT_PER_SECOND, ConstantMultiplier, DispatchClass, Weight}, + BoundedVec, PalletId, }; use frame_system::{ limits::{BlockLength, BlockWeights}, - EnsureRoot, EnsureSigned + EnsureRoot, EnsureSigned, }; pub use sp_consensus_aura::sr25519::AuthorityId as AuraId; pub use sp_runtime::{DispatchError, MultiAddress, Perbill, Permill}; @@ -57,9 +54,11 @@ use xcm::latest::prelude::BodyId; pub use constants::{fee::*, time::*}; pub use primitives::{ - currency::{ZERO, PLAY, GAME, DOT, AssetIds, AssetIdMapping, CurrencyId, CustomMetadata, ForeignAssetId, TokenSymbol}, - dollar, cent, millicent, - Amount, ReserveIdentifier + cent, + currency::{ + AssetIdMapping, AssetIds, CurrencyId, CustomMetadata, ForeignAssetId, TokenSymbol, DOT, GAME, PLAY, ZERO, + }, + dollar, millicent, Amount, ReserveIdentifier, }; use orml_asset_registry::SequentialId; @@ -123,13 +122,8 @@ pub type UncheckedExtrinsic = generic::UncheckedExtrinsic; /// Executive: handles dispatch to the various modules. -pub type Executive = frame_executive::Executive< - Runtime, - Block, - frame_system::ChainContext, - Runtime, - AllPalletsWithSystem, ->; +pub type Executive = + frame_executive::Executive, Runtime, AllPalletsWithSystem>; /// Opaque types. These are used by the CLI to instantiate machinery that don't need to know /// the specifics of the runtime. They can then be made to be agnostic over specific formats @@ -163,7 +157,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, - state_version: 1 + state_version: 1, }; // Unit = the base number of indivisible units for balances @@ -188,7 +182,10 @@ const MAXIMUM_BLOCK_WEIGHT: Weight = WEIGHT_PER_SECOND / 2; /// The version information used to identify this runtime when compiled natively. #[cfg(feature = "std")] pub fn native_version() -> NativeVersion { - NativeVersion { runtime_version: VERSION, can_author_with: Default::default() } + NativeVersion { + runtime_version: VERSION, + can_author_with: Default::default(), + } } parameter_types! { @@ -227,22 +224,22 @@ impl Contains for BaseFilter { // Disable direct calls to pallet_uniques !matches!( call, - Call::Uniques(pallet_uniques::Call::approve_transfer { .. }) | - Call::Uniques(pallet_uniques::Call::burn { .. }) | - Call::Uniques(pallet_uniques::Call::cancel_approval { .. }) | - Call::Uniques(pallet_uniques::Call::clear_collection_metadata { .. }) | - Call::Uniques(pallet_uniques::Call::clear_metadata { .. }) | - Call::Uniques(pallet_uniques::Call::create { .. }) | - Call::Uniques(pallet_uniques::Call::destroy { .. }) | - Call::Uniques(pallet_uniques::Call::force_item_status { .. }) | - Call::Uniques(pallet_uniques::Call::force_create { .. }) | - Call::Uniques(pallet_uniques::Call::freeze_collection { .. }) | - Call::Uniques(pallet_uniques::Call::mint { .. }) | - Call::Uniques(pallet_uniques::Call::redeposit { .. }) | - Call::Uniques(pallet_uniques::Call::set_collection_metadata { .. }) | - Call::Uniques(pallet_uniques::Call::thaw_collection { .. }) | - Call::Uniques(pallet_uniques::Call::transfer { .. }) | - Call::Uniques(pallet_uniques::Call::transfer_ownership { .. }) + Call::Uniques(pallet_uniques::Call::approve_transfer { .. }) + | Call::Uniques(pallet_uniques::Call::burn { .. }) + | Call::Uniques(pallet_uniques::Call::cancel_approval { .. }) + | Call::Uniques(pallet_uniques::Call::clear_collection_metadata { .. }) + | Call::Uniques(pallet_uniques::Call::clear_metadata { .. }) + | Call::Uniques(pallet_uniques::Call::create { .. }) + | Call::Uniques(pallet_uniques::Call::destroy { .. }) + | Call::Uniques(pallet_uniques::Call::force_item_status { .. }) + | Call::Uniques(pallet_uniques::Call::force_create { .. }) + | Call::Uniques(pallet_uniques::Call::freeze_collection { .. }) + | Call::Uniques(pallet_uniques::Call::mint { .. }) + | Call::Uniques(pallet_uniques::Call::redeposit { .. }) + | Call::Uniques(pallet_uniques::Call::set_collection_metadata { .. }) + | Call::Uniques(pallet_uniques::Call::thaw_collection { .. }) + | Call::Uniques(pallet_uniques::Call::transfer { .. }) + | Call::Uniques(pallet_uniques::Call::transfer_ownership { .. }) ) } } @@ -357,6 +354,9 @@ impl pallet_transaction_payment::Config for Runtime { type OperationalFeeMultiplier = OperationalFeeMultiplier; } +// SBP-M2 review: get rid of pallet_sudo +// Sudo actions deny true decentralization +// Use mechanisms like council instead. impl pallet_sudo::Config for Runtime { type Event = Event; type Call = Call; @@ -399,7 +399,6 @@ impl pallet_child_bounties::Config for Runtime { type WeightInfo = pallet_child_bounties::weights::SubstrateWeight; } - parameter_types! { pub const CouncilMotionDuration: BlockNumber = 5 * DAYS; pub const CouncilMaxMembers: u32 = 100; @@ -605,6 +604,8 @@ parameter_types! { } // We allow root only to execute privileged collator selection operations. +// SBP-M2 review: you should have an entity like a council +// instead of providing the whole power to the single user (Root account) pub type CollatorSelectionUpdateOrigin = EnsureRoot; impl pallet_collator_selection::Config for Runtime { @@ -659,6 +660,8 @@ parameter_type_with_key! { } /// Allow asset registration only from root origin +// SBP-M2 review: root account has too much power +// Need for a council pub struct AssetAuthority; impl EnsureOriginWithArg> for AssetAuthority { type Success = (); @@ -885,14 +888,16 @@ fn option_filter_keys_to_set>( Some(filter_keys) => { let tree = filter_keys .into_iter() - .map(|filter_keys| -> pallet_rmrk_rpc_runtime_api::Result> { - filter_keys - .try_into() - .map_err(|_| DispatchError::Other("Can't read filter key")) - }) + .map( + |filter_keys| -> pallet_rmrk_rpc_runtime_api::Result> { + filter_keys + .try_into() + .map_err(|_| DispatchError::Other("Can't read filter key")) + }, + ) .collect::>>()?; Ok(Some(tree)) - }, + } None => Ok(None), } } @@ -1184,13 +1189,12 @@ impl cumulus_pallet_parachain_system::CheckInherents for CheckInherents { .read_slot() .expect("Could not read the relay chain slot from the proof"); - let inherent_data = - cumulus_primitives_timestamp::InherentDataProvider::from_relay_chain_slot_and_duration( - relay_chain_slot, - sp_std::time::Duration::from_secs(6), - ) - .create_inherent_data() - .expect("Could not create the timestamp inherent data"); + let inherent_data = cumulus_primitives_timestamp::InherentDataProvider::from_relay_chain_slot_and_duration( + relay_chain_slot, + sp_std::time::Duration::from_secs(6), + ) + .create_inherent_data() + .expect("Could not create the timestamp inherent data"); inherent_data.check_extrinsics(block) } diff --git a/bin/zero/runtime/Cargo.toml b/bin/zero/runtime/Cargo.toml index a35a76ae27..e63b5c6090 100644 --- a/bin/zero/runtime/Cargo.toml +++ b/bin/zero/runtime/Cargo.toml @@ -81,6 +81,7 @@ parachain-info = { git = "https://github.com/paritytech/cumulus", default-featur primitives = { version = "2.0.0", package = "zero-primitives", default-features = false, path = "../../../modules/primitives" } # ORML +# SBP-M2 review: why you use your copy of orml instead of following main implementation? orml-asset-registry = { path = "../../../modules/orml/asset-registry", default-features = false } orml-currencies = { path = "../../../modules/orml/currencies", default-features = false } orml-tokens = { path = "../../../modules/orml/tokens", default-features = false } diff --git a/modules/asset-registry/src/lib.rs b/modules/asset-registry/src/lib.rs index 30ce8b4772..f315864abd 100644 --- a/modules/asset-registry/src/lib.rs +++ b/modules/asset-registry/src/lib.rs @@ -4,6 +4,7 @@ #![cfg_attr(not(feature = "std"), no_std)] #![allow(clippy::unused_unit)] +// SBP-M2 review: #TODO comment #![allow(deprecated)] // TODO: clean transactional use frame_support::{ @@ -16,18 +17,15 @@ use frame_support::{ }; use frame_system::pallet_prelude::*; use primitives::{ - currency::{ - AssetIds, AssetMetadata, TokenInfo, - }, + currency::{AssetIds, AssetMetadata, TokenInfo}, CurrencyId, }; use sp_std::{boxed::Box, vec::Vec}; - -mod mock; -mod tests; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; +mod mock; +mod tests; pub mod weights; pub use pallet::*; @@ -63,6 +61,9 @@ pub mod pallet { AssetIdExisted, } + // SBP-M2 review: you should not include asset metadata in event while it can contain huge data + // amount + // If someone is interested there should be possibility to query it from chain state #[pallet::event] #[pallet::generate_deposit(fn deposit_event)] pub enum Event { @@ -108,6 +109,7 @@ pub mod pallet { impl GenesisBuild for GenesisConfig { fn build(&self) { self.assets.iter().for_each(|(asset, ed)| { + // SBP-M2 review: consider adding some log when sth go wrong during genesis build assert_ok!(Pallet::::do_register_native_asset( *asset, &AssetMetadata { @@ -123,7 +125,6 @@ pub mod pallet { #[pallet::call] impl Pallet { - #[pallet::weight(T::WeightInfo::register_native_asset())] #[transactional] pub fn register_native_asset( @@ -147,6 +148,7 @@ pub mod pallet { pub fn update_native_asset( origin: OriginFor, currency_id: CurrencyId, + // SBP-M2 review: why `Box` is used for extrinsic parameters? metadata: Box>>, ) -> DispatchResult { T::RegisterOrigin::ensure_origin(origin)?; @@ -163,13 +165,14 @@ pub mod pallet { } impl Pallet { - fn do_register_native_asset(asset: CurrencyId, metadata: &AssetMetadata>) -> DispatchResult { AssetMetadatas::::try_mutate( AssetIds::NativeAssetId(asset), |maybe_asset_metadatas| -> DispatchResult { ensure!(maybe_asset_metadatas.is_none(), Error::::AssetIdExisted); + // SBP-M2 review: without adding metadate to event + // You would not need to clone metadata here *maybe_asset_metadatas = Some(metadata.clone()); Ok(()) }, @@ -178,12 +181,16 @@ impl Pallet { Ok(()) } + // SBP-M2 review: these two functions look almost the same + // Think about merging them into single one with possibility to provide appropriate check + // mechanism fn do_update_native_asset(currency_id: CurrencyId, metadata: &AssetMetadata>) -> DispatchResult { AssetMetadatas::::try_mutate( AssetIds::NativeAssetId(currency_id), |maybe_asset_metadatas| -> DispatchResult { ensure!(maybe_asset_metadatas.is_some(), Error::::AssetIdNotExists); + // SBP-M2 review: same as above *maybe_asset_metadatas = Some(metadata.clone()); Ok(()) }, diff --git a/modules/asset-registry/src/tests.rs b/modules/asset-registry/src/tests.rs index c950b34d10..ed4ccb063a 100644 --- a/modules/asset-registry/src/tests.rs +++ b/modules/asset-registry/src/tests.rs @@ -1,17 +1,19 @@ #![cfg(test)] use super::*; -use mock::{*, Event}; use frame_support::{assert_noop, assert_ok}; use frame_system::RawOrigin; +use mock::{Event, *}; use primitives::TokenSymbol; - #[test] fn register_native_asset_works() { new_test_ext().execute_with(|| { System::set_block_number(3); + // SBP-M2 review: you can create metadate once and then compare to copy instead of + // initalization of 2 the same structs. assert_ok!(AssetRegistry::register_native_asset( + // SBP-M2 review: you should provide entity like council to get rid of sudo calls RawOrigin::Root.into(), CurrencyId::Token(TokenSymbol::DOT), Box::new(AssetMetadata { @@ -40,6 +42,8 @@ fn register_native_asset_works() { minimal_balance: 1, }) ); + // SBP-M2 review: it should be another test case + // Try to follow assumption - 1 test case per simple check // Can't duplicate assert_noop!( AssetRegistry::register_native_asset( @@ -57,6 +61,7 @@ fn register_native_asset_works() { }); } +// SBP-M2 review: above comments should be applied here #[test] fn update_native_asset_works() { new_test_ext().execute_with(|| { diff --git a/modules/primitives/src/currency.rs b/modules/primitives/src/currency.rs index abcc329aeb..8bb4f1e79d 100644 --- a/modules/primitives/src/currency.rs +++ b/modules/primitives/src/currency.rs @@ -5,7 +5,7 @@ use codec::{Decode, Encode, MaxEncodedLen}; use num_enum::{IntoPrimitive, TryFromPrimitive}; use scale_info::TypeInfo; use sp_runtime::RuntimeDebug; -use sp_std::{prelude::*, convert::TryFrom}; +use sp_std::{convert::TryFrom, prelude::*}; #[cfg(feature = "std")] use serde::{Deserialize, Serialize}; @@ -121,13 +121,12 @@ pub trait AssetIdMapping { fn get_currency_id(multi_location: MultiLocation) -> Option; } - #[derive(Encode, Decode, Eq, PartialEq, Copy, Clone, RuntimeDebug, PartialOrd, Ord, TypeInfo, MaxEncodedLen)] #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[cfg_attr(feature = "std", serde(rename_all = "camelCase"))] pub enum CurrencyId { Token(TokenSymbol), - ForeignAsset(ForeignAssetId) + ForeignAsset(ForeignAssetId), } impl CurrencyId { @@ -154,6 +153,8 @@ pub enum AssetIds { #[derive(Clone, Eq, PartialEq, RuntimeDebug, Encode, Decode, TypeInfo)] pub struct AssetMetadata { + // SBP-M2 review: unbounded vectors are high security issues + // You should have limited length for name&symbol pub name: Vec, pub symbol: Vec, pub decimals: u8, @@ -162,5 +163,5 @@ pub struct AssetMetadata { #[derive(scale_info::TypeInfo, Encode, Decode, Clone, Eq, PartialEq, Debug)] pub struct CustomMetadata { - pub fee_per_second: u128 + pub fee_per_second: u128, }