Merge pull request #3728 from anoma/brent/improve-gov-output

Improve logging for validator voting period

.changelog/unreleased/improvements/3728-improve-gov-output.md

@@ -0,0 +1,3 @@
+- Improve the proposal result query to be more descriptive and detail
+  the validator voting period. Fix some other small logging bugs.
+  ([\#3728](https://github.com/anoma/namada/pull/3728))

crates/apps_lib/src/client/rpc.rs

@@ -450,33 +450,88 @@ pub async fn query_proposal_result<N: Namada>(
         namada_sdk::rpc::query_proposal_by_id(context.client(), proposal_id)
             .await;
 
-    if let (Ok(Some(proposal_result)), Ok(Some(proposal_query))) =
-        (proposal_result, proposal_query)
-    {
-        display_line!(context.io(), "Proposal Id: {} ", proposal_id);
-        if current_epoch >= proposal_query.voting_end_epoch {
-            display_line!(context.io(), "{:4}{}", "", proposal_result);
-        } else {
+    match (proposal_query, proposal_result) {
+        // The proposal is found in storage and voting has at least begun (a
+        // result is being tallied or is completely tallied)
+        (Ok(Some(proposal_query)), Ok(Some(proposal_result))) => {
+            display_line!(context.io(), "Proposal Id: {} ", proposal_id);
+            if current_epoch >= proposal_query.voting_end_epoch {
+                display_line!(
+                    context.io(),
+                    "{:4}The voting period has ended.",
+                    ""
+                );
+                display_line!(context.io(), "{:4}{}", "", proposal_result);
+            } else {
+                display_line!(
+                    context.io(),
+                    "{:4}The voting period is underway and will continue \
+                     until epoch {} begins.",
+                    "",
+                    proposal_query.voting_end_epoch,
+                );
+                if let Ok(Some(last_epoch)) =
+                    namada_sdk::governance::utils::last_validator_voting_epoch(
+                        proposal_query.voting_start_epoch,
+                        proposal_query.voting_end_epoch,
+                    )
+                {
+                    display_line!(
+                        context.io(),
+                        "{:4}NOTE: Validators can vote only until the end of \
+                         epoch {}.",
+                        "",
+                        last_epoch
+                    )
+                }
+                let res = format!("{}", proposal_result);
+                if let Some(idx) = res.find(' ') {
+                    let slice = &res[idx..];
+                    display_line!(context.io(), "{:4}Currently{}", "", slice);
+                } else {
+                    display_line!(
+                        context.io(),
+                        "{:4}Error parsing the result string",
+                        "",
+                    );
+                }
+            }
+        }
+        // The proposal is found in storage but no result is found (voting has
+        // not begun)
+        (Ok(Some(proposal_query)), Ok(None)) => {
+            display_line!(context.io(), "Proposal Id: {} ", proposal_id);
+            display_line!(
+                context.io(),
+                "{:4}The voting period has not begun yet.",
+                ""
+            );
             display_line!(
                 context.io(),
-                "{:4}Still voting until epoch {} begins.",
+                "{:4}Start epoch: {}. End epoch: {}",
                 "",
+                proposal_query.voting_start_epoch,
                 proposal_query.voting_end_epoch
             );
-            let res = format!("{}", proposal_result);
-            if let Some(idx) = res.find(' ') {
-                let slice = &res[idx..];
-                display_line!(context.io(), "{:4}Currently{}", "", slice);
-            } else {
+            if let Ok(Some(last_epoch)) =
+                namada_sdk::governance::utils::last_validator_voting_epoch(
+                    proposal_query.voting_start_epoch,
+                    proposal_query.voting_end_epoch,
+                )
+            {
                 display_line!(
                     context.io(),
-                    "{:4}Error parsing the result string",
+                    "{:4}NOTE: Validators will be able to vote only until the \
+                     end of epoch {}.",
                     "",
-                );
+                    last_epoch
+                )
             }
         }
-    } else {
-        edisplay_line!(context.io(), "Proposal {} not found.", proposal_id);
+        // The proposal could not be found
+        _ => {
+            edisplay_line!(context.io(), "Proposal {} not found.", proposal_id);
+        }
     }
 }
 

crates/apps_lib/src/client/tx.rs

@@ -995,7 +995,7 @@ where
                 e.to_string(),
             )
         })?;
-        let author_balane = namada_sdk::rpc::get_token_balance(
+        let author_balance = namada_sdk::rpc::get_token_balance(
             namada.client(),
             &namada.native_token(),
             &proposal.proposal.author,
@@ -1006,7 +1006,7 @@ where
             .validate(
                 &governance_parameters,
                 current_epoch,
-                author_balane,
+                author_balance,
                 args.tx.force,
             )
             .map_err(|e| {

crates/core/src/uint.rs

@@ -289,6 +289,19 @@ impl Uint {
             }
         }
     }
+
+    /// Return the ceiling of the product of an integer and a fraction
+    /// (represented as `num / denom`)
+    pub fn frac_mul_ceil(
+        &self,
+        num: Self,
+        denom: Self,
+    ) -> Result<Self, arith::Error> {
+        let val = checked!(self * num)?;
+        let adj_den = checked!(denom - Uint::one())?;
+        let val = checked!(val + adj_den)?;
+        checked!(val / denom)
+    }
 }
 
 construct_uint! {

crates/governance/src/storage/proposal.rs

@@ -96,7 +96,13 @@ impl TryFrom<DefaultProposal> for InitProposalData {
             content: Hash::default(),
             author: value.proposal.author,
             r#type: match value.data {
-                Some(_) => ProposalType::DefaultWithWasm(Hash::default()),
+                Some(bytes) => {
+                    if bytes.is_empty() {
+                        ProposalType::Default
+                    } else {
+                        ProposalType::DefaultWithWasm(Hash::default())
+                    }
+                }
                 None => ProposalType::Default,
             },
             voting_start_epoch: value.proposal.voting_start_epoch,

crates/governance/src/utils.rs

@@ -8,6 +8,7 @@ use namada_core::chain::Epoch;
 use namada_core::collections::HashMap;
 use namada_core::dec::Dec;
 use namada_core::token;
+use namada_core::uint::Uint;
 use namada_macros::BorshDeserializer;
 #[cfg(feature = "migrations")]
 use namada_migrations::*;
@@ -111,8 +112,8 @@ pub enum TallyResult {
 impl Display for TallyResult {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            TallyResult::Passed => write!(f, "passed"),
-            TallyResult::Rejected => write!(f, "rejected"),
+            TallyResult::Passed => write!(f, "Passed"),
+            TallyResult::Rejected => write!(f, "Rejected"),
         }
     }
 }
@@ -247,7 +248,7 @@ impl Display for ProposalResult {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let threshold = match self.tally_type {
             TallyType::TwoFifths => {
-                self.total_voting_power.mul_ceil(Dec::two_thirds())
+                self.total_voting_power.mul_ceil(Dec::two_fifths())
             }
             TallyType::LessOneHalfOverOneThirdNay => Ok(token::Amount::zero()),
             _ => self.total_voting_power.mul_ceil(Dec::one_third()),
@@ -422,22 +423,29 @@ pub fn compute_proposal_result(
 }
 
 /// Calculate the valid voting window for a validator given proposal epoch
-/// details. The valid window is within 2/3 of the voting period.
-/// NOTE: technically the window can be more generous than 2/3 since the end
-/// epoch is a valid epoch for voting too.
-/// Returns `false` if any arithmetic fails.
+/// details. The valid window is approximately within 2/3 of the voting period.
+/// NOTE: technically the window can be more generous than 2/3, especially for
+/// low `voting_end_epoch - voting_start_epoch`, but it will never be less than
+/// 2/3. Returns `false` if any arithmetic fails.
 pub fn is_valid_validator_voting_period(
     current_epoch: Epoch,
     voting_start_epoch: Epoch,
     voting_end_epoch: Epoch,
 ) -> bool {
     if voting_start_epoch >= voting_end_epoch {
         false
+    } else if voting_end_epoch
+        .0
+        .checked_sub(voting_start_epoch.0)
+        .unwrap() // safe unwrap
+        <= 2
+    {
+        current_epoch == voting_start_epoch
     } else {
         (|| {
-            // From e_cur <= e_start + 2/3 * (e_end - e_start)
+            // From e_cur < e_start + 2/3 * (e_end - e_start)
             let is_within_two_thirds = checked!(
-                current_epoch * 3 <= voting_start_epoch + voting_end_epoch * 2
+                current_epoch * 3 < voting_start_epoch + voting_end_epoch * 2
             )
             .ok()?;
 
@@ -447,6 +455,37 @@ pub fn is_valid_validator_voting_period(
     }
 }
 
+/// Returns the latest epoch in which a validator can vote, given the voting
+/// start and end epochs. If the pair of start and end epoch is invalid, then
+/// return `None`.
+pub fn last_validator_voting_epoch(
+    voting_start_epoch: Epoch,
+    voting_end_epoch: Epoch,
+) -> Result<Option<Epoch>, arith::Error> {
+    if voting_start_epoch >= voting_end_epoch {
+        return Ok(None);
+    }
+
+    #[allow(clippy::arithmetic_side_effects)]
+    let diff_epochs = voting_end_epoch.0 - voting_start_epoch.0;
+
+    if diff_epochs <= 2 {
+        Ok(Some(voting_start_epoch))
+    } else {
+        let diff_epochs = Uint::from_u64(diff_epochs);
+        // ceiling multiplication by 2/3
+        let valid_period =
+            u64::try_from(diff_epochs.frac_mul_ceil(2.into(), 3.into())?).ok();
+
+        if let Some(period) = valid_period {
+            let latest = checked!(voting_start_epoch + period - 1u64)?;
+            Ok(Some(latest))
+        } else {
+            Ok(None)
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     use std::ops::{Add, Sub};
@@ -1420,6 +1459,7 @@ mod test {
 
     #[test]
     fn test_validator_voting_period() {
+        // Voting period of 2 epochs
         assert!(!is_valid_validator_voting_period(
             0.into(),
             2.into(),
@@ -1430,7 +1470,7 @@ mod test {
             2.into(),
             4.into()
         ));
-        assert!(is_valid_validator_voting_period(
+        assert!(!is_valid_validator_voting_period(
             3.into(),
             2.into(),
             4.into()
@@ -1440,13 +1480,20 @@ mod test {
             2.into(),
             4.into()
         ));
+        assert_eq!(
+            last_validator_voting_epoch(2.into(), 4.into())
+                .unwrap()
+                .unwrap(),
+            2.into()
+        );
 
+        // Voting period of 3 epochs
         assert!(is_valid_validator_voting_period(
             3.into(),
             2.into(),
             5.into()
         ));
-        assert!(is_valid_validator_voting_period(
+        assert!(!is_valid_validator_voting_period(
             4.into(),
             2.into(),
             5.into()
@@ -1456,5 +1503,27 @@ mod test {
             2.into(),
             5.into()
         ));
+        assert_eq!(
+            last_validator_voting_epoch(2.into(), 5.into())
+                .unwrap()
+                .unwrap(),
+            3.into()
+        );
+
+        for end_epoch in 1u64..=20 {
+            let last = last_validator_voting_epoch(0.into(), end_epoch.into())
+                .unwrap()
+                .unwrap();
+            assert!(is_valid_validator_voting_period(
+                last,
+                0.into(),
+                end_epoch.into()
+            ));
+            assert!(!is_valid_validator_voting_period(
+                last.next(),
+                0.into(),
+                end_epoch.into()
+            ));
+        }
     }
 }

crates/governance/src/vp/mod.rs

@@ -2187,7 +2187,7 @@ mod test {
     }
 
     #[test]
-    fn test_goverance_vote_validator_success() {
+    fn test_governance_vote_validator_success() {
         let mut state = init_storage();
 
         let proposal_id = 0;
@@ -2268,7 +2268,7 @@ mod test {
 
         let height = state.in_mem().get_block_height().0 + (7 * 2);
 
-        update_epoch_to(&mut state, 7, height);
+        update_epoch_to(&mut state, 6, height);
 
         let validator_address = established_address_1();
 
@@ -2316,7 +2316,7 @@ mod test {
     }
 
     #[test]
-    fn test_goverance_vote_validator_out_of_voting_window_fail() {
+    fn test_governance_vote_validator_out_of_voting_window_fail() {
         let mut state = init_storage();
 
         let proposal_id = 0;
@@ -2445,7 +2445,7 @@ mod test {
     }
 
     #[test]
-    fn test_goverance_vote_validator_fail() {
+    fn test_governance_vote_validator_fail() {
         let mut state = init_storage();
 
         let proposal_id = 0;
@@ -2574,7 +2574,7 @@ mod test {
     }
 
     #[test]
-    fn test_goverance_vote_delegator_success() {
+    fn test_governance_vote_delegator_success() {
         let mut state = init_storage();
 
         let proposal_id = 0;
@@ -2720,7 +2720,7 @@ mod test {
     }
 
     #[test]
-    fn test_goverance_vote_delegator_fail() {
+    fn test_governance_vote_delegator_fail() {
         let mut state = init_storage();
 
         let proposal_id = 0;
@@ -2866,7 +2866,7 @@ mod test {
     }
 
     #[test]
-    fn test_goverance_vote_invalid_verifier_fail() {
+    fn test_governance_vote_invalid_verifier_fail() {
         let mut state = init_storage();
 
         let proposal_id = 0;