From fdad00056fa0f5187a097b79415800c992e6254b Mon Sep 17 00:00:00 2001 From: Islam Date: Sat, 30 Apr 2022 09:19:30 +0000 Subject: [PATCH 1/2] (feat) Included review score onchain. * Added review score in review struct * Added number of reviews and sum of score for easy average on project * Cleaned up genesis config to depend on extrinsics * Removed expect(s) of collateralise to return err instead. * Increase user rank point by one in reward_user * Reordered mutations in a way that the least likely to err are called last. * Added more tests to chocolate pallet **Review rewarding logic for a more graceful way of handling rewarding * ToDo: Move docs on projectIO impl to trait definition. --- pallets/chocolate/Cargo.toml | 4 +- pallets/chocolate/src/constants.rs | 10 +- pallets/chocolate/src/lib.rs | 147 ++++++++++------------- pallets/chocolate/src/mock.rs | 4 +- pallets/chocolate/src/tests.rs | 8 +- pallets/users/src/lib.rs | 2 - primitives/chocolate-projects/Cargo.toml | 8 +- primitives/chocolate-projects/src/lib.rs | 8 +- primitives/chocolate-users/src/lib.rs | 11 +- types.json | 7 +- 10 files changed, 97 insertions(+), 112 deletions(-) diff --git a/pallets/chocolate/Cargo.toml b/pallets/chocolate/Cargo.toml index 32bed72..8006935 100644 --- a/pallets/chocolate/Cargo.toml +++ b/pallets/chocolate/Cargo.toml @@ -91,8 +91,8 @@ version = '0.1.0' path = '../../primitives/chocolate-projects' default-features = false version = '0.1.0' -# # added dep - is this tight coupling?? -[dependencies.pallet-users] +# # added dep - required for testing +[dev-dependencies.pallet-users] default-features = false path = '../users' version = '0.1.0' diff --git a/pallets/chocolate/src/constants.rs b/pallets/chocolate/src/constants.rs index 9726408..bc2c741 100644 --- a/pallets/chocolate/src/constants.rs +++ b/pallets/chocolate/src/constants.rs @@ -7,10 +7,10 @@ pub mod project { b"QmVzxGUaVF4HVfvtoaVqXBcVGqyEq72TAp8s9u9pmH5Vra", ]; - pub const REVS: [&[u8]; 4] = [ - b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review1.json", - b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review2.json", - b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review3.json", - b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review4.json", + pub const REVS: [(u8,&[u8]); 4] = [ + (3,b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review1.json"), + (5,b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review2.json"), + (5,b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review3.json"), + (3,b"QmdKx4pmnJUP5GdjtpJE2ei4xeaRKQWYwvXGuVY1AbAwDM/review4.json"), ]; } diff --git a/pallets/chocolate/src/lib.rs b/pallets/chocolate/src/lib.rs index 8ace640..9e22f1e 100644 --- a/pallets/chocolate/src/lib.rs +++ b/pallets/chocolate/src/lib.rs @@ -23,6 +23,7 @@ pub mod pallet { use chocolate_projects::*; use chocolate_users::UserIO; use frame_support::{ + assert_ok, dispatch::DispatchResult, pallet_prelude::*, sp_runtime::traits::Saturating, @@ -30,7 +31,7 @@ pub mod pallet { Currency, ExistenceRequirement::KeepAlive, Imbalance, OnUnbalanced, ReservableCurrency, }, }; - use frame_system::pallet_prelude::*; + use frame_system::{pallet_prelude::*, Origin}; use sp_runtime::{traits::CheckedDiv, ArithmeticError}; use sp_std::str; use sp_std::vec::Vec; @@ -135,6 +136,8 @@ pub mod pallet { AcceptingNotProposed, /// The checked division method failed, either due to overflow/underflow or because of division by zero. CheckedDivisionFailed, + /// Review score is out of range 1-5 + ReviewScoreOutOfRange, } // Dispatchable functions must be annotated with a weight and must return a DispatchResult. #[pallet::call] @@ -142,7 +145,7 @@ pub mod pallet { /// Create a project /// /// - O(1). - /// - Init: Index starts at 0 + /// - Init: Index starts at 1 #[pallet::weight(10_000 + T::DbWeight::get().reads_writes(1,3))] pub fn create_project(origin: OriginFor, project_meta: Vec) -> DispatchResult { let who = ensure_signed(origin)?; @@ -169,7 +172,7 @@ pub mod pallet { #[pallet::weight(10_000 + T::DbWeight::get().reads_writes(2,3))] pub fn create_review( origin: OriginFor, - review_meta: Vec, + review_meta: (u8, Vec), project_id: ProjectID, ) -> DispatchResult { let who = ensure_signed(origin)?; @@ -178,6 +181,7 @@ pub mod pallet { >::get(project_id).ok_or(Error::::NoProjectWithId)?; ensure!(!>::contains_key(&who, project_id), Error::::DuplicateReview); ensure!(this_project.owner_id.ne(&who), Error::::OwnerReviewedProject); + ensure!(review_meta.0 <= 5 && review_meta.0 >= 1, Error::::ReviewScoreOutOfRange); let reserve = Pallet::::can_collateralise(&who)?; // Fallible MUTATIONS Pallet::::collateralise(&who, reserve)?; @@ -190,10 +194,11 @@ pub mod pallet { project_id, Review { user_id: who.clone(), - content: review_meta, + content: review_meta.1, project_id, proposal_status: Default::default(), point_snapshot: user.rank_points, + review_score: review_meta.0, }, ); >::mutate(project_id, |project| { @@ -227,6 +232,8 @@ pub mod pallet { Pallet::::reward_user(&user_id, &mut project, &review)?; review.proposal_status.status = Status::Accepted; review.proposal_status.reason = Reason::PassedRequirements; + project.number_of_reviews= project.number_of_reviews.saturating_add(1); + project.total_review_score= project.total_review_score.saturating_add(u64::from(review.review_score)); // STORAGE MUTATIONS >::mutate(&user_id, project_id, |r| { *r = Option::Some(review); @@ -310,11 +317,11 @@ pub mod pallet { if _missing_reward > BalanceOf::::from(0u32) { // assuming our can_unreserve failed // rollback ---- + // It Should be enough to rollback following our initial unreserve T::Currency::reserve( &project_struct.owner_id, amount.saturating_sub(_missing_reward), - ) - .expect("Should be enough to rollback following our initial unreserve"); + )?; return Err(Error::::RewardInconsistent.into()); } // Update the reward on project. @@ -337,10 +344,9 @@ pub mod pallet { /// Release the collateral held by the account. Should only be called in the context of acceptance. /// Does no checks. Assumes the state is as required. /// - /// **Requires** : check_collateral - pub fn release_collateral(who: &T::AccountId) -> DispatchResult { + /// **Requires** : check_collateral. Calls currency::unreserve + pub fn release_collateral(who: &T::AccountId) { T::Currency::unreserve(&who, T::UserCollateral::get()); - Ok(()) } /// Reward the user for their contribution to the project. Assumed to be called after acceptance. /// @@ -351,6 +357,7 @@ pub mod pallet { review: &ReviewAl, ) -> DispatchResult { let reward = project.reward.clone(); + let mut user = T::UsersOutlet::get_user_by_id(&who).ok_or(Error::::NoneValue)?; // Reward calc // reward is reward * (user_point/ttl_project_point )-- use fixed point attr of BalanceOf and move vars around in eqn. @@ -358,17 +365,23 @@ pub mod pallet { let balance_rev_sshot = BalanceOf::::from(review.point_snapshot); let balance_div = reward .checked_div(&balance_prj_score) - .ok_or(DispatchError::Arithmetic(ArithmeticError::DivisionByZero))?; + .ok_or({ + ensure!(balance_prj_score != BalanceOf::::from(0u32),DispatchError::Arithmetic(ArithmeticError::DivisionByZero)); + ensure!(reward > balance_prj_score, DispatchError::Arithmetic(ArithmeticError::Underflow)); + DispatchError::Arithmetic(ArithmeticError::Overflow) + })?; let reward_fraction = balance_div.saturating_mul(balance_rev_sshot); // Unreserve our final decision from project. // We expect projects to not edit this reserve. What if they do?? - Users tx start failing: Ask users to Report! if found, and track txs - // Mutations - Pallet::::release_collateral(who)?; - Pallet::::reward(project, reward_fraction).expect("should be able to reward"); // nothing should fail after release - T::Currency::transfer(&project.owner_id, who, reward_fraction, KeepAlive) - .expect("should be enough to safely transfer"); + // Mutations - Fallible. Expect: All of these to rollback changes if they fail. + user.rank_points =user.rank_points.saturating_add(1); + Pallet::::reward(project, reward_fraction)?; + T::Currency::transfer(&project.owner_id, who, reward_fraction, KeepAlive)?; + T::UsersOutlet::update_user(&who,user)?; + // Mutations - Infallible + Pallet::::release_collateral(who); Ok(()) } /// Check if a **user** can serve up the required collateral @@ -413,76 +426,38 @@ pub mod pallet { metadata: Vec, status: Status, reason: Reason, - count: ProjectID, ) -> ProjectAl { - let mut project = ProjectAl::::new(who.clone(), metadata); - let mut user = T::UsersOutlet::get_user_by_id(&who).unwrap_or_default(); // FALLIBLE MUTATIONS - Pallet::::reserve_reward(&mut project) - .expect("The project owner should have sufficient balance"); - user.project_id = Some(count); + let t = Origin::::Signed(who.clone()); + assert_ok!(Pallet::::create_project(t.into(), metadata.clone())); + let next_index = >::get().unwrap_or_default(); + let index = next_index.saturating_sub(1); + // STORAGE MUTATIONS + let mut project = >::get(index).unwrap(); project.proposal_status.status = status; project.proposal_status.reason = reason; - // STORAGE MUTATIONS - >::insert(count, project.clone()); - T::UsersOutlet::update_user(&who, user).expect("User should exist"); + >::insert(index, project.clone()); project } /// Create a set of reviews from a set of ids as needed and places them in storage - pub fn initialize_reviews( - acnt_ids: Vec, - project: &mut ProjectAl, - count: ProjectID, - ) -> Vec> { + pub fn initialize_reviews(acnt_ids: Vec) { + let proj = >::get().unwrap_or_default(); + let project_id = proj.saturating_sub(1); let acnt_ids_iter = acnt_ids.iter(); - let mut local_pt = count; - // 15 is our target "gp". Pseudo random. This seed seems good enough. - let mut spread_points = || { - local_pt = local_pt.saturating_add(local_pt.saturating_add(7)); - local_pt = local_pt.saturating_mul(17) % 15u32; - if local_pt == 0 { - local_pt = local_pt.saturating_add(7); - } - local_pt - }; // intialize review contents with their ids - let list_of_revs: Vec> = constants::project::REVS - .iter() - .zip(acnt_ids_iter) - .map(|(rev, id)| { - let reserve = Pallet::::can_collateralise(id).expect( - "The user should have the required balance, enough to avoid reaping too", - ); - let _ = Pallet::::collateralise(id, reserve); - // force collateralise each so we can immediately apply accept i.e update stake on project and supply reward. - let mut user = T::UsersOutlet::get_user_by_id(id).unwrap_or_default(); - - user.rank_points = spread_points(); - // init rev - let mut review: ReviewAl = Default::default(); - review.project_id = count; - review.proposal_status.status = Status::Accepted; - review.content = rev.to_vec(); - review.user_id = id.clone(); - review.point_snapshot = user.rank_points; - project.total_user_scores = - project.total_user_scores.saturating_add(user.rank_points); - - T::UsersOutlet::update_user(id, user).expect("User should exist"); - >::insert(id.clone(), count, review.clone()); - - review - }) - .collect(); - - // storage mutations - >::mutate(count, |p| *p = Some(project.clone())); - for elem in list_of_revs.iter() { - let _ = Pallet::::reward_user(&elem.user_id, project, &elem) - .expect("The collateral and all exists"); - >::mutate(count, |p| *p = Some(project.clone())); + for (rev, id) in constants::project::REVS.iter().zip(acnt_ids_iter.clone()) { + let dispatch = Pallet::::create_review( + Origin::::Signed(id.clone()).into(), + (rev.0, rev.1.to_vec()), + project_id, + ); + assert_ok!(dispatch); + } + // Accept the reviews. + for (_, id) in constants::project::REVS.iter().zip(acnt_ids_iter){ + let dispatch2 =Pallet::::accept_review(Origin::::Root.into(), id.clone(), project_id); + assert_ok!(dispatch2); } - list_of_revs } } /// Genesis config for the chocolate pallet @@ -508,12 +483,15 @@ pub mod pallet { let iter_users = (&self.init_users).iter(); for id in iter_users.clone() { T::UsersOutlet::set_user(id, Default::default()); - }; + } // setup a counter to serve as project index - let mut count: ProjectID = 1; let meta: Vec> = constants::project::METADATA.iter().map(|each| each.to_vec()).collect(); - let init_projects_w_users: Vec<_> = (&self.init_projects).into_iter().zip(iter_users).map(|((s,r),accnt)|(accnt,s.clone(),r.clone())).collect(); + let init_projects_w_users: Vec<_> = (&self.init_projects) + .into_iter() + .zip(iter_users) + .map(|((s, r), accnt)| (accnt, s.clone(), r.clone())) + .collect(); let zipped = (init_projects_w_users).into_iter().zip(meta.iter()); // create project from associated metadata in zip. for each in zipped { @@ -521,7 +499,8 @@ pub mod pallet { let meta_cid = meta_ref.to_owned(); let (acnt, stat, reas) = project_ref.to_owned(); // Filter ids so generated reviews do not include project owner - let filtered_ids: Vec<_> = (&self.init_users).iter() + let filtered_ids: Vec<_> = (&self.init_users) + .iter() .filter(|id| acnt.ne(id)) .map(|long| long.clone()) .collect(); @@ -541,13 +520,9 @@ pub mod pallet { .for_each(|id| T::Currency::resolve_creating(id, T::Currency::issue(total))); // create reviews and projects and store. - let mut returnable = - Pallet::::initialize_project(acnt.clone(), meta_cid, stat, reas, count); - let _reviews: Vec<_> = - Pallet::::initialize_reviews(filtered_ids, &mut returnable, count); - // STORAGE MUTATIONS -- after due to mut - count += 1; - >::put(count); + + Pallet::::initialize_project(acnt.clone(), meta_cid, stat, reas); + Pallet::::initialize_reviews(filtered_ids); } // Fill the treasury - A little hack. let imbalance = T::Currency::issue(T::RewardCap::get()); diff --git a/pallets/chocolate/src/mock.rs b/pallets/chocolate/src/mock.rs index d8ce5f8..785575e 100644 --- a/pallets/chocolate/src/mock.rs +++ b/pallets/chocolate/src/mock.rs @@ -13,7 +13,7 @@ type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; use chocolate_projects::{Reason, Status}; -// The runtime is an enum. omoshiroi +// The runtime is an enum. // Configure a mock runtime to test the pallet. frame_support::construct_runtime!( pub enum Test where @@ -85,7 +85,7 @@ impl pallet_users::Config for Test { type Event = Event; } parameter_types! { - pub const Cap: u128 = 5; + pub const Cap: u128 = 100; pub const UserCollateral: u128 = 10; } // our configs start here diff --git a/pallets/chocolate/src/tests.rs b/pallets/chocolate/src/tests.rs index 02b815a..94cf814 100644 --- a/pallets/chocolate/src/tests.rs +++ b/pallets/chocolate/src/tests.rs @@ -20,14 +20,16 @@ fn create_project_should_fail() { fn create_review_should_work() { choc_ext().execute_with(|| { // Dispatch a signed extrinsic. - assert_ok!(ChocolateModule::create_review(Origin::signed(6), [42_u8].to_vec(), 1_u32)); + assert_ok!(ChocolateModule::create_review(Origin::signed(6), (3,[42_u8].to_vec()), 1_u32)); }); } #[test] fn create_review_should_fail() { choc_ext().execute_with(|| { // Based on current genesis config. - assert_err!(ChocolateModule::create_review(Origin::signed(1), [40_u8].to_vec(), 1_u32),Error::::OwnerReviewedProject); - assert_err!(ChocolateModule::create_review(Origin::signed(2), [40_u8].to_vec(), 1_u32),Error::::DuplicateReview); + assert_err!(ChocolateModule::create_review(Origin::signed(1), (3,[40_u8].to_vec()), 1_u32),Error::::OwnerReviewedProject); + assert_err!(ChocolateModule::create_review(Origin::signed(2), (3,[40_u8].to_vec()), 1_u32),Error::::DuplicateReview); + assert_err!(ChocolateModule::create_review(Origin::signed(6), (60,[40_u8].to_vec()), 1_u32),Error::::ReviewScoreOutOfRange); + }); } diff --git a/pallets/users/src/lib.rs b/pallets/users/src/lib.rs index b8bcaed..0827fa4 100644 --- a/pallets/users/src/lib.rs +++ b/pallets/users/src/lib.rs @@ -78,7 +78,6 @@ pub mod pallet { let user = self::Users::::get(id).unwrap_or_default(); user.project_id.is_some() } - /// Allows us to check if the user even exists before calling get by id. fn check_user_exists(id: &T::AccountId) -> bool { self::Users::::contains_key(id) } @@ -90,7 +89,6 @@ pub mod pallet { } user } - /// Idempotent. Simply inserts in storage. fn set_user(id: &T::AccountId, user: User) -> () { if Self::check_user_exists(id) { return (); diff --git a/primitives/chocolate-projects/Cargo.toml b/primitives/chocolate-projects/Cargo.toml index 115ae7c..0d02c7c 100644 --- a/primitives/chocolate-projects/Cargo.toml +++ b/primitives/chocolate-projects/Cargo.toml @@ -7,9 +7,6 @@ description = "Primitives crate for chocolate projects" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [package.metadata.docs.rs] targets = ['x86_64-unknown-linux-gnu'] -# ----------------_ dup -[dev-dependencies.serde] -version = '1.0.126' [dev-dependencies.sp-core] default-features = false @@ -73,16 +70,13 @@ default-features = false git = 'https://github.com/paritytech/substrate.git' tag = 'monthly-2021-08' version = '4.0.0-dev' -# added pallets - duplicate with dev serde +# added dep [dependencies.serde] version = "1.0.126" optional = true features = ['derive'] -# # added dep - [features] default = ['std'] -# or here? runtime-benchmarks = ['frame-benchmarking'] std = [ 'serde', diff --git a/primitives/chocolate-projects/src/lib.rs b/primitives/chocolate-projects/src/lib.rs index 7b30e44..087e511 100644 --- a/primitives/chocolate-projects/src/lib.rs +++ b/primitives/chocolate-projects/src/lib.rs @@ -18,6 +18,8 @@ pub struct Review { pub project_id: ProjectID, /// A snapshot of the user's rank at the time of review pub point_snapshot: u32, + /// Score of a review + pub review_score: u8 } /// The metadata of a project. @@ -87,8 +89,12 @@ pub struct Project { pub reward: Balance, /// A sum of all the scores of reviews proposed to the project. Saturate when u32::MAX. pub total_user_scores: u32, + /// The total review scores for a project + pub total_review_score: u64, + /// The number of reviews submitted + pub number_of_reviews: u32 } -// ------------------------------------------------------------^edit + impl + Default> Project { /// Set useful defaults. /// Initialises a project with defaults on everything except id and metadata diff --git a/primitives/chocolate-users/src/lib.rs b/primitives/chocolate-users/src/lib.rs index 0b8ad7d..fa1700f 100644 --- a/primitives/chocolate-users/src/lib.rs +++ b/primitives/chocolate-users/src/lib.rs @@ -4,19 +4,26 @@ use codec::{Decode, Encode}; use frame_support::dispatch::{ DispatchResult}; use frame_system::Config; -#[derive(Encode, Decode, Default, Clone)] +#[derive(Encode, Decode, Clone)] pub struct User { pub rank_points: u32, pub project_id: Option, } - +impl Default for User{ + fn default() -> Self{ + // Start from 1 because of total project score calc to avoid accidentally recording zero when we use Default::default() + User { rank_points: 1, project_id: Option::None } + } +} /// UserIO trait for CRUD on users store pub trait UserIO { fn get_user_by_id(id: &T::AccountId) -> Option; fn check_owns_project(id: &T::AccountId) -> bool; + /// Allows us to check if the user even exists before calling get by id. fn check_user_exists(id: &T::AccountId) -> bool; /// Checks if the user exists, else creates a new user with wanted defaults. fn get_or_create_default(id: &T::AccountId) -> User; + /// Idempotent. Simply creates item in storage if it doesn't already exist. Use update_user if you'd like to mutate the user after knowing it's been created fn set_user(id: &T::AccountId, user: User) -> (); fn update_user(id: &T::AccountId, user: User) -> DispatchResult; } diff --git a/types.json b/types.json index c95c175..323ff1b 100644 --- a/types.json +++ b/types.json @@ -16,14 +16,17 @@ "metadata": "MetaData", "proposalStatus": "ProposalStatus", "reward": "Balance", - "totalUserScores": "u32" + "totalUserScores": "u32", + "totalReviewScore": " u64", + "numberOfReviews": "u32" }, "Review": { "proposalStatus": "ProposalStatus", "userID": "AccountId", "content": "Text", "projectID": "ProjectID", - "pointSnapshot": "u32" + "pointSnapshot": "u32", + "reviewScore": "u8" }, "ProposalStatus": { From f027af2f155e956a0ed0cb87ae75d5e01caf6068 Mon Sep 17 00:00:00 2001 From: Islam Date: Sun, 1 May 2022 20:30:49 +0000 Subject: [PATCH 2/2] sync types.json --- types.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/types.json b/types.json index 323ff1b..ae3db0a 100644 --- a/types.json +++ b/types.json @@ -7,8 +7,8 @@ "ReviewAl": "Review", "ProjectAl": "Project", "User": { - "rank_points": "u32", - "project_id": "Option" + "rankPoints": "u32", + "projectId": "Option" }, "Project": { "ownerID": "AccountId",