SBP Milestone 2 review

pallets/afloat/README.md

@@ -1 +1,2 @@
-License: Unlicense
+// SBP-M2 review: Update readme
+License: Unlicense

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::*;

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<T: Config> Pallet<T> {
 	pub fn do_initial_setup(creator: T::AccountId, admin: T::AccountId) -> DispatchResult {
 		let creator_user: User<T> = User {
@@ -210,7 +213,7 @@ impl<T: Config> Pallet<T> {
 	///
 	pub fn do_delete_user(_actor: T::AccountId, user_address: T::AccountId) -> DispatchResult {
 		ensure!(<UserInfo<T>>::contains_key(user_address.clone()), Error::<T>::UserNotFound);
-		
+
 		Self::remove_from_afloat_collection(user_address.clone(), FruniqueRole::Collaborator)?;
 		Self::remove_from_afloat_marketplace(user_address.clone())?;
 
@@ -221,7 +224,7 @@ impl<T: Config> Pallet<T> {
 
 	pub fn create_afloat_collection(origin: OriginFor<T>,
 		metadata: CollectionDescription<T>,
-		admin: T::AccountId, ) -> DispatchResult 
+		admin: T::AccountId, ) -> DispatchResult
 		where
 		<T as pallet_uniques::Config>::CollectionId: From<u32>,
 		{
@@ -240,6 +243,7 @@ impl<T: Config> Pallet<T> {
 	}
 
 	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::<T>::get().unwrap();
 		pallet_fruniques::Pallet::<T>::insert_auth_in_frunique_collection(invitee,
 		collection_id,
@@ -248,6 +252,7 @@ impl<T: Config> Pallet<T> {
 	}
 
 	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::<T>::get().unwrap();
 		pallet_fruniques::Pallet::<T>::remove_auth_from_frunique_collection(invitee,
 		collection_id,
@@ -256,6 +261,7 @@ impl<T: Config> Pallet<T> {
 	}
 
 	pub fn remove_from_afloat_marketplace(invitee: T::AccountId) -> DispatchResult {
+		// SBP-M2 review: Please remove unwrap() and manage error properly
 		let marketplace_id = AfloatMarketPlaceId::<T>::get().unwrap();
 		pallet_gated_marketplace::Pallet::<T>::remove_from_market_lists(invitee, MarketplaceRole::Participant, marketplace_id)
 	}

pallets/afloat/src/lib.rs

@@ -103,8 +103,9 @@ pub mod pallet {
 		<T as pallet_uniques::Config>::CollectionId, // Afloat's frunique collection id
 	>;
 
+	// SBP-M2 review: Missing extrinsic documentation and code is not well formatted
 	#[pallet::call]
-	impl<T: Config> Pallet<T> 
+	impl<T: Config> Pallet<T>
 	where
 	T: pallet_uniques::Config<CollectionId = CollectionId>
 	{
@@ -120,17 +121,19 @@ 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<T> = BoundedVec::try_from(b"Afloat".to_vec()).expect("Label too long");
 
 			pallet_mapped_assets::Pallet::<T>::create(
 				origin.clone(),
 				asset_id,
 				T::Lookup::unlookup(creator.clone()),
 				min_balance,
-			)?; 
+			)?;
 
 			pallet_fruniques::Pallet::<T>::do_initial_setup()?;
-			
+
 			Self::create_afloat_collection(RawOrigin::Signed(creator.clone()).into(), metadata, admin.clone())?;
 
 			pallet_gated_marketplace::Pallet::<T>::do_initial_setup()?;
@@ -158,6 +161,7 @@ pub mod pallet {
 		pub fn kill_storage(origin: OriginFor<T>) -> DispatchResult {
 			ensure_signed(origin.clone())?;
 			<AfloatMarketPlaceId<T>>::kill();
+			// SBP-M2 review: Remove let _, use ? operator instead
 			let _ = <UserInfo<T>>::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)?;
 

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;

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};

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::*;

pallets/bitcoin-vaults/src/functions.rs

@@ -21,7 +21,9 @@ impl<T: Config> Pallet<T> {
 		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<T>) -> DispatchResult {
+		// SBP-M2 review: Please resolve TODOs
 		//TODO vault_id exist?
 		// generate vault id
 		ensure!(vault.signers_are_unique(), Error::<T>::DuplicateVaultMembers);
@@ -34,6 +36,7 @@ impl<T: Config> Pallet<T> {
 			if !<XpubsByOwner<T>>::contains_key(acc.clone()) {
 				return Err(Error::<T>::XPubNotFound);
 			}
+
 			<VaultsBySigner<T>>::try_mutate(acc, |vault_vec| vault_vec.try_push(vault_id.clone()))
 				.map_err(|_| Error::<T>::SignerVaultLimit)
 		})?;
@@ -98,6 +101,7 @@ impl<T: Config> Pallet<T> {
 	pub fn do_propose(proposal: Proposal<T>) -> 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!(!<Proposals<T>>::contains_key(&proposal_id), Error::<T>::AlreadyProposed);
 		<Proposals<T>>::insert(proposal_id, proposal.clone());
 		<ProposalsByVault<T>>::try_mutate(proposal.vault_id, |proposals| {
@@ -123,6 +127,8 @@ impl<T: Config> Pallet<T> {
 		<Proposals<T>>::try_mutate::<_, (), DispatchError, _>(proposal_id, |proposal| {
 			proposal.as_ref().ok_or(Error::<T>::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::<T>::AlreadySigned);
@@ -150,6 +156,8 @@ impl<T: Config> Pallet<T> {
 		ensure!(proposal.offchain_status.eq(&BDKStatus::Valid), Error::<T>::InvalidProposal);
 		// can be called by any vault signer
 		ensure!(vault.is_vault_member(&signer), Error::<T>::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<T: Config> Pallet<T> {
 		<ProofOfReserves<T>>::try_mutate::<_, (), DispatchError, _>(vault_id, |maybe_proof| {
 			maybe_proof.as_ref().ok_or(Error::<T>::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<T: Config> Pallet<T> {
 					.try_push(signature)
 					.map_err(|_| Error::<T>::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 >= <Vaults<T>>::get(vault_id).unwrap().threshold
 				{
 					// set ReadyToFinalize when the threshold is reached
@@ -257,6 +268,7 @@ impl<T: Config> Pallet<T> {
 	// 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 <Xpubs<T>>::contains_key(xpub_hash) {
+			// SBP-M2 review: No need to use clone()
 			if let Some(owned_hash) = <XpubsByOwner<T>>::get(who.clone()) {
 				match xpub_hash == owned_hash {
 					true => return XpubStatus::Owned,
@@ -334,6 +346,7 @@ impl<T: Config> Pallet<T> {
 	}
 	/*---- Offchain utilities ----*/
 
+	// SBP-M2 review: Reusing the type which should be created for key
 	pub fn get_pending_vaults() -> Vec<[u8; 32]> {
 		<Vaults<T>>::iter()
 			.filter_map(|(entry, vault)| {
@@ -399,6 +412,8 @@ impl<T: Config> Pallet<T> {
 			.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
 	//     <Proposals<T>>::iter().filter_map(| (id,p) |{
@@ -478,6 +493,7 @@ impl<T: Config> Pallet<T> {
 		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<JsonValue> = xpubs
 			.iter()
 			.map(|xpub| {
@@ -537,6 +553,7 @@ impl<T: Config> Pallet<T> {
 		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<SingleVaultPayload> {
 		let mut generated_vaults = Vec::<SingleVaultPayload>::new();
 		pending_vaults.iter().for_each(|vault_to_complete| {
@@ -561,6 +578,7 @@ impl<T: Config> Pallet<T> {
 		generated_vaults
 	}
 
+	// SBP-M2 review: Redundant cloning on multiple types
 	pub fn gen_proposal_json_body(proposal_id: [u8; 32]) -> Result<Vec<u8>, OffchainStatus> {
 		let mut body = Vec::new();
 		let proposal = <Proposals<T>>::get(proposal_id)
@@ -636,6 +654,7 @@ impl<T: Config> Pallet<T> {
 		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<u8>,
@@ -712,6 +731,8 @@ impl<T: Config> Pallet<T> {
 		Ok(jsonSerialize::format(&json_object, 4))
 	}
 
+	// SBP-M2 review: Please remove commented code
+
 	// pub fn bdk_gen_finalized_proposal(proposal_id: [u8;32])-> Result<Vec<u8>,OffchainStatus >{
 	//     let raw_json = Self::gen_finalize_json_body(proposal_id)?;
 	//     let request_body =