diff --git a/.changelog/unreleased/bug-fixes/2440-pgf-minors.md b/.changelog/unreleased/bug-fixes/2440-pgf-minors.md new file mode 100644 index 0000000000..3a0d76b7ba --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2440-pgf-minors.md @@ -0,0 +1,2 @@ +- Restrict the reward distribution of a steward to a maximum of 100. + ([\#2440](https://github.com/anoma/namada/pull/2440)) \ No newline at end of file diff --git a/.github/workflows/scripts/e2e.json b/.github/workflows/scripts/e2e.json index 95d689644c..d9b43599d9 100644 --- a/.github/workflows/scripts/e2e.json +++ b/.github/workflows/scripts/e2e.json @@ -16,6 +16,7 @@ "e2e::ledger_tests::pgf_governance_proposal": 320, "e2e::ledger_tests::proposal_submission": 200, "e2e::ledger_tests::proposal_change_shielded_reward": 200, + "e2e::pgf_steward_change_commissions": 30, "e2e::ledger_tests::run_ledger": 5, "e2e::ledger_tests::run_ledger_load_state_and_reset": 23, "e2e::ledger_tests::test_namada_shuts_down_if_tendermint_dies": 2, @@ -35,4 +36,4 @@ "e2e::wallet_tests::wallet_encrypted_key_cmds": 1, "e2e::wallet_tests::wallet_encrypted_key_cmds_env_var": 1, "e2e::wallet_tests::wallet_unencrypted_key_cmds": 1 -} +} \ No newline at end of file diff --git a/crates/governance/src/pgf/cli/steward.rs b/crates/governance/src/pgf/cli/steward.rs index dd411ac190..6cfbf61b26 100644 --- a/crates/governance/src/pgf/cli/steward.rs +++ b/crates/governance/src/pgf/cli/steward.rs @@ -4,6 +4,8 @@ use namada_core::types::address::Address; use namada_core::types::dec::Dec; use serde::{Deserialize, Serialize}; +use crate::pgf::REWARD_DISTRIBUTION_LIMIT; + #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] /// Struct holding data about a steward commission pub struct Commission { @@ -22,6 +24,10 @@ impl TryFrom<&[u8]> for Commission { impl Commission { /// Check if a steward commission is valid pub fn is_valid(&self) -> bool { + if self.reward_distribution.len() as u64 > REWARD_DISTRIBUTION_LIMIT { + return false; + } + let mut sum = Dec::zero(); for percentage in self.reward_distribution.values() { sum = sum.add(percentage); diff --git a/crates/governance/src/pgf/mod.rs b/crates/governance/src/pgf/mod.rs index a9398a75d9..77302b6ebd 100644 --- a/crates/governance/src/pgf/mod.rs +++ b/crates/governance/src/pgf/mod.rs @@ -13,3 +13,6 @@ pub mod storage; /// The Pgf internal address pub const ADDRESS: Address = Address::Internal(InternalAddress::Pgf); + +/// Upper limit on the number of reward distribution per steawrd +pub const REWARD_DISTRIBUTION_LIMIT: u64 = 100; diff --git a/crates/governance/src/pgf/storage/steward.rs b/crates/governance/src/pgf/storage/steward.rs index 2ed4b41111..ce6855a130 100644 --- a/crates/governance/src/pgf/storage/steward.rs +++ b/crates/governance/src/pgf/storage/steward.rs @@ -4,6 +4,8 @@ use borsh::{BorshDeserialize, BorshSerialize}; use namada_core::types::address::Address; use namada_core::types::dec::Dec; +use crate::pgf::REWARD_DISTRIBUTION_LIMIT; + #[derive(Clone, Debug, BorshSerialize, BorshDeserialize, PartialEq)] /// Struct holding data about a pgf steward pub struct StewardDetail { @@ -24,6 +26,10 @@ impl StewardDetail { /// Check if reward distribution is valid pub fn is_valid_reward_distribution(&self) -> bool { + if self.reward_distribution.len() as u64 > REWARD_DISTRIBUTION_LIMIT { + return false; + } + let mut sum = Dec::zero(); for percentage in self.reward_distribution.values().cloned() { if percentage < Dec::zero() || percentage > Dec::one() { @@ -31,6 +37,7 @@ impl StewardDetail { } sum += percentage; } + sum <= Dec::one() } } diff --git a/crates/namada/src/ledger/pgf/mod.rs b/crates/namada/src/ledger/pgf/mod.rs index 2244f9e6f0..eb7ec70a98 100644 --- a/crates/namada/src/ledger/pgf/mod.rs +++ b/crates/namada/src/ledger/pgf/mod.rs @@ -70,8 +70,10 @@ where let is_valid = if total_stewards_pre < total_stewards_post { false } else { - // if a steward resign or update commissions, check the - // for signature authorization + // if a steward resign, check the signature + // if a steward update the reward distribution (so + // total_stewards_pre == total_stewards_post) check + // signature and if commission are valid let steward_address = pgf_storage::is_stewards_key(key); if let Some(address) = steward_address { let steward_post = pgf::storage::get_steward( @@ -83,7 +85,10 @@ where steward.is_valid_reward_distribution() && verifiers.contains(address) } - _ => verifiers.contains(address), + Ok(None) => verifiers.contains(address), + // if reading from storage returns an error, + // just return false + Err(_) => false, } } else { false diff --git a/crates/tests/src/e2e/ledger_tests.rs b/crates/tests/src/e2e/ledger_tests.rs index f5cf2f496b..239461dc04 100644 --- a/crates/tests/src/e2e/ledger_tests.rs +++ b/crates/tests/src/e2e/ledger_tests.rs @@ -10,6 +10,7 @@ //! `NAMADA_E2E_KEEP_TEMP=true`. #![allow(clippy::type_complexity)] +use std::collections::HashMap; use std::path::PathBuf; use std::process::Command; use std::str::FromStr; @@ -29,9 +30,12 @@ use namada_apps::config::ethereum_bridge; use namada_apps::config::utils::convert_tm_addr_to_socket_addr; use namada_apps::facade::tendermint_config::net::Address as TendermintAddress; use namada_core::types::token::NATIVE_MAX_DECIMAL_PLACES; +use namada_sdk::governance::pgf::cli::steward::Commission; use namada_sdk::masp::fs::FsShieldedUtils; use namada_test_utils::TestWasms; +use namada_tx_prelude::dec::Dec; use namada_vp_prelude::BTreeSet; +use serde::Serialize; use serde_json::json; use setup::constants::*; use setup::Test; @@ -2086,13 +2090,6 @@ fn pgf_governance_proposal() -> Result<()> { Default::default(); genesis.parameters.parameters.min_num_of_blocks = 4; genesis.parameters.parameters.max_expected_time_per_block = 1; - genesis.parameters.pgf_params.stewards = - BTreeSet::from_iter([get_established_addr_from_pregenesis( - "albert-key", - base_dir, - &genesis, - ) - .unwrap()]); setup::set_validators(1, genesis, base_dir, |_| 0) }, None, @@ -2380,6 +2377,108 @@ fn pgf_governance_proposal() -> Result<()> { Ok(()) } +/// Test if a steward can correctly change his distribution reward +#[test] +fn pgf_steward_change_commissions() -> Result<()> { + let test = setup::network( + |mut genesis, base_dir: &_| { + genesis.parameters.parameters.epochs_per_year = + epochs_per_year_from_min_duration(1); + genesis.parameters.parameters.max_proposal_bytes = + Default::default(); + genesis.parameters.parameters.min_num_of_blocks = 4; + genesis.parameters.parameters.max_expected_time_per_block = 1; + genesis.parameters.pgf_params.stewards_inflation_rate = + Dec::from_str("0.1").unwrap(); + genesis.parameters.pgf_params.stewards = + BTreeSet::from_iter([get_established_addr_from_pregenesis( + "albert-key", + base_dir, + &genesis, + ) + .unwrap()]); + setup::set_validators(1, genesis, base_dir, |_| 0) + }, + None, + )?; + + set_ethereum_bridge_mode( + &test, + &test.net.chain_id, + Who::Validator(0), + ethereum_bridge::ledger::Mode::Off, + None, + ); + + let namadac_help = vec!["--help"]; + + let mut client = run!(test, Bin::Client, namadac_help, Some(40))?; + client.exp_string("Namada client command line interface.")?; + client.assert_success(); + + // Run the ledger node + let _bg_ledger = + start_namada_ledger_node_wait_wasm(&test, Some(0), Some(40))? + .background(); + + let validator_one_rpc = get_actor_rpc(&test, Who::Validator(0)); + + let albert = find_address(&test, ALBERT)?; + let bertha = find_address(&test, BERTHA)?; + let christel = find_address(&test, CHRISTEL)?; + + // Query pgf stewards + let query_pgf = vec!["query-pgf", "--node", &validator_one_rpc]; + + let mut client = run!(test, Bin::Client, query_pgf, Some(30))?; + client.exp_string("Pgf stewards:")?; + client.exp_string(&format!("- {}", albert))?; + client.exp_string("Reward distribution:")?; + client.exp_string(&format!("- 1 to {}", albert))?; + client.exp_string("Pgf fundings: no fundings are currently set.")?; + client.assert_success(); + + let commission = Commission { + reward_distribution: HashMap::from_iter([ + (albert.clone(), Dec::from_str("0.25").unwrap()), + (bertha.clone(), Dec::from_str("0.70").unwrap()), + (christel.clone(), Dec::from_str("0.05").unwrap()), + ]), + }; + + let commission_path = + prepare_steward_commission_update_data(&test, commission); + + // Update steward commissions + let tx_args = vec![ + "update-steward-rewards", + "--steward", + ALBERT, + "--data-path", + commission_path.to_str().unwrap(), + "--node", + &validator_one_rpc, + ]; + let mut client = run!(test, Bin::Client, tx_args, Some(40))?; + client.exp_string(TX_APPLIED_SUCCESS)?; + client.assert_success(); + + // 14. Query pgf stewards + let query_pgf = vec!["query-pgf", "--node", &validator_one_rpc]; + + let mut client = run!(test, Bin::Client, query_pgf, Some(30))?; + client.exp_string("Pgf stewards:")?; + client.exp_string(&format!("- {}", albert))?; + client.exp_string("Reward distribution:")?; + client.exp_string(&format!("- 0.25 to {}", albert))?; + client.exp_string(&format!("- 0.7 to {}", bertha))?; + client.exp_string(&format!("- 0.05 to {}", christel))?; + client.exp_string("Pgf fundings: no fundings are currently set.")?; + client.assert_success(); + + Ok(()) +} + /// In this test we: /// 1. Run the ledger node /// 2. Create an offline proposal @@ -2459,10 +2558,7 @@ fn proposal_offline() -> Result<()> { ); let valid_proposal_json_path = test.test_dir.path().join("valid_proposal.json"); - generate_proposal_json_file( - valid_proposal_json_path.as_path(), - &valid_proposal_json, - ); + write_json_file(valid_proposal_json_path.as_path(), &valid_proposal_json); let mut epoch = get_epoch(&test, &validator_one_rpc).unwrap(); while epoch.0 <= 3 { @@ -2537,10 +2633,10 @@ fn proposal_offline() -> Result<()> { Ok(()) } -fn generate_proposal_json_file( - proposal_path: &std::path::Path, - proposal_content: &serde_json::Value, -) { +fn write_json_file(proposal_path: &std::path::Path, proposal_content: T) +where + T: Serialize, +{ let intent_writer = std::fs::OpenOptions::new() .create(true) .write(true) @@ -2548,7 +2644,7 @@ fn generate_proposal_json_file( .open(proposal_path) .unwrap(); - serde_json::to_writer(intent_writer, proposal_content).unwrap(); + serde_json::to_writer(intent_writer, &proposal_content).unwrap(); } /// In this test we intentionally make a validator node double sign blocks @@ -3080,13 +3176,22 @@ pub fn prepare_proposal_data( let valid_proposal_json_path = test.test_dir.path().join("valid_proposal.json"); - generate_proposal_json_file( - valid_proposal_json_path.as_path(), - &valid_proposal_json, - ); + write_json_file(valid_proposal_json_path.as_path(), valid_proposal_json); valid_proposal_json_path } +/// Prepare steward commission reward in the test temp directory. +/// This can be submitted with "update-steward-commission" command. +pub fn prepare_steward_commission_update_data( + test: &setup::Test, + data: impl serde::Serialize, +) -> PathBuf { + let valid_commission_json_path = + test.test_dir.path().join("commission.json"); + write_json_file(valid_commission_json_path.as_path(), &data); + valid_commission_json_path +} + #[test] fn deactivate_and_reactivate_validator() -> Result<()> { let pipeline_len = 2;