Fee foreign reserve #4644
Fee foreign reserve #4644
Conversation
grarco
added
the
breaking:consensus
crates/node/src/shell/mod.rs
Outdated
| let minimum_gas_price = | ||
| parameters::read_gas_cost(storage, &wrapper.fee.token) | ||
| .expect("Must be able to read gas cost parameter") | ||
| .ok_or(Error::TxApply(protocol::Error::FeeError(format!( | ||
| "The provided {} token is not allowed for fee payment", | ||
| wrapper.fee.token | ||
| ))))?; |
There was a problem hiding this comment.
do we need to recompute this? isn't it already passed to fee_data_check, which calls get_fee_components?
There was a problem hiding this comment.
The issue is that fee_data_check is also called by prepare_proposal where the minimum gas price could be the one set locally by the block proposer and not the protocol one. We could pass the minimum gas price to fee_data_check as an Option and make sure that prepare_proposal passes in a None (or maybe a None only when a custom value overrides the protocol one), but it would make it slightly easier to misuse this function.
Actually, the protocol minimum is red even in prepare proposal to compare it with the custom local value, I'll try to see if there's a clean way to propagate that
There was a problem hiding this comment.
There's an attempt in 1e7cdcf. It's slightly more complicated than I wanted to (an extra small trait) to ensure that the gas price coming from the local config of a block proposer does not leak to other modules/steps of comet consensus
crates/node/src/protocol.rs
Outdated
| }; | ||
| shell_params.state.write_log_mut().emit_event( | ||
| TokenEvent { | ||
| descriptor: FEE_PAYMENT_DESCRIPTOR, |
There was a problem hiding this comment.
| descriptor: FEE_PAYMENT_DESCRIPTOR, | |
| descriptor: FEE_PAYMENT_DESCRIPTOR, |
should we use a different descriptor here? wrapper-fee-treasury or smth, idk
There was a problem hiding this comment.
We can do that yes, should we also create a new one for when we burn the base fee (native token)? Maybe we can just create two descriptors, one for the tip and one for the base fee. It looks like no external tool is using these events so even changing them shouldn't require modifications anywhere else
There was a problem hiding this comment.
yeah, I think one for tips and another for base fees is enough
There was a problem hiding this comment.
I've split the event descriptor in 93de5ad. I've also changed the logic to emit the tip event only if a tip is present
|
Can the internal PGF account receive ICS20 transfers? This is part of the requirements for #4496. Even if it should already work, I think it would be good to add a test-case for this. |
@cwgoes the pgf vp is quite permissive with respect to balance changes. it doesn't validate any balance key changes, so anyone is free to send money to the PGF account, whether it's through an ICS-20 transfer, or any other mechanism. the only checks to balances are performed by the multitoken vp, that verify if the changes balance out to 0 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4644 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Describe your changes
Closes #4496.
Changes the base fee logic: burns it if the fee token is the native one and sends it to PGF otherwise. For now the base fees for non-native tokens is sent to the PGF internal account. An alternative would be to create a new internal account for this specific purpose, here's a list of possible pros of both solutions:
Checklist before merging
breaking::labelsnamada-docsreponamada-indexerornamada-masp-indexer, a corresponding PR is opened in that repo