From a2ff78d478b8715f5bbde86d863de783109a88dd Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 14 Aug 2024 10:45:26 +0200 Subject: [PATCH 1/9] change to transparent addr --- crates/ibc/src/lib.rs | 48 +++-------------- crates/ibc/src/msg.rs | 35 +++--------- crates/node/src/protocol.rs | 6 ++- crates/sdk/src/tx.rs | 103 +++++++++++++----------------------- 4 files changed, 53 insertions(+), 139 deletions(-) diff --git a/crates/ibc/src/lib.rs b/crates/ibc/src/lib.rs index 293b6d48ad..d9b56dc983 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, @@ -638,33 +636,11 @@ 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, false) + } else { + None } } _ => None, @@ -746,18 +722,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..bfaf37d2b6 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, false) + } _ => None, } } @@ -181,12 +167,7 @@ pub fn extract_masp_tx_from_packet( &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 - } + decode_ibc_shielding_data(memo).map(|data| data.0) } fn extract_memo_from_packet( @@ -220,9 +201,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/node/src/protocol.rs b/crates/node/src/protocol.rs index 46a3e69307..d04531b370 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -749,8 +749,10 @@ where state.write_log_mut().drop_tx(); tracing::error!( "The first transaction in the batch failed to pay \ - fees via the MASP, some VPs rejected it: {:#?}", - result.vps_result.rejected_vps + fees via the MASP, some VPs rejected it: {:#?}, \ + errors {:?}", + result.vps_result.rejected_vps, + result.vps_result.errors, ); } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index ef0cc79080..72f4554572 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, }; @@ -68,7 +66,7 @@ use namada_tx::data::{ }; pub use namada_tx::{Authorization, *}; use num_traits::Zero; -use rand_core::{OsRng, RngCore}; +use rand_core::OsRng; use crate::args::{ SdkTypes, TxShieldedTransferData, TxShieldingTransferData, @@ -147,6 +145,8 @@ pub const TX_REDELEGATE_WASM: &str = "tx_redelegate.wasm"; /// Default timeout in seconds for requests to the `/accepted` /// and `/applied` ABCI query endpoints. const DEFAULT_NAMADA_EVENTS_MAX_WAIT_TIME_SECONDS: u64 = 60; +/// The alias prefix of a generated IBC refund target +const IBC_REFUND_TARGET_ALIAS_PREFIX: &str = "ibc-refund-target-"; /// Capture the result of running a transaction #[derive(Debug)] @@ -2658,38 +2658,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 +3905,48 @@ 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 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; - wallet - .insert_payment_addr(alias, payment_addr, false) + let (alias, _) = wallet + .gen_store_secret_key( + SchemeType::Ed25519, + Some(IBC_REFUND_TARGET_ALIAS_PREFIX.to_string()), + 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 new reund address failed".to_string()) + })?; + Ok(Some((*addr).clone())) } (_, Some(_)) => Err(Error::Other( "Refund target can't be specified for non-shielded transfer" From a2efb042f6ca0607607901954d48d23206087cd4 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 14 Aug 2024 14:54:55 +0200 Subject: [PATCH 2/9] add shielded mode to transfer context --- crates/ibc/src/context/nft_transfer.rs | 25 +++++++++++++++++++++++- crates/ibc/src/context/token_transfer.rs | 22 +++++++++++++++++++-- crates/ibc/src/lib.rs | 6 ++++++ 3 files changed, 50 insertions(+), 3 deletions(-) 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 d9b56dc983..b5c28cff79 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -612,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, @@ -623,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, From 88533d7e515c0d40de679241f55a1ac75eb88f73 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 15 Aug 2024 11:51:28 +0200 Subject: [PATCH 3/9] fix tests --- crates/ibc/src/lib.rs | 2 +- crates/ibc/src/msg.rs | 14 +++----------- crates/node/src/protocol.rs | 6 ++---- crates/sdk/src/tx.rs | 9 ++++++--- crates/tests/src/e2e/ibc_tests.rs | 22 ++++++++++++++++------ 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/crates/ibc/src/lib.rs b/crates/ibc/src/lib.rs index b5c28cff79..21abdf28a1 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -644,7 +644,7 @@ where let masp_tx = match &*envelope { MsgEnvelope::Packet(PacketMsg::Recv(msg)) => { if self.is_receiving_success(msg)? { - extract_masp_tx_from_packet(&msg.packet, false) + extract_masp_tx_from_packet(&msg.packet) } else { None } diff --git a/crates/ibc/src/msg.rs b/crates/ibc/src/msg.rs index bfaf37d2b6..8fdb922ed6 100644 --- a/crates/ibc/src/msg.rs +++ b/crates/ibc/src/msg.rs @@ -142,7 +142,7 @@ pub fn extract_masp_tx_from_envelope( ) -> Option { match envelope { MsgEnvelope::Packet(PacketMsg::Recv(msg)) => { - extract_masp_tx_from_packet(&msg.packet, false) + extract_masp_tx_from_packet(&msg.packet) } _ => None, } @@ -157,16 +157,8 @@ 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)?; +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) } diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index d04531b370..46a3e69307 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -749,10 +749,8 @@ where state.write_log_mut().drop_tx(); tracing::error!( "The first transaction in the batch failed to pay \ - fees via the MASP, some VPs rejected it: {:#?}, \ - errors {:?}", - result.vps_result.rejected_vps, - result.vps_result.errors, + fees via the MASP, some VPs rejected it: {:#?}", + result.vps_result.rejected_vps ); } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 72f4554572..1a5834b02f 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -66,7 +66,7 @@ use namada_tx::data::{ }; pub use namada_tx::{Authorization, *}; use num_traits::Zero; -use rand_core::OsRng; +use rand_core::{OsRng, RngCore}; use crate::args::{ SdkTypes, TxShieldedTransferData, TxShieldingTransferData, @@ -146,7 +146,7 @@ pub const TX_REDELEGATE_WASM: &str = "tx_redelegate.wasm"; /// and `/applied` ABCI query endpoints. const DEFAULT_NAMADA_EVENTS_MAX_WAIT_TIME_SECONDS: u64 = 60; /// The alias prefix of a generated IBC refund target -const IBC_REFUND_TARGET_ALIAS_PREFIX: &str = "ibc-refund-target-"; +const IBC_REFUND_TARGET_ALIAS_PREFIX: &str = "ibc-refund-target"; /// Capture the result of running a transaction #[derive(Debug)] @@ -3930,7 +3930,10 @@ async fn get_refund_target( let (alias, _) = wallet .gen_store_secret_key( SchemeType::Ed25519, - Some(IBC_REFUND_TARGET_ALIAS_PREFIX.to_string()), + Some(format!( + "{IBC_REFUND_TARGET_ALIAS_PREFIX}-{}", + rng.next_u64() + )), false, None, &mut rng, diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index a4bde77ed9..e3888b3d67 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -306,7 +306,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 +344,23 @@ 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)?; + // TODO: Check the refund // 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 +371,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 +383,9 @@ 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)?; + // TODO: Check the refund Ok(()) } From 4118d744aaa1ad250e8600f7fd023d8833d45c07 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 15 Aug 2024 13:39:17 +0200 Subject: [PATCH 4/9] fix default refund target --- crates/sdk/src/tx.rs | 24 ++++++++++++------------ crates/tests/src/e2e/ibc_tests.rs | 7 +++++-- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 1a5834b02f..9d3ec4b5f6 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -66,7 +66,7 @@ use namada_tx::data::{ }; pub use namada_tx::{Authorization, *}; use num_traits::Zero; -use rand_core::{OsRng, RngCore}; +use rand_core::OsRng; use crate::args::{ SdkTypes, TxShieldedTransferData, TxShieldingTransferData, @@ -142,11 +142,12 @@ pub const TX_UPDATE_STEWARD_COMMISSION: &str = /// Redelegate transaction WASM path pub const TX_REDELEGATE_WASM: &str = "tx_redelegate.wasm"; +/// Default refund target alias for IBC shielded transfers +pub const IBC_REFUND_TARGET_ALIAS: &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; -/// The alias prefix of a generated IBC refund target -const IBC_REFUND_TARGET_ALIAS_PREFIX: &str = "ibc-refund-target"; /// Capture the result of running a transaction #[derive(Debug)] @@ -3924,16 +3925,13 @@ async fn get_refund_target( Some(TransferTarget::Address(addr)), ) => Ok(Some(addr.clone())), (TransferSource::ExtendedSpendingKey(_), None) => { - // Generate a new transparent address + // Generate a new transparent address if it doesn't exist let mut rng = OsRng; let mut wallet = context.wallet_mut().await; - let (alias, _) = wallet + wallet .gen_store_secret_key( SchemeType::Ed25519, - Some(format!( - "{IBC_REFUND_TARGET_ALIAS_PREFIX}-{}", - rng.next_u64() - )), + Some(IBC_REFUND_TARGET_ALIAS.to_string()), false, None, &mut rng, @@ -3946,9 +3944,11 @@ async fn get_refund_target( wallet.save().map_err(|e| { Error::Other(format!("Saving wallet error: {e}")) })?; - let addr = wallet.find_address(alias).ok_or_else(|| { - Error::Other("Finding the new reund address failed".to_string()) - })?; + let addr = wallet + .find_address(IBC_REFUND_TARGET_ALIAS) + .ok_or_else(|| { + Error::Other("Finding the reund address failed".to_string()) + })?; Ok(Some((*addr).clone())) } (_, Some(_)) => Err(Error::Other( diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index e3888b3d67..5e7fa39a76 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -81,6 +81,7 @@ use namada_sdk::storage::{BlockHeight, Epoch, Key}; use namada_sdk::tendermint::abci::Event as AbciEvent; use namada_sdk::tendermint::block::Height as TmHeight; use namada_sdk::token::Amount; +use namada_sdk::tx::IBC_REFUND_TARGET_ALIAS; use namada_test_utils::TestWasms; use prost::Message; use setup::constants::*; @@ -346,7 +347,8 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { wait_for_packet_relay(&port_id_a, &channel_id_a, &test_a)?; // Check the balance of the source shielded account check_balance(&test_a, AA_VIEWING_KEY, BTC, 80)?; - // TODO: Check the refund + // Check the refund + check_balance(&test_a, IBC_REFUND_TARGET_ALIAS, BTC, 10)?; // Stop Hermes for timeout test let mut hermes = bg_hermes.foreground(); @@ -385,7 +387,8 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { wait_for_packet_relay(&port_id_a, &channel_id_a, &test_a)?; // Check the balance of the source shielded account check_balance(&test_a, AA_VIEWING_KEY, BTC, 70)?; - // TODO: Check the refund + // Check the refund + check_balance(&test_a, IBC_REFUND_TARGET_ALIAS, BTC, 20)?; Ok(()) } From db26e06b98620f522148df80243830ec8308ead7 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 15 Aug 2024 16:01:22 +0200 Subject: [PATCH 5/9] fix to check the existing addr --- crates/sdk/src/tx.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 9d3ec4b5f6..9383ea70b2 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3928,6 +3928,9 @@ async fn get_refund_target( // Generate a new transparent address if it doesn't exist let mut rng = OsRng; let mut wallet = context.wallet_mut().await; + if let Some(addr) = wallet.find_address(IBC_REFUND_TARGET_ALIAS) { + return Ok(Some(addr.into_owned())); + } wallet .gen_store_secret_key( SchemeType::Ed25519, @@ -3949,7 +3952,7 @@ async fn get_refund_target( .ok_or_else(|| { Error::Other("Finding the reund address failed".to_string()) })?; - Ok(Some((*addr).clone())) + Ok(Some(addr.into_owned())) } (_, Some(_)) => Err(Error::Other( "Refund target can't be specified for non-shielded transfer" From 21d49a543b746ebcc5245eb67ff0381991364035 Mon Sep 17 00:00:00 2001 From: yito88 Date: Fri, 16 Aug 2024 15:45:42 +0200 Subject: [PATCH 6/9] add changelog --- .../bug-fixes/3620-ibc-refund-transparent.md | 2 ++ crates/ibc/src/lib.rs | 16 ++++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/3620-ibc-refund-transparent.md 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/src/lib.rs b/crates/ibc/src/lib.rs index 21abdf28a1..ac3fd25d1b 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -642,12 +642,16 @@ 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(PacketMsg::Recv(msg)) => { - if self.is_receiving_success(msg)? { - extract_masp_tx_from_packet(&msg.packet) - } else { - None - } + MsgEnvelope::Packet(PacketMsg::Recv(msg)) + if self.is_receiving_success(msg)? => + { + extract_masp_tx_from_packet(&msg.packet) + } + MsgEnvelope::Packet(PacketMsg::Ack(msg)) => { + // TODO: 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, }; From 17b54dfe768dd4a02fad7f1b5e320ddd5172bddd Mon Sep 17 00:00:00 2001 From: yito88 Date: Fri, 16 Aug 2024 23:15:14 +0200 Subject: [PATCH 7/9] generate key --- crates/sdk/src/tx.rs | 22 +++++++++++----------- crates/tests/src/e2e/ibc_tests.rs | 23 +++++++++++++++++++++-- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 9383ea70b2..5562748b8f 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -66,7 +66,7 @@ use namada_tx::data::{ }; pub use namada_tx::{Authorization, *}; use num_traits::Zero; -use rand_core::OsRng; +use rand_core::{OsRng, RngCore}; use crate::args::{ SdkTypes, TxShieldedTransferData, TxShieldingTransferData, @@ -142,8 +142,8 @@ pub const TX_UPDATE_STEWARD_COMMISSION: &str = /// Redelegate transaction WASM path pub const TX_REDELEGATE_WASM: &str = "tx_redelegate.wasm"; -/// Default refund target alias for IBC shielded transfers -pub const IBC_REFUND_TARGET_ALIAS: &str = "ibc-refund-target"; +/// 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. @@ -3928,13 +3928,15 @@ async fn get_refund_target( // Generate a new transparent address if it doesn't exist let mut rng = OsRng; let mut wallet = context.wallet_mut().await; - if let Some(addr) = wallet.find_address(IBC_REFUND_TARGET_ALIAS) { - return Ok(Some(addr.into_owned())); + 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 .gen_store_secret_key( SchemeType::Ed25519, - Some(IBC_REFUND_TARGET_ALIAS.to_string()), + Some(alias.clone()), false, None, &mut rng, @@ -3947,11 +3949,9 @@ async fn get_refund_target( wallet.save().map_err(|e| { Error::Other(format!("Saving wallet error: {e}")) })?; - let addr = wallet - .find_address(IBC_REFUND_TARGET_ALIAS) - .ok_or_else(|| { - Error::Other("Finding the reund address failed".to_string()) - })?; + 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( diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 5e7fa39a76..aef4b65bd7 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -81,7 +81,6 @@ use namada_sdk::storage::{BlockHeight, Epoch, Key}; use namada_sdk::tendermint::abci::Event as AbciEvent; use namada_sdk::tendermint::block::Height as TmHeight; use namada_sdk::token::Amount; -use namada_sdk::tx::IBC_REFUND_TARGET_ALIAS; use namada_test_utils::TestWasms; use prost::Message; use setup::constants::*; @@ -104,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 = @@ -388,7 +389,7 @@ fn run_ledger_ibc_with_hermes() -> Result<()> { // 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, 20)?; + check_balance(&test_a, IBC_REFUND_TARGET_ALIAS, BTC, 10)?; Ok(()) } @@ -2223,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) => { From 1a1938576c67ce2a4103989433c5ba0a4dc9b407 Mon Sep 17 00:00:00 2001 From: yito88 Date: Mon, 19 Aug 2024 12:12:57 +0200 Subject: [PATCH 8/9] is_apple_silicon --- crates/ibc/build.rs | 10 ++++++++++ crates/ibc/src/lib.rs | 5 +++-- 2 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 crates/ibc/build.rs diff --git a/crates/ibc/build.rs b/crates/ibc/build.rs new file mode 100644 index 0000000000..5dca319440 --- /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/lib.rs b/crates/ibc/src/lib.rs index ac3fd25d1b..91b9037e80 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -647,9 +647,10 @@ where { extract_masp_tx_from_packet(&msg.packet) } + #[cfg(is_apple_silicon)] MsgEnvelope::Packet(PacketMsg::Ack(msg)) => { - // TODO: This is unneeded but wasm compilation error - // happened if deleted on MacOS with Apple Silicon + // 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 } From 8541049bee834a9342ee53b4b66dadf8a0dafa6a Mon Sep 17 00:00:00 2001 From: yito88 Date: Mon, 19 Aug 2024 12:25:07 +0200 Subject: [PATCH 9/9] fix for clippy --- crates/ibc/build.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/build.rs b/crates/ibc/build.rs index 5dca319440..d9f7b88152 100644 --- a/crates/ibc/build.rs +++ b/crates/ibc/build.rs @@ -5,6 +5,6 @@ fn main() { if host_arch == "aarch64-apple-darwin" { println!("cargo:rustc-cfg=is_apple_silicon"); - println!("cargo::rustc-check-cfg=cfg(is_apple_silicon)"); } + println!("cargo::rustc-check-cfg=cfg(is_apple_silicon)"); }