From cd464f9e1739174a3a2544666ca85459d70fbec5 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 22 Dec 2023 16:09:55 +0100 Subject: [PATCH 01/11] Improves formatting of `ProposalResult` --- core/src/ledger/governance/storage/vote.rs | 4 +- core/src/ledger/governance/utils.rs | 57 +++++++++++++--------- 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/core/src/ledger/governance/storage/vote.rs b/core/src/ledger/governance/storage/vote.rs index 9be49fa9b7..e819629a09 100644 --- a/core/src/ledger/governance/storage/vote.rs +++ b/core/src/ledger/governance/storage/vote.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use super::super::cli::onchain::ProposalVote; use super::proposal::ProposalType; -/// The type of a governance vote with the optional associated Memo +/// The type of a governance vote #[derive( Debug, Clone, @@ -18,7 +18,7 @@ use super::proposal::ProposalType; Deserialize, )] pub enum VoteType { - /// A default vote without Memo + /// A default vote Default, /// A vote for the PGF stewards PGFSteward, diff --git a/core/src/ledger/governance/utils.rs b/core/src/ledger/governance/utils.rs index 6b3129b588..06308c1f71 100644 --- a/core/src/ledger/governance/utils.rs +++ b/core/src/ledger/governance/utils.rs @@ -52,6 +52,7 @@ impl Vote { } /// Represent a tally type +#[derive(Copy, Clone, BorshSerialize, BorshDeserialize)] pub enum TallyType { /// Represent a tally type for proposal requiring 2/3 of the total voting /// power to be yay @@ -81,7 +82,7 @@ impl TallyType { } /// The result of a proposal -#[derive(Copy, Clone, BorshSerialize, BorshDeserialize)] +#[derive(Copy, Clone, Debug, BorshSerialize, BorshDeserialize)] pub enum TallyResult { /// Proposal was accepted with the associated value Passed, @@ -142,6 +143,8 @@ impl TallyResult { pub struct ProposalResult { /// The result of a proposal pub result: TallyResult, + /// The type of tally required for this proposal + pub tally_type: TallyType, /// The total voting power during the proposal tally pub total_voting_power: VotePower, /// The total voting power from yay votes @@ -152,27 +155,6 @@ pub struct ProposalResult { pub total_abstain_power: VotePower, } -impl Display for ProposalResult { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let percentage = self - .total_yay_power - .checked_div(self.total_voting_power) - .unwrap_or_default(); - - write!( - f, - "{} with {} yay votes and {} nay votes ({:.2}%)", - self.result, - self.total_yay_power.to_string_native(), - self.total_nay_power.to_string_native(), - percentage - .checked_mul(token::Amount::from_u64(100)) - .unwrap_or_default() - .to_string_native() - ) - } -} - impl ProposalResult { /// Return true if at least 1/3 of the total voting power voted and at least /// two third of the non-abstained voting power voted nay @@ -189,7 +171,31 @@ impl ProposalResult { } } +impl Display for ProposalResult { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let threshold = match self.tally_type { + TallyType::TwoThirds => self.total_voting_power / 3 * 2, + _ => { + let threshold_one_third = self.total_voting_power / 3; + threshold_one_third / 2 + } + }; + + write!( + f, + "{} with {} yay votes, {} nay votes and {} abstain votes, \ + threshold was: {}", + self.result, + self.total_yay_power.to_string_native(), + self.total_nay_power.to_string_native(), + self.total_abstain_power.to_string_native(), + threshold.to_string_native() + ) + } +} + /// /// General rappresentation of a vote +#[derive(Debug)] pub enum TallyVote { /// Rappresent a vote for a proposal onchain OnChain(StorageProposalVote), @@ -254,6 +260,7 @@ impl TallyVote { /// Proposal structure holding votes information necessary to compute the /// outcome +#[derive(Default, Debug)] pub struct ProposalVotes { /// Map from validator address to vote pub validators_vote: HashMap, @@ -269,7 +276,7 @@ pub struct ProposalVotes { pub fn compute_proposal_result( votes: ProposalVotes, total_voting_power: VotePower, - tally_at: TallyType, + tally_type: TallyType, ) -> ProposalResult { let mut yay_voting_power = VotePower::default(); let mut nay_voting_power = VotePower::default(); @@ -310,6 +317,7 @@ pub fn compute_proposal_result( // Force failure of the proposal return ProposalResult { result: TallyResult::Rejected, + tally_type, total_voting_power: VotePower::default(), total_yay_power: VotePower::default(), total_nay_power: VotePower::default(), @@ -352,7 +360,7 @@ pub fn compute_proposal_result( } let tally_result = TallyResult::new( - &tally_at, + &tally_type, yay_voting_power, nay_voting_power, abstain_voting_power, @@ -361,6 +369,7 @@ pub fn compute_proposal_result( ProposalResult { result: tally_result, + tally_type, total_voting_power, total_yay_power: yay_voting_power, total_nay_power: nay_voting_power, From fa47efcb5136344aa544b39d82cedb264f6c5c37 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 22 Dec 2023 17:51:18 +0100 Subject: [PATCH 02/11] Adds `query_proposal_votes` --- apps/src/lib/cli.rs | 58 +++++++++++++++++++++++++++++ apps/src/lib/cli/client.rs | 11 ++++++ apps/src/lib/client/rpc.rs | 43 +++++++++++++++++++++ core/src/ledger/governance/utils.rs | 9 +++++ sdk/src/args.rs | 11 ++++++ sdk/src/queries/vp/governance.rs | 6 +-- 6 files changed, 135 insertions(+), 3 deletions(-) diff --git a/apps/src/lib/cli.rs b/apps/src/lib/cli.rs index fca330ec48..3807f16b6d 100644 --- a/apps/src/lib/cli.rs +++ b/apps/src/lib/cli.rs @@ -257,6 +257,7 @@ pub mod cmds { .subcommand(QueryResult::def().display_order(5)) .subcommand(QueryRawBytes::def().display_order(5)) .subcommand(QueryProposal::def().display_order(5)) + .subcommand(QueryProposalVotes::def().display_order(5)) .subcommand(QueryProposalResult::def().display_order(5)) .subcommand(QueryProtocolParameters::def().display_order(5)) .subcommand(QueryPgf::def().display_order(5)) @@ -330,6 +331,8 @@ pub mod cmds { let query_result = Self::parse_with_ctx(matches, QueryResult); let query_raw_bytes = Self::parse_with_ctx(matches, QueryRawBytes); let query_proposal = Self::parse_with_ctx(matches, QueryProposal); + let query_proposal_votes = + Self::parse_with_ctx(matches, QueryProposalVotes); let query_proposal_result = Self::parse_with_ctx(matches, QueryProposalResult); let query_protocol_parameters = @@ -385,6 +388,7 @@ pub mod cmds { .or(query_result) .or(query_raw_bytes) .or(query_proposal) + .or(query_proposal_votes) .or(query_proposal_result) .or(query_protocol_parameters) .or(query_pgf) @@ -472,6 +476,7 @@ pub mod cmds { QueryFindValidator(QueryFindValidator), QueryRawBytes(QueryRawBytes), QueryProposal(QueryProposal), + QueryProposalVotes(QueryProposalVotes), QueryProposalResult(QueryProposalResult), QueryProtocolParameters(QueryProtocolParameters), QueryPgf(QueryPgf), @@ -1011,6 +1016,28 @@ pub mod cmds { } } + #[derive(Debug, Clone)] + pub struct QueryProposalVotes(pub args::QueryProposalVotes); + + impl SubCmd for QueryProposalVotes { + const CMD: &'static str = "query-proposal-votes"; + + fn parse(matches: &ArgMatches) -> Option + where + Self: Sized, + { + matches.subcommand_matches(Self::CMD).map(|matches| { + QueryProposalVotes(args::QueryProposalVotes::parse(matches)) + }) + } + + fn def() -> App { + App::new(Self::CMD) + .about("Query votes for the proposal.") + .add_args::>() + } + } + #[derive(Clone, Debug)] pub struct QueryProposal(pub args::QueryProposal); @@ -3058,6 +3085,7 @@ pub mod args { pub const VALIDATOR_ETH_HOT_KEY: ArgOpt = arg_opt("eth-hot-key"); pub const VALUE: Arg = arg("value"); + pub const VOTER_OPT: ArgOpt = arg_opt("voter"); pub const VIEWING_KEY: Arg = arg("key"); pub const VP: ArgOpt = arg_opt("vp"); pub const WALLET_ALIAS_FORCE: ArgFlag = flag("wallet-alias-force"); @@ -4788,6 +4816,36 @@ pub mod args { } } + impl CliToSdk> for QueryProposalVotes { + fn to_sdk(self, ctx: &mut Context) -> QueryProposalVotes { + QueryProposalVotes:: { + query: self.query.to_sdk(ctx), + proposal_id: self.proposal_id, + voter: self.voter.map(|x| ctx.borrow_chain_or_exit().get(&x)), + } + } + } + + impl Args for QueryProposalVotes { + fn parse(matches: &ArgMatches) -> Self { + let query = Query::parse(matches); + let proposal_id = PROPOSAL_ID.parse(matches); + let voter = VOTER_OPT.parse(matches); + + Self { + query, + proposal_id, + voter, + } + } + + fn def(app: App) -> App { + app.add_args::>() + .arg(PROPOSAL_ID_OPT.def().help("The proposal identifier.")) + .arg(VOTER_OPT.def().help("The address of the proposal voter.")) + } + } + #[derive(Clone, Debug)] pub struct QueryProposalResult { /// Common query args diff --git a/apps/src/lib/cli/client.rs b/apps/src/lib/cli/client.rs index 601988a5f8..2ae441d2d2 100644 --- a/apps/src/lib/cli/client.rs +++ b/apps/src/lib/cli/client.rs @@ -580,6 +580,17 @@ impl CliApi { let namada = ctx.to_sdk(client, io); rpc::query_proposal_result(&namada, args).await; } + Sub::QueryProposalVotes(QueryProposalVotes(mut args)) => { + let client = client.unwrap_or_else(|| { + C::from_tendermint_address( + &mut args.query.ledger_address, + ) + }); + client.wait_until_node_is_synced(&io).await?; + let args = args.to_sdk(&mut ctx); + let namada = ctx.to_sdk(client, io); + rpc::query_proposal_votes(&namada, args).await; + } Sub::QueryProtocolParameters(QueryProtocolParameters( mut args, )) => { diff --git a/apps/src/lib/client/rpc.rs b/apps/src/lib/client/rpc.rs index bc57f58f14..7dac09517c 100644 --- a/apps/src/lib/client/rpc.rs +++ b/apps/src/lib/client/rpc.rs @@ -726,6 +726,49 @@ async fn get_ibc_denom_alias( .unwrap_or(ibc_denom.as_ref().to_string()) } +/// Query votes for the given proposal +pub async fn query_proposal_votes( + context: &impl Namada, + args: args::QueryProposalVotes, +) { + let result = namada_sdk::rpc::query_proposal_votes( + context.client(), + args.proposal_id, + ) + .await + .unwrap(); + + match args.voter { + Some(voter) => { + match result.into_iter().find(|vote| vote.delegator == voter) { + Some(vote) => display_line!( + context.io(), + "The address {} voted {} on proposal {}", + voter, + vote, + args.proposal_id + ), + None => display_line!( + context.io(), + "The address {} has not voted on proposal {}", + voter, + args.proposal_id + ), + } + } + None => { + display_line!( + context.io(), + "Votes for proposal id {}", + args.proposal_id + ); + for vote in result { + display_line!(context.io(), "{}", vote); + } + } + } +} + /// Query Proposals pub async fn query_proposal(context: &impl Namada, args: args::QueryProposal) { let current_epoch = query_and_print_epoch(context).await; diff --git a/core/src/ledger/governance/utils.rs b/core/src/ledger/governance/utils.rs index 06308c1f71..1a811b6b65 100644 --- a/core/src/ledger/governance/utils.rs +++ b/core/src/ledger/governance/utils.rs @@ -44,6 +44,15 @@ pub struct Vote { pub data: StorageProposalVote, } +impl Display for Vote { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + writeln!(f, "Proposal vote")?; + writeln!(f, "Validator: {}", self.validator)?; + writeln!(f, "Delegator: {}", self.delegator)?; + writeln!(f, "Vote: {}", self.data) + } +} + impl Vote { /// Check if a vote is from a validator pub fn is_validator(&self) -> bool { diff --git a/sdk/src/args.rs b/sdk/src/args.rs index c12ecdf537..9b465671e7 100644 --- a/sdk/src/args.rs +++ b/sdk/src/args.rs @@ -1129,6 +1129,17 @@ impl RevealPk { } } +/// Query proposal votes +#[derive(Clone, Debug)] +pub struct QueryProposalVotes { + /// Common query args + pub query: Query, + /// Proposal id + pub proposal_id: u64, + /// Voter address + pub voter: Option, +} + /// Query proposal #[derive(Clone, Debug)] pub struct QueryProposal { diff --git a/sdk/src/queries/vp/governance.rs b/sdk/src/queries/vp/governance.rs index 1e3a5a8ece..ce3633f396 100644 --- a/sdk/src/queries/vp/governance.rs +++ b/sdk/src/queries/vp/governance.rs @@ -15,7 +15,7 @@ router! {GOV, ( "parameters" ) -> GovernanceParameters = parameters, } -/// Find if the given address belongs to a validator account. +/// Query the provided proposal id fn proposal_id( ctx: RequestCtx<'_, D, H, V, T>, id: u64, @@ -27,7 +27,7 @@ where storage_api::governance::get_proposal_by_id(ctx.wl_storage, id) } -/// Find if the given address belongs to a validator account. +/// Query all the votes for the given proposal id fn proposal_id_votes( ctx: RequestCtx<'_, D, H, V, T>, id: u64, @@ -39,7 +39,7 @@ where storage_api::governance::get_proposal_votes(ctx.wl_storage, id) } -/// Get the governane parameters +/// Get the governance parameters fn parameters( ctx: RequestCtx<'_, D, H, V, T>, ) -> storage_api::Result From fe5a6ba47792caf8d2ff79bc23e57771bdeb7428 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 22 Dec 2023 18:44:57 +0100 Subject: [PATCH 03/11] Improves governance messages --- apps/src/lib/client/rpc.rs | 12 +++--------- core/src/ledger/governance/utils.rs | 11 +++++------ 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/apps/src/lib/client/rpc.rs b/apps/src/lib/client/rpc.rs index 7dac09517c..7997db2706 100644 --- a/apps/src/lib/client/rpc.rs +++ b/apps/src/lib/client/rpc.rs @@ -741,13 +741,7 @@ pub async fn query_proposal_votes( match args.voter { Some(voter) => { match result.into_iter().find(|vote| vote.delegator == voter) { - Some(vote) => display_line!( - context.io(), - "The address {} voted {} on proposal {}", - voter, - vote, - args.proposal_id - ), + Some(vote) => display_line!(context.io(), "{}", vote,), None => display_line!( context.io(), "The address {} has not voted on proposal {}", @@ -759,11 +753,11 @@ pub async fn query_proposal_votes( None => { display_line!( context.io(), - "Votes for proposal id {}", + "Votes for proposal id {}\n", args.proposal_id ); for vote in result { - display_line!(context.io(), "{}", vote); + display_line!(context.io(), "{}\n", vote); } } } diff --git a/core/src/ledger/governance/utils.rs b/core/src/ledger/governance/utils.rs index 1a811b6b65..a65f17a3fa 100644 --- a/core/src/ledger/governance/utils.rs +++ b/core/src/ledger/governance/utils.rs @@ -46,10 +46,8 @@ pub struct Vote { impl Display for Vote { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "Proposal vote")?; - writeln!(f, "Validator: {}", self.validator)?; - writeln!(f, "Delegator: {}", self.delegator)?; - writeln!(f, "Vote: {}", self.data) + writeln!(f, "Voter: {}", self.delegator)?; + write!(f, "Vote: {}", self.data) } } @@ -192,12 +190,13 @@ impl Display for ProposalResult { write!( f, - "{} with {} yay votes, {} nay votes and {} abstain votes, \ - threshold was: {}", + "{} with {} yay votes, {} nay votes and {} abstain votes, total \ + voting power: {} threshold was: {}", self.result, self.total_yay_power.to_string_native(), self.total_nay_power.to_string_native(), self.total_abstain_power.to_string_native(), + self.total_voting_power.to_string_native(), threshold.to_string_native() ) } From 909c948f737454024a8a6c236408e8c228604aca Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 22 Dec 2023 18:45:20 +0100 Subject: [PATCH 04/11] Returns early if proposal voter doesn't have a stake --- sdk/src/tx.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sdk/src/tx.rs b/sdk/src/tx.rs index de0474615b..fcb96cbd4a 100644 --- a/sdk/src/tx.rs +++ b/sdk/src/tx.rs @@ -1873,6 +1873,12 @@ pub async fn build_vote_proposal( .cloned() .collect::>(); + if delegations.is_empty() { + return Err(Error::Other( + "Voter address must have delegations".to_string(), + )); + } + let data = VoteProposalData { id: proposal_id, vote: storage_vote, From 691fc8b164c6e9f405d1a109379ebc83001dfd36 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 27 Dec 2023 17:15:24 +0100 Subject: [PATCH 05/11] Removes useless `VoteType` enum. Misc refactors --- .../lib/node/ledger/shell/finalize_block.rs | 6 +- benches/native_vps.rs | 6 +- benches/txs.rs | 6 +- benches/vps.rs | 10 +- core/src/ledger/governance/cli/offline.rs | 5 - core/src/ledger/governance/cli/onchain.rs | 5 - core/src/ledger/governance/storage/vote.rs | 103 ++---------------- core/src/ledger/governance/utils.rs | 4 +- sdk/src/signing.rs | 11 +- sdk/src/tx.rs | 8 +- shared/src/ledger/governance/mod.rs | 10 -- 11 files changed, 25 insertions(+), 149 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 0e46577a0d..af29326e9f 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -817,9 +817,7 @@ mod test_finalize_block { use namada::core::ledger::eth_bridge::storage::wrapped_erc20s; use namada::core::ledger::governance::storage::keys::get_proposal_execution_key; use namada::core::ledger::governance::storage::proposal::ProposalType; - use namada::core::ledger::governance::storage::vote::{ - StorageProposalVote, VoteType, - }; + use namada::core::ledger::governance::storage::vote::StorageProposalVote; use namada::core::ledger::replay_protection; use namada::core::types::storage::KeySeg; use namada::eth_bridge::storage::bridge_pool::{ @@ -1644,7 +1642,7 @@ mod test_finalize_block { }; // Add a proposal to be accepted and one to be rejected. - add_proposal(0, StorageProposalVote::Yay(VoteType::Default)); + add_proposal(0, StorageProposalVote::Yay); add_proposal(1, StorageProposalVote::Nay); // Commit the genesis state diff --git a/benches/native_vps.rs b/benches/native_vps.rs index a1cf7e42f1..9989a0b7c2 100644 --- a/benches/native_vps.rs +++ b/benches/native_vps.rs @@ -7,9 +7,7 @@ use criterion::{criterion_group, criterion_main, Criterion}; use masp_primitives::bls12_381; use masp_primitives::sapling::Node; use namada::core::ledger::governance::storage::proposal::ProposalType; -use namada::core::ledger::governance::storage::vote::{ - StorageProposalVote, VoteType, -}; +use namada::core::ledger::governance::storage::vote::StorageProposalVote; use namada::core::ledger::ibc::{IbcActions, TransferModule}; use namada::core::ledger::pgf::storage::steward::StewardDetail; use namada::core::ledger::storage_api::{StorageRead, StorageWrite}; @@ -87,7 +85,7 @@ fn governance(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay(VoteType::Default), + vote: StorageProposalVote::Yay, voter: defaults::albert_address(), delegations: vec![defaults::validator_address()], }, diff --git a/benches/txs.rs b/benches/txs.rs index 523cd48489..03b78f2bd0 100644 --- a/benches/txs.rs +++ b/benches/txs.rs @@ -3,9 +3,7 @@ use std::str::FromStr; use criterion::{criterion_group, criterion_main, Criterion}; use namada::core::ledger::governance::storage::proposal::ProposalType; -use namada::core::ledger::governance::storage::vote::{ - StorageProposalVote, VoteType, -}; +use namada::core::ledger::governance::storage::vote::StorageProposalVote; use namada::core::ledger::pgf::storage::steward::StewardDetail; use namada::core::types::key::{ common, SecretKey as SecretKeyInterface, SigScheme, @@ -550,7 +548,7 @@ fn vote_proposal(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay(VoteType::Default), + vote: StorageProposalVote::Yay, voter: defaults::albert_address(), delegations: vec![defaults::validator_address()], }, diff --git a/benches/vps.rs b/benches/vps.rs index 620d14c43f..e3fb0d0c30 100644 --- a/benches/vps.rs +++ b/benches/vps.rs @@ -1,9 +1,7 @@ use std::collections::BTreeSet; use criterion::{criterion_group, criterion_main, Criterion}; -use namada::core::ledger::governance::storage::vote::{ - StorageProposalVote, VoteType, -}; +use namada::core::ledger::governance::storage::vote::StorageProposalVote; use namada::core::types::address::{self, Address}; use namada::core::types::key::{ common, SecretKey as SecretKeyInterface, SigScheme, @@ -99,7 +97,7 @@ fn vp_user(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay(VoteType::Default), + vote: StorageProposalVote::Yay, voter: defaults::albert_address(), delegations: vec![defaults::validator_address()], }, @@ -239,7 +237,7 @@ fn vp_implicit(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay(VoteType::Default), + vote: StorageProposalVote::Yay, voter: Address::from(&implicit_account.to_public()), delegations: vec![], /* NOTE: no need to bond tokens because the * implicit vp doesn't check that */ @@ -396,7 +394,7 @@ fn vp_validator(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay(VoteType::Default), + vote: StorageProposalVote::Yay, voter: defaults::validator_address(), delegations: vec![], }, diff --git a/core/src/ledger/governance/cli/offline.rs b/core/src/ledger/governance/cli/offline.rs index ccf06a0b98..3a6e5ebb99 100644 --- a/core/src/ledger/governance/cli/offline.rs +++ b/core/src/ledger/governance/cli/offline.rs @@ -251,11 +251,6 @@ impl OfflineVote { self.vote.is_abstain() } - /// Check if two votes are equal - pub fn is_same_side(&self, other: &Self) -> bool { - self.vote.is_same_side(&other.vote) - } - /// compute the hash of a proposal pub fn compute_hash(&self) -> Hash { let proposal_hash_data = self.proposal_hash.serialize_to_vec(); diff --git a/core/src/ledger/governance/cli/onchain.rs b/core/src/ledger/governance/cli/onchain.rs index 60e70ddf7d..bc691d87c7 100644 --- a/core/src/ledger/governance/cli/onchain.rs +++ b/core/src/ledger/governance/cli/onchain.rs @@ -360,9 +360,4 @@ impl ProposalVote { pub fn is_abstain(&self) -> bool { matches!(self, ProposalVote::Abstain) } - - /// Check if two votes are equal - pub fn is_same_side(&self, other: &Self) -> bool { - std::mem::discriminant(self) == std::mem::discriminant(other) - } } diff --git a/core/src/ledger/governance/storage/vote.rs b/core/src/ledger/governance/storage/vote.rs index e819629a09..acde07f141 100644 --- a/core/src/ledger/governance/storage/vote.rs +++ b/core/src/ledger/governance/storage/vote.rs @@ -4,27 +4,6 @@ use borsh::{BorshDeserialize, BorshSerialize}; use serde::{Deserialize, Serialize}; use super::super::cli::onchain::ProposalVote; -use super::proposal::ProposalType; - -/// The type of a governance vote -#[derive( - Debug, - Clone, - PartialEq, - BorshSerialize, - BorshDeserialize, - Eq, - Serialize, - Deserialize, -)] -pub enum VoteType { - /// A default vote - Default, - /// A vote for the PGF stewards - PGFSteward, - /// A vote for a PGF payment proposal - PGFPayment, -} #[derive( Debug, @@ -39,7 +18,7 @@ pub enum VoteType { /// The vote for a proposal pub enum StorageProposalVote { /// Yes - Yay(VoteType), + Yay, /// No Nay, /// Abstain @@ -49,7 +28,7 @@ pub enum StorageProposalVote { impl StorageProposalVote { /// Check if a vote is yay pub fn is_yay(&self) -> bool { - matches!(self, StorageProposalVote::Yay(_)) + matches!(self, StorageProposalVote::Yay) } /// Check if a vote is nay @@ -61,55 +40,14 @@ impl StorageProposalVote { pub fn is_abstain(&self) -> bool { matches!(self, StorageProposalVote::Abstain) } +} - /// Check if two votes are equal - pub fn is_same_side(&self, other: &Self) -> bool { - std::mem::discriminant(self) == std::mem::discriminant(other) - } - - /// Check if vote is of type default - pub fn is_default_vote(&self) -> bool { - matches!( - self, - StorageProposalVote::Yay(VoteType::Default) - | StorageProposalVote::Nay - ) - } - - /// Check if a vote is compatible with a proposal - pub fn is_compatible(&self, proposal_type: &ProposalType) -> bool { - match self { - StorageProposalVote::Yay(vote_type) => proposal_type.eq(vote_type), - StorageProposalVote::Nay => true, - StorageProposalVote::Abstain => true, - } - } - - /// Create a new vote - pub fn build( - proposal_vote: &ProposalVote, - proposal_type: &ProposalType, - ) -> Option { - match (proposal_vote, proposal_type) { - (ProposalVote::Yay, ProposalType::Default(_)) => { - Some(StorageProposalVote::Yay(VoteType::Default)) - } - (ProposalVote::Yay, ProposalType::PGFSteward(_)) => { - Some(StorageProposalVote::Yay(VoteType::PGFSteward)) - } - (ProposalVote::Yay, ProposalType::PGFPayment(_)) => { - Some(StorageProposalVote::Yay(VoteType::PGFPayment)) - } - (ProposalVote::Nay, ProposalType::Default(_)) => { - Some(StorageProposalVote::Nay) - } - (ProposalVote::Nay, ProposalType::PGFSteward(_)) => { - Some(StorageProposalVote::Nay) - } - (ProposalVote::Nay, ProposalType::PGFPayment(_)) => { - Some(StorageProposalVote::Nay) - } - _ => None, +impl From<&ProposalVote> for StorageProposalVote { + fn from(value: &ProposalVote) -> Self { + match value { + ProposalVote::Yay => StorageProposalVote::Yay, + ProposalVote::Nay => StorageProposalVote::Nay, + ProposalVote::Abstain => StorageProposalVote::Abstain, } } } @@ -117,34 +55,13 @@ impl StorageProposalVote { impl Display for StorageProposalVote { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - StorageProposalVote::Yay(vote_type) => match vote_type { - VoteType::Default - | VoteType::PGFSteward - | VoteType::PGFPayment => write!(f, "yay"), - }, - + StorageProposalVote::Yay => write!(f, "yay"), StorageProposalVote::Nay => write!(f, "nay"), StorageProposalVote::Abstain => write!(f, "abstain"), } } } -impl PartialEq for ProposalType { - fn eq(&self, other: &VoteType) -> bool { - match self { - Self::Default(_) => { - matches!(other, VoteType::Default) - } - Self::PGFSteward(_) => { - matches!(other, VoteType::PGFSteward) - } - Self::PGFPayment(_) => { - matches!(other, VoteType::PGFPayment) - } - } - } -} - #[cfg(any(test, feature = "testing"))] /// Testing helpers and strategies for governance votes pub mod testing { diff --git a/core/src/ledger/governance/utils.rs b/core/src/ledger/governance/utils.rs index a65f17a3fa..24ee23c128 100644 --- a/core/src/ledger/governance/utils.rs +++ b/core/src/ledger/governance/utils.rs @@ -256,10 +256,10 @@ impl TallyVote { ) -> Result { match (self, other) { (TallyVote::OnChain(vote), TallyVote::OnChain(other_vote)) => { - Ok(vote.is_same_side(other_vote)) + Ok(vote == other_vote) } (TallyVote::Offline(vote), TallyVote::Offline(other_vote)) => { - Ok(vote.is_same_side(other_vote)) + Ok(vote.vote == other_vote.vote) } _ => Err("Cannot compare different variants of governance votes"), } diff --git a/sdk/src/signing.rs b/sdk/src/signing.rs index 38aec72d4b..0eb76d620c 100644 --- a/sdk/src/signing.rs +++ b/sdk/src/signing.rs @@ -40,7 +40,7 @@ use super::masp::{ShieldedContext, ShieldedTransfer}; use crate::args::SdkTypes; use crate::core::ledger::governance::storage::proposal::ProposalType; use crate::core::ledger::governance::storage::vote::{ - StorageProposalVote, VoteType, + StorageProposalVote, }; use crate::core::types::eth_bridge_pool::PendingTransfer; use crate::error::{EncodingError, Error, TxError}; @@ -900,14 +900,7 @@ struct LedgerProposalVote<'a>(&'a StorageProposalVote); impl<'a> Display for LedgerProposalVote<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.0 { - StorageProposalVote::Yay(vote_type) => match vote_type { - VoteType::Default => write!(f, "yay"), - VoteType::PGFSteward => write!(f, "yay for PGF steward"), - VoteType::PGFPayment => { - write!(f, "yay for PGF payment proposal") - } - }, - + StorageProposalVote::Yay => write!(f, "yay"), StorageProposalVote::Nay => write!(f, "nay"), StorageProposalVote::Abstain => write!(f, "abstain"), } diff --git a/sdk/src/tx.rs b/sdk/src/tx.rs index fcb96cbd4a..a0e762d8c4 100644 --- a/sdk/src/tx.rs +++ b/sdk/src/tx.rs @@ -1843,13 +1843,7 @@ pub async fn build_vote_proposal( return Err(Error::from(TxError::ProposalDoesNotExist(proposal_id))); }; - let storage_vote = - StorageProposalVote::build(&proposal_vote, &proposal.r#type) - .ok_or_else(|| { - Error::from(TxError::Other( - "Should be able to build the proposal vote".to_string(), - )) - })?; + let storage_vote = StorageProposalVote::from(&proposal_vote); let is_validator = rpc::is_validator(context.client(), voter).await?; diff --git a/shared/src/ledger/governance/mod.rs b/shared/src/ledger/governance/mod.rs index 610aaa831f..edcf77ff2d 100644 --- a/shared/src/ledger/governance/mod.rs +++ b/shared/src/ledger/governance/mod.rs @@ -9,7 +9,6 @@ use namada_core::ledger::governance::storage::keys as gov_storage; use namada_core::ledger::governance::storage::proposal::{ AddRemove, ProposalType, }; -use namada_core::ledger::governance::storage::vote::StorageProposalVote; use namada_core::ledger::governance::utils::is_valid_validator_voting_period; use namada_core::ledger::storage; use namada_core::ledger::storage_api::account; @@ -188,7 +187,6 @@ where gov_storage::get_voting_start_epoch_key(proposal_id); let voting_end_epoch_key = gov_storage::get_voting_end_epoch_key(proposal_id); - let proposal_type_key = gov_storage::get_proposal_type_key(proposal_id); let current_epoch = self.ctx.get_block_epoch()?; @@ -197,14 +195,10 @@ where self.force_read(&voting_start_epoch_key, ReadType::Pre)?; let pre_voting_end_epoch: Epoch = self.force_read(&voting_end_epoch_key, ReadType::Pre)?; - let proposal_type: ProposalType = - self.force_read(&proposal_type_key, ReadType::Pre)?; let voter = gov_storage::get_voter_address(key); let delegation_address = gov_storage::get_vote_delegation_address(key); - let vote: StorageProposalVote = self.force_read(key, ReadType::Post)?; - let (voter_address, delegation_address) = match (voter, delegation_address) { (Some(voter_address), Some(delegator_address)) => { @@ -238,10 +232,6 @@ where return Ok(false); } - if !vote.is_compatible(&proposal_type) { - return Err(Error::InvalidVoteType); - } - // first check if validator, then check if delegator let is_validator = self .is_validator( From c2fa5291b5f648adbdfade9b7eb0e7d882c35a4e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 27 Dec 2023 17:55:15 +0100 Subject: [PATCH 06/11] Removes duplicated proposal vote enum --- apps/src/lib/client/tx.rs | 3 +- .../lib/node/ledger/shell/finalize_block.rs | 6 +- benches/native_vps.rs | 6 +- benches/txs.rs | 6 +- benches/vps.rs | 8 +-- core/src/ledger/governance/cli/offline.rs | 2 +- core/src/ledger/governance/cli/onchain.rs | 49 --------------- core/src/ledger/governance/storage/vote.rs | 61 ++++++++----------- core/src/ledger/governance/utils.rs | 10 +-- core/src/ledger/storage_api/governance.rs | 8 +-- core/src/types/transaction/governance.rs | 4 +- light_sdk/src/transaction/governance.rs | 4 +- sdk/src/signing.rs | 12 ++-- sdk/src/tx.rs | 7 +-- 14 files changed, 59 insertions(+), 127 deletions(-) diff --git a/apps/src/lib/client/tx.rs b/apps/src/lib/client/tx.rs index f57cddd4eb..faf5236d2b 100644 --- a/apps/src/lib/client/tx.rs +++ b/apps/src/lib/client/tx.rs @@ -11,8 +11,9 @@ use namada::core::ledger::governance::cli::offline::{ OfflineProposal, OfflineSignedProposal, OfflineVote, }; use namada::core::ledger::governance::cli::onchain::{ - DefaultProposal, PgfFundingProposal, PgfStewardProposal, ProposalVote, + DefaultProposal, PgfFundingProposal, PgfStewardProposal, }; +use namada::core::ledger::governance::storage::vote::ProposalVote; use namada::core::ledger::storage::EPOCH_SWITCH_BLOCKS_DELAY; use namada::ibc::apps::transfer::types::Memo; use namada::proto::{CompressedSignature, Section, Signer, Tx}; diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index af29326e9f..3620d2f3b0 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -817,7 +817,7 @@ mod test_finalize_block { use namada::core::ledger::eth_bridge::storage::wrapped_erc20s; use namada::core::ledger::governance::storage::keys::get_proposal_execution_key; use namada::core::ledger::governance::storage::proposal::ProposalType; - use namada::core::ledger::governance::storage::vote::StorageProposalVote; + use namada::core::ledger::governance::storage::vote::ProposalVote; use namada::core::ledger::replay_protection; use namada::core::types::storage::KeySeg; use namada::eth_bridge::storage::bridge_pool::{ @@ -1642,8 +1642,8 @@ mod test_finalize_block { }; // Add a proposal to be accepted and one to be rejected. - add_proposal(0, StorageProposalVote::Yay); - add_proposal(1, StorageProposalVote::Nay); + add_proposal(0, ProposalVote::Yay); + add_proposal(1, ProposalVote::Nay); // Commit the genesis state shell.wl_storage.commit_block().unwrap(); diff --git a/benches/native_vps.rs b/benches/native_vps.rs index 9989a0b7c2..a4829c4601 100644 --- a/benches/native_vps.rs +++ b/benches/native_vps.rs @@ -7,7 +7,7 @@ use criterion::{criterion_group, criterion_main, Criterion}; use masp_primitives::bls12_381; use masp_primitives::sapling::Node; use namada::core::ledger::governance::storage::proposal::ProposalType; -use namada::core::ledger::governance::storage::vote::StorageProposalVote; +use namada::core::ledger::governance::storage::vote::ProposalVote; use namada::core::ledger::ibc::{IbcActions, TransferModule}; use namada::core::ledger::pgf::storage::steward::StewardDetail; use namada::core::ledger::storage_api::{StorageRead, StorageWrite}; @@ -85,7 +85,7 @@ fn governance(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay, + vote: ProposalVote::Yay, voter: defaults::albert_address(), delegations: vec![defaults::validator_address()], }, @@ -101,7 +101,7 @@ fn governance(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Nay, + vote: ProposalVote::Nay, voter: defaults::validator_address(), delegations: vec![], }, diff --git a/benches/txs.rs b/benches/txs.rs index 03b78f2bd0..d7290852bb 100644 --- a/benches/txs.rs +++ b/benches/txs.rs @@ -3,7 +3,7 @@ use std::str::FromStr; use criterion::{criterion_group, criterion_main, Criterion}; use namada::core::ledger::governance::storage::proposal::ProposalType; -use namada::core::ledger::governance::storage::vote::StorageProposalVote; +use namada::core::ledger::governance::storage::vote::ProposalVote; use namada::core::ledger::pgf::storage::steward::StewardDetail; use namada::core::types::key::{ common, SecretKey as SecretKeyInterface, SigScheme, @@ -548,7 +548,7 @@ fn vote_proposal(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay, + vote: ProposalVote::Yay, voter: defaults::albert_address(), delegations: vec![defaults::validator_address()], }, @@ -561,7 +561,7 @@ fn vote_proposal(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Nay, + vote: ProposalVote::Nay, voter: defaults::validator_address(), delegations: vec![], }, diff --git a/benches/vps.rs b/benches/vps.rs index e3fb0d0c30..2a0036aadf 100644 --- a/benches/vps.rs +++ b/benches/vps.rs @@ -1,7 +1,7 @@ use std::collections::BTreeSet; use criterion::{criterion_group, criterion_main, Criterion}; -use namada::core::ledger::governance::storage::vote::StorageProposalVote; +use namada::core::ledger::governance::storage::vote::ProposalVote; use namada::core::types::address::{self, Address}; use namada::core::types::key::{ common, SecretKey as SecretKeyInterface, SigScheme, @@ -97,7 +97,7 @@ fn vp_user(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay, + vote: ProposalVote::Yay, voter: defaults::albert_address(), delegations: vec![defaults::validator_address()], }, @@ -237,7 +237,7 @@ fn vp_implicit(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay, + vote: ProposalVote::Yay, voter: Address::from(&implicit_account.to_public()), delegations: vec![], /* NOTE: no need to bond tokens because the * implicit vp doesn't check that */ @@ -394,7 +394,7 @@ fn vp_validator(c: &mut Criterion) { TX_VOTE_PROPOSAL_WASM, VoteProposalData { id: 0, - vote: StorageProposalVote::Yay, + vote: ProposalVote::Yay, voter: defaults::validator_address(), delegations: vec![], }, diff --git a/core/src/ledger/governance/cli/offline.rs b/core/src/ledger/governance/cli/offline.rs index 3a6e5ebb99..cdd800a600 100644 --- a/core/src/ledger/governance/cli/offline.rs +++ b/core/src/ledger/governance/cli/offline.rs @@ -6,8 +6,8 @@ use borsh::{BorshDeserialize, BorshSerialize}; use borsh_ext::BorshSerializeExt; use serde::{Deserialize, Serialize}; -use super::onchain::ProposalVote; use super::validation::{is_valid_tally_epoch, ProposalValidation}; +use crate::ledger::governance::storage::vote::ProposalVote; use crate::proto::SignatureIndex; use crate::types::account::AccountPublicKeysMap; use crate::types::address::Address; diff --git a/core/src/ledger/governance/cli/onchain.rs b/core/src/ledger/governance/cli/onchain.rs index bc691d87c7..976557c913 100644 --- a/core/src/ledger/governance/cli/onchain.rs +++ b/core/src/ledger/governance/cli/onchain.rs @@ -312,52 +312,3 @@ pub struct PgfFundingTarget { /// Target address pub address: Address, } - -/// Represent an proposal vote -#[derive( - Debug, - Clone, - BorshSerialize, - BorshDeserialize, - Serialize, - Deserialize, - PartialEq, -)] -pub enum ProposalVote { - /// Represent an yay proposal vote - Yay, - /// Represent an nay proposal vote - Nay, - /// Represent an abstain proposal vote - Abstain, -} - -impl TryFrom for ProposalVote { - type Error = String; - - fn try_from(value: String) -> Result { - match value.trim().to_lowercase().as_str() { - "yay" => Ok(ProposalVote::Yay), - "nay" => Ok(ProposalVote::Nay), - "abstain" => Ok(ProposalVote::Abstain), - _ => Err("invalid vote".to_string()), - } - } -} - -impl ProposalVote { - /// Check if the vote type is yay - pub fn is_yay(&self) -> bool { - matches!(self, ProposalVote::Yay) - } - - /// Check if the vote type is nay - pub fn is_nay(&self) -> bool { - matches!(self, ProposalVote::Nay) - } - - /// Check if the vote type is abstain - pub fn is_abstain(&self) -> bool { - matches!(self, ProposalVote::Abstain) - } -} diff --git a/core/src/ledger/governance/storage/vote.rs b/core/src/ledger/governance/storage/vote.rs index acde07f141..2016383664 100644 --- a/core/src/ledger/governance/storage/vote.rs +++ b/core/src/ledger/governance/storage/vote.rs @@ -3,8 +3,6 @@ use std::fmt::Display; use borsh::{BorshDeserialize, BorshSerialize}; use serde::{Deserialize, Serialize}; -use super::super::cli::onchain::ProposalVote; - #[derive( Debug, Clone, @@ -16,7 +14,7 @@ use super::super::cli::onchain::ProposalVote; Deserialize, )] /// The vote for a proposal -pub enum StorageProposalVote { +pub enum ProposalVote { /// Yes Yay, /// No @@ -25,39 +23,42 @@ pub enum StorageProposalVote { Abstain, } -impl StorageProposalVote { +impl ProposalVote { /// Check if a vote is yay pub fn is_yay(&self) -> bool { - matches!(self, StorageProposalVote::Yay) + matches!(self, ProposalVote::Yay) } /// Check if a vote is nay pub fn is_nay(&self) -> bool { - matches!(self, StorageProposalVote::Nay) + matches!(self, ProposalVote::Nay) } /// Check if a vote is abstain pub fn is_abstain(&self) -> bool { - matches!(self, StorageProposalVote::Abstain) + matches!(self, ProposalVote::Abstain) } } -impl From<&ProposalVote> for StorageProposalVote { - fn from(value: &ProposalVote) -> Self { - match value { - ProposalVote::Yay => StorageProposalVote::Yay, - ProposalVote::Nay => StorageProposalVote::Nay, - ProposalVote::Abstain => StorageProposalVote::Abstain, +impl Display for ProposalVote { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ProposalVote::Yay => write!(f, "yay"), + ProposalVote::Nay => write!(f, "nay"), + ProposalVote::Abstain => write!(f, "abstain"), } } } -impl Display for StorageProposalVote { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - StorageProposalVote::Yay => write!(f, "yay"), - StorageProposalVote::Nay => write!(f, "nay"), - StorageProposalVote::Abstain => write!(f, "abstain"), +impl TryFrom for ProposalVote { + type Error = String; + + fn try_from(value: String) -> Result { + match value.trim().to_lowercase().as_str() { + "yay" => Ok(ProposalVote::Yay), + "nay" => Ok(ProposalVote::Nay), + "abstain" => Ok(ProposalVote::Abstain), + _ => Err("invalid vote".to_string()), } } } @@ -66,28 +67,14 @@ impl Display for StorageProposalVote { /// Testing helpers and strategies for governance votes pub mod testing { use proptest::prelude::{Just, Strategy}; - use proptest::prop_compose; use super::*; - prop_compose! { - /// Geerate an arbitrary vote type - pub fn arb_vote_type()(discriminant in 0..3) -> VoteType { - match discriminant { - 0 => VoteType::Default, - 1 => VoteType::PGFSteward, - 2 => VoteType::PGFPayment, - _ => unreachable!(), - } - } - } - /// Generate an arbitrary proposal vote - pub fn arb_proposal_vote() -> impl Strategy { - arb_vote_type() - .prop_map(StorageProposalVote::Yay) + pub fn arb_proposal_vote() -> impl Strategy { + Just(ProposalVote::Yay) .boxed() - .prop_union(Just(StorageProposalVote::Nay).boxed()) - .or(Just(StorageProposalVote::Abstain).boxed()) + .prop_union(Just(ProposalVote::Nay).boxed()) + .or(Just(ProposalVote::Abstain).boxed()) } } diff --git a/core/src/ledger/governance/utils.rs b/core/src/ledger/governance/utils.rs index 24ee23c128..fc23a27afe 100644 --- a/core/src/ledger/governance/utils.rs +++ b/core/src/ledger/governance/utils.rs @@ -5,7 +5,7 @@ use borsh::{BorshDeserialize, BorshSerialize}; use super::cli::offline::OfflineVote; use super::storage::proposal::ProposalType; -use super::storage::vote::StorageProposalVote; +use super::storage::vote::ProposalVote; use crate::types::address::Address; use crate::types::storage::Epoch; use crate::types::token; @@ -41,7 +41,7 @@ pub struct Vote { /// Field holding the address of the delegator pub delegator: Address, /// Field holding vote data - pub data: StorageProposalVote, + pub data: ProposalVote, } impl Display for Vote { @@ -206,13 +206,13 @@ impl Display for ProposalResult { #[derive(Debug)] pub enum TallyVote { /// Rappresent a vote for a proposal onchain - OnChain(StorageProposalVote), + OnChain(ProposalVote), /// Rappresent a vote for a proposal offline Offline(OfflineVote), } -impl From for TallyVote { - fn from(vote: StorageProposalVote) -> Self { +impl From for TallyVote { + fn from(vote: ProposalVote) -> Self { Self::OnChain(vote) } } diff --git a/core/src/ledger/storage_api/governance.rs b/core/src/ledger/storage_api/governance.rs index 3f3dfb2af6..4daf731fb4 100644 --- a/core/src/ledger/storage_api/governance.rs +++ b/core/src/ledger/storage_api/governance.rs @@ -10,7 +10,7 @@ use crate::ledger::governance::storage::keys as governance_keys; use crate::ledger::governance::storage::proposal::{ ProposalType, StorageProposal, }; -use crate::ledger::governance::storage::vote::StorageProposalVote; +use crate::ledger::governance::storage::vote::ProposalVote; use crate::ledger::governance::utils::Vote; use crate::ledger::governance::ADDRESS as governance_address; use crate::ledger::storage_api::{self, StorageRead, StorageWrite}; @@ -168,10 +168,8 @@ where { let vote_prefix_key = governance_keys::get_proposal_vote_prefix_key(proposal_id); - let vote_iter = storage_api::iter_prefix::( - storage, - &vote_prefix_key, - )?; + let vote_iter = + storage_api::iter_prefix::(storage, &vote_prefix_key)?; let votes = vote_iter .filter_map(|vote_result| { diff --git a/core/src/types/transaction/governance.rs b/core/src/types/transaction/governance.rs index 8e43b488e3..0e7f41179b 100644 --- a/core/src/types/transaction/governance.rs +++ b/core/src/types/transaction/governance.rs @@ -10,7 +10,7 @@ use crate::ledger::governance::cli::onchain::{ use crate::ledger::governance::storage::proposal::{ AddRemove, PGFAction, PGFTarget, ProposalType, }; -use crate::ledger::governance::storage::vote::StorageProposalVote; +use crate::ledger::governance::storage::vote::ProposalVote; use crate::types::address::Address; use crate::types::hash::Hash; use crate::types::storage::Epoch; @@ -73,7 +73,7 @@ pub struct VoteProposalData { /// The proposal id pub id: u64, /// The proposal vote - pub vote: StorageProposalVote, + pub vote: ProposalVote, /// The proposal author address pub voter: Address, /// Delegator addresses diff --git a/light_sdk/src/transaction/governance.rs b/light_sdk/src/transaction/governance.rs index ca6183569e..c09f0fc8c1 100644 --- a/light_sdk/src/transaction/governance.rs +++ b/light_sdk/src/transaction/governance.rs @@ -1,5 +1,5 @@ use namada_core::ledger::governance::storage::proposal::ProposalType; -use namada_core::ledger::governance::storage::vote::StorageProposalVote; +use namada_core::ledger::governance::storage::vote::ProposalVote; use namada_core::proto::Tx; use namada_core::types::address::Address; use namada_core::types::hash::Hash; @@ -75,7 +75,7 @@ impl VoteProposal { /// Build a raw VoteProposal transaction from the given parameters pub fn new( id: u64, - vote: StorageProposalVote, + vote: ProposalVote, voter: Address, delegations: Vec
, args: GlobalArgs, diff --git a/sdk/src/signing.rs b/sdk/src/signing.rs index 0eb76d620c..e52eb668b4 100644 --- a/sdk/src/signing.rs +++ b/sdk/src/signing.rs @@ -39,9 +39,7 @@ use tokio::sync::RwLock; use super::masp::{ShieldedContext, ShieldedTransfer}; use crate::args::SdkTypes; use crate::core::ledger::governance::storage::proposal::ProposalType; -use crate::core::ledger::governance::storage::vote::{ - StorageProposalVote, -}; +use crate::core::ledger::governance::storage::vote::ProposalVote; use crate::core::types::eth_bridge_pool::PendingTransfer; use crate::error::{EncodingError, Error, TxError}; use crate::ibc::apps::transfer::types::msgs::transfer::MsgTransfer; @@ -895,14 +893,14 @@ fn to_ledger_decimal(amount: &str) -> String { /// A ProposalVote wrapper that prints the spending cap with Ledger decimal /// formatting. -struct LedgerProposalVote<'a>(&'a StorageProposalVote); +struct LedgerProposalVote<'a>(&'a ProposalVote); impl<'a> Display for LedgerProposalVote<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.0 { - StorageProposalVote::Yay => write!(f, "yay"), - StorageProposalVote::Nay => write!(f, "nay"), - StorageProposalVote::Abstain => write!(f, "abstain"), + ProposalVote::Yay => write!(f, "yay"), + ProposalVote::Nay => write!(f, "nay"), + ProposalVote::Abstain => write!(f, "abstain"), } } } diff --git a/sdk/src/tx.rs b/sdk/src/tx.rs index a0e762d8c4..e86bfc0c1c 100644 --- a/sdk/src/tx.rs +++ b/sdk/src/tx.rs @@ -26,10 +26,9 @@ use namada_core::ibc::core::host::types::identifiers::{ChannelId, PortId}; use namada_core::ibc::primitives::{Msg, Timestamp as IbcTimestamp}; use namada_core::ledger::governance::cli::onchain::{ DefaultProposal, OnChainProposal, PgfFundingProposal, PgfStewardProposal, - ProposalVote, }; use namada_core::ledger::governance::storage::proposal::ProposalType; -use namada_core::ledger::governance::storage::vote::StorageProposalVote; +use namada_core::ledger::governance::storage::vote::ProposalVote; use namada_core::ledger::ibc::storage::channel_key; use namada_core::ledger::pgf::cli::steward::Commission; use namada_core::types::address::{Address, InternalAddress, MASP}; @@ -1843,8 +1842,6 @@ pub async fn build_vote_proposal( return Err(Error::from(TxError::ProposalDoesNotExist(proposal_id))); }; - let storage_vote = StorageProposalVote::from(&proposal_vote); - let is_validator = rpc::is_validator(context.client(), voter).await?; if !proposal.can_be_voted(epoch, is_validator) { @@ -1875,7 +1872,7 @@ pub async fn build_vote_proposal( let data = VoteProposalData { id: proposal_id, - vote: storage_vote, + vote: proposal_vote, voter: voter.clone(), delegations, }; From 63e6611e461566ad3ccbad5b306a10cbd687ccbb Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 28 Dec 2023 12:28:12 +0100 Subject: [PATCH 07/11] Fixes broken governance test --- tests/src/e2e/ledger_tests.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/src/e2e/ledger_tests.rs b/tests/src/e2e/ledger_tests.rs index ca123601a1..60da7a9b3e 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -1992,8 +1992,8 @@ fn proposal_submission() -> Result<()> { // this is valid because the client filter ALBERT delegation and there are // none let mut client = run!(test, Bin::Client, submit_proposal_vote, Some(15))?; - client.exp_string(TX_APPLIED_SUCCESS)?; - client.assert_success(); + client.exp_string("Voter address must have delegations")?; + client.assert_failure(); // 11. Query the proposal and check the result let mut epoch = get_epoch(&test, &validator_one_rpc).unwrap(); @@ -2013,7 +2013,9 @@ fn proposal_submission() -> 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 and 900.000000 nay votes (0.%)", + "passed with 100000.000000 yay votes, 900.000000 nay votes and \ + 0.000000 abstain votes, total voting power: 100900.000000 threshold \ + was: 67266.666666", )?; client.assert_success(); From 5f735e03a72cec6e65d2002811fc59a23bde969f Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 5 Jan 2024 14:42:20 +0000 Subject: [PATCH 08/11] Make `proposal_id` a required field in gov proposals --- apps/src/lib/bench_utils.rs | 2 +- apps/src/lib/node/ledger/shell/finalize_block.rs | 2 +- benches/native_vps.rs | 6 +++--- benches/txs.rs | 4 ++-- core/src/ledger/governance/cli/onchain.rs | 2 +- core/src/ledger/storage_api/governance.rs | 9 ++++----- core/src/types/transaction/governance.rs | 4 ++-- light_sdk/src/transaction/governance.rs | 2 +- sdk/src/signing.rs | 9 +++------ 9 files changed, 18 insertions(+), 22 deletions(-) diff --git a/apps/src/lib/bench_utils.rs b/apps/src/lib/bench_utils.rs index ddfae1a4cc..da55dfc8a8 100644 --- a/apps/src/lib/bench_utils.rs +++ b/apps/src/lib/bench_utils.rs @@ -244,7 +244,7 @@ impl Default for BenchShell { let signed_tx = bench_shell.generate_tx( TX_INIT_PROPOSAL_WASM, InitProposalData { - id: None, + id: 0, content: content_section.get_hash(), author: defaults::albert_address(), r#type: ProposalType::Default(None), diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 3620d2f3b0..7ec6e3dd42 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -1612,7 +1612,7 @@ mod test_finalize_block { shell.proposal_data.insert(proposal_id); let proposal = InitProposalData { - id: Some(proposal_id), + id: proposal_id, content: Hash::default(), author: validator.clone(), voting_start_epoch: Epoch::default(), diff --git a/benches/native_vps.rs b/benches/native_vps.rs index a4829c4601..6959dfafe4 100644 --- a/benches/native_vps.rs +++ b/benches/native_vps.rs @@ -126,7 +126,7 @@ fn governance(c: &mut Criterion) { shell.generate_tx( TX_INIT_PROPOSAL_WASM, InitProposalData { - id: None, + id: 0, content: content_section.get_hash(), author: defaults::albert_address(), r#type: ProposalType::Default(None), @@ -178,7 +178,7 @@ fn governance(c: &mut Criterion) { shell.generate_tx( TX_INIT_PROPOSAL_WASM, InitProposalData { - id: Some(1), + id: 1, content: content_section.get_hash(), author: defaults::albert_address(), r#type: ProposalType::Default(Some( @@ -250,7 +250,7 @@ fn governance(c: &mut Criterion) { // let governance_proposal = shell.generate_tx( // TX_INIT_PROPOSAL_WASM, // InitProposalData { -// id: None, +// id: 0, // content: content_section.get_hash(), // author: defaults::albert_address(), // r#type: ProposalType::Default(None), diff --git a/benches/txs.rs b/benches/txs.rs index d7290852bb..2e32fed2ef 100644 --- a/benches/txs.rs +++ b/benches/txs.rs @@ -462,7 +462,7 @@ fn init_proposal(c: &mut Criterion) { shell.generate_tx( TX_INIT_PROPOSAL_WASM, InitProposalData { - id: None, + id: 0, content: content_section.get_hash(), author: defaults::albert_address(), r#type: ProposalType::Default(None), @@ -512,7 +512,7 @@ fn init_proposal(c: &mut Criterion) { shell.generate_tx( TX_INIT_PROPOSAL_WASM, InitProposalData { - id: Some(1), + id: 1, content: content_section.get_hash(), author: defaults::albert_address(), r#type: ProposalType::Default(Some( diff --git a/core/src/ledger/governance/cli/onchain.rs b/core/src/ledger/governance/cli/onchain.rs index 976557c913..14c93a068d 100644 --- a/core/src/ledger/governance/cli/onchain.rs +++ b/core/src/ledger/governance/cli/onchain.rs @@ -20,7 +20,7 @@ use crate::types::storage::Epoch; /// The proposal structure pub struct OnChainProposal { /// The proposal id - pub id: Option, + pub id: u64, /// The proposal content pub content: BTreeMap, /// The proposal author address diff --git a/core/src/ledger/storage_api/governance.rs b/core/src/ledger/storage_api/governance.rs index 4daf731fb4..592734eb89 100644 --- a/core/src/ledger/storage_api/governance.rs +++ b/core/src/ledger/storage_api/governance.rs @@ -31,11 +31,10 @@ where S: StorageRead + StorageWrite, { let counter_key = governance_keys::get_counter_key(); - let proposal_id = if let Some(id) = data.id { - id - } else { - storage.read(&counter_key)?.unwrap() - }; + let proposal_id = storage.read(&counter_key)?.expect( + "Storage should have been initialized with an initial governance \ + proposal id", + ); let content_key = governance_keys::get_content_key(proposal_id); storage.write_bytes(&content_key, content)?; diff --git a/core/src/types/transaction/governance.rs b/core/src/types/transaction/governance.rs index 0e7f41179b..ce41734192 100644 --- a/core/src/types/transaction/governance.rs +++ b/core/src/types/transaction/governance.rs @@ -34,7 +34,7 @@ pub enum ProposalError { )] pub struct InitProposalData { /// The proposal id - pub id: Option, + pub id: u64, /// The proposal content pub content: Hash, /// The proposal author address @@ -180,7 +180,7 @@ pub mod tests { prop_compose! { /// Generate a proposal initialization pub fn arb_init_proposal()( - id: Option, + id: u64, content in arb_hash(), author in arb_non_internal_address(), r#type in arb_proposal_type(), diff --git a/light_sdk/src/transaction/governance.rs b/light_sdk/src/transaction/governance.rs index c09f0fc8c1..a3a37a6fd2 100644 --- a/light_sdk/src/transaction/governance.rs +++ b/light_sdk/src/transaction/governance.rs @@ -19,7 +19,7 @@ impl InitProposal { /// Build a raw InitProposal transaction from the given parameters #[allow(clippy::too_many_arguments)] pub fn new( - id: Option, + id: u64, content: Hash, author: Address, r#type: ProposalType, diff --git a/sdk/src/signing.rs b/sdk/src/signing.rs index e52eb668b4..87488cf4a2 100644 --- a/sdk/src/signing.rs +++ b/sdk/src/signing.rs @@ -1086,9 +1086,7 @@ pub async fn to_ledger_vector( .hash(); tv.output.push("Type : Init proposal".to_string()); - if let Some(id) = init_proposal_data.id.as_ref() { - tv.output.push(format!("ID : {}", id)); - } + tv.output.push(format!("ID : {}", init_proposal_data.id)); tv.output.extend(vec![ format!( "Proposal type : {}", @@ -1107,9 +1105,8 @@ pub async fn to_ledger_vector( format!("Content : {}", HEXLOWER.encode(&extra.0)), ]); - if let Some(id) = init_proposal_data.id.as_ref() { - tv.output_expert.push(format!("ID : {}", id)); - } + tv.output_expert + .push(format!("ID : {}", init_proposal_data.id)); tv.output_expert.extend(vec![ format!( "Proposal type : {}", From 06e61724cc9dd19e25e36f5ef1d1e2e5131f74c6 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 5 Jan 2024 15:07:54 +0000 Subject: [PATCH 09/11] Fix docstr errs --- core/src/types/dec.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/src/types/dec.rs b/core/src/types/dec.rs index 4ec4e223ef..878ac57914 100644 --- a/core/src/types/dec.rs +++ b/core/src/types/dec.rs @@ -67,9 +67,9 @@ impl Dec { /// the division is impossible (e.g., division by zero or overflow), `None` /// is returned. /// - /// Example: - /// ``` - /// + /// For instance: + /// + /// ```ignore /// let x = Dec::new(3, 1).unwrap(); // Represents 0.3 /// let y = Dec::new(2, 1).unwrap(); // Represents 0.2 /// let result = x.trunc_div(&y).unwrap(); @@ -77,9 +77,11 @@ impl Dec { /// ``` /// /// # Arguments + /// /// * `rhs`: The right-hand side `Dec` value for the division. /// /// # Returns + /// /// An `Option` which is `Some` with the result if the division is /// successful, or `None` if the division cannot be performed. pub fn trunc_div(&self, rhs: &Self) -> Option { From 72da2ed955c9305db278e4a4ec45ee47edd68699 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 9 Jan 2024 12:55:50 +0100 Subject: [PATCH 10/11] `prepare_proposal_data` test helper requires proposal id --- tests/src/e2e/ledger_tests.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/src/e2e/ledger_tests.rs b/tests/src/e2e/ledger_tests.rs index 60da7a9b3e..4184c6de7e 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -1811,6 +1811,7 @@ fn proposal_submission() -> Result<()> { let albert = find_address(&test, ALBERT)?; let valid_proposal_json_path = prepare_proposal_data( &test, + 0, albert, TestWasms::TxProposalCode.read_bytes(), 12, @@ -1883,6 +1884,7 @@ fn proposal_submission() -> Result<()> { let albert = find_address(&test, ALBERT)?; let invalid_proposal_json = prepare_proposal_data( &test, + 1, albert, TestWasms::TxProposalCode.read_bytes(), 1, @@ -2141,7 +2143,7 @@ fn pgf_governance_proposal() -> Result<()> { }; let valid_proposal_json_path = - prepare_proposal_data(&test, albert, pgf_stewards, 12); + prepare_proposal_data(&test, 0, albert, pgf_stewards, 12); let validator_one_rpc = get_actor_rpc(&test, Who::Validator(0)); let submit_proposal_args = vec![ @@ -2330,7 +2332,7 @@ fn pgf_governance_proposal() -> Result<()> { }; let valid_proposal_json_path = - prepare_proposal_data(&test, albert, pgf_funding, 36); + prepare_proposal_data(&test, 1, albert, pgf_funding, 36); let validator_one_rpc = get_actor_rpc(&test, Who::Validator(0)); let submit_proposal_args = vec![ @@ -2926,6 +2928,7 @@ fn implicit_account_reveal_pk() -> Result<()> { let author = find_address(&test, source).unwrap(); let valid_proposal_json_path = prepare_proposal_data( &test, + 0, author, TestWasms::TxProposalCode.read_bytes(), 12, @@ -3042,12 +3045,14 @@ fn test_epoch_sleep() -> Result<()> { /// This can be submitted with "init-proposal" command. fn prepare_proposal_data( test: &setup::Test, + id: u64, source: Address, data: impl serde::Serialize, start_epoch: u64, ) -> PathBuf { let valid_proposal_json = json!({ "proposal": { + "id": id, "content": { "title": "TheTitle", "authors": "test@test.com", From a4fa3181e595fcbd0d032a528df42cf4c161b18f Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 9 Jan 2024 12:59:28 +0100 Subject: [PATCH 11/11] Changelog #2365 --- .changelog/unreleased/improvements/2365-required-proposal-id.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/2365-required-proposal-id.md diff --git a/.changelog/unreleased/improvements/2365-required-proposal-id.md b/.changelog/unreleased/improvements/2365-required-proposal-id.md new file mode 100644 index 0000000000..4debed4491 --- /dev/null +++ b/.changelog/unreleased/improvements/2365-required-proposal-id.md @@ -0,0 +1,2 @@ +- When constructing a governance proposal the id is now a required field. + ([\#2365](https://github.com/anoma/namada/pull/2365)) \ No newline at end of file