Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify fee modifiers #1420

Closed
wants to merge 18 commits into from
Closed

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