Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fees module refactor#2012

Closed
xlc wants to merge 4 commits intoparitytech:masterfrom
xlc:fees
Closed

Fees module refactor#2012
xlc wants to merge 4 commits intoparitytech:masterfrom
xlc:fees

Conversation

@xlc
Copy link
Contributor

@xlc xlc commented Mar 16, 2019

Closes #1993

let new_fee = current_fee.checked_add(&amount).ok_or_else(|| "fee got overflow after charge")?;

T::TransferAsset::withdraw(transactor, amount, WithdrawReason::TransactionPayment)?;
T::TransferAsset::withdraw(transactor, amount, WithdrawReason::Reserve)?;
Copy link
Member

Choose a reason for hiding this comment

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

why is this reserve?

@gavofyork
Copy link
Member

gavofyork commented Mar 17, 2019

I think the ChargeFee module confuses things in general as it presupposes that Substrate actually has a cohesive and uniform notion of fee. As I mentioned in the issue, it doesn't - economic protections against spam are implemented in numerous ways, only one of which is a straight fee. Reserve and refund doesn't work well beyond the simple assumptions made here (e.g., governance modules make reservations and issue potential refunds that span multiple blocks).

All in all, I would propose to revert the fees module entirely (it was merged without my consent in the first place) and instead introduce a number of ZCAs in any existing modules that do things that you'd want to catch and potentially handle differently. This could include, e.g. Balance module making a new account or transfer charge. There can then be a fees module introduced which can be hooked into all of these events without compromising the performance or API hygiene of the main SRML modules.

@xlc
Copy link
Contributor Author

xlc commented Mar 17, 2019

@gavofyork I have removed the ChargeFee trait and only keep the make transaction payment functionality in fees module.
I still want to keep fees module for TransactionBaseFee and TransactionByteFee. Previously they belongs to balances module which I don't think it is the most sensible place.

@gavofyork
Copy link
Member

gavofyork commented Mar 18, 2019

#2028 is the way things need to go.

This removes all the fees stuff, replacing ChargeFee with MakePayment. I generally agree that it's not a great fit to have the tx byte/base fee in balances, but I'm also not convinced that setting up a whole other module for it is sensible either. In any case, you can just make an impl of MakePayment for the fees module and use that if you'd prefer to handle fees that way and you'll not take a performance hit.

With all the abstractions I introduce in #2028, you should still be able to do everything you need, including a fairly holistic fees amalgamation module.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants