address comments

src/bin/cli/rewards/veterans.rs

@@ -1,7 +1,9 @@
 use super::Error;
-use catalyst_toolbox::rewards::veterans;
+use catalyst_toolbox::community_advisors::models::VeteranRankingRow;
+use catalyst_toolbox::rewards::veterans::{self, VcaRewards, VeteranAdvisorIncentive};
 use catalyst_toolbox::rewards::Rewards;
 use catalyst_toolbox::utils::csv;
+use serde::Serialize;
 use std::path::PathBuf;
 use structopt::StructOpt;
 
@@ -42,15 +44,42 @@ impl VeteransRewards {
             max_rankings_reputation,
             max_rankings_rewards,
         } = self;
-        let reviews: Vec<veterans::VeteranRankingRow> = csv::load_data_from_csv::<_, b','>(&from)?;
+        let reviews: Vec<VeteranRankingRow> = csv::load_data_from_csv::<_, b','>(&from)?;
         let results = veterans::calculate_veteran_advisors_incentives(
             &reviews,
             total_rewards,
             min_rankings..=max_rankings_rewards,
             min_rankings..=max_rankings_reputation,
         );
-        csv::dump_data_to_csv(&results.into_iter().collect::<Vec<_>>(), &to)?;
+
+        csv::dump_data_to_csv(&rewards_to_csv_data(results), &to).unwrap();
 
         Ok(())
     }
 }
+
+fn rewards_to_csv_data(rewards: VcaRewards) -> Vec<impl Serialize> {
+    #[derive(Serialize)]
+    struct Entry {
+        id: String,
+        rewards: Rewards,
+        reputation: u64,
+    }
+
+    rewards
+        .into_iter()
+        .map(
+            |(
+                id,
+                VeteranAdvisorIncentive {
+                    rewards,
+                    reputation,
+                },
+            )| Entry {
+                id,
+                rewards,
+                reputation,
+            },
+        )
+        .collect()
+}

src/community_advisors/models/de.rs

@@ -1,7 +1,9 @@
 use crate::utils::serde::deserialize_truthy_falsy;
 use serde::Deserialize;
 
+/// (Proposal Id, Assessor Id), an assessor cannot assess the same proposal more than once
 pub type AdvisorReviewId = (String, String);
+pub type VeteranAdvisorId = String;
 
 #[derive(Deserialize)]
 pub struct AdvisorReviewRow {
@@ -34,6 +36,24 @@ pub struct AdvisorReviewRow {
     filtered_out: bool,
 }
 
+#[derive(Deserialize)]
+pub struct VeteranRankingRow {
+    pub proposal_id: String,
+    #[serde(alias = "Assessor")]
+    pub assessor: String,
+    #[serde(alias = "Excellent", deserialize_with = "deserialize_truthy_falsy")]
+    excellent: bool,
+    #[serde(alias = "Good", deserialize_with = "deserialize_truthy_falsy")]
+    good: bool,
+    #[serde(
+        default,
+        alias = "Filtered Out",
+        deserialize_with = "deserialize_truthy_falsy"
+    )]
+    filtered_out: bool,
+    pub vca: VeteranAdvisorId,
+}
+
 #[derive(Hash, Clone, PartialEq, Eq, Debug)]
 pub enum ReviewRanking {
     Excellent,
@@ -48,31 +68,40 @@ impl ReviewRanking {
     }
 }
 
-impl AdvisorReviewRow {
+impl VeteranRankingRow {
     pub fn score(&self) -> ReviewRanking {
-        match (self.excellent, self.good, self.filtered_out) {
-            (true, false, false) => ReviewRanking::Excellent,
-            (false, true, false) => ReviewRanking::Good,
-            (false, false, true) => ReviewRanking::FilteredOut,
-            (false, false, false) => ReviewRanking::NA,
-            _ => {
-                // This should never happen, from the source of information a review could be either
-                // Excellent or Good or not assessed. It cannot be both and it is considered
-                // a malformed information input.
-                panic!(
-                    "Invalid combination of scores from assessor {} for proposal {}",
-                    self.assessor, self.proposal_id
-                )
-            }
-        }
+        ranking_from_bools(self.excellent, self.good, self.filtered_out)
     }
 
-    /// Returns a unique identifier of this review
-    pub fn id(&self) -> AdvisorReviewId {
+    pub fn review_id(&self) -> AdvisorReviewId {
         (self.proposal_id.clone(), self.assessor.clone())
     }
 }
 
+impl AdvisorReviewRow {
+    pub fn score(&self) -> ReviewRanking {
+        ranking_from_bools(self.excellent, self.good, self.filtered_out)
+    }
+}
+
+fn ranking_from_bools(excellent: bool, good: bool, filtered_out: bool) -> ReviewRanking {
+    match (excellent, good, filtered_out) {
+        (true, false, false) => ReviewRanking::Excellent,
+        (false, true, false) => ReviewRanking::Good,
+        (false, false, true) => ReviewRanking::FilteredOut,
+        (false, false, false) => ReviewRanking::NA,
+        _ => {
+            // This should never happen, from the source of information a review could be either
+            // Excellent or Good or not assessed. It cannot be both and it is considered
+            // a malformed information input.
+            panic!(
+                "Invalid combination of scores {} {} {}",
+                excellent, good, filtered_out
+            )
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::ReviewRanking;

src/community_advisors/models/mod.rs

@@ -2,7 +2,9 @@ mod de;
 
 use serde::{Deserialize, Deserializer};
 
-pub use de::{AdvisorReviewId, AdvisorReviewRow, ReviewRanking};
+pub use de::{
+    AdvisorReviewId, AdvisorReviewRow, ReviewRanking, VeteranAdvisorId, VeteranRankingRow,
+};
 
 pub enum ProposalStatus {
     Approved,

src/rewards/veterans.rs

@@ -1,54 +1,41 @@
 use crate::community_advisors::models::{
-    AdvisorReviewId, AdvisorReviewRow,
     ReviewRanking::{self, *},
+    VeteranAdvisorId, VeteranRankingRow,
 };
 use crate::rewards::Rewards;
 use itertools::Itertools;
 use rust_decimal::{prelude::ToPrimitive, Decimal};
 use std::borrow::Borrow;
 use std::collections::{BTreeMap, HashMap};
 
-use serde::{Deserialize, Serialize};
-
-pub type VeteranAdvisorId = String;
+use serde::Serialize;
 
 #[derive(Serialize)]
 pub struct VeteranAdvisorIncentive {
     pub rewards: Rewards,
     pub reputation: u64,
 }
 
+pub type VcaRewards = HashMap<VeteranAdvisorId, VeteranAdvisorIncentive>;
 pub type EligibilityThresholds = std::ops::RangeInclusive<usize>;
 
 // TODO: for the sake of clarity, introduce a different naming between ca reviews and vca ranking
 
 // Supposing to have a file with all the rankings for each review
 // e.g. something like an expanded version of a AdvisorReviewRow
 // [proposal_id, advisor, ratings, ..(other fields from AdvisorReviewRow).., ranking (good/excellent/filtered out), vca]
-#[derive(Deserialize)]
-pub struct VeteranRankingRow {
-    #[serde(flatten)]
-    advisor_review_row: AdvisorReviewRow,
-    vca: VeteranAdvisorId,
-}
-
-impl VeteranRankingRow {
-    fn score(&self) -> ReviewRanking {
-        self.advisor_review_row.score()
-    }
-
-    fn review_id(&self) -> AdvisorReviewId {
-        self.advisor_review_row.id()
-    }
-}
 
 fn calc_final_ranking_per_review(rankings: &[impl Borrow<VeteranRankingRow>]) -> ReviewRanking {
-    let rankings_majority = rankings.len() / 2;
+    let rankings_majority = Decimal::from(rankings.len()) / Decimal::from(2);
     let ranks = rankings.iter().counts_by(|r| r.borrow().score());
 
     match (ranks.get(&FilteredOut), ranks.get(&Excellent)) {
-        (Some(filtered_out), _) if filtered_out > &rankings_majority => ReviewRanking::FilteredOut,
-        (_, Some(excellent)) if excellent > &rankings_majority => ReviewRanking::Excellent,
+        (Some(filtered_out), _) if Decimal::from(*filtered_out) >= rankings_majority => {
+            ReviewRanking::FilteredOut
+        }
+        (_, Some(excellent)) if Decimal::from(*excellent) > rankings_majority => {
+            ReviewRanking::Excellent
+        }
         _ => ReviewRanking::Good,
     }
 }
@@ -69,7 +56,6 @@ fn rewards_disagreement_discount(agreement_rate: Decimal) -> Decimal {
 }
 
 fn reputation_disagreement_discount(agreement_rate: Decimal) -> Decimal {
-    println!("discount rate {}", agreement_rate);
     if agreement_rate >= Decimal::new(6, 1) {
         Decimal::ONE
     } else {