diff --git a/.changelog/unreleased/improvements/3629-improve-init-proposal-validation.md b/.changelog/unreleased/improvements/3629-improve-init-proposal-validation.md new file mode 100644 index 0000000000..6f00cdc2c0 --- /dev/null +++ b/.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)) \ 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 11d7dd99bf..160fdff26e 100644 --- a/crates/apps_lib/src/client/rpc.rs +++ b/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( + 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; diff --git a/crates/governance/src/cli/onchain.rs b/crates/governance/src/cli/onchain.rs index f904ef0094..5186009b48 100644 --- a/crates/governance/src/cli/onchain.rs +++ b/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( diff --git a/crates/governance/src/cli/validation.rs b/crates/governance/src/cli/validation.rs index 90982a6077..37c8707fa8 100644 --- a/crates/governance/src/cli/validation.rs +++ b/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( diff --git a/crates/sdk/src/rpc.rs b/crates/sdk/src/rpc.rs index 6111774823..7651822ce0 100644 --- a/crates/sdk/src/rpc.rs +++ b/crates/sdk/src/rpc.rs @@ -936,32 +936,49 @@ pub async fn get_public_key_at( } /// Query the proposal result -pub async fn query_proposal_result( - client: &C, +pub async fn query_proposal_result( + context: &N, proposal_id: u64, ) -> Result, 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::>( - 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::>( + 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( 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( } false => { let voting_power = get_bond_amount_at( - client, + context.client(), &vote.delegator, &vote.validator, tally_epoch, diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 9c52c05624..ef0cc79080 100644 --- a/crates/sdk/src/tx.rs +++ b/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 { diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index f24213ee4b..9980581239 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/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();