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

Apply WeightToFeePolynomials to pallet_transaction_payment's length fee #10785

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Feb 2, 2022

This PR is one potential way to address #10784. It adds the same WeightToFeePolynomial mechanics used for weight fees in pallet_transaction_payment to length fee. The main goal is to provide more flexibility in charging for transaction size; see the issue for more info on the motivation.

This PR would need substantial work, and would probably introduce breaking changes wherever the pallet's config is used.

polkadot companion: paritytech/polkadot#5028
cumulus companion: paritytech/cumulus#1152

@ggwpez ggwpez added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 3, 2022
@shawntabrizi
Copy link
Member

seems like a fine change to me

@@ -536,6 +538,10 @@ where
}
}

fn length_to_fee(length: u32) -> BalanceOf<T> {
T::LengthToFee::calc(&(length as u64)) // TODO: saturating / upper bound
Copy link
Contributor Author

@notlesh notlesh Mar 3, 2022

Choose a reason for hiding this comment

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

I had left this TODO here because fn weight_to_fee below provides an upper bound.

While I'm not quite sure why it's needed in weight_to_fee, it's guaranteed to be no larger than u32::MAX in the case of length_to_fee, which is likely much smaller than max_block.

Another pair of eyes would be great though.

@notlesh notlesh marked this pull request as ready for review March 3, 2022 21:50
frame/support/src/weights.rs Outdated Show resolved Hide resolved
frame/transaction-payment/src/lib.rs Outdated Show resolved Hide resolved
frame/transaction-payment/src/lib.rs Outdated Show resolved Hide resolved
@notlesh
Copy link
Contributor Author

notlesh commented Mar 29, 2022

Any update on this, esp. @kianenigma ?

@kianenigma kianenigma added A8-mergeoncegreen and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 31, 2022
@kianenigma
Copy link
Contributor

some warnings seem legit, CI will run now.

@notlesh
Copy link
Contributor Author

notlesh commented Apr 6, 2022

I'm guessing my polkadot companion PR has CI blocked as well:

paritytech/polkadot#5028

@notlesh
Copy link
Contributor Author

notlesh commented Apr 6, 2022

My companion polkadot PR is building locally for me. If I'm reading CONTRIBUTING.adoc correctly, I think its CI is expected to be failing at this point. Let me know if I can do anything else to push this along.

@notlesh
Copy link
Contributor Author

notlesh commented Apr 7, 2022

I added a cumulus companion PR: paritytech/cumulus#1152

It may need CI enabled for me.
Also, I think it and my polkadot companion (paritytech/polkadot#5028) need labels added.

Thanks :)

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit a1497f5 into paritytech:master Apr 7, 2022
acatangiu pushed a commit to acatangiu/substrate that referenced this pull request Apr 8, 2022
…th fee (paritytech#10785)

* Apply WeightToFeePolynomials to length fee

* Remove TransactionByteFee

* Add test cases for ConstantModifierFee

* Restore import

* Use pallet::constant_name

* Remove irrelevant TODO comment

* Update frame/support/src/weights.rs

* Update frame/transaction-payment/src/lib.rs

* Update frame/transaction-payment/src/lib.rs

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* s/ConstantModifierFee/ConstantMultiplier/

* Impl LengthToFee for test configs

* fmt

* Remove unused import

* Impl WeightToFeePolynomial for byte fee in ExtBuilder

* Remove unused import

* fix doc

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
…th fee (paritytech#10785)

* Apply WeightToFeePolynomials to length fee

* Remove TransactionByteFee

* Add test cases for ConstantModifierFee

* Restore import

* Use pallet::constant_name

* Remove irrelevant TODO comment

* Update frame/support/src/weights.rs

* Update frame/transaction-payment/src/lib.rs

* Update frame/transaction-payment/src/lib.rs

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* s/ConstantModifierFee/ConstantMultiplier/

* Impl LengthToFee for test configs

* fmt

* Remove unused import

* Impl WeightToFeePolynomial for byte fee in ExtBuilder

* Remove unused import

* fix doc

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…th fee (paritytech#10785)

* Apply WeightToFeePolynomials to length fee

* Remove TransactionByteFee

* Add test cases for ConstantModifierFee

* Restore import

* Use pallet::constant_name

* Remove irrelevant TODO comment

* Update frame/support/src/weights.rs

* Update frame/transaction-payment/src/lib.rs

* Update frame/transaction-payment/src/lib.rs

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* s/ConstantModifierFee/ConstantMultiplier/

* Impl LengthToFee for test configs

* fmt

* Remove unused import

* Impl WeightToFeePolynomial for byte fee in ExtBuilder

* Remove unused import

* fix doc

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
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.
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
None yet
Development

Successfully merging this pull request may close these issues.

7 participants