HB SBP Milestone review #2

pallets/hyperdrive/src/lib.rs

@@ -334,6 +334,13 @@ pub mod pallet {
         /// We fail with a [`DispatchError`] if the given `proof` is invalid.
         /// Any error happening afterwards, while decoding the payload and triggering actions, emits an event informing about the error but does not fail the extrinsic.
         /// This is necessary to make [`MessageSequenceId`] update in any case.
+
+        /// <SBP Milestone Review II
+        ///
+        /// The submit_message() benchmark is defined here but however it is not defined in the benchmarks.rs file.   
+        /// Is it possible that the benchmark is not implemented yet or do i'm missing anything?
+        ///
+        /// >
         #[pallet::call_index(2)]
         #[pallet::weight(< T as Config<I>>::WeightInfo::submit_message())]
         pub fn submit_message(
@@ -368,6 +375,11 @@ pub mod pallet {
 
         /// Updates the target chain owner (contract address) in storage. Can only be called by a privileged/root account.
         #[pallet::call_index(3)]
+        /// <SBP Milestone Review II
+        ///
+        /// Missing benchmarking.
+        ///
+        /// >
         #[pallet::weight(Weight::from_parts(10_000, 0).saturating_add(T::DbWeight::get().reads_writes(1, 0)))]
         pub fn update_target_chain_owner(
             origin: OriginFor<T>,
@@ -430,6 +442,11 @@ pub mod pallet {
             Self::process_action(payload_bytes)
         }
 
+        /// <HB SBP Milestone Review II
+        ///
+        /// You can remove the transactional macro since it's already happening out of the box.
+        ///
+        /// >
         #[transactional]
         fn process_action(message_bytes: &Vec<u8>) -> Result<(), ProcessMessageResult> {
             let action = T::MessageParser::parse_value(message_bytes)

pallets/marketplace/src/lib.rs

@@ -96,6 +96,13 @@ pub mod pallet {
 
     pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(4);
 
+    /// <HB SBP Milestone Review II
+    ///
+    /// Important: Macro `without_storage_info` is being decomissioned and it should not be used since it allows unbounded vecs
+    /// that might cause the PoV to exceed the block  size limit (whole point of Weights V2)
+    ///
+    /// Please check: https://github.com/paritytech/substrate/issues/8629
+    /// >
     #[pallet::pallet]
     #[pallet::without_storage_info]
     #[pallet::storage_version(STORAGE_VERSION)]
@@ -360,6 +367,11 @@ pub mod pallet {
         }
     }
 
+    /// <HB SBP Milestone Review II
+    ///
+    /// Please do not forget to remove this once the runtime upgrade is enacted.
+    ///
+    /// >
     #[pallet::hooks]
     impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
         fn on_runtime_upgrade() -> frame_support::weights::Weight {
@@ -413,6 +425,12 @@ pub mod pallet {
 
         /// Proposes processors to match with a job. The match fails if it conflicts with the processor's schedule.
         #[pallet::call_index(2)]
+        /// <SBP Milestone Review II
+        ///
+        /// The propose_matching() benchmark is defined here but however it is not defined in the benchmarks.rs file.   
+        /// Is it possible that the benchmark is not implemented yet or do i'm missing anything?
+        ///
+        /// >
         #[pallet::weight(< T as Config >::WeightInfo::propose_matching())]
         pub fn propose_matching(
             origin: OriginFor<T>,
@@ -486,6 +504,12 @@ pub mod pallet {
         /// the report is accepted if `[now, now + tolerance]` overlaps with an execution of the schedule agreed on.
         /// `tolerance` is a pallet config value.
         #[pallet::call_index(4)]
+        /// <SBP Milestone Review II
+        ///
+        /// The report() benchmark is defined here but however it is not defined in the benchmarks.rs file.   
+        /// Is it possible that the benchmark is not implemented yet or do i'm missing anything?
+        ///
+        /// >
         #[pallet::weight(< T as Config >::WeightInfo::report())]
         pub fn report(
             origin: OriginFor<T>,
@@ -573,6 +597,12 @@ pub mod pallet {
 
         /// Called by processors when the assigned job can be finalized.
         #[pallet::call_index(5)]
+        /// <SBP Milestone Review II
+        ///
+        /// The finalize_job() benchmark is defined here but however it is not defined in the benchmarks.rs file.   
+        /// Is it possible that the benchmark is not implemented yet or do i'm missing anything?
+        ///
+        /// >
         #[pallet::weight(<T as Config>::WeightInfo::finalize_job())]
         pub fn finalize_job(
             origin: OriginFor<T>,
@@ -654,6 +684,12 @@ pub mod pallet {
         ///
         /// For details see [`Pallet<T>::finalize_jobs_for`].
         #[pallet::call_index(6)]
+        /// <SBP Milestone Review II
+        ///
+        /// The finalize_jobs() benchmark is defined here but however it is not defined in the benchmarks.rs file.   
+        /// Is it possible that the benchmark is not implemented yet or do i'm missing anything?
+        ///
+        /// >
         #[pallet::weight(<T as Config>::WeightInfo::finalize_jobs())]
         pub fn finalize_jobs(
             origin: OriginFor<T>,
@@ -675,6 +711,12 @@ pub mod pallet {
         }
     }
 
+    /// <HB SBP Milestone Review II
+    ///
+    /// Have you considerded moving this impl into a separate file as impls.rs?
+    /// IMHO it would make the code more readable.
+    ///     
+    /// /// >
     impl<T: Config> JobHooks<T> for Pallet<T> {
         /// Registers a job in the marketplace by providing a [JobRegistration].
         /// If a job for the same `(accountId, script)` was previously registered, it will be overwritten.

pallets/marketplace/src/migration.rs

@@ -64,6 +64,12 @@ fn migrate_to_v2<T: Config>() -> Weight {
     T::DbWeight::get().reads_writes(count + 1, count + 1)
 }
 
+/// <HB SBP Milestone Review II
+///
+/// I would strongly recommend to test this migration with a forked chain with a tool like chopsticks.
+/// It's important to make sure that the migration is not taking all the weights available in the block(s).
+///
+/// >
 fn migrate_to_v3<T: Config>() -> Weight {
     let mut count = 0u32;
     count += StoredJobStatus::<T>::clear(10_000, None).loops;

pallets/marketplace/src/types.rs

@@ -70,6 +70,11 @@ pub struct AdvertisementRestriction<AccountId> {
     /// Storage capacity in bytes not to be exceeded in matching. The associated fee is listed in [pricing].
     pub storage_capacity: u32,
     /// An optional array of the [AccountId]s of consumers whose jobs should get accepted. If the array is [None], then jobs from all consumers are accepted.
+    /// <HB SBP Milestone Review II
+    ///
+    /// Please bound this Vec.
+    ///
+    /// >
     pub allowed_consumers: Option<Vec<MultiOrigin<AccountId>>>,
     /// The modules available to the job on processor.
     pub available_modules: JobModules,
@@ -185,6 +190,11 @@ pub struct JobRequirements<Reward, AccountId> {
     pub min_reputation: Option<u128>,
     /// Optional match provided with the job requirements. If provided, it gets processed instantaneously during
     /// registration call and validation errors lead to abortion of the call.
+    /// <HB SBP Milestone Review II
+    ///
+    /// Please bound this Vec.
+    ///
+    /// >
     pub instant_match: Option<Vec<PlannedExecution<AccountId>>>,
 }
 
@@ -194,6 +204,11 @@ pub struct Match<AcurastAccountId> {
     /// The job to match.
     pub job_id: JobId<AcurastAccountId>,
     /// The sources to match each of the job's slots with.
+    /// <HB SBP Milestone Review II
+    ///
+    /// Please bound this Vec.
+    ///
+    /// >
     pub sources: Vec<PlannedExecution<AcurastAccountId>>,
 }
 
@@ -205,6 +220,11 @@ pub struct Match<AcurastAccountId> {
 #[derive(RuntimeDebug, Encode, Decode, TypeInfo, Clone, PartialEq)]
 pub struct PartialJobRegistration<Reward, AccountId> {
     /// An optional array of the [AccountId]s allowed to fulfill the job. If the array is [None], then all sources are allowed.
+    /// <HB SBP Milestone Review II
+    ///
+    /// Please bound this Vec.
+    ///
+    /// >
     pub allowed_sources: Option<Vec<AccountId>>,
     /// A boolean indicating if only verified sources can fulfill the job. A verified source is one that has provided a valid key attestation.
     pub allow_only_verified_sources: bool,

pallets/marketplace/src/weights.rs

@@ -59,6 +59,11 @@ impl<T: frame_system::Config> WeightInfo for Weights<T> {
             .saturating_add(T::DbWeight::get().reads(2))
             .saturating_add(T::DbWeight::get().writes(3))
     }
+    /// <HB SBP Milestone Review II
+    ///
+    /// From now on all the ref_time values are the same. I suspect those values are used as placeholders and not as a result of a benchmarking execution?
+    ///     
+    /// /// >
     fn propose_matching() -> Weight {
         // Minimum execution time:  nanoseconds.
         Weight::from_parts(129_864_000, 0)