Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SBP Milestone III review #8

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

SBP Milestone III review #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6afb345
Milestone III + review comment
ayushmishra2005 Dec 8, 2022
61e42b9
Update lib.rs
ayushmishra2005 Dec 14, 2022
3066bdf
Update lib.rs
ayushmishra2005 Dec 14, 2022
4729dc3
Update lib.rs
ayushmishra2005 Dec 14, 2022
8166bb3
Update lib.rs
ayushmishra2005 Dec 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
66 changes: 65 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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::*;
Expand All @@ -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<T: Config> {
// 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<u8>,
Vec<u8>,
),
// SBP M3 Review: This is not needed. Please read review comment of `get_item`
/// Event emitted when an item is read successfully
ItemRead(
Vec<u8>,
),
// 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,
Expand All @@ -66,19 +80,24 @@ pub mod pallet {

#[pallet::error]
pub enum Error<T> {
// 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<T: Config> Error<T> {
fn dispatch_error(err: StorageError) -> DispatchResult {
match err {
Expand All @@ -94,18 +113,25 @@ pub mod pallet {
pub struct Pallet<T>(_);

#[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<T: Config> = 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<u8, Configurable length>'
// Take this https://github.com/paritytech/substrate/blob/6e73c85b7ddbc2ec2a9f6629ddc06aca2f83bcf3/frame/assets/src/lib.rs#L334 as an example
Vec<u8>,
ValueQuery,
>;

// SBP M3 Review: Why this is needed?
#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}

// 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.
Expand All @@ -115,14 +141,37 @@ pub mod pallet {
#[pallet::weight(T::WeightInfo::add_item())]
pub fn add_item(
origin: OriginFor<T>,
// SBP M3 Review: Its a good practice to put both variable in a struct
item_type: Vec<u8>,
item: Vec<u8>,
) -> 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<u8, ConstU32<128>> =
// item.try_into().map_err(|_| Error::<T>::ItemExceedMax128)?;
//
// let item_type: BoundedVec<u8, ConstU32<64>> =
// item_type.try_into().map_err(|_| Error::<T>::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!(!<ItemStore<T>>::contains_key(&id), Error::<T>::ItemTypeAlreadyExists);
//
// <ItemStore<T>>::insert(&id, item.clone());
//
// Self::deposit_event(Event::ItemAdded(
// sender,
// item_type.to_vec(),
// item.to_vec()
// ));

ensure!(item_type.len() <= 64, Error::<T>::ItemTypeExceedMax64);
ensure!(item.len() <= 128, Error::<T>::ItemExceedMax128);

Expand All @@ -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(
Expand Down Expand Up @@ -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<T: Config> = StorageMap<_, Blake2_128Concat, T::Hash, Product<T::AccountId, T::Hash>>;
// ```
// 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<T>,
Expand All @@ -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<T: Config> Storage<T::AccountId>
for Pallet<T>
Expand Down Expand Up @@ -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<u8> = value.to_vec();
let mut bytes_to_hash: Vec<u8> = account.encode().as_slice().to_vec();
Expand Down