From 4421b671967b5f85e704e4b145d87763188195c5 Mon Sep 17 00:00:00 2001 From: Pawan Singh Bisht Date: Tue, 16 May 2023 20:16:41 +0530 Subject: [PATCH] SBP-M2 review --- pallets/afloat/README.md | 3 ++- pallets/afloat/src/benchmarking.rs | 2 ++ pallets/afloat/src/functions.rs | 10 ++++++-- pallets/afloat/src/lib.rs | 11 +++++--- pallets/afloat/src/mock.rs | 1 + pallets/afloat/src/tests.rs | 1 + pallets/bitcoin-vaults/src/benchmarking.rs | 1 + pallets/bitcoin-vaults/src/functions.rs | 21 ++++++++++++++++ pallets/bitcoin-vaults/src/lib.rs | 29 ++++++++++++++++++++++ pallets/bitcoin-vaults/src/types.rs | 5 ++++ pallets/fruniques/src/benchmarking.rs | 2 ++ pallets/fruniques/src/functions.rs | 15 +++++++++++ pallets/fruniques/src/lib.rs | 6 +++++ pallets/fruniques/src/mock.rs | 2 ++ pallets/fruniques/src/tests.rs | 1 + pallets/fruniques/src/types.rs | 2 ++ pallets/gated-marketplace/src/functions.rs | 19 ++++++++++---- pallets/gated-marketplace/src/lib.rs | 18 ++++++++++++++ pallets/mapped-assets/Cargo.toml | 1 + pallets/mapped-assets/src/lib.rs | 4 +-- pallets/rbac/Cargo.toml | 1 + pallets/rbac/src/benchmarking.rs | 1 + pallets/rbac/src/functions.rs | 3 ++- pallets/rbac/src/lib.rs | 1 + 24 files changed, 146 insertions(+), 14 deletions(-) diff --git a/pallets/afloat/README.md b/pallets/afloat/README.md index 8d751a422..b99f30675 100644 --- a/pallets/afloat/README.md +++ b/pallets/afloat/README.md @@ -1 +1,2 @@ -License: Unlicense \ No newline at end of file +// SBP-M2 review: Update readme +License: Unlicense diff --git a/pallets/afloat/src/benchmarking.rs b/pallets/afloat/src/benchmarking.rs index d496a9fc8..405f043b2 100644 --- a/pallets/afloat/src/benchmarking.rs +++ b/pallets/afloat/src/benchmarking.rs @@ -1,3 +1,5 @@ +//SBP-M2 review: Implement benchmarking for all the extrinsics, it is must before going to production + //! Benchmarking setup for pallet-template use super::*; diff --git a/pallets/afloat/src/functions.rs b/pallets/afloat/src/functions.rs index bb51d0a99..b6eaa938e 100644 --- a/pallets/afloat/src/functions.rs +++ b/pallets/afloat/src/functions.rs @@ -7,8 +7,11 @@ use pallet_gated_marketplace::types::MarketplaceRole; use pallet_fruniques::types::CollectionDescription; use pallet_fruniques::types::FruniqueRole; use frame_support::pallet_prelude::*; + +// SBP-M2 review: Please remove this commented line // use frame_support::traits::OriginTrait; +// SBP-M2 review: Please remove unwraps and manage error in all places impl Pallet { pub fn do_initial_setup(creator: T::AccountId, admin: T::AccountId) -> DispatchResult { let creator_user: User = User { @@ -210,7 +213,7 @@ impl Pallet { /// pub fn do_delete_user(_actor: T::AccountId, user_address: T::AccountId) -> DispatchResult { ensure!(>::contains_key(user_address.clone()), Error::::UserNotFound); - + Self::remove_from_afloat_collection(user_address.clone(), FruniqueRole::Collaborator)?; Self::remove_from_afloat_marketplace(user_address.clone())?; @@ -221,7 +224,7 @@ impl Pallet { pub fn create_afloat_collection(origin: OriginFor, metadata: CollectionDescription, - admin: T::AccountId, ) -> DispatchResult + admin: T::AccountId, ) -> DispatchResult where ::CollectionId: From, { @@ -240,6 +243,7 @@ impl Pallet { } pub fn add_to_afloat_collection(invitee: T::AccountId, role: FruniqueRole) -> DispatchResult { + // SBP-M2 review: Please remove unwrap() and manage error properly let collection_id = AfloatCollectionId::::get().unwrap(); pallet_fruniques::Pallet::::insert_auth_in_frunique_collection(invitee, collection_id, @@ -248,6 +252,7 @@ impl Pallet { } pub fn remove_from_afloat_collection(invitee: T::AccountId, role: FruniqueRole) -> DispatchResult { + // SBP-M2 review: Please remove unwrap() and manage error properly let collection_id = AfloatCollectionId::::get().unwrap(); pallet_fruniques::Pallet::::remove_auth_from_frunique_collection(invitee, collection_id, @@ -256,6 +261,7 @@ impl Pallet { } pub fn remove_from_afloat_marketplace(invitee: T::AccountId) -> DispatchResult { + // SBP-M2 review: Please remove unwrap() and manage error properly let marketplace_id = AfloatMarketPlaceId::::get().unwrap(); pallet_gated_marketplace::Pallet::::remove_from_market_lists(invitee, MarketplaceRole::Participant, marketplace_id) } diff --git a/pallets/afloat/src/lib.rs b/pallets/afloat/src/lib.rs index c5d1bc220..385ae4449 100644 --- a/pallets/afloat/src/lib.rs +++ b/pallets/afloat/src/lib.rs @@ -103,8 +103,9 @@ pub mod pallet { ::CollectionId, // Afloat's frunique collection id >; + // SBP-M2 review: Missing extrinsic documentation and code is not well formatted #[pallet::call] - impl Pallet + impl Pallet where T: pallet_uniques::Config { @@ -120,6 +121,8 @@ pub mod pallet { ensure_signed(origin.clone())?; let asset_id: T::AssetId = Default::default(); let min_balance: T::Balance = T::Balance::from(1u32); + // SBP-M2 review: Returning error is a good approach instead of panic. + // Suggestion: A generic error enum for conversions can be used throughout the code let metadata: CollectionDescription = BoundedVec::try_from(b"Afloat".to_vec()).expect("Label too long"); pallet_mapped_assets::Pallet::::create( @@ -127,10 +130,10 @@ pub mod pallet { asset_id, T::Lookup::unlookup(creator.clone()), min_balance, - )?; + )?; pallet_fruniques::Pallet::::do_initial_setup()?; - + Self::create_afloat_collection(RawOrigin::Signed(creator.clone()).into(), metadata, admin.clone())?; pallet_gated_marketplace::Pallet::::do_initial_setup()?; @@ -158,6 +161,7 @@ pub mod pallet { pub fn kill_storage(origin: OriginFor) -> DispatchResult { ensure_signed(origin.clone())?; >::kill(); + // SBP-M2 review: Remove let _, use ? operator instead let _ = >::clear(1000, None); Ok(()) } @@ -176,6 +180,7 @@ pub mod pallet { address: T::AccountId, args: UpdateUserArgs, ) -> DispatchResult { + // SBP-M2 review: Pleaser resolve this // TODO: Check if the user is editing himself or is an admin let who = ensure_signed(origin)?; diff --git a/pallets/afloat/src/mock.rs b/pallets/afloat/src/mock.rs index ffb891d77..54de15bf1 100644 --- a/pallets/afloat/src/mock.rs +++ b/pallets/afloat/src/mock.rs @@ -62,6 +62,7 @@ impl system::Config for Test { type MaxConsumers = frame_support::traits::ConstU32<16>; } +// SBP-M2 review: Not up to date with config impl pallet_afloat::Config for Test { type RuntimeEvent = RuntimeEvent; type Moment = u64; diff --git a/pallets/afloat/src/tests.rs b/pallets/afloat/src/tests.rs index bc3b5fc27..f5d4ac121 100644 --- a/pallets/afloat/src/tests.rs +++ b/pallets/afloat/src/tests.rs @@ -1,2 +1,3 @@ +// SBP-M2 review: No test cases use crate::{mock::*, Error}; use frame_support::{assert_noop, assert_ok}; diff --git a/pallets/bitcoin-vaults/src/benchmarking.rs b/pallets/bitcoin-vaults/src/benchmarking.rs index 95363a16e..d799be12b 100644 --- a/pallets/bitcoin-vaults/src/benchmarking.rs +++ b/pallets/bitcoin-vaults/src/benchmarking.rs @@ -1,3 +1,4 @@ +//SBP-M2 review: Implement benchmarking for all the extrinsics, it is must before going to production //! Benchmarking setup for pallet-template use super::*; diff --git a/pallets/bitcoin-vaults/src/functions.rs b/pallets/bitcoin-vaults/src/functions.rs index 0a814a4e8..b8fe9d359 100644 --- a/pallets/bitcoin-vaults/src/functions.rs +++ b/pallets/bitcoin-vaults/src/functions.rs @@ -21,7 +21,9 @@ impl Pallet { Ok(()) } + // SBP-M2 review: No need to use clone on vault_id, as [u8; 32] implements copy trait. pub fn do_insert_vault(vault: Vault) -> DispatchResult { + // SBP-M2 review: Please resolve TODOs //TODO vault_id exist? // generate vault id ensure!(vault.signers_are_unique(), Error::::DuplicateVaultMembers); @@ -34,6 +36,7 @@ impl Pallet { if !>::contains_key(acc.clone()) { return Err(Error::::XPubNotFound); } + >::try_mutate(acc, |vault_vec| vault_vec.try_push(vault_id.clone())) .map_err(|_| Error::::SignerVaultLimit) })?; @@ -98,6 +101,7 @@ impl Pallet { pub fn do_propose(proposal: Proposal) -> DispatchResult { Self::vault_comprobations(proposal.vault_id, &proposal.proposer)?; let proposal_id = proposal.using_encoded(blake2_256); + // SBP-M2 review: No need to use reference as [u8; 32] implements copy trait ensure!(!>::contains_key(&proposal_id), Error::::AlreadyProposed); >::insert(proposal_id, proposal.clone()); >::try_mutate(proposal.vault_id, |proposals| { @@ -123,6 +127,8 @@ impl Pallet { >::try_mutate::<_, (), DispatchError, _>(proposal_id, |proposal| { proposal.as_ref().ok_or(Error::::ProposalNotFound)?; if let Some(p) = proposal { + // SBP-M2 review: Try to incorporate any() instead + // SBP-M2 reivew: Suggestion: p.signed_psbts.iter().any(|&signature| signature.signer == signer); let signed_already = p.signed_psbts.iter().find(|&signature| signature.signer == signer).is_some(); ensure!(!signed_already, Error::::AlreadySigned); @@ -150,6 +156,8 @@ impl Pallet { ensure!(proposal.offchain_status.eq(&BDKStatus::Valid), Error::::InvalidProposal); // can be called by any vault signer ensure!(vault.is_vault_member(&signer), Error::::SignerPermissionsNeeded); + + //SBP-M2 review: As per this comment why we are not returning error if it's Finalized? // if its finalized then fire error "already finalized" or "already broadcasted" ensure!( proposal.status.eq(&ProposalStatus::Pending) @@ -205,6 +213,8 @@ impl Pallet { >::try_mutate::<_, (), DispatchError, _>(vault_id, |maybe_proof| { maybe_proof.as_ref().ok_or(Error::::ProofNotFound)?; if let Some(proof) = maybe_proof { + // SBP-M2 review: Try to incorporate any() instead + // SBP-M2 review: Suggestion: proof.signed_psbts.iter().any(|&signature| signature.signer == signer) let signed_already = proof .signed_psbts .iter() @@ -216,6 +226,7 @@ impl Pallet { .try_push(signature) .map_err(|_| Error::::ExceedMaxCosignersPerVault)?; // this should never fail, earlier vault comprobations ensure it: + // SBP-M2 review: Still we can incorporate ? (operator) over unwrap as this function returns DispatchResult if proof.signed_psbts.len() as u32 >= >::get(vault_id).unwrap().threshold { // set ReadyToFinalize when the threshold is reached @@ -257,6 +268,7 @@ impl Pallet { // check if the xpub is free to take/update or if its owned by the account pub fn get_xpub_status(who: T::AccountId, xpub_hash: [u8; 32]) -> XpubStatus { if >::contains_key(xpub_hash) { + // SBP-M2 review: No need to use clone() if let Some(owned_hash) = >::get(who.clone()) { match xpub_hash == owned_hash { true => return XpubStatus::Owned, @@ -334,6 +346,7 @@ impl Pallet { } /*---- Offchain utilities ----*/ + // SBP-M2 review: Reusing the type which should be created for key pub fn get_pending_vaults() -> Vec<[u8; 32]> { >::iter() .filter_map(|(entry, vault)| { @@ -399,6 +412,8 @@ impl Pallet { .collect() } + // SBP-M2 review: Remove commented lines + // pub fn get_proposals_to_broadcast()-> Vec<[u8;32]>{ // // offchain status == valid and proposal status ready to ReadyToBroadcast // >::iter().filter_map(| (id,p) |{ @@ -478,6 +493,7 @@ impl Pallet { let xpubs = Self::get_accounts_xpubs(vault_signers).map_err(|_| { Self::build_offchain_err(false, "One of the cosigner xpubs wasn't found") })?; + // SBP-M2 review: Code should not contain unwrap(), error handling is must let mapped_xpubs: Vec = xpubs .iter() .map(|xpub| { @@ -537,6 +553,7 @@ impl Pallet { Self::parse_vault_descriptors(body_str) } + // SBP-M2 review: Redundant cloning on vault_to_complete pub fn gen_vaults_payload_by_bulk(pending_vaults: Vec<[u8; 32]>) -> Vec { let mut generated_vaults = Vec::::new(); pending_vaults.iter().for_each(|vault_to_complete| { @@ -561,6 +578,7 @@ impl Pallet { generated_vaults } + // SBP-M2 review: Redundant cloning on multiple types pub fn gen_proposal_json_body(proposal_id: [u8; 32]) -> Result, OffchainStatus> { let mut body = Vec::new(); let proposal = >::get(proposal_id) @@ -636,6 +654,7 @@ impl Pallet { Ok(response_body) } + // SBP-M2 review: Redundant cloning on multiple types pub fn gen_proposals_payload_by_bulk( pending_proposals: Vec<[u8; 32]>, api_endpoint: Vec, @@ -712,6 +731,8 @@ impl Pallet { Ok(jsonSerialize::format(&json_object, 4)) } + // SBP-M2 review: Please remove commented code + // pub fn bdk_gen_finalized_proposal(proposal_id: [u8;32])-> Result,OffchainStatus >{ // let raw_json = Self::gen_finalize_json_body(proposal_id)?; // let request_body = diff --git a/pallets/bitcoin-vaults/src/lib.rs b/pallets/bitcoin-vaults/src/lib.rs index 1769f7ad6..a4d15b171 100644 --- a/pallets/bitcoin-vaults/src/lib.rs +++ b/pallets/bitcoin-vaults/src/lib.rs @@ -17,6 +17,7 @@ pub mod types; #[frame_support::pallet] pub mod pallet { use frame_support::pallet_prelude::*; + // SBP-M2 review: Please remove commented lines //#[cfg(feature = "std")] //use frame_support::serde::{Deserialize, Serialize}; use crate::types::*; @@ -193,6 +194,8 @@ pub mod pallet { pub(super) type Xpubs = StorageMap< _, Identity, + // SBP-M2 review: Key shouldn't be used directly as [u8; 32], create a type of it and use in all places. + // Best place to have that type in premitives.rs and move other constants also in that file [u8; 32], //that's the blake 2 hash result BoundedVec, OptionQuery, @@ -296,6 +299,7 @@ pub mod pallet { let pending_vaults = Self::get_pending_vaults(); let pending_proposals = Self::get_pending_proposals(); let proposals_to_finalize = Self::get_proposals_to_finalize(); + // SBP-M2 review: Remove commented line //let proposals_to_broadcast = Self::get_proposals_to_finalize(); log::info!("Pending vaults {:?}", pending_vaults.len()); // This validation needs to be done after the lock: @@ -353,10 +357,14 @@ pub mod pallet { ensure!(!>::contains_key(who.clone()), Error::::UserAlreadyHasXpub); let manual_hash = xpub.clone().using_encoded(blake2_256); // Assert if the input xpub is free to take (or if the user owns it) + + // SBP-M2 review: Please remove clone from manual_hash as it's type implements copy trait match Self::get_xpub_status(who.clone(), manual_hash.clone()) { XpubStatus::Owned => log::info!("Xpub owned, nothing to insert"), XpubStatus::Taken => Err(Error::::XPubAlreadyTaken)?, //xpub taken: abort tx XpubStatus::Free => { + + // SBP-M2 review: Please remove commented lines // xpub free: erase unused xpub and insert on maps // if >::contains_key(who.clone()) { // Self::remove_xpub_from_pallet_storage(who.clone())?; @@ -406,6 +414,7 @@ pub mod pallet { .collect::>(); ensure!(vaults.is_empty(), Error::::XpubLinkedToVault); + // SBP-M2 review: Unnecessary clone as value is not being used further Self::do_remove_xpub(who.clone()) } @@ -445,6 +454,7 @@ pub mod pallet { Error::::InvalidVaultThreshold ); let vault = Vault:: { + // SBP-M2 review: Unnecessary clone as value is not being used further owner: who.clone(), threshold, description, @@ -504,6 +514,7 @@ pub mod pallet { let who = ensure_signed(origin.clone())?; // ensure user is in the vault let proposal = Proposal:: { + // SBP-M2 review: Unnecessary clone as value is not being used further proposer: who.clone(), vault_id, status: ProposalStatus::Pending, @@ -520,6 +531,7 @@ pub mod pallet { Self::do_propose(proposal) } + // SBP-M2 review: Test case missing for this extrinsic /// Proposal removal /// /// Tries to remove a specified proposal. Only the user who created the proposal can remove it. @@ -534,12 +546,15 @@ pub mod pallet { let proposal = >::get(proposal_id).ok_or(Error::::ProposalNotFound)?; // Only vault proposer can remove // validation before do_remove_proposal because the user is not needed anymore + + // SBP-M2 review: Please resolve this //TODO: proposal cannot be erased if readyToBroadcast() finalized or broadcasted // -> erase only pending ensure!(proposal.proposer.eq(&who), Error::::ProposerPermissionsNeeded); Self::do_remove_proposal(proposal_id) } + // SBP-M2 review: Test case missing for this extrinsic /// BDK URL insertion /// /// Changes the BDK-services endpoint, useful for pointing to the btc mainnet or testnet @@ -607,6 +622,7 @@ pub mod pallet { Self::do_finalize_psbt(who, proposal_id, broadcast) } + // SBP-M2 review: Test case missing for this extrinsic /// Broadcast PSBT /// /// Queries a proposal to be broadcasted in case it wasn't on the finalization step. @@ -622,6 +638,11 @@ pub mod pallet { #[pallet::weight(Weight::from_ref_time(10_000) + T::DbWeight::get().writes(1))] pub fn broadcast_psbt(origin: OriginFor, proposal_id: [u8; 32]) -> DispatchResult { let who = ensure_signed(origin.clone())?; + // SBP-M2 review: Why same function? What if someone set broadcast as true on + // finalize_psbt extrinsic? Having same functionality on two extrinsics is not correct. + // What you can do either wrap both in one and specify the bool flag in depth or set + // broadcast as false in one and true in other we you like to use the same do_finalize_psbt() + // in both cases. Self::do_finalize_psbt(who, proposal_id, true) } @@ -693,6 +714,7 @@ pub mod pallet { Self::do_finalize_proof(who, vault_id, psbt) } + // SBP-M2 review: Test case missing for this extrinsic /// Kill almost all storage /// /// Use with caution! @@ -702,6 +724,7 @@ pub mod pallet { #[pallet::weight(Weight::from_ref_time(10_000) + T::DbWeight::get().writes(1))] pub fn kill_storage(origin: OriginFor) -> DispatchResult { T::ChangeBDKOrigin::ensure_origin(origin.clone())?; + // SBP-M2 review: Remove let _, instead use ? operator let _ = >::clear(1000, None); let _ = >::clear(1000, None); let _ = >::clear(1000, None); @@ -712,6 +735,7 @@ pub mod pallet { Ok(()) } + // SBP-M2 review: Test case missing for this extrinsic /// Extrinsic to insert a valid vault descriptor /// /// Meant to be unsigned with signed payload and used by an offchain worker @@ -729,6 +753,7 @@ pub mod pallet { .vaults_payload .iter() .find_map(|vault_payload| { + // SBP-M2 review: Use ? operator instead of expect (on other cases also) let output_descriptor = BoundedVec::::try_from( vault_payload.output_descriptor.clone(), ) @@ -743,6 +768,8 @@ pub mod pallet { }; let status: BDKStatus = vault_payload.clone().status.into(); + + // SBP-M2 review: Remove this commented line. //assert!(Self::do_insert_descriptors(vault_payload.vault_id,descriptors).is_ok()); let tx_res = Self::do_insert_descriptors(vault_payload.vault_id, descriptors, status); @@ -754,6 +781,7 @@ pub mod pallet { .unwrap_or(Ok(())) } + // SBP-M2 review: Test case missing for this extrinsic /// Extrinsic to insert a valid proposal PSBT /// /// Meant to be unsigned with signed payload and used by an offchain worker @@ -786,6 +814,7 @@ pub mod pallet { Ok(()) } + // SBP-M2 review: Test case missing for this extrinsic /// Extrinsic to insert a valid proposal TX_ID /// /// Meant to be unsigned with signed payload and used by an offchain worker diff --git a/pallets/bitcoin-vaults/src/types.rs b/pallets/bitcoin-vaults/src/types.rs index c3168fb61..9ecdeb4e2 100644 --- a/pallets/bitcoin-vaults/src/types.rs +++ b/pallets/bitcoin-vaults/src/types.rs @@ -7,6 +7,8 @@ use sp_runtime::sp_std::vec::Vec; pub type Description = BoundedVec::VaultDescriptionMaxLen>; pub type PSBT = BoundedVec::PSBTMaxLen>; + +// SBP-M2 review: Please remove commented lines //pub type AccountId = <::Signer as IdentifyAccount>::AccountId; /*--- Constants section ---*/ //pub const BDK_SERVICES_URL: &[u8] = b"https://bdk.hashed.systems"; @@ -139,6 +141,7 @@ impl Proposal { self.status.is_ready_to_finalize() && self.offchain_status.eq(&BDKStatus::Valid) } + // SBP-M2 review: Please remove commented lines // pub fn can_be_broadcasted(&self) -> bool { // self.status.eq(&ProposalStatus::ReadyToBroadcast) && self.offchain_status.eq(&BDKStatus::Valid) // } @@ -288,6 +291,8 @@ pub enum ProposalStatus { Broadcasted, } +// SBP-M2 review: Try incorporate matches! to improve code quality +// Eg: matches!(*self, ProposalStatus::ReadyToFinalize(_)) impl ProposalStatus { pub fn is_ready_to_finalize(&self) -> bool { match *self { diff --git a/pallets/fruniques/src/benchmarking.rs b/pallets/fruniques/src/benchmarking.rs index d496a9fc8..405f043b2 100644 --- a/pallets/fruniques/src/benchmarking.rs +++ b/pallets/fruniques/src/benchmarking.rs @@ -1,3 +1,5 @@ +//SBP-M2 review: Implement benchmarking for all the extrinsics, it is must before going to production + //! Benchmarking setup for pallet-template use super::*; diff --git a/pallets/fruniques/src/functions.rs b/pallets/fruniques/src/functions.rs index ac9372b6f..6d7fd5f6c 100644 --- a/pallets/fruniques/src/functions.rs +++ b/pallets/fruniques/src/functions.rs @@ -14,8 +14,10 @@ use frame_support::PalletId; // use frame_support::traits::OriginTrait; use sp_runtime::traits::AccountIdConversion; use sp_runtime::{sp_std::vec::Vec, Permill}; +// SBP-M2 review: Please remove commented line // use sp_runtime::traits::StaticLookup; +// SBP-M2 review: This file contains multiple code quality issues, please run cargo clippy to find those impl Pallet { pub fn u32_to_instance_id(input: u32) -> T::ItemId where @@ -40,12 +42,14 @@ impl Pallet { } pub fn permill_to_percent(input: Permill) -> u32 { + // SBP-M2 review: deconstruct() return u32, no need to type cast input.deconstruct() as u32 } pub fn bytes_to_string(input: Vec) -> String { let mut s = String::default(); for x in input { + // SBP-M2 review: commented line //let c: char = x.into(); s.push(x as char); } @@ -58,6 +62,7 @@ impl Pallet { ::unlookup(account_id.clone()) } + // SBP-M2 review: No usage found, and will panic in case of error. Error should be handled. /// Helper function for printing purposes pub fn get_nft_attribute( class_id: &T::CollectionId, @@ -76,6 +81,7 @@ impl Pallet { } pub fn is_frozen(collection_id: &T::CollectionId, instance_id: &T::ItemId) -> bool { + // SBP-M2 review: Please remove unwrap(), error should be handled properly let frunique: FruniqueData = >::try_get(&collection_id, &instance_id).unwrap(); @@ -83,6 +89,7 @@ impl Pallet { } pub fn collection_exists(class_id: &T::CollectionId) -> bool { + // SBP-M2 review: can be simplified like this: pallet_uniques::Pallet::::collection_owner(*class_id).is_some() if let Some(_owner) = pallet_uniques::Pallet::::collection_owner(*class_id) { return true; } @@ -90,6 +97,7 @@ impl Pallet { } pub fn instance_exists(class_id: &T::CollectionId, instance_id: &T::ItemId) -> bool { + // SBP-M2 review: can be simplified like above if let Some(_owner) = pallet_uniques::Pallet::::owner(*class_id, *instance_id) { return true; } @@ -187,6 +195,7 @@ impl Pallet { ::ItemId: From, { let nex_item: ItemId = >::try_get(collection).unwrap_or(0); + // SBP-M2 review: Should perform safe math operation. Eg: use of saturating_add/saturating_inc() >::insert(collection, nex_item + 1); let item = Self::u32_to_instance_id(nex_item); @@ -294,6 +303,7 @@ impl Pallet { false, )?; + // SBP-M2 review: Unsafe operation, please use safe math operation. Eg: use of saturating_add/saturating_inc() >::put(Self::next_collection() + 1); Ok(class_id) @@ -359,12 +369,14 @@ impl Pallet { Error::::FruniqueNotFound ); + // SBP-M2 review: Please remove unwrap(), error should be handled properly let frunique_parent: FruniqueData = >::try_get(&parent_info.collection_id, &parent_info.parent_id).unwrap(); ensure!(!frunique_parent.frozen, Error::::ParentFrozen); ensure!(!frunique_parent.redeemed, Error::::ParentAlreadyRedeemed); + // SBP-M2 review: Unsafe operation, please use safe math operation. Eg: saturating_mul let child_percentage: Permill = parent_info.parent_weight * frunique_parent.weight; let parent_data: ParentInfo = ParentInfo { @@ -426,6 +438,7 @@ impl Pallet { ensure!(Self::collection_exists(&collection), Error::::CollectionNotFound); ensure!(Self::instance_exists(&collection, &item), Error::::FruniqueNotFound); + // SBP-M2 review: Please remove unwrap(), error should be handled properly let frunique_data: FruniqueData = >::try_get(collection, item).unwrap(); @@ -453,6 +466,7 @@ impl Pallet { collection: T::CollectionId, item: T::ItemId, ) -> CollectionDescription { + // SBP-M2 review: Please remove unwrap(), error should be handled properly let frunique_data = >::try_get(collection, item).unwrap(); frunique_data.metadata } @@ -462,6 +476,7 @@ impl Pallet { IdOrVec::Vec(Self::module_name().as_bytes().to_vec()) } + // SBP-M2 review: Please handle error properly // Helper function to get the pallet account as a AccountId pub fn pallet_account() -> T::AccountId { let pallet_name = Self::module_name().as_bytes().to_vec(); diff --git a/pallets/fruniques/src/lib.rs b/pallets/fruniques/src/lib.rs index 34701ddfe..04e4a7c48 100644 --- a/pallets/fruniques/src/lib.rs +++ b/pallets/fruniques/src/lib.rs @@ -8,6 +8,7 @@ mod mock; #[cfg(test)] mod tests; +// SBP-M2 reviewL: Pallet should contain benchmarks // #[cfg(feature = "runtime-benchmarks")] // mod benchmarking; @@ -202,9 +203,11 @@ pub mod pallet { where T: pallet_uniques::Config, { + // SBP-M2 review: Missing doc #[pallet::call_index(1)] #[pallet::weight(Weight::from_ref_time(10_000) + T::DbWeight::get().writes(10))] pub fn initial_setup(origin: OriginFor, freezer: T::AccountId) -> DispatchResult { + // SBP-M2 review: Is it correct? //Transfer the balance T::RemoveOrigin::ensure_origin(origin.clone())?; @@ -228,6 +231,7 @@ pub mod pallet { metadata: CollectionDescription, ) -> DispatchResult { let admin: T::AccountId = ensure_signed(origin.clone())?; + // SBP-M2 review: Please remove this // let admin: T::AccountId = frame_system::RawOrigin::Root.into(); Self::do_create_collection(origin, metadata, admin.clone())?; @@ -376,6 +380,8 @@ pub mod pallet { instance_id, |frunique_data| -> DispatchResult { let frunique = frunique_data.as_mut().ok_or(Error::::FruniqueNotFound)?; + // SBP-M2 review: Try using + // if frunique.verified || frunique.verified_by.is_some() if frunique.verified == true || frunique.verified_by.is_some() { return Err(Error::::FruniqueAlreadyVerified.into()); } diff --git a/pallets/fruniques/src/mock.rs b/pallets/fruniques/src/mock.rs index fa9ab40b9..9371779b2 100644 --- a/pallets/fruniques/src/mock.rs +++ b/pallets/fruniques/src/mock.rs @@ -136,6 +136,8 @@ impl pallet_rbac::Config for Test { type MaxUsersPerRole = MaxUsersPerRole; type RemoveOrigin = EnsureRoot; } + +//SBP-M2 review: Please remove commented lines // Build genesis storage according to the mock runtime. // pub(crate) fn new_test_ext() -> sp_io::TestExternalities { // frame_system::GenesisConfig::default().build_storage::().unwrap().into() diff --git a/pallets/fruniques/src/tests.rs b/pallets/fruniques/src/tests.rs index a0bcd355d..79b1d9fb5 100644 --- a/pallets/fruniques/src/tests.rs +++ b/pallets/fruniques/src/tests.rs @@ -67,6 +67,7 @@ fn create_collection_works() { }) } +// SBP-M2 review: This test is failing please update it #[test] fn spawn_extrinsic_works() { new_test_ext().execute_with(|| { diff --git a/pallets/fruniques/src/types.rs b/pallets/fruniques/src/types.rs index 3cb22c39b..716cb3708 100644 --- a/pallets/fruniques/src/types.rs +++ b/pallets/fruniques/src/types.rs @@ -11,11 +11,13 @@ pub type AttributeValue = BoundedVec::Value pub type Attributes = Vec<(AttributeKey, AttributeValue)>; pub type Children = BoundedVec, ::ChildMaxLen>; +// SBP-M2 review: Why this additional type as it is being used in one place only? pub type StringLimit = BoundedVec::StringLimit>; pub type CollectionId = u32; pub type ItemId = u32; +// SBP-M2 review: Use BoundedVec directly instead of creating another type pub type CollectionDescription = StringLimit; pub type ParentId = ItemId; diff --git a/pallets/gated-marketplace/src/functions.rs b/pallets/gated-marketplace/src/functions.rs index 9bc0cd192..3a52b0a37 100644 --- a/pallets/gated-marketplace/src/functions.rs +++ b/pallets/gated-marketplace/src/functions.rs @@ -11,6 +11,8 @@ use frame_support::traits::ExistenceRequirement::KeepAlive; use frame_support::traits::Time; use sp_runtime::Permill; +// SBP-M2 review: This file contains multiple code quality issues, please run cargo clippy to find those, +// remove commented lines and resolve TODOs impl Pallet { pub fn do_initial_setup() -> DispatchResult { let pallet_id = Self::pallet_id(); @@ -232,9 +234,9 @@ impl Pallet { marketplace_id: [u8; 32], ) -> DispatchResult { - //since users can self-enroll, the caller of this function must validate + //since users can self-enroll, the caller of this function must validate //that the user is indeed the owner of the address by using ensure_signed - + //ensure the account is not already in the marketplace ensure!( @@ -248,10 +250,10 @@ impl Pallet { // ensure the marketplace exist ensure!(>::contains_key(marketplace_id), Error::::MarketplaceNotFound); - + Self::insert_in_auth_market_lists(account.clone(), MarketplaceRole::Participant, marketplace_id)?; Self::deposit_event(Event::AuthorityAdded(account, MarketplaceRole::Participant)); - + Ok(()) } @@ -406,7 +408,7 @@ impl Pallet { percentage: u32, ) -> DispatchResult { - + //ensure the marketplace exists ensure!(>::contains_key(marketplace_id), Error::::MarketplaceNotFound); @@ -877,6 +879,7 @@ impl Pallet { pub fn do_block_user( authority: T::AccountId, + // SBP-M2 review: type MarketplaceId can be used here and other relevant places marketplace_id: [u8; 32], user: T::AccountId, ) -> DispatchResult { @@ -932,6 +935,7 @@ impl Pallet { Ok(()) } + // SBP-M2 review: type MarketplaceId can be used here fn is_user_blocked(user: T::AccountId, marketplace_id: [u8; 32]) -> bool { >::get(marketplace_id).contains(&user) } @@ -1035,6 +1039,7 @@ impl Pallet { >::remove(_k1, marketplace_id); }); + // SBP-M2 review: Error should ne handled, please remove let _ // remove from ApplicantsByMarketplace list let _ = >::clear_prefix(marketplace_id, 1000, None); @@ -1282,10 +1287,14 @@ impl Pallet { |redemption_data| -> DispatchResult { let redemption_data = redemption_data.as_mut().ok_or(Error::::RedemptionRequestNotFound)?; + + // SBP-M2 review: Can be simplified + // ensure!(!redemption_data.is_redeemed, Error::::RedemptionRequestAlreadyRedeemed); ensure!( redemption_data.is_redeemed == false, Error::::RedemptionRequestAlreadyRedeemed ); + // SBP-M2 review: Can be simplified like above ensure!( redemption_data.is_redeemed == false, Error::::RedemptionRequestAlreadyRedeemed diff --git a/pallets/gated-marketplace/src/lib.rs b/pallets/gated-marketplace/src/lib.rs index 040e45bee..6c0466e57 100644 --- a/pallets/gated-marketplace/src/lib.rs +++ b/pallets/gated-marketplace/src/lib.rs @@ -11,6 +11,7 @@ mod tests; pub mod functions; pub mod types; +// SBP-M2 review: Pallet should implement benchmarks #[frame_support::pallet] pub mod pallet { use frame_support::pallet_prelude::*; @@ -161,6 +162,7 @@ pub mod pallet { ValueQuery, >; + // SBP-M2 review: Please remove commented line #[pallet::storage] #[pallet::getter(fn offers_info)] pub(super) type OffersInfo = StorageMap< @@ -339,6 +341,7 @@ pub mod pallet { where T: pallet_uniques::Config, { + // SBP-M2 review: Missing doc #[pallet::call_index(0)] #[pallet::weight(Weight::from_ref_time(10_000) + T::DbWeight::get().writes(10))] pub fn initial_setup(origin: OriginFor) -> DispatchResult { @@ -347,6 +350,7 @@ pub mod pallet { Ok(()) } + // SBP-M2 review: Incomplete doc, missing params /// Create a new marketplace. /// /// Creates a new marketplace with the given label @@ -365,6 +369,7 @@ pub mod pallet { sell_fee: u32, ) -> DispatchResult { let who = ensure_signed(origin)?; // origin will be market owner + // SBP-M2 review: Naming convention can be improved let m = Marketplace { label, buy_fee: Permill::from_percent(buy_fee), @@ -374,6 +379,8 @@ pub mod pallet { Self::do_create_marketplace(who, admin, m) } + + // SBP-M2 review: Duplicate line and incorrect param name. Please correct this /// Block or Unblock a user from apllying to a marketplace. /// /// Blocks or Unblocks a user from applying to a marketplace. @@ -420,6 +427,7 @@ pub mod pallet { #[pallet::weight(Weight::from_ref_time(10_000) + T::DbWeight::get().writes(1))] pub fn apply( origin: OriginFor, + // SBP-M2 review: type MarketplaceId can be used here and other relevant places marketplace_id: [u8; 32], // Getting encoding errors from polkadotjs if an object vector have optional fields fields: Fields, @@ -476,6 +484,7 @@ pub mod pallet { Self::do_apply(who, custodian, marketplace_id, application) } + // SBP-M2 review: Incomplete doc, missing params /// Accept or reject an application. /// /// If the application is accepted, @@ -509,6 +518,7 @@ pub mod pallet { Self::do_enroll(who, marketplace_id, account_or_application, approved, feedback) } + // SBP-M2 review: Incomplete doc, missing params /// Invite a user to a marketplace. /// /// The admin of the marketplace can invite a user to the marketplace. @@ -589,6 +599,7 @@ pub mod pallet { Self::do_remove_authority(who, account, authority_type, marketplace_id) } + // SBP-M2 review: Incomplete doc, incorrect param /// Update marketplace's label. /// /// This extrinsic updates the label of the selected marketplace. @@ -638,6 +649,7 @@ pub mod pallet { Self::do_remove_marketplace(who, marketplace_id) } + // SBP-M2 review: Incomplete doc, missing param /// Enlist a sell order. /// /// This extrinsic creates a sell order in the selected marketplace. @@ -676,6 +688,7 @@ pub mod pallet { ) } + // SBP-M2 review: Missing param in doc /// Accepts a sell order. /// /// This extrinsic is called by the user who wants to buy the item. @@ -722,6 +735,7 @@ pub mod pallet { Self::do_remove_offer(who, offer_id) } + // SBP-M2 review: Missing doc param /// Enlist a buy order. /// /// This extrinsic creates a buy order in the selected marketplace. @@ -759,6 +773,7 @@ pub mod pallet { ) } + // SBP-M2 review: Extra param /// Accepts a buy order. /// /// This extrinsic is called by the owner of the item who accepts the buy offer created by a market participant. @@ -801,6 +816,7 @@ pub mod pallet { let who = ensure_signed(origin)?; match redeem { RedeemArgs::AskForRedemption { collection_id, item_id } => { + // SBP-M2 review: Return can be removed return Self::do_ask_for_redeem(who, marketplace, collection_id, item_id); }, RedeemArgs::AcceptRedemption(redemption_id) => { @@ -809,8 +825,10 @@ pub mod pallet { } } + // SBP-M2 review: Pleaser resolve this //TODO: Add CRUD operations for the offers + // SBP-M2 review: Pleasre remove let _ and handle error properly /// Kill all the stored data. /// /// This function is used to kill ALL the stored data. diff --git a/pallets/mapped-assets/Cargo.toml b/pallets/mapped-assets/Cargo.toml index d1013d536..697da2edf 100644 --- a/pallets/mapped-assets/Cargo.toml +++ b/pallets/mapped-assets/Cargo.toml @@ -1,3 +1,4 @@ +# SBP-M2 review: Please update package details [package] name = "pallet-mapped-assets" version = "4.0.0-dev" diff --git a/pallets/mapped-assets/src/lib.rs b/pallets/mapped-assets/src/lib.rs index d3a58ad7b..cbda181f6 100644 --- a/pallets/mapped-assets/src/lib.rs +++ b/pallets/mapped-assets/src/lib.rs @@ -189,7 +189,7 @@ impl AssetsCallback for DefaultCallback { } } - +// SBP-M2 review: Add benchmarks for remaining extrinsics #[frame_support::pallet] pub mod pallet { use super::*; @@ -1600,7 +1600,7 @@ pub mod pallet { let id: T::AssetId = id.into(); Self::do_refund(id, ensure_signed(origin)?, allow_burn) } - + /// Reserve some assets on an account for a specific asset. /// diff --git a/pallets/rbac/Cargo.toml b/pallets/rbac/Cargo.toml index 6a455ca9c..119df4736 100644 --- a/pallets/rbac/Cargo.toml +++ b/pallets/rbac/Cargo.toml @@ -1,3 +1,4 @@ +# SBP-M2 review: Please update package details [package] name = "pallet-rbac" version = "4.0.0-dev" diff --git a/pallets/rbac/src/benchmarking.rs b/pallets/rbac/src/benchmarking.rs index d496a9fc8..ecf13b133 100644 --- a/pallets/rbac/src/benchmarking.rs +++ b/pallets/rbac/src/benchmarking.rs @@ -1,3 +1,4 @@ +// SBP-M2 review: Implement benchmarks //! Benchmarking setup for pallet-template use super::*; diff --git a/pallets/rbac/src/functions.rs b/pallets/rbac/src/functions.rs index 0e8c725d2..ffa45eb2a 100644 --- a/pallets/rbac/src/functions.rs +++ b/pallets/rbac/src/functions.rs @@ -106,6 +106,7 @@ impl RoleBasedAccessControl for Pallet { pallet: IdOrVec, roles: Vec>, ) -> Result, DispatchError> { + // SBP-M2 review: type RoleId can be reused here and please incorporate other relevant places as well let mut role_ids = Vec::<[u8; 32]>::new(); for role in roles { role_ids.push(Self::create_role(role.to_owned())?); @@ -159,7 +160,7 @@ impl RoleBasedAccessControl for Pallet { let pallet_id = pallet.to_id(); // checks for duplicates: ensure!(Self::has_unique_elements(roles.clone()), Error::::DuplicateRole); - let pallet_roles = >::get(&pallet_id); + let pallet_roles = >:: get(&pallet_id); for id in roles.clone() { ensure!(!pallet_roles.contains(&id), Error::::RoleAlreadyLinkedToPallet); } diff --git a/pallets/rbac/src/lib.rs b/pallets/rbac/src/lib.rs index 408f6b1f1..df8745d51 100644 --- a/pallets/rbac/src/lib.rs +++ b/pallets/rbac/src/lib.rs @@ -215,6 +215,7 @@ pub mod pallet { NotAuthorized, } + // SBP-M2 review: Missing document #[pallet::call] impl Pallet { #[pallet::call_index(0)]