Transfer Ethereum fees to Treasury & Author, rather than burning them #1388
Conversation
| fn withdraw_fee(who: &H160, fee: U256) -> Result<Self::LiquidityInfo, Error<R>> { | ||
| if fee.is_zero() { | ||
| return Ok(None); | ||
| } | ||
| let account_id = R::AddressMapping::into_account_id(*who); | ||
| Balances::<R>::withdraw( | ||
| &account_id, | ||
| fee.unique_saturated_into(), | ||
| WithdrawReasons::FEE, | ||
| ExistenceRequirement::AllowDeath, | ||
| ) | ||
| .map(Some) | ||
| .map_err(|_| Error::<R>::BalanceLow) | ||
| } |
There was a problem hiding this comment.
I wondered if we could implement this trait with calls to our pallet-fees. Does it make sense? i.e. in this case this would be a call to Fees::withdraw_fee()
There was a problem hiding this comment.
If we're not going to use fee_to_author and fee_to_treasury (See my response below), I'm not sure that it makes sense to go through the Fees pallet just for this?
| .map_err(|_| Error::<R>::BalanceLow) | ||
| } | ||
|
|
||
| fn correct_and_deposit_fee( |
There was a problem hiding this comment.
And this, in turn, would be calls to Fees::fee_to_author() and Fees::fee_to_treasury().
There was a problem hiding this comment.
fee_to_author and fee_to_treasury both do a withdraw internally, so they're not suitable here.
| base_fee: U256, | ||
| already_withdrawn: Self::LiquidityInfo, | ||
| ) -> Self::LiquidityInfo { | ||
| let Some(paid) = already_withdrawn else { return None }; |
There was a problem hiding this comment.
This line is so cool that even Github is unable to highlight it well 😲
There was a problem hiding this comment.
rustfmt also unfortunately does not handle let else properly yet, so we should probably only use it for small things like this. Otherwise we'll end up with a bunch of reformats when rustfmt finally gets updated
runtime/common/src/evm/fees.rs
Outdated
| // Handle base fee and tips | ||
| let (treasury_amount, mut author_amount) = base_fee.ration( | ||
| TREASURY_FEE_RATIO.deconstruct(), | ||
| (Perbill::one() - TREASURY_FEE_RATIO).deconstruct(), | ||
| ); | ||
| tip.merge_into(&mut author_amount); | ||
| <Treasury<R> as OnUnbalanced<_>>::on_unbalanced(treasury_amount); | ||
| <ToAuthor<R> as OnUnbalanced<_>>::on_unbalanced(author_amount); |
There was a problem hiding this comment.
This section reminds me to DealWithFees type, located in runtime/common/src/lib.rs. I think the implementation is the same. Maybe we can call it to avoid duplicate code:
DealWithFees::on_unbalanceds([base_fee, tip].iter());Or similar.
There was a problem hiding this comment.
👍 yeah, originally I had the tip handling done in pay_priority_fee, but since I rolled it all into one place it makes sense to call up into DealWithFees so we only have one place to update
There was a problem hiding this comment.
LGTM! I have a tiny nit that totally can be ignored.
A dumb question from my side would be, why we do not simply apply the impl_on_charge_evm_transaction macro boilerplate as done in Moonbeam?
| // If we tried to refund something, the account still empty and the ED is set to | ||
| // 0, we call `make_free_balance_be` with the refunded amount. | ||
| let refund_imbalance = if Balances::<R>::minimum_balance().is_zero() | ||
| && refund_amount > <Balance<R>>::zero() |
There was a problem hiding this comment.
| && refund_amount > <Balance<R>>::zero() | |
| && !refund_amount.is_zero() |
|
What is the state of this? |
This implements a custom `OnChargeEVMTransaction` for our chain. Unfortunately the EVM handling of base_fee+priority is slightly different from how pallet_transaction_payment handles fees+tips, so we have to reimplement quite a bit more logic than I would have ideally liked. Fortunately, it's all pretty straightforward fee handling bits.
|
@mustermeiszer I suppose we still want this at some point or can we close it? |
|
Yes, I would say so. |
This implements a custom
OnChargeEVMTransactionfor our chain.Unfortunately the EVM handling of base_fee+priority is slightly different from how pallet_transaction_payment handles fees+tips, so we have to reimplement quite a bit more logic than I would have ideally liked. Fortunately, it's all pretty straightforward fee handling bits.
Changes and Descriptions
Checklist: