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

Impl transaction-payment GenesisConfig with NextFeeMultplier support#12177

Merged
paritytech-processbot[bot] merged 5 commits intoparitytech:masterfrom
notlesh:notlesh-transaction-payment-multiplier-genesis
Sep 5, 2022
Merged

Impl transaction-payment GenesisConfig with NextFeeMultplier support#12177
paritytech-processbot[bot] merged 5 commits intoparitytech:masterfrom
notlesh:notlesh-transaction-payment-multiplier-genesis

Conversation

@notlesh
Copy link
Contributor

@notlesh notlesh commented Sep 2, 2022

This allows an initial value for NextFeeMultiplier to be provided at genesis.

The current implementation hard-codes the initial value to FixedU128::saturating_from_integer(1) when it is absent. While this can be adjusted arbitrarily with FeeMultiplierUpdate, this doesn't kick in until the end of block 1.

This means that block 1 fees are impractical on any runtime for which 1 is not a sane value. This is particularly problematic for testing.

An alternative approach would be to add an initial value to the runtime config, but this didn't seem worth the extra bloat to me.

@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Sep 3, 2022
@dmitry-markin
Copy link
Contributor

dmitry-markin commented Sep 3, 2022

Looks good to me, but I'll let more experienced colleagues to check (assigned). Could you run cargo +nightly fmt to make the gitlab-cargo-fmt CI check happy?

@ggwpez
Copy link
Member

ggwpez commented Sep 4, 2022

/cmd queue -c fmt $ 1

@command-bot
Copy link

command-bot bot commented Sep 4, 2022 •
edited
Loading

"$PIPELINE_SCRIPTS_DIR/fmt.sh" 1 was queued.

Comment /cmd cancel 46-b734b1cc-f095-4642-9683-83be50c6c0d6 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 4, 2022

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/fmt.sh" 1 has finished. Result:

Error: remote: Gitlab::GitAccess::ForbiddenError
fatal: unable to access 'https://token:{SECRET}@gitlab.parity.io/parity/mirrors/substrate.git/': The requested URL returned error: 403
Error: remote: Gitlab::GitAccess::ForbiddenError
fatal: unable to access 'https://token:{SECRET}@gitlab.parity.io/parity/mirrors/substrate.git/': The requested URL returned error: 403
    at ChildProcess.<anonymous> (file:///app/src/shell.ts:127:27)
    at ChildProcess.emit (node:events:390:28)
    at ChildProcess.emit (node:domain:475:12)
    at maybeClose (node:internal/child_process:1064:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:301:5)

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

@ggwpez ggwpez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fmt bot does not seem to work yet.
Btw you could add a test for it, but i dont think its necessary.

@shawntabrizi
Copy link
Member

shawntabrizi commented Sep 4, 2022

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented Sep 4, 2022

Waiting for commit status.

@paritytech-processbot
Copy link

paritytech-processbot bot commented Sep 4, 2022

Merge cancelled due to error. Error: Statuses failed for 5ed0add

@notlesh
Copy link
Contributor Author

notlesh commented Sep 4, 2022

I'll be happy to add a test, and I'd also like to create and document a const for the value 1.

Also, what's this gitspiegel failure?

@shawntabrizi
Copy link
Member

shawntabrizi commented Sep 4, 2022

Also, what's this gitspiegel failure?

No idea, probably intermittent, and you can ignore

notlesh
notlesh commented Sep 4, 2022
View reviewed changes
frame/transaction-payment/src/lib.rs

/// Default value for NextFeeMultiplier. This is used in genesis and is also used in
/// NextFeeMultiplierOnEmpty() to provide a value when none exists in storage.
const MULTIPLIER_DEFAULT_VALUE: Multiplier = Multiplier::from_u32(1);
Copy link
Contributor Author

@notlesh notlesh Sep 4, 2022

Choose a reason for hiding this comment

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

Please review, esp. that from_u32(1) is equivalent to saturating_from_integer(1). The latter wasn't const and couldn't be used here.

My tests should verify this, but I'd appreciate another look at it.

@notlesh
Copy link
Contributor Author

notlesh commented Sep 4, 2022

Fmt bot does not seem to work yet. Btw you could add a test for it, but i dont think its necessary.

I added a couple very basic tests. If there's something else you'd like to see tested, let me know.

kianenigma
kianenigma approved these changes Sep 4, 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.

@dmitry-markin don't see why this needs an audit?

@dmitry-markin
Copy link
Contributor

dmitry-markin commented Sep 4, 2022

@dmitry-markin don't see why this needs an audit?

@kianenigma guess I missed the part of the label description stating that external audit is required.

@dmitry-markin dmitry-markin removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Sep 4, 2022
@ggwpez
Copy link
Member

ggwpez commented Sep 5, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit fd399c1 into paritytech:master Sep 5, 2022
@notlesh notlesh mentioned this pull request Jan 9, 2023
Merged
5 tasks
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@notlesh @shawntabrizi @ark0f
Impl transaction-payment GenesisConfig with NextFeeMultplier support (p…
7bfcc5e 
…aritytech#12177)

* Impl GenesisConfig with NextFeeMultplier support

* Update lib.rs

* Use documented const

* Unit test multiplier genesis

* fmt

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@KiChjang KiChjang KiChjang approved these changes

@kianenigma kianenigma kianenigma approved these changes

@bkchr bkchr bkchr approved these changes

@ggwpez ggwpez ggwpez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@notlesh @dmitry-markin @ggwpez @shawntabrizi @KiChjang @kianenigma @bkchr