37 improve reputation #116
Conversation
|
Can you merge |
There was a problem hiding this comment.
- Resolve the merge conflicts with universal-develop.
- Solve the Clippy warnings.
- In the current implementation, ratings are too generically implemented, while we need them only for our Profile. This is overengineering for the problem we are trying to solve. On the good side, you have created a generic solution that others can reuse. However, I would still like
Reputationto be tied to a Profile. - TIght coupling is a mess when you need to update the version. Consider loosely coupling.
- Implement some tests in Task pallet that verify that Profile reputation is updated when task is accepted with a certain rating.
| type Event = Event; | ||
| type ReputationHandler = pallet_reputation::impls::ReputationHandler; | ||
| type DefaultReputation = DefaultReputation; | ||
| type MaximumRatingsPer = MaximumRatingsPer; |
| @@ -101,14 +106,13 @@ pub mod pallet { | |||
| pub name: BoundedVec<u8, T::MaxUsernameLen>, | |||
| pub interests: BoundedVec<u8, T::MaxInterestsLen>, | |||
| pub balance: Option<BalanceOf<T>>, | |||
| pub reputation: u32, | |||
There was a problem hiding this comment.
We have to keep the reputation as part of the profile struct.
|
|
||
| #[pallet::storage] | ||
| #[pallet::getter(fn reputation_of)] | ||
| pub type RepInfoOf<T: Config> = StorageMap<_, Twox64Concat, T::AccountId, Reputable<T>, OptionQuery>; |
There was a problem hiding this comment.
Based on my initial design, reputation was tied to profile, now it is tied to AccountId.
| pallet_profile::Pallet::<T>::add_reputation(&task.initiator)?; | ||
| pallet_profile::Pallet::<T>::add_reputation(&task.volunteer)?; | ||
| if *task_status == TaskStatus::Accepted { | ||
| // TODO:pallet_profile::Pallet::<T>::add_reputation(&task.initiator)?; |
| } | ||
|
|
||
| /// Remove a reputation record from storage. | ||
| pub fn remove_reputation_record(account: T::AccountId) -> DispatchResult { |
There was a problem hiding this comment.
We should not allow removing of reputation. Not even by a sudo account.
|
|
||
| parameter_types! { | ||
| #[derive(TypeInfo, MaxEncodedLen, Encode)] | ||
| pub MaximumRatingsPer: u32 = 5; |
There was a problem hiding this comment.
I see MaximumRatingsPer is the maximum rate per rating. Maybe call it MaxRatePerRating.
| @@ -183,7 +186,7 @@ pub mod pallet { | |||
|
|
|||
| /// Configure the pallet by specifying the parameters and types on which it depends. | |||
| #[pallet::config] | |||
| pub trait Config: frame_system::Config + pallet_profile::Config { | |||
| pub trait Config: frame_system::Config + pallet_reputation::Config + pallet_profile::Config { | |||
There was a problem hiding this comment.
The tight coupling will be a problem.
|
|
||
| // Assert logic follows as described in: https://github.com/UniversalDot/universal-dot-node/issues/37 | ||
| assert_ok!(Reputation::rate_account(&0, &bounded_vec![1u8, 1u8])); | ||
| let rep_record = RepInfoOf::<Test>::get(0).unwrap(); |
There was a problem hiding this comment.
Based on the design, 1 rating gets -2, and in this one gets a -4.
There was a problem hiding this comment.
Change the implementation, or change the design. :)
| pub type Rating = u8; | ||
| use crate::traits::ReputationHandler; | ||
|
|
||
| pub const MAX_CREDIBILITY: CredibilityUnit = 1000; |
There was a problem hiding this comment.
How is Max Credibility decided?
Reputation pallet with basic implementation,