Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add ConstFeeMultiplier to the transaction payment pallet #12222

Merged
merged 8 commits into from
Sep 12, 2022
Merged

Add ConstFeeMultiplier to the transaction payment pallet #12222

merged 8 commits into from
Sep 12, 2022

Conversation

mrshiposha
Copy link
Contributor

@mrshiposha mrshiposha commented Sep 8, 2022 •
edited
Loading

Currently, the template node sets the FeeMultiplierUpdate of the pallet_transaction_payment::Config to the unit type.
It seems not obvious (and barely noticeable) that the default implementation of the MultiplierUpdate trait returns 0 (and not 1).
Because of that, the transaction weights will be discarded due to the multiplication by 0 inside the transaction payment pallet.

This PR suggests using the FeeMultipleUpdate equivalent to the SlowAdjustingFeeUpdate from the Polkadot runtime common.

I believe it will fix the non-obvious behavior and will add extra useful info to the template node.

polkadot address: 11Ze4Gi9iHZ1qPEweUewWk5iUTYL4Zd18VxKZ6cHhqyrfep

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Sep 8, 2022

User @mrshiposha, please sign the CLA here.

@ggwpez
Copy link
Member

ggwpez commented Sep 8, 2022

The template node was kept simple on purpose, to not confuse new developers.
Not sure what the idea was here. Maybe @kianenigma remembers?

@kianenigma
Copy link
Contributor

Yeah, this is intentional. The SlowAdjustment is a feature made for Polkadot, and I don't think we should use it in node-template.

That being said, I think the intention is to disable any weight multiplier update for node-template. This means that the default value should be set to 1, and the convert should be struct Identity. The current code is somewhat different because it basically disables any weight fee by using a multiplier that's zero.

kianenigma
kianenigma suggested changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Valid concern, but needs a different approach! I would appreciate if you fix this.

@mrshiposha
Copy link
Contributor Author

@kianenigma maybe it would be nice to change the default implementation of the MultiplierUpdate to return 1 in the fn min()?

So the default implementation won't change the multiplier. And I will revert all the changes to the template node.
What do you think?

@ggwpez
Copy link
Member

ggwpez commented Sep 9, 2022

maybe it would be nice to change the default implementation of the MultiplierUpdate to return 1 in the fn min()?

IMO it would be better to add a new implement for the trait, since we extensively use () in testing.
Something like ConstFeeMultiplier<Get<u32>> maybe.

@mrshiposha mrshiposha changed the title Fix FeeMultiplierUpdate in the template node Add ConstFeeMultiplier to the transaction payment pallet Sep 9, 2022
kianenigma
kianenigma approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Thanks! Can you put a polkadot address in the description for a tip https://github.com/apps/substrate-tip-bot?

@kianenigma kianenigma added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 9, 2022
@mrshiposha
Copy link
Contributor Author

Thanks! Can you put a polkadot address in the description for a tip https://github.com/apps/substrate-tip-bot?

Added, thank you! :)

ggwpez
ggwpez approved these changes Sep 10, 2022
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks!
CI is currently broken, but that's not your fault.

@ggwpez ggwpez added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Sep 10, 2022
@kianenigma
Copy link
Contributor

/tip small

@substrate-tip-bot
Copy link

@kianenigma A small tip was successfully submitted for MrShiposha (11Ze4Gi9iHZ1qPEweUewWk5iUTYL4Zd18VxKZ6cHhqyrfep on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

@kianenigma kianenigma mentioned this pull request Sep 12, 2022
Closed
@ggwpez
Copy link
Member

ggwpez commented Sep 12, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit c997a62 into paritytech:master Sep 12, 2022
@4meta5 4meta5 mentioned this pull request Oct 17, 2022
Closed
1 task
@github-actions github-actions bot mentioned this pull request Nov 10, 2022
Closed
@github-actions github-actions bot mentioned this pull request Nov 23, 2022
Closed
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@mrshiposha @ark0f
Add ConstFeeMultiplier to the transaction payment pallet (paritytec…
3ac7f22 
…h#12222)

* fix: FeeMultiplierUpdate

* fix: cargo fmt

* fix: rustdoc

* Revert "fix: rustdoc"

This reverts commit 96b6ad8.

* Revert "fix: cargo fmt"

This reverts commit 1301652.

* Revert "fix: FeeMultiplierUpdate"

This reverts commit 2cbddd0.

* feat: add ConstFeeMultiplier

* fix: use cConstFeeMultiplier in the template node
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@ggwpez ggwpez ggwpez approved these changes

Assignees
No one assigned
Labels
C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrshiposha @ggwpez @kianenigma