Merge pull request #3611 from anoma/murisi/fix-masp-vp-mint-case

Murisi/fix masp vp mint case

.changelog/unreleased/bug-fixes/3611-fix-masp-vp-mint-case.md

@@ -0,0 +1,2 @@
+- Fix the behavior of the MASP VP when processing an IBC Receive message
+  involves unescrowing. ([\#3611](https://github.com/anoma/namada/pull/3611))

crates/ibc/src/lib.rs

@@ -101,7 +101,11 @@ pub use nft::*;
 use primitives::Timestamp;
 use prost::Message;
 use thiserror::Error;
-use trace::{convert_to_address, ibc_trace_for_nft, is_sender_chain_source};
+use trace::{
+    convert_to_address, ibc_trace_for_nft,
+    is_receiver_chain_source as is_receiver_chain_source_str,
+    is_sender_chain_source,
+};
 
 use crate::storage::{
     channel_counter_key, client_counter_key, connection_counter_key,
@@ -387,6 +391,7 @@ where
     Ok(())
 }
 
+// Check that the packet receipt key has been changed
 fn check_packet_receiving(
     msg: &IbcMsgRecvPacket,
     keys_changed: &BTreeSet<Key>,
@@ -398,7 +403,7 @@ fn check_packet_receiving(
     );
     if !keys_changed.contains(&receipt_key) {
         return Err(namada_storage::Error::new_alloc(format!(
-            "The packet has not been received: Port ID  {}, Channel ID {}, \
+            "The packet has not been received: Port ID {}, Channel ID {}, \
              Sequence {}",
             msg.packet.port_id_on_b,
             msg.packet.chan_id_on_b,
@@ -434,7 +439,7 @@ where
         let token = convert_to_address(ibc_trace).into_storage_result()?;
         let delta = ValueSum::from_pair(token, *amount);
         // If there is a transfer to the IBC account, then deduplicate the
-        // balance increase since we already accounted for it above
+        // balance increase since we already account for it below
         if is_sender_chain_source(ibc_trace, src_port_id, src_channel_id) {
             let ibc_taddr = addr_taddr(address::IBC);
             let post_entry = accum
@@ -475,7 +480,8 @@ where
     S: StorageRead,
 {
     // Ensure that the event corresponds to the current changes to storage
-    let ack_key = storage::ack_key(dst_port_id, dst_channel_id, sequence); // If the receive is a success, then the commitment is unique
+    let ack_key = storage::ack_key(dst_port_id, dst_channel_id, sequence);
+    // If the receive is a success, then the commitment is unique
     let succ_ack_commitment = compute_ack_commitment(
         &AcknowledgementStatus::success(ack_success_b64()).into(),
     );
@@ -501,8 +507,8 @@ fn apply_recv_msg<S>(
 where
     S: StorageRead,
 {
+    // First check that the packet receipt is reflecteed in the state changes
     check_packet_receiving(msg, keys_changed)?;
-
     // If the transfer was a failure, then enable funds to
     // be withdrawn from the IBC internal address
     if is_receiving_success(
@@ -512,31 +518,41 @@ where
         msg.packet.seq_on_a,
     )? {
         for ibc_trace in ibc_traces {
-            // Get the received token
-            let token = received_ibc_token(
-                ibc_trace,
+            // Only artificially increase the IBC internal address pre-balance
+            // if receiving involves minting. We do not do this in the unescrow
+            // case since the pre-balance already accounts for the amount being
+            // received.
+            if !is_receiver_chain_source_str(
+                &ibc_trace,
                 &msg.packet.port_id_on_a,
                 &msg.packet.chan_id_on_a,
-                &msg.packet.port_id_on_b,
-                &msg.packet.chan_id_on_b,
-            )
-            .into_storage_result()?;
-            let delta = ValueSum::from_pair(token.clone(), amount);
-            // Enable funds to be taken from the IBC internal
-            // address and be deposited elsewhere
-            // Required for the IBC internal Address to release
-            // funds
-            let ibc_taddr = addr_taddr(address::IBC);
-            let pre_entry = accum
-                .pre
-                .get(&ibc_taddr)
-                .cloned()
-                .unwrap_or(ValueSum::zero());
-            accum.pre.insert(
-                ibc_taddr,
-                checked!(pre_entry + &delta)
-                    .map_err(namada_storage::Error::new)?,
-            );
+            ) {
+                // Get the received token
+                let token = received_ibc_token(
+                    ibc_trace,
+                    &msg.packet.port_id_on_a,
+                    &msg.packet.chan_id_on_a,
+                    &msg.packet.port_id_on_b,
+                    &msg.packet.chan_id_on_b,
+                )
+                .into_storage_result()?;
+                let delta = ValueSum::from_pair(token.clone(), amount);
+                // Enable funds to be taken from the IBC internal
+                // address and be deposited elsewhere
+                // Required for the IBC internal Address to release
+                // funds
+                let ibc_taddr = addr_taddr(address::IBC);
+                let pre_entry = accum
+                    .pre
+                    .get(&ibc_taddr)
+                    .cloned()
+                    .unwrap_or(ValueSum::zero());
+                accum.pre.insert(
+                    ibc_taddr,
+                    checked!(pre_entry + &delta)
+                        .map_err(namada_storage::Error::new)?,
+                );
+            }
         }
     }
     Ok(accum)

crates/ibc/src/trace.rs

@@ -108,13 +108,24 @@ pub fn is_nft_trace(
     }
 }
 
-/// Return true if the source of the given IBC trace is this chain
+/// Returns true if the denomination originally came from the sender chain, and
+/// false otherwise.
 pub fn is_sender_chain_source(
     trace: impl AsRef<str>,
     src_port_id: &PortId,
     src_channel_id: &ChannelId,
 ) -> bool {
-    !trace
+    !is_receiver_chain_source(trace, src_port_id, src_channel_id)
+}
+
+/// Returns true if the denomination originally came from the receiving chain,
+/// and false otherwise.
+pub fn is_receiver_chain_source(
+    trace: impl AsRef<str>,
+    src_port_id: &PortId,
+    src_channel_id: &ChannelId,
+) -> bool {
+    trace
         .as_ref()
         .starts_with(&format!("{src_port_id}/{src_channel_id}"))
 }

crates/shielded_token/src/vp.rs

@@ -60,16 +60,16 @@ where
     pub _marker: PhantomData<(Params, Gov, Ibc, TransToken, Transfer)>,
 }
 
-// The balances changed by the transaction, split between masp and non-masp
-// balances. The masp collection carries the token addresses. The collection of
-// the other balances maps the token address to the addresses of the
-// senders/receivers, their balance diff and whether this is positive or
-// negative diff
+// Balances changed by a transaction
 #[derive(Default, Debug, Clone)]
 struct ChangedBalances {
+    // Maps asset types to their decodings
     tokens: BTreeMap<AssetType, (Address, token::Denomination, MaspDigitPos)>,
+    // Map between MASP transparent address and Namada types
     decoder: BTreeMap<TransparentAddress, TAddrData>,
+    // Balances before the tx
     pre: BTreeMap<TransparentAddress, ValueSum<Address, Amount>>,
+    // Balances after the tx
     post: BTreeMap<TransparentAddress, ValueSum<Address, Amount>>,
 }
 
@@ -673,7 +673,7 @@ fn validate_transparent_input<A: Authorization>(
                 .checked_sub(&ValueSum::from_pair(asset.token.clone(), amount))
                 .ok_or_else(|| {
                     Error::NativeVpError(native_vp::Error::SimpleMessage(
-                        "Overflow in bundle balance",
+                        "Underflow in bundle balance",
                     ))
                 })?;
         }
@@ -703,7 +703,7 @@ fn validate_transparent_input<A: Authorization>(
                     .checked_sub(&ValueSum::from_pair(token.clone(), amount))
                     .ok_or_else(|| {
                         Error::NativeVpError(native_vp::Error::SimpleMessage(
-                            "Overflow in bundle balance",
+                            "Underflow in bundle balance",
                         ))
                     })?;
             }
@@ -751,7 +751,7 @@ fn validate_transparent_output(
                 .checked_sub(&ValueSum::from_pair(asset.token.clone(), amount))
                 .ok_or_else(|| {
                     Error::NativeVpError(native_vp::Error::SimpleMessage(
-                        "Overflow in bundle balance",
+                        "Underflow in bundle balance",
                     ))
                 })?;
         }
@@ -766,7 +766,7 @@ fn validate_transparent_output(
                 .checked_sub(&ValueSum::from_pair(token.clone(), amount))
                 .ok_or_else(|| {
                     Error::NativeVpError(native_vp::Error::SimpleMessage(
-                        "Overflow in bundle balance",
+                        "Underflow in bundle balance",
                     ))
                 })?;
         }
@@ -869,7 +869,7 @@ fn apply_balance_component(
     })
 }
 
-// Verify that the pre balance + the Sapling value balance = the post balance
+// Verify that the pre balance - the Sapling value balance = the post balance
 // using the decodings in tokens and conversion_state for assistance.
 fn verify_sapling_balancing_value(
     pre: &ValueSum<Address, Amount>,
@@ -945,18 +945,19 @@ where
             !is_masp_transfer_key(key) && !is_masp_token_map_key(key)
         });
 
+        // Check that the transaction didn't write unallowed masp keys
+        if non_allowed_changes {
+            return Err(Error::NativeVpError(native_vp::Error::SimpleMessage(
+                "Found modifications to non-allowed masp keys",
+            )));
+        }
         let masp_token_map_changed = masp_keys_changed
             .iter()
             .any(|key| is_masp_token_map_key(key));
         let masp_transfer_changes = masp_keys_changed
             .iter()
             .any(|key| is_masp_transfer_key(key));
-        // Check that the transaction didn't write unallowed masp keys
-        if non_allowed_changes {
-            Err(Error::NativeVpError(native_vp::Error::SimpleMessage(
-                "Found modifications to non-allowed masp keys",
-            )))
-        } else if masp_token_map_changed && masp_transfer_changes {
+        if masp_token_map_changed && masp_transfer_changes {
             Err(Error::NativeVpError(native_vp::Error::SimpleMessage(
                 "Cannot simultaneously do governance proposal and MASP \
                  transfer",

crates/systems/src/ibc.rs

@@ -29,7 +29,7 @@ pub trait Read<S> {
 /// Balances changed by a transaction
 #[derive(Default, Debug, Clone)]
 pub struct ChangedBalances {
-    /// Map between MASP transparent address and namada types
+    /// Map between MASP transparent address and Namada types
     pub decoder: BTreeMap<TransparentAddress, TAddrData>,
     /// Balances before the tx
     pub pre: BTreeMap<TransparentAddress, ValueSum<Address, token::Amount>>,

crates/tests/src/e2e/ibc_tests.rs

@@ -551,6 +551,31 @@ fn ibc_namada_gaia() -> Result<()> {
     check_balance(&test, AB_VIEWING_KEY, &ibc_denom, 40)?;
     check_gaia_balance(&test_gaia, GAIA_USER, GAIA_COIN, 810)?;
 
+    // Shielding transfer back from Gaia to Namada
+    let ibc_denom = format!("{port_id_gaia}/{channel_id_gaia}/{token_addr}");
+    let memo_path = gen_ibc_shielding_data(
+        &test,
+        AA_PAYMENT_ADDRESS,
+        &ibc_denom,
+        100,
+        &port_id_namada,
+        &channel_id_namada,
+    )?;
+    transfer_from_gaia(
+        &test_gaia,
+        GAIA_USER,
+        AA_PAYMENT_ADDRESS,
+        get_gaia_denom_hash(&ibc_denom),
+        100000000,
+        &port_id_gaia,
+        &channel_id_gaia,
+        Some(memo_path),
+    )?;
+    wait_for_packet_relay(&port_id_gaia, &channel_id_gaia, &test)?;
+
+    // Check the token on Namada
+    check_balance(&test, AA_VIEWING_KEY, APFEL, 100)?;
+
     Ok(())
 }