diff --git a/src/benchmarking.rs b/src/benchmarking.rs index 51a80cf..bdaa9c3 100644 --- a/src/benchmarking.rs +++ b/src/benchmarking.rs @@ -15,8 +15,14 @@ const CALLER_ACCOUNT_STR: &str = "Iredia1"; const ITEM_TYPE_BYTES: &[u8; 4] = b"test"; const ITEM_BYTES: &[u8; 9] = b"123456789"; +// SBP M3 Review: For benchmarking, we should try to cover worst case scenario +// For example: Here we can use maximum length of string for item type and item + benchmarks! { add_item { + // SBP M3 Review: Let's use Substrate way of benchmarking account id + // let caller: T::AccountId = whitelisted_caller(); + // please see https://github.com/paritytech/substrate/blob/ea3ca3f757ff9d9559665719a77da81f4cf0f0ce/bin/node-template/pallets/template/src/benchmarking.rs#L13 let caller: T::AccountId = account(CALLER_ACCOUNT_STR,0, 0); }: _(RawOrigin::Signed(caller.clone()), ITEM_TYPE_BYTES.to_vec(), ITEM_BYTES.to_vec()) diff --git a/src/lib.rs b/src/lib.rs index 9312be3..eb7c7de 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,18 +1,27 @@ +// SBP M3 Review: Add copyrights + +// SBP M3 Review: Pallet documentation should be more descriptive. For example: You can add `Overview`, `Goals`, information about dispatchable functions etc +// You can refer any Substrate pallet + //! # PEAQ Storage Pallet //! //! The Storage pallet allows storing and managing IPFS CID ( content identifiers ) on the blockchain. #![cfg_attr(not(feature = "std"), no_std)] +// SBP M3 Review: Please run `cargo fmt` to improve formatting, remove extra spaces/lines +// SBP M3 Review: File names are very generic. They should be very specific. pub mod structs; pub mod enums; pub mod traits; #[cfg(test)] +// SBP M3 Review: Please run `cargo fmt` mod mock; #[cfg(feature = "runtime-benchmarks")] +// SBP M3 Review: Please run `cargo fmt` mod benchmarking; pub mod weights; @@ -23,6 +32,7 @@ pub use pallet::*; #[frame_support::pallet] pub mod pallet { + // SBP M3 Review: Better to use `use super::*;` use super::WeightInfo; use crate::enums::StorageError; use crate::traits::*; @@ -40,22 +50,26 @@ pub mod pallet { type WeightInfo: WeightInfo; } + // SBP M3 Review: Remove below description // Pallets use events to inform users when important changes are made. // Event documentation should end with an array that provides descriptive names for parameters. // https://docs.substrate.io/v3/runtime/events-and-errors #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { + // SBP M3 Review: It should be something like 'A storage item has been added. [who, item_type, item]' /// Event emitted when an storage item has been added. [who, item_type, item] ItemAdded( T::AccountId, Vec, Vec, ), + // SBP M3 Review: This is not needed. Please read review comment of `get_item` /// Event emitted when an item is read successfully ItemRead( Vec, ), + // SBP M3 Review: It should be something like 'An item has been updated. [who, item_type, item]' /// Event emitted when an item has been updated. [who, item_type, item] ItemUpdated( T::AccountId, @@ -66,19 +80,24 @@ pub mod pallet { #[pallet::error] pub enum Error { + // SBP M3 Review: Outer line doc should start with 3 slashes // Item not found with the given account and item_type ItemNotFound, + // SBP M3 Review: Outer line doc should start with 3 slashes // Item already exists with the given account and item_type ItemTypeAlreadyExists, + // SBP M3 Review: Outer line doc should start with 3 slashes // Item type is greater that 64 ItemTypeExceedMax64, + // SBP M3 Review: Outer line doc should start with 3 slashes // Item is greater that 128 ItemExceedMax128, } + // SBP M3 Review: This is not needed impl Error { fn dispatch_error(err: StorageError) -> DispatchResult { match err { @@ -94,18 +113,25 @@ pub mod pallet { pub struct Pallet(_); #[pallet::storage] + // SBP M3 Review: function name should be 'item_store'. + // It should follow substrate storage naming convention. Please read https://docs.substrate.io/build/runtime-storage/#declaring-storage-items #[pallet::getter(fn item_of)] pub(super) type ItemStore = StorageMap< _, Blake2_128Concat, [u8; 32], + // SBP M3 Review: Please use BoundedVec(https://crates.parity.io/frame_support/storage/bounded_vec/struct.BoundedVec.html) rather than Vec<_>. + // For example: here you can use 'BoundedVec' + // Take this https://github.com/paritytech/substrate/blob/6e73c85b7ddbc2ec2a9f6629ddc06aca2f83bcf3/frame/assets/src/lib.rs#L334 as an example Vec, ValueQuery, >; + // SBP M3 Review: Why this is needed? #[pallet::hooks] impl Hooks> for Pallet {} + // SBP M3 Review: Remove below description // Dispatchable functions allow users to interact with the pallet and invoke state changes. // These functions materialize as "extrinsics", which are often compared to transactions. // Dispatchable functions must be annotated with a weight and must return a DispatchResult. @@ -115,14 +141,37 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::add_item())] pub fn add_item( origin: OriginFor, + // SBP M3 Review: Its a good practice to put both variable in a struct item_type: Vec, item: Vec, ) -> DispatchResult { + // SBP M3 Review: This description is not needed // Check that an extrinsic was signed and get the signer // This fn returns an error if the extrinsic is not signed // https://docs.substrate.io/v3/runtime/origins let sender = ensure_signed(origin)?; + // SBP M3 Review: If you have bounded vec in storage, + // you can use below pattern for validation and storage. This will be more clean and substarte way. + // let item: BoundedVec> = + // item.try_into().map_err(|_| Error::::ItemExceedMax128)?; + // + // let item_type: BoundedVec> = + // item_type.try_into().map_err(|_| Error::::ItemTypeExceedMax64)?; + // + // let id = Self::get_hashed_key(&sender, item_type.to_vec()); + // + // // Check if item already exists with the given account and item_type + // ensure!(!>::contains_key(&id), Error::::ItemTypeAlreadyExists); + // + // >::insert(&id, item.clone()); + // + // Self::deposit_event(Event::ItemAdded( + // sender, + // item_type.to_vec(), + // item.to_vec() + // )); + ensure!(item_type.len() <= 64, Error::::ItemTypeExceedMax64); ensure!(item.len() <= 128, Error::::ItemExceedMax128); @@ -140,6 +189,7 @@ pub mod pallet { Ok(()) } + // SBP M3 Review: `add_item` feedback also applies here. Please refactor accordingly /// Update an existing item in the storage #[pallet::weight(T::WeightInfo::update_item())] pub fn update_item( @@ -168,7 +218,19 @@ pub mod pallet { Ok(()) } - /// Read storage item + // SBP M3 Review: It is strongly not recommended to have dispatchable function to + // read item from storage. We can either use getter function or expose RPC end point + // Read storage item. + // For example: If we have a storage map: + // ``` + // #[pallet::storage] + // #[pallet::getter(fn product_information)] + // pub type ProductInformation = StorageMap<_, Blake2_128Concat, T::Hash, Product>; + // ``` + // We can read the item by caling `Self::product_information(id)`. + // + // Please refer this https://docs.substrate.io/build/runtime-storage/#declaring-storage-items + // to get better understanding of declaring storage items in Substarte pallet. #[pallet::weight(T::WeightInfo::get_item())] pub fn get_item( origin: OriginFor, @@ -190,6 +252,7 @@ pub mod pallet { } } + // SBP M3 Review: After addressing above review comments, we won't need these methods. // implements the Storage trait to satisfied the required methods impl Storage for Pallet @@ -246,6 +309,7 @@ pub mod pallet { None } + // SBP M3 Review: This is more like a utility function and can be moved in primitives or common util class fn get_hashed_key(account: &T::AccountId, value: &[u8]) -> [u8; 32] { let mut bytes_in_value: Vec = value.to_vec(); let mut bytes_to_hash: Vec = account.encode().as_slice().to_vec();