diff --git a/.changelog/unreleased/bug-fixes/3620-ibc-refund-transparent.md b/.changelog/unreleased/bug-fixes/3620-ibc-refund-transparent.md new file mode 100644 index 0000000000..f6d7ad7d65 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3620-ibc-refund-transparent.md @@ -0,0 +1,2 @@ +- Support only a transparent address as a refund target of IBC shielding + transfer ([\#3620](https://github.com/anoma/namada/issues/3620)) \ No newline at end of file diff --git a/crates/ibc/build.rs b/crates/ibc/build.rs new file mode 100644 index 0000000000..d9f7b88152 --- /dev/null +++ b/crates/ibc/build.rs @@ -0,0 +1,10 @@ +/// Set `is_apple_silicon` flag to avoid a wasm compilation error +fn main() { + let host_arch = + std::env::var("HOST").expect("HOST environment variable not found"); + + if host_arch == "aarch64-apple-darwin" { + println!("cargo:rustc-cfg=is_apple_silicon"); + } + println!("cargo::rustc-check-cfg=cfg(is_apple_silicon)"); +} diff --git a/crates/ibc/src/context/nft_transfer.rs b/crates/ibc/src/context/nft_transfer.rs index a767834d80..d2c50e90b1 100644 --- a/crates/ibc/src/context/nft_transfer.rs +++ b/crates/ibc/src/context/nft_transfer.rs @@ -14,7 +14,7 @@ use ibc::apps::nft_transfer::types::{ }; use ibc::core::handler::types::error::ContextError; use ibc::core::host::types::identifiers::{ChannelId, PortId}; -use namada_core::address::Address; +use namada_core::address::{Address, MASP}; use namada_core::token::Amount; use namada_systems::trans_token; @@ -28,6 +28,7 @@ where C: IbcCommonContext, { inner: Rc>, + is_shielded: bool, _marker: PhantomData, } @@ -40,10 +41,16 @@ where pub fn new(inner: Rc>) -> Self { Self { inner, + is_shielded: false, _marker: PhantomData, } } + /// Set to enable a shielded transfer + pub fn enable_shielded_transfer(&mut self) { + self.is_shielded = true; + } + /// Update the mint amount of the token fn update_mint_amount( &self, @@ -163,6 +170,12 @@ where // The metadata should exist self.get_nft(class_id, token_id)?; + let from_account = if self.is_shielded { + &MASP + } else { + from_account + }; + // Check the account owns the NFT if self.inner.borrow().is_nft_owned::( class_id, @@ -228,6 +241,8 @@ where // Metadata should exist self.get_nft(class_id, token_id)?; + let account = if self.is_shielded { &MASP } else { account }; + // Check the account owns the NFT if self .inner @@ -313,6 +328,12 @@ where self.add_withdraw(&ibc_token)?; + let from_account = if self.is_shielded { + &MASP + } else { + from_account + }; + self.inner .borrow_mut() .transfer_token( @@ -392,6 +413,8 @@ where self.update_mint_amount(&ibc_token, false)?; self.add_withdraw(&ibc_token)?; + let account = if self.is_shielded { &MASP } else { account }; + self.inner .borrow_mut() .burn_token(account, &ibc_token, Amount::from_u64(1)) diff --git a/crates/ibc/src/context/token_transfer.rs b/crates/ibc/src/context/token_transfer.rs index ebfd0acd5b..da3bedc41c 100644 --- a/crates/ibc/src/context/token_transfer.rs +++ b/crates/ibc/src/context/token_transfer.rs @@ -12,7 +12,7 @@ use ibc::apps::transfer::types::{Memo, PrefixedCoin, PrefixedDenom}; use ibc::core::channel::types::error::ChannelError; use ibc::core::handler::types::error::ContextError; use ibc::core::host::types::identifiers::{ChannelId, PortId}; -use namada_core::address::{Address, InternalAddress}; +use namada_core::address::{Address, InternalAddress, MASP}; use namada_core::token::Amount; use namada_core::uint::Uint; @@ -27,6 +27,7 @@ where { inner: Rc>, verifiers: Rc>>, + is_shielded: bool, } impl TokenTransferContext @@ -38,7 +39,11 @@ where inner: Rc>, verifiers: Rc>>, ) -> Self { - Self { inner, verifiers } + Self { + inner, + verifiers, + is_shielded: false, + } } /// Insert a verifier address whose VP will verify the tx. @@ -46,6 +51,11 @@ where self.verifiers.borrow_mut().insert(addr.clone()); } + /// Set to enable a shielded transfer + pub fn enable_shielded_transfer(&mut self) { + self.is_shielded = true; + } + /// Get the token address and the amount from PrefixedCoin. If the base /// denom is not an address, it returns `IbcToken` fn get_token_amount( @@ -251,6 +261,12 @@ where self.insert_verifier(&ibc_token); } + let from_account = if self.is_shielded { + &MASP + } else { + from_account + }; + self.inner .borrow_mut() .transfer_token( @@ -325,6 +341,8 @@ where self.insert_verifier(&ibc_token); } + let account = if self.is_shielded { &MASP } else { account }; + // The burn is "unminting" from the minted balance self.inner .borrow_mut() diff --git a/crates/ibc/src/lib.rs b/crates/ibc/src/lib.rs index 293b6d48ad..91b9037e80 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -61,9 +61,7 @@ use ibc::apps::transfer::handler::{ 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::core::channel::types::acknowledgement::{ - Acknowledgement, AcknowledgementStatus, -}; +use ibc::core::channel::types::acknowledgement::AcknowledgementStatus; use ibc::core::channel::types::commitment::compute_ack_commitment; use ibc::core::channel::types::msgs::{ MsgRecvPacket as IbcMsgRecvPacket, PacketMsg, @@ -614,6 +612,9 @@ where self.verifiers.clone(), ); self.insert_verifiers()?; + if msg.transfer.is_some() { + token_transfer_ctx.enable_shielded_transfer(); + } send_transfer_execute( &mut self.ctx, &mut token_transfer_ctx, @@ -625,6 +626,9 @@ where IbcMessage::NftTransfer(msg) => { let mut nft_transfer_ctx = NftTransferContext::<_, Token>::new(self.ctx.inner.clone()); + if msg.transfer.is_some() { + nft_transfer_ctx.enable_shielded_transfer(); + } send_nft_transfer_execute( &mut self.ctx, &mut nft_transfer_ctx, @@ -638,34 +642,17 @@ where .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(packet_msg) => { - match packet_msg { - PacketMsg::Recv(msg) => { - if self.is_receiving_success(msg)? { - extract_masp_tx_from_packet( - &msg.packet, - false, - ) - } else { - None - } - } - PacketMsg::Ack(msg) => { - if is_ack_successful(&msg.acknowledgement)? { - // No refund - None - } else { - extract_masp_tx_from_packet( - &msg.packet, - true, - ) - } - } - PacketMsg::Timeout(msg) => { - extract_masp_tx_from_packet(&msg.packet, true) - } - _ => None, - } + MsgEnvelope::Packet(PacketMsg::Recv(msg)) + if self.is_receiving_success(msg)? => + { + extract_masp_tx_from_packet(&msg.packet) + } + #[cfg(is_apple_silicon)] + MsgEnvelope::Packet(PacketMsg::Ack(msg)) => { + // NOTE: This is unneeded but wasm compilation error + // happened if deleted on macOS with Apple Silicon + let _ = extract_masp_tx_from_packet(&msg.packet); + None } _ => None, }; @@ -746,18 +733,6 @@ where } } -fn is_ack_successful(ack: &Acknowledgement) -> Result { - let acknowledgement = serde_json::from_slice::( - ack.as_ref(), - ) - .map_err(|e| { - Error::TokenTransfer(TokenTransferError::Other(format!( - "Decoding the acknowledgement failed: {e}" - ))) - })?; - Ok(acknowledgement.is_successful()) -} - /// Tries to decode transaction data to an `IbcMessage` pub fn decode_message( tx_data: &[u8], diff --git a/crates/ibc/src/msg.rs b/crates/ibc/src/msg.rs index 44b23fae74..8fdb922ed6 100644 --- a/crates/ibc/src/msg.rs +++ b/crates/ibc/src/msg.rs @@ -128,12 +128,7 @@ impl BorshSchema for MsgNftTransfer { /// Shielding data in IBC packet memo #[derive(Debug, Clone, BorshDeserialize, BorshSerialize)] -pub struct IbcShieldingData { - /// MASP transaction for receiving the token - pub shielding: Option, - /// MASP transaction for refunding the token - pub refund: Option, -} +pub struct IbcShieldingData(pub MaspTransaction); impl From for String { fn from(data: IbcShieldingData) -> Self { @@ -146,18 +141,9 @@ pub fn extract_masp_tx_from_envelope( envelope: &MsgEnvelope, ) -> Option { match envelope { - MsgEnvelope::Packet(packet_msg) => match packet_msg { - PacketMsg::Recv(msg) => { - extract_masp_tx_from_packet(&msg.packet, false) - } - PacketMsg::Ack(msg) => { - extract_masp_tx_from_packet(&msg.packet, true) - } - PacketMsg::Timeout(msg) => { - extract_masp_tx_from_packet(&msg.packet, true) - } - _ => None, - }, + MsgEnvelope::Packet(PacketMsg::Recv(msg)) => { + extract_masp_tx_from_packet(&msg.packet) + } _ => None, } } @@ -171,22 +157,9 @@ pub fn decode_ibc_shielding_data( } /// Extract MASP transaction from IBC packet memo -pub fn extract_masp_tx_from_packet( - packet: &Packet, - is_sender: bool, -) -> Option { - let port_id = if is_sender { - &packet.port_id_on_a - } else { - &packet.port_id_on_b - }; - let memo = extract_memo_from_packet(packet, port_id)?; - let shielding_data = decode_ibc_shielding_data(memo)?; - if is_sender { - shielding_data.refund - } else { - shielding_data.shielding - } +pub fn extract_masp_tx_from_packet(packet: &Packet) -> Option { + let memo = extract_memo_from_packet(packet, &packet.port_id_on_b)?; + decode_ibc_shielding_data(memo).map(|data| data.0) } fn extract_memo_from_packet( @@ -220,9 +193,5 @@ fn extract_memo_from_packet( /// Get IBC memo string from MASP transaction for receiving pub fn convert_masp_tx_to_ibc_memo(transaction: &MaspTransaction) -> String { - let shielding_data = IbcShieldingData { - shielding: Some(transaction.clone()), - refund: None, - }; - shielding_data.into() + IbcShieldingData(transaction.clone()).into() } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index ef0cc79080..5562748b8f 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -18,7 +18,6 @@ use masp_primitives::transaction::components::transparent::fees::{ }; use masp_primitives::transaction::components::I128Sum; use masp_primitives::transaction::{builder, Transaction as MaspTransaction}; -use masp_primitives::zip32::ExtendedFullViewingKey; use namada_account::{InitAccount, UpdateAccount}; use namada_core::address::{Address, IBC, MASP}; use namada_core::arith::checked; @@ -37,8 +36,7 @@ use namada_core::ibc::core::host::types::identifiers::{ChannelId, PortId}; use namada_core::ibc::primitives::Timestamp as IbcTimestamp; use namada_core::key::{self, *}; use namada_core::masp::{ - AssetData, ExtendedSpendingKey, MaspEpoch, PaymentAddress, TransferSource, - TransferTarget, + AssetData, ExtendedSpendingKey, MaspEpoch, TransferSource, TransferTarget, }; use namada_core::storage; use namada_core::storage::Epoch; @@ -53,7 +51,7 @@ use namada_governance::storage::proposal::{ use namada_governance::storage::vote::ProposalVote; use namada_ibc::storage::channel_key; use namada_ibc::trace::is_nft_trace; -use namada_ibc::{IbcShieldingData, MsgNftTransfer, MsgTransfer}; +use namada_ibc::{MsgNftTransfer, MsgTransfer}; use namada_proof_of_stake::parameters::{ PosParams, MAX_VALIDATOR_METADATA_LEN, }; @@ -144,6 +142,9 @@ pub const TX_UPDATE_STEWARD_COMMISSION: &str = /// Redelegate transaction WASM path pub const TX_REDELEGATE_WASM: &str = "tx_redelegate.wasm"; +/// Refund target alias prefix for IBC shielded transfers +const IBC_REFUND_ALIAS_PREFIX: &str = "ibc-refund-target"; + /// Default timeout in seconds for requests to the `/accepted` /// and `/applied` ABCI query endpoints. const DEFAULT_NAMADA_EVENTS_MAX_WAIT_TIME_SECONDS: u64 = 60; @@ -2658,38 +2659,13 @@ pub async fn build_ibc_transfer( (args.source.spending_key().is_some() && refund_target.is_some()) || (args.source.address().is_some() && refund_target.is_none()) ); - // Reconstruct the memo for refunding if needed - let memo = if let Some(refund_target) = refund_target { - // Generate MASP transaction for refunding the token - let masp_transfer_data = vec![MaspTransferData { - source: TransferSource::Address(IBC), - target: TransferTarget::PaymentAddress(refund_target), - token: args.token.clone(), - amount: validated_amount, - }]; - let masp_tx = construct_shielded_parts( - context, - masp_transfer_data, - None, - !(args.tx.dry_run || args.tx.dry_run_wrapper), - ) - .await? - .map(|(shielded_transfer, _)| shielded_transfer.masp_tx); - let shielding_data = IbcShieldingData { - shielding: args - .ibc_shielding_data - .as_ref() - .and_then(|data| data.shielding.clone()), - refund: masp_tx, - }; - Some(shielding_data.into()) - } else { - args.ibc_shielding_data - .as_ref() - .map_or(args.ibc_memo.clone(), |shielding_data| { - Some(shielding_data.clone().into()) - }) - }; + // The memo is either IbcShieldingData or just a memo + let memo = args + .ibc_shielding_data + .as_ref() + .map_or(args.ibc_memo.clone(), |shielding_data| { + Some(shielding_data.clone().into()) + }); // If the refund address is given, set the refund address. It is used only // when refunding and won't affect the actual transfer because the actual // source will be the MASP address and the MASP transaction is generated by @@ -3930,52 +3906,53 @@ async fn target_exists_or_err( } /// Returns the given refund target address if the given address is valid for -/// the IBC shielded transfer. Returns an error if the address is transparent -/// or the address is given for non-shielded transfer. +/// the IBC shielded transfer. Returns an error if the address is a payment +/// address or given for non-shielded transfer. async fn get_refund_target( context: &impl Namada, source: &TransferSource, refund_target: &Option, -) -> Result> { +) -> Result> { match (source, refund_target) { - ( - TransferSource::ExtendedSpendingKey(_), - Some(TransferTarget::PaymentAddress(pa)), - ) => Ok(Some(*pa)), + (_, Some(TransferTarget::PaymentAddress(pa))) => { + Err(Error::Other(format!( + "Supporting only a transparent address as a refund target: {}", + pa, + ))) + } ( TransferSource::ExtendedSpendingKey(_), Some(TransferTarget::Address(addr)), - ) => Err(Error::Other(format!( - "Transparent address can't be specified as a refund target: {}", - addr, - ))), - (TransferSource::ExtendedSpendingKey(spending_key), None) => { - // Generate a new payment address - let viewing_key = - ExtendedFullViewingKey::from(&(*spending_key).into()).fvk.vk; + ) => Ok(Some(addr.clone())), + (TransferSource::ExtendedSpendingKey(_), None) => { + // Generate a new transparent address if it doesn't exist let mut rng = OsRng; - let (div, _g_d) = crate::masp::find_valid_diversifier(&mut rng); - let payment_addr: PaymentAddress = viewing_key - .to_payment_address(div) - .ok_or_else(|| { - Error::Other( - "Converting to a payment address failed".to_string(), - ) - })? - .into(); - let alias = format!("ibc-refund-target-{}", rng.next_u64()); let mut wallet = context.wallet_mut().await; + let mut alias = + format!("{IBC_REFUND_ALIAS_PREFIX}-{}", rng.next_u64()); + while wallet.find_address(&alias).is_some() { + alias = format!("{IBC_REFUND_ALIAS_PREFIX}-{}", rng.next_u64()); + } wallet - .insert_payment_addr(alias, payment_addr, false) + .gen_store_secret_key( + SchemeType::Ed25519, + Some(alias.clone()), + false, + None, + &mut rng, + ) .ok_or_else(|| { Error::Other( - "Adding a new payment address failed".to_string(), + "Adding a new refund address failed".to_string(), ) })?; wallet.save().map_err(|e| { Error::Other(format!("Saving wallet error: {e}")) })?; - Ok(Some(payment_addr)) + let addr = wallet.find_address(alias).ok_or_else(|| { + Error::Other("Finding the reund address failed".to_string()) + })?; + Ok(Some(addr.into_owned())) } (_, Some(_)) => Err(Error::Other( "Refund target can't be specified for non-shielded transfer" diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index a4bde77ed9..aef4b65bd7 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -103,6 +103,8 @@ use crate::strings::{ }; use crate::{run, run_as}; +const IBC_REFUND_TARGET_ALIAS: &str = "ibc-refund-target"; + #[test] fn run_ledger_ibc() -> Result<()> { let update_genesis = @@ -306,7 +308,7 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { let shielding_data_path = gen_ibc_shielding_data( &test_b, AB_PAYMENT_ADDRESS, - token_addr, + &token_addr, 1_000_000_000, &port_id_b, &channel_id_b, @@ -344,14 +346,24 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { false, )?; wait_for_packet_relay(&port_id_a, &channel_id_a, &test_a)?; - // The balance should not be changed - check_shielded_balances(&port_id_b, &channel_id_b, &test_a, &test_b)?; + // Check the balance of the source shielded account + check_balance(&test_a, AA_VIEWING_KEY, BTC, 80)?; + // Check the refund + check_balance(&test_a, IBC_REFUND_TARGET_ALIAS, BTC, 10)?; // Stop Hermes for timeout test let mut hermes = bg_hermes.foreground(); hermes.interrupt()?; // Send transfer will be timed out (refund) + let shielding_data_path = gen_ibc_shielding_data( + &test_b, + AB_PAYMENT_ADDRESS, + token_addr, + 1_000_000_000, + &port_id_b, + &channel_id_b, + )?; transfer( &test_a, A_SPENDING_KEY, @@ -362,7 +374,7 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { &port_id_a, &channel_id_a, Some(Duration::new(10, 0)), - None, + Some(shielding_data_path), None, false, )?; @@ -374,8 +386,10 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { let _bg_hermes = hermes.background(); wait_for_packet_relay(&port_id_a, &channel_id_a, &test_a)?; - // The balance should not be changed - check_shielded_balances(&port_id_b, &channel_id_b, &test_a, &test_b)?; + // Check the balance of the source shielded account + check_balance(&test_a, AA_VIEWING_KEY, BTC, 70)?; + // Check the refund + check_balance(&test_a, IBC_REFUND_TARGET_ALIAS, BTC, 10)?; Ok(()) } @@ -2210,6 +2224,24 @@ fn transfer( tx_args.push(&memo); } + if sender.as_ref().starts_with("zsk") { + let mut cmd = run!( + test, + Bin::Wallet, + &[ + "gen", + "--alias", + IBC_REFUND_TARGET_ALIAS, + "--alias-force", + "--unsafe-dont-encrypt" + ], + Some(20), + )?; + cmd.assert_success(); + tx_args.push("--refund-target"); + tx_args.push(IBC_REFUND_TARGET_ALIAS); + } + let mut client = run!(test, Bin::Client, tx_args, Some(300))?; match expected_err { Some(err) => {