Hector Bulgarini SBP Milestone Review 2

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

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

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> {

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

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)

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)