Better Parameterisation for Fee system

[kianenigma]

Makes is slightly more substrate-style by using Get instead of floating constant or hardcoded values.

---

• Needs a Polkadot PR.

[gui1117]

This change one invariant that all types declared through parameter-types! are used for associated type with Get.

now some type define there are actually being implemented some trait in node/runtime/src/impl.rs using their values.

That can be ok, but I feel like it can start confusing some ppl, parameter-types! gets a bit more complexity as to set a correct value you shouldn't only go into the module definition but also to the impl module.

Though I'm ok with this.

[kianenigma]

My argument was that seeing all of these numbers, regardless of how they are interpreted, on one place is much better. Before, the 25% used to be somewhere else in consts.rs and the 1000 was hardcoded. I think being actually inside of parameter_types! {} is the most expressive way + prevents anyone from forgetting to configure these numbers.

[kianenigma]

I can also get rid of the weight in the interface of FeeMultiplierUpdate -- don't know how but I managed to miss the fact that storage is actually readable in runtime before. Has a few failing tests due to rounding error that needs a bit more time.

CC @4meta5 you might be interested in these changes as I recall exactly that I mis-guided you when you also asked if it is possible to directly read a storage in the fee update implementation.

[gui1117]

if this is only for testing, then why not ?

Suggested change

	#[cfg(any(feature = "std", test))]
	#[cfg(test)]

[kianenigma]

Fair point and I (naively) used the gating feature that I used here https://github.com/paritytech/substrate/pull/3823/files#diff-24bd3cac4cae1c620f82f7aa79ef28ceR707

Declaring the function with just #[cfg(test)] was not enough and it cannot be found in tests though 🤔 and I had to make it test or std. Do you know the reason of the difference

[kianenigma]

I agree that logically it should be just cfg[test]. Something is wrong maybe in cargo files

[gui1117]

oh actually this is because the test configure is only set for the tested crate.
if you test the runtime then sr-arithmetic isn't compiled with test.

I don't know what is the prefered way to solve this

[kianenigma]

(won't work as discussed on off-band. will leave the comment open)

[gui1117]

code is OK but wonder about API again maybe we would prefer not having this code in runtime/impl.rs as probably quite some other runtime will implement the same.

In contracts it is organized a bit differently and maybe we should inspiration from it:
on constracts::trait there is this

type ComputeDispatchFee: ComputeDispatchFee<<Self as Trait>::Call, BalanceOf<Self>>;

and then in contracts crate there is :

/// The default dispatch fee computor computes the fee in the same way that
/// the implementation of `ChargeTransactionPayment` for the Balances module does. Note that this only takes a fixed
/// fee based on size. Unlike the balances module, weight-fee is applied.
pub struct DefaultDispatchFeeComputor<T: Trait>(PhantomData<T>);
impl<T: Trait> ComputeDispatchFee<<T as Trait>::Call, BalanceOf<T>> for DefaultDispatchFeeComputor<T> {
	fn compute_dispatch_fee(call: &<T as Trait>::Call) -> BalanceOf<T> {
              ...
	}
}

EDIT: maybe we would want to have some default struct with an unnamed field like

struct OneWayToComputeStuffUsingPerbill(pub Perbill)

[kianenigma]

So the dilemma is to leave it in impl.rs or move it to srml/transaction-payment.

In practice, I am fine with both. We are providing a default behaviour for something which at the end of the day the user should define.

But what I don't like about putting this targetedAdjustment in a substrate module is that it is too polkadot-specific. In otherwords, naming a polkadot requirement as the default of substrate is not good. Although, I have made the struct configurable here by being able to change the 25% value. But still the whole logic of "Changing fees based on congestion" seems polkadot-specific to me.

I am even tempted to wipe this ting from substrate node and only leave it in polkadot repo. Substrate node can just use () or ConvertInto, meaning that fees will not change over time.

[amarsinghcodes]

But what I don't like about putting this targetedAdjustment in a substrate module is that it is too polkadot-specific. In otherwords, naming a polkadot requirement as the default of substrate is not good.

Agreed, I am pro putting this logic in Polkadot and using () for the Substrate node. I appreciate the separation and think the default for a Substrate node should be no change in fees over time. In this case, documentation could guide users to configure fees in a similar way to Polkadot as an example (@JoshOrndorff should be kept aware bc of JoshOrndorff/substrate-recipes#62), but I really don't like having the Polkadot slow-adjusting fee model as the default in Substrate...

I can expand on this, but I basically don't expect most developers building with Substrate to adopt the same fee structure as a relay chain. The economics of parachains/sovereign chains should and will be very different. Personally, I think something like BRAQ (section 4.2, page 9) or some similar model for subsidizing actions within a defined period of blocks might be more appropriate than charging per method call.

[gui1117]

is it on purpose that block weight changed from 1000 to 0 ?

[kianenigma]

yeah just makes a bit more sense: the test is supposed to simulate an empty chain. Nonetheless the test is not that important as you see and just runs a simulation.

As I see A1-onice, maybe I should review once finished

[kianenigma]

@thiolliere we are writing a doc about weights and I thought I would keep this Iced to apply very small changes to docs and prevent opening new PRs every day.