Merge pull request #3804 from anoma/grarco/fix-transfer-dest-vp-trigger

Fix transfer triggered vps

.changelog/unreleased/bug-fixes/3804-fix-transfer-dest-vp-trigger.md

@@ -0,0 +1,3 @@
+- The multitoken vp now checks that the involved parties
+  validate the transaction. Improved tests and transfer code.
+  ([\#3804](https://github.com/anoma/namada/pull/3804))

clippy.toml

@@ -8,6 +8,7 @@ disallowed-methods = [
   { path = "chrono::Utc::now", reason = "Do not use current date/time in code that must be deterministic" },
   { path = "namada_core::time::DateTimeUtc::now", reason = "Do not use current date/time in code that must be deterministic" },
   { path = "wasmtimer::std::Instant", reason = "Do not use current date/time in code that must be deterministic" },
+  { path = "std::result::Result::unwrap_or_default", reason = "Do not silently ignore an error that could be caused by gas" },
 ]
 allow-dbg-in-tests = true
 allow-print-in-tests = true

crates/apps_lib/src/client/utils.rs

@@ -991,6 +991,7 @@ pub async fn sign_genesis_tx(
         if let Some(output_path) = output.as_ref() {
             // If the output path contains existing signed txs, we append
             // the newly signed txs to the file
+            #[allow(clippy::disallowed_methods)]
             let mut prev_txs =
                 genesis::templates::read_transactions(output_path)
                     .unwrap_or_default();

crates/governance/src/finalize_block.rs

@@ -324,6 +324,7 @@ where
         if vote.is_validator() {
             let vote_data = vote.data.clone();
 
+            #[allow(clippy::disallowed_methods)]
             let validator_stake = PoS::read_validator_stake::<crate::Store<_>>(
                 storage, validator, epoch,
             )

crates/governance/src/utils.rs

@@ -221,6 +221,7 @@ impl ProposalResult {
     /// Return true if at least 2/3 of the total voting power voted and at least
     /// two third of the non-abstained voting power voted nay.
     /// Returns `false` if any arithmetic fails.
+    #[allow(clippy::disallowed_methods)]
     pub fn two_thirds_nay_over_two_thirds_total(&self) -> bool {
         (|| {
             let two_thirds_power =

crates/governance/src/vp/pgf.rs

@@ -119,15 +119,10 @@ where
             match key_type {
                 KeyType::Stewards(steward_address) => {
                     let stewards_have_increased = {
-                        // TODO(namada#3238): we should check errors here, which
-                        // could be out-of-gas related
-                        let total_stewards_pre = pgf_storage::stewards_handle()
-                            .len(&ctx.pre())
-                            .unwrap_or_default();
+                        let total_stewards_pre =
+                            pgf_storage::stewards_handle().len(&ctx.pre())?;
                         let total_stewards_post =
-                            pgf_storage::stewards_handle()
-                                .len(&ctx.post())
-                                .unwrap_or_default();
+                            pgf_storage::stewards_handle().len(&ctx.post())?;
 
                         total_stewards_pre < total_stewards_post
                     };

crates/ibc/src/lib.rs

@@ -32,6 +32,7 @@ use std::collections::BTreeSet;
 use std::fmt::Debug;
 use std::marker::PhantomData;
 use std::rc::Rc;
+use std::str::FromStr;
 
 pub use actions::transfer_over_ibc;
 use apps::transfer::types::packet::PacketData;
@@ -54,13 +55,16 @@ use ibc::apps::nft_transfer::types::msgs::transfer::MsgTransfer as IbcMsgNftTran
 use ibc::apps::nft_transfer::types::{
     ack_success_b64, is_receiver_chain_source as is_nft_receiver_chain_source,
     PrefixedClassId, TokenId, TracePrefix as NftTracePrefix,
+    PORT_ID_STR as NFT_PORT_ID_STR,
 };
 use ibc::apps::transfer::handler::{
     send_transfer_execute, send_transfer_validate,
 };
 use ibc::apps::transfer::types::error::TokenTransferError;
 use ibc::apps::transfer::types::msgs::transfer::MsgTransfer as IbcMsgTransfer;
-use ibc::apps::transfer::types::{is_receiver_chain_source, TracePrefix};
+use ibc::apps::transfer::types::{
+    is_receiver_chain_source, TracePrefix, PORT_ID_STR as FT_PORT_ID_STR,
+};
 use ibc::core::channel::types::acknowledgement::AcknowledgementStatus;
 use ibc::core::channel::types::commitment::compute_ack_commitment;
 use ibc::core::channel::types::msgs::{
@@ -139,6 +143,8 @@ pub enum Error {
     ChainId(IdentifierError),
     #[error("Verifier insertion error: {0}")]
     Verifier(StorageError),
+    #[error("IBC error: {0}")]
+    Other(String),
 }
 
 impl From<Error> for StorageError {
@@ -612,6 +618,18 @@ where
                     self.ctx.inner.clone(),
                     self.verifiers.clone(),
                 );
+                // Add the source to the set of verifiers
+                self.verifiers.borrow_mut().insert(
+                    Address::from_str(msg.message.packet_data.sender.as_ref())
+                        .map_err(|_| {
+                            Error::TokenTransfer(TokenTransferError::Other(
+                                format!(
+                                    "Cannot convert the sender address {}",
+                                    msg.message.packet_data.sender
+                                ),
+                            ))
+                        })?,
+                );
                 self.insert_verifiers()?;
                 if msg.transfer.is_some() {
                     token_transfer_ctx.enable_shielded_transfer();
@@ -630,6 +648,19 @@ where
                 if msg.transfer.is_some() {
                     nft_transfer_ctx.enable_shielded_transfer();
                 }
+                // Add the source to the set of verifiers
+                self.verifiers.borrow_mut().insert(
+                    Address::from_str(msg.message.packet_data.sender.as_ref())
+                        .map_err(|_| {
+                            Error::NftTransfer(NftTransferError::Other(
+                                format!(
+                                    "Cannot convert the sender address {}",
+                                    msg.message.packet_data.sender
+                                ),
+                            ))
+                        })?,
+                );
+                self.insert_verifiers()?;
                 send_nft_transfer_execute(
                     &mut self.ctx,
                     &mut nft_transfer_ctx,
@@ -639,8 +670,21 @@ where
                 Ok((msg.transfer, None))
             }
             IbcMessage::Envelope(envelope) => {
+                if let Some(verifier) = get_envelope_verifier(envelope.as_ref())
+                {
+                    self.verifiers.borrow_mut().insert(
+                        Address::from_str(verifier.as_ref()).map_err(|_| {
+                            Error::Other(format!(
+                                "Cannot convert the address {}",
+                                verifier,
+                            ))
+                        })?,
+                    );
+                    self.insert_verifiers()?;
+                }
                 execute(&mut self.ctx, &mut self.router, *envelope.clone())
                     .map_err(|e| Error::Context(Box::new(e)))?;
+
                 // Extract MASP tx from the memo in the packet if needed
                 let masp_tx = match &*envelope {
                     MsgEnvelope::Packet(PacketMsg::Recv(msg))
@@ -734,6 +778,70 @@ where
     }
 }
 
+// Extract the involved namada address from the packet (either sender or
+// receiver) to trigger its vp. Returns None if an address could not be found
+fn get_envelope_verifier(
+    envelope: &MsgEnvelope,
+) -> Option<ibc::primitives::Signer> {
+    match envelope {
+        MsgEnvelope::Packet(PacketMsg::Recv(msg)) => {
+            match msg.packet.port_id_on_b.as_str() {
+                FT_PORT_ID_STR => {
+                    serde_json::from_slice::<PacketData>(&msg.packet.data)
+                        .ok()
+                        .map(|packet_data| packet_data.receiver)
+                }
+                NFT_PORT_ID_STR => {
+                    serde_json::from_slice::<NftPacketData>(&msg.packet.data)
+                        .ok()
+                        .map(|packet_data| packet_data.receiver)
+                }
+                _ => None,
+            }
+        }
+        MsgEnvelope::Packet(PacketMsg::Ack(msg)) => serde_json::from_slice::<
+            AcknowledgementStatus,
+        >(
+            msg.acknowledgement.as_ref(),
+        )
+        .map_or(None, |ack| {
+            if ack.is_successful() {
+                None
+            } else {
+                match msg.packet.port_id_on_a.as_str() {
+                    FT_PORT_ID_STR => {
+                        serde_json::from_slice::<PacketData>(&msg.packet.data)
+                            .ok()
+                            .map(|packet_data| packet_data.sender)
+                    }
+                    NFT_PORT_ID_STR => serde_json::from_slice::<NftPacketData>(
+                        &msg.packet.data,
+                    )
+                    .ok()
+                    .map(|packet_data| packet_data.sender),
+                    _ => None,
+                }
+            }
+        }),
+        MsgEnvelope::Packet(PacketMsg::Timeout(msg)) => {
+            match msg.packet.port_id_on_a.as_str() {
+                FT_PORT_ID_STR => {
+                    serde_json::from_slice::<PacketData>(&msg.packet.data)
+                        .ok()
+                        .map(|packet_data| packet_data.sender)
+                }
+                NFT_PORT_ID_STR => {
+                    serde_json::from_slice::<NftPacketData>(&msg.packet.data)
+                        .ok()
+                        .map(|packet_data| packet_data.sender)
+                }
+                _ => None,
+            }
+        }
+        _ => None,
+    }
+}
+
 /// Tries to decode transaction data to an `IbcMessage`
 pub fn decode_message<Transfer: BorshDeserialize>(
     tx_data: &[u8],

crates/node/src/shell/mod.rs

@@ -633,6 +633,7 @@ where
         };
 
         if let Some(schedule_migration) = scheduled_migration.as_ref() {
+            #[allow(clippy::disallowed_methods)]
             let current = state.get_block_height().unwrap_or_default();
             if schedule_migration.height < current {
                 panic!(

crates/node/src/shell/snapshots.rs

@@ -112,6 +112,7 @@ impl Shell<storage::PersistentDB, Sha256Hasher> {
         }
         match self.syncing.as_ref() {
             None => {
+                #[allow(clippy::disallowed_methods)]
                 if self.state.get_block_height().unwrap_or_default().0
                     < u64::from(req.snapshot.height)
                 {

crates/node/src/shell/testing/node.rs

@@ -361,6 +361,7 @@ impl MockNode {
     }
 
     pub fn block_height(&self) -> BlockHeight {
+        #[allow(clippy::disallowed_methods)]
         self.shell
             .lock()
             .unwrap()

crates/node/src/shims/abcipp_shim_types.rs

@@ -223,6 +223,7 @@ pub mod shim {
                 let header = req.header;
                 FinalizeBlock {
                     header: BlockHeader {
+                        #[allow(clippy::disallowed_methods)]
                         hash: Hash::try_from(header.app_hash.as_bytes())
                             .unwrap_or_default(),
                         time: DateTimeUtc::try_from(header.time).unwrap(),

crates/proof_of_stake/src/rewards.rs

@@ -261,12 +261,14 @@ where
 
         // Ensure TM stake updates properly with a debug_assert
         if cfg!(debug_assertions) {
+            #[allow(clippy::disallowed_methods)]
+            let validator_vp = i64::try_from(validator_vp).unwrap_or_default();
             debug_assert_eq!(
                 into_tm_voting_power(
                     params.tm_votes_per_token,
                     stake_from_deltas,
                 ),
-                i64::try_from(validator_vp).unwrap_or_default(),
+                validator_vp
             );
         }
 

crates/proof_of_stake/src/tests/test_pos.rs

@@ -1159,6 +1159,7 @@ fn test_unslashed_bond_amount_aux(validators: Vec<GenesisValidator>) {
         Epoch(0),
         current_epoch + params.pipeline_len,
     ) {
+        #[allow(clippy::disallowed_methods)]
         let amount = bond_amount(
             &storage,
             &BondId {

crates/proof_of_stake/src/tests/test_slash_and_redel.rs

@@ -1638,6 +1638,7 @@ fn test_slashed_bond_amount_aux(validators: Vec<GenesisValidator>) {
 
     let pipeline_epoch = current_epoch + params.pipeline_len;
 
+    #[allow(clippy::disallowed_methods)]
     let del_bond_amount = bond_amount(
         &storage,
         &BondId {
@@ -1648,6 +1649,7 @@ fn test_slashed_bond_amount_aux(validators: Vec<GenesisValidator>) {
     )
     .unwrap_or_default();
 
+    #[allow(clippy::disallowed_methods)]
     let self_bond_amount = bond_amount(
         &storage,
         &BondId {

crates/sdk/src/lib.rs

@@ -82,7 +82,7 @@ pub use {namada_io as io, namada_wallet as wallet};
 use crate::masp::ShieldedContext;
 
 /// Default gas-limit
-pub const DEFAULT_GAS_LIMIT: u64 = 200_000;
+pub const DEFAULT_GAS_LIMIT: u64 = 250_000;
 
 /// An interface for high-level interaction with the Namada SDK
 #[cfg_attr(feature = "async-send", async_trait::async_trait)]

crates/sdk/src/queries/vp/token.rs

@@ -101,6 +101,7 @@ pub mod client_only_methods {
             let balance = if response.data.is_empty() {
                 token::Amount::zero()
             } else {
+                #[allow(clippy::disallowed_methods)]
                 token::Amount::try_from_slice(&response.data)
                     .unwrap_or_default()
             };
@@ -124,6 +125,7 @@ pub mod client_only_methods {
             let tokens = if response.data.is_empty() {
                 token::Amount::zero()
             } else {
+                #[allow(clippy::disallowed_methods)]
                 token::Amount::try_from_slice(&response.data)
                     .unwrap_or_default()
             };

crates/sdk/src/rpc.rs

@@ -994,10 +994,12 @@ pub async fn query_proposal_result<C: namada_io::Client + Sync>(
 
             let is_author_pgf_steward =
                 is_steward(client, &proposal.author).await;
+            #[allow(clippy::disallowed_methods)]
             let votes = query_proposal_votes(client, proposal_id)
                 .await
                 .unwrap_or_default();
             let tally_type = proposal.get_tally_type(is_author_pgf_steward);
+            #[allow(clippy::disallowed_methods)]
             let total_active_voting_power =
                 get_total_active_voting_power(client, tally_epoch)
                     .await
@@ -1008,6 +1010,7 @@ pub async fn query_proposal_result<C: namada_io::Client + Sync>(
             for vote in votes {
                 match vote.is_validator() {
                     true => {
+                        #[allow(clippy::disallowed_methods)]
                         let voting_power = get_validator_stake(
                             client,
                             tally_epoch,
@@ -1023,6 +1026,7 @@ pub async fn query_proposal_result<C: namada_io::Client + Sync>(
                         );
                     }
                     false => {
+                        #[allow(clippy::disallowed_methods)]
                         let voting_power = get_bond_amount_at(
                             client,
                             &vote.delegator,

crates/sdk/src/signing.rs

@@ -483,6 +483,7 @@ pub async fn validate_transparent_fee<N: Namada>(
     let fee_payer_address = Address::from(fee_payer);
 
     let balance_key = balance_key(&args.fee_token, &fee_payer_address);
+    #[allow(clippy::disallowed_methods)]
     let balance = rpc::query_storage_value::<_, token::Amount>(
         context.client(),
         &balance_key,

crates/sdk/src/tx.rs

@@ -3151,6 +3151,7 @@ async fn get_masp_fee_payment_amount<N: Namada>(
 ) -> Result<Option<MaspFeeData>> {
     let fee_payer_address = Address::from(fee_payer);
     let balance_key = balance_key(&args.fee_token, &fee_payer_address);
+    #[allow(clippy::disallowed_methods)]
     let balance = rpc::query_storage_value::<_, token::Amount>(
         context.client(),
         &balance_key,
@@ -3492,6 +3493,7 @@ async fn construct_shielded_parts<N: Namada>(
 
     // Get the decoded asset types used in the transaction to give offline
     // wallet users more information
+    #[allow(clippy::disallowed_methods)]
     let asset_types = used_asset_types(context, &shielded_parts.builder)
         .await
         .unwrap_or_default();