Merge branch 'fraccaman/pgf-minors' (#2440)

* origin/fraccaman/pgf-minors:
  added changelog
  added reward distribution limit for stewards, minors

.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))

.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
-}
+}

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);

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;

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,13 +26,18 @@ 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() {
                 return false;
             }
             sum += percentage;
         }
+
         sum <= Dec::one()
     }
 }

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

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,18 +2633,18 @@ fn proposal_offline() -> Result<()> {
     Ok(())
 }
 
-fn generate_proposal_json_file(
-    proposal_path: &std::path::Path,
-    proposal_content: &serde_json::Value,
-) {
+fn write_json_file<T>(proposal_path: &std::path::Path, proposal_content: T)
+where
+    T: Serialize,
+{
     let intent_writer = std::fs::OpenOptions::new()
         .create(true)
         .write(true)
         .truncate(true)
         .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;