[ABO-237] Affiliate Pallet #392
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #392 +/- ##
===========================================
- Coverage 77.27% 77.12% -0.15%
===========================================
Files 36 37 +1
Lines 4189 4267 +78
===========================================
+ Hits 3237 3291 +54
- Misses 952 976 +24 ☔ View full report in Codecov by Sentry. |
pallets/ajuna-affiliate/src/lib.rs
Outdated
| /// Stores the affiliated accounts from the perspectives of the affiliatee | ||
| #[pallet::storage] | ||
| #[pallet::getter(fn affiliatees)] | ||
| pub type Affiliatees<T: Config> = |
There was a problem hiding this comment.
I think we should use more common names.
LinkedAccounts
There was a problem hiding this comment.
I like that Affiliatees implies a direction, while LinkedAccounts does not. However, I do agree that the names are quite confusing, and even if Affiliatee has a notion of direction, it is undefined by the terminology who earns the rewards. If possible, we should find a term that defines both or add this extra part in the documentation.
Haha, this reminds me again that there are only two hard software problems: 1. cache invalidation, 2. variable naming.
There was a problem hiding this comment.
How about we name them: Downline and Upline accounts? I think this solves both of our issues:
- It is directional
- It uses the common affiliate marketing terms
So:
- Affiliatees -> DownlineAccounts
- Affiliators: can stay the same; these are accounts, which have downline accounts
- Affiliatables: As @darkfriend77 suggests, I think it is unneded too.
Then do some renaming based on this suggestion.
| /// There is no account set as the organizer | ||
| OrganizerNotSet, | ||
| /// An account cannot affiliate itself | ||
| CannotAffiliateSelf, |
There was a problem hiding this comment.
Good catch, would replace in general Affiliate with Link
There was a problem hiding this comment.
Nice, good job keeping the code fairly simple! I really think we have mostly on issue to solve: naming. Even though the naming is correct, it was really hard for me to keep track of which Aff* does what. I think I have added a good terminology suggestion in one of the comments, which we can refine.
| @@ -118,7 +118,7 @@ impl<T: Config> AvatarCombinator<T> { | |||
| }) | |||
| .transpose()?; | |||
|
|
|||
| Ok((leader_output, other_output.into_iter().chain(additional_output.into_iter()).collect())) | |||
| Ok((leader_output, other_output.into_iter().chain(additional_output).collect())) | |||
There was a problem hiding this comment.
Another thing, I am quite picky with: feature PRs should not implement other unrelated fixes. Especially for me who is new to the code base, this makes my life easier. :)
pallets/ajuna-affiliate/src/lib.rs
Outdated
| /// Stores the affiliated accounts from the perspectives of the affiliatee | ||
| #[pallet::storage] | ||
| #[pallet::getter(fn affiliatees)] | ||
| pub type Affiliatees<T: Config> = |
There was a problem hiding this comment.
How about we name them: Downline and Upline accounts? I think this solves both of our issues:
- It is directional
- It uses the common affiliate marketing terms
So:
- Affiliatees -> DownlineAccounts
- Affiliators: can stay the same; these are accounts, which have downline accounts
- Affiliatables: As @darkfriend77 suggests, I think it is unneded too.
Then do some renaming based on this suggestion.
| type RuleIdentifier: Parameter + MaxEncodedLen; | ||
|
|
||
| /// The rule type at runtime. | ||
| type RuntimeRule: Parameter + MaxEncodedLen; |
There was a problem hiding this comment.
Yes, exactly, this is how I meant it. I have been thinking some more about a trait bound we could introduce here, like ExecuteRule. However, as this pallet has no predefined hooks, meaning the pallet that uses the affiliate pallet can decide when to use this it won't be needed.
However, then that pallet needs to invoke the execution of the rule itself, then the question is, why do we even store the rule here? It is not needed, the invoking pallet already knows, which rule it wants to execute. Then this pallet, would actually only need to handle affiliate relations and no rules at all, and the other pallet simply wants to check if an account is an affiliator or not.
Alternatively, if we really and this to be generic, then maybe the RuleId does not correspond to the extrinsic enum variant. This would allow some runtime to even have two pallets, one that wants to add affiliate relations, and another one that would want to execute the rule. In this case, we'd need to store the rule here, but we'd need to have a trait bound so that the other pallet could execute the rule, if it finds one.
This is probably one more fundamental thing we have to discuss in the brainstorming session tomorrow.
There was a problem hiding this comment.
Yeah, I get what you mean, it really is something that we should discuss in the meeting
Description
See here for details.
Type of changes
build: Changes that affect the build system or external dependencies (eg, Cargo, Docker)ci: Changes to CI configurationdocs: Changes to documentation onlyfeat: Changes to add a new featurefix: Changes to fix a bugrefactor: Changes that do not alter functionalitystyle: Changes to format the codetest: Changes to add missing tests or correct existing testsChecklist
cargo fmt --allcargo clippy --all-features --all-targetscargo test --workspace --all-features --all-targets