diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index a0249caca3..4c18bbb601 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -691,7 +691,11 @@ where .expect("Missing masp fee payment gas limit in storage") .min(tx_gas_meter.borrow().tx_gas_limit.into()); - let mut gas_meter = TxGasMeter::new(min_gas_limit); + let mut gas_meter = TxGasMeter::new( + namada_gas::Gas::from_whole_units(min_gas_limit).ok_or_else(|| { + Error::GasError("Overflow in gas expansion".to_string()) + })?, + ); gas_meter .copy_consumed_gas_from(&tx_gas_meter.borrow()) .map_err(|e| Error::GasError(e.to_string()))?; diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index a94c6d9724..67f0c69589 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -2138,9 +2138,6 @@ pub struct Tx { pub gas_limit: GasLimit, /// The optional expiration of the transaction pub expiration: TxExpiration, - // FIXME: maybe should move this out of here, it's only needed for txs that - // pay the fees via the masp, so it should go together with the optional - // gas spending keys /// Generate an ephimeral signing key to be used only once to sign a /// wrapper tx pub disposable_signing_key: bool, diff --git a/crates/sdk/src/error.rs b/crates/sdk/src/error.rs index a8934b1961..f0c22adfca 100644 --- a/crates/sdk/src/error.rs +++ b/crates/sdk/src/error.rs @@ -264,7 +264,10 @@ pub enum TxSubmitError { #[error("Proposal end epoch is not in the storage.")] EpochNotInStorage, /// Couldn't understand who the fee payer is - #[error("Either --signing-keys or --gas-payer must be available.")] + #[error( + "Either --signing-keys, --gas-payer or --disposable-gas-payer must be \ + available." + )] InvalidFeePayer, /// Account threshold is not set #[error("Account threshold must be set.")] diff --git a/crates/sdk/src/masp.rs b/crates/sdk/src/masp.rs index 66c3d68dc4..aff209ed6c 100644 --- a/crates/sdk/src/masp.rs +++ b/crates/sdk/src/masp.rs @@ -9,6 +9,7 @@ use std::path::PathBuf; use borsh::{BorshDeserialize, BorshSerialize}; use borsh_ext::BorshSerializeExt; +use itertools::Itertools; use lazy_static::lazy_static; use masp_primitives::asset_type::AssetType; #[cfg(feature = "mainnet")] @@ -50,7 +51,7 @@ use masp_proofs::bls12_381::Bls12; use masp_proofs::prover::LocalTxProver; use masp_proofs::sapling::BatchValidator; use namada_core::address::Address; -use namada_core::arith::{CheckedAdd, CheckedSub}; +use namada_core::arith::CheckedAdd; use namada_core::collections::{HashMap, HashSet}; use namada_core::dec::Dec; pub use namada_core::masp::{ @@ -81,7 +82,9 @@ use thiserror::Error; use crate::error::{Error, QueryError}; use crate::io::Io; use crate::queries::Client; -use crate::rpc::{query_block, query_conversion, query_denom}; +use crate::rpc::{ + query_block, query_conversion, query_denom, query_native_token, +}; use crate::{display_line, edisplay_line, rpc, MaybeSend, MaybeSync, Namada}; /// Env var to point to a dir with MASP parameters. When not specified, @@ -124,6 +127,7 @@ pub struct ShieldedTransfer { /// The data for a masp fee payment #[allow(missing_docs)] +#[derive(Debug)] pub struct MaspFeeData { pub sources: Vec, pub target: Address, @@ -171,6 +175,11 @@ struct MaspTxReorderedData { denoms: HashMap, } +// Data about the unspent amounts for any given shielded source coming from the +// spent notes in their posses that have been added to the builder. Can be used +// to either pay fees or to return a change +type Changes = HashMap; + /// Shielded pool data for a token #[allow(missing_docs)] #[derive(Debug, BorshSerialize, BorshDeserialize, BorshDeserializer)] @@ -531,15 +540,62 @@ pub fn find_valid_diversifier( } /// Determine if using the current note would actually bring us closer to our -/// target -pub fn is_amount_required(src: I128Sum, dest: I128Sum, delta: I128Sum) -> bool { - let gap = dest - src; +/// target. Returns the unused amounts (change) of delta if any +pub fn is_amount_required( + src: I128Sum, + dest: I128Sum, + normed_delta: I128Sum, + opt_delta: Option, +) -> Option { + let mut changes = None; + let gap = dest.clone() - src; + for (asset_type, value) in gap.components() { - if *value > 0 && delta[asset_type] > 0 { - return true; - } + if *value > 0 && normed_delta[asset_type] > 0 { + let signed_change_amt = + checked!(normed_delta[asset_type] - *value).unwrap_or_default(); + let unsigned_change_amt = if signed_change_amt > 0 { + signed_change_amt + } else { + // Even if there's no change we still need to set the return + // value of this function to be Some so that the caller sees + // that this note should be used + 0 + }; + + let change_amt = I128Sum::from_nonnegative( + asset_type.to_owned(), + unsigned_change_amt, + ) + .expect("Change is guaranteed to be non-negative"); + changes = changes + .map(|prev| prev + change_amt.clone()) + .or(Some(change_amt)); + } + } + + // Because of the way conversions are computed, we need an extra step here + // if the token is not the native one + if let Some(delta) = opt_delta { + // Only if this note is going to be used, handle the assets in delta + // (not normalized) that are not part of dest + changes = changes.map(|mut chngs| { + for (delta_asset_type, delta_amt) in delta.components() { + if !dest.asset_types().contains(delta_asset_type) { + let rmng = I128Sum::from_nonnegative( + delta_asset_type.to_owned(), + *delta_amt, + ) + .expect("Change is guaranteed to be non-negative"); + chngs += rmng; + } + } + + chngs + }); } - false + + changes } /// a masp change @@ -1385,14 +1441,17 @@ impl ShieldedContext { /// Collect enough unspent notes in this context to exceed the given amount /// of the specified asset type. Return the total value accumulated plus /// notes and the corresponding diversifiers/merkle paths that were used to - /// achieve the total value. + /// achieve the total value. Updates the changes map. + #[allow(clippy::too_many_arguments)] pub async fn collect_unspent_notes( &mut self, context: &impl Namada, spent_notes: &mut SpentNotesTracker, - vk: &ViewingKey, + sk: namada_core::masp::ExtendedSpendingKey, + is_native_token: bool, target: I128Sum, target_epoch: MaspEpoch, + changes: &mut Changes, ) -> Result< ( I128Sum, @@ -1401,6 +1460,7 @@ impl ShieldedContext { ), Error, > { + let vk = &to_viewing_key(&sk.into()).vk; // TODO: we should try to use the smallest notes possible to fund the // transaction to allow people to fetch less often // Establish connection with which to do exchange rate queries @@ -1447,16 +1507,29 @@ impl ShieldedContext { ) .await?; + let opt_delta = if is_native_token { + None + } else { + Some(contr.clone()) + }; // Use this note only if it brings us closer to our target - if is_amount_required( + if let Some(change) = is_amount_required( normed_val_acc.clone(), target.clone(), normed_contr.clone(), + opt_delta, ) { // Be sure to record the conversions used in computing // accumulated value val_acc += contr; normed_val_acc += normed_contr; + + // Update the changes + changes + .entry(sk) + .and_modify(|amt| *amt += &change) + .or_insert(change); + // Commit the conversions that were used to exchange conversions = proposed_convs; let merkle_path = self @@ -1693,6 +1766,7 @@ impl ShieldedContext { // destination are shielded return Ok(None); }; + let mut changes = Changes::default(); for (MaspSourceTransferData { source, token }, amount) in &source_data { Self::add_inputs( @@ -1704,6 +1778,7 @@ impl ShieldedContext { epoch, &denoms, &mut notes_tracker, + &mut changes, ) .await?; } @@ -1749,12 +1824,13 @@ impl ShieldedContext { epoch, &mut denoms, &mut notes_tracker, + &mut changes, ) .await?; } // Finally, add outputs representing the change from this payment. - Self::add_changes(&mut builder, &source_data)?; + Self::add_changes(&mut builder, changes)?; let builder_clone = builder.clone().map_builder(WalletMap); // Build and return the constructed transaction @@ -1868,9 +1944,7 @@ impl ShieldedContext { })) } - // Add the necessary transaction inputs to the builder. Returns the actual - // amount of inputs added to the transaction if these are shielded, `None` - // if transparent inputs + // Add the necessary transaction inputs to the builder. #[allow(clippy::too_many_arguments)] async fn add_inputs( context: &impl Namada, @@ -1881,11 +1955,10 @@ impl ShieldedContext { epoch: MaspEpoch, denoms: &HashMap, notes_tracker: &mut SpentNotesTracker, + changes: &mut Changes, ) -> Result, TransferErr> { - let spending_key = source.spending_key(); - // We want to fund our transaction solely from supplied spending key - let spending_key = spending_key.map(|x| x.into()); + let spending_key = source.spending_key(); // Now we build up the transaction within this object @@ -1914,6 +1987,8 @@ impl ShieldedContext { // If there are shielded inputs let added_amt = if let Some(sk) = spending_key { + let is_native_token = + &query_native_token(context.client()).await? == token; // Locate unspent notes that can help us meet the transaction // amount let (added_amount, unspent_notes, used_convs) = context @@ -1922,15 +1997,22 @@ impl ShieldedContext { .collect_unspent_notes( context, notes_tracker, - &to_viewing_key(&sk).vk, + sk, + is_native_token, I128Sum::from_sum(masp_amount), epoch, + changes, ) .await?; // Commit the notes found to our transaction for (diversifier, note, merkle_path) in unspent_notes { builder - .add_sapling_spend(sk, diversifier, note, merkle_path) + .add_sapling_spend( + sk.into(), + diversifier, + note, + merkle_path, + ) .map_err(|e| TransferErr::Build { error: builder::Error::SaplingBuild(e), data: None, @@ -2166,6 +2248,7 @@ impl ShieldedContext { epoch: MaspEpoch, denoms: &mut HashMap, notes_tracker: &mut SpentNotesTracker, + changes: &mut Changes, ) -> Result<(), TransferErr> { if denoms.get(token).is_none() { if let Some(denom) = query_denom(context.client(), token).await { @@ -2203,46 +2286,66 @@ impl ShieldedContext { let mut fees = I128Sum::zero(); // Convert the shortfall into a I128Sum for (asset_type, val) in asset_types.iter().zip(raw_amount) { - fees = - checked!(fees + &I128Sum::from_pair(*asset_type, val.into())) - .map_err(|e| TransferErr::General(e.into()))?; + fees += I128Sum::from_nonnegative(*asset_type, val.into()) + .map_err(|()| { + TransferErr::General(Error::Other( + "Fee amount is expected expected to be non-negative" + .to_string(), + )) + })?; } // 1. Try to use the change to pay fees - for (asset_type, amt) in builder.value_balance().components() { - if let Ordering::Greater = amt.cmp(&0) { - // Look for changes that match the fee asset types - for (fee_asset_type, fee_amt) in fees + let mut temp_changes = Changes::default(); + + for (sp, changes) in changes.iter() { + for (asset_type, change) in changes.components() { + for (_, fee_amt) in fees .clone() .components() .filter(|(axt, _)| *axt == asset_type) { - let transparent_target_hash = { - ripemd::Ripemd160::digest(sha2::Sha256::digest( - target.serialize_to_vec().as_ref(), + // Get the minimum between the available change and + // the due fee + let output_amt = I128Sum::from_nonnegative( + asset_type.to_owned(), + *change.min(fee_amt), + ) + .map_err(|()| { + TransferErr::General(Error::Other( + "Fee amount is expected to be non-negative" + .to_string(), )) - }; - - builder - .add_transparent_output( - &TransparentAddress(transparent_target_hash.into()), - *fee_asset_type, - // Get the minimum between the available change and - // the due fee - *amt.min(fee_amt) as u64, - ) - .map_err(|e| TransferErr::Build { - error: builder::Error::TransparentBuild(e), - data: None, - })?; - - fees = checked!( - fees - &ValueSum::from_pair( - asset_type.to_owned(), - amt.to_owned() + })?; + let denominated_output_amt = context + .shielded_mut() + .await + .convert_masp_amount_to_namada( + context.client(), + // Safe to unwrap + denoms.get(token).unwrap().to_owned(), + output_amt.clone(), ) + .await?; + + Self::add_outputs( + context, + builder, + TransferSource::ExtendedSpendingKey(sp.to_owned()), + &TransferTarget::Address(target.clone()), + token.clone(), + denominated_output_amt, + epoch, + denoms, ) - .map_err(|e| TransferErr::General(e.into()))?; + .await?; + + fees -= &output_amt; + // Update the changes + temp_changes + .entry(*sp) + .and_modify(|amt| *amt += &output_amt) + .or_insert(output_amt); } } @@ -2251,6 +2354,25 @@ impl ShieldedContext { } } + // Decrease the changes by the amounts used for fee payment + for (sp, temp_changes) in temp_changes.iter() { + for (asset_type, temp_change) in temp_changes.components() { + let output_amt = I128Sum::from_nonnegative( + asset_type.to_owned(), + *temp_change, + ) + .map_err(|()| { + TransferErr::General(Error::Other( + "Fee amount is expected expected to be non-negative" + .to_string(), + )) + })?; + + // Entry is guaranteed to be in the map + changes.entry(*sp).and_modify(|amt| *amt -= &output_amt); + } + } + if !fees.is_zero() { // 2. Look for unused spent notes of the sources and the optional // gas spending keys (sources first) @@ -2261,44 +2383,75 @@ impl ShieldedContext { .map(TransferSource::ExtendedSpendingKey), ) { - let Some(found_amt) = Self::add_inputs( - context, - builder, - &fee_source, - token, - amount, - epoch, - denoms, - notes_tracker, - ) - .await? - else { - continue; - }; - let denom_amt = context - .shielded_mut() - .await - .convert_masp_amount_to_namada( - context.client(), - denoms.get(token).unwrap().to_owned(), - found_amt.clone(), + for (asset_type, fee_amt) in fees.clone().components() { + let input_amt = I128Sum::from_nonnegative( + asset_type.to_owned(), + *fee_amt, + ) + .map_err(|()| { + TransferErr::General(Error::Other( + "Fee amount is expected expected to be \ + non-negative" + .to_string(), + )) + })?; + let denominated_fee = context + .shielded_mut() + .await + .convert_masp_amount_to_namada( + context.client(), + // Safe to unwrap + denoms.get(token).unwrap().to_owned(), + input_amt.clone(), + ) + .await?; + + let Some(found_amt) = Self::add_inputs( + context, + builder, + &fee_source, + token, + &denominated_fee, + epoch, + denoms, + notes_tracker, + changes, + ) + .await? + else { + continue; + }; + // Pick the minimum between the due fee and the amount found + let output_amt = match found_amt.partial_cmp(&input_amt) { + None | Some(Ordering::Less) => found_amt, + _ => input_amt.clone(), + }; + let denom_amt = context + .shielded_mut() + .await + .convert_masp_amount_to_namada( + context.client(), + // Safe to unwrap + denoms.get(token).unwrap().to_owned(), + output_amt.clone(), + ) + .await?; + + Self::add_outputs( + context, + builder, + fee_source.clone(), + &TransferTarget::Address(target.clone()), + token.clone(), + denom_amt, + epoch, + denoms, ) .await?; - Self::add_outputs( - context, - builder, - fee_source, - &TransferTarget::Address(target.clone()), - token.clone(), - denom_amt, - epoch, - denoms, - ) - .await?; + fees -= &output_amt; + } - fees = checked!(fees - &found_amt) - .map_err(|e| TransferErr::General(e.into()))?; if fees.is_zero() { break; } @@ -2319,68 +2472,45 @@ impl ShieldedContext { Ok(()) } - // Add the changes back to the sources to balance the transaction. This - // function has to be called after `add_fees` cause we might have some - // change coming from there too + // Consumes the changes and adds them back to the original sources to + // balance the transaction. This function has to be called after + // `add_fees` cause we might have some change coming from there too #[allow(clippy::result_large_err)] fn add_changes( builder: &mut Builder, - source_data: &HashMap, + changes: Changes, ) -> Result<(), TransferErr> { - for (MaspSourceTransferData { source, token }, amount) in source_data { - if let Some(sk) = - source.spending_key().map(ExtendedSpendingKey::from) - { - // Represents the amount of inputs we are short by - let mut additional = I128Sum::zero(); - for (asset_type, amt) in builder.value_balance().components() { - match amt.cmp(&0) { - Ordering::Greater => { - // Send the change in this asset type back to the - // sender - builder - .add_sapling_output( - Some(sk.expsk.ovk), - sk.default_address().1, - *asset_type, - *amt as u64, - MemoBytes::empty(), - ) - .map_err(|e| TransferErr::Build { - error: builder::Error::SaplingBuild(e), - data: None, - })?; - } - Ordering::Less => { - // Record how much of the current asset type we are - // short by - additional += - I128Sum::from_nonnegative(*asset_type, -*amt) - .map_err(|()| { - Error::Other(format!( - "from non negative conversion: {}", - line!() - )) - })?; - } - Ordering::Equal => {} - } - } - // If we are short by a non-zero amount, then we have - // insufficient funds - if !additional.is_zero() { - return Result::Err(TransferErr::Build { - error: builder::Error::InsufficientFunds(additional), - data: Some(MaspDataLog { - source: Some(source.to_owned()), - token: token.to_owned(), - amount: *amount, - }), - }); + for (sp, changes) in changes.into_iter() { + for (asset_type, amt) in changes.components() { + if let Ordering::Greater = amt.cmp(&0) { + let sk = ExtendedSpendingKey::from(sp.to_owned()); + // Send the change in this asset type back to the sender + builder + .add_sapling_output( + Some(sk.expsk.ovk), + sk.default_address().1, + *asset_type, + *amt as u64, + MemoBytes::empty(), + ) + .map_err(|e| TransferErr::Build { + error: builder::Error::SaplingBuild(e), + data: None, + })?; } } } + // Final safety check on the value balance to verify that the + // transaction is balanced + let value_balance = builder.value_balance(); + if !value_balance.is_zero() { + return Result::Err(TransferErr::Build { + error: builder::Error::InsufficientFunds(value_balance), + data: None, + }); + } + Ok(()) } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 16aac6aca5..b793240c84 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2466,7 +2466,7 @@ pub async fn build_ibc_transfer( Some(source.clone()), ) .await?; - let (fee_amount, updated_balance) = + let (fee_per_gas_unit, updated_balance) = if let TransferSource::ExtendedSpendingKey(_) = args.source { // MASP fee payment (validate_fee(context, &args.tx).await?, None) @@ -2684,7 +2684,7 @@ pub async fn build_ibc_transfer( prepare_tx( &args.tx, &mut tx, - fee_amount, + fee_per_gas_unit, signing_data.fee_payer.clone(), ) .await?; @@ -2958,7 +2958,7 @@ pub async fn build_shielded_transfer( .await?; // Shielded fee payment - let fee_amount = validate_fee(context, &args.tx).await?; + let fee_per_gas_unit = validate_fee(context, &args.tx).await?; let mut transfer_data = vec![]; for TxShieldedTransferData { @@ -2985,7 +2985,7 @@ pub async fn build_shielded_transfer( let masp_fee_data = get_masp_fee_payment_amount( context, &args.tx, - fee_amount, + fee_per_gas_unit, &signing_data.fee_payer, args.gas_spending_keys.clone(), ) @@ -2996,7 +2996,7 @@ pub async fn build_shielded_transfer( .map(|fee_data| token::UnshieldingTransferData { target: fee_data.target.to_owned(), token: fee_data.token.to_owned(), - amount: fee_amount, + amount: fee_data.amount, }); let shielded_parts = construct_shielded_parts( @@ -3049,7 +3049,7 @@ pub async fn build_shielded_transfer( args.tx_code_path.clone(), data, add_shielded_parts, - fee_amount, + fee_per_gas_unit, &signing_data.fee_payer, ) .await?; @@ -3230,7 +3230,7 @@ pub async fn build_unshielding_transfer( .await?; // Shielded fee payment - let fee_amount = validate_fee(context, &args.tx).await?; + let fee_per_gas_unit = validate_fee(context, &args.tx).await?; let mut transfer_data = vec![]; let mut data = vec![]; @@ -3263,7 +3263,7 @@ pub async fn build_unshielding_transfer( let masp_fee_data = get_masp_fee_payment_amount( context, &args.tx, - fee_amount, + fee_per_gas_unit, &signing_data.fee_payer, args.gas_spending_keys.clone(), ) @@ -3328,7 +3328,7 @@ pub async fn build_unshielding_transfer( args.tx_code_path.clone(), data, add_shielded_parts, - fee_amount, + fee_per_gas_unit, &signing_data.fee_payer, ) .await?; diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 634db5ce63..a26eba96b2 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -18,9 +18,9 @@ use test_log::test; use super::setup; use crate::e2e::setup::constants::{ AA_PAYMENT_ADDRESS, AA_VIEWING_KEY, AB_PAYMENT_ADDRESS, AB_VIEWING_KEY, - AC_PAYMENT_ADDRESS, ALBERT, ALBERT_KEY, A_SPENDING_KEY, BB_PAYMENT_ADDRESS, - BERTHA, BERTHA_KEY, BTC, B_SPENDING_KEY, CHRISTEL, CHRISTEL_KEY, ETH, MASP, - NAM, + AC_PAYMENT_ADDRESS, AC_VIEWING_KEY, ALBERT, ALBERT_KEY, A_SPENDING_KEY, + BB_PAYMENT_ADDRESS, BERTHA, BERTHA_KEY, BTC, B_SPENDING_KEY, CHRISTEL, + CHRISTEL_KEY, ETH, MASP, NAM, }; use crate::strings::TX_APPLIED_SUCCESS; @@ -2175,7 +2175,7 @@ fn dynamic_assets() -> Result<()> { // Test fee payment in masp: // // 1. Masp fee payment runs out of gas -// 3. Valid fee payment (also check that the first tx in the batch is executed +// 2. Valid fee payment (also check that the first tx in the batch is executed // only once) #[test] fn masp_fee_payment() -> Result<()> { @@ -2187,7 +2187,7 @@ fn masp_fee_payment() -> Result<()> { genesis.parameters.parameters.masp_fee_payment_gas_limit = 20_000; genesis })?; - _ = node.next_epoch(); + _ = node.next_masp_epoch(); // Add the relevant viewing keys to the wallet otherwise the shielded // context won't precache the masp data @@ -2223,7 +2223,7 @@ fn masp_fee_payment() -> Result<()> { &node, Bin::Client, vec![ - "transfer", + "shield", "--source", ALBERT_KEY, "--target", @@ -2237,6 +2237,7 @@ fn masp_fee_payment() -> Result<()> { ], )?; node.assert_success(); + _ = node.next_masp_epoch(); // sync shielded context run( &node, @@ -2262,29 +2263,33 @@ fn masp_fee_payment() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 50000")); - _ = node.next_masp_epoch(); - // 1. Out of gas for masp fee payment - run( - &node, - Bin::Client, - vec![ - "transfer", - "--source", - A_SPENDING_KEY, - "--target", - AB_PAYMENT_ADDRESS, - "--token", - NAM, - "--amount", - "1", - "--gas-limit", - "5000", - "--ledger-address", - validator_one_rpc, - ], - )?; - node.assert_success(); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "transfer", + "--source", + A_SPENDING_KEY, + "--target", + AB_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "1", + "--gas-limit", + "2000", + "--gas-price", + "1", + "--disposable-gas-payer", + "--ledger-address", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_err()); + _ = node.next_masp_epoch(); // sync shielded context run( &node, @@ -2309,8 +2314,6 @@ fn masp_fee_payment() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 50000")); - _ = node.next_masp_epoch(); - // 2. Valid masp fee payment run( &node, @@ -2329,6 +2332,7 @@ fn masp_fee_payment() -> Result<()> { "20000", "--gas-price", "1", + "--disposable-gas-payer", "--ledger-address", validator_one_rpc, ], @@ -2342,7 +2346,7 @@ fn masp_fee_payment() -> Result<()> { )?; node.assert_success(); // Check the exact balance of the tx source to ensure that the masp fee - // payement transaction was executed only once + // payment transaction was executed only once let captured = CapturedOutput::of(|| { run( &node, @@ -2360,6 +2364,23 @@ fn masp_fee_payment() -> Result<()> { }); assert!(captured.result.is_ok()); assert!(captured.contains("nam: 20000")); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AB_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 10000")); Ok(()) } @@ -2375,7 +2396,7 @@ fn masp_fee_payment_gas_limit() -> Result<()> { let (mut node, _services) = setup::initialize_genesis(|mut genesis| { // Set an insufficient gas limit for masp fee payment to force all // transactions to fail - genesis.parameters.parameters.masp_fee_payment_gas_limit = 5_000; + genesis.parameters.parameters.masp_fee_payment_gas_limit = 3_000; genesis })?; _ = node.next_masp_epoch(); @@ -2414,11 +2435,11 @@ fn masp_fee_payment_gas_limit() -> Result<()> { &node, Bin::Client, vec![ - "transfer", + "shield", "--source", - A_SPENDING_KEY, + ALBERT_KEY, "--target", - AB_PAYMENT_ADDRESS, + AA_PAYMENT_ADDRESS, "--token", NAM, "--amount", @@ -2429,7 +2450,34 @@ fn masp_fee_payment_gas_limit() -> Result<()> { )?; node.assert_success(); - _ = node.next_epoch(); + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + // Check that the balance hasn't changed + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 1000000")); // Masp fee payment with huge gas, check that the tx still fails because of // the protocol param @@ -2438,9 +2486,9 @@ fn masp_fee_payment_gas_limit() -> Result<()> { &node, Bin::Client, vec![ - "transfer", + "unshield", "--source", - ALBERT_KEY, + A_SPENDING_KEY, "--target", BERTHA, "--token", @@ -2451,6 +2499,7 @@ fn masp_fee_payment_gas_limit() -> Result<()> { "100000", "--gas-price", "1", + "--disposable-gas-payer", "--ledger-address", validator_one_rpc, ], @@ -2459,7 +2508,7 @@ fn masp_fee_payment_gas_limit() -> Result<()> { assert!(captured.result.is_err()); node.assert_success(); - _ = node.next_epoch(); + _ = node.next_masp_epoch(); // sync shielded context run( @@ -2490,3 +2539,700 @@ fn masp_fee_payment_gas_limit() -> Result<()> { Ok(()) } + +// Test masp fee payement with an unshield to a non-disposable address with +// already some funds on it. +#[test] +fn masp_fee_payment_with_non_disposable() -> Result<()> { + // This address doesn't matter for tests. But an argument is required. + let validator_one_rpc = "http://127.0.0.1:26567"; + // Download the shielded pool parameters before starting node + let _ = FsShieldedUtils::new(PathBuf::new()); + let (mut node, _services) = setup::initialize_genesis(|mut genesis| { + genesis.parameters.parameters.masp_fee_payment_gas_limit = 20_000; + genesis + })?; + _ = node.next_masp_epoch(); + + // Add the relevant viewing keys to the wallet otherwise the shielded + // context won't precache the masp data + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_a", + "--value", + AA_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_b", + "--value", + AB_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + + // Shield some tokens + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT_KEY, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + // Drain balance of fee payer + "1999999", + // Pay gas transparently + "--gas-payer", + BERTHA_KEY, + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 1999999")); + + // Masp fee payment to non-disposable address + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "unshield", + "--source", + A_SPENDING_KEY, + "--target", + BERTHA, + "--token", + NAM, + "--amount", + "1", + "--gas-limit", + "20000", + "--gas-price", + "1", + "--gas-payer", + ALBERT_KEY, + "--ledger-address", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + node.assert_success(); + + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 1979999")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + ALBERT_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0")); + + Ok(()) +} + +// Test masp fee payement with a custom provided spending key. Check that fees +// are splitted between the actual source of the payment and this gas spending +// key +#[test] +fn masp_fee_payment_with_custom_spending_key() -> Result<()> { + // This address doesn't matter for tests. But an argument is required. + let validator_one_rpc = "http://127.0.0.1:26567"; + // Download the shielded pool parameters before starting node + let _ = FsShieldedUtils::new(PathBuf::new()); + let (mut node, _services) = setup::initialize_genesis(|mut genesis| { + genesis.parameters.parameters.masp_fee_payment_gas_limit = 20_000; + genesis + })?; + _ = node.next_masp_epoch(); + + // Add the relevant viewing keys to the wallet otherwise the shielded + // context won't precache the masp data + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_a", + "--value", + AA_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_b", + "--value", + AB_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_c", + "--value", + AC_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + + // Shield some tokens + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT_KEY, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "10000", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT_KEY, + "--target", + AB_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "30000", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 10000")); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AB_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 30000")); + + // Masp fee payment with custom gas payer + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "transfer", + "--source", + A_SPENDING_KEY, + "--target", + AC_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "9000", + "--gas-limit", + "20000", + "--gas-price", + "1", + "--gas-spending-key", + B_SPENDING_KEY, + "--disposable-gas-payer", + "--ledger-address", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + node.assert_success(); + + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AB_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 11000")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AC_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 9000")); + + Ok(()) +} + +// Test masp fee payement with a different token from the one used in the +// transaction itself and with the support of a different key for gas payment +#[test] +fn masp_fee_payment_with_different_token() -> Result<()> { + // This address doesn't matter for tests. But an argument is required. + let validator_one_rpc = "http://127.0.0.1:26567"; + // Download the shielded pool parameters before starting node + let _ = FsShieldedUtils::new(PathBuf::new()); + let (mut node, _services) = setup::initialize_genesis(|mut genesis| { + genesis.parameters.parameters.masp_fee_payment_gas_limit = 20_000; + // Whitelist BTC for gas payment + genesis.parameters.parameters.minimum_gas_price.insert( + "btc".into(), + DenominatedAmount::new(1.into(), namada::token::Denomination(6)), + ); + genesis + })?; + _ = node.next_masp_epoch(); + + // Add the relevant viewing keys to the wallet otherwise the shielded + // context won't precache the masp data + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_a", + "--value", + AA_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_b", + "--value", + AB_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + + // Shield some tokens + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT_KEY, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "1", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + BTC, + "--amount", + "1000", + "--gas-payer", + ALBERT_KEY, + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT, + "--target", + AB_PAYMENT_ADDRESS, + "--token", + BTC, + "--amount", + "20000", + "--gas-payer", + ALBERT_KEY, + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 1")); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + BTC, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("btc: 1000")); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AB_VIEWING_KEY, + "--token", + BTC, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("btc: 20000")); + + // Masp fee payment with custom token and gas payer + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "transfer", + "--source", + A_SPENDING_KEY, + "--target", + AB_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "1", + "--gas-token", + BTC, + "--gas-limit", + "20000", + "--gas-price", + "1", + "--gas-spending-key", + B_SPENDING_KEY, + "--disposable-gas-payer", + "--ledger-address", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + node.assert_success(); + + _ = node.next_masp_epoch(); + + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + node.assert_success(); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AB_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 1")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + BTC, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("btc: 0")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AB_VIEWING_KEY, + "--token", + BTC, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("btc: 1000")); + + Ok(()) +}