Removes MASP shielded fees

apps/src/lib/client/tx.rs

@@ -40,7 +40,7 @@ use namada::ledger::governance::storage as gov_storage;
 use namada::ledger::masp;
 use namada::ledger::pos::{CommissionPair, PosParams};
 use namada::proto::Tx;
-use namada::types::address::{masp, masp_tx_key, Address};
+use namada::types::address::{masp, Address};
 use namada::types::governance::{
     OfflineProposal, OfflineVote, Proposal, ProposalVote,
 };
@@ -69,7 +69,7 @@ use super::types::ShieldedTransferContext;
 use crate::cli::context::WalletAddress;
 use crate::cli::{args, safe_exit, Context};
 use crate::client::rpc::{query_conversion, query_storage_value};
-use crate::client::signing::{find_keypair, sign_tx, tx_signer, TxSigningKey};
+use crate::client::signing::{find_keypair, sign_tx, TxSigningKey};
 use crate::client::tendermint_rpc_types::{TxBroadcastData, TxResponse};
 use crate::client::types::ParsedTxTransferArgs;
 use crate::facade::tendermint_config::net::Address as TendermintAddress;
@@ -1353,7 +1353,6 @@ fn convert_amount(
 pub async fn gen_shielded_transfer<C>(
     ctx: &mut C,
     args: &ParsedTxTransferArgs,
-    shielded_gas: bool,
 ) -> Result<Option<(Transaction, TransactionMetadata)>, builder::Error>
 where
     C: ShieldedTransferContext,
@@ -1372,26 +1371,20 @@ where
     // Convert transaction amount into MASP types
     let (asset_type, amount) = convert_amount(epoch, &args.token, args.amount);
 
+    // Fees are always paid outside of MASP
+    builder.set_fee(Amount::zero())?;
+
     // Transactions with transparent input and shielded output
     // may be affected if constructed close to epoch boundary
     let mut epoch_sensitive: bool = false;
     // If there are shielded inputs
     if let Some(sk) = spending_key {
-        // Transaction fees need to match the amount in the wrapper Transfer
-        // when MASP source is used
-        let (_, fee) =
-            convert_amount(epoch, &args.tx.fee_token, args.tx.fee_amount);
-        builder.set_fee(fee.clone())?;
-        // FIXME: fix gas here?
-        // If the gas is coming from the shielded pool, then our shielded inputs
-        // must also cover the gas fee
-        let required_amt = if shielded_gas { amount + fee } else { amount };
         // Locate unspent notes that can help us meet the transaction amount
         let (_, unspent_notes, used_convs) = ctx
             .collect_unspent_notes(
                 args.tx.ledger_address.clone(),
                 &to_viewing_key(&sk).vk,
-                required_amt,
+                amount,
                 epoch,
             )
             .await;
@@ -1410,10 +1403,6 @@ where
             }
         }
     } else {
-        // No transfer fees come from the shielded transaction for non-MASP
-        // sources
-        //FIXME: fix gas here?
-        builder.set_fee(Amount::zero())?;
         // We add a dummy UTXO to our transaction, but only the source of the
         // parent Transfer object is used to validate fund availability
         let secp_sk =
@@ -1612,37 +1601,15 @@ pub async fn submit_transfer(mut ctx: Context, args: args::TxTransfer) {
     };
 
     let masp_addr = masp();
-    // For MASP sources, use a special sentinel key recognized by VPs as default
-    // signer. Also, if the transaction is shielded, redact the amount and token
+    // For MASP sources, redact the amount and token
     // types by setting the transparent value to 0 and token type to a constant.
     // This has no side-effect because transaction is to self.
-    let (default_signer, amount, token) =
-        if source == masp_addr && target == masp_addr {
-            // TODO Refactor me, we shouldn't rely on any specific token here.
-            (
-                TxSigningKey::SecretKey(masp_tx_key()),
-                0.into(),
-                ctx.native_token.clone(),
-            )
-        } else if source == masp_addr {
-            (
-                TxSigningKey::SecretKey(masp_tx_key()),
-                args.amount,
-                parsed_args.token.clone(),
-            )
-        } else {
-            (
-                TxSigningKey::WalletAddress(args.source.to_address()),
-                args.amount,
-                parsed_args.token.clone(),
-            )
-        };
-    // If our chosen signer is the MASP sentinel key, then our shielded inputs
-    // will need to cover the gas fees.
-    let chosen_signer = tx_signer(&mut ctx, &args.tx, default_signer.clone())
-        .await
-        .ref_to();
-    let shielded_gas = masp_tx_key().ref_to() == chosen_signer;
+    let (amount, token) = if source == masp_addr && target == masp_addr {
+        // TODO Refactor me, we shouldn't rely on any specific token here.
+        (0.into(), ctx.native_token.clone())
+    } else {
+        (args.amount, parsed_args.token.clone())
+    };
     // Determine whether to pin this transaction to a storage key
     let key = match ctx.get(&args.target) {
         TransferTarget::PaymentAddress(pa) if pa.is_pinned() => Some(pa.hash()),
@@ -1682,8 +1649,7 @@ pub async fn submit_transfer(mut ctx: Context, args: args::TxTransfer) {
                 // short-circuited
                 let _ = ctx.shielded.save();
                 let stx_result =
-                    gen_shielded_transfer(&mut ctx, &parsed_args, shielded_gas)
-                        .await;
+                    gen_shielded_transfer(&mut ctx, &parsed_args).await;
                 match stx_result {
                     Ok(stx) => stx.map(|x| x.0),
                     Err(builder::Error::ChangeIsNegative(_)) => {

apps/src/lib/node/ledger/shell/finalize_block.rs

@@ -531,13 +531,8 @@ where
         #[cfg(not(feature = "mainnet"))] has_valid_pow: bool,
     ) -> Result<()> {
         // Charge fee
-        let fee_payer = if wrapper.pk != address::masp_tx_key().ref_to() {
-            wrapper.fee_payer()
-        } else {
-            address::masp() //FIXME: here?
-        };
-
-        let balance_key = token::balance_key(&wrapper.fee.token, &fee_payer);
+        let balance_key =
+            token::balance_key(&wrapper.fee.token, &wrapper.fee_payer());
         let balance: token::Amount = self
             .wl_storage
             .read(&balance_key)

apps/src/lib/node/ledger/shell/mod.rs

@@ -35,7 +35,7 @@ use namada::ledger::storage_api::{self, StorageRead};
 use namada::ledger::{ibc, parameters, pos, protocol, replay_protection};
 use namada::proof_of_stake::{self, read_pos_params, slash};
 use namada::proto::{self, Tx};
-use namada::types::address::{masp, masp_tx_key, Address};
+use namada::types::address::Address;
 use namada::types::chain::ChainId;
 use namada::types::internal::WrapperTxInQueue;
 use namada::types::key::*;
@@ -734,14 +734,9 @@ where
                 return response;
             }
 
-            // Check balance for fee
-            let fee_payer = if wrapper.pk != masp_tx_key().ref_to() {
-                wrapper.fee_payer()
-            } else {
-                masp()
-            };
             // check that the fee payer has sufficient balance
-            let balance = self.get_balance(&wrapper.fee.token, &fee_payer);
+            let balance =
+                self.get_balance(&wrapper.fee.token, &wrapper.fee_payer());
 
             // In testnets with a faucet, tx is allowed to skip fees if
             // it includes a valid PoW
@@ -1166,7 +1161,7 @@ mod test_utils {
             },
             &keypair,
             Epoch(0),
-            0.into(),
+            1.into(),
             tx,
             Default::default(),
             #[cfg(not(feature = "mainnet"))]
@@ -1237,9 +1232,7 @@ mod test_mempool_validate {
     use crate::facade::tendermint_proto::abci::RequestInitChain;
     use namada::proof_of_stake::Epoch;
     use namada::proto::SignedTxData;
-    use namada::types::transaction::{
-        Fee, GasLimit, WrapperTx, GAS_LIMIT_RESOLUTION,
-    };
+    use namada::types::transaction::{Fee, GasLimit, WrapperTx};
 
     use super::test_utils::TestShell;
     use super::{MempoolTxType, *};
@@ -1406,6 +1399,14 @@ mod test_mempool_validate {
     #[test]
     fn test_replay_attack() {
         let (mut shell, _) = TestShell::new();
+        shell.init_chain(RequestInitChain {
+            time: Some(Timestamp {
+                seconds: 0,
+                nanos: 0,
+            }),
+            chain_id: ChainId::default().to_string(),
+            ..Default::default()
+        });
 
         let keypair = super::test_utils::gen_keypair();
 
@@ -1423,7 +1424,7 @@ mod test_mempool_validate {
             },
             &keypair,
             Epoch(0),
-            0.into(),
+            1.into(),
             tx,
             Default::default(),
             #[cfg(not(feature = "mainnet"))]

apps/src/lib/node/ledger/shell/process_proposal.rs

@@ -401,20 +401,9 @@ where
                                 "Couldn't write wrapper tx hash to write log",
                             );
 
-                        // If the public key corresponds to the MASP sentinel
-                        // transaction key, then the fee payer is effectively
-                        // the MASP, otherwise derive
-                        // they payer from public key.
-                        //FIXME: review this part
-                        let fee_payer = if wrapper.pk != masp_tx_key().ref_to()
-                        {
-                            wrapper.fee_payer()
-                        } else {
-                            masp()
-                        };
                         // check that the fee payer has sufficient balance
                         let balance =
-                            self.get_balance(&wrapper.fee.token, &fee_payer);
+                            self.get_balance(&wrapper.fee.token, &wrapper.fee_payer());
 
                         // In testnets, tx is allowed to skip fees if it
                         // includes a valid PoW

benches/mod.rs

@@ -614,11 +614,7 @@ impl BenchShieldedCtx {
             &[],
         ));
         let shielded = async_runtime
-            .block_on(gen_shielded_transfer(
-                self,
-                &mock_parsed_transfer_args,
-                false,
-            ))
+            .block_on(gen_shielded_transfer(self, &mock_parsed_transfer_args))
             .unwrap()
             .map(|x| x.0);