Skip to content

Conversation

batconjurer
Copy link
Collaborator

Describe your changes

Closes #4716

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@github-actions github-actions bot added the breaking:api public API breaking change label Jul 8, 2025
@batconjurer batconjurer requested review from grarco, murisi, sug0 and tzemanovic and removed request for sug0 July 14, 2025 08:21
@batconjurer batconjurer added breaking:cli command line breaking change breaking:client Namada client breaking change breaking:consensus Consensus breaking change that requires a hard-fork breaking: tx Transaction format breaking change breaking:state State breaking change that is not backwards compatible with a state recorded by current version breaking:SDK SDK breaking change labels Jul 14, 2025
@batconjurer batconjurer mentioned this pull request Jul 14, 2025
6 tasks
/// Wrap a header with a section for the purposes of computing hashes
Header(Header),
/// The account paying the fee for a shielding transaction
ShieldingFee {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick idea I haven't reasoned too deeply about (especially in terms of safety): would it be possible to avoid this new section type and encode all the necessary data directly inside the Transfer data section? Maybe we could just add two entires: the fee payer as a source and the PGF account as the target. The multitransfer function will just update the balances accordingly. The masp vp should have all the necessary data to check if the PGF account has been credited the fees when the vector of inputs of the transparent bundle is non-empty. It should also be able to verify that the amount credited to the PGF account is in a supported token and matches the expected amount for that token

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difficult part would be knowing which masp tx was paid for it multiple shields are in a batch. We need a commitment.

Copy link
Collaborator

@sug0 sug0 Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grarco I'm doing something similar in the FMD PR I have open

/// Pointers to MASP data within a transfer tx
pub shielded_data: Option<MaspTxData>,

pub struct MaspTxData {
/// Id of the MASP transaction.
///
/// This is used to look-up a MASP transaction section.
pub masp_tx_id: MaspTxId,
/// Section hash of the FMD flag ciphertext.
///
/// This is used to look-up a transaction data section
/// containing an FMD flag ciphertext.
pub flag_ciphertext_sechash: Hash,
}

the fmd section is stored in Section::ExtraData, thus is implicitly signed when we sign the tx_transfer data

this still breaks the hw wallet tho, I think, because the parsing of the masp tx data section is now different (the field is larger). it def did in the fmd pr

@batconjurer batconjurer changed the title Adding a shielding fee for normal shielding txs. Still need to check IBC Adding a shielding fee for normal shielding txs. Jul 16, 2025
Copy link
Collaborator

@sug0 sug0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall this is correct, in terms of impl alone. we should check in with @cwgoes that this meets the desired design principles. also, should pitch in some opinion from @murisi about the hw wallet changes required

@batconjurer batconjurer force-pushed the bat/add-shielding-fee-section branch from 9f91824 to 41cda8f Compare July 25, 2025 14:09
@batconjurer batconjurer requested review from grarco, sug0 and yito88 and removed request for murisi July 28, 2025 09:41
@brentstone
Copy link
Collaborator

Let's get some new reviews here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking:api public API breaking change breaking:cli command line breaking change breaking:client Namada client breaking change breaking:consensus Consensus breaking change that requires a hard-fork breaking:SDK SDK breaking change breaking:state State breaking change that is not backwards compatible with a state recorded by current version breaking: tx Transaction format breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add fees for shielding to the MASP

5 participants