impl and test Remove for cPGF

Author: brentstone

crates/governance/src/cli/onchain.rs

@@ -289,6 +289,15 @@ impl PgfAction {
     }
 }
 
+impl Display for PgfAction {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            PgfAction::Add => write!(f, "Add"),
+            PgfAction::Remove => write!(f, "Remove"),
+        }
+    }
+}
+
 /// PGF funding
 #[derive(
     Debug,
@@ -301,7 +310,7 @@ impl PgfAction {
 )]
 pub struct PgfFunding {
     /// PGF continuous funding
-    pub continuous: Vec<ContPGFTarget>,
+    pub continuous: Vec<PgfContinuous>,
     /// PGF retro fundings
     pub retro: Vec<PGFTarget>,
 }
@@ -341,6 +350,12 @@ pub struct PgfContinuous {
     pub action: PgfAction,
 }
 
+impl Display for PgfContinuous {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "\n{}:\n{}", &self.action, &self.target)
+    }
+}
+
 /// PGF retro funding
 #[derive(
     Debug,

crates/governance/src/pgf/storage/keys.rs

@@ -61,7 +61,8 @@ pub fn fundings_key_prefix() -> Key {
     }
 }
 
-/// LazyMap handler for the pgf fundings substorage
+/// Nested LazyMap handler for the continuous PGF substorage. Structure: Address
+/// -> Proposal ID -> Target data
 pub fn fundings_handle()
 -> lazy_map::NestedMap<String, LazyMap<u64, ContPGFTarget>> {
     LazyMap::open(fundings_key_prefix())

crates/governance/src/storage/mod.rs

@@ -17,6 +17,7 @@ use namada_core::hash::Hash;
 use namada_core::token;
 use namada_state::{iter_prefix, Error, Result, StorageRead, StorageWrite};
 use namada_systems::trans_token;
+use proposal::{ContPGFTarget, PGFAction};
 
 use crate::parameters::GovernanceParameters;
 use crate::storage::keys as governance_keys;
@@ -52,7 +53,7 @@ where
     storage.write(&author_key, data.author.clone())?;
 
     let proposal_type_key = governance_keys::get_proposal_type_key(proposal_id);
-    match data.r#type {
+    match data.r#type.clone() {
         ProposalType::DefaultWithWasm(_) => {
             storage.write(&proposal_type_key, data.r#type.clone())?;
             let proposal_code_key =
@@ -62,6 +63,35 @@ where
                 code.ok_or(Error::new_const("Missing proposal code"))?;
             storage.write(&proposal_code_key, proposal_code)?;
         }
+        ProposalType::PGFPayment(a) => {
+            let transformed_data = a
+                .into_iter()
+                .map(|action| match action {
+                    proposal::PGFAction::Continuous(add_remove) => {
+                        let pgf_target = match add_remove {
+                            proposal::AddRemove::Add(c_tgt) => {
+                                proposal::AddRemove::Add(ContPGFTarget {
+                                    target: c_tgt.target,
+                                    end_epoch: c_tgt.end_epoch,
+                                    proposal_id,
+                                })
+                            }
+                            proposal::AddRemove::Remove(c) => {
+                                proposal::AddRemove::Remove(c)
+                            }
+                        };
+                        proposal::PGFAction::Continuous(pgf_target)
+                    }
+                    proposal::PGFAction::Retro(pgftarget) => {
+                        proposal::PGFAction::Retro(pgftarget)
+                    }
+                })
+                .collect::<BTreeSet<PGFAction>>();
+            storage.write(
+                &proposal_type_key,
+                ProposalType::PGFPayment(transformed_data),
+            )?;
+        }
         _ => storage.write(&proposal_type_key, data.r#type.clone())?,
     }
 

crates/governance/src/storage/proposal.rs

@@ -139,11 +139,12 @@ impl TryFrom<PgfFundingProposal> for InitProposalData {
             .continuous
             .iter()
             .cloned()
-            .map(|c_target| {
-                if c_target.target.amount().is_zero() {
-                    PGFAction::Continuous(AddRemove::Remove(c_target))
-                } else {
-                    PGFAction::Continuous(AddRemove::Add(c_target))
+            .map(|c_target| match c_target.action {
+                PgfAction::Add => {
+                    PGFAction::Continuous(AddRemove::Add(c_target.target))
+                }
+                PgfAction::Remove => {
+                    PGFAction::Continuous(AddRemove::Remove(c_target.target))
                 }
             })
             .collect::<BTreeSet<PGFAction>>();
@@ -284,6 +285,8 @@ pub struct ContPGFTarget {
     pub target: PGFTarget,
     /// The epoch at which the funding ends, if any
     pub end_epoch: Option<Epoch>,
+    /// The proposal ID that added this PGF payment
+    pub proposal_id: u64,
 }
 
 impl Display for ContPGFTarget {
@@ -746,10 +749,11 @@ pub mod testing {
         pub fn arb_cpgf_target()(
             target in arb_pgf_target(),
             end_epoch in arb_epoch_opt(),
+            proposal_id in 0..u64::MAX,
         ) -> ContPGFTarget {
             ContPGFTarget {
                 target,
-                end_epoch,
+                end_epoch,proposal_id
             }
         }
     }

crates/governance/src/vp/mod.rs

@@ -527,18 +527,9 @@ where
                     ));
                 }
 
-                // can't remove and add the same target in the same proposal
-                let are_targets_unique = are_continuous_add_targets_unique
-                    .intersection(&are_continuous_remove_targets_unique)
-                    .count() as u64
-                    == 0;
-
-                are_targets_unique.ok_or_else(|| {
-                    Error::new_const(
-                        "One or more payment targets were added and removed \
-                         in the same proposal",
-                    )
-                })
+                // Should we add a check to the VP for the new data format?
+
+                Ok(())
             }
             // Default proposal condition are checked already for all other
             // proposals.

crates/sdk/src/signing.rs

@@ -998,10 +998,12 @@ fn proposal_type_to_ledger_vector(
                     PGFAction::Continuous(AddRemove::Add(ContPGFTarget {
                         target: PGFTarget::Internal(target),
                         end_epoch,
+                        proposal_id,
                     })) => {
                         output.push(
                             "PGF Action : Add Continuous Payment".to_string(),
                         );
+                        output.push(format!("Proposal ID: {}", proposal_id));
                         output.push(format!("Target: {}", target.target));
                         output.push(format!(
                             "Amount: NAM {}",
@@ -1021,10 +1023,12 @@ fn proposal_type_to_ledger_vector(
                     PGFAction::Continuous(AddRemove::Add(ContPGFTarget {
                         target: PGFTarget::Ibc(target),
                         end_epoch,
+                        proposal_id,
                     })) => {
                         output.push(
                             "PGF Action : Add Continuous Payment".to_string(),
                         );
+                        output.push(format!("Proposal ID: {}", proposal_id));
                         output.push(format!("Target: {}", target.target));
                         output.push(format!(
                             "Amount: NAM {}",
@@ -1048,12 +1052,14 @@ fn proposal_type_to_ledger_vector(
                         ContPGFTarget {
                             target: PGFTarget::Internal(target),
                             end_epoch,
+                            proposal_id,
                         },
                     )) => {
                         output.push(
                             "PGF Action : Remove Continuous Payment"
                                 .to_string(),
                         );
+                        output.push(format!("Proposal ID: {}", proposal_id));
                         output.push(format!("Target: {}", target.target));
                         output.push(format!(
                             "Amount: NAM {}",
@@ -1074,12 +1080,14 @@ fn proposal_type_to_ledger_vector(
                         ContPGFTarget {
                             target: PGFTarget::Ibc(target),
                             end_epoch,
+                            proposal_id,
                         },
                     )) => {
                         output.push(
                             "PGF Action : Remove Continuous Payment"
                                 .to_string(),
                         );
+                        output.push(format!("Proposal ID: {}", proposal_id));
                         output.push(format!("Target: {}", target.target));
                         output.push(format!(
                             "Amount: NAM {}",
@@ -3086,6 +3094,7 @@ mod test_signing {
                         amount: Amount::zero(),
                     }),
                     end_epoch: Some(Epoch::from(1)),
+                    proposal_id: 0,
                 }), // TODO: ask Murisi if this is ok
             )])),
             &tx,
@@ -3111,6 +3120,7 @@ mod test_signing {
                         amount: Amount::zero(),
                     }),
                     end_epoch: Some(Epoch::from(1)),
+                    proposal_id: 0,
                 }), // TODO: ask Murisi if this is ok
             )])),
             &tx,
@@ -3161,6 +3171,7 @@ mod test_signing {
                         channel_id: ChannelId::new(16),
                     }),
                     end_epoch: None,
+                    proposal_id: 0,
                 }), // TODO: ask Murisi if this is ok
             )])),
             &tx,
@@ -3191,6 +3202,7 @@ mod test_signing {
                         channel_id: ChannelId::new(16),
                     }),
                     end_epoch: None,
+                    proposal_id: 0,
                 }), // TODO: ask Murisi if this is ok
             )])),
             &tx,

crates/tests/src/e2e/ibc_tests.rs

@@ -26,7 +26,9 @@ use namada_apps_lib::tendermint_rpc::{Client, HttpClient, Url};
 use namada_core::masp::PaymentAddress;
 use namada_sdk::address::MASP;
 use namada_sdk::chain::Epoch;
-use namada_sdk::governance::cli::onchain::PgfFunding;
+use namada_sdk::governance::cli::onchain::{
+    PgfAction, PgfContinuous, PgfFunding,
+};
 use namada_sdk::governance::pgf::ADDRESS as PGF_ADDRESS;
 use namada_sdk::governance::storage::proposal::{PGFIbcTarget, PGFTarget};
 use namada_sdk::ibc::apps::nft_transfer::types::{
@@ -2661,14 +2663,18 @@ fn propose_funding(
     src_channel_id: &ChannelId,
 ) -> Result<Epoch> {
     let pgf_funding = PgfFunding {
-        continuous: vec![ContPGFTarget {
-            target: PGFTarget::Ibc(PGFIbcTarget {
-                amount: Amount::native_whole(10),
-                target: continuous_receiver.as_ref().to_string(),
-                port_id: src_port_id.clone(),
-                channel_id: src_channel_id.clone(),
-            }),
-            end_epoch: None,
+        continuous: vec![PgfContinuous {
+            target: ContPGFTarget {
+                target: PGFTarget::Ibc(PGFIbcTarget {
+                    amount: Amount::native_whole(10),
+                    target: continuous_receiver.as_ref().to_string(),
+                    port_id: src_port_id.clone(),
+                    channel_id: src_channel_id.clone(),
+                }),
+                end_epoch: None,
+                proposal_id: 0,
+            },
+            action: PgfAction::Add,
         }],
         retro: vec![PGFTarget::Ibc(PGFIbcTarget {
             amount: Amount::native_whole(5),