Merge pull request #115 from hbulgarini/main

SBP Milestone Review 3

pallets/briefs/src/lib.rs

@@ -149,6 +149,11 @@ pub mod pallet {
     impl<T: Config> Pallet<T> {
         /// Approve an account so that they can be accepted as an applicant.
         #[pallet::call_index(1)]
+        /// <HB SBP Review:
+        /// 
+        /// It seems you guys forgot to add the weight from the benchmarking result.
+        /// 
+        /// >
         #[pallet::weight(10_000)]
         pub fn add_to_fellowship(
             origin: OriginFor<T>,
@@ -185,6 +190,13 @@ pub mod pallet {
                 Error::<T>::BriefAlreadyExists
             );
 
+            /// <HB SBP Review:
+            /// 
+            /// Re: sp_arithmetic library
+            /// For the portion of the code below just acummulating the total percentage of the milestones with u32 seems to be enough, 
+            /// but using the sp_arithmetic library is a safer practice.
+            /// 
+            /// >
             // Validation
             let total_percentage = milestones
                 .iter()
@@ -209,10 +221,25 @@ pub mod pallet {
             //     Error::<T>::OnlyApprovedAccountPermitted
             // );
 
+            /// <HB SBP Review:
+            /// 
+            /// Usually balances reverves are fixed and determined at the runtime level since it is supposed to be a storage sanity measure.
+            /// With the current design i could just reserve 0.000001 USD and that would be still chip to attack the network.
+            /// As you are working in a multi-currency environment, i would suggest creating a new pallet that might define reserve values per currency.
+            /// This new pallet would require root origin and it might be called from goverance chain. 
+            /// Or another option would be to only accept deposits in the native currency of the chain.
+            /// 
+            /// >
             <T as Config>::RMultiCurrency::reserve(currency_id, &who, initial_contribution)?;
 
+            /// <HB SBP Review:
+            /// 
+            /// Please avoid this kind of hardcodes as 0u32.
+            /// You can use : Zero::zero()
+            /// use sp_runtime::traits::{Zero};
+            /// >
             if initial_contribution > 0u32.into() {
-                BriefContributions::<T>::try_mutate(brief_id, |contributions| {
+                 ::<T>::try_mutate(brief_id, |contributions| {
                     // This should never fail as the the bound is ensured when a brief is created.
                     let _ = contributions
                         .try_insert(
@@ -264,6 +291,10 @@ pub mod pallet {
                 Error::<T>::NotAuthorised
             );
 
+            /// <HB SBP Review:
+            /// 
+            /// Same as the previous comment, please about reserves amount.
+            /// >
             <T as Config>::RMultiCurrency::reserve(brief_record.currency_id, &who, amount)?;
 
             BriefContributions::<T>::try_mutate(brief_id, |contributions| {

pallets/grants/src/lib.rs

@@ -1,6 +1,5 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
-
 pub use pallet::*;
 
 #[cfg(test)]
@@ -21,7 +20,6 @@ mod test_utils;
 pub mod weights;
 pub use weights::*;
 
-
 #[frame_support::pallet]
 pub mod pallet {
     use super::*;
@@ -145,6 +143,13 @@ pub mod pallet {
         ) -> DispatchResult {
             let submitter = ensure_signed(origin)?;
 
+            /// <HB SBP Review:
+            ///
+            /// Re: sp_arithmetic library
+            /// For the portion of the code below just acummulating the total percentage of the milestones with u32 seems to be enough,
+            /// but using the sp_arithmetic library is a safer practice.
+            ///
+            /// >
             let total_percentage = proposed_milestones
                 .iter()
                 .fold(0u32, |acc, x| acc.saturating_add(x.percentage_to_unlock));

pallets/proposals/src/impls.rs

@@ -5,6 +5,13 @@ use sp_runtime::traits::Saturating;
 use sp_std::{collections::btree_map::BTreeMap, vec};
 pub const MAX_PERCENTAGE: u32 = 100u32;
 use scale_info::prelude::format;
+
+/// <HB SBP Review:
+///
+/// I would use checked_div for some divisions to be sure.
+///
+/// >
+
 impl<T: Config> Pallet<T> {
     /// The account ID of the fund pot.
     ///
@@ -143,6 +150,11 @@ impl<T: Config> Pallet<T> {
         project.currency_id = currency_id;
         project.agreement_hash = agreement_hash;
 
+        /// <HB SBP Review:
+        ///
+        /// Maybe instead of using inset, this is a good candidate for try_mutate as well?
+        ///
+        /// >
         // Add project to list
         <Projects<T>>::insert(project_key, project);
 
@@ -196,6 +208,11 @@ impl<T: Config> Pallet<T> {
         project_key: ProjectKey,
         value: BalanceOf<T>,
     ) -> DispatchResultWithPostInfo {
+        /// <HB SBP Review:
+        ///
+        /// I would avoid hardocoding types of this kind. Please use Zero::zero() instead.
+        ///
+        /// >
         // TODO add configurable value for min and max contribution per contributor
         ensure!(value > (0_u32).into(), Error::<T>::InvalidParam);
         let now = <frame_system::Pallet<T>>::block_number();
@@ -499,6 +516,11 @@ impl<T: Config> Pallet<T> {
             Error::<T>::MilestoneDoesNotExist
         );
 
+        /// <HB SBP Review:
+        ///
+        /// Please remove this unwrap and manage the error properly.
+        ///
+        /// >
         let mut milestone = project.milestones.get_mut(&milestone_key).unwrap().clone();
 
         // set is_approved
@@ -572,6 +594,11 @@ impl<T: Config> Pallet<T> {
             Error::<T>::NoAvailableFundsToWithdraw
         );
 
+        /// HB SBP Review:
+        ///
+        /// This is a good example about how sp_arithmetic can be used to manage percentages in a safe way.
+        ///
+        /// >
         let fee = withdrawable.saturating_mul(<T as Config>::ImbueFee::get().into())
             / MAX_PERCENTAGE.into();
         let withdrawn = withdrawable.saturating_sub(fee);
@@ -631,6 +658,11 @@ impl<T: Config> Pallet<T> {
         for (who, contribution) in project.contributions.iter() {
             let project_account_id = Self::project_account_id(project_key);
 
+            /// <HB SBP Review:
+            ///
+            /// Unsafe operation. Please use checked_mul or saturated_mul instead.
+            /// And for divisions i always try to use cheched_div just to be sure.
+            /// >
             let refund_amount: BalanceOf<T> =
                 ((contribution).value * locked_milestone_percentage.into()) / MAX_PERCENTAGE.into();
 

pallets/proposals/src/lib.rs

@@ -36,6 +36,12 @@ pub mod migration;
 pub mod impls;
 pub use impls::*;
 
+/// <HB SBP Review:
+///
+///
+/// Why are these two constants not configurable as the others?
+///
+/// >
 // The Constants associated with the bounded parameters
 type MaxProjectKeysPerRound = ConstU32<1000>;
 type MaxWhitelistPerProject = ConstU32<10000>;
@@ -61,6 +67,12 @@ type BoundedProjectKeys = BoundedVec<ProjectKey, MaxProjectKeysPerRound>;
 type BoundedMilestoneKeys<T> = BoundedVec<ProjectKey, <T as Config>::MaxMilestonesPerProject>;
 pub type BoundedProposedMilestones<T> =
     BoundedVec<ProposedMilestone, <T as Config>::MaxMilestonesPerProject>;
+
+/// <HB SBP Review:
+///
+/// I think the project is missing a primitives.rs file where all these kind of definitions should be placed.
+///
+/// >
 pub type AgreementHash = H256;
 
 #[frame_support::pallet]
@@ -111,13 +123,18 @@ pub mod pallet {
 
         /// The storage deposit taken when a project is created and returned on deletion/completion.
         type ProjectStorageDeposit: Get<BalanceOf<Self>>;
-        
+
         // Imbue fee in percent 0-99
         type ImbueFee: Get<u8>;
     }
 
     #[pallet::pallet]
     #[pallet::generate_store(pub(super) trait Store)]
+    /// <HB SBP Review:
+    ///
+    /// CRITICAL: This macro should be removed asap. This basically allows storing unbounded Vecs on storage items.
+    ///
+    /// >
     #[pallet::without_storage_info]
     pub struct Pallet<T>(PhantomData<T>);
 
@@ -183,6 +200,11 @@ pub mod pallet {
     #[pallet::getter(fn refund_queue)]
     pub type RefundQueue<T> = StorageValue<
         _,
+        /// <HB SBP Review:
+        ///
+        /// Unbounded Vec on a storage item. This should be addressed before deploying.
+        ///
+        /// >
         Vec<(
             AccountIdOf<T>,
             ProjectAccountId<T>,
@@ -321,6 +343,11 @@ pub mod pallet {
 
     #[pallet::hooks]
     impl<T: Config> Hooks<T::BlockNumber> for Pallet<T> {
+        /// <HB SBP Review:
+        /// 
+        /// I see this hook valid on testnet but if you will deploy this with weights v2 already, you can totally remove this.
+        /// 
+        /// >
         fn on_runtime_upgrade() -> Weight {
             let mut weight = T::DbWeight::get().reads_writes(1, 1);
             if StorageVersion::<T>::get() == Release::V0
@@ -347,6 +374,14 @@ pub mod pallet {
             currency_id: common_types::CurrencyId,
         ) -> DispatchResultWithPostInfo {
             let who = ensure_signed(origin)?;
+
+            /// <HB SBP Review:
+            /// 
+            /// Re: sp_arithmetic library
+            /// For the portion of the code below just acummulating the total percentage of the milestones with u32 seems to be enough, 
+            /// but using the sp_arithmetic library is a safer practice.
+            /// 
+            /// >
             // Validation
             let total_percentage = proposed_milestones
                 .iter()
@@ -358,6 +393,11 @@ pub mod pallet {
                 Error::<T>::MilestonesTotalPercentageMustEqual100
             );
 
+            /// <HB SBP Review:
+            /// 
+            /// Remove the let_ ?
+            /// 
+            /// >
             let _ = Self::new_project(
                 // TODO: Optimise
                 who,
@@ -382,6 +422,13 @@ pub mod pallet {
         ) -> DispatchResultWithPostInfo {
             let who = ensure_signed(origin)?;
 
+            /// <HB SBP Review:
+            /// 
+            /// Re: sp_arithmetic library
+            /// For the portion of the code below just acummulating the total percentage of the milestones with u32 seems to be enough, 
+            /// but using the sp_arithmetic library is a safer practice.
+            /// 
+            /// >
             let total_percentage = proposed_milestones
                 .iter()
                 .fold(0, |acc: u32, ms: &ProposedMilestone| {
@@ -504,6 +551,11 @@ pub mod pallet {
         /// Contribute to a project
         #[pallet::call_index(6)]
         #[pallet::weight(<T as Config>::WeightInfo::contribute())]
+        /// <HB SBP Review:
+        /// 
+        /// It is not needed anymore to use the transactional macro anymore since it is already added by default for every extrinsic.
+        /// 
+        /// >
         #[transactional]
         pub fn contribute(
             origin: OriginFor<T>,
@@ -650,6 +702,11 @@ pub enum RoundType {
     VoteOfNoConfidence,
 }
 
+/// <HB SBP Review:
+/// 
+/// I suspect this comes from the weights v2 migration?
+/// 
+/// >
 #[derive(Encode, Decode, TypeInfo, PartialEq)]
 #[repr(u32)]
 pub enum Release {
@@ -664,6 +721,11 @@ impl Default for Release {
     }
 }
 
+/// <HB SBP Review:
+///
+/// This Round struct is storing an unbounded Vec. Please bound all your vecs.
+///
+/// >
 /// The round struct contains all the data associated with a given round.
 /// A round may include multiple projects.
 #[derive(Encode, Decode, PartialEq, Eq, Clone, Debug, TypeInfo)]
@@ -692,6 +754,13 @@ impl<BlockNumber: From<u32>> Round<BlockNumber> {
     }
 }
 
+/// <HB SBP Review:
+///
+/// I would recommend to consider library sp_arithmetic for all what it is percentage related: https://docs.rs/sp-arithmetic/15.0.0/sp_arithmetic/per_things/index.html
+/// This would give more flexibility and safety at the time to maniputale percentages.
+///
+/// >
+
 /// The milestones provided by the user to define the milestones of a project.
 /// TODO: add ipfs hash like in the grants pallet and
 /// TODO: move these to a common repo (common_types will do)
@@ -722,6 +791,11 @@ pub struct Vote<Balance> {
 impl<Balance: From<u32>> Default for Vote<Balance> {
     fn default() -> Self {
         Self {
+            /// <HB SBP Review:
+            /// 
+            /// I would avoid hardocoding types of this kind. Please use Zero::zero() instead.
+            /// 
+            /// >
             yay: (0_u32).into(),
             nay: (0_u32).into(),
             is_approved: false,