Skip to content

Add M2 review comments #169

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.

Sign up for GitHub

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

Merged
vayesy merged 1 commit into from
Oct 28, 2022
Merged

Add M2 review comments #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
106 changes: 55 additions & 51 deletions bin/subzero/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -123,13 +122,8 @@ pub type UncheckedExtrinsic = generic::UncheckedExtrinsic<Address, Call, Signatu
pub type CheckedExtrinsic = generic::CheckedExtrinsic<AccountId, Call, SignedExtra>;

/// Executive: handles dispatch to the various modules.
pub type Executive = frame_executive::Executive<
Runtime,
Block,
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
>;
pub type Executive =
frame_executive::Executive<Runtime, Block, frame_system::ChainContext<Runtime>, 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
Expand Down Expand Up @@ -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
Expand All @@ -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! {
Expand Down Expand Up @@ -227,22 +224,22 @@ impl Contains<Call> 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 { .. })
)
}
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -399,7 +399,6 @@ impl pallet_child_bounties::Config for Runtime {
type WeightInfo = pallet_child_bounties::weights::SubstrateWeight<Runtime>;
}


parameter_types! {
pub const CouncilMotionDuration: BlockNumber = 5 * DAYS;
pub const CouncilMaxMembers: u32 = 100;
Expand Down Expand Up @@ -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<AccountId>;

impl pallet_collator_selection::Config for Runtime {
Expand Down Expand Up @@ -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<Origin, Option<u32>> for AssetAuthority {
type Success = ();
Expand Down Expand Up @@ -885,14 +888,16 @@ fn option_filter_keys_to_set<StringLimit: frame_support::traits::Get<u32>>(
Some(filter_keys) => {
let tree = filter_keys
.into_iter()
.map(|filter_keys| -> pallet_rmrk_rpc_runtime_api::Result<BoundedVec<u8, StringLimit>> {
filter_keys
.try_into()
.map_err(|_| DispatchError::Other("Can't read filter key"))
})
.map(
|filter_keys| -> pallet_rmrk_rpc_runtime_api::Result<BoundedVec<u8, StringLimit>> {
filter_keys
.try_into()
.map_err(|_| DispatchError::Other("Can't read filter key"))
},
)
.collect::<pallet_rmrk_rpc_runtime_api::Result<BTreeSet<_>>>()?;
Ok(Some(tree))
},
}
None => Ok(None),
}
}
Expand Down Expand Up @@ -1184,13 +1189,12 @@ impl cumulus_pallet_parachain_system::CheckInherents<Block> 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)
}
Expand Down
1 change: 1 addition & 0 deletions bin/zero/runtime/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Member

@vayesy vayesy Oct 27, 2022

Choose a reason for hiding this comment

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

It's actually original version of ORML without any modification. We just use it as a submodule, but github link should also work fine, thanks.

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 }
Expand Down
23 changes: 15 additions & 8 deletions modules/asset-registry/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -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::*;
Expand Down Expand Up @@ -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<T: Config> {
Expand Down Expand Up @@ -108,6 +109,7 @@ pub mod pallet {
impl<T: Config> GenesisBuild<T> for GenesisConfig<T> {
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::<T>::do_register_native_asset(
*asset,
&AssetMetadata {
Expand All @@ -123,7 +125,6 @@ pub mod pallet {

#[pallet::call]
impl<T: Config> Pallet<T> {

#[pallet::weight(T::WeightInfo::register_native_asset())]
#[transactional]
pub fn register_native_asset(
Expand All @@ -147,6 +148,7 @@ pub mod pallet {
pub fn update_native_asset(
origin: OriginFor<T>,
currency_id: CurrencyId,
// SBP-M2 review: why `Box` is used for extrinsic parameters?
metadata: Box<AssetMetadata<BalanceOf<T>>>,
) -> DispatchResult {
T::RegisterOrigin::ensure_origin(origin)?;
Expand All @@ -163,13 +165,14 @@ pub mod pallet {
}

impl<T: Config> Pallet<T> {

fn do_register_native_asset(asset: CurrencyId, metadata: &AssetMetadata<BalanceOf<T>>) -> DispatchResult {
AssetMetadatas::<T>::try_mutate(
AssetIds::NativeAssetId(asset),
|maybe_asset_metadatas| -> DispatchResult {
ensure!(maybe_asset_metadatas.is_none(), Error::<T>::AssetIdExisted);

// SBP-M2 review: without adding metadate to event
// You would not need to clone metadata here
*maybe_asset_metadatas = Some(metadata.clone());
Ok(())
},
Expand All @@ -178,12 +181,16 @@ impl<T: Config> Pallet<T> {
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<BalanceOf<T>>) -> DispatchResult {
AssetMetadatas::<T>::try_mutate(
AssetIds::NativeAssetId(currency_id),
|maybe_asset_metadatas| -> DispatchResult {
ensure!(maybe_asset_metadatas.is_some(), Error::<T>::AssetIdNotExists);

// SBP-M2 review: same as above
*maybe_asset_metadatas = Some(metadata.clone());
Ok(())
},
Expand Down
9 changes: 7 additions & 2 deletions modules/asset-registry/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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(
Expand All @@ -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(|| {
Expand Down
9 changes: 5 additions & 4 deletions modules/primitives/src/currency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -121,13 +121,12 @@ pub trait AssetIdMapping<ForeignAssetId, MultiLocation, AssetMetadata> {
fn get_currency_id(multi_location: MultiLocation) -> Option<CurrencyId>;
}


#[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 {
Expand All @@ -154,6 +153,8 @@ pub enum AssetIds {

#[derive(Clone, Eq, PartialEq, RuntimeDebug, Encode, Decode, TypeInfo)]
pub struct AssetMetadata<Balance> {
// SBP-M2 review: unbounded vectors are high security issues
// You should have limited length for name&symbol
pub name: Vec<u8>,
pub symbol: Vec<u8>,
pub decimals: u8,
Expand All @@ -162,5 +163,5 @@ pub struct AssetMetadata<Balance> {

#[derive(scale_info::TypeInfo, Encode, Decode, Clone, Eq, PartialEq, Debug)]
pub struct CustomMetadata {
pub fee_per_second: u128
pub fee_per_second: u128,
}