Vca reward #84
Vca reward #84
Conversation
src/rewards/mod.rs
Outdated
| use rust_decimal::Decimal; | ||
| pub type Funds = Decimal; | ||
| // Lets match to the same type as the funds, but naming it funds would be confusing | ||
| pub type Rewards = Funds; |
There was a problem hiding this comment.
Maybe better in this case to alias to the Decimal type explicitly
src/rewards/veterans.rs
Outdated
| .into_iter() | ||
| .map(|(review, rankings)| (review, calc_final_ranking_per_review(&rankings))) | ||
| .collect::<BTreeMap<_, _>>(); | ||
| dbg!(&final_rankings_per_review); |
There was a problem hiding this comment.
| dbg!(&final_rankings_per_review); |
src/rewards/veterans.rs
Outdated
| }) | ||
| .counts_by(|ranking| ranking.vca.clone()); | ||
|
|
||
| dbg!(&rankings_per_vca); |
There was a problem hiding this comment.
| dbg!(&rankings_per_vca); |
src/rewards/veterans.rs
Outdated
| .counts_by(|ranking| ranking.vca.clone()); | ||
|
|
||
| dbg!(&rankings_per_vca); | ||
| dbg!(&eligible_rankings_per_vca); |
There was a problem hiding this comment.
| dbg!(&eligible_rankings_per_vca); |
| use crate::utils::serde::deserialize_truthy_falsy; | ||
| use serde::Deserialize; | ||
|
|
||
| pub type AdvisorReviewId = (String, String); |
There was a problem hiding this comment.
Maybe document this alias? It's not really obvious what these 2 strings are.
| pub reputation: u64, | ||
| } | ||
|
|
||
| pub type EligibilityThresholds = std::ops::RangeInclusive<usize>; |
There was a problem hiding this comment.
nit: EligibilityRange? :)
There was a problem hiding this comment.
yeah, it's a bit counterintuitive the way it's written, but it was to keep naming close to the requirements
src/rewards/veterans.rs
Outdated
| } | ||
|
|
||
| fn calc_final_ranking_per_review(rankings: &[impl Borrow<VeteranRankingRow>]) -> ReviewRanking { | ||
| let rankings_majority = rankings.len() / 2; |
There was a problem hiding this comment.
What is the type of rankings_majority? Is it going to lead to any possible mistakes? I'm always a bit wary of divisions. Maybe employ decimals here as well?
|
I had to refactor |
eugene-babichenko
left a comment
eugene-babichenko
left a comment
There was a problem hiding this comment.
The code in general is LGTM, let's see how it survives real-life testing.
No description provided.