From 25b0bd16a438b309fddcf97b7049bfe3d4408716 Mon Sep 17 00:00:00 2001
From: Hector Bulgarini <hbulgarini@gmail.com>
Date: Tue, 16 May 2023 18:05:26 +0200
Subject: [PATCH] Hector Bulgarini SBP Milestone Review 2

---
 common/src/traits.rs                  |  7 +++++++
 nft/src/lib.rs                        | 21 +++++++++++++++++++++
 nft/src/weights.rs                    |  6 ++++++
 tee/src/lib.rs                        | 19 +++++++++++++++++++
 tee/src/weights.rs                    |  6 ++++++
 transmission-protocols/src/weights.rs |  6 ++++++
 6 files changed, 65 insertions(+)

diff --git a/common/src/traits.rs b/common/src/traits.rs
index 6e0c7a5a..48835328 100644
--- a/common/src/traits.rs
+++ b/common/src/traits.rs
@@ -110,6 +110,13 @@ pub trait TEEExt {
 	type AccountId: Clone + PartialEq + Debug;
 	type MaxUriLen: Get<u32>;
 	/// Returns operator address and cluster id for a given enclave address
+	///
+	/// <HB SBP Milestone Review
+	///
+	/// This function can receive a reference instead of a value and do not force the caller to
+	/// clone the value passed.
+	///
+	/// >
 	fn ensure_enclave(account: Self::AccountId) -> Option<(ClusterId, Self::AccountId)>;
 
 	/// Register and assign an enclave
diff --git a/nft/src/lib.rs b/nft/src/lib.rs
index ea5a5218..ef1bb56f 100644
--- a/nft/src/lib.rs
+++ b/nft/src/lib.rs
@@ -442,6 +442,11 @@ pub mod pallet {
 		/// Create a new NFT with the provided details. An ID will be auto
 		/// generated and logged as an event, The caller of this function
 		/// will become the owner of the new NFT.
+		/// <HB SBP Milestone Review
+		///
+		/// Why not moving this function to the benchmarking file?
+		///
+		/// >
 		#[pallet::weight((
             {
 				if let Some(collection_id) = &collection_id {
@@ -524,6 +529,12 @@ pub mod pallet {
 		/// once the NFT is removed (burned) from the storage there is no way to
 		/// get it back.
 		/// Must be called by the owner of the NFT.
+		///
+		/// <HB SBP Milestone Review
+		///
+		/// Why not moving this function to the benchmarking file?
+		///
+		/// >
 		#[pallet::weight((
             {
 				let nft = Nfts::<T>::get(nft_id).ok_or(Error::<T>::NFTNotFound);
@@ -859,6 +870,11 @@ pub mod pallet {
 		/// Add an NFT to a collection.
 		/// Can only be called by owner of the collection, NFT
 		/// must not be in collection and collection must not be closed or has reached limit.
+		/// <HB SBP Milestone Review
+		///
+		/// Why not moving this function to the benchmarking file?
+		///
+		/// >
 		#[pallet::weight((
             {
 				let collection = Collections::<T>::get(collection_id).ok_or(Error::<T>::CollectionNotFound);
@@ -1001,6 +1017,11 @@ pub mod pallet {
 
 			// Create NFT
 			Self::create_nft(origin.clone(), offchain_data, royalty, collection_id, is_soulbound)?;
+			/// <HB SBP Milestone Review
+			///
+			/// unsafe operation, very unlikely to fail but still possible.
+			/// I would use saturating_sub() instad.
+			/// >
 			let nft_id = NextNFTId::<T>::get() - 1;
 
 			// Add a secret to the NFT
diff --git a/nft/src/weights.rs b/nft/src/weights.rs
index 5136b6a7..a702d5e2 100644
--- a/nft/src/weights.rs
+++ b/nft/src/weights.rs
@@ -42,6 +42,12 @@ pub trait WeightInfo {
 	fn set_collection_offchaindata() -> Weight;
 }
 
+/// <HB SBP Milestone Review
+///
+/// Did you forget to replace this weight file with the one generated by the benchmarking process?
+///
+/// >
+
 /// Weight functions for `ternoa_nft`.
 pub struct TernoaWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for TernoaWeight<T> {
diff --git a/tee/src/lib.rs b/tee/src/lib.rs
index d5d96ff5..bf1e4bf0 100644
--- a/tee/src/lib.rs
+++ b/tee/src/lib.rs
@@ -237,6 +237,12 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
 
+			/// <HB SBP Review
+			///
+			/// This .clones() can be removed as the ensure! does not take ownership of the
+			/// parameters.
+			///
+			/// >
 			ensure!(who.clone() != enclave_address.clone(), Error::<T>::OperatorAndEnclaveAreSame);
 			ensure!(
 				EnclaveRegistrations::<T>::get(&who).is_none(),
@@ -302,6 +308,12 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
 
+			/// <HB SBP Review
+			///
+			/// This .clones() can be removed as the ensure! does not take ownership of the
+			/// parameters.
+			///
+			/// >
 			ensure!(
 				who.clone() != new_enclave_address.clone(),
 				Error::<T>::OperatorAndEnclaveAreSame
@@ -524,6 +536,13 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			ensure_root(origin)?;
 
+			/// <HB SBP Review
+			///
+			/// This .clones() can be removed as the ensure! does not take ownership of the
+			/// parameters.
+			///
+			/// >
+
 			ensure!(
 				operator_address.clone() != new_enclave_address.clone(),
 				Error::<T>::OperatorAndEnclaveAreSame
diff --git a/tee/src/weights.rs b/tee/src/weights.rs
index 3421ac51..86118d5f 100644
--- a/tee/src/weights.rs
+++ b/tee/src/weights.rs
@@ -30,6 +30,12 @@ pub trait WeightInfo {
 	fn remove_cluster() -> Weight;
 }
 
+/// <HB SBP Milestone Review
+///
+/// Did you forget to replace this weight file with the one generated by the benchmarking process?
+///
+/// >
+
 impl WeightInfo for () {
 	fn register_enclave() -> Weight {
 		Weight::from_ref_time(10_000_000 as u64)
diff --git a/transmission-protocols/src/weights.rs b/transmission-protocols/src/weights.rs
index 398f786d..271fdfbd 100644
--- a/transmission-protocols/src/weights.rs
+++ b/transmission-protocols/src/weights.rs
@@ -24,6 +24,12 @@ pub trait WeightInfo {
 	fn set_protocol_fee() -> Weight;
 }
 
+/// <HB SBP Milestone Review
+///
+/// Did you forget to replace this weight file with the one generated by the benchmarking process?
+///
+/// >
+
 impl WeightInfo for () {
 	fn set_transmission_protocol(_s: u32) -> Weight {
 		Weight::from_ref_time(10_000_000 as u64)