-
Notifications
You must be signed in to change notification settings - Fork 1k
Frontend MASP sustainability fee #4790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
82b94e5 to
4514cac
Compare
🧪 CI InsightsHere's what we observed from your CI run for a13d611. ✅ Passed Jobs With Interesting Signals
|
crates/apps_lib/src/cli.rs
Outdated
| .def() | ||
| .help(wrap!("The amount to transfer in decimal.")), | ||
| ) | ||
| .arg(__TEST_FRONTEND_SUS_FEE.def().help(wrap!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we bother exposing this in the cli as it's just for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we still need to expose them to run the integration and e2e tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a clever way to keep this from showing up in the cli help menu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hid them in e90c619
cc897ad to
abeb496
Compare
9072bfd to
b6de931
Compare
batconjurer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
crates/sdk/src/tx.rs
Outdated
| // NOTE: The frontend fee should NOT account for the masp fee | ||
| // payment amount | ||
| let sus_fee_amt = namada_token::Amount::from_uint( | ||
| validated_amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this block of code for the percentage calculation isn't nice looking and appears in a few spots. Can we hide it in the amount module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3fe86a7, but the new function does also the validation part and lives in the same module
d3f672b to
a13d611
Compare
Frontend MASP sustainability fee (backport #4790)
…pr-4790 Frontend MASP sustainability fee (backport #4790)
Describe your changes
Adds support for an optional masp frontend providers sustainability fees. This fee is supported as both a transparent and a shielded transfer (with the exception of incoming shielding IBC packets). To support it the followings have been changed:
Shielding and (IBC) unshielding
The sdk has been extended to allow specifying the entry for this fee. This is an optional target and a percentage which will be applied to all inputs. The sdk adjusts the input amounts to account for this extra fee.
There's one limitation for the IBC unshielding case. If the transaction ultimately fails (rejected by the target chain or timed out) we refund the amount of the unshielding to a disposable address. The frontend fees are not part of this amount and do not get refunded: to support this we'd need to temporarily escrow those tokens to an internal address and finalize the fee payment only once the target chain has confirmed the transaction to be successful (this would require some protocol support and should probably be done in conjunction with #4726). On top of this, the unshielding refunds are done to a disposable transparent address from which the user might want to reshield the tokens: this would incur in another frontend fee event leading to multiple fees applied to the same overall action.
Incoming IBC shielding packets
The optional target and percentage of the fee can be passed to the
gen_ibc_shielding_transferfunction. Since IBC is limited to a single token, the asset is automatically inferred to be the same of the transfer and fees are not supported when shielding nfts. The fee amount itself will be encoded as an additional shielded output of the MASP bundle so that we can avoid the need for protocol-breaking changes.Osmosis swaps
Osmosis swaps are ultimately collapsed to the above ibc cases and so the same rules apply. In case the swap was fully shielded the frontend provider should refrain from charging a fee since this should count as a fully shielded transaction.
Checklist before merging
breaking::labelsnamada-docsreponamada-indexerornamada-masp-indexer, a corresponding PR is opened in that repo