From 05c584a00642124194f14102e3f41544bac24078 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 19 Jun 2024 17:05:18 +0200 Subject: [PATCH] fix for apply_recv_msg --- crates/ibc/src/lib.rs | 8 +- crates/ibc/src/storage.rs | 80 +++++++++++++- crates/namada/src/ledger/native_vp/masp.rs | 122 +++++++++++---------- crates/sdk/src/tx.rs | 11 +- 4 files changed, 146 insertions(+), 75 deletions(-) diff --git a/crates/ibc/src/lib.rs b/crates/ibc/src/lib.rs index 5bdf40bc229..a52e7f982e9 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -433,12 +433,8 @@ pub fn received_ibc_token( dest_port_id, dest_channel_id, )?; - if ibc_trace.contains('/') { - Ok(storage::ibc_token(ibc_trace)) - } else { - Address::decode(ibc_trace) - .map_err(|e| Error::Trace(format!("Invalid base token: {e}"))) - } + storage::convert_to_address(ibc_trace) + .map_err(|e| Error::Trace(format!("Invalid base token: {e}"))) } /// Returns the trace path and the token string if the denom is an IBC diff --git a/crates/ibc/src/storage.rs b/crates/ibc/src/storage.rs index 36e6dd4458a..7c02e377026 100644 --- a/crates/ibc/src/storage.rs +++ b/crates/ibc/src/storage.rs @@ -2,7 +2,10 @@ use std::str::FromStr; -use ibc::apps::nft_transfer::types::{PrefixedClassId, TokenId}; +use ibc::apps::nft_transfer::types::{ + PrefixedClassId, TokenId, TracePath as NftTracePath, +}; +use ibc::apps::transfer::types::{PrefixedDenom, TracePath}; use ibc::core::client::types::Height; use ibc::core::host::types::identifiers::{ ChannelId, ClientId, ConnectionId, PortId, Sequence, @@ -47,8 +50,8 @@ pub enum Error { StorageKey(namada_core::storage::Error), #[error("Invalid Key: {0}")] InvalidKey(String), - #[error("Port capability error: {0}")] - InvalidPortCapability(String), + #[error("Invalid IBC trace: {0}")] + InvalidIbcTrace(String), } /// IBC storage functions result @@ -512,7 +515,76 @@ pub fn ibc_token_for_nft( class_id: &PrefixedClassId, token_id: &TokenId, ) -> Address { - ibc_token(format!("{class_id}/{token_id}")) + ibc_token(ibc_trace_for_nft(class_id, token_id)) +} + +/// Obtain the IBC trace from the given NFT class ID and NFT ID +pub fn ibc_trace_for_nft( + class_id: &PrefixedClassId, + token_id: &TokenId, +) -> String { + format!("{class_id}/{token_id}") +} + +/// Convert the given IBC trace to [`Address`] +pub fn convert_to_address(ibc_trace: impl AsRef) -> Result
{ + // validation + if is_ibc_denom(&ibc_trace).is_none() && is_nft_trace(&ibc_trace).is_none() + { + return Err(Error::InvalidIbcTrace(format!( + "Invalid IBC trace: {}", + ibc_trace.as_ref() + ))); + } + + if ibc_trace.as_ref().contains('/') { + Ok(ibc_token(ibc_trace.as_ref())) + } else { + Address::decode(ibc_trace.as_ref()) + .map_err(|e| Error::InvalidIbcTrace(e.to_string())) + } +} + +/// Returns the trace path and the token string if the denom is an IBC +/// denom. +pub fn is_ibc_denom(denom: impl AsRef) -> Option<(TracePath, String)> { + let prefixed_denom = PrefixedDenom::from_str(denom.as_ref()).ok()?; + let base_denom = prefixed_denom.base_denom.to_string(); + if prefixed_denom.trace_path.is_empty() || base_denom.contains('/') { + // The denom is just a token or an NFT trace + return None; + } + // The base token isn't decoded because it could be non Namada token + Some((prefixed_denom.trace_path, base_denom)) +} + +/// Returns the trace path and the token string if the trace is an NFT one +pub fn is_nft_trace( + trace: impl AsRef, +) -> Option<(NftTracePath, String, String)> { + // The trace should be {port}/{channel}/.../{class_id}/{token_id} + if let Some((class_id, token_id)) = trace.as_ref().rsplit_once('/') { + let prefixed_class_id = PrefixedClassId::from_str(class_id).ok()?; + // The base token isn't decoded because it could be non Namada token + Some(( + prefixed_class_id.trace_path, + prefixed_class_id.base_class_id.to_string(), + token_id.to_string(), + )) + } else { + None + } +} + +/// Return true if the source of the given IBC trace is this chain +pub fn is_sender_chain_source( + trace: impl AsRef, + src_port_id: &PortId, + src_channel_id: &ChannelId, +) -> bool { + !trace + .as_ref() + .starts_with(&format!("{src_port_id}/{src_channel_id}")) } /// Returns true if the given key is for IBC diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index bf829d32da0..fe8b88069d0 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -16,14 +16,17 @@ use namada_core::address::Address; use namada_core::arith::{checked, CheckedAdd, CheckedSub}; use namada_core::booleans::BoolResultUnitExt; use namada_core::collections::HashSet; +use namada_core::ibc::apps::nft_transfer::types::packet::PacketData as NftPacketData; use namada_core::ibc::apps::transfer::types::packet::PacketData; use namada_core::masp::{addr_taddr, encode_asset_type, ibc_taddr, MaspEpoch}; use namada_core::storage::Key; use namada_gas::GasMetering; use namada_governance::storage::is_proposal_accepted; use namada_ibc::core::channel::types::msgs::MsgRecvPacket as IbcMsgRecvPacket; -use namada_ibc::core::host::types::identifiers::{ChannelId, PortId}; -use namada_ibc::storage::ibc_token; +use namada_ibc::core::host::types::identifiers::{ChannelId, PortId, Sequence}; +use namada_ibc::storage::{ + convert_to_address, ibc_trace_for_nft, is_sender_chain_source, +}; use namada_ibc::IbcMessage; use namada_sdk::masp::{verify_shielded_tx, TAddrData}; use namada_state::{ConversionState, OptionExt, ResultExt, StateRead}; @@ -50,10 +53,9 @@ use crate::sdk::ibc::core::channel::types::acknowledgement::AcknowledgementStatu use crate::sdk::ibc::core::channel::types::commitment::{ compute_ack_commitment, AcknowledgementCommitment, }; -use crate::sdk::ibc::core::channel::types::packet::Packet; use crate::token; use crate::token::MaspDigitPos; -use crate::uint::{Uint, I320}; +use crate::uint::I320; use crate::vm::WasmCacheAccess; #[allow(missing_docs)] @@ -340,18 +342,11 @@ where amount: Amount, receiver: impl AsRef, ) -> Result { - let token = if ibc_trace.as_ref().contains('/') { - Address::decode(ibc_trace.as_ref()).into_storage_result()? - } else { - ibc_token(ibc_trace.as_ref()) - }; + let token = convert_to_address(&ibc_trace).into_storage_result()?; let delta = ValueSum::from_pair(token.clone(), amount); // If there is a transfer to the IBC account, then deduplicate the // balance increase since we already accounted for it above - if !ibc_trace - .as_ref() - .starts_with(&format!("{src_port_id}/{src_channel_id}")) - { + if is_sender_chain_source(ibc_trace, src_port_id, src_channel_id) { let post_entry = acc.post.entry(addr_taddr(IBC)).or_insert(ValueSum::zero()); *post_entry = @@ -371,19 +366,12 @@ where // Check if IBC message was received successfully in this state transition fn is_receiving_success( &self, - packet: &Packet, - keys_changed: &BTreeSet, + src_port_id: &PortId, + src_channel_id: &ChannelId, + sequence: Sequence, ) -> Result { // Ensure that the event corresponds to the current changes to storage - let ack_key = storage::ack_key( - &packet.port_id_on_a, - &packet.chan_id_on_a, - packet.seq_on_a, - ); - if !keys_changed.contains(&ack_key) { - // Ignore packet if it was not acknowledged during this state change - return Ok(false); - } + let ack_key = storage::ack_key(src_port_id, src_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(), @@ -403,20 +391,20 @@ where &self, mut acc: ChangedBalances, msg: &IbcMsgRecvPacket, - packet_data: &PacketData, - keys_changed: &BTreeSet, + ibc_trace: impl AsRef, + amount: Amount, ) -> Result { // If the transfer was a failure, then enable funds to // be withdrawn from the IBC internal address - if self.is_receiving_success(&msg.packet, keys_changed)? { + if self.is_receiving_success( + &msg.packet.port_id_on_a, + &msg.packet.chan_id_on_a, + msg.packet.seq_on_a, + )? { // Mirror how the IBC token is derived in // gen_ibc_shielded_transfer in the non-refund case - let ibc_denom = self.query_ibc_denom( - packet_data.token.denom.to_string(), - Some(&Address::Internal(InternalAddress::Ibc)), - )?; let token = namada_ibc::received_ibc_token( - ibc_denom, + ibc_trace, &msg.packet.port_id_on_a, &msg.packet.chan_id_on_a, &msg.packet.port_id_on_b, @@ -424,10 +412,7 @@ where ) .into_storage_result() .map_err(Error::NativeVpError)?; - let delta = ValueSum::from_pair( - token.clone(), - Amount::from_uint(Uint(*packet_data.token.amount), 0).unwrap(), - ); + 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 @@ -445,7 +430,6 @@ where &self, mut acc: ChangedBalances, ibc_msg: &IbcMessage, - keys_changed: &BTreeSet, ) -> Result { match ibc_msg { // This event is emitted on the sender @@ -477,9 +461,9 @@ where let addr = TAddrData::Ibc(receiver.clone()); acc.decoder.insert(ibc_taddr(receiver.clone()), addr); for token_id in &msg.message.packet_data.token_ids.0 { - let ibc_trace = format!( - "{}/{}", - msg.message.packet_data.class_id, token_id + let ibc_trace = ibc_trace_for_nft( + &msg.message.packet_data.class_id, + token_id, ); let amount = Amount::from_u64(1); acc = self.apply_transfer_msg( @@ -493,22 +477,46 @@ where } } // This event is emitted on the receiver - IbcMessage::RecvPacket(msg) - if msg.message.packet.port_id_on_b.as_str() == PORT_ID_STR => - { - let packet_data = serde_json::from_slice::( - &msg.message.packet.data, - ) - .map_err(native_vp::Error::new)?; - let receiver = packet_data.receiver.to_string(); - let addr = TAddrData::Ibc(receiver.clone()); - acc.decoder.insert(ibc_taddr(receiver), addr); - acc = self.apply_recv_msg( - acc, - &msg.message, - &packet_data, - keys_changed, - )?; + IbcMessage::RecvPacket(msg) => { + if msg.message.packet.port_id_on_b.as_str() == PORT_ID_STR { + let packet_data = serde_json::from_slice::( + &msg.message.packet.data, + ) + .map_err(native_vp::Error::new)?; + let receiver = packet_data.receiver.to_string(); + let addr = TAddrData::Ibc(receiver.clone()); + acc.decoder.insert(ibc_taddr(receiver), addr); + let ibc_denom = packet_data.token.denom.to_string(); + let amount = packet_data + .token + .amount + .try_into() + .into_storage_result()?; + acc = self.apply_recv_msg( + acc, + &msg.message, + ibc_denom, + amount, + )?; + } else { + let packet_data = serde_json::from_slice::( + &msg.message.packet.data, + ) + .map_err(native_vp::Error::new)?; + let receiver = packet_data.receiver.to_string(); + let addr = TAddrData::Ibc(receiver.clone()); + acc.decoder.insert(ibc_taddr(receiver), addr); + for token_id in &packet_data.token_ids.0 { + let ibc_trace = + ibc_trace_for_nft(&packet_data.class_id, token_id); + acc = self.apply_recv_msg( + acc, + &msg.message, + ibc_trace, + Amount::from_u64(1), + )?; + } + } } // Ignore all other IBC events _ => {} @@ -584,7 +592,7 @@ where let changed_balances = ibc_msgs.iter().try_fold(changed_balances, |acc, ibc_msg| { // Apply all IBC packets to the changed balances structure - self.apply_ibc_packet(acc, ibc_msg, keys_changed) + self.apply_ibc_packet(acc, ibc_msg) })?; Ok(changed_balances) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 9b7cf111f3f..0c4dd1a6dc2 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -50,7 +50,7 @@ use namada_governance::storage::proposal::{ InitProposalData, ProposalType, VoteProposalData, }; use namada_governance::storage::vote::ProposalVote; -use namada_ibc::storage::{channel_key, ibc_token}; +use namada_ibc::storage::{channel_key, convert_to_address}; use namada_ibc::{is_nft_trace, MsgNftTransfer, MsgTransfer}; use namada_proof_of_stake::parameters::{ PosParams, MAX_VALIDATOR_METADATA_LEN, @@ -3488,13 +3488,8 @@ pub async fn gen_ibc_shielding_transfer( let ibc_denom = rpc::query_ibc_denom(context, &args.token, Some(&source)).await; let token = if args.refund { - if ibc_denom.contains('/') { - ibc_token(ibc_denom) - } else { - // the token is a base token - Address::decode(&ibc_denom) - .map_err(|e| Error::Other(format!("Invalid token: {e}")))? - } + convert_to_address(ibc_denom) + .map_err(|e| Error::Other(format!("Invalid token: {e}")))? } else { // Need to check the prefix namada_ibc::received_ibc_token(