Merge pull request #3629 from anoma/fraccaman/improve-init-proposal-v…

…alidation

Improve governance client validation

.changelog/unreleased/improvements/3629-improve-init-proposal-validation.md

@@ -0,0 +1,2 @@
+- Improve governance client side validation.
+  ([\#3629](https://github.com/anoma/namada/pull/3629))

crates/apps_lib/src/client/rpc.rs

@@ -435,16 +435,15 @@ async fn query_shielded_balance(
     }
 }
 
-pub async fn query_proposal_result(
-    context: &impl Namada,
+pub async fn query_proposal_result<N: Namada>(
+    context: &N,
     args: args::QueryProposalResult,
 ) {
     let proposal_id = args.proposal_id;
 
     let current_epoch = query_epoch(context.client()).await.unwrap();
     let proposal_result =
-        namada_sdk::rpc::query_proposal_result(context.client(), proposal_id)
-            .await;
+        namada_sdk::rpc::query_proposal_result(context, proposal_id).await;
     let proposal_query =
         namada_sdk::rpc::query_proposal_by_id(context.client(), proposal_id)
             .await;

crates/governance/src/cli/onchain.rs

@@ -71,17 +71,12 @@ impl DefaultProposal {
         if force {
             return Ok(self);
         }
-        is_valid_start_epoch(
-            self.proposal.voting_start_epoch,
-            current_epoch,
-            governance_parameters.min_proposal_voting_period,
-        )?;
+        is_valid_start_epoch(self.proposal.voting_start_epoch, current_epoch)?;
         is_valid_end_epoch(
             self.proposal.voting_start_epoch,
             self.proposal.voting_end_epoch,
             current_epoch,
             governance_parameters.min_proposal_voting_period,
-            governance_parameters.min_proposal_voting_period,
             governance_parameters.max_proposal_period,
         )?;
         is_valid_activation_epoch(
@@ -149,17 +144,12 @@ impl PgfStewardProposal {
         if force {
             return Ok(self);
         }
-        is_valid_start_epoch(
-            self.proposal.voting_start_epoch,
-            current_epoch,
-            governance_parameters.min_proposal_voting_period,
-        )?;
+        is_valid_start_epoch(self.proposal.voting_start_epoch, current_epoch)?;
         is_valid_end_epoch(
             self.proposal.voting_start_epoch,
             self.proposal.voting_end_epoch,
             current_epoch,
             governance_parameters.min_proposal_voting_period,
-            governance_parameters.min_proposal_voting_period,
             governance_parameters.max_proposal_period,
         )?;
         is_valid_activation_epoch(
@@ -222,17 +212,12 @@ impl PgfFundingProposal {
         if force {
             return Ok(self);
         }
-        is_valid_start_epoch(
-            self.proposal.voting_start_epoch,
-            current_epoch,
-            governance_parameters.min_proposal_voting_period,
-        )?;
+        is_valid_start_epoch(self.proposal.voting_start_epoch, current_epoch)?;
         is_valid_end_epoch(
             self.proposal.voting_start_epoch,
             self.proposal.voting_end_epoch,
             current_epoch,
             governance_parameters.min_proposal_voting_period,
-            governance_parameters.min_proposal_voting_period,
             governance_parameters.max_proposal_period,
         )?;
         is_valid_activation_epoch(

crates/governance/src/cli/validation.rs

@@ -17,14 +17,13 @@ pub enum ProposalValidation {
     /// The proposal start epoch is invalid
     #[error(
         "Invalid proposal start epoch: {0} must be greater than current epoch \
-         {1} and a multiple of {2}"
+         {1}"
     )]
-    InvalidStartEpoch(Epoch, Epoch, u64),
+    InvalidStartEpoch(Epoch, Epoch),
     /// The proposal difference between start and end epoch is invalid
     #[error(
         "Invalid proposal end epoch: difference between proposal start and \
-         end epoch must be at least {0}, at max {1} and the end epoch must be \
-         a multiple of {0}"
+         end epoch must be at least {0}, at max {1}"
     )]
     InvalidStartEndDifference(u64, u64),
     /// The proposal difference between end and activation epoch is invalid
@@ -85,19 +84,15 @@ pub fn is_valid_author_balance(
 pub fn is_valid_start_epoch(
     proposal_start_epoch: Epoch,
     current_epoch: Epoch,
-    proposal_epoch_multiplier: u64,
 ) -> Result<(), ProposalValidation> {
     let start_epoch_greater_than_current = proposal_start_epoch > current_epoch;
-    let start_epoch_is_multipler =
-        checked!(proposal_start_epoch.0 % proposal_epoch_multiplier)? == 0;
 
-    if start_epoch_greater_than_current && start_epoch_is_multipler {
+    if start_epoch_greater_than_current {
         Ok(())
     } else {
         Err(ProposalValidation::InvalidStartEpoch(
             proposal_start_epoch,
             current_epoch,
-            proposal_epoch_multiplier,
         ))
     }
 }
@@ -106,21 +101,16 @@ pub fn is_valid_end_epoch(
     proposal_start_epoch: Epoch,
     proposal_end_epoch: Epoch,
     _current_epoch: Epoch,
-    proposal_epoch_multiplier: u64,
     min_proposal_voting_period: u64,
     max_proposal_period: u64,
 ) -> Result<(), ProposalValidation> {
     let voting_period =
         checked!(proposal_end_epoch.0 - proposal_start_epoch.0)?;
-    let end_epoch_is_multipler =
-        checked!(proposal_end_epoch % proposal_epoch_multiplier)
-            .map_err(ProposalValidation::Arith)?
-            == Epoch(0);
     let is_valid_voting_period = voting_period > 0
         && voting_period >= min_proposal_voting_period
         && min_proposal_voting_period <= max_proposal_period;
 
-    if end_epoch_is_multipler && is_valid_voting_period {
+    if is_valid_voting_period {
         Ok(())
     } else {
         Err(ProposalValidation::InvalidStartEndDifference(

crates/sdk/src/rpc.rs

@@ -936,32 +936,49 @@ pub async fn get_public_key_at<C: crate::queries::Client + Sync>(
 }
 
 /// Query the proposal result
-pub async fn query_proposal_result<C: crate::queries::Client + Sync>(
-    client: &C,
+pub async fn query_proposal_result<N: Namada>(
+    context: &N,
     proposal_id: u64,
 ) -> Result<Option<ProposalResult>, Error> {
-    let proposal = query_proposal_by_id(client, proposal_id).await?;
+    let proposal = query_proposal_by_id(context.client(), proposal_id).await?;
     let proposal = if let Some(proposal) = proposal {
         proposal
     } else {
         return Ok(None);
     };
-    let stored_proposal_result = convert_response::<C, Option<ProposalResult>>(
-        RPC.vp().gov().proposal_result(client, &proposal_id).await,
-    )?;
+
+    let current_epoch = query_epoch(context.client()).await?;
+    if current_epoch < proposal.voting_start_epoch {
+        display_line!(
+            context.io(),
+            "Proposal {} is still pending, voting period will start in {} \
+             epochs.",
+            proposal_id,
+            proposal.voting_end_epoch.0 - current_epoch.0
+        );
+    }
+
+    let stored_proposal_result =
+        convert_response::<N::Client, Option<ProposalResult>>(
+            RPC.vp()
+                .gov()
+                .proposal_result(context.client(), &proposal_id)
+                .await,
+        )?;
+
     let proposal_result = match stored_proposal_result {
         Some(proposal_result) => proposal_result,
         None => {
             let tally_epoch = proposal.voting_end_epoch;
 
             let is_author_pgf_steward =
-                is_steward(client, &proposal.author).await;
-            let votes = query_proposal_votes(client, proposal_id)
+                is_steward(context.client(), &proposal.author).await;
+            let votes = query_proposal_votes(context.client(), proposal_id)
                 .await
                 .unwrap_or_default();
             let tally_type = proposal.get_tally_type(is_author_pgf_steward);
             let total_active_voting_power =
-                get_total_active_voting_power(client, tally_epoch)
+                get_total_active_voting_power(context.client(), tally_epoch)
                     .await
                     .unwrap_or_default();
 
@@ -971,7 +988,7 @@ pub async fn query_proposal_result<C: crate::queries::Client + Sync>(
                 match vote.is_validator() {
                     true => {
                         let voting_power = get_validator_stake(
-                            client,
+                            context.client(),
                             tally_epoch,
                             &vote.validator,
                         )
@@ -986,7 +1003,7 @@ pub async fn query_proposal_result<C: crate::queries::Client + Sync>(
                     }
                     false => {
                         let voting_power = get_bond_amount_at(
-                            client,
+                            context.client(),
                             &vote.delegator,
                             &vote.validator,
                             tally_epoch,

crates/sdk/src/tx.rs

@@ -2091,7 +2091,8 @@ pub async fn build_vote_proposal(
     if !proposal.can_be_voted(current_epoch, is_validator) {
         edisplay_line!(
             context.io(),
-            "Proposal {} cannot be voted on anymore.",
+            "Proposal {} cannot be voted on, either the voting period ended \
+             or the proposal is still pending.",
             proposal_id
         );
         if is_validator {

crates/tests/src/integration/ledger_tests.rs

@@ -771,60 +771,6 @@ fn proposal_submission() -> Result<()> {
     assert_matches!(captured.result, Ok(_));
     assert!(captured.contains("nam: 500"));
 
-    // 6. Submit an invalid proposal
-    // proposal is invalid due to voting_end_epoch - voting_start_epoch < 3
-    let invalid_proposal_json = prepare_proposal_data(
-        node.test_dir.path(),
-        albert,
-        TestWasms::TxProposalCode.read_bytes(),
-        1,
-    );
-
-    let submit_proposal_args = apply_use_device(vec![
-        "init-proposal",
-        "--data-path",
-        invalid_proposal_json.to_str().unwrap(),
-        "--node",
-        &validator_one_rpc,
-    ]);
-
-    let captured =
-        CapturedOutput::of(|| run(&node, Bin::Client, submit_proposal_args));
-    assert!(captured.result.is_err());
-    println!("{:?}", captured.result);
-    assert!(captured.err_contains(
-        "Proposal data are invalid: Invalid proposal start epoch: 1 must be \
-         greater than current epoch .* and a multiple of 3"
-    ));
-
-    // 7. Check invalid proposal was not submitted
-    let proposal_query_args = vec![
-        "query-proposal",
-        "--proposal-id",
-        "1",
-        "--node",
-        &validator_one_rpc,
-    ];
-    let captured =
-        CapturedOutput::of(|| run(&node, Bin::Client, proposal_query_args));
-    assert_matches!(captured.result, Ok(_));
-    assert!(captured.contains("No proposal found with id: 1"));
-
-    // 8. Query token balance (funds shall not be submitted)
-    let query_balance_args = vec![
-        "balance",
-        "--owner",
-        ALBERT,
-        "--token",
-        NAM,
-        "--node",
-        &validator_one_rpc,
-    ];
-    let captured =
-        CapturedOutput::of(|| run(&node, Bin::Client, query_balance_args));
-    assert_matches!(captured.result, Ok(_));
-    assert!(captured.contains("nam: 1979500"));
-
     // 9.1. Send a yay vote from a validator
     while node.current_epoch().0 <= 13 {
         node.next_epoch();