Skip to content

Unify fee modifiers#1420

Closed
notlesh wants to merge 18 commits intomasterfrom
notlesh-fixed-substrate-fee-modifier
Closed

Unify fee modifiers#1420
notlesh wants to merge 18 commits intomasterfrom
notlesh-fixed-substrate-fee-modifier

Conversation

@notlesh
Copy link
Contributor

@notlesh notlesh commented Apr 19, 2022

What does it do?

This accomplishes two things:

  1. Remove the floating congestion-based fee modifier previously used for pallet transaction-payment
  2. Unify fee multipliers for both Ethereum and Substrate transactions

Note that transaction-payment doesn't actually use the MultiplierUpdate trait outside of its integrity_test hook, it actually uses the Convert trait at runtime:

https://github.com/paritytech/substrate/blob/9461b2de04210c6c193726a745c3ec6552b4ce9f/frame/transaction-payment/src/lib.rs#L344

The previously used TargetedFeeAdjustment impl used these values, but without that they are completely unused at runtime (please review).

I believe, then, that this will work with fixed fees for now and should also be fine in the future when EIP-1559 congestion-based fee modifier is properly implemented.

This is closely related to #1353 but should be fine if done independently.

⚠️ Breaking Changes ⚠️

⚠️ All substrate-based fees will be impacted by this
⚠️ Some txns in the mempool prior to this change (runtime upgrade) may become invalid
⚠️ RPC calls to estimate fees will return different values
🧷 This only affects moonbase runtimes currently

TODO:

  • Bounds checking (U256 -> U128 -> FixedU128)
  • DRY (move to common)
  • Tests, esp. that the values satisfy integrity_test

@notlesh notlesh added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 19, 2022
@notlesh notlesh added D2-breaksapi This PR changes public API; next release should be major. D3-breaksconsensus The PR alters the node runtime and/or the SRML modules such that the logic is materially different. D10-breaksdocs Changes to code that require changes in documentation labels Jun 7, 2022
Comment on lines +2519 to +2521
// TODO: there may be a better way to do this (use a different pub fn from
// transaction-payment for testing?)
let known_encoded_overhead_bytes = 11;
Copy link
Contributor Author

@notlesh notlesh Jun 7, 2022

Choose a reason for hiding this comment

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

This test sucks. Basically, I tried to exploit the fact that remark's weight fn returns its length, but after some debugging this appears to be the encoded length, not the actual remark length.

#[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))]

The overhead increases as the remark grows in size, probably because it takes more bytes to describe the size of a larger vec.

I think the test does its job, but it's pretty ugly. I'd like to find a cleaner way to accomplish the same thing.

@notlesh
Copy link
Contributor Author

notlesh commented Sep 13, 2022

Closing this in favor of #1765

@notlesh notlesh closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D2-breaksapi This PR changes public API; next release should be major. D3-breaksconsensus The PR alters the node runtime and/or the SRML modules such that the logic is materially different. D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. D10-breaksdocs Changes to code that require changes in documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant