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

fix: Update fee calc to use system::constants::BlockWeights.per_class.normal.base_extrinsic #388

Merged
merged 12 commits into from
Jan 18, 2021

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jan 14, 2021

closes: #385

For review discussion

Should we take into account DispatchClass when using baseExtrinsic? At the moment polkadot runtimes have the baseExtrinsic for each class is fixed: https://github.com/paritytech/polkadot/blob/cd0e9186a6620aab846a8a54969a0e5bbecbd060/runtime/common/src/lib.rs#L88

From my understanding the changes would roughly require a refactor of the calc_fee.rs logic to do something like take in the dispatch class for each calc_fee() call and pass a mapping of the base_extrinsic weight for each dispatch class when creating CalcFee.from_params().

My two cents is that we should ship this fix ASAP, and then get the more thorough fix out next week.

@emostov emostov marked this pull request as draft January 14, 2021 02:07
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Please keep in mind that this only works for non-normal transactions if they share the same base extrinsic weight.

src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
@emostov emostov added B3 - API Noteworthy API is not broken, but worth noting I2 - Bug 🐜 Sidecar has a bug labels Jan 14, 2021
@emostov emostov changed the title fix: Update fee calc to use system::blockWeight fix: Update fee calc to use system::constants::BlockWeights.per_class.normal.base_extrinsic Jan 14, 2021
@emostov emostov marked this pull request as ready for review January 14, 2021 20:30
@emostov emostov added the P2 - ASAP Get fixed ASAP label Jan 15, 2021
Comment on lines 344 to 346
? (api.consts.system.extrinsicBaseWeight as AbstractInt)
: // We assume all dispatch classes have the same base_extrinsic value
api.consts.system.blockWeights.perClass.normal.baseExtrinsic;

Choose a reason for hiding this comment

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

so the js api has the value api.consts.system.extrinsicBaseWeight set before the weight refactor and for a chain that has it, the latter one?

Copy link
Contributor

Choose a reason for hiding this comment

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

If ^^^ is correct it'd be good to annotate the code above with something like "Behaviour changed with substrate PR #6629 (polkadot runtime vX.Y.Z)".

@kianenigma
Copy link

Sensible to do as you said:

My two cents is that we should ship this fix ASAP, and then get the more thorough fix out next week.

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

I have a poor understanding on what is going on here, apologies.

I think this change merits a test or two.

Comment on lines 344 to 346
? (api.consts.system.extrinsicBaseWeight as AbstractInt)
: // We assume all dispatch classes have the same base_extrinsic value
api.consts.system.blockWeights.perClass.normal.baseExtrinsic;
Copy link
Contributor

Choose a reason for hiding this comment

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

If ^^^ is correct it'd be good to annotate the code above with something like "Behaviour changed with substrate PR #6629 (polkadot runtime vX.Y.Z)".

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM; we'll revisit this more thoroughly in subsequent releases.

@emostov
Copy link
Contributor Author

emostov commented Jan 15, 2021

@TarikGul , @dvdplm , @kianenigma - I just re-did the whole thing. I forgot that the api always uses the latests metadata. If we are doing a query in a historical runtime we need to fetch the old metadata, decorate it, and grab the constant. If we are doing a query in the latest runtime we can save the effort and just use the decorated metadata built into the api. The logic is worth another look over

?.extrinsicBaseWeight as AbstractInt;
const extrinsicBaseWeightExists =
api.consts.system.extrinsicBaseWeight ||
api.consts.system.blockWeights.perClass.normal.baseExtrinsic;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are making an assumption here that if a runtime has one of these in the metadata currently, all past runtimes will have it. Since we only support fee calcs on a limited number of chains I think this is ok for now

@dvdplm
Copy link
Contributor

dvdplm commented Jan 18, 2021

The logic is worth another look over

Still LGTM.

@emostov emostov merged commit 5ec24e6 into master Jan 18, 2021
@emostov emostov deleted the zeke-fix-weight branch January 18, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B3 - API Noteworthy API is not broken, but worth noting I2 - Bug 🐜 Sidecar has a bug P2 - ASAP Get fixed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

partialFee value is always 0 on local development with polkadot 1.8.27
4 participants