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: discriminate extrinsic_base_weight based on dispatch class #414

Merged
merged 33 commits into from
Feb 9, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Feb 2, 2021

Info:

This PR addresses Issues #393. There were a number of changes to the structure of createCalcFee. Both on the typescript side and the rust-wasm package.

Changelog

@substrate/calc package (Rust-wasm)

./calc/src/calc_fee.rs

In this file I removed the extrinsic_base_weight from from_params which was getting called in createCalcFee once, and moved it to calc_fee. When fetch_block iterates through the extrinsic's it will call calc_fee and recalculate the fee based on the extrinsic dispatch class.

BlocksService (Typescript)

./src/services/blocks/BlocksService.ts

Added a private helper function getParentParentHash.

Removed the block parameter from createCalcFee and added parentParentHash instead. This is because the function createCalcFee only took in the block parameter for the parentParentHash runtime work around (which I removed from the function). I needed to find the parentParentHash inside of fetchBlock instead, therefore for optimization purposes I removed it to avoid multiple rpc calls.

I also transferred a lot of the extrinsicBaseWeight logic from createCalcFee and put it into fetchBlock where we iterate through the extrinsics. This is also why i make an rpc call for the metadata right before we iterate.

Extra

The one issue I am not too sure about is publishing @substrate/calc, currently I have the package set to file:./calc/pkg, not sure if i should change it back, but the tests will break if I do because it uses the older calc_fee.

@emostov
Copy link
Contributor

emostov commented Feb 2, 2021

The one issue I am not too sure about is publishing @substrate/calc, currently I have the package set to file:./calc/pkg, not sure if i should change it back, but the tests will break if I do because it uses the older calc_fee.

One way to handle this is once this PR is totally approved, npm publish calc from this branch, update the package.json to use that new version, make sure tests run and then merge this branch.

src/services/blocks/BlocksService.ts Show resolved Hide resolved
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Logic checks out with me with me. My main concern is just making sure we give a decent effort with optimization. I mention it in my code comments, but

  1. we should only fetch metadata as needed and it should be parallel with other querys if possible (I think this would mean doing it with the querys that are currently in createCalcFee.
  2. we should only need to create the decorated metadata once. (Potentially all of this could be done in createCalcFee and the decorated metadata could be returned if relevant)

The one other thing is just make sure to bump calc to 0.2.0

package.json Outdated
@@ -39,7 +39,7 @@
"@polkadot/api": "^3.6.4",
"@polkadot/apps-config": "^0.77.1",
"@polkadot/util-crypto": "^5.4.4",
"@substrate/calc": "^0.1.3",
"@substrate/calc": "file:./calc/pkg",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder that this should be change to "^0.2.0" before it gets merged.

weight,
len,
unadjusted_len_fee,
unadjusted_weight_fee,
adjustable_fee,
adjusted_fee,
base_fee,
result
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Cargo.toml version should get bumped to "0.2.0" since this is a breaking change, but we are on major 0 so we just bump minor. After bumping in Cargo.toml you should be able to run build.sh and the package.json should update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rad, thanks for explaining. Yea was a little confused at first with the versioning.

src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
TarikGul and others added 6 commits February 4, 2021 10:58
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
src/services/blocks/BlocksService.ts Show resolved Hide resolved
src/types/responses/Block.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Show resolved Hide resolved
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

LGTM!

All thats left to do is publish calc to npm, update sidecars package.json to use calc from npm, run tests and merge

TarikGul and others added 4 commits February 5, 2021 22:18
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
@TarikGul TarikGul merged commit ff98c76 into master Feb 9, 2021
@TarikGul TarikGul deleted the base-extrinsic-calc-fee branch February 9, 2021 19:29
This pull request was closed.
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.

4 participants