Add OnFeeCharged trait in fee modules.

[shaunxw]

PR for issue #1923

This is an improvement to fees module. The OnFeeCharged trait gives a chance to deal with accumulated fees.

[parity-cla-bot]

It looks like @shaopengw signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[parity-cla-bot]

It looks like @shaopengw signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[bkchr]

Could you do this implementation please like seen here.

[bkchr]

Can we not combine this with the loop below? To have this loop just once?

[shaunxw]

@bkchr all updated :]

[bkchr]

Looks good so far, but still some stuff need to be improved.

[bkchr]

Could you please add a test for this as well? Maybe just extend the on_finalise_should_work test and check that on_fee_charged is called with the correct amount.

[gavofyork]

This could be done more cleanly by adding an event hook OnTransactionFeePaid into executive (or, probably better, system::Trait which would be called from executive) which could then be hooked in to fees::Trait::OnFeeChanged. This way you can avoid a second loop over all the transactions at the end of the block and writes into storage needed for CurrentTransactionFee.

[xlc]

I believe that is basically the ChargeFee trait. Storage write (or some in memory write) is necessary to combine multiple fees together and emit a single event with the accumulated fee (#1815). I don't think there is other way to achieve that in the current substrate.

[gavofyork]

In-memory write is way faster than storage round-trip, so I would prefer it to be donee thtat way, especially since it will affect all chains, even those with no use for this fee mechanism.

[xlc]

Make sense. Is there existing in memory store can be used? Or need to crate this new mechanism? which will be out of scope of this PR and shouldn’t prevent this PR from been merged.

[shaunxw]

@bkchr Could you please suggest a clean way(or existing example in srml) to check if on_fee_charged called with correct amount in unit tests?

The one I could come up with is a mock implementing OnFeeCharged, but then a global variable is needed to hold the amount so it could be checked after on_finalise call, and creating global variables in mock.rs doesn't sound a ideal solution.

[bkchr]

Create some type in fees/src/mock.rs that implements OnFeeCharged and add this here.
When on_fee_charged is called, write the value into storage and get that value afterwards.
Checking the value can be done here:

substrate/srml/fees/src/tests.rs

Line 172 in c4ca27f

]);

[shaunxw]

@bkchr Test added. :)

[bkchr]

I'm happy now, but we still need to integrate the requested changes by @gavofyork.
@gavofyork I'm not 100% sure on how to implement your requested changes, could you explain it more detailed for us?
Do you want to have 1 storage item for all transaction fees? So, that we don't require to load extrinsic_count items from the storage?

[gavofyork]

There would be no need for any storage used. Basically, you just call the transaction-fee-paid hook right at the point that it is paid (in executive) rather than putting it all in storage and then re-looping in on_finalize.

[xlc]

That's one possible way to do it but one of the goal of this fees module is to provide accumulated tx fees. Take balances transfer for example, it should charge fee twice (after #1815 is merged), and I don't want it emit fees.Charged event twice. This fees module will sum all the charged fees and emit one fees.Charged event with total fees been charged.

[gavofyork]

That's not going to fly: Chains that don't care about this (e.g. Polkadot) should not have to pay the extra storage i/o just because other chains decide they wants to amalgamate all this info.

If you have a callback based solution then your chain has the information is needs and can mutate it, store it and act upon it as desired, all without impacting on the performance of other chains that don't care.

[gavofyork]

superceded in #2048