Skip to content

Dynamic Fee Adjustment Crate #161

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

Closed
wants to merge 2 commits into from
Closed

Conversation

gabriele-0201
Copy link
Contributor

This PR extracts the fee adjustment logic into its own crate, requiring multiple modifications because each parameter needs to be passed to the struct instead of being used directly.

Open questions:

  • Is DynamicFeeAdjustment an acceptable name?
  • Is the positioning of the crate within the repository structure acceptable?
  • Is it ok to define the storage item NextLengthMultiplier inside the crate?

TODOs:

Copy link
Contributor Author

gabriele-0201 commented Dec 28, 2023

@@ -0,0 +1,35 @@
[package]
name = "dynamic-fee-adjustment"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about sugondat-length-fee-adjustment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, for now, I used pallet-sugondat-length-fee-adjustment. The only thing is that the pallet currently implements both length and weight to the fee. So, pallet-sugondat-dynamic-fee-adjustment is probably more general?

@rphmeier
Copy link
Contributor

rphmeier commented Dec 28, 2023

Is the positioning of the crate within the repository structure acceptable?

Seems fine to me! I also considered placing it within the runtime directory as it's a runtime utility crate, but we don't have many of those and a flat structure will be easier to navigate until overcrowded.

Is it ok to define the storage item NextLengthMultiplier inside the crate?

No. That should be done in each runtime individually. It would work, I think, but convention is not to define storage items within crates that are implicitly used by structures from that crate. That's for a good reason: there would be hidden magic when using the struct.

@rphmeier
Copy link
Contributor

rphmeier commented Dec 28, 2023

Actually, reflecting on the few questions you have brought to mind another approach.

I believe that this should be refactored into a new pallet. There are a few reasons for this:

  1. We need a storage item, or perhaps several, for handling parameters. Definitely the NextLengthMultiplier would be better as a pallet storage item.
  2. We will need some on_initialize logic to handle skipped blocks after node: use limiting proposer #157 . The logic will be extremely light, but requires some calculation. I suggest that in this PR, the pallet should not have any on-initialize logic.

This also answers the directory location question: it would exist in pallets, alongside blobs. I recommend for this pallet to require Config: pallet_transaction_payment::Config, and it can simply invoke <Config as pallet_transaction_payment::Config>::MultiplierUpdate internally to solve the issue of too many generic parameters.

@rphmeier rphmeier force-pushed the gab_fee_adjustment_tests branch from 101b79a to ea1deab Compare December 28, 2023 20:12
Base automatically changed from gab_fee_adjustment_tests to main December 28, 2023 20:14
@gabriele-0201 gabriele-0201 force-pushed the gab_dynamic_fee_adjutment branch from ba39405 to 7153de9 Compare January 2, 2024 15:04
@gabriele-0201 gabriele-0201 force-pushed the gab_dynamic_fee_adjutment branch from 7153de9 to a97d7d3 Compare January 2, 2024 15:17
@gabriele-0201 gabriele-0201 force-pushed the gab_dynamic_fee_adjutment branch from a97d7d3 to b7c6eb8 Compare January 2, 2024 15:29
@gabriele-0201
Copy link
Contributor Author

gabriele-0201 commented Jan 2, 2024

The implementation has been moved to #174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants