From 8fca6fac1162c0fce408e68c1481ea92b295f05d Mon Sep 17 00:00:00 2001
From: Hector Bulgarini <hbulgarini@gmail.com>
Date: Tue, 29 Aug 2023 17:01:07 +0200
Subject: [PATCH 1/2] Review in progress

---
 pallets/dao-bounties/src/benchmarking.rs     | 22 +++++++++++-
 pallets/dao-collective/src/benchmarking.rs   | 12 +++++++
 pallets/dao-collective/src/weights.rs        |  6 ++++
 pallets/dao-subscription/src/benchmarking.rs |  9 +++++
 pallets/dao-subscription/src/lib.rs          | 35 +++++++++++++++++---
 primitives/dao/src/lib.rs                    | 26 +++++++++++++--
 runtime/src/xcm_config.rs                    | 11 ++++++
 7 files changed, 114 insertions(+), 7 deletions(-)

diff --git a/pallets/dao-bounties/src/benchmarking.rs b/pallets/dao-bounties/src/benchmarking.rs
index 0fd6685..9b22463 100644
--- a/pallets/dao-bounties/src/benchmarking.rs
+++ b/pallets/dao-bounties/src/benchmarking.rs
@@ -17,6 +17,21 @@ const SEED: u32 = 0;
 
 // Create the pre-requisite information needed to create a dao.
 fn setup_dao_payload<T: Config<I>, I: 'static>() -> Vec<u8> {
+	/// <SBP MR2
+	///
+	/// I have a concern about this setup_dao_payload fn that seems to be common for all the
+	/// benchmarks:
+	///
+	/// I understand this function is to initilize the payload for the dao during the benchmarking
+	/// executions. As per i was able to understand, these values are mostly used by the methods
+	/// exposed by T:DaoProvider which as far as i have reviewed, it does not execute any o(n)
+	/// operations, but as the code extense and i don't have it 100% on top of my head i wonder if
+	/// there is any part of the code where the function being benchmarked is influenced by the size
+	/// of any of the fields in the json, which are currently hardcoded.
+	///
+	/// In case they are not, this comment can be dismissed.
+	///
+	/// >
 	let dao_json = json!({
 		"name": "name",
 		"purpose": "purpose",
@@ -41,7 +56,6 @@ fn setup_dao_payload<T: Config<I>, I: 'static>() -> Vec<u8> {
 fn create_dao<T: Config<I>, I: 'static>() -> Result<(), DispatchError> {
 	let caller = account("caller", 0, SEED);
 	let data = setup_dao_payload::<T, I>();
-
 	T::DaoProvider::create_dao(caller, vec![], vec![], data)?;
 
 	let dao_account_id = T::DaoProvider::dao_account_id(0);
@@ -122,6 +136,12 @@ fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::
 	frame_system::Pallet::<T>::assert_last_event(generic_event.into());
 }
 
+/// <SBP MR2
+///
+/// Is there any reason why most of the benchmarks do not attach "verify"s functions?
+/// I think its healty that every benchmark function verify that its execution was successful.
+///
+/// >
 benchmarks_instance_pallet! {
 	create_bounty {
 		create_dao::<T, I>()?;
diff --git a/pallets/dao-collective/src/benchmarking.rs b/pallets/dao-collective/src/benchmarking.rs
index e5f8f2c..2d5d515 100644
--- a/pallets/dao-collective/src/benchmarking.rs
+++ b/pallets/dao-collective/src/benchmarking.rs
@@ -118,6 +118,13 @@ benchmarks_instance_pallet! {
 		let bounded = T::Preimages::bound(proposal.clone())?.transmute();
 		let proposal_hash = T::Hashing::hash_of(&bounded);
 
+		/// <SBP MR2
+		///
+		/// Why don't generate a vector lenght based oin the ProposalMetadataLimit?
+		/// Or even sending this as a complexity parameter as you are doing with the other storage items?
+		///
+		/// Even if "meta" will not be stored in the DB, the data included in the events will allocate storage from the PoV anyway.
+		/// >
 		let meta: Option<Vec<u8>> = Some("Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor \
 			incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud \
 			exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure \
@@ -416,6 +423,11 @@ benchmarks_instance_pallet! {
 		let m in 4 .. T::MaxMembers::get();
 		let p in 1 .. T::MaxProposals::get();
 
+		/// <SBP MR2
+		///
+		/// Is there any reason why this function is not using the b linear parameter as the others?
+		///
+		///  >
 		let bytes = 100;
 		let bytes_in_storage = bytes + size_of::<u32>() as u32;
 
diff --git a/pallets/dao-collective/src/weights.rs b/pallets/dao-collective/src/weights.rs
index 54e7893..046c0ff 100644
--- a/pallets/dao-collective/src/weights.rs
+++ b/pallets/dao-collective/src/weights.rs
@@ -149,6 +149,12 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	/// The range of component `b` is `[1, 1024]`.
 	/// The range of component `m` is `[4, 100]`.
 	/// The range of component `p` is `[1, 50]`.
+	/// <HB SBP M2 Review
+	/// 
+	/// I don't know the internals, but for some reason the benchmarking macro is really using the bytes parameter for the benchmarking function.
+	/// There is nothingto do here, but in case you want to optimize this result, this can be a starting point.
+	/// 
+	/// >
 	fn close_early_approved(_b: u32, m: u32, p: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `871 + b * (1 ±0) + m * (64 ±0) + p * (185 ±0)`
diff --git a/pallets/dao-subscription/src/benchmarking.rs b/pallets/dao-subscription/src/benchmarking.rs
index ace1cc5..0bf9f59 100644
--- a/pallets/dao-subscription/src/benchmarking.rs
+++ b/pallets/dao-subscription/src/benchmarking.rs
@@ -20,6 +20,15 @@ benchmarks! {
 
 	set_subscription_tier {
 		let (tier, details) = Subscription::<T>::get_default_tier_details();
+		/// <SBP MR2
+		///
+		/// Is it possible that taking into account that SubscriptionDetailsOf<T> is composed by TokenBalances:
+		/// pub type TokenBalances<AssetId, Balance, TokenBalancesLimit> = BoundedVec<(AssetId, Balance), TokenBalancesLimit>;
+		///
+		/// the call can be called with a different token_prices len size and make a discrepancy with the the default len comming from get_default_tier_details()?
+		///
+		///
+		/// ?
 	}: _(RawOrigin::Root, tier.clone(), details.clone())
 	verify {
 		let subscription_tier = Subscription::<T>::subscription_tiers(tier).unwrap();
diff --git a/pallets/dao-subscription/src/lib.rs b/pallets/dao-subscription/src/lib.rs
index b70e222..15ba0e0 100644
--- a/pallets/dao-subscription/src/lib.rs
+++ b/pallets/dao-subscription/src/lib.rs
@@ -191,7 +191,10 @@ pub mod pallet {
 			tier: VersionedDaoSubscriptionTier,
 			details: SubscriptionDetailsOf<T>,
 		) -> DispatchResult {
-			ensure_root(origin.clone())?;
+			/// <SBP MR2
+			/// You can remove this .clone()
+			/// >
+			ensure_root(origin)?;
 
 			Self::do_set_subscription_tier(tier, details)
 		}
@@ -203,6 +206,9 @@ pub mod pallet {
 			dao_id: DaoId,
 			reason: SuspensionReason,
 		) -> DispatchResult {
+			/// <SBP MR2
+			/// You can remove this .clone()
+			/// >
 			ensure_root(origin.clone())?;
 
 			Subscriptions::<T>::try_mutate(
@@ -295,9 +301,13 @@ pub mod pallet {
 
 								extra_check(subscription)?;
 
-								subscription.fn_balance = fn_balance
-									.checked_sub(1)
-									.ok_or(Error::<T>::FunctionBalanceLow)?;
+								/// <SBP MR2
+								///
+								/// Is this kind of used to assure that at least the account has
+								/// balance 1 for existencial deposit? It's not wrong but i
+								/// wonder if there could be an impl fn for this where yoiu don't
+								/// hardcode the 1 value >
+								subscription.fn_balance = fn_balance.checked_sub(1).ok_or(Error::<T>::FunctionBalanceLow)?;
 
 								let (block_number, fn_calls) = subscription.fn_per_block;
 
@@ -330,6 +340,11 @@ pub mod pallet {
 			T::TreasuryPalletId::get().into_account_truncating()
 		}
 
+		/// <SBP MR2
+		///
+		/// As the TODO says, this is a good candidate to be configured in the chain_spec + Defaults
+		///
+		/// >
 		// TODO: try to use default for subscription details
 		pub fn get_default_tier_details() -> (VersionedDaoSubscriptionTier, SubscriptionDetailsOf<T>)
 		{
@@ -467,6 +482,13 @@ impl<T: Config>
 	fn unsubscribe(dao_id: DaoId) -> Result<(), DispatchError> {
 		let subscription = Subscriptions::<T>::get(dao_id);
 		if subscription.is_some() {
+			///< SBP MR2
+			///
+			/// Is this the only required action for subscribing ?
+			/// What about the opther storage items ? I wonder if it worths to do some cleanup for
+			/// storage optimization once they unsubscribe happens
+			///
+			/// >
 			Subscriptions::<T>::remove(dao_id);
 
 			Self::deposit_event(Event::DaoUnsubscribed { dao_id });
@@ -549,6 +571,11 @@ impl<T: Config>
 
 						Self::pay_for_subscription(account_id, &details, token_id)?;
 
+						/// <SBP MR2
+						///
+						/// Unnecesary clones ?
+						///
+						/// >
 						subscription.tier = tier.clone();
 						subscription.details = details.clone();
 
diff --git a/primitives/dao/src/lib.rs b/primitives/dao/src/lib.rs
index 8a20869..02764ca 100644
--- a/primitives/dao/src/lib.rs
+++ b/primitives/dao/src/lib.rs
@@ -601,8 +601,19 @@ pub enum SuspensionReason {
 
 #[derive(Encode, Decode, Clone, PartialEq, TypeInfo, RuntimeDebug, MaxEncodedLen)]
 pub enum DaoSubscriptionStatus<BlockNumber> {
-	Active { until: BlockNumber },
-	Suspended { at: BlockNumber, reason: SuspensionReason },
+	/// <SBP MR2
+	///
+	/// Please take into account that in release 1.0 the api for block number changes to  
+	/// frame_system::pallet_prelude::BlockNumberFor;
+	///
+	/// >
+	Active {
+		until: BlockNumber,
+	},
+	Suspended {
+		at: BlockNumber,
+		reason: SuspensionReason,
+	},
 }
 
 pub type DaoFunctionBalance = u32;
@@ -630,6 +641,17 @@ pub struct DaoSubscription<BlockNumber, Tier, Details, AssetId> {
 	pub subscribed_at: BlockNumber,
 	pub last_renewed_at: Option<BlockNumber>,
 	pub status: DaoSubscriptionStatus<BlockNumber>,
+	/// <SBP M2
+	///
+	/// I struggle to understand the purpose of this field since it saw in the tests the folliwing:
+	///
+	/// fn_balance: DEFAULT_FUNCTION_CALL_LIMIT,
+	///
+	/// i would suggest to rename these fn fields with something more descriptive.
+	/// Personally, in the code i was expecting kind of a closure assigment in my first read (then
+	/// i realized it was not the case)
+	///
+	/// >
 	pub fn_balance: DaoFunctionBalance,
 	pub fn_per_block: FunctionPerBlock<BlockNumber, DaoFunctionBalance>,
 }
diff --git a/runtime/src/xcm_config.rs b/runtime/src/xcm_config.rs
index 82aaa90..18bb43c 100644
--- a/runtime/src/xcm_config.rs
+++ b/runtime/src/xcm_config.rs
@@ -154,6 +154,12 @@ where
 	}
 }
 
+
+	/// < SBP MR2
+	/// 
+	/// This struct can be removed since is not being used anywhere. 
+	/// 
+	/// >
 // See issue #5233
 pub struct DenyReserveTransferToRelayChain;
 impl ShouldExecute for DenyReserveTransferToRelayChain {
@@ -200,6 +206,11 @@ impl ShouldExecute for DenyReserveTransferToRelayChain {
 
 type Barrier = (
 	TakeWeightCredit,
+	/// < SBP MR2
+	/// 
+	/// AllowTopLevelPaidExecutionFrom<Everything> duplicated.
+	/// 
+	/// >
 	AllowTopLevelPaidExecutionFrom<Everything>,
 	AllowTopLevelPaidExecutionFrom<Everything>,
 	AllowKnownQueryResponses<PolkadotXcm>,

From aace0f72b21bc0b6d34b87adccbd197f0d7ff832 Mon Sep 17 00:00:00 2001
From: Hector Bulgarini <hbulgarini@gmail.com>
Date: Wed, 30 Aug 2023 17:17:15 +0200
Subject: [PATCH 2/2] Last comment

---
 runtime/src/xcm_config.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/runtime/src/xcm_config.rs b/runtime/src/xcm_config.rs
index 18bb43c..23574ae 100644
--- a/runtime/src/xcm_config.rs
+++ b/runtime/src/xcm_config.rs
@@ -208,7 +208,7 @@ type Barrier = (
 	TakeWeightCredit,
 	/// < SBP MR2
 	/// 
-	/// AllowTopLevelPaidExecutionFrom<Everything> duplicated.
+	/// Remove AllowTopLevelPaidExecutionFrom<Everything> duplicated.
 	/// 
 	/// >
 	AllowTopLevelPaidExecutionFrom<Everything>,