From acd54b88d2eb8b4f5910c47ce1b7e33acac3e083 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Jul 2025 17:54:07 +0200 Subject: [PATCH 01/18] Extends SDK's arguments to include a frontend sustainability fee --- crates/apps_lib/src/cli.rs | 13 +++++++++++-- crates/apps_lib/src/cli/client.rs | 3 ++- crates/sdk/src/args.rs | 32 ++++++++++++++++++++++++++----- crates/sdk/src/lib.rs | 4 ++++ 4 files changed, 44 insertions(+), 8 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 24fbf760f5c..2a801ea20db 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -318,7 +318,9 @@ pub mod cmds { Self::parse_with_ctx(matches, TxShieldingTransfer); let tx_unshielding_transfer = Self::parse_with_ctx(matches, TxUnshieldingTransfer); - let tx_ibc_transfer = Self::parse_with_ctx(matches, TxIbcTransfer); + let tx_ibc_transfer = Self::parse_with_ctx(matches, |cmd| { + TxIbcTransfer(Box::new(cmd)) + }); let tx_osmosis_swap = Self::parse_with_ctx(matches, |cmd| { TxOsmosisSwap(Box::new(cmd)) }); @@ -507,7 +509,7 @@ pub mod cmds { TxShieldedTransfer(TxShieldedTransfer), TxShieldingTransfer(TxShieldingTransfer), TxUnshieldingTransfer(TxUnshieldingTransfer), - TxIbcTransfer(TxIbcTransfer), + TxIbcTransfer(Box), TxOsmosisSwap(Box), QueryResult(QueryResult), TxUpdateAccount(TxUpdateAccount), @@ -4953,6 +4955,7 @@ pub mod args { sources: data, targets, tx_code_path: self.tx_code_path.to_path_buf(), + frontend_sus_fee: None, }) } } @@ -4981,6 +4984,7 @@ pub mod args { sources: data, targets, tx_code_path, + frontend_sus_fee: None, } } @@ -5128,6 +5132,7 @@ pub mod args { ibc_memo: self.ibc_memo, gas_spending_key, tx_code_path: self.tx_code_path.to_path_buf(), + frontend_sus_fee: None, }) } } @@ -5169,6 +5174,8 @@ pub mod args { ibc_memo, gas_spending_key, tx_code_path, + // FIXME: is it ok to skip the frontend fee from the cli? + frontend_sus_fee: None, } } @@ -7231,6 +7238,7 @@ pub mod args { IbcShieldingTransferAsset::Address(chain_ctx.get(&addr)) } }, + frontend_sus_fee: None, }) } } @@ -7266,6 +7274,7 @@ pub mod args { channel_id, token, }, + frontend_sus_fee: None, } } diff --git a/crates/apps_lib/src/cli/client.rs b/crates/apps_lib/src/cli/client.rs index c6a8abc6125..bc24b60adef 100644 --- a/crates/apps_lib/src/cli/client.rs +++ b/crates/apps_lib/src/cli/client.rs @@ -101,7 +101,8 @@ impl CliApi { let namada = ctx.to_sdk(client, io); tx::submit_unshielding_transfer(&namada, args).await?; } - Sub::TxIbcTransfer(TxIbcTransfer(args)) => { + Sub::TxIbcTransfer(args) => { + let TxIbcTransfer(args) = *args; let chain_ctx = ctx.borrow_mut_chain_or_exit(); let ledger_address = chain_ctx.get(&args.tx.ledger_address); diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 99d13c1b49e..8c59935c9c6 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -385,10 +385,13 @@ pub struct TxTransparentSource { pub struct TxShieldingTransfer { /// Common tx arguments pub tx: Tx, - /// Transfer target address + /// Transfer target data pub targets: Vec>, - /// Transfer-specific data + /// Transfer source data pub sources: Vec>, + // FIXME: update the build function to use this + /// The optional data for the frontend sustainability fee + pub frontend_sus_fee: Option>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -432,11 +435,14 @@ pub struct TxTransparentTarget { pub struct TxUnshieldingTransfer { /// Common tx arguments pub tx: Tx, - /// Transfer source spending key + /// Transfer source data pub sources: Vec>, - /// Transfer-specific data + // FIXME: check if we need to do anything when building this tx to handle + // the new fee + /// Transfer target data (potentially also carries data for the frontend + /// sustainability fee) pub targets: Vec>, - /// Optional additional keys for gas payment + /// Optional additional key for gas payment pub gas_spending_key: Option, /// Path to the TX WASM code file pub tx_code_path: PathBuf, @@ -521,11 +527,17 @@ pub enum Slippage { /// An token swap on Osmosis #[derive(Debug, Clone)] pub struct TxOsmosisSwap { + // FIXME: this field contains the frontend fee, make sure to use it in the + // building function /// The IBC transfer data pub transfer: TxIbcTransfer, /// The token we wish to receive (on Namada) pub output_denom: String, /// Address of the recipient on Namada + // FIXME: actually, in the case of an unshield and reshield we might not + // take the fee at all. If either side is shielded we take the fees. If + // both are shielded we should either take the fees only once or not take + // them at all pub recipient: Either, /// Address to receive funds exceeding the minimum amount, /// in case of IBC shieldings @@ -720,6 +732,8 @@ impl TxOsmosisSwap { ), ), expiration: transfer.tx.expiration.clone(), + // The frontend fee should be set in the transfer object + frontend_sus_fee: None, }, ) .await? @@ -819,6 +833,11 @@ pub struct TxIbcTransfer { pub ibc_memo: Option, /// Optional additional keys for gas payment pub gas_spending_key: Option, + // FIXME: can join this with other arguments? I don't think so + // FIXME: update the build function to use this field + // FIXME: can we join this with refund_target? + /// The optional data for the frontend sustainability fee + pub frontend_sus_fee: Option>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -3240,6 +3259,9 @@ pub struct GenIbcShieldingTransfer { pub expiration: TxExpiration, /// Asset to shield over IBC to Namada pub asset: IbcShieldingTransferAsset, + // FIXME: update the build function to use this + /// The optional data for the frontend sustainability fee + pub frontend_sus_fee: Option>, } /// IBC shielding transfer asset, to be used by [`GenIbcShieldingTransfer`] diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index 48132e3d6e1..e54ddbed6d9 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -191,12 +191,14 @@ pub trait Namada: NamadaIo { &self, targets: Vec, sources: Vec, + frontend_sus_fee: Option, ) -> args::TxShieldingTransfer { args::TxShieldingTransfer { sources, targets, tx_code_path: PathBuf::from(TX_TRANSFER_WASM), tx: self.tx_builder(), + frontend_sus_fee, } } @@ -301,6 +303,7 @@ pub trait Namada: NamadaIo { token: Address, amount: InputAmount, channel_id: ChannelId, + frontend_sus_fee: Option, ) -> args::TxIbcTransfer { args::TxIbcTransfer { source, @@ -317,6 +320,7 @@ pub trait Namada: NamadaIo { gas_spending_key: Default::default(), tx: self.tx_builder(), tx_code_path: PathBuf::from(TX_IBC_WASM), + frontend_sus_fee, } } From 14cc724d8acd108fcfcc42c620e70c0303c24048 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 31 Jul 2025 12:49:20 +0200 Subject: [PATCH 02/18] Updates the SDK builder functions to account for the sus fee --- crates/apps_lib/src/cli.rs | 3 +- crates/sdk/src/args.rs | 35 ++++++++------ crates/sdk/src/tx.rs | 97 +++++++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 18 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 2a801ea20db..cefb19d5823 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -5174,7 +5174,6 @@ pub mod args { ibc_memo, gas_spending_key, tx_code_path, - // FIXME: is it ok to skip the frontend fee from the cli? frontend_sus_fee: None, } } @@ -5251,6 +5250,7 @@ pub mod args { route: self.route, osmosis_lcd_rpc: self.osmosis_lcd_rpc, osmosis_sqs_rpc: self.osmosis_sqs_rpc, + frontend_sus_fee: None, }) } } @@ -5303,6 +5303,7 @@ pub mod args { route, osmosis_lcd_rpc, osmosis_sqs_rpc, + frontend_sus_fee: None, } } diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 8c59935c9c6..5bffd3149ff 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -389,7 +389,6 @@ pub struct TxShieldingTransfer { pub targets: Vec>, /// Transfer source data pub sources: Vec>, - // FIXME: update the build function to use this /// The optional data for the frontend sustainability fee pub frontend_sus_fee: Option>, /// Path to the TX WASM code file @@ -437,8 +436,6 @@ pub struct TxUnshieldingTransfer { pub tx: Tx, /// Transfer source data pub sources: Vec>, - // FIXME: check if we need to do anything when building this tx to handle - // the new fee /// Transfer target data (potentially also carries data for the frontend /// sustainability fee) pub targets: Vec>, @@ -524,20 +521,20 @@ pub enum Slippage { }, } +// FIXME: do we need a new event for the frontend fee? +// FIXME: should the fee be taken shielded? It might avoid us to update the ibc +// methods actually. Yes but it would produce a substantial amount of notes with +// very little amounts FIXME: what happens to the sus fee if we wanto to +// shield/unshield more than one asset? It should probably be a vector of +// targets, one for every asset /// An token swap on Osmosis #[derive(Debug, Clone)] pub struct TxOsmosisSwap { - // FIXME: this field contains the frontend fee, make sure to use it in the - // building function /// The IBC transfer data pub transfer: TxIbcTransfer, /// The token we wish to receive (on Namada) pub output_denom: String, /// Address of the recipient on Namada - // FIXME: actually, in the case of an unshield and reshield we might not - // take the fee at all. If either side is shielded we take the fees. If - // both are shielded we should either take the fees only once or not take - // them at all pub recipient: Either, /// Address to receive funds exceeding the minimum amount, /// in case of IBC shieldings @@ -555,6 +552,11 @@ pub struct TxOsmosisSwap { pub osmosis_lcd_rpc: Option, /// REST rpc endpoint to Osmosis SQS pub osmosis_sqs_rpc: Option, + /// The optional data for the frontend sustainability fee + /// NOTE: if the swap is shielded (from MASP to MASP), no sustainability + /// fee should be taken + // FIXME: try to join this with recipient + pub frontend_sus_fee: Option>, } impl TxOsmosisSwap { @@ -622,6 +624,7 @@ impl TxOsmosisSwap { osmosis_lcd_rpc, osmosis_sqs_rpc, output_denom: namada_output_denom, + frontend_sus_fee, } = self; let osmosis_lcd_rpc = osmosis_lcd_rpc @@ -732,8 +735,7 @@ impl TxOsmosisSwap { ), ), expiration: transfer.tx.expiration.clone(), - // The frontend fee should be set in the transfer object - frontend_sus_fee: None, + frontend_sus_fee, }, ) .await? @@ -828,15 +830,17 @@ pub struct TxIbcTransfer { /// Refund target address when the shielded transfer failure pub refund_target: Option, /// IBC shielding transfer data for the destination chain + // FIXME: here the shielding transaction to reapply to namada, it should + // carry the sus fee pub ibc_shielding_data: Option, /// Memo for IBC transfer packet pub ibc_memo: Option, /// Optional additional keys for gas payment pub gas_spending_key: Option, - // FIXME: can join this with other arguments? I don't think so - // FIXME: update the build function to use this field - // FIXME: can we join this with refund_target? /// The optional data for the frontend sustainability fee + // FIXME: this should probably be an either with ibc_shielding_data. Yes + // but there would still be the room for errors, maybe need marker traits? + // Not sure... pub frontend_sus_fee: Option>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, @@ -3259,8 +3263,9 @@ pub struct GenIbcShieldingTransfer { pub expiration: TxExpiration, /// Asset to shield over IBC to Namada pub asset: IbcShieldingTransferAsset, - // FIXME: update the build function to use this /// The optional data for the frontend sustainability fee + /// NOTE: if the shielding operation is part of a swap, and this is + /// shielded (from MASP to MASP), no sustainability fee should be taken pub frontend_sus_fee: Option>, } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 214c30487b7..a1b68d4e2b5 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2782,7 +2782,7 @@ pub async fn build_ibc_transfer( query_wasm_code_hash(context, args.tx_code_path.to_str().unwrap()) .await .map_err(|e| Error::from(QueryError::Wasm(e.to_string())))?; - let masp_transfer_data = MaspTransferData { + let mut masp_transfer_data = MaspTransferData { sources: vec![( args.source.clone(), args.token.clone(), @@ -2827,6 +2827,57 @@ pub async fn build_ibc_transfer( masp_fee_data }; + if let Some(TxTransparentTarget { + target, + token, + amount, + }) = &args.frontend_sus_fee + { + match (&source, &args.ibc_shielding_data) { + (&MASP, None) => { + // Validate the amount given + let validated_amount = validate_amount( + context, + amount.to_owned(), + token, + args.tx.force, + ) + .await?; + masp_transfer_data.targets.push(( + TransferTarget::Address(target.to_owned()), + token.to_owned(), + validated_amount, + )); + + transfer = transfer + .credit( + target.to_owned(), + token.to_owned(), + validated_amount, + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; + } + (&MASP, Some(_)) => { + return Err(Error::Other( + "A frontend sustainability fee was requested but the ibc \ + roundtrip is shielded" + .to_string(), + )); + } + (_, _) => { + return Err(Error::Other( + "A frontend sustainability fee was requested but the ibc \ + source is transparent. If the transaction is a roundtrip \ + (e.g. swap), and the return target is shielded, the fee \ + should be taken from the shielding transaction instead." + .to_string(), + )); + } + } + } + // For transfer from a spending key let shielded_parts = construct_shielded_parts( context, @@ -3422,6 +3473,7 @@ async fn get_masp_fee_payment_amount( } /// Build a shielding transfer +// FIXME: need to test the fee pub async fn build_shielding_transfer( context: &N, args: &mut args::TxShieldingTransfer, @@ -3524,6 +3576,22 @@ pub async fn build_shielding_transfer( .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } + if let Some(TxTransparentTarget { + target, + token, + amount, + }) = &args.frontend_sus_fee + { + // Validate the amount given + let validated_amount = + validate_amount(context, amount.to_owned(), token, args.tx.force) + .await?; + + data = data + .credit(target.to_owned(), token.to_owned(), validated_amount) + .ok_or(Error::Other("Combined transfer overflows".to_string()))?; + } + let shielded_parts = construct_shielded_parts( context, transfer_data, @@ -4149,14 +4217,39 @@ pub async fn gen_ibc_shielding_transfer( .precompute_asset_types(context.client(), tokens) .await; + let extra_target = match &args.frontend_sus_fee { + Some(TxTransparentTarget { + target, + token, + amount, + }) => { + // Validate the amount given + let validated_amount = + validate_amount(context, amount.to_owned(), token, false) + .await?; + + vec![( + TransferTarget::Address(target.to_owned()), + token.to_owned(), + validated_amount, + )] + } + None => vec![], + }; + let masp_transfer_data = MaspTransferData { sources: vec![( TransferSource::Address(source.clone()), token.clone(), validated_amount, )], - targets: vec![(args.target, token.clone(), validated_amount)], + targets: [ + extra_target, + vec![(args.target, token.clone(), validated_amount)], + ] + .concat(), }; + let shielded_transfer = { let mut shielded = context.shielded_mut().await; shielded From 765f3d93cbbe7a48a580f04e5617d9144e2a23f5 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Aug 2025 18:38:24 +0200 Subject: [PATCH 03/18] Integration tests for frontend fees and fixes to the sdk builders --- crates/apps_lib/src/cli.rs | 132 +++++++++++++++++--- crates/sdk/src/tx.rs | 77 ++++++++---- crates/tests/src/integration/masp.rs | 177 +++++++++++++++++++++++++++ 3 files changed, 344 insertions(+), 42 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index cefb19d5823..d1f9685c0f2 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3548,6 +3548,9 @@ pub mod args { pub const FEE_PAYER_OPT: ArgOpt = arg_opt("gas-payer"); pub const FILE_PATH: Arg = arg("file"); pub const FORCE: ArgFlag = flag("force"); + #[cfg(any(test, feature = "testing"))] + pub const FRONTEND_SUS_FEE: ArgOpt = + arg_opt("frontend-sus-fee"); pub const FULL_RESET: ArgFlag = flag("full-reset"); pub const GAS_LIMIT: ArgDefault = arg_default( "gas-limit", @@ -4930,11 +4933,11 @@ pub mod args { ctx: &mut Context, ) -> Result, Self::Error> { let tx = self.tx.to_sdk(ctx)?; - let mut data = vec![]; + let mut sources = vec![]; let chain_ctx = ctx.borrow_mut_chain_or_exit(); for transfer_data in self.sources { - data.push(TxTransparentSource { + sources.push(TxTransparentSource { source: chain_ctx.get(&transfer_data.source), token: chain_ctx.get(&transfer_data.token), amount: transfer_data.amount, @@ -4949,13 +4952,19 @@ pub mod args { amount: transfer_data.amount, }); } + let frontend_sus_fee = + self.frontend_sus_fee.map(|fee| TxTransparentTarget { + target: chain_ctx.get(&fee.target), + token: chain_ctx.get(&fee.token), + amount: fee.amount, + }); Ok(TxShieldingTransfer:: { tx, - sources: data, + sources, targets, tx_code_path: self.tx_code_path.to_path_buf(), - frontend_sus_fee: None, + frontend_sus_fee, }) } } @@ -4966,13 +4975,49 @@ pub mod args { let source = SOURCE.parse(matches); let target = PAYMENT_ADDRESS_TARGET.parse(matches); let token = TOKEN.parse(matches); - let amount = InputAmount::Unvalidated(AMOUNT.parse(matches)); + let raw_amount = AMOUNT.parse(matches); + let amount = InputAmount::Unvalidated(raw_amount); let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); - let data = vec![TxTransparentSource { + + #[cfg(any(test, feature = "testing"))] + let frontend_sus_fee = FRONTEND_SUS_FEE.parse(matches).map( + |target| // Take a constant fee of 1 on top of the input amount + TxTransparentTarget { + target, + token: token.clone(), + amount: InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ), + }, + ); + + #[cfg(not(any(test, feature = "testing")))] + let frontend_sus_fee = None; + + let mut sources = if frontend_sus_fee.is_some() { + // Adjust the inputs to account for the extra token + vec![TxTransparentSource { + source: source.clone(), + token: token.clone(), + amount: InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ), + }] + } else { + vec![] + }; + + sources.push(TxTransparentSource { source, token: token.clone(), amount, - }]; + }); let targets = vec![TxShieldedTarget { target, token, @@ -4981,15 +5026,16 @@ pub mod args { Self { tx, - sources: data, + sources, targets, tx_code_path, - frontend_sus_fee: None, + frontend_sus_fee, } } fn def(app: App) -> App { - app.add_args::>() + let app = app + .add_args::>() .arg(SOURCE.def().help(wrap!( "The transparent source account address. The source's key \ will be used to produce the signature." @@ -5004,7 +5050,15 @@ pub mod args { AMOUNT .def() .help(wrap!("The amount to transfer in decimal.")), - ) + ); + + #[cfg(any(test, feature = "testing"))] + let app = app.arg(FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will take \ + the masp sustainability fee." + ))); + + app } } @@ -5056,31 +5110,63 @@ pub mod args { let source = SPENDING_KEY_SOURCE.parse(matches); let target = TARGET.parse(matches); let token = TOKEN.parse(matches); - let amount = InputAmount::Unvalidated(AMOUNT.parse(matches)); + let raw_amount = AMOUNT.parse(matches); + let amount = InputAmount::Unvalidated(raw_amount); let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); - let data = vec![TxTransparentTarget { - target, + let targets = vec![TxTransparentTarget { + target: target.clone(), token: token.clone(), amount, }]; let sources = vec![TxShieldedSource { - source, - token, + source: source.clone(), + token: token.clone(), amount, }]; let gas_spending_key = GAS_SPENDING_KEY.parse(matches); + #[cfg(any(test, feature = "testing"))] + let mut sources = sources; + #[cfg(any(test, feature = "testing"))] + let mut targets = targets; + #[cfg(any(test, feature = "testing"))] + if let Some(fee_target) = FRONTEND_SUS_FEE.parse(matches) { + // Take a constant fee of 1 on top of the input amount + targets.push(TxTransparentTarget { + target: fee_target, + token: token.clone(), + amount: InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ), + }); + + sources.push(TxShieldedSource { + source, + token, + amount: InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ), + }) + }; + Self { tx, sources, - targets: data, + targets, gas_spending_key, tx_code_path, } } fn def(app: App) -> App { - app.add_args::>() + let app = app + .add_args::>() .arg( SPENDING_KEY_SOURCE .def() @@ -5101,7 +5187,15 @@ pub mod args { "The optional spending key that will be used for gas \ payment. When not provided the source spending key will \ be used." - ))) + ))); + + #[cfg(any(test, feature = "testing"))] + let app = app.arg(FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will take \ + the masp sustainability fee." + ))); + + app } } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index a1b68d4e2b5..dda6a2f2040 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2827,6 +2827,7 @@ pub async fn build_ibc_transfer( masp_fee_data }; + // FIXME: adjust this if let Some(TxTransparentTarget { target, token, @@ -3473,7 +3474,6 @@ async fn get_masp_fee_payment_amount( } /// Build a shielding transfer -// FIXME: need to test the fee pub async fn build_shielding_transfer( context: &N, args: &mut args::TxShieldingTransfer, @@ -3512,11 +3512,14 @@ pub async fn build_shielding_transfer( let mut transfer_data = MaspTransferData::default(); let mut data = token::Transfer::default(); - for TxTransparentSource { - source, - token, - amount, - } in &args.sources + for ( + id, + TxTransparentSource { + source, + token, + amount, + }, + ) in args.sources.iter().enumerate() { // Validate the amount given let validated_amount = @@ -3544,10 +3547,53 @@ pub async fn build_shielding_transfer( .await?; } + // The frontend sustainability fee (when provided) must be paid by the + // first source + let masp_shield_amount = match (id, &args.frontend_sus_fee) { + ( + 0, + Some(TxTransparentTarget { + target: sus_fee_target, + token: sus_fee_token, + amount: sus_fee_amt, + }), + ) => { + if sus_fee_token != token { + return Err(Error::Other( + "The sustainability fee token does not match the \ + token of the first transaction's source" + .to_string(), + )); + } + + // Validate the amount given + let validated_fee_amount = validate_amount( + context, + sus_fee_amt.to_owned(), + sus_fee_token, + args.tx.force, + ) + .await?; + + data = data + .credit( + sus_fee_target.to_owned(), + sus_fee_token.to_owned(), + validated_fee_amount, + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; + + checked!(validated_amount - validated_fee_amount)? + } + _ => validated_amount, + }; + transfer_data.sources.push(( TransferSource::Address(source.to_owned()), token.to_owned(), - validated_amount, + masp_shield_amount, )); data = data @@ -3576,22 +3622,6 @@ pub async fn build_shielding_transfer( .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } - if let Some(TxTransparentTarget { - target, - token, - amount, - }) = &args.frontend_sus_fee - { - // Validate the amount given - let validated_amount = - validate_amount(context, amount.to_owned(), token, args.tx.force) - .await?; - - data = data - .credit(target.to_owned(), token.to_owned(), validated_amount) - .ok_or(Error::Other("Combined transfer overflows".to_string()))?; - } - let shielded_parts = construct_shielded_parts( context, transfer_data, @@ -4217,6 +4247,7 @@ pub async fn gen_ibc_shielding_transfer( .precompute_asset_types(context.client(), tokens) .await; + // FIXME: need to adjust this let extra_target = match &args.frontend_sus_fee { Some(TxTransparentTarget { target, diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 912aaa8ba66..9dfed850b2a 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -10152,3 +10152,180 @@ fn history_with_conversions() -> Result<()> { Ok(()) } + +// Test that shielding and unshielding transactions can pay a small fee to a +// transparent address as a form of sustainability fee for frontend providers +#[test] +fn frontend_sus_fee() -> 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::setup()?; + // Wait till epoch boundary + node.next_masp_epoch(); + + // Initialize address of the frontend provider with no balance + let (frontend_alias, _frontend_key) = + make_temp_account(&node, validator_one_rpc, "Frontend", NAM, 0)?; + + // Test sus fee when shielding. Send 10 NAM from Albert to PA and 1 NAM to a + // transparent address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + ALBERT, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "10", + "--frontend-sus-fee", + frontend_alias, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // sync the shielded context + run( + &node, + Bin::Client, + vec![ + "shielded-sync", + "--viewing-keys", + AA_VIEWING_KEY, + "--node", + validator_one_rpc, + ], + )?; + + // Assert NAM balance at VK(A) is 10 + 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: 10")); + + // Assert NAM balance at the frontend is 1 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + frontend_alias, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 1")); + + // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 1 NAM to + // a transparent address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + ALBERT, + "--token", + NAM, + "--amount", + "9", + "--frontend-sus-fee", + frontend_alias, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // sync the shielded context + run( + &node, + Bin::Client, + vec![ + "shielded-sync", + "--viewing-keys", + AA_VIEWING_KEY, + "--node", + validator_one_rpc, + ], + )?; + + // Assert NAM balance at VK(A) is 0 + 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")); + + // Assert NAM balance at the frontend is 2 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + frontend_alias, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 2")); + + Ok(()) +} From 35777190f7fed604e6d5045018d4f67df83c0dfa Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 12 Aug 2025 16:33:40 +0200 Subject: [PATCH 04/18] Tests masp frontend fee for ibc transactions --- .github/workflows/scripts/e2e.json | 1 + crates/apps/Cargo.toml | 2 + crates/apps_lib/src/cli.rs | 76 +++++++++++++-- crates/sdk/src/args.rs | 9 +- crates/sdk/src/tx.rs | 38 ++++---- crates/tests/src/e2e/ibc_tests.rs | 148 ++++++++++++++++++++++++++++- 6 files changed, 246 insertions(+), 28 deletions(-) diff --git a/.github/workflows/scripts/e2e.json b/.github/workflows/scripts/e2e.json index e542f119779..432ed520db2 100644 --- a/.github/workflows/scripts/e2e.json +++ b/.github/workflows/scripts/e2e.json @@ -1,5 +1,6 @@ { "e2e::eth_bridge_tests::everything": 4, + "e2e::ibc_tests::frontend_sus_fee": 415, "e2e::ibc_tests::ibc_transfers": 414, "e2e::ibc_tests::ibc_nft_transfers": 224, "e2e::ibc_tests::pgf_over_ibc": 415, diff --git a/crates/apps/Cargo.toml b/crates/apps/Cargo.toml index 29b534de9c5..a14ab40e243 100644 --- a/crates/apps/Cargo.toml +++ b/crates/apps/Cargo.toml @@ -54,6 +54,8 @@ mainnet = ["namada_apps_lib/mainnet"] jemalloc = ["namada_node/jemalloc"] migrations = ["namada_apps_lib/migrations"] namada-eth-bridge = ["namada_apps_lib/namada-eth-bridge"] +# FIXME: how to enforce this in ci? +testing = ["namada_apps_lib/testing"] [dependencies] namada_apps_lib.workspace = true diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index d1f9685c0f2..408550c2abc 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3551,6 +3551,9 @@ pub mod args { #[cfg(any(test, feature = "testing"))] pub const FRONTEND_SUS_FEE: ArgOpt = arg_opt("frontend-sus-fee"); + #[cfg(any(test, feature = "testing"))] + pub const FRONTEND_SUS_FEE_IBC: ArgOpt = + arg_opt("frontend-sus-fee-ibc"); pub const FULL_RESET: ArgFlag = flag("full-reset"); pub const GAS_LIMIT: ArgDefault = arg_default( "gas-limit", @@ -5237,7 +5240,9 @@ pub mod args { let source = TRANSFER_SOURCE.parse(matches); let receiver = RECEIVER.parse(matches); let token = TOKEN.parse(matches); - let amount = InputAmount::Unvalidated(AMOUNT.parse(matches)); + + let raw_amount = AMOUNT.parse(matches); + let amount = InputAmount::Unvalidated(raw_amount); let port_id = PORT_ID.parse(matches); let channel_id = CHANNEL_ID.parse(matches); let timeout_height = TIMEOUT_HEIGHT.parse(matches); @@ -5253,6 +5258,28 @@ pub mod args { let ibc_memo = IBC_MEMO.parse(matches); let gas_spending_key = GAS_SPENDING_KEY.parse(matches); let tx_code_path = PathBuf::from(TX_IBC_WASM); + // FIXME: this api is to confusing, split the amount into two, + // source amount and target amount + #[cfg(any(test, feature = "testing"))] + let frontend_sus_fee = + FRONTEND_SUS_FEE.parse(matches).map(|target| + // Take a constant fee of 1 on top of the input amount + TxTransparentTarget { + target, + token: token.clone(), + amount: InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ), + }); + + #[cfg(not(any(test, feature = "testing")))] + let frontend_sus_fee = None; + + eprintln!("AMOUNT IN CLI: {:#?}", amount); //FIXME: remove + Self { tx, source, @@ -5263,12 +5290,13 @@ pub mod args { channel_id, timeout_height, timeout_sec_offset, + // FIXME: check this refund, we should not refund the fee refund_target, ibc_shielding_data, ibc_memo, gas_spending_key, tx_code_path, - frontend_sus_fee: None, + frontend_sus_fee, } } @@ -7312,6 +7340,9 @@ pub mod args { ) -> Result, Self::Error> { let query = self.query.to_sdk(ctx)?; let chain_ctx = ctx.borrow_chain_or_exit(); + let frontend_sus_fee = self + .frontend_sus_fee + .map(|(target, amount)| (chain_ctx.get(&target), amount)); Ok(GenIbcShieldingTransfer:: { query, @@ -7333,7 +7364,7 @@ pub mod args { IbcShieldingTransferAsset::Address(chain_ctx.get(&addr)) } }, - frontend_sus_fee: None, + frontend_sus_fee, }) } } @@ -7344,7 +7375,8 @@ pub mod args { let output_folder = OUTPUT_FOLDER_PATH.parse(matches); let target = TRANSFER_TARGET.parse(matches); let token = TOKEN_STR.parse(matches); - let amount = InputAmount::Unvalidated(AMOUNT.parse(matches)); + let raw_amount = AMOUNT.parse(matches); + let amount = InputAmount::Unvalidated(raw_amount); let expiration = EXPIRATION_OPT.parse(matches); let port_id = PORT_ID.parse(matches); let channel_id = CHANNEL_ID.parse(matches); @@ -7357,6 +7389,27 @@ pub mod args { None => TxExpiration::Default, } }; + // FIXME: this api is to confusing, split the amount into two, + // source amount and target amount + #[cfg(any(test, feature = "testing"))] + let frontend_sus_fee = FRONTEND_SUS_FEE_IBC.parse(matches).map( + |target| // Take a constant fee of 1 on top of the input amount + ( + target, + //FIXME: this means we can't do anything when it comes to nfts for this frontend fee + InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ), + ), + ); + + #[cfg(not(any(test, feature = "testing")))] + let frontend_sus_fee = None; + + eprintln!("AMOUNT IN CLI: {:#?}", amount); //FIXME: remove Self { query, @@ -7369,12 +7422,13 @@ pub mod args { channel_id, token, }, - frontend_sus_fee: None, + frontend_sus_fee, } } fn def(app: App) -> App { - app.add_args::>() + let app = app + .add_args::>() .arg(OUTPUT_FOLDER_PATH.def().help(wrap!( "The output folder path where the artifact will be stored." ))) @@ -7410,7 +7464,15 @@ pub mod args { ))) .arg(CHANNEL_ID.def().help(wrap!( "The channel ID via which the token is received." - ))) + ))); + + #[cfg(any(test, feature = "testing"))] + let app = app.arg(FRONTEND_SUS_FEE_IBC.def().help(wrap!( + "The optional address of the frontend provider that will take \ + the masp sustainability fee." + ))); + + app } } diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 5bffd3149ff..449f00c2d0f 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -556,7 +556,7 @@ pub struct TxOsmosisSwap { /// NOTE: if the swap is shielded (from MASP to MASP), no sustainability /// fee should be taken // FIXME: try to join this with recipient - pub frontend_sus_fee: Option>, + pub frontend_sus_fee: Option<(C::TransferTarget, InputAmount)>, } impl TxOsmosisSwap { @@ -841,6 +841,7 @@ pub struct TxIbcTransfer { // FIXME: this should probably be an either with ibc_shielding_data. Yes // but there would still be the room for errors, maybe need marker traits? // Not sure... + // FIXME: support this in the client for testing only pub frontend_sus_fee: Option>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, @@ -3263,10 +3264,12 @@ pub struct GenIbcShieldingTransfer { pub expiration: TxExpiration, /// Asset to shield over IBC to Namada pub asset: IbcShieldingTransferAsset, - /// The optional data for the frontend sustainability fee + /// The optional data for the frontend sustainability fee (the target and + /// the amount, the token must be the same as the one involved in the + /// shielding transaction since ics-20 only supports a single asset) /// NOTE: if the shielding operation is part of a swap, and this is /// shielded (from MASP to MASP), no sustainability fee should be taken - pub frontend_sus_fee: Option>, + pub frontend_sus_fee: Option<(C::TransferTarget, InputAmount)>, } /// IBC shielding transfer asset, to be used by [`GenIbcShieldingTransfer`] diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index dda6a2f2040..d68be089d92 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -4247,32 +4247,32 @@ pub async fn gen_ibc_shielding_transfer( .precompute_asset_types(context.client(), tokens) .await; - // FIXME: need to adjust this - let extra_target = match &args.frontend_sus_fee { - Some(TxTransparentTarget { - target, - token, - amount, - }) => { + let (extra_target, source_amount) = match &args.frontend_sus_fee { + Some((target, amount)) => { // Validate the amount given - let validated_amount = - validate_amount(context, amount.to_owned(), token, false) + let validated_fee_amount = + validate_amount(context, amount.to_owned(), &token, false) .await?; + let source_amount = + checked!(validated_amount + validated_fee_amount)?; - vec![( - TransferTarget::Address(target.to_owned()), - token.to_owned(), - validated_amount, - )] + ( + vec![( + target.to_owned(), + token.to_owned(), + validated_fee_amount, + )], + source_amount, + ) } - None => vec![], + None => (vec![], validated_amount), }; let masp_transfer_data = MaspTransferData { sources: vec![( TransferSource::Address(source.clone()), token.clone(), - validated_amount, + source_amount, )], targets: [ extra_target, @@ -4281,6 +4281,9 @@ pub async fn gen_ibc_shielding_transfer( .concat(), }; + // eprintln!("MASP TRANSFER DATA: {:#?}", masp_transfer_data); //FIXME: + // remove + let shielded_transfer = { let mut shielded = context.shielded_mut().await; shielded @@ -4296,6 +4299,9 @@ pub async fn gen_ibc_shielding_transfer( .map_err(|err| TxSubmitError::MaspError(err.to_string()))? }; + // eprintln!("GENERATED MASP BUNDLE: {:#?}", shielded_transfer); //FIXME: + // remove + Ok(shielded_transfer.map(|st| st.masp_tx)) } diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 3d3a2298bf5..1cfef2ccf58 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -146,6 +146,7 @@ fn ibc_transfers() -> Result<()> { None, None, false, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -224,6 +225,7 @@ fn ibc_transfers() -> Result<()> { None, None, false, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -254,6 +256,7 @@ fn ibc_transfers() -> Result<()> { 100, &port_id_namada, &channel_id_namada, + None, )?; transfer_from_cosmos( &test_gaia, @@ -305,6 +308,7 @@ fn ibc_transfers() -> Result<()> { None, None, true, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -324,6 +328,7 @@ fn ibc_transfers() -> Result<()> { 1, &port_id_namada, &channel_id_namada, + None, )?; transfer_from_cosmos( &test_gaia, @@ -358,6 +363,7 @@ fn ibc_transfers() -> Result<()> { None, None, false, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -387,6 +393,7 @@ fn ibc_transfers() -> Result<()> { None, None, false, + None, )?; // wait for the timeout sleep(10); @@ -420,6 +427,7 @@ fn ibc_transfers() -> Result<()> { None, None, true, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -451,6 +459,7 @@ fn ibc_transfers() -> Result<()> { None, None, true, + None, )?; // wait for the timeout sleep(10); @@ -506,6 +515,7 @@ fn ibc_transfers() -> Result<()> { 100, &port_id_namada, &channel_id_namada, + None, )?; transfer_from_cosmos( &test_gaia, @@ -610,6 +620,7 @@ fn ibc_nft_transfers() -> Result<()> { None, None, false, + None, )?; clear_packet(&hermes_dir, &port_id_namada, &channel_id_namada, &test)?; check_balance(&test, &namada_receiver, &ibc_trace_on_namada, 0)?; @@ -623,6 +634,7 @@ fn ibc_nft_transfers() -> Result<()> { 1, &port_id_namada, &channel_id_namada, + None, )?; nft_transfer_from_cosmos( &test_cosmwasm, @@ -672,6 +684,7 @@ fn ibc_nft_transfers() -> Result<()> { None, None, true, + None, )?; clear_packet(&hermes_dir, &port_id_namada, &channel_id_namada, &test)?; check_shielded_balance(&test, AB_VIEWING_KEY, &ibc_trace_on_namada, 0)?; @@ -959,6 +972,7 @@ fn ibc_token_inflation() -> Result<()> { 1, &port_id_namada, &channel_id_namada, + None, )?; transfer_from_cosmos( &test_gaia, @@ -1150,6 +1164,7 @@ fn ibc_rate_limit() -> Result<()> { None, None, false, + None, )?; // Transfer 1 NAM from Namada to Gaia again will fail @@ -1170,6 +1185,7 @@ fn ibc_rate_limit() -> Result<()> { ), None, false, + None, )?; // wait for the next epoch @@ -1194,6 +1210,7 @@ fn ibc_rate_limit() -> Result<()> { None, None, false, + None, )?; // wait for the next epoch @@ -1304,6 +1321,7 @@ fn ibc_unlimited_channel() -> Result<()> { ), None, false, + None, )?; // Proposal on Namada @@ -1362,6 +1380,7 @@ fn ibc_unlimited_channel() -> Result<()> { None, None, false, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -1693,6 +1712,7 @@ fn ibc_pfm_happy_flows() -> Result<()> { None, None, false, + None, )?; wait_for_packet_relay( @@ -1990,6 +2010,7 @@ fn ibc_pfm_unhappy_flows() -> Result<()> { None, None, false, + None, )?; wait_for_packet_relay( @@ -2314,6 +2335,7 @@ fn ibc_shielded_recv_middleware_happy_flow() -> Result<()> { 8, &port_id_namada, &channel_id_namada, + None, )?; let masp_receiver = match iter { // Test addresses encoded using `bech32m`... @@ -2353,6 +2375,7 @@ fn ibc_shielded_recv_middleware_happy_flow() -> Result<()> { None, Some(&memo), true, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -2419,6 +2442,7 @@ fn ibc_shielded_recv_middleware_unhappy_flow() -> Result<()> { 8, &port_id_namada, &channel_id_namada, + None, )?; let memo = packet_forward_memo( MASP.to_string().into(), @@ -2445,6 +2469,7 @@ fn ibc_shielded_recv_middleware_unhappy_flow() -> Result<()> { None, Some(&memo), false, + None, )?; wait_for_packet_relay(&hermes_dir, &port_id_gaia, &channel_id_gaia, &test)?; @@ -2703,6 +2728,7 @@ fn try_invalid_transfers( Some(&format!("Invalid IBC denom: {nam_addr}")), None, false, + None, )?; // invalid channel @@ -2720,6 +2746,7 @@ fn try_invalid_transfers( Some("No channel end: port transfer, channel channel-42"), None, false, + None, )?; Ok(()) @@ -2776,6 +2803,7 @@ fn transfer( expected_err: Option<&str>, ibc_memo: Option<&str>, gen_refund_target: bool, + frontend_sus_fee: Option<&str>, ) -> Result { let rpc = get_actor_rpc(test, Who::Validator(0)); @@ -2797,7 +2825,7 @@ fn transfer( "--port-id", &port_id, "--gas-limit", - "60000", + "70000", "--node", &rpc, ]); @@ -2843,6 +2871,11 @@ fn transfer( tx_args.push(IBC_REFUND_TARGET_ALIAS); } + if let Some(target) = frontend_sus_fee { + tx_args.push("--frontend-sus-fee"); + tx_args.push(target.as_ref()); + } + let mut client = run!(test, Bin::Client, tx_args, Some(300))?; match expected_err { Some(err) => { @@ -3464,12 +3497,13 @@ fn gen_ibc_shielding_data( amount: u64, port_id: &PortId, channel_id: &ChannelId, + frontend_sus_fee: Option<&str>, ) -> Result { let rpc = get_actor_rpc(dst_test, Who::Validator(0)); let output_folder = dst_test.test_dir.path().to_string_lossy(); let amount = amount.to_string(); - let args = vec![ + let mut args = vec![ "ibc-gen-shielding", "--output-folder-path", &output_folder, @@ -3486,6 +3520,10 @@ fn gen_ibc_shielding_data( "--node", &rpc, ]; + if let Some(target) = frontend_sus_fee { + args.push("--frontend-sus-fee-ibc"); + args.push(target.as_ref()); + } let mut client = run!(dst_test, Bin::Client, args, Some(120))?; let (_unread, matched) = @@ -3807,6 +3845,111 @@ fn nft_transfer_from_cosmos( Ok(()) } +// Verify that MASP frontend fees can be paid also when doing IBC transactions, +// specifically an unshielding originating from Namada and a shielding tx +// originating from a foreign chain +#[test] +fn frontend_sus_fee() -> Result<()> { + let update_genesis = + |mut genesis: templates::All, base_dir: &_| { + genesis.parameters.parameters.epochs_per_year = + epochs_per_year_from_min_duration(1800); + genesis.parameters.ibc_params.default_mint_limit = + Amount::max_signed(); + genesis + .parameters + .ibc_params + .default_per_epoch_throughput_limit = Amount::max_signed(); + setup::set_validators(1, genesis, base_dir, |_| 0, vec![]) + }; + let (ledger, gaia, test, test_gaia) = + run_namada_cosmos(CosmosChainType::Gaia(None), update_genesis)?; + let _bg_ledger = ledger.background(); + let _bg_gaia = gaia.background(); + + let hermes_dir = setup_hermes(&test, &test_gaia)?; + let port_id_namada = FT_PORT_ID.parse().unwrap(); + let port_id_gaia = FT_PORT_ID.parse().unwrap(); + let (channel_id_namada, channel_id_gaia) = create_channel_with_hermes( + &hermes_dir, + &test, + &test_gaia, + &port_id_namada, + &port_id_gaia, + )?; + + // Start relaying + let hermes = run_hermes(&hermes_dir)?; + let _bg_hermes = hermes.background(); + + // Shielding transfer 100 samoleans from Gaia to Namada (this command will + // actually add another extra token in the output as the frontend sus fee) + let shielding_data_path = gen_ibc_shielding_data( + &test, + AA_PAYMENT_ADDRESS, + COSMOS_COIN, + 100, + &port_id_namada, + &channel_id_namada, + Some(ESTER), + )?; + transfer_from_cosmos( + &test_gaia, + COSMOS_USER, + MASP.to_string(), + COSMOS_COIN, + // 1 extra token for the frontend sus fee + 101, + &port_id_gaia, + &channel_id_gaia, + Some(Either::Left(shielding_data_path)), + None, + )?; + wait_for_packet_relay( + &hermes_dir, + &port_id_gaia, + &channel_id_gaia, + &test_gaia, + )?; + // Check the token on Namada + let ibc_denom_on_namada = + format!("{port_id_namada}/{channel_id_namada}/{COSMOS_COIN}"); + check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 100)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 899)?; + check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; + + // Unshielding transfer 10 samoleans from Namada to Gaia + let gaia_receiver = find_cosmos_address(&test_gaia, COSMOS_USER)?; + transfer( + &test, + A_SPENDING_KEY, + &gaia_receiver, + &ibc_denom_on_namada, + // An extra token will be added to this amount as a frontend masp fee + 10, + Some(BERTHA_KEY), + &port_id_namada, + &channel_id_namada, + None, + None, + None, + None, + true, + Some(ESTER), + )?; + wait_for_packet_relay( + &hermes_dir, + &port_id_namada, + &channel_id_namada, + &test, + )?; + check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 89)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 909)?; + check_balance(&test, ESTER, &ibc_denom_on_namada, 2)?; + + Ok(()) +} + /// Basic Osmosis test that checks if the chain has been set up correctly. #[test] fn osmosis_basic() -> Result<()> { @@ -3930,6 +4073,7 @@ fn osmosis_xcs() -> Result<()> { None, None, false, + None, )?; // Transfer Samoleans from Gaia transfer_from_cosmos( From 1d0872f528d66faae8e46b652dda7c5891c203cb Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Aug 2025 16:48:21 +0200 Subject: [PATCH 05/18] Vectorized frontend sus fee for shielding ops --- crates/apps_lib/src/cli.rs | 41 +++++++++-------- crates/sdk/src/args.rs | 13 +++--- crates/sdk/src/lib.rs | 2 +- crates/sdk/src/tx.rs | 90 ++++++++++++++++---------------------- 4 files changed, 67 insertions(+), 79 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 408550c2abc..36bac200032 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -4955,12 +4955,14 @@ pub mod args { amount: transfer_data.amount, }); } - let frontend_sus_fee = - self.frontend_sus_fee.map(|fee| TxTransparentTarget { + let mut frontend_sus_fee = vec![]; + for fee in self.frontend_sus_fee { + frontend_sus_fee.push(TxTransparentTarget { target: chain_ctx.get(&fee.target), token: chain_ctx.get(&fee.token), amount: fee.amount, }); + } Ok(TxShieldingTransfer:: { tx, @@ -4983,24 +4985,28 @@ pub mod args { let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); #[cfg(any(test, feature = "testing"))] - let frontend_sus_fee = FRONTEND_SUS_FEE.parse(matches).map( - |target| // Take a constant fee of 1 on top of the input amount - TxTransparentTarget { - target, - token: token.clone(), - amount: InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), + let frontend_sus_fee = + FRONTEND_SUS_FEE.parse(matches).map_or(vec![], |target| { + vec![TxTransparentTarget { + target, + token: token.clone(), + amount: InputAmount::Unvalidated( + token::DenominatedAmount::new( + // Take a constant fee of 1 on top of the input + // amount + 1.into(), + raw_amount.denom(), ), - }, - ); + ), + }] + }); #[cfg(not(any(test, feature = "testing")))] - let frontend_sus_fee = None; + let frontend_sus_fee = vec![]; - let mut sources = if frontend_sus_fee.is_some() { + let mut sources = if frontend_sus_fee.is_empty() { + vec![] + } else { // Adjust the inputs to account for the extra token vec![TxTransparentSource { source: source.clone(), @@ -5012,8 +5018,6 @@ pub mod args { ), ), }] - } else { - vec![] }; sources.push(TxTransparentSource { @@ -5128,6 +5132,7 @@ pub mod args { }]; let gas_spending_key = GAS_SPENDING_KEY.parse(matches); + // FIXME: single cfg here #[cfg(any(test, feature = "testing"))] let mut sources = sources; #[cfg(any(test, feature = "testing"))] diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 449f00c2d0f..6d8f377c0b3 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -390,7 +390,7 @@ pub struct TxShieldingTransfer { /// Transfer source data pub sources: Vec>, /// The optional data for the frontend sustainability fee - pub frontend_sus_fee: Option>, + pub frontend_sus_fee: Vec>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -521,12 +521,6 @@ pub enum Slippage { }, } -// FIXME: do we need a new event for the frontend fee? -// FIXME: should the fee be taken shielded? It might avoid us to update the ibc -// methods actually. Yes but it would produce a substantial amount of notes with -// very little amounts FIXME: what happens to the sus fee if we wanto to -// shield/unshield more than one asset? It should probably be a vector of -// targets, one for every asset /// An token swap on Osmosis #[derive(Debug, Clone)] pub struct TxOsmosisSwap { @@ -841,7 +835,9 @@ pub struct TxIbcTransfer { // FIXME: this should probably be an either with ibc_shielding_data. Yes // but there would still be the room for errors, maybe need marker traits? // Not sure... - // FIXME: support this in the client for testing only + // FIXME: should this be a tuple (receiver, amount) to force the token to + // be the same as that of the transfer? FIXME: should the frontend fees + // always be specified just as a percentage and a target address? pub frontend_sus_fee: Option>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, @@ -3269,6 +3265,7 @@ pub struct GenIbcShieldingTransfer { /// shielding transaction since ics-20 only supports a single asset) /// NOTE: if the shielding operation is part of a swap, and this is /// shielded (from MASP to MASP), no sustainability fee should be taken + // FIXME: transparent target only pub frontend_sus_fee: Option<(C::TransferTarget, InputAmount)>, } diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index e54ddbed6d9..ac7c9cc24e1 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -191,7 +191,7 @@ pub trait Namada: NamadaIo { &self, targets: Vec, sources: Vec, - frontend_sus_fee: Option, + frontend_sus_fee: Vec, ) -> args::TxShieldingTransfer { args::TxShieldingTransfer { sources, diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index d68be089d92..ee4c8993fbd 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3512,14 +3512,11 @@ pub async fn build_shielding_transfer( let mut transfer_data = MaspTransferData::default(); let mut data = token::Transfer::default(); - for ( - id, - TxTransparentSource { - source, - token, - amount, - }, - ) in args.sources.iter().enumerate() + for TxTransparentSource { + source, + token, + amount, + } in &args.sources { // Validate the amount given let validated_amount = @@ -3547,53 +3544,10 @@ pub async fn build_shielding_transfer( .await?; } - // The frontend sustainability fee (when provided) must be paid by the - // first source - let masp_shield_amount = match (id, &args.frontend_sus_fee) { - ( - 0, - Some(TxTransparentTarget { - target: sus_fee_target, - token: sus_fee_token, - amount: sus_fee_amt, - }), - ) => { - if sus_fee_token != token { - return Err(Error::Other( - "The sustainability fee token does not match the \ - token of the first transaction's source" - .to_string(), - )); - } - - // Validate the amount given - let validated_fee_amount = validate_amount( - context, - sus_fee_amt.to_owned(), - sus_fee_token, - args.tx.force, - ) - .await?; - - data = data - .credit( - sus_fee_target.to_owned(), - sus_fee_token.to_owned(), - validated_fee_amount, - ) - .ok_or(Error::Other( - "Combined transfer overflows".to_string(), - ))?; - - checked!(validated_amount - validated_fee_amount)? - } - _ => validated_amount, - }; - transfer_data.sources.push(( TransferSource::Address(source.to_owned()), token.to_owned(), - masp_shield_amount, + validated_amount, )); data = data @@ -3622,6 +3576,38 @@ pub async fn build_shielding_transfer( .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } + for args::TxTransparentTarget { + target: sus_fee_target, + token: sus_fee_token, + amount: sus_fee_amt, + } in &args.frontend_sus_fee + { + // Validate the amount given + let validated_fee_amount = validate_amount( + context, + sus_fee_amt.to_owned(), + sus_fee_token, + args.tx.force, + ) + .await?; + + // Decrease the amount to shield to account for this frontend fee, take + // it from the first shielding input that matches the fee token + for (_, token, amount) in &mut transfer_data.sources { + if token == sus_fee_token { + *amount = checked!(amount - validated_fee_amount)?; + break; + } + } + data = data + .credit( + sus_fee_target.to_owned(), + sus_fee_token.to_owned(), + validated_fee_amount, + ) + .ok_or(Error::Other("Combined transfer overflows".to_string()))?; + } + let shielded_parts = construct_shielded_parts( context, transfer_data, From b33f48b4e0981894478d7af3885356c3c50cac5b Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Aug 2025 16:57:27 +0200 Subject: [PATCH 06/18] Updates frontend fee interface --- .github/workflows/scripts/e2e.json | 2 +- crates/apps/Cargo.toml | 2 - crates/apps_lib/src/cli.rs | 142 ++++++++++++----------------- crates/sdk/src/args.rs | 14 +-- crates/sdk/src/tx.rs | 9 +- 5 files changed, 60 insertions(+), 109 deletions(-) diff --git a/.github/workflows/scripts/e2e.json b/.github/workflows/scripts/e2e.json index 432ed520db2..d965f0bdc55 100644 --- a/.github/workflows/scripts/e2e.json +++ b/.github/workflows/scripts/e2e.json @@ -1,6 +1,6 @@ { "e2e::eth_bridge_tests::everything": 4, - "e2e::ibc_tests::frontend_sus_fee": 415, + "e2e::ibc_tests::frontend_sus_fee": 350, "e2e::ibc_tests::ibc_transfers": 414, "e2e::ibc_tests::ibc_nft_transfers": 224, "e2e::ibc_tests::pgf_over_ibc": 415, diff --git a/crates/apps/Cargo.toml b/crates/apps/Cargo.toml index a14ab40e243..29b534de9c5 100644 --- a/crates/apps/Cargo.toml +++ b/crates/apps/Cargo.toml @@ -54,8 +54,6 @@ mainnet = ["namada_apps_lib/mainnet"] jemalloc = ["namada_node/jemalloc"] migrations = ["namada_apps_lib/migrations"] namada-eth-bridge = ["namada_apps_lib/namada-eth-bridge"] -# FIXME: how to enforce this in ci? -testing = ["namada_apps_lib/testing"] [dependencies] namada_apps_lib.workspace = true diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 36bac200032..d85625ac9fb 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3548,12 +3548,6 @@ pub mod args { pub const FEE_PAYER_OPT: ArgOpt = arg_opt("gas-payer"); pub const FILE_PATH: Arg = arg("file"); pub const FORCE: ArgFlag = flag("force"); - #[cfg(any(test, feature = "testing"))] - pub const FRONTEND_SUS_FEE: ArgOpt = - arg_opt("frontend-sus-fee"); - #[cfg(any(test, feature = "testing"))] - pub const FRONTEND_SUS_FEE_IBC: ArgOpt = - arg_opt("frontend-sus-fee-ibc"); pub const FULL_RESET: ArgFlag = flag("full-reset"); pub const GAS_LIMIT: ArgDefault = arg_default( "gas-limit", @@ -3699,6 +3693,14 @@ pub mod args { pub const TARGET: Arg = arg("target"); pub const TARGET_OPT: ArgOpt = arg_opt("target"); pub const TEMPLATES_PATH: Arg = arg("templates-path"); + // WARNING: use only for testing purposes, MASP frontend fees don't make + // sense when operating from the CLI + pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = + arg_opt("frontend-sus-fee"); + // WARNING: use only for testing purposes, MASP frontend fees don't make + // sense when operating from the CLI + pub const __TEST_FRONTEND_SUS_FEE_IBC: ArgOpt = + arg_opt("frontend-sus-fee-ibc"); pub const TIMEOUT_HEIGHT: ArgOpt = arg_opt("timeout-height"); pub const TIMEOUT_SEC_OFFSET: ArgOpt = arg_opt("timeout-sec-offset"); pub const TM_ADDRESS_OPT: ArgOpt = arg_opt("tm-address"); @@ -4983,10 +4985,9 @@ pub mod args { let raw_amount = AMOUNT.parse(matches); let amount = InputAmount::Unvalidated(raw_amount); let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); - - #[cfg(any(test, feature = "testing"))] - let frontend_sus_fee = - FRONTEND_SUS_FEE.parse(matches).map_or(vec![], |target| { + let frontend_sus_fee = __TEST_FRONTEND_SUS_FEE + .parse(matches) + .map_or(vec![], |target| { vec![TxTransparentTarget { target, token: token.clone(), @@ -5001,9 +5002,6 @@ pub mod args { }] }); - #[cfg(not(any(test, feature = "testing")))] - let frontend_sus_fee = vec![]; - let mut sources = if frontend_sus_fee.is_empty() { vec![] } else { @@ -5041,8 +5039,7 @@ pub mod args { } fn def(app: App) -> App { - let app = app - .add_args::>() + app.add_args::>() .arg(SOURCE.def().help(wrap!( "The transparent source account address. The source's key \ will be used to produce the signature." @@ -5057,15 +5054,11 @@ pub mod args { AMOUNT .def() .help(wrap!("The amount to transfer in decimal.")), - ); - - #[cfg(any(test, feature = "testing"))] - let app = app.arg(FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will take \ - the masp sustainability fee." - ))); - - app + ) + .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will \ + take the masp sustainability fee." + ))) } } @@ -5120,25 +5113,19 @@ pub mod args { let raw_amount = AMOUNT.parse(matches); let amount = InputAmount::Unvalidated(raw_amount); let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); - let targets = vec![TxTransparentTarget { + let mut targets = vec![TxTransparentTarget { target: target.clone(), token: token.clone(), amount, }]; - let sources = vec![TxShieldedSource { + let mut sources = vec![TxShieldedSource { source: source.clone(), token: token.clone(), amount, }]; let gas_spending_key = GAS_SPENDING_KEY.parse(matches); - // FIXME: single cfg here - #[cfg(any(test, feature = "testing"))] - let mut sources = sources; - #[cfg(any(test, feature = "testing"))] - let mut targets = targets; - #[cfg(any(test, feature = "testing"))] - if let Some(fee_target) = FRONTEND_SUS_FEE.parse(matches) { + if let Some(fee_target) = __TEST_FRONTEND_SUS_FEE.parse(matches) { // Take a constant fee of 1 on top of the input amount targets.push(TxTransparentTarget { target: fee_target, @@ -5161,7 +5148,7 @@ pub mod args { ), ), }) - }; + } Self { tx, @@ -5173,8 +5160,7 @@ pub mod args { } fn def(app: App) -> App { - let app = app - .add_args::>() + app.add_args::>() .arg( SPENDING_KEY_SOURCE .def() @@ -5195,15 +5181,11 @@ pub mod args { "The optional spending key that will be used for gas \ payment. When not provided the source spending key will \ be used." - ))); - - #[cfg(any(test, feature = "testing"))] - let app = app.arg(FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will take \ - the masp sustainability fee." - ))); - - app + ))) + .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will \ + take the masp sustainability fee." + ))) } } @@ -5263,11 +5245,8 @@ pub mod args { let ibc_memo = IBC_MEMO.parse(matches); let gas_spending_key = GAS_SPENDING_KEY.parse(matches); let tx_code_path = PathBuf::from(TX_IBC_WASM); - // FIXME: this api is to confusing, split the amount into two, - // source amount and target amount - #[cfg(any(test, feature = "testing"))] let frontend_sus_fee = - FRONTEND_SUS_FEE.parse(matches).map(|target| + __TEST_FRONTEND_SUS_FEE.parse(matches).map(|target| // Take a constant fee of 1 on top of the input amount TxTransparentTarget { target, @@ -5280,11 +5259,6 @@ pub mod args { ), }); - #[cfg(not(any(test, feature = "testing")))] - let frontend_sus_fee = None; - - eprintln!("AMOUNT IN CLI: {:#?}", amount); //FIXME: remove - Self { tx, source, @@ -5295,7 +5269,6 @@ pub mod args { channel_id, timeout_height, timeout_sec_offset, - // FIXME: check this refund, we should not refund the fee refund_target, ibc_shielding_data, ibc_memo, @@ -5351,6 +5324,15 @@ pub mod args { payment (if this is a shielded action). When not \ provided the source spending key will be used." ))) + .arg( + __TEST_FRONTEND_SUS_FEE + .def() + .conflicts_with(IBC_SHIELDING_DATA_PATH.name) + .help(wrap!( + "The optional address of the frontend provider \ + that will take the masp sustainability fee." + )), + ) } } @@ -7394,27 +7376,20 @@ pub mod args { None => TxExpiration::Default, } }; - // FIXME: this api is to confusing, split the amount into two, - // source amount and target amount - #[cfg(any(test, feature = "testing"))] - let frontend_sus_fee = FRONTEND_SUS_FEE_IBC.parse(matches).map( - |target| // Take a constant fee of 1 on top of the input amount - ( - target, - //FIXME: this means we can't do anything when it comes to nfts for this frontend fee - InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), + let frontend_sus_fee = + __TEST_FRONTEND_SUS_FEE_IBC.parse(matches).map(|target| { + ( + target, + InputAmount::Unvalidated( + token::DenominatedAmount::new( + // Take a constant fee of 1 on top of the input + // amount + 1.into(), + raw_amount.denom(), ), - ), - ); - - #[cfg(not(any(test, feature = "testing")))] - let frontend_sus_fee = None; - - eprintln!("AMOUNT IN CLI: {:#?}", amount); //FIXME: remove + ), + ) + }); Self { query, @@ -7432,8 +7407,7 @@ pub mod args { } fn def(app: App) -> App { - let app = app - .add_args::>() + app.add_args::>() .arg(OUTPUT_FOLDER_PATH.def().help(wrap!( "The output folder path where the artifact will be stored." ))) @@ -7469,15 +7443,11 @@ pub mod args { ))) .arg(CHANNEL_ID.def().help(wrap!( "The channel ID via which the token is received." - ))); - - #[cfg(any(test, feature = "testing"))] - let app = app.arg(FRONTEND_SUS_FEE_IBC.def().help(wrap!( - "The optional address of the frontend provider that will take \ - the masp sustainability fee." - ))); - - app + ))) + .arg(__TEST_FRONTEND_SUS_FEE_IBC.def().help(wrap!( + "The optional address of the frontend provider that will \ + take the masp sustainability fee." + ))) } } diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 6d8f377c0b3..75a55319d0e 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -549,8 +549,7 @@ pub struct TxOsmosisSwap { /// The optional data for the frontend sustainability fee /// NOTE: if the swap is shielded (from MASP to MASP), no sustainability /// fee should be taken - // FIXME: try to join this with recipient - pub frontend_sus_fee: Option<(C::TransferTarget, InputAmount)>, + pub frontend_sus_fee: Option<(C::Address, InputAmount)>, } impl TxOsmosisSwap { @@ -824,20 +823,12 @@ pub struct TxIbcTransfer { /// Refund target address when the shielded transfer failure pub refund_target: Option, /// IBC shielding transfer data for the destination chain - // FIXME: here the shielding transaction to reapply to namada, it should - // carry the sus fee pub ibc_shielding_data: Option, /// Memo for IBC transfer packet pub ibc_memo: Option, /// Optional additional keys for gas payment pub gas_spending_key: Option, /// The optional data for the frontend sustainability fee - // FIXME: this should probably be an either with ibc_shielding_data. Yes - // but there would still be the room for errors, maybe need marker traits? - // Not sure... - // FIXME: should this be a tuple (receiver, amount) to force the token to - // be the same as that of the transfer? FIXME: should the frontend fees - // always be specified just as a percentage and a target address? pub frontend_sus_fee: Option>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, @@ -3265,8 +3256,7 @@ pub struct GenIbcShieldingTransfer { /// shielding transaction since ics-20 only supports a single asset) /// NOTE: if the shielding operation is part of a swap, and this is /// shielded (from MASP to MASP), no sustainability fee should be taken - // FIXME: transparent target only - pub frontend_sus_fee: Option<(C::TransferTarget, InputAmount)>, + pub frontend_sus_fee: Option<(C::Address, InputAmount)>, } /// IBC shielding transfer asset, to be used by [`GenIbcShieldingTransfer`] diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index ee4c8993fbd..9a68206cfb1 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2827,7 +2827,6 @@ pub async fn build_ibc_transfer( masp_fee_data }; - // FIXME: adjust this if let Some(TxTransparentTarget { target, token, @@ -4244,7 +4243,7 @@ pub async fn gen_ibc_shielding_transfer( ( vec![( - target.to_owned(), + TransferTarget::Address(target.to_owned()), token.to_owned(), validated_fee_amount, )], @@ -4267,9 +4266,6 @@ pub async fn gen_ibc_shielding_transfer( .concat(), }; - // eprintln!("MASP TRANSFER DATA: {:#?}", masp_transfer_data); //FIXME: - // remove - let shielded_transfer = { let mut shielded = context.shielded_mut().await; shielded @@ -4285,9 +4281,6 @@ pub async fn gen_ibc_shielding_transfer( .map_err(|err| TxSubmitError::MaspError(err.to_string()))? }; - // eprintln!("GENERATED MASP BUNDLE: {:#?}", shielded_transfer); //FIXME: - // remove - Ok(shielded_transfer.map(|st| st.masp_tx)) } From 4b1398f2e523bae2bbe76609b4f59e0427d6eff6 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 5 Sep 2025 17:06:59 +0200 Subject: [PATCH 07/18] Shielded frontend fee target for incoming IBC packets --- crates/apps_lib/src/cli.rs | 11 +++++++++-- crates/sdk/src/args.rs | 5 +++-- crates/sdk/src/tx.rs | 11 +++++++++-- crates/tests/src/e2e/ibc_tests.rs | 14 +++++++++----- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index d85625ac9fb..46de95b2a4c 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3693,13 +3693,14 @@ pub mod args { pub const TARGET: Arg = arg("target"); pub const TARGET_OPT: ArgOpt = arg_opt("target"); pub const TEMPLATES_PATH: Arg = arg("templates-path"); + // FIXME: add the test prelude in the cli args too // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = arg_opt("frontend-sus-fee"); // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI - pub const __TEST_FRONTEND_SUS_FEE_IBC: ArgOpt = + pub const __TEST_FRONTEND_SUS_FEE_IBC: ArgOpt = arg_opt("frontend-sus-fee-ibc"); pub const TIMEOUT_HEIGHT: ArgOpt = arg_opt("timeout-height"); pub const TIMEOUT_SEC_OFFSET: ArgOpt = arg_opt("timeout-sec-offset"); @@ -5200,6 +5201,12 @@ pub mod args { let chain_ctx = ctx.borrow_mut_chain_or_exit(); let gas_spending_key = self.gas_spending_key.map(|key| chain_ctx.get_cached(&key)); + let frontend_sus_fee = + self.frontend_sus_fee.map(|fee| TxTransparentTarget { + target: chain_ctx.get(&fee.target), + token: chain_ctx.get(&fee.token), + amount: fee.amount, + }); Ok(TxIbcTransfer:: { tx, @@ -5216,7 +5223,7 @@ pub mod args { ibc_memo: self.ibc_memo, gas_spending_key, tx_code_path: self.tx_code_path.to_path_buf(), - frontend_sus_fee: None, + frontend_sus_fee, }) } } diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 75a55319d0e..c32f7a9ea99 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -549,7 +549,7 @@ pub struct TxOsmosisSwap { /// The optional data for the frontend sustainability fee /// NOTE: if the swap is shielded (from MASP to MASP), no sustainability /// fee should be taken - pub frontend_sus_fee: Option<(C::Address, InputAmount)>, + pub frontend_sus_fee: Option<(C::PaymentAddress, InputAmount)>, } impl TxOsmosisSwap { @@ -3244,6 +3244,7 @@ pub struct GenIbcShieldingTransfer { /// The output directory path to where serialize the data pub output_folder: Option, /// The target address + // FIXME: why isn't this a payment address? pub target: C::TransferTarget, /// Transferred token amount pub amount: InputAmount, @@ -3256,7 +3257,7 @@ pub struct GenIbcShieldingTransfer { /// shielding transaction since ics-20 only supports a single asset) /// NOTE: if the shielding operation is part of a swap, and this is /// shielded (from MASP to MASP), no sustainability fee should be taken - pub frontend_sus_fee: Option<(C::Address, InputAmount)>, + pub frontend_sus_fee: Option<(C::PaymentAddress, InputAmount)>, } /// IBC shielding transfer asset, to be used by [`GenIbcShieldingTransfer`] diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 9a68206cfb1..f8905c3ccfb 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2843,6 +2843,12 @@ pub async fn build_ibc_transfer( args.tx.force, ) .await?; + + masp_transfer_data.sources.push(( + args.source.clone(), + args.token.clone(), + validated_amount, + )); masp_transfer_data.targets.push(( TransferTarget::Address(target.to_owned()), token.to_owned(), @@ -2850,7 +2856,8 @@ pub async fn build_ibc_transfer( )); transfer = transfer - .credit( + .transfer( + source.to_owned(), target.to_owned(), token.to_owned(), validated_amount, @@ -4243,7 +4250,7 @@ pub async fn gen_ibc_shielding_transfer( ( vec![( - TransferTarget::Address(target.to_owned()), + TransferTarget::PaymentAddress(target.to_owned()), token.to_owned(), validated_fee_amount, )], diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 1cfef2ccf58..225ffcbc5d1 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -3274,7 +3274,7 @@ fn transfer_from_cosmos( let chain_type = CosmosChainType::chain_type(test.net.chain_id.as_str()).unwrap(); let rpc = format!("tcp://127.0.0.1:{}", chain_type.get_rpc_port_number()); - // If the receiver is a pyament address we want to mask it to the more + // If the receiver is a payment address we want to mask it to the more // general MASP internal address to improve on privacy let receiver = match PaymentAddress::from_str(receiver.as_ref()) { Ok(_) => MASP.to_string(), @@ -3292,6 +3292,8 @@ fn transfer_from_cosmos( sender.as_ref(), "--gas-prices", "0.001stake", + "--gas", + "250000", "--node", &rpc, "--keyring-backend", @@ -3891,7 +3893,7 @@ fn frontend_sus_fee() -> Result<()> { 100, &port_id_namada, &channel_id_namada, - Some(ESTER), + Some(AC_PAYMENT_ADDRESS), )?; transfer_from_cosmos( &test_gaia, @@ -3915,10 +3917,11 @@ fn frontend_sus_fee() -> Result<()> { let ibc_denom_on_namada = format!("{port_id_namada}/{channel_id_namada}/{COSMOS_COIN}"); check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 100)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 1)?; check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 899)?; - check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; - // Unshielding transfer 10 samoleans from Namada to Gaia + // Unshielding transfer 10 samoleans from Namada to Gaia with transparent + // frontend fee FIXME: need to test also the transparent version let gaia_receiver = find_cosmos_address(&test_gaia, COSMOS_USER)?; transfer( &test, @@ -3944,8 +3947,9 @@ fn frontend_sus_fee() -> Result<()> { &test, )?; check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 89)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 1)?; + check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 909)?; - check_balance(&test, ESTER, &ibc_denom_on_namada, 2)?; Ok(()) } From 2418cf553ccda8f89111ac93276b7d4580f7d309 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 6 Sep 2025 12:10:49 +0200 Subject: [PATCH 08/18] Adds support for shielded masp frontend fees --- crates/apps_lib/src/cli.rs | 213 +++++++++++++++++++++++-------------- crates/sdk/src/args.rs | 14 ++- crates/sdk/src/lib.rs | 12 ++- crates/sdk/src/tx.rs | 166 ++++++++++++++++++++++++----- 4 files changed, 290 insertions(+), 115 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 46de95b2a4c..cfd475dd000 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3696,7 +3696,7 @@ pub mod args { // FIXME: add the test prelude in the cli args too // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI - pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = + pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = arg_opt("frontend-sus-fee"); // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI @@ -4960,11 +4960,27 @@ pub mod args { } let mut frontend_sus_fee = vec![]; for fee in self.frontend_sus_fee { - frontend_sus_fee.push(TxTransparentTarget { - target: chain_ctx.get(&fee.target), - token: chain_ctx.get(&fee.token), - amount: fee.amount, - }); + match fee { + Either::Left(transparent_target) => { + frontend_sus_fee.push(Either::Left( + TxTransparentTarget { + target: chain_ctx + .get(&transparent_target.target), + token: chain_ctx.get(&transparent_target.token), + amount: transparent_target.amount, + }, + )); + } + Either::Right(shielded_target) => { + frontend_sus_fee.push(Either::Right( + TxShieldedTarget { + target: chain_ctx.get(&shielded_target.target), + token: chain_ctx.get(&shielded_target.token), + amount: shielded_target.amount, + }, + )); + } + } } Ok(TxShieldingTransfer:: { @@ -4986,49 +5002,40 @@ pub mod args { let raw_amount = AMOUNT.parse(matches); let amount = InputAmount::Unvalidated(raw_amount); let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); + let sources = vec![TxTransparentSource { + source: source.clone(), + token: token.clone(), + amount, + }]; + let targets = vec![TxShieldedTarget { + target, + token: token.clone(), + amount, + }]; let frontend_sus_fee = __TEST_FRONTEND_SUS_FEE .parse(matches) - .map_or(vec![], |target| { - vec![TxTransparentTarget { - target, - token: token.clone(), - amount: InputAmount::Unvalidated( - token::DenominatedAmount::new( - // Take a constant fee of 1 on top of the input - // amount - 1.into(), - raw_amount.denom(), - ), - ), - }] - }); - - let mut sources = if frontend_sus_fee.is_empty() { - vec![] - } else { - // Adjust the inputs to account for the extra token - vec![TxTransparentSource { - source: source.clone(), - token: token.clone(), - amount: InputAmount::Unvalidated( + .map_or(Default::default(), |fee_target| { + // Take a constant fee of 1 on top of the input amount + let amount = InputAmount::Unvalidated( token::DenominatedAmount::new( 1.into(), raw_amount.denom(), ), - ), - }] - }; + ); - sources.push(TxTransparentSource { - source, - token: token.clone(), - amount, - }); - let targets = vec![TxShieldedTarget { - target, - token, - amount, - }]; + vec![match PaymentAddress::from_str(&fee_target.raw) { + Ok(_) => Either::Right(TxShieldedTarget { + target: fee_target.to_payment_address(), + token: token.clone(), + amount, + }), + Err(_) => Either::Left(TxTransparentTarget { + target: fee_target.to_address(), + token, + amount, + }), + }] + }); Self { tx, @@ -5092,6 +5099,31 @@ pub mod args { amount: transfer_data.amount, }); } + + let mut frontend_sus_fee = vec![]; + for fee in self.frontend_sus_fee { + match fee { + Either::Left(transparent_target) => { + frontend_sus_fee.push(Either::Left( + TxTransparentTarget { + target: chain_ctx + .get(&transparent_target.target), + token: chain_ctx.get(&transparent_target.token), + amount: transparent_target.amount, + }, + )); + } + Either::Right(shielded_target) => { + frontend_sus_fee.push(Either::Right( + TxShieldedTarget { + target: chain_ctx.get(&shielded_target.target), + token: chain_ctx.get(&shielded_target.token), + amount: shielded_target.amount, + }, + )); + } + } + } let gas_spending_key = self.gas_spending_key.map(|key| chain_ctx.get_cached(&key)); @@ -5101,6 +5133,7 @@ pub mod args { gas_spending_key, sources, tx_code_path: self.tx_code_path.to_path_buf(), + frontend_sus_fee, }) } } @@ -5114,42 +5147,42 @@ pub mod args { let raw_amount = AMOUNT.parse(matches); let amount = InputAmount::Unvalidated(raw_amount); let tx_code_path = PathBuf::from(TX_TRANSFER_WASM); - let mut targets = vec![TxTransparentTarget { + let targets = vec![TxTransparentTarget { target: target.clone(), token: token.clone(), amount, }]; - let mut sources = vec![TxShieldedSource { + let sources = vec![TxShieldedSource { source: source.clone(), token: token.clone(), amount, }]; let gas_spending_key = GAS_SPENDING_KEY.parse(matches); - if let Some(fee_target) = __TEST_FRONTEND_SUS_FEE.parse(matches) { - // Take a constant fee of 1 on top of the input amount - targets.push(TxTransparentTarget { - target: fee_target, - token: token.clone(), - amount: InputAmount::Unvalidated( + let frontend_sus_fee = __TEST_FRONTEND_SUS_FEE + .parse(matches) + .map_or(Default::default(), |fee_target| { + // Take a constant fee of 1 on top of the input amount + let amount = InputAmount::Unvalidated( token::DenominatedAmount::new( 1.into(), raw_amount.denom(), ), - ), - }); + ); - sources.push(TxShieldedSource { - source, - token, - amount: InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ), - }) - } + vec![match PaymentAddress::from_str(&fee_target.raw) { + Ok(_) => Either::Right(TxShieldedTarget { + target: fee_target.to_payment_address(), + token: token.clone(), + amount, + }), + Err(_) => Either::Left(TxTransparentTarget { + target: fee_target.to_address(), + token, + amount, + }), + }] + }); Self { tx, @@ -5157,6 +5190,7 @@ pub mod args { targets, gas_spending_key, tx_code_path, + frontend_sus_fee, } } @@ -5201,12 +5235,22 @@ pub mod args { let chain_ctx = ctx.borrow_mut_chain_or_exit(); let gas_spending_key = self.gas_spending_key.map(|key| chain_ctx.get_cached(&key)); - let frontend_sus_fee = - self.frontend_sus_fee.map(|fee| TxTransparentTarget { - target: chain_ctx.get(&fee.target), - token: chain_ctx.get(&fee.token), - amount: fee.amount, - }); + let frontend_sus_fee = self.frontend_sus_fee.map(|fee| match fee { + Either::Left(transparent_target) => { + Either::Left(TxTransparentTarget { + target: chain_ctx.get(&transparent_target.target), + token: chain_ctx.get(&transparent_target.token), + amount: transparent_target.amount, + }) + } + Either::Right(shielded_target) => { + Either::Right(TxShieldedTarget { + target: chain_ctx.get(&shielded_target.target), + token: chain_ctx.get(&shielded_target.token), + amount: shielded_target.amount, + }) + } + }); Ok(TxIbcTransfer:: { tx, @@ -5253,18 +5297,27 @@ pub mod args { let gas_spending_key = GAS_SPENDING_KEY.parse(matches); let tx_code_path = PathBuf::from(TX_IBC_WASM); let frontend_sus_fee = - __TEST_FRONTEND_SUS_FEE.parse(matches).map(|target| - // Take a constant fee of 1 on top of the input amount - TxTransparentTarget { - target, + __TEST_FRONTEND_SUS_FEE.parse(matches).map(|fee_target| { + // Take a constant fee of 1 on top of the input amount + let amount = InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ); + match PaymentAddress::from_str(&fee_target.raw) { + Ok(_) => Either::Right(TxShieldedTarget { + target: fee_target.to_payment_address(), token: token.clone(), - amount: InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ), - }); + amount, + }), + Err(_) => Either::Left(TxTransparentTarget { + target: fee_target.to_address(), + token: token.clone(), + amount, + }), + } + }); Self { tx, diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index c32f7a9ea99..06853968575 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -390,7 +390,10 @@ pub struct TxShieldingTransfer { /// Transfer source data pub sources: Vec>, /// The optional data for the frontend sustainability fee - pub frontend_sus_fee: Vec>, + // FIXME: should join this with sources? No maybe better to define a single + // percentage to apply to all inputs and a single receiver + pub frontend_sus_fee: + Vec, TxShieldedTarget>>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -436,11 +439,13 @@ pub struct TxUnshieldingTransfer { pub tx: Tx, /// Transfer source data pub sources: Vec>, - /// Transfer target data (potentially also carries data for the frontend - /// sustainability fee) + /// Transfer target data pub targets: Vec>, /// Optional additional key for gas payment pub gas_spending_key: Option, + /// The optional data for the frontend sustainability fee + pub frontend_sus_fee: + Vec, TxShieldedTarget>>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -829,7 +834,8 @@ pub struct TxIbcTransfer { /// Optional additional keys for gas payment pub gas_spending_key: Option, /// The optional data for the frontend sustainability fee - pub frontend_sus_fee: Option>, + pub frontend_sus_fee: + Option, TxShieldedTarget>>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index ac7c9cc24e1..714e0a7895a 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -191,7 +191,9 @@ pub trait Namada: NamadaIo { &self, targets: Vec, sources: Vec, - frontend_sus_fee: Vec, + frontend_sus_fee: Vec< + either::Either, + >, ) -> args::TxShieldingTransfer { args::TxShieldingTransfer { sources, @@ -209,6 +211,9 @@ pub trait Namada: NamadaIo { sources: Vec, targets: Vec, gas_spending_key: Option, + frontend_sus_fee: Vec< + either::Either, + >, ) -> args::TxUnshieldingTransfer { args::TxUnshieldingTransfer { sources, @@ -216,6 +221,7 @@ pub trait Namada: NamadaIo { gas_spending_key, tx_code_path: PathBuf::from(TX_TRANSFER_WASM), tx: self.tx_builder(), + frontend_sus_fee, } } @@ -303,7 +309,9 @@ pub trait Namada: NamadaIo { token: Address, amount: InputAmount, channel_id: ChannelId, - frontend_sus_fee: Option, + frontend_sus_fee: Option< + either::Either, + >, ) -> args::TxIbcTransfer { args::TxIbcTransfer { source, diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index f8905c3ccfb..8cd113aad89 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -75,7 +75,10 @@ pub use namada_tx::{Authorization, *}; use num_traits::Zero; use rand_core::{OsRng, RngCore}; -use crate::args::{SdkTypes, TxTransparentSource, TxTransparentTarget}; +use crate::args::{ + SdkTypes, TxShieldedSource, TxShieldedTarget, TxTransparentSource, + TxTransparentTarget, +}; use crate::borsh::BorshSerializeExt; use crate::control_flow::time; use crate::error::{EncodingError, Error, QueryError, Result, TxSubmitError}; @@ -2827,12 +2830,20 @@ pub async fn build_ibc_transfer( masp_fee_data }; - if let Some(TxTransparentTarget { - target, - token, - amount, - }) = &args.frontend_sus_fee - { + if let Some(target) = &args.frontend_sus_fee { + let (target, token, amount) = match target { + either::Either::Left(TxTransparentTarget { + target, + token, + amount, + }) => (TransferTarget::Address(target.to_owned()), token, amount), + either::Either::Right(TxShieldedTarget { + target, + token, + amount, + }) => (TransferTarget::PaymentAddress(*target), token, amount), + }; + match (&source, &args.ibc_shielding_data) { (&MASP, None) => { // Validate the amount given @@ -2850,7 +2861,7 @@ pub async fn build_ibc_transfer( validated_amount, )); masp_transfer_data.targets.push(( - TransferTarget::Address(target.to_owned()), + target.clone(), token.to_owned(), validated_amount, )); @@ -2858,7 +2869,7 @@ pub async fn build_ibc_transfer( transfer = transfer .transfer( source.to_owned(), - target.to_owned(), + target.effective_address(), token.to_owned(), validated_amount, ) @@ -3482,7 +3493,7 @@ async fn get_masp_fee_payment_amount( /// Build a shielding transfer pub async fn build_shielding_transfer( context: &N, - args: &mut args::TxShieldingTransfer, + args: &args::TxShieldingTransfer, bparams: &mut impl BuildParams, ) -> Result<(Tx, SigningTxData, MaspEpoch)> { let source = if args.sources.len() == 1 { @@ -3582,12 +3593,20 @@ pub async fn build_shielding_transfer( .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } - for args::TxTransparentTarget { - target: sus_fee_target, - token: sus_fee_token, - amount: sus_fee_amt, - } in &args.frontend_sus_fee - { + for (idx, target) in args.frontend_sus_fee.iter().enumerate() { + let (sus_fee_target, sus_fee_token, sus_fee_amt) = match target { + either::Either::Left(TxTransparentTarget { + target, + token, + amount, + }) => (TransferTarget::Address(target.to_owned()), token, amount), + either::Either::Right(TxShieldedTarget { + target, + token, + amount, + }) => (TransferTarget::PaymentAddress(*target), token, amount), + }; + // Validate the amount given let validated_fee_amount = validate_amount( context, @@ -3597,21 +3616,44 @@ pub async fn build_shielding_transfer( ) .await?; - // Decrease the amount to shield to account for this frontend fee, take - // it from the first shielding input that matches the fee token - for (_, token, amount) in &mut transfer_data.sources { - if token == sus_fee_token { - *amount = checked!(amount - validated_fee_amount)?; - break; + // Transfer the frontend fee, take the amount from the source matching + // the index of this fee entry + match &args.sources.get(idx) { + // FIXME: better to remove the token from the fee argument and just + // use the source one + Some(TxTransparentSource { source, token, .. }) + if token == sus_fee_token => + { + data = data + .transfer( + source.to_owned(), + sus_fee_target.effective_address(), + sus_fee_token.to_owned(), + validated_fee_amount, + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; + + if let TransferTarget::PaymentAddress(_) = sus_fee_target { + // FIXME: in this case also need to update the masp sources + // Add the extra shielding target for the masp frontend + // sustainability fee + transfer_data.targets.push(( + sus_fee_target, + sus_fee_token.to_owned(), + validated_fee_amount, + )); + } + } + _ => { + return Err(Error::Other( + "Token mismatch between shielding input and the \ + corresponding frontend masp fee" + .to_string(), + )); } } - data = data - .credit( - sus_fee_target.to_owned(), - sus_fee_token.to_owned(), - validated_fee_amount, - ) - .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } let shielded_parts = construct_shielded_parts( @@ -3735,6 +3777,72 @@ pub async fn build_unshielding_transfer( .debit(MASP, token.to_owned(), validated_amount) .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } + for (idx, target) in args.frontend_sus_fee.iter().enumerate() { + let (sus_fee_target, sus_fee_token, sus_fee_amt) = match target { + either::Either::Left(TxTransparentTarget { + target, + token, + amount, + }) => (TransferTarget::Address(target.to_owned()), token, amount), + either::Either::Right(TxShieldedTarget { + target, + token, + amount, + }) => (TransferTarget::PaymentAddress(*target), token, amount), + }; + + // Validate the amount given + let validated_fee_amount = validate_amount( + context, + sus_fee_amt.to_owned(), + sus_fee_token, + args.tx.force, + ) + .await?; + + // Transfer the frontend fee, take the amount from the source matching + // the index of this fee entry + match &args.sources.get(idx) { + // FIXME: better to remove the token from the fee argument and just + // use the source one + Some(TxShieldedSource { source, token, .. }) + if token == sus_fee_token => + { + let source = TransferSource::ExtendedKey(*source); + + data = data + .transfer( + source.effective_address(), + sus_fee_target.effective_address(), + sus_fee_token.to_owned(), + validated_fee_amount, + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; + + // Add the extra unshielding source and target for the masp + // frontend sustainability fee + transfer_data.sources.push(( + source, + sus_fee_token.to_owned(), + validated_fee_amount, + )); + transfer_data.targets.push(( + sus_fee_target, + sus_fee_token.to_owned(), + validated_fee_amount, + )); + } + _ => { + return Err(Error::Other( + "Token mismatch between shielding input and the \ + corresponding frontend masp fee" + .to_string(), + )); + } + } + } // Add masp fee payment if necessary let masp_fee_data = if skip_fee_handling { From 8b38f4b810e56053c1a39015866ed2b7a3ca0171 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 6 Sep 2025 14:37:41 +0200 Subject: [PATCH 09/18] Adds tests for shielded masp frontend fees --- crates/apps_lib/src/cli.rs | 215 +++++++++++++++++++-------- crates/sdk/src/tx.rs | 14 +- crates/tests/src/e2e/ibc_tests.rs | 56 ++++++- crates/tests/src/integration/masp.rs | 108 +++++++++++++- 4 files changed, 311 insertions(+), 82 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index cfd475dd000..a1c28b8ec9b 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3693,15 +3693,18 @@ pub mod args { pub const TARGET: Arg = arg("target"); pub const TARGET_OPT: ArgOpt = arg_opt("target"); pub const TEMPLATES_PATH: Arg = arg("templates-path"); - // FIXME: add the test prelude in the cli args too // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI - pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = - arg_opt("frontend-sus-fee"); + pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = + arg_opt("test-frontend-sus-fee"); + // WARNING: use only for testing purposes, MASP frontend fees don't make + // sense when operating from the CLI + pub const __TEST_FRONTEND_SUS_FEE_SHIELDED: ArgOpt = + arg_opt("test-frontend-sus-fee-shielded"); // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI pub const __TEST_FRONTEND_SUS_FEE_IBC: ArgOpt = - arg_opt("frontend-sus-fee-ibc"); + arg_opt("test-frontend-sus-fee-ibc"); pub const TIMEOUT_HEIGHT: ArgOpt = arg_opt("timeout-height"); pub const TIMEOUT_SEC_OFFSET: ArgOpt = arg_opt("timeout-sec-offset"); pub const TM_ADDRESS_OPT: ArgOpt = arg_opt("tm-address"); @@ -5012,9 +5015,9 @@ pub mod args { token: token.clone(), amount, }]; - let frontend_sus_fee = __TEST_FRONTEND_SUS_FEE - .parse(matches) - .map_or(Default::default(), |fee_target| { + let frontend_sus_fee = match __TEST_FRONTEND_SUS_FEE.parse(matches) + { + Some(address) => { // Take a constant fee of 1 on top of the input amount let amount = InputAmount::Unvalidated( token::DenominatedAmount::new( @@ -5023,19 +5026,33 @@ pub mod args { ), ); - vec![match PaymentAddress::from_str(&fee_target.raw) { - Ok(_) => Either::Right(TxShieldedTarget { - target: fee_target.to_payment_address(), - token: token.clone(), - amount, - }), - Err(_) => Either::Left(TxTransparentTarget { - target: fee_target.to_address(), - token, - amount, - }), - }] - }); + vec![Either::Left(TxTransparentTarget { + target: address, + token, + amount, + })] + } + None => { + __TEST_FRONTEND_SUS_FEE_SHIELDED.parse(matches).map_or( + Default::default(), + |payment_address| { + // Take a constant fee of 1 on top of the input + // amount + let amount = InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ); + vec![Either::Right(TxShieldedTarget { + target: payment_address, + token: token.clone(), + amount, + })] + }, + ) + } + }; Self { tx, @@ -5063,10 +5080,26 @@ pub mod args { .def() .help(wrap!("The amount to transfer in decimal.")), ) - .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will \ - take the masp sustainability fee." - ))) + .arg( + __TEST_FRONTEND_SUS_FEE + .def() + .help(wrap!( + "The optional transparent address of the frontend \ + provider that will take the masp sustainability \ + fee." + )) + .conflicts_with(__TEST_FRONTEND_SUS_FEE_SHIELDED.name), + ) + .arg( + __TEST_FRONTEND_SUS_FEE_SHIELDED + .def() + .help(wrap!( + "The optional payment address of the frontend \ + provider that will take the masp sustainability \ + fee." + )) + .conflicts_with(__TEST_FRONTEND_SUS_FEE.name), + ) } } @@ -5159,9 +5192,9 @@ pub mod args { }]; let gas_spending_key = GAS_SPENDING_KEY.parse(matches); - let frontend_sus_fee = __TEST_FRONTEND_SUS_FEE - .parse(matches) - .map_or(Default::default(), |fee_target| { + let frontend_sus_fee = match __TEST_FRONTEND_SUS_FEE.parse(matches) + { + Some(address) => { // Take a constant fee of 1 on top of the input amount let amount = InputAmount::Unvalidated( token::DenominatedAmount::new( @@ -5170,19 +5203,33 @@ pub mod args { ), ); - vec![match PaymentAddress::from_str(&fee_target.raw) { - Ok(_) => Either::Right(TxShieldedTarget { - target: fee_target.to_payment_address(), - token: token.clone(), - amount, - }), - Err(_) => Either::Left(TxTransparentTarget { - target: fee_target.to_address(), - token, - amount, - }), - }] - }); + vec![Either::Left(TxTransparentTarget { + target: address, + token, + amount, + })] + } + None => { + __TEST_FRONTEND_SUS_FEE_SHIELDED.parse(matches).map_or( + Default::default(), + |payment_address| { + // Take a constant fee of 1 on top of the input + // amount + let amount = InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ); + vec![Either::Right(TxShieldedTarget { + target: payment_address, + token: token.clone(), + amount, + })] + }, + ) + } + }; Self { tx, @@ -5217,10 +5264,26 @@ pub mod args { payment. When not provided the source spending key will \ be used." ))) - .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will \ - take the masp sustainability fee." - ))) + .arg( + __TEST_FRONTEND_SUS_FEE + .def() + .help(wrap!( + "The optional transparent address of the frontend \ + provider that will take the masp sustainability \ + fee." + )) + .conflicts_with(__TEST_FRONTEND_SUS_FEE_SHIELDED.name), + ) + .arg( + __TEST_FRONTEND_SUS_FEE_SHIELDED + .def() + .help(wrap!( + "The optional payment address of the frontend \ + provider that will take the masp sustainability \ + fee." + )) + .conflicts_with(__TEST_FRONTEND_SUS_FEE.name), + ) } } @@ -5296,8 +5359,9 @@ pub mod args { let ibc_memo = IBC_MEMO.parse(matches); let gas_spending_key = GAS_SPENDING_KEY.parse(matches); let tx_code_path = PathBuf::from(TX_IBC_WASM); - let frontend_sus_fee = - __TEST_FRONTEND_SUS_FEE.parse(matches).map(|fee_target| { + let frontend_sus_fee = match __TEST_FRONTEND_SUS_FEE.parse(matches) + { + Some(address) => { // Take a constant fee of 1 on top of the input amount let amount = InputAmount::Unvalidated( token::DenominatedAmount::new( @@ -5305,19 +5369,33 @@ pub mod args { raw_amount.denom(), ), ); - match PaymentAddress::from_str(&fee_target.raw) { - Ok(_) => Either::Right(TxShieldedTarget { - target: fee_target.to_payment_address(), - token: token.clone(), - amount, - }), - Err(_) => Either::Left(TxTransparentTarget { - target: fee_target.to_address(), - token: token.clone(), - amount, - }), - } - }); + + Some(Either::Left(TxTransparentTarget { + target: address, + token: token.clone(), + amount, + })) + } + None => { + __TEST_FRONTEND_SUS_FEE_SHIELDED.parse(matches).map( + |payment_address| { + // Take a constant fee of 1 on top of the input + // amount + let amount = InputAmount::Unvalidated( + token::DenominatedAmount::new( + 1.into(), + raw_amount.denom(), + ), + ); + Either::Right(TxShieldedTarget { + target: payment_address, + token: token.clone(), + amount, + }) + }, + ) + } + }; Self { tx, @@ -5387,11 +5465,22 @@ pub mod args { .arg( __TEST_FRONTEND_SUS_FEE .def() - .conflicts_with(IBC_SHIELDING_DATA_PATH.name) .help(wrap!( - "The optional address of the frontend provider \ - that will take the masp sustainability fee." - )), + "The optional transparent address of the frontend \ + provider that will take the masp sustainability \ + fee." + )) + .conflicts_with(__TEST_FRONTEND_SUS_FEE_SHIELDED.name), + ) + .arg( + __TEST_FRONTEND_SUS_FEE_SHIELDED + .def() + .help(wrap!( + "The optional payment address of the frontend \ + provider that will take the masp sustainability \ + fee." + )) + .conflicts_with(__TEST_FRONTEND_SUS_FEE.name), ) } } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 8cd113aad89..8511d604a01 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3619,8 +3619,6 @@ pub async fn build_shielding_transfer( // Transfer the frontend fee, take the amount from the source matching // the index of this fee entry match &args.sources.get(idx) { - // FIXME: better to remove the token from the fee argument and just - // use the source one Some(TxTransparentSource { source, token, .. }) if token == sus_fee_token => { @@ -3636,9 +3634,13 @@ pub async fn build_shielding_transfer( ))?; if let TransferTarget::PaymentAddress(_) = sus_fee_target { - // FIXME: in this case also need to update the masp sources - // Add the extra shielding target for the masp frontend - // sustainability fee + // Add the extra shielding source and target for the masp + // frontend sustainability fee + transfer_data.sources.push(( + TransferSource::Address(source.to_owned()), + sus_fee_token.to_owned(), + validated_fee_amount, + )); transfer_data.targets.push(( sus_fee_target, sus_fee_token.to_owned(), @@ -3803,8 +3805,6 @@ pub async fn build_unshielding_transfer( // Transfer the frontend fee, take the amount from the source matching // the index of this fee entry match &args.sources.get(idx) { - // FIXME: better to remove the token from the fee argument and just - // use the source one Some(TxShieldedSource { source, token, .. }) if token == sus_fee_token => { diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 225ffcbc5d1..307518d7d29 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -2788,6 +2788,11 @@ fn transfer_on_chain( Ok(()) } +enum MaspFrontendFee<'alias> { + Transparent(&'alias str), + Shielded(&'alias str), +} + #[allow(clippy::too_many_arguments)] fn transfer( test: &Test, @@ -2803,7 +2808,7 @@ fn transfer( expected_err: Option<&str>, ibc_memo: Option<&str>, gen_refund_target: bool, - frontend_sus_fee: Option<&str>, + frontend_sus_fee: Option, ) -> Result { let rpc = get_actor_rpc(test, Who::Validator(0)); @@ -2872,8 +2877,16 @@ fn transfer( } if let Some(target) = frontend_sus_fee { - tx_args.push("--frontend-sus-fee"); - tx_args.push(target.as_ref()); + match target { + MaspFrontendFee::Transparent(address) => { + tx_args.push("--test-frontend-sus-fee"); + tx_args.push(address); + } + MaspFrontendFee::Shielded(payment_address) => { + tx_args.push("--test-frontend-sus-fee-shielded"); + tx_args.push(payment_address); + } + } } let mut client = run!(test, Bin::Client, tx_args, Some(300))?; @@ -3523,7 +3536,7 @@ fn gen_ibc_shielding_data( &rpc, ]; if let Some(target) = frontend_sus_fee { - args.push("--frontend-sus-fee-ibc"); + args.push("--test-frontend-sus-fee-ibc"); args.push(target.as_ref()); } @@ -3921,7 +3934,7 @@ fn frontend_sus_fee() -> Result<()> { check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 899)?; // Unshielding transfer 10 samoleans from Namada to Gaia with transparent - // frontend fee FIXME: need to test also the transparent version + // frontend fee let gaia_receiver = find_cosmos_address(&test_gaia, COSMOS_USER)?; transfer( &test, @@ -3938,7 +3951,7 @@ fn frontend_sus_fee() -> Result<()> { None, None, true, - Some(ESTER), + Some(MaspFrontendFee::Transparent(ESTER)), )?; wait_for_packet_relay( &hermes_dir, @@ -3951,6 +3964,37 @@ fn frontend_sus_fee() -> Result<()> { check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 909)?; + // Unshielding transfer 10 samoleans from Namada to Gaia with shielded + // frontend fee + let gaia_receiver = find_cosmos_address(&test_gaia, COSMOS_USER)?; + transfer( + &test, + A_SPENDING_KEY, + &gaia_receiver, + &ibc_denom_on_namada, + // An extra token will be added to this amount as a frontend masp fee + 10, + Some(BERTHA_KEY), + &port_id_namada, + &channel_id_namada, + None, + None, + None, + None, + true, + Some(MaspFrontendFee::Shielded(AC_PAYMENT_ADDRESS)), + )?; + wait_for_packet_relay( + &hermes_dir, + &port_id_namada, + &channel_id_namada, + &test, + )?; + check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 78)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 2)?; + check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 919)?; + Ok(()) } diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 9dfed850b2a..f2b3203920a 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -10185,7 +10185,7 @@ fn frontend_sus_fee() -> Result<()> { NAM, "--amount", "10", - "--frontend-sus-fee", + "--test-frontend-sus-fee", frontend_alias, "--signing-keys", ALBERT_KEY, @@ -10197,6 +10197,34 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains(TX_APPLIED_SUCCESS)); + // Test sus fee when shielding. Send 10 NAM from Albert to PA and 1 NAM to a + // shielded address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + ALBERT, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "10", + "--test-frontend-sus-fee-shielded", + AC_PAYMENT_ADDRESS, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + // sync the shielded context run( &node, @@ -10205,12 +10233,13 @@ fn frontend_sus_fee() -> Result<()> { "shielded-sync", "--viewing-keys", AA_VIEWING_KEY, + AC_VIEWING_KEY, "--node", validator_one_rpc, ], )?; - // Assert NAM balance at VK(A) is 10 + // Assert NAM balance at VK(A) is 20 let captured = CapturedOutput::of(|| { run( &node, @@ -10227,9 +10256,9 @@ fn frontend_sus_fee() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 10")); + assert!(captured.contains("nam: 20")); - // Assert NAM balance at the frontend is 1 + // Assert NAM balance at the transparent frontend is 1 let captured = CapturedOutput::of(|| { run( &node, @@ -10248,6 +10277,25 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 1")); + // Assert NAM balance at the shielded frontend is 1 + 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: 1")); + // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 1 NAM to // a transparent address owned by the frontend provider let captured = CapturedOutput::of(|| { @@ -10264,7 +10312,7 @@ fn frontend_sus_fee() -> Result<()> { NAM, "--amount", "9", - "--frontend-sus-fee", + "--test-frontend-sus-fee", frontend_alias, "--signing-keys", ALBERT_KEY, @@ -10276,6 +10324,34 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains(TX_APPLIED_SUCCESS)); + // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 1 NAM to + // a shielded address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + ALBERT, + "--token", + NAM, + "--amount", + "9", + "--test-frontend-sus-fee-shielded", + AC_PAYMENT_ADDRESS, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + // sync the shielded context run( &node, @@ -10284,6 +10360,7 @@ fn frontend_sus_fee() -> Result<()> { "shielded-sync", "--viewing-keys", AA_VIEWING_KEY, + AC_VIEWING_KEY, "--node", validator_one_rpc, ], @@ -10308,7 +10385,7 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 0")); - // Assert NAM balance at the frontend is 2 + // Assert NAM balance at the transparent frontend is 2 let captured = CapturedOutput::of(|| { run( &node, @@ -10327,5 +10404,24 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 2")); + // Assert NAM balance at the shielded frontend is 2 + 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: 2")); + Ok(()) } From 3cacd32174603d6c1416b2f89f2ba65a93c053b8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 6 Sep 2025 14:49:26 +0200 Subject: [PATCH 10/18] Forces ibc shielding target argument to be a payment address --- crates/apps_lib/src/cli.rs | 8 ++++++-- crates/sdk/src/args.rs | 8 ++------ crates/sdk/src/tx.rs | 6 +++++- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index a1c28b8ec9b..35e529286d7 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -7509,7 +7509,7 @@ pub mod args { fn parse(matches: &ArgMatches) -> Self { let query = Query::parse(matches); let output_folder = OUTPUT_FOLDER_PATH.parse(matches); - let target = TRANSFER_TARGET.parse(matches); + let target = PAYMENT_ADDRESS_TARGET.parse(matches); let token = TOKEN_STR.parse(matches); let raw_amount = AMOUNT.parse(matches); let amount = InputAmount::Unvalidated(raw_amount); @@ -7560,7 +7560,11 @@ pub mod args { .arg(OUTPUT_FOLDER_PATH.def().help(wrap!( "The output folder path where the artifact will be stored." ))) - .arg(TRANSFER_TARGET.def().help(wrap!("The target address."))) + .arg( + PAYMENT_ADDRESS_TARGET + .def() + .help(wrap!("The shielded target account address.")), + ) .arg(TOKEN.def().help(wrap!("The transfer token."))) .arg( AMOUNT diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 06853968575..745e87d0458 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -719,10 +719,7 @@ impl TxOsmosisSwap { ledger_address: transfer.tx.ledger_address.clone(), }, output_folder: None, - target: - namada_core::masp::TransferTarget::PaymentAddress( - payment_addr, - ), + target: payment_addr, asset: IbcShieldingTransferAsset::Address( namada_output_addr, ), @@ -3250,8 +3247,7 @@ pub struct GenIbcShieldingTransfer { /// The output directory path to where serialize the data pub output_folder: Option, /// The target address - // FIXME: why isn't this a payment address? - pub target: C::TransferTarget, + pub target: C::PaymentAddress, /// Transferred token amount pub amount: InputAmount, /// The optional expiration of the masp shielding transaction diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 8511d604a01..3b1bfc4df6c 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -4376,7 +4376,11 @@ pub async fn gen_ibc_shielding_transfer( )], targets: [ extra_target, - vec![(args.target, token.clone(), validated_amount)], + vec![( + TransferTarget::PaymentAddress(args.target), + token.clone(), + validated_amount, + )], ] .concat(), }; From ce7150d9305c7bdb86670cdc4e6ef8d0dec6a809 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sun, 7 Sep 2025 14:01:58 +0200 Subject: [PATCH 11/18] Frontend MASP fee argument as a percentage --- crates/apps_lib/src/cli.rs | 301 ++++------------------- crates/sdk/src/args.rs | 15 +- crates/sdk/src/lib.rs | 12 +- crates/sdk/src/tx.rs | 353 ++++++++++++++++----------- crates/tests/src/e2e/ibc_tests.rs | 41 ++-- crates/tests/src/integration/masp.rs | 21 +- 6 files changed, 286 insertions(+), 457 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 35e529286d7..dde1b70af35 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -3695,14 +3695,10 @@ pub mod args { pub const TEMPLATES_PATH: Arg = arg("templates-path"); // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI - pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = + pub const __TEST_FRONTEND_SUS_FEE: ArgOpt = arg_opt("test-frontend-sus-fee"); // WARNING: use only for testing purposes, MASP frontend fees don't make // sense when operating from the CLI - pub const __TEST_FRONTEND_SUS_FEE_SHIELDED: ArgOpt = - arg_opt("test-frontend-sus-fee-shielded"); - // WARNING: use only for testing purposes, MASP frontend fees don't make - // sense when operating from the CLI pub const __TEST_FRONTEND_SUS_FEE_IBC: ArgOpt = arg_opt("test-frontend-sus-fee-ibc"); pub const TIMEOUT_HEIGHT: ArgOpt = arg_opt("timeout-height"); @@ -4961,30 +4957,10 @@ pub mod args { amount: transfer_data.amount, }); } - let mut frontend_sus_fee = vec![]; - for fee in self.frontend_sus_fee { - match fee { - Either::Left(transparent_target) => { - frontend_sus_fee.push(Either::Left( - TxTransparentTarget { - target: chain_ctx - .get(&transparent_target.target), - token: chain_ctx.get(&transparent_target.token), - amount: transparent_target.amount, - }, - )); - } - Either::Right(shielded_target) => { - frontend_sus_fee.push(Either::Right( - TxShieldedTarget { - target: chain_ctx.get(&shielded_target.target), - token: chain_ctx.get(&shielded_target.token), - amount: shielded_target.amount, - }, - )); - } - } - } + let frontend_sus_fee = + self.frontend_sus_fee.map(|(target, percentage)| { + (chain_ctx.get(&target), percentage) + }); Ok(TxShieldingTransfer:: { tx, @@ -5015,44 +4991,11 @@ pub mod args { token: token.clone(), amount, }]; - let frontend_sus_fee = match __TEST_FRONTEND_SUS_FEE.parse(matches) - { - Some(address) => { - // Take a constant fee of 1 on top of the input amount - let amount = InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ); - - vec![Either::Left(TxTransparentTarget { - target: address, - token, - amount, - })] - } - None => { - __TEST_FRONTEND_SUS_FEE_SHIELDED.parse(matches).map_or( - Default::default(), - |payment_address| { - // Take a constant fee of 1 on top of the input - // amount - let amount = InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ); - vec![Either::Right(TxShieldedTarget { - target: payment_address, - token: token.clone(), - amount, - })] - }, - ) - } - }; + let frontend_sus_fee = + __TEST_FRONTEND_SUS_FEE.parse(matches).map(|target| { + // Take a constant fee of 10% on top of the input amount + (target, Dec::new(1, 1).unwrap()) + }); Self { tx, @@ -5080,26 +5023,10 @@ pub mod args { .def() .help(wrap!("The amount to transfer in decimal.")), ) - .arg( - __TEST_FRONTEND_SUS_FEE - .def() - .help(wrap!( - "The optional transparent address of the frontend \ - provider that will take the masp sustainability \ - fee." - )) - .conflicts_with(__TEST_FRONTEND_SUS_FEE_SHIELDED.name), - ) - .arg( - __TEST_FRONTEND_SUS_FEE_SHIELDED - .def() - .help(wrap!( - "The optional payment address of the frontend \ - provider that will take the masp sustainability \ - fee." - )) - .conflicts_with(__TEST_FRONTEND_SUS_FEE.name), - ) + .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will \ + take the masp sustainability fee." + ))) } } @@ -5133,30 +5060,11 @@ pub mod args { }); } - let mut frontend_sus_fee = vec![]; - for fee in self.frontend_sus_fee { - match fee { - Either::Left(transparent_target) => { - frontend_sus_fee.push(Either::Left( - TxTransparentTarget { - target: chain_ctx - .get(&transparent_target.target), - token: chain_ctx.get(&transparent_target.token), - amount: transparent_target.amount, - }, - )); - } - Either::Right(shielded_target) => { - frontend_sus_fee.push(Either::Right( - TxShieldedTarget { - target: chain_ctx.get(&shielded_target.target), - token: chain_ctx.get(&shielded_target.token), - amount: shielded_target.amount, - }, - )); - } - } - } + let frontend_sus_fee = + self.frontend_sus_fee.map(|(target, percentage)| { + (chain_ctx.get(&target), percentage) + }); + let gas_spending_key = self.gas_spending_key.map(|key| chain_ctx.get_cached(&key)); @@ -5192,44 +5100,11 @@ pub mod args { }]; let gas_spending_key = GAS_SPENDING_KEY.parse(matches); - let frontend_sus_fee = match __TEST_FRONTEND_SUS_FEE.parse(matches) - { - Some(address) => { - // Take a constant fee of 1 on top of the input amount - let amount = InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ); - - vec![Either::Left(TxTransparentTarget { - target: address, - token, - amount, - })] - } - None => { - __TEST_FRONTEND_SUS_FEE_SHIELDED.parse(matches).map_or( - Default::default(), - |payment_address| { - // Take a constant fee of 1 on top of the input - // amount - let amount = InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ); - vec![Either::Right(TxShieldedTarget { - target: payment_address, - token: token.clone(), - amount, - })] - }, - ) - } - }; + let frontend_sus_fee = + __TEST_FRONTEND_SUS_FEE.parse(matches).map(|target| { + // Take a constant fee of 10% on top of the input amount + (target, Dec::new(1, 1).unwrap()) + }); Self { tx, @@ -5264,26 +5139,10 @@ pub mod args { payment. When not provided the source spending key will \ be used." ))) - .arg( - __TEST_FRONTEND_SUS_FEE - .def() - .help(wrap!( - "The optional transparent address of the frontend \ - provider that will take the masp sustainability \ - fee." - )) - .conflicts_with(__TEST_FRONTEND_SUS_FEE_SHIELDED.name), - ) - .arg( - __TEST_FRONTEND_SUS_FEE_SHIELDED - .def() - .help(wrap!( - "The optional payment address of the frontend \ - provider that will take the masp sustainability \ - fee." - )) - .conflicts_with(__TEST_FRONTEND_SUS_FEE.name), - ) + .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will \ + take the masp sustainability fee." + ))) } } @@ -5298,22 +5157,10 @@ pub mod args { let chain_ctx = ctx.borrow_mut_chain_or_exit(); let gas_spending_key = self.gas_spending_key.map(|key| chain_ctx.get_cached(&key)); - let frontend_sus_fee = self.frontend_sus_fee.map(|fee| match fee { - Either::Left(transparent_target) => { - Either::Left(TxTransparentTarget { - target: chain_ctx.get(&transparent_target.target), - token: chain_ctx.get(&transparent_target.token), - amount: transparent_target.amount, - }) - } - Either::Right(shielded_target) => { - Either::Right(TxShieldedTarget { - target: chain_ctx.get(&shielded_target.target), - token: chain_ctx.get(&shielded_target.token), - amount: shielded_target.amount, - }) - } - }); + let frontend_sus_fee = + self.frontend_sus_fee.map(|(target, percentage)| { + (chain_ctx.get(&target), percentage) + }); Ok(TxIbcTransfer:: { tx, @@ -5359,43 +5206,11 @@ pub mod args { let ibc_memo = IBC_MEMO.parse(matches); let gas_spending_key = GAS_SPENDING_KEY.parse(matches); let tx_code_path = PathBuf::from(TX_IBC_WASM); - let frontend_sus_fee = match __TEST_FRONTEND_SUS_FEE.parse(matches) - { - Some(address) => { - // Take a constant fee of 1 on top of the input amount - let amount = InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ); - - Some(Either::Left(TxTransparentTarget { - target: address, - token: token.clone(), - amount, - })) - } - None => { - __TEST_FRONTEND_SUS_FEE_SHIELDED.parse(matches).map( - |payment_address| { - // Take a constant fee of 1 on top of the input - // amount - let amount = InputAmount::Unvalidated( - token::DenominatedAmount::new( - 1.into(), - raw_amount.denom(), - ), - ); - Either::Right(TxShieldedTarget { - target: payment_address, - token: token.clone(), - amount, - }) - }, - ) - } - }; + let frontend_sus_fee = + __TEST_FRONTEND_SUS_FEE.parse(matches).map(|target| { + // Take a constant fee of 10% on top of the input amount + (target, Dec::new(1, 1).unwrap()) + }); Self { tx, @@ -5462,26 +5277,10 @@ pub mod args { payment (if this is a shielded action). When not \ provided the source spending key will be used." ))) - .arg( - __TEST_FRONTEND_SUS_FEE - .def() - .help(wrap!( - "The optional transparent address of the frontend \ - provider that will take the masp sustainability \ - fee." - )) - .conflicts_with(__TEST_FRONTEND_SUS_FEE_SHIELDED.name), - ) - .arg( - __TEST_FRONTEND_SUS_FEE_SHIELDED - .def() - .help(wrap!( - "The optional payment address of the frontend \ - provider that will take the masp sustainability \ - fee." - )) - .conflicts_with(__TEST_FRONTEND_SUS_FEE.name), - ) + .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( + "The optional address of the frontend provider that will \ + take the masp sustainability fee." + ))) } } @@ -7525,19 +7324,11 @@ pub mod args { None => TxExpiration::Default, } }; - let frontend_sus_fee = - __TEST_FRONTEND_SUS_FEE_IBC.parse(matches).map(|target| { - ( - target, - InputAmount::Unvalidated( - token::DenominatedAmount::new( - // Take a constant fee of 1 on top of the input - // amount - 1.into(), - raw_amount.denom(), - ), - ), - ) + let frontend_sus_fee = __TEST_FRONTEND_SUS_FEE_IBC + .parse(matches) + .map(|payment_address| { + // Take a constant fee of 10% on top of the input amount + (payment_address, Dec::new(1, 1).unwrap()) }); Self { diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 745e87d0458..4cc00334a5b 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -390,10 +390,7 @@ pub struct TxShieldingTransfer { /// Transfer source data pub sources: Vec>, /// The optional data for the frontend sustainability fee - // FIXME: should join this with sources? No maybe better to define a single - // percentage to apply to all inputs and a single receiver - pub frontend_sus_fee: - Vec, TxShieldedTarget>>, + pub frontend_sus_fee: Option<(C::TransferTarget, Dec)>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -444,8 +441,7 @@ pub struct TxUnshieldingTransfer { /// Optional additional key for gas payment pub gas_spending_key: Option, /// The optional data for the frontend sustainability fee - pub frontend_sus_fee: - Vec, TxShieldedTarget>>, + pub frontend_sus_fee: Option<(C::TransferTarget, Dec)>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -554,7 +550,7 @@ pub struct TxOsmosisSwap { /// The optional data for the frontend sustainability fee /// NOTE: if the swap is shielded (from MASP to MASP), no sustainability /// fee should be taken - pub frontend_sus_fee: Option<(C::PaymentAddress, InputAmount)>, + pub frontend_sus_fee: Option<(C::PaymentAddress, Dec)>, } impl TxOsmosisSwap { @@ -831,8 +827,7 @@ pub struct TxIbcTransfer { /// Optional additional keys for gas payment pub gas_spending_key: Option, /// The optional data for the frontend sustainability fee - pub frontend_sus_fee: - Option, TxShieldedTarget>>, + pub frontend_sus_fee: Option<(C::TransferTarget, Dec)>, /// Path to the TX WASM code file pub tx_code_path: PathBuf, } @@ -3259,7 +3254,7 @@ pub struct GenIbcShieldingTransfer { /// shielding transaction since ics-20 only supports a single asset) /// NOTE: if the shielding operation is part of a swap, and this is /// shielded (from MASP to MASP), no sustainability fee should be taken - pub frontend_sus_fee: Option<(C::PaymentAddress, InputAmount)>, + pub frontend_sus_fee: Option<(C::PaymentAddress, Dec)>, } /// IBC shielding transfer asset, to be used by [`GenIbcShieldingTransfer`] diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index 714e0a7895a..9d2282f1336 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -191,9 +191,7 @@ pub trait Namada: NamadaIo { &self, targets: Vec, sources: Vec, - frontend_sus_fee: Vec< - either::Either, - >, + frontend_sus_fee: Option<(TransferTarget, Dec)>, ) -> args::TxShieldingTransfer { args::TxShieldingTransfer { sources, @@ -211,9 +209,7 @@ pub trait Namada: NamadaIo { sources: Vec, targets: Vec, gas_spending_key: Option, - frontend_sus_fee: Vec< - either::Either, - >, + frontend_sus_fee: Option<(TransferTarget, Dec)>, ) -> args::TxUnshieldingTransfer { args::TxUnshieldingTransfer { sources, @@ -309,9 +305,7 @@ pub trait Namada: NamadaIo { token: Address, amount: InputAmount, channel_id: ChannelId, - frontend_sus_fee: Option< - either::Either, - >, + frontend_sus_fee: Option<(TransferTarget, Dec)>, ) -> args::TxIbcTransfer { args::TxIbcTransfer { source, diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 3b1bfc4df6c..e6419737432 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -26,7 +26,7 @@ use namada_core::address::{Address, IBC, MASP}; use namada_core::arith::checked; use namada_core::chain::Epoch; use namada_core::collections::HashSet; -use namada_core::dec::Dec; +use namada_core::dec::{Dec, POS_DECIMAL_PRECISION}; use namada_core::hash::Hash; use namada_core::ibc::apps::nft_transfer::types::PrefixedClassId; use namada_core::ibc::apps::nft_transfer::types::msgs::transfer::MsgTransfer as IbcMsgNftTransfer; @@ -75,10 +75,7 @@ pub use namada_tx::{Authorization, *}; use num_traits::Zero; use rand_core::{OsRng, RngCore}; -use crate::args::{ - SdkTypes, TxShieldedSource, TxShieldedTarget, TxTransparentSource, - TxTransparentTarget, -}; +use crate::args::{SdkTypes, TxTransparentSource, TxTransparentTarget}; use crate::borsh::BorshSerializeExt; use crate::control_flow::time; use crate::error::{EncodingError, Error, QueryError, Result, TxSubmitError}; @@ -2830,27 +2827,39 @@ pub async fn build_ibc_transfer( masp_fee_data }; - if let Some(target) = &args.frontend_sus_fee { - let (target, token, amount) = match target { - either::Either::Left(TxTransparentTarget { - target, - token, - amount, - }) => (TransferTarget::Address(target.to_owned()), token, amount), - either::Either::Right(TxShieldedTarget { - target, - token, - amount, - }) => (TransferTarget::PaymentAddress(*target), token, amount), - }; - + if let Some((target, percentage)) = &args.frontend_sus_fee { match (&source, &args.ibc_shielding_data) { (&MASP, None) => { + // NOTE: The frontend fee should NOT account for the masp fee + // payment amount + let sus_fee_amt = namada_token::Amount::from_uint( + validated_amount + .amount() + .raw_amount() + .checked_mul_div( + percentage.abs(), + namada_core::uint::Uint::exp10( + POS_DECIMAL_PRECISION as _, + ), + ) + .ok_or_else(|| { + Error::Other( + "Overflow in masp frontend fee computation" + .to_string(), + ) + })? + .0, + 0, + ) + .map_err(|e| Error::Other(e.to_string()))?; // Validate the amount given - let validated_amount = validate_amount( + let validated_fee_amount = validate_amount( context, - amount.to_owned(), - token, + args::InputAmount::Unvalidated(DenominatedAmount::new( + sus_fee_amt, + validated_amount.denom(), + )), + &args.token, args.tx.force, ) .await?; @@ -2858,20 +2867,20 @@ pub async fn build_ibc_transfer( masp_transfer_data.sources.push(( args.source.clone(), args.token.clone(), - validated_amount, + validated_fee_amount, )); masp_transfer_data.targets.push(( target.clone(), - token.to_owned(), - validated_amount, + args.token.to_owned(), + validated_fee_amount, )); transfer = transfer .transfer( source.to_owned(), target.effective_address(), - token.to_owned(), - validated_amount, + args.token.to_owned(), + validated_fee_amount, ) .ok_or(Error::Other( "Combined transfer overflows".to_string(), @@ -3593,69 +3602,75 @@ pub async fn build_shielding_transfer( .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } - for (idx, target) in args.frontend_sus_fee.iter().enumerate() { - let (sus_fee_target, sus_fee_token, sus_fee_amt) = match target { - either::Either::Left(TxTransparentTarget { - target, - token, - amount, - }) => (TransferTarget::Address(target.to_owned()), token, amount), - either::Either::Right(TxShieldedTarget { - target, - token, - amount, - }) => (TransferTarget::PaymentAddress(*target), token, amount), - }; - - // Validate the amount given - let validated_fee_amount = validate_amount( - context, - sus_fee_amt.to_owned(), - sus_fee_token, - args.tx.force, - ) - .await?; - - // Transfer the frontend fee, take the amount from the source matching - // the index of this fee entry - match &args.sources.get(idx) { - Some(TxTransparentSource { source, token, .. }) - if token == sus_fee_token => - { - data = data - .transfer( - source.to_owned(), - sus_fee_target.effective_address(), - sus_fee_token.to_owned(), - validated_fee_amount, + if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { + let mut extra_transfer: Vec<( + Address, + Address, + Address, + DenominatedAmount, + )> = Default::default(); + // Transfer the frontend fee, take the fee percentage from every source + for (account, denominated_amt) in &data.sources { + let sus_fee_amt = namada_token::Amount::from_uint( + denominated_amt + .amount() + .raw_amount() + .checked_mul_div( + percentage.abs(), + namada_core::uint::Uint::exp10( + POS_DECIMAL_PRECISION as _, + ), ) - .ok_or(Error::Other( - "Combined transfer overflows".to_string(), - ))?; + .ok_or_else(|| { + Error::Other( + "Overflow in masp frontend fee computation" + .to_string(), + ) + })? + .0, + 0, + ) + .map_err(|e| Error::Other(e.to_string()))?; + // Validate the amount given + let validated_fee_amount = validate_amount( + context, + args::InputAmount::Unvalidated(DenominatedAmount::new( + sus_fee_amt, + denominated_amt.denom(), + )), + &account.token, + args.tx.force, + ) + .await?; + extra_transfer.push(( + account.owner.to_owned(), + sus_fee_target.effective_address(), + account.token.to_owned(), + validated_fee_amount, + )); - if let TransferTarget::PaymentAddress(_) = sus_fee_target { - // Add the extra shielding source and target for the masp - // frontend sustainability fee - transfer_data.sources.push(( - TransferSource::Address(source.to_owned()), - sus_fee_token.to_owned(), - validated_fee_amount, - )); - transfer_data.targets.push(( - sus_fee_target, - sus_fee_token.to_owned(), - validated_fee_amount, - )); - } - } - _ => { - return Err(Error::Other( - "Token mismatch between shielding input and the \ - corresponding frontend masp fee" - .to_string(), + if sus_fee_target.payment_address().is_some() { + // Add the extra shielding source and target for the masp + // frontend sustainability fee + transfer_data.sources.push(( + TransferSource::Address(account.owner.to_owned()), + account.token.to_owned(), + validated_fee_amount, + )); + transfer_data.targets.push(( + sus_fee_target.to_owned(), + account.token.to_owned(), + validated_fee_amount, )); } } + + // Merge the extra transfer data with the default one + for (source, target, token, amount) in extra_transfer { + data = data.transfer(source, target, token, amount).ok_or( + Error::Other("Combined transfer overflows".to_string()), + )?; + } } let shielded_parts = construct_shielded_parts( @@ -3779,68 +3794,85 @@ pub async fn build_unshielding_transfer( .debit(MASP, token.to_owned(), validated_amount) .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } - for (idx, target) in args.frontend_sus_fee.iter().enumerate() { - let (sus_fee_target, sus_fee_token, sus_fee_amt) = match target { - either::Either::Left(TxTransparentTarget { - target, - token, - amount, - }) => (TransferTarget::Address(target.to_owned()), token, amount), - either::Either::Right(TxShieldedTarget { - target, - token, - amount, - }) => (TransferTarget::PaymentAddress(*target), token, amount), - }; - - // Validate the amount given - let validated_fee_amount = validate_amount( - context, - sus_fee_amt.to_owned(), - sus_fee_token, - args.tx.force, - ) - .await?; - - // Transfer the frontend fee, take the amount from the source matching - // the index of this fee entry - match &args.sources.get(idx) { - Some(TxShieldedSource { source, token, .. }) - if token == sus_fee_token => - { - let source = TransferSource::ExtendedKey(*source); - data = data - .transfer( - source.effective_address(), - sus_fee_target.effective_address(), - sus_fee_token.to_owned(), - validated_fee_amount, + if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { + let mut extra_transfer: Vec<( + TransferSource, + Address, + DenominatedAmount, + )> = Default::default(); + // Transfer the frontend fee, take the fee percentage from every source + // FIXME: wrong this should be computed on the targets. Actually maybe + // not if all the sources are unshielded which might be the case + // FIXME: should we loop on the masp transfer data instead also in the + // other builders? What if we add dummy notes? It doesn't matter since + // we will produce them later not here + // FIXME: actually, we should probably loop on the args.sources, same in + // the other builders + for (source, token, denominated_amt) in &transfer_data.sources { + // NOTE: The frontend fee should NOT account for the masp fee + // payment amount + let sus_fee_amt = namada_token::Amount::from_uint( + denominated_amt + .amount() + .raw_amount() + .checked_mul_div( + percentage.abs(), + namada_core::uint::Uint::exp10( + POS_DECIMAL_PRECISION as _, + ), ) - .ok_or(Error::Other( - "Combined transfer overflows".to_string(), - ))?; + .ok_or_else(|| { + Error::Other( + "Overflow in masp frontend fee computation" + .to_string(), + ) + })? + .0, + 0, + ) + .map_err(|e| Error::Other(e.to_string()))?; + // Validate the amount given + let validated_fee_amount = validate_amount( + context, + args::InputAmount::Unvalidated(DenominatedAmount::new( + sus_fee_amt, + denominated_amt.denom(), + )), + token, + args.tx.force, + ) + .await?; + extra_transfer.push(( + source.to_owned(), + token.to_owned(), + validated_fee_amount, + )); - // Add the extra unshielding source and target for the masp - // frontend sustainability fee - transfer_data.sources.push(( - source, - sus_fee_token.to_owned(), - validated_fee_amount, - )); - transfer_data.targets.push(( - sus_fee_target, - sus_fee_token.to_owned(), + data = data + .transfer( + source.effective_address(), + sus_fee_target.effective_address(), + token.to_owned(), validated_fee_amount, - )); - } - _ => { - return Err(Error::Other( - "Token mismatch between shielding input and the \ - corresponding frontend masp fee" - .to_string(), - )); - } + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; + + // Add the extra shielding source and target for the masp + // frontend sustainability fee + transfer_data.targets.push(( + sus_fee_target.to_owned(), + token.to_owned(), + validated_fee_amount, + )); + } + + // Merge the extra shielding source for the masp frontend sustainability + // fee + for (source, token, amount) in extra_transfer { + transfer_data.sources.push((source, token, amount)); } } @@ -4348,11 +4380,38 @@ pub async fn gen_ibc_shielding_transfer( .await; let (extra_target, source_amount) = match &args.frontend_sus_fee { - Some((target, amount)) => { + Some((target, percentage)) => { + let sus_fee_amt = namada_token::Amount::from_uint( + validated_amount + .amount() + .raw_amount() + .checked_mul_div( + percentage.abs(), + namada_core::uint::Uint::exp10( + POS_DECIMAL_PRECISION as _, + ), + ) + .ok_or_else(|| { + Error::Other( + "Overflow in masp frontend fee computation" + .to_string(), + ) + })? + .0, + 0, + ) + .map_err(|e| Error::Other(e.to_string()))?; // Validate the amount given - let validated_fee_amount = - validate_amount(context, amount.to_owned(), &token, false) - .await?; + let validated_fee_amount = validate_amount( + context, + args::InputAmount::Unvalidated(DenominatedAmount::new( + sus_fee_amt, + validated_amount.denom(), + )), + &token, + false, + ) + .await?; let source_amount = checked!(validated_amount + validated_fee_amount)?; diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 307518d7d29..21b94dc99a3 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -2788,11 +2788,6 @@ fn transfer_on_chain( Ok(()) } -enum MaspFrontendFee<'alias> { - Transparent(&'alias str), - Shielded(&'alias str), -} - #[allow(clippy::too_many_arguments)] fn transfer( test: &Test, @@ -2808,7 +2803,7 @@ fn transfer( expected_err: Option<&str>, ibc_memo: Option<&str>, gen_refund_target: bool, - frontend_sus_fee: Option, + frontend_sus_fee: Option<&str>, ) -> Result { let rpc = get_actor_rpc(test, Who::Validator(0)); @@ -2877,16 +2872,8 @@ fn transfer( } if let Some(target) = frontend_sus_fee { - match target { - MaspFrontendFee::Transparent(address) => { - tx_args.push("--test-frontend-sus-fee"); - tx_args.push(address); - } - MaspFrontendFee::Shielded(payment_address) => { - tx_args.push("--test-frontend-sus-fee-shielded"); - tx_args.push(payment_address); - } - } + tx_args.push("--test-frontend-sus-fee"); + tx_args.push(target); } let mut client = run!(test, Bin::Client, tx_args, Some(300))?; @@ -3913,8 +3900,8 @@ fn frontend_sus_fee() -> Result<()> { COSMOS_USER, MASP.to_string(), COSMOS_COIN, - // 1 extra token for the frontend sus fee - 101, + // 10 extra tokens for the frontend sus fee + 110, &port_id_gaia, &channel_id_gaia, Some(Either::Left(shielding_data_path)), @@ -3930,8 +3917,9 @@ fn frontend_sus_fee() -> Result<()> { let ibc_denom_on_namada = format!("{port_id_namada}/{channel_id_namada}/{COSMOS_COIN}"); check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 100)?; - check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 1)?; - check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 899)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 10)?; + check_balance(&test, ESTER, &ibc_denom_on_namada, 0)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 890)?; // Unshielding transfer 10 samoleans from Namada to Gaia with transparent // frontend fee @@ -3951,7 +3939,7 @@ fn frontend_sus_fee() -> Result<()> { None, None, true, - Some(MaspFrontendFee::Transparent(ESTER)), + Some(ESTER), )?; wait_for_packet_relay( &hermes_dir, @@ -3960,13 +3948,12 @@ fn frontend_sus_fee() -> Result<()> { &test, )?; check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 89)?; - check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 1)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 10)?; check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; - check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 909)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 900)?; // Unshielding transfer 10 samoleans from Namada to Gaia with shielded // frontend fee - let gaia_receiver = find_cosmos_address(&test_gaia, COSMOS_USER)?; transfer( &test, A_SPENDING_KEY, @@ -3982,7 +3969,7 @@ fn frontend_sus_fee() -> Result<()> { None, None, true, - Some(MaspFrontendFee::Shielded(AC_PAYMENT_ADDRESS)), + Some(AC_PAYMENT_ADDRESS), )?; wait_for_packet_relay( &hermes_dir, @@ -3991,9 +3978,9 @@ fn frontend_sus_fee() -> Result<()> { &test, )?; check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 78)?; - check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 2)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 11)?; check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; - check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 919)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 910)?; Ok(()) } diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index f2b3203920a..a42ef6f3f13 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -10213,7 +10213,7 @@ fn frontend_sus_fee() -> Result<()> { NAM, "--amount", "10", - "--test-frontend-sus-fee-shielded", + "--test-frontend-sus-fee", AC_PAYMENT_ADDRESS, "--signing-keys", ALBERT_KEY, @@ -10296,8 +10296,11 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 1")); - // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 1 NAM to - // a transparent address owned by the frontend provider + // FIXME: add a test with a non-native token + // FIXME: add test with masp fee unshielding (also for the ibc case) and + // check that the amount unshielded for fees is not subject to this frontend + // fee Test sus fee when unshielding. Send 9 NAM from PA to Albert and + // 0.9 NAM to a transparent address owned by the frontend provider let captured = CapturedOutput::of(|| { run( &node, @@ -10324,8 +10327,8 @@ fn frontend_sus_fee() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains(TX_APPLIED_SUCCESS)); - // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 1 NAM to - // a shielded address owned by the frontend provider + // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 0.9 NAM + // to a shielded address owned by the frontend provider let captured = CapturedOutput::of(|| { run( &node, @@ -10340,7 +10343,7 @@ fn frontend_sus_fee() -> Result<()> { NAM, "--amount", "9", - "--test-frontend-sus-fee-shielded", + "--test-frontend-sus-fee", AC_PAYMENT_ADDRESS, "--signing-keys", ALBERT_KEY, @@ -10383,7 +10386,7 @@ fn frontend_sus_fee() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 0")); + assert!(captured.contains("nam: 0.2")); // Assert NAM balance at the transparent frontend is 2 let captured = CapturedOutput::of(|| { @@ -10402,7 +10405,7 @@ fn frontend_sus_fee() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 2")); + assert!(captured.contains("nam: 1.9")); // Assert NAM balance at the shielded frontend is 2 let captured = CapturedOutput::of(|| { @@ -10421,7 +10424,7 @@ fn frontend_sus_fee() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 2")); + assert!(captured.contains("nam: 1.9")); Ok(()) } From 718b77e6f2e6230e9584c444294a30da9321b492 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 8 Sep 2025 12:09:03 +0200 Subject: [PATCH 12/18] More masp frontened fee tests --- crates/tests/src/e2e/ibc_tests.rs | 75 +++++ crates/tests/src/integration/masp.rs | 439 ++++++++++++++++++--------- 2 files changed, 363 insertions(+), 151 deletions(-) diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 21b94dc99a3..24a6651a50c 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -147,6 +147,7 @@ fn ibc_transfers() -> Result<()> { None, false, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -226,6 +227,7 @@ fn ibc_transfers() -> Result<()> { None, false, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -309,6 +311,7 @@ fn ibc_transfers() -> Result<()> { None, true, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -364,6 +367,7 @@ fn ibc_transfers() -> Result<()> { None, false, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -394,6 +398,7 @@ fn ibc_transfers() -> Result<()> { None, false, None, + None, )?; // wait for the timeout sleep(10); @@ -428,6 +433,7 @@ fn ibc_transfers() -> Result<()> { None, true, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -460,6 +466,7 @@ fn ibc_transfers() -> Result<()> { None, true, None, + None, )?; // wait for the timeout sleep(10); @@ -621,6 +628,7 @@ fn ibc_nft_transfers() -> Result<()> { None, false, None, + None, )?; clear_packet(&hermes_dir, &port_id_namada, &channel_id_namada, &test)?; check_balance(&test, &namada_receiver, &ibc_trace_on_namada, 0)?; @@ -685,6 +693,7 @@ fn ibc_nft_transfers() -> Result<()> { None, true, None, + None, )?; clear_packet(&hermes_dir, &port_id_namada, &channel_id_namada, &test)?; check_shielded_balance(&test, AB_VIEWING_KEY, &ibc_trace_on_namada, 0)?; @@ -1165,6 +1174,7 @@ fn ibc_rate_limit() -> Result<()> { None, false, None, + None, )?; // Transfer 1 NAM from Namada to Gaia again will fail @@ -1186,6 +1196,7 @@ fn ibc_rate_limit() -> Result<()> { None, false, None, + None, )?; // wait for the next epoch @@ -1211,6 +1222,7 @@ fn ibc_rate_limit() -> Result<()> { None, false, None, + None, )?; // wait for the next epoch @@ -1322,6 +1334,7 @@ fn ibc_unlimited_channel() -> Result<()> { None, false, None, + None, )?; // Proposal on Namada @@ -1381,6 +1394,7 @@ fn ibc_unlimited_channel() -> Result<()> { None, false, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -1713,6 +1727,7 @@ fn ibc_pfm_happy_flows() -> Result<()> { None, false, None, + None, )?; wait_for_packet_relay( @@ -2011,6 +2026,7 @@ fn ibc_pfm_unhappy_flows() -> Result<()> { None, false, None, + None, )?; wait_for_packet_relay( @@ -2376,6 +2392,7 @@ fn ibc_shielded_recv_middleware_happy_flow() -> Result<()> { Some(&memo), true, None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -2470,6 +2487,7 @@ fn ibc_shielded_recv_middleware_unhappy_flow() -> Result<()> { Some(&memo), false, None, + None, )?; wait_for_packet_relay(&hermes_dir, &port_id_gaia, &channel_id_gaia, &test)?; @@ -2729,6 +2747,7 @@ fn try_invalid_transfers( None, false, None, + None, )?; // invalid channel @@ -2747,6 +2766,7 @@ fn try_invalid_transfers( None, false, None, + None, )?; Ok(()) @@ -2804,6 +2824,7 @@ fn transfer( ibc_memo: Option<&str>, gen_refund_target: bool, frontend_sus_fee: Option<&str>, + gas_token: Option<&str>, ) -> Result { let rpc = get_actor_rpc(test, Who::Validator(0)); @@ -2830,6 +2851,10 @@ fn transfer( &rpc, ]); + if let Some(token) = gas_token { + tx_args.extend_from_slice(&["--gas-token", token]); + } + if let Some(ibc_memo) = ibc_memo { tx_args.extend_from_slice(&["--ibc-memo", ibc_memo]); } @@ -3940,6 +3965,7 @@ fn frontend_sus_fee() -> Result<()> { None, true, Some(ESTER), + None, )?; wait_for_packet_relay( &hermes_dir, @@ -3970,6 +3996,7 @@ fn frontend_sus_fee() -> Result<()> { None, true, Some(AC_PAYMENT_ADDRESS), + None, )?; wait_for_packet_relay( &hermes_dir, @@ -3982,6 +4009,53 @@ fn frontend_sus_fee() -> Result<()> { check_balance(&test, ESTER, &ibc_denom_on_namada, 1)?; check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 910)?; + // Shield 50 nams + transfer_on_chain( + &test, + "shield", + ALBERT_KEY, + AA_PAYMENT_ADDRESS, + NAM, + 50, + ALBERT_KEY, + &[], + )?; + check_shielded_balance(&test, AA_VIEWING_KEY, NAM, 50)?; + check_balance(&test, ESTER, NAM, 1000000)?; + // Unshielding transfer 10 samoleans from Namada to Gaia with transparent + // frontend fee. Also pay gas fees via the masp and verify that this amount + // is not subject to frontend masp fees (no recursive fees) + transfer( + &test, + A_SPENDING_KEY, + &gaia_receiver, + &ibc_denom_on_namada, + // An extra token will be added to this amount as a frontend masp fee + 10, + Some(BERTHA_KEY), + &port_id_namada, + &channel_id_namada, + None, + None, + None, + None, + true, + Some(ESTER), + Some(NAM), + )?; + wait_for_packet_relay( + &hermes_dir, + &port_id_namada, + &channel_id_namada, + &test, + )?; + check_shielded_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 67)?; + check_shielded_balance(&test, AC_VIEWING_KEY, &ibc_denom_on_namada, 11)?; + check_shielded_balance(&test, AC_VIEWING_KEY, NAM, 0)?; + check_balance(&test, ESTER, &ibc_denom_on_namada, 2)?; + check_balance(&test, ESTER, NAM, 1000000)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 920)?; + Ok(()) } @@ -4109,6 +4183,7 @@ fn osmosis_xcs() -> Result<()> { None, false, None, + None, )?; // Transfer Samoleans from Gaia transfer_from_cosmos( diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index a42ef6f3f13..99616bd6fc8 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -10161,7 +10161,14 @@ fn frontend_sus_fee() -> Result<()> { 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::setup()?; + let (mut node, _services) = setup::initialize_genesis(|mut genesis| { + // Whitelist BTC for gas payment + genesis.parameters.parameters.minimum_gas_price.insert( + "btc".into(), + DenominatedAmount::new(1.into(), token::Denomination(5)), + ); + genesis + })?; // Wait till epoch boundary node.next_masp_epoch(); @@ -10169,166 +10176,275 @@ fn frontend_sus_fee() -> Result<()> { let (frontend_alias, _frontend_key) = make_temp_account(&node, validator_one_rpc, "Frontend", NAM, 0)?; - // Test sus fee when shielding. Send 10 NAM from Albert to PA and 1 NAM to a - // transparent address owned by the frontend provider - let captured = CapturedOutput::of(|| { - run( - &node, - Bin::Client, - apply_use_device(vec![ - "shield", - "--source", - ALBERT, - "--target", - AA_PAYMENT_ADDRESS, - "--token", - NAM, - "--amount", - "10", - "--test-frontend-sus-fee", - frontend_alias, - "--signing-keys", - ALBERT_KEY, - "--node", - validator_one_rpc, - ]), - ) - }); - assert!(captured.result.is_ok()); - assert!(captured.contains(TX_APPLIED_SUCCESS)); - - // Test sus fee when shielding. Send 10 NAM from Albert to PA and 1 NAM to a - // shielded address owned by the frontend provider - let captured = CapturedOutput::of(|| { - run( - &node, - Bin::Client, - apply_use_device(vec![ - "shield", - "--source", - ALBERT, - "--target", - AA_PAYMENT_ADDRESS, - "--token", - NAM, - "--amount", - "10", - "--test-frontend-sus-fee", - AC_PAYMENT_ADDRESS, - "--signing-keys", - ALBERT_KEY, - "--node", - validator_one_rpc, - ]), - ) - }); - assert!(captured.result.is_ok()); - assert!(captured.contains(TX_APPLIED_SUCCESS)); + for token in [NAM, BTC] { + // Test sus fee when shielding. Send 20 tokens from Albert to PA and 3 + // tokens to a transparent address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + ALBERT, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + token, + "--amount", + "20", + "--test-frontend-sus-fee", + frontend_alias, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); - // sync the shielded context - run( - &node, - Bin::Client, - vec![ - "shielded-sync", - "--viewing-keys", - AA_VIEWING_KEY, - AC_VIEWING_KEY, - "--node", - validator_one_rpc, - ], - )?; + // Test sus fee when shielding. Send 10 tokens from Albert to PA and 1 + // token to a shielded address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + ALBERT, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + token, + "--amount", + "20", + "--test-frontend-sus-fee", + AC_PAYMENT_ADDRESS, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); - // Assert NAM balance at VK(A) is 20 - let captured = CapturedOutput::of(|| { + // sync the shielded context run( &node, Bin::Client, vec![ - "balance", - "--owner", + "shielded-sync", + "--viewing-keys", AA_VIEWING_KEY, - "--token", - NAM, + AC_VIEWING_KEY, "--node", validator_one_rpc, ], - ) - }); - assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 20")); + )?; - // Assert NAM balance at the transparent frontend is 1 - let captured = CapturedOutput::of(|| { - run( - &node, - Bin::Client, - vec![ - "balance", - "--owner", - frontend_alias, - "--token", - NAM, - "--node", - validator_one_rpc, - ], - ) - }); - assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 1")); + // Assert token balance at VK(A) is 40 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + token, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(&format!("{}: 40", token.to_lowercase()))); - // Assert NAM balance at the shielded frontend is 1 - let captured = CapturedOutput::of(|| { + // Assert token balance at the transparent frontend is 2 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + frontend_alias, + "--token", + token, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(&format!("{}: 2", token.to_lowercase()))); + + // Assert token balance at the shielded frontend is 2 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AC_VIEWING_KEY, + "--token", + token, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(&format!("{}: 2", token.to_lowercase()))); + } + + for token in [NAM, BTC] { + // Test sus fee when unshielding. Send 9 tokens from PA to Albert and + // 0.9 tokens to a transparent address owned by the frontend provider + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + ALBERT, + "--token", + token, + "--amount", + "9", + "--test-frontend-sus-fee", + frontend_alias, + "--signing-keys", + ALBERT_KEY, + "--gas-limit", + "60000", + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // Test sus fee when unshielding. Send 8 tokens from PA to Albert and + // 0.8 tokens to a shielded address owned by the frontend provider. Also + // pay gas fees via the masp in this case and check that the frontend + // fees does not account for the fee unshielding amount (no recursive + // fees) + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + ALBERT, + "--token", + token, + "--amount", + "8", + "--test-frontend-sus-fee", + AC_PAYMENT_ADDRESS, + "--gas-token", + token, + "--gas-limit", + "70000", + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // sync the shielded context run( &node, Bin::Client, vec![ - "balance", - "--owner", + "shielded-sync", + "--viewing-keys", + AA_VIEWING_KEY, AC_VIEWING_KEY, - "--token", - NAM, "--node", validator_one_rpc, ], - ) - }); - assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 1")); + )?; - // FIXME: add a test with a non-native token - // FIXME: add test with masp fee unshielding (also for the ibc case) and - // check that the amount unshielded for fees is not subject to this frontend - // fee Test sus fee when unshielding. Send 9 NAM from PA to Albert and - // 0.9 NAM to a transparent address owned by the frontend provider - let captured = CapturedOutput::of(|| { - run( - &node, - Bin::Client, - apply_use_device(vec![ - "unshield", - "--source", - AA_VIEWING_KEY, - "--target", - ALBERT, - "--token", - NAM, - "--amount", - "9", - "--test-frontend-sus-fee", - frontend_alias, - "--signing-keys", - ALBERT_KEY, - "--node", - validator_one_rpc, - ]), - ) - }); - assert!(captured.result.is_ok()); - assert!(captured.contains(TX_APPLIED_SUCCESS)); + // Assert token balance at VK(A) is 20.6 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + token, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(&format!("{}: 20.6", token.to_lowercase()))); + + // Assert token balance at the transparent frontend is 2.9 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + frontend_alias, + "--token", + token, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(&format!("{}: 2.9", token.to_lowercase()))); + + // Assert token balance at the shielded frontend is 2.8 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AC_VIEWING_KEY, + "--token", + token, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(&format!("{}: 2.8", token.to_lowercase()))); + } - // Test sus fee when unshielding. Send 9 NAM from PA to Albert and 0.9 NAM - // to a shielded address owned by the frontend provider + // Test sus fee when unshielding. Send 10 BTCs from PA to Albert and + // 1 BTC to a shielded address owned by the frontend provider. Also pay gas + // fees via the masp with a different token and check that the frontend fees + // does not account for the fee unshielding amount (no recursive fees) let captured = CapturedOutput::of(|| { run( &node, @@ -10340,13 +10456,15 @@ fn frontend_sus_fee() -> Result<()> { "--target", ALBERT, "--token", - NAM, + BTC, "--amount", - "9", + "10", "--test-frontend-sus-fee", AC_PAYMENT_ADDRESS, - "--signing-keys", - ALBERT_KEY, + "--gas-token", + NAM, + "--gas-limit", + "100000", "--node", validator_one_rpc, ]), @@ -10369,7 +10487,7 @@ fn frontend_sus_fee() -> Result<()> { ], )?; - // Assert NAM balance at VK(A) is 0 + // Assert BTC balance at VK(A) is 9.6 let captured = CapturedOutput::of(|| { run( &node, @@ -10379,16 +10497,16 @@ fn frontend_sus_fee() -> Result<()> { "--owner", AA_VIEWING_KEY, "--token", - NAM, + BTC, "--node", validator_one_rpc, ], ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 0.2")); + assert!(captured.contains("btc: 9.6")); - // Assert NAM balance at the transparent frontend is 2 + // Assert NAM balance at VK(A) is 19.6 let captured = CapturedOutput::of(|| { run( &node, @@ -10396,7 +10514,7 @@ fn frontend_sus_fee() -> Result<()> { vec![ "balance", "--owner", - frontend_alias, + AA_VIEWING_KEY, "--token", NAM, "--node", @@ -10405,9 +10523,28 @@ fn frontend_sus_fee() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 1.9")); + assert!(captured.contains("nam: 19.6")); + + // Assert BTC balance at the shielded frontend is 3.8 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AC_VIEWING_KEY, + "--token", + BTC, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("btc: 3.8")); - // Assert NAM balance at the shielded frontend is 2 + // Assert NAM balance at the shielded frontend is 2.8 let captured = CapturedOutput::of(|| { run( &node, @@ -10424,7 +10561,7 @@ fn frontend_sus_fee() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 1.9")); + assert!(captured.contains("nam: 2.8")); Ok(()) } From 8070560882c819a74cf5d82bbeafb99d76abd2e8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 8 Sep 2025 12:40:12 +0200 Subject: [PATCH 13/18] Refactors masp frontend fee handling in the sdk --- crates/sdk/src/tx.rs | 136 +++++++++++++++++-------------------------- 1 file changed, 52 insertions(+), 84 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index e6419737432..b871d103215 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3579,40 +3579,12 @@ pub async fn build_shielding_transfer( data = data .debit(source.to_owned(), token.to_owned(), validated_amount) .ok_or(Error::Other("Combined transfer overflows".to_string()))?; - } - - for args::TxShieldedTarget { - target, - token, - amount, - } in &args.targets - { - // Validate the amount given - let validated_amount = - validate_amount(context, amount.to_owned(), token, args.tx.force) - .await?; - transfer_data.targets.push(( - TransferTarget::PaymentAddress(target.to_owned()), - token.to_owned(), - validated_amount, - )); - - data = data - .credit(MASP, token.to_owned(), validated_amount) - .ok_or(Error::Other("Combined transfer overflows".to_string()))?; - } - if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { - let mut extra_transfer: Vec<( - Address, - Address, - Address, - DenominatedAmount, - )> = Default::default(); - // Transfer the frontend fee, take the fee percentage from every source - for (account, denominated_amt) in &data.sources { + // Transfer the frontend fee (if required), take the fee percentage from + // every source + if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { let sus_fee_amt = namada_token::Amount::from_uint( - denominated_amt + validated_amount .amount() .raw_amount() .checked_mul_div( @@ -3636,41 +3608,58 @@ pub async fn build_shielding_transfer( context, args::InputAmount::Unvalidated(DenominatedAmount::new( sus_fee_amt, - denominated_amt.denom(), + validated_amount.denom(), )), - &account.token, + token, args.tx.force, ) .await?; - extra_transfer.push(( - account.owner.to_owned(), - sus_fee_target.effective_address(), - account.token.to_owned(), - validated_fee_amount, - )); + data = data + .transfer( + source.to_owned(), + sus_fee_target.effective_address(), + token.to_owned(), + validated_fee_amount, + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; if sus_fee_target.payment_address().is_some() { - // Add the extra shielding source and target for the masp - // frontend sustainability fee + // Add the extra shielding source and target transfer_data.sources.push(( - TransferSource::Address(account.owner.to_owned()), - account.token.to_owned(), + TransferSource::Address(source.to_owned()), + token.to_owned(), validated_fee_amount, )); transfer_data.targets.push(( sus_fee_target.to_owned(), - account.token.to_owned(), + token.to_owned(), validated_fee_amount, )); } } + } - // Merge the extra transfer data with the default one - for (source, target, token, amount) in extra_transfer { - data = data.transfer(source, target, token, amount).ok_or( - Error::Other("Combined transfer overflows".to_string()), - )?; - } + for args::TxShieldedTarget { + target, + token, + amount, + } in &args.targets + { + // Validate the amount given + let validated_amount = + validate_amount(context, amount.to_owned(), token, args.tx.force) + .await?; + transfer_data.targets.push(( + TransferTarget::PaymentAddress(target.to_owned()), + token.to_owned(), + validated_amount, + )); + + data = data + .credit(MASP, token.to_owned(), validated_amount) + .ok_or(Error::Other("Combined transfer overflows".to_string()))?; } let shielded_parts = construct_shielded_parts( @@ -3793,27 +3782,14 @@ pub async fn build_unshielding_transfer( data = data .debit(MASP, token.to_owned(), validated_amount) .ok_or(Error::Other("Combined transfer overflows".to_string()))?; - } - if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { - let mut extra_transfer: Vec<( - TransferSource, - Address, - DenominatedAmount, - )> = Default::default(); - // Transfer the frontend fee, take the fee percentage from every source - // FIXME: wrong this should be computed on the targets. Actually maybe - // not if all the sources are unshielded which might be the case - // FIXME: should we loop on the masp transfer data instead also in the - // other builders? What if we add dummy notes? It doesn't matter since - // we will produce them later not here - // FIXME: actually, we should probably loop on the args.sources, same in - // the other builders - for (source, token, denominated_amt) in &transfer_data.sources { + // Transfer the frontend fee (if required), take the fee percentage from + // every source + if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { // NOTE: The frontend fee should NOT account for the masp fee // payment amount let sus_fee_amt = namada_token::Amount::from_uint( - denominated_amt + validated_amount .amount() .raw_amount() .checked_mul_div( @@ -3837,21 +3813,15 @@ pub async fn build_unshielding_transfer( context, args::InputAmount::Unvalidated(DenominatedAmount::new( sus_fee_amt, - denominated_amt.denom(), + validated_amount.denom(), )), token, args.tx.force, ) .await?; - extra_transfer.push(( - source.to_owned(), - token.to_owned(), - validated_fee_amount, - )); - data = data .transfer( - source.effective_address(), + MASP, sus_fee_target.effective_address(), token.to_owned(), validated_fee_amount, @@ -3860,20 +3830,18 @@ pub async fn build_unshielding_transfer( "Combined transfer overflows".to_string(), ))?; - // Add the extra shielding source and target for the masp - // frontend sustainability fee + // Add the extra unshielding source and target + transfer_data.sources.push(( + TransferSource::ExtendedKey(source.to_owned()), + token.to_owned(), + validated_fee_amount, + )); transfer_data.targets.push(( sus_fee_target.to_owned(), token.to_owned(), validated_fee_amount, )); } - - // Merge the extra shielding source for the masp frontend sustainability - // fee - for (source, token, amount) in extra_transfer { - transfer_data.sources.push((source, token, amount)); - } } // Add masp fee payment if necessary From f0c492591497d80b4784be8545cc32809113a1ee Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 8 Sep 2025 14:41:20 +0200 Subject: [PATCH 14/18] Adds balance check when shielding with masp frontend fees --- crates/sdk/src/tx.rs | 115 +++++++++++++++++++++++++------------------ 1 file changed, 68 insertions(+), 47 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index b871d103215..a37598705ca 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3549,6 +3549,60 @@ pub async fn build_shielding_transfer( validate_amount(context, amount.to_owned(), token, args.tx.force) .await?; + // Compute the frontend fee (if required), take the fee percentage from + // every source + let validated_frontend_fee_amt = if let Some(( + sus_fee_target, + percentage, + )) = &args.frontend_sus_fee + { + let sus_fee_amt = namada_token::Amount::from_uint( + validated_amount + .amount() + .raw_amount() + .checked_mul_div( + percentage.abs(), + namada_core::uint::Uint::exp10( + POS_DECIMAL_PRECISION as _, + ), + ) + .ok_or_else(|| { + Error::Other( + "Overflow in masp frontend fee computation" + .to_string(), + ) + })? + .0, + 0, + ) + .map_err(|e| Error::Other(e.to_string()))?; + // Validate the amount given + Some(( + sus_fee_target, + validate_amount( + context, + args::InputAmount::Unvalidated(DenominatedAmount::new( + sus_fee_amt, + validated_amount.denom(), + )), + token, + args.tx.force, + ) + .await?, + )) + } else { + None + }; + let total_input_amt = checked!( + validated_amount + + validated_frontend_fee_amt + .map(|(_, amt)| amt) + .unwrap_or_else(|| DenominatedAmount::new( + namada_token::Amount::zero(), + validated_amount.denom() + )) + )?; + // Check the balance of the source if let Some(updated_balance) = &updated_balance { let check_balance = if &updated_balance.source == source @@ -3562,7 +3616,7 @@ pub async fn build_shielding_transfer( check_balance_too_low_err( token, source, - validated_amount.amount(), + total_input_amt.amount(), check_balance, args.tx.force, context, @@ -3577,54 +3631,12 @@ pub async fn build_shielding_transfer( )); data = data - .debit(source.to_owned(), token.to_owned(), validated_amount) + .debit(source.to_owned(), token.to_owned(), total_input_amt) .ok_or(Error::Other("Combined transfer overflows".to_string()))?; - // Transfer the frontend fee (if required), take the fee percentage from - // every source - if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { - let sus_fee_amt = namada_token::Amount::from_uint( - validated_amount - .amount() - .raw_amount() - .checked_mul_div( - percentage.abs(), - namada_core::uint::Uint::exp10( - POS_DECIMAL_PRECISION as _, - ), - ) - .ok_or_else(|| { - Error::Other( - "Overflow in masp frontend fee computation" - .to_string(), - ) - })? - .0, - 0, - ) - .map_err(|e| Error::Other(e.to_string()))?; - // Validate the amount given - let validated_fee_amount = validate_amount( - context, - args::InputAmount::Unvalidated(DenominatedAmount::new( - sus_fee_amt, - validated_amount.denom(), - )), - token, - args.tx.force, - ) - .await?; - data = data - .transfer( - source.to_owned(), - sus_fee_target.effective_address(), - token.to_owned(), - validated_fee_amount, - ) - .ok_or(Error::Other( - "Combined transfer overflows".to_string(), - ))?; - + if let Some((sus_fee_target, validated_fee_amount)) = + validated_frontend_fee_amt + { if sus_fee_target.payment_address().is_some() { // Add the extra shielding source and target transfer_data.sources.push(( @@ -3638,6 +3650,15 @@ pub async fn build_shielding_transfer( validated_fee_amount, )); } + data = data + .credit( + sus_fee_target.effective_address(), + token.to_owned(), + validated_fee_amount, + ) + .ok_or(Error::Other( + "Combined transfer overflows".to_string(), + ))?; } } From c5fef2be3543811501e88f6c50edbec3a43f9f76 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 8 Sep 2025 15:34:32 +0200 Subject: [PATCH 15/18] Tests insufficients masp sus fee balances --- crates/tests/src/e2e/ibc_tests.rs | 27 +++ crates/tests/src/integration/masp.rs | 262 +++++++++++++++++++++++++++ 2 files changed, 289 insertions(+) diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 24a6651a50c..aaf61f6ed3f 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -4056,6 +4056,33 @@ fn frontend_sus_fee() -> Result<()> { check_balance(&test, ESTER, NAM, 1000000)?; check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 920)?; + // Unshielding transfer more samoleans than available with transparent + // frontend fee and shielded gas fees. Verify that client checks prevent + // this + transfer( + &test, + A_SPENDING_KEY, + &gaia_receiver, + &ibc_denom_on_namada, + // An extra 10 tokens will be added to this amount as a frontend masp + // fee + 100, + None, + &port_id_namada, + &channel_id_namada, + None, + None, + Some( + "Failed to construct MASP transaction shielded parts: \ + Insufficient funds: 43 \ + tnam1p5n6vw2v870lnjwu7h0l4humkhlf5d78ay693qmv missing", + ), + None, + true, + Some(ESTER), + None, + )?; + Ok(()) } diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 99616bd6fc8..3d416ee0697 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -10565,3 +10565,265 @@ fn frontend_sus_fee() -> Result<()> { Ok(()) } + +// Check that the clients performs balance checks correctly when adding a masp +// frontend sus fee +#[test] +fn frontend_sus_fee_client_checks() -> 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::setup()?; + // Wait till epoch boundary + node.next_masp_epoch(); + + // Initialize source address + let (source, _source_key) = + make_temp_account(&node, validator_one_rpc, "Source", NAM, 100)?; + + // Test a shielding tx where the amount and the transparent masp fee amount + // together exceed the source balance + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + source, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "100", + "--test-frontend-sus-fee", + ALBERT, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_err()); + assert!(captured.contains( + "is lower than the amount to be transferred. Amount to transfer is \ + 110.000000 and the balance is 100.000000." + )); + + // Test a shielding tx where the amount and the shielded masp fee amount + // together exceed the source balance + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + source, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "100", + "--test-frontend-sus-fee", + AC_PAYMENT_ADDRESS, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_err()); + assert!(captured.contains( + "is lower than the amount to be transferred. Amount to transfer is \ + 110.000000 and the balance is 100.000000." + )); + + // Test a shielding tx where the amount, the gas fee amount and the shielded + // masp fee amount together exceed the source balance + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + source, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "100", + "--test-frontend-sus-fee", + AC_PAYMENT_ADDRESS, + "--signing-keys", + source, + "--gas-limit", + "100000", + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_err()); + assert!(captured.contains( + "is lower than the amount to be transferred. Amount to transfer is \ + 110.000000 and the balance is 99.000000." + )); + + // Shield some tokens to the source + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "shield", + "--source", + ALBERT, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "100", + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // sync the shielded context + run( + &node, + Bin::Client, + vec![ + "shielded-sync", + "--viewing-keys", + AA_VIEWING_KEY, + "--node", + validator_one_rpc, + ], + )?; + 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: 100")); + + // Test an unshielding tx where the amount and the transparent masp fee + // amount together exceed the source balance + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + source, + "--token", + NAM, + "--amount", + "100", + "--test-frontend-sus-fee", + ALBERT, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_err()); + assert!(captured.contains( + "Failed to construct MASP transaction shielded parts: Insufficient \ + funds: 10 tnam1q9kn74xfzytqkqyycfrhycr8ajam8ny935cge0z5 missing" + )); + + // Test an unshielding tx where the amount and the shielded masp fee amount + // together exceed the source balance + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + source, + "--token", + NAM, + "--amount", + "100", + "--test-frontend-sus-fee", + AC_PAYMENT_ADDRESS, + "--signing-keys", + ALBERT_KEY, + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_err()); + assert!(captured.contains( + "Failed to construct MASP transaction shielded parts: Insufficient \ + funds: 10 tnam1q9kn74xfzytqkqyycfrhycr8ajam8ny935cge0z5 missing" + )); + + // Test an unshielding tx where the amount, the gas fee unshielding amount + // and the shielded masp fee amount together exceed the source balance + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "unshield", + "--source", + AA_VIEWING_KEY, + "--target", + source, + "--token", + NAM, + "--amount", + "100", + "--test-frontend-sus-fee", + AC_PAYMENT_ADDRESS, + "--gas-limit", + "100000", + "--node", + validator_one_rpc, + ]), + ) + }); + assert!(captured.result.is_err()); + assert!(captured.contains( + "Failed to construct MASP transaction shielded parts: Insufficient \ + funds: 11 tnam1q9kn74xfzytqkqyycfrhycr8ajam8ny935cge0z5 missing" + )); + + Ok(()) +} From 2c8247206dc9793697e50bdac35ff4a7862e1e0c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 9 Sep 2025 17:16:16 +0200 Subject: [PATCH 16/18] Hides test cli arguments --- crates/apps_lib/src/cli.rs | 52 ++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index dde1b70af35..c71552360a5 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -5023,10 +5023,15 @@ pub mod args { .def() .help(wrap!("The amount to transfer in decimal.")), ) - .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will \ - take the masp sustainability fee." - ))) + .arg( + __TEST_FRONTEND_SUS_FEE + .def() + .help(wrap!( + "The optional address of the frontend provider \ + that will take the masp sustainability fee." + )) + .hide(true), + ) } } @@ -5139,10 +5144,15 @@ pub mod args { payment. When not provided the source spending key will \ be used." ))) - .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will \ - take the masp sustainability fee." - ))) + .arg( + __TEST_FRONTEND_SUS_FEE + .def() + .help(wrap!( + "The optional address of the frontend provider \ + that will take the masp sustainability fee." + )) + .hide(true), + ) } } @@ -5277,10 +5287,15 @@ pub mod args { payment (if this is a shielded action). When not \ provided the source spending key will be used." ))) - .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( - "The optional address of the frontend provider that will \ - take the masp sustainability fee." - ))) + .arg( + __TEST_FRONTEND_SUS_FEE + .def() + .help(wrap!( + "The optional address of the frontend provider \ + that will take the masp sustainability fee." + )) + .hide(true), + ) } } @@ -7388,10 +7403,15 @@ pub mod args { .arg(CHANNEL_ID.def().help(wrap!( "The channel ID via which the token is received." ))) - .arg(__TEST_FRONTEND_SUS_FEE_IBC.def().help(wrap!( - "The optional address of the frontend provider that will \ - take the masp sustainability fee." - ))) + .arg( + __TEST_FRONTEND_SUS_FEE_IBC + .def() + .help(wrap!( + "The optional address of the frontend provider \ + that will take the masp sustainability fee." + )) + .hide(true), + ) } } From 2757e68d6fa729db0b714989a90147de251cc5cf Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 9 Sep 2025 17:42:30 +0200 Subject: [PATCH 17/18] Refactors computation of the masp sus fee into a function --- crates/sdk/src/tx.rs | 167 ++++++++++-------------------- crates/tests/src/e2e/ibc_tests.rs | 4 + 2 files changed, 60 insertions(+), 111 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index a37598705ca..e502da71d6f 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2832,33 +2832,10 @@ pub async fn build_ibc_transfer( (&MASP, None) => { // NOTE: The frontend fee should NOT account for the masp fee // payment amount - let sus_fee_amt = namada_token::Amount::from_uint( - validated_amount - .amount() - .raw_amount() - .checked_mul_div( - percentage.abs(), - namada_core::uint::Uint::exp10( - POS_DECIMAL_PRECISION as _, - ), - ) - .ok_or_else(|| { - Error::Other( - "Overflow in masp frontend fee computation" - .to_string(), - ) - })? - .0, - 0, - ) - .map_err(|e| Error::Other(e.to_string()))?; - // Validate the amount given - let validated_fee_amount = validate_amount( + let validated_fee_amount = compute_masp_frontend_sus_fee( context, - args::InputAmount::Unvalidated(DenominatedAmount::new( - sus_fee_amt, - validated_amount.denom(), - )), + &validated_amount, + percentage, &args.token, args.tx.force, ) @@ -3499,6 +3476,45 @@ async fn get_masp_fee_payment_amount( }) } +// Extract the validate amount for the masp frontend sustainability fee +async fn compute_masp_frontend_sus_fee( + context: &impl Namada, + input_amount: &namada_token::DenominatedAmount, + percentage: &namada_core::dec::Dec, + token: &Address, + force: bool, +) -> Result { + let sus_fee_amt = namada_token::Amount::from_uint( + input_amount + .amount() + .raw_amount() + .checked_mul_div( + percentage.abs(), + namada_core::uint::Uint::exp10(POS_DECIMAL_PRECISION as _), + ) + .ok_or_else(|| { + Error::Other( + "Overflow in masp frontend fee computation".to_string(), + ) + })? + .0, + 0, + ) + .map_err(|e| Error::Other(e.to_string()))?; + + // Validate the amount given + validate_amount( + context, + args::InputAmount::Unvalidated(DenominatedAmount::new( + sus_fee_amt, + input_amount.denom(), + )), + token, + force, + ) + .await +} + /// Build a shielding transfer pub async fn build_shielding_transfer( context: &N, @@ -3556,40 +3572,15 @@ pub async fn build_shielding_transfer( percentage, )) = &args.frontend_sus_fee { - let sus_fee_amt = namada_token::Amount::from_uint( - validated_amount - .amount() - .raw_amount() - .checked_mul_div( - percentage.abs(), - namada_core::uint::Uint::exp10( - POS_DECIMAL_PRECISION as _, - ), - ) - .ok_or_else(|| { - Error::Other( - "Overflow in masp frontend fee computation" - .to_string(), - ) - })? - .0, - 0, + let validated_fee_amount = compute_masp_frontend_sus_fee( + context, + &validated_amount, + percentage, + token, + args.tx.force, ) - .map_err(|e| Error::Other(e.to_string()))?; - // Validate the amount given - Some(( - sus_fee_target, - validate_amount( - context, - args::InputAmount::Unvalidated(DenominatedAmount::new( - sus_fee_amt, - validated_amount.denom(), - )), - token, - args.tx.force, - ) - .await?, - )) + .await?; + Some((sus_fee_target, validated_fee_amount)) } else { None }; @@ -3809,33 +3800,10 @@ pub async fn build_unshielding_transfer( if let Some((sus_fee_target, percentage)) = &args.frontend_sus_fee { // NOTE: The frontend fee should NOT account for the masp fee // payment amount - let sus_fee_amt = namada_token::Amount::from_uint( - validated_amount - .amount() - .raw_amount() - .checked_mul_div( - percentage.abs(), - namada_core::uint::Uint::exp10( - POS_DECIMAL_PRECISION as _, - ), - ) - .ok_or_else(|| { - Error::Other( - "Overflow in masp frontend fee computation" - .to_string(), - ) - })? - .0, - 0, - ) - .map_err(|e| Error::Other(e.to_string()))?; - // Validate the amount given - let validated_fee_amount = validate_amount( + let validated_fee_amount = compute_masp_frontend_sus_fee( context, - args::InputAmount::Unvalidated(DenominatedAmount::new( - sus_fee_amt, - validated_amount.denom(), - )), + &validated_amount, + percentage, token, args.tx.force, ) @@ -4370,33 +4338,10 @@ pub async fn gen_ibc_shielding_transfer( let (extra_target, source_amount) = match &args.frontend_sus_fee { Some((target, percentage)) => { - let sus_fee_amt = namada_token::Amount::from_uint( - validated_amount - .amount() - .raw_amount() - .checked_mul_div( - percentage.abs(), - namada_core::uint::Uint::exp10( - POS_DECIMAL_PRECISION as _, - ), - ) - .ok_or_else(|| { - Error::Other( - "Overflow in masp frontend fee computation" - .to_string(), - ) - })? - .0, - 0, - ) - .map_err(|e| Error::Other(e.to_string()))?; - // Validate the amount given - let validated_fee_amount = validate_amount( + let validated_fee_amount = compute_masp_frontend_sus_fee( context, - args::InputAmount::Unvalidated(DenominatedAmount::new( - sus_fee_amt, - validated_amount.denom(), - )), + &validated_amount, + percentage, &token, false, ) diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index aaf61f6ed3f..2e5873b4dd6 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -1426,6 +1426,8 @@ fn ibc_unlimited_channel() -> Result<()> { None, None, false, + None, + None, )?; wait_for_packet_relay( &hermes_dir, @@ -1456,6 +1458,8 @@ fn ibc_unlimited_channel() -> Result<()> { None, None, false, + None, + None, )?; // wait for the timeout sleep(10); From a13d611e54463c9992c98b5fece292a49aee2d1a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Aug 2025 18:40:24 +0200 Subject: [PATCH 18/18] Changelog #4790 --- .changelog/unreleased/features/4790-frontend-sus-fees.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/features/4790-frontend-sus-fees.md diff --git a/.changelog/unreleased/features/4790-frontend-sus-fees.md b/.changelog/unreleased/features/4790-frontend-sus-fees.md new file mode 100644 index 00000000000..f54c71cf27a --- /dev/null +++ b/.changelog/unreleased/features/4790-frontend-sus-fees.md @@ -0,0 +1,2 @@ +- Added support for MASP frontend providers sustainability fees. + ([\#4790](https://github.com/anoma/namada/pull/4790)) \ No newline at end of file