From fb9edd2616bc6f2c53d6b6e28ecde2b27a32be50 Mon Sep 17 00:00:00 2001 From: "Alisher A. Khassanov" Date: Thu, 7 Dec 2023 10:39:01 +0600 Subject: [PATCH] Add TODOs to not implemented suggestions --- Cargo.toml | 1 + runtime/devnet/src/lib.rs | 10 ++++++++++ runtime/mainnet/src/lib.rs | 10 ++++++++++ 3 files changed, 21 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 5661ae1..1ee4dc4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,3 +137,4 @@ 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. +# TODO (@khssnv): zombienet added, consider parachains-integration-tests and chopsticks diff --git a/runtime/devnet/src/lib.rs b/runtime/devnet/src/lib.rs index e87d60f..3ac1a4a 100644 --- a/runtime/devnet/src/lib.rs +++ b/runtime/devnet/src/lib.rs @@ -370,9 +370,11 @@ impl pallet_assets::Config for Runtime { type Currency = Balances; // SBP-M1 review: consider whether anyone should be able to permissionlessly create an asset - // should probably be set to MQTY admin origin only. + // TODO (@khssnv): consider Asset originator or Verifier origin only type CreateOrigin = AsEnsureOriginWithArg>; // SBP-M1 review: may need to be root or MQTY admin origin to allow force_set_metadata for // fractionalised assets - see EitherOf. + // TODO (@khssnv): consider Verifier origin type ForceOrigin = EnsureRoot; type AssetDeposit = AssetDeposit; type AssetAccountDeposit = ConstU128<{ deposit(1, 16) }>; @@ -614,9 +616,11 @@ impl pallet_identity::Config for Runtime { type MaxAdditionalFields = ConstU32<100>; type MaxRegistrars = ConstU32<20>; // SBP-M1 review: consider what happens with slashed funds - e.g. treasury + // TODO (@khssnv) type Slashed = (); type ForceOrigin = EnsureRoot; // SBP-M1 review: should be EnsureRoot or MQTY admin origin to maintain registrar integrity + // TODO (@khssnv): consider Verifier origin type RegistrarOrigin = EnsureRoot; type WeightInfo = (); } @@ -634,8 +638,10 @@ impl pallet_nfts::Config for Runtime { type CollectionDeposit = ConstU128<{ 100 * MQTY }>; type ItemDeposit = ConstU128<{ 1 * MQTY }>; // SBP-M1 review: provide justification as to how 10 is determined. + // TODO (@khssnv): reconsider type MetadataDepositBase = ConstU128<{ 10 * MQTY }>; // SBP-M1 review: provide justification as to how 10 is determined. + // TODO (@khssnv): reconsider type AttributeDepositBase = ConstU128<{ 10 * MQTY }>; type DepositPerByte = ConstU128<{ deposit(0, 1) }>; type StringLimit = ConstU32<256>; @@ -657,6 +663,7 @@ impl pallet_nfts::Config for Runtime { // screenshots imply that asset verification is required, so assume the onchain creation of // collections should only be carried out by MQTY admin (e.g. configure CreateOrigin as MQTY // admin) and then assets (NFTs) minted by the collection admin once verified. + // TODO (@khssnv): consider Asset originator or Verifier origin only type CreateOrigin = AsEnsureOriginWithArg>; type Locker = (); } @@ -665,6 +672,7 @@ parameter_types! { pub const NftFractionalizationPalletId: PalletId = PalletId(*b"fraction"); pub FractionalizedAssetSymbol: BoundedVec = (*b"FRAC").to_vec().try_into().expect("bad nft-fractionalization asset symbol"); // SBP-M1 review: consider something more informative like 'Fractionalized Asset'. May not matter though, as it will probably require an assets::force_set_metadata to customise the fractionalized asset metadata after the NFT has been fractionalized. + // TODO (@khssnv) pub FractionalizedAssetName: BoundedVec = (*b"Frac").to_vec().try_into().expect("bad nft-fractionalization asset name"); } @@ -676,6 +684,7 @@ impl pallet_nft_fractionalization::Config for Runtime { // whether this is intentional. I see the Asset Hub on Kusama is configured this way though. // Suggest adding NftFractionalizationDeposit alias to AssetDeposit or NftsCollectionDeposit // with a comment as to why it is being used in your runtime for clarity. + // TODO (@khssnv): reconsider type Deposit = AssetDeposit; type NftCollectionId = ::CollectionId; type NftId = ::ItemId; @@ -755,6 +764,7 @@ mod benches { [pallet_nft_fractionalization, NftFractionalization] [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 ); } diff --git a/runtime/mainnet/src/lib.rs b/runtime/mainnet/src/lib.rs index a29d95c..73e3417 100644 --- a/runtime/mainnet/src/lib.rs +++ b/runtime/mainnet/src/lib.rs @@ -370,9 +370,11 @@ impl pallet_assets::Config for Runtime { type Currency = Balances; // SBP-M1 review: consider whether anyone should be able to permissionlessly create an asset - // should probably be set to MQTY admin origin only. + // TODO (@khssnv): consider Asset originator or Verifier origin only type CreateOrigin = AsEnsureOriginWithArg>; // SBP-M1 review: may need to be root or MQTY admin origin to allow force_set_metadata for // fractionalised assets - see EitherOf. + // TODO (@khssnv): consider Verifier origin type ForceOrigin = EnsureRoot; type AssetDeposit = AssetDeposit; type AssetAccountDeposit = ConstU128<{ deposit(1, 16) }>; @@ -614,9 +616,11 @@ impl pallet_identity::Config for Runtime { type MaxAdditionalFields = ConstU32<100>; type MaxRegistrars = ConstU32<20>; // SBP-M1 review: consider what happens with slashed funds - e.g. treasury + // TODO (@khssnv) type Slashed = (); type ForceOrigin = EnsureRoot; // SBP-M1 review: should be EnsureRoot or MQTY admin origin to maintain registrar integrity + // TODO (@khssnv): consider Verifier origin type RegistrarOrigin = EnsureRoot; type WeightInfo = (); } @@ -634,8 +638,10 @@ impl pallet_nfts::Config for Runtime { type CollectionDeposit = ConstU128<{ 100 * MQTY }>; type ItemDeposit = ConstU128<{ 1 * MQTY }>; // SBP-M1 review: provide justification as to how 10 is determined. + // TODO (@khssnv): reconsider type MetadataDepositBase = ConstU128<{ 10 * MQTY }>; // SBP-M1 review: provide justification as to how 10 is determined. + // TODO (@khssnv): reconsider type AttributeDepositBase = ConstU128<{ 10 * MQTY }>; type DepositPerByte = ConstU128<{ deposit(0, 1) }>; type StringLimit = ConstU32<256>; @@ -657,6 +663,7 @@ impl pallet_nfts::Config for Runtime { // screenshots imply that asset verification is required, so assume the onchain creation of // collections should only be carried out by MQTY admin (e.g. configure CreateOrigin as MQTY // admin) and then assets (NFTs) minted by the collection admin once verified. + // TODO (@khssnv): consider Asset originator or Verifier origin only type CreateOrigin = AsEnsureOriginWithArg>; type Locker = (); } @@ -665,6 +672,7 @@ parameter_types! { pub const NftFractionalizationPalletId: PalletId = PalletId(*b"fraction"); pub FractionalizedAssetSymbol: BoundedVec = (*b"FRAC").to_vec().try_into().expect("bad nft-fractionalization asset symbol"); // SBP-M1 review: consider something more informative like 'Fractionalized Asset'. May not matter though, as it will probably require an assets::force_set_metadata to customise the fractionalized asset metadata after the NFT has been fractionalized. + // TODO (@khssnv) pub FractionalizedAssetName: BoundedVec = (*b"Frac").to_vec().try_into().expect("bad nft-fractionalization asset name"); } @@ -676,6 +684,7 @@ impl pallet_nft_fractionalization::Config for Runtime { // whether this is intentional. I see the Asset Hub on Kusama is configured this way though. // Suggest adding NftFractionalizationDeposit alias to AssetDeposit or NftsCollectionDeposit // with a comment as to why it is being used in your runtime for clarity. + // TODO (@khssnv): reconsider type Deposit = AssetDeposit; type NftCollectionId = ::CollectionId; type NftId = ::ItemId; @@ -755,6 +764,7 @@ mod benches { [pallet_nft_fractionalization, NftFractionalization] [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 ); }