diff --git a/.changelog/unreleased/improvements/3728-improve-gov-output.md b/.changelog/unreleased/improvements/3728-improve-gov-output.md new file mode 100644 index 0000000000..6463dd7f54 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/apps_lib/src/client/rpc.rs b/crates/apps_lib/src/client/rpc.rs index 6c2a1a929b..7005d2f72f 100644 --- a/crates/apps_lib/src/client/rpc.rs +++ b/crates/apps_lib/src/client/rpc.rs @@ -450,33 +450,88 @@ pub async fn query_proposal_result( 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); + } } } diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 6b39bbd4f6..3719951e35 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/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| { diff --git a/crates/core/src/uint.rs b/crates/core/src/uint.rs index 40c07eeafb..18224342a3 100644 --- a/crates/core/src/uint.rs +++ b/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 { + let val = checked!(self * num)?; + let adj_den = checked!(denom - Uint::one())?; + let val = checked!(val + adj_den)?; + checked!(val / denom) + } } construct_uint! { diff --git a/crates/governance/src/storage/proposal.rs b/crates/governance/src/storage/proposal.rs index 60e49c9150..c8cfd4d146 100644 --- a/crates/governance/src/storage/proposal.rs +++ b/crates/governance/src/storage/proposal.rs @@ -96,7 +96,13 @@ impl TryFrom 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, diff --git a/crates/governance/src/utils.rs b/crates/governance/src/utils.rs index 3dee0bbe1d..5221173e64 100644 --- a/crates/governance/src/utils.rs +++ b/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,10 +423,10 @@ 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, @@ -433,11 +434,18 @@ pub fn is_valid_validator_voting_period( ) -> 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, 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() + )); + } } } diff --git a/crates/governance/src/vp/mod.rs b/crates/governance/src/vp/mod.rs index a7b8114baf..2ec68c3076 100644 --- a/crates/governance/src/vp/mod.rs +++ b/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; diff --git a/crates/proof_of_stake/src/rewards.rs b/crates/proof_of_stake/src/rewards.rs index 471ef02ea9..a60b9b31dc 100644 --- a/crates/proof_of_stake/src/rewards.rs +++ b/crates/proof_of_stake/src/rewards.rs @@ -159,14 +159,12 @@ impl PosRewardsCalculator { /// Implement as ceiling of (2/3) * validator set stake fn get_min_required_votes(&self) -> token::Amount { - (self + let min_votes = self .total_stake - .checked_mul(2_u64) - .expect("Amount overflow while computing minimum required votes") - .checked_add((3u64 - 1u64).into()) - .expect("Amount overflow while computing minimum required votes")) - .checked_div_u64(3u64) - .expect("Div by non-zero cannot fail") + .raw_amount() + .frac_mul_ceil(2.into(), 3.into()) + .expect("Amount overflow while computing minimum required votes"); + min_votes.into() } } diff --git a/crates/sdk/src/rpc.rs b/crates/sdk/src/rpc.rs index 5a3e19971f..55be2a7e65 100644 --- a/crates/sdk/src/rpc.rs +++ b/crates/sdk/src/rpc.rs @@ -953,12 +953,7 @@ pub async fn query_proposal_result( let current_epoch = query_epoch(client).await?; if current_epoch < proposal.voting_start_epoch { - return Err(Error::Other(format!( - "Proposal {} is still pending, voting period will start in {} \ - epochs.", - proposal_id, - proposal.voting_end_epoch.0 - current_epoch.0 - ))); + return Ok(None); } let stored_proposal_result = convert_response::>( diff --git a/crates/tests/src/e2e/ledger_tests.rs b/crates/tests/src/e2e/ledger_tests.rs index 4470a85e39..160338c268 100644 --- a/crates/tests/src/e2e/ledger_tests.rs +++ b/crates/tests/src/e2e/ledger_tests.rs @@ -2134,10 +2134,9 @@ fn proposal_change_shielded_reward() -> Result<()> { let mut client = run!(test, Bin::Client, query_proposal, Some(15))?; client.exp_string("Proposal Id: 0")?; client.exp_string( - "passed with 100000.000000 yay votes, 900.000000 nay votes and \ + "Passed with 100000.000000 yay votes, 900.000000 nay votes and \ 0.000000 abstain votes, total voting power: 100900.000000, threshold \ - (fraction) of total voting power needed to tally: 67266.666667 \ - (0.666666666669)", + (fraction) of total voting power needed to tally: 40360.000000 (0.4)", )?; client.assert_success(); diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 9926204a54..3d73f4ee73 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -854,10 +854,9 @@ fn proposal_submission() -> Result<()> { assert_matches!(captured.result, Ok(_)); assert!(captured.contains("Proposal Id: 0")); let expected = regex::escape( - "passed with 120000.000000 yay votes, 900.000000 nay votes and \ + "Passed with 120000.000000 yay votes, 900.000000 nay votes and \ 0.000000 abstain votes, total voting power: 120900.000000, threshold \ - (fraction) of total voting power needed to tally: 80600.000000 \ - (0.666666666666)", + (fraction) of total voting power needed to tally: 48360.000000 (0.4)", ); assert!(captured.contains(&expected)); @@ -1209,7 +1208,7 @@ fn pgf_governance_proposal() -> Result<()> { let captured = CapturedOutput::of(|| run(&node, Bin::Client, query_proposal)); assert_matches!(captured.result, Ok(_)); - assert!(captured.contains("passed")); + assert!(captured.contains("Passed")); // 5. Wait proposals grace and check proposal author funds while node.current_epoch().0 < 31 { diff --git a/scripts/default_proposal_template.json b/scripts/default_proposal_template.json index af529aa5eb..563696e964 100644 --- a/scripts/default_proposal_template.json +++ b/scripts/default_proposal_template.json @@ -14,6 +14,5 @@ "voting_start_epoch": 0, "voting_end_epoch": 0, "activation_epoch": 0 - }, - "data": [] -} \ No newline at end of file + } +} diff --git a/scripts/default_proposal_template_with_wasm.json b/scripts/default_proposal_template_with_wasm.json new file mode 100644 index 0000000000..af529aa5eb --- /dev/null +++ b/scripts/default_proposal_template_with_wasm.json @@ -0,0 +1,19 @@ +{ + "proposal": { + "content": { + "title": "XXX", + "authors": "XXX, XXX", + "discussions-to": "XXX@YYY.ZZZ", + "created": "YYYY-MM-DDTHH:MM:SSZ", + "abstract": "XXX", + "motivation": "XXX", + "details": "XXX", + "requires": "0" + }, + "author": "tnamXXX", + "voting_start_epoch": 0, + "voting_end_epoch": 0, + "activation_epoch": 0 + }, + "data": [] +} \ No newline at end of file