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: add support for Parallel and Heiko #1012

Merged
merged 4 commits into from
Aug 8, 2022

Conversation

mclyk
Copy link
Contributor

@mclyk mclyk commented Aug 5, 2022

No description provided.

@mclyk mclyk requested a review from a team as a code owner August 5, 2022 14:35
@TarikGul
Copy link
Member

TarikGul commented Aug 5, 2022

hey @MrPai Please see this comment I put on a similar PR that was just submitted: #1009 (comment)

Same applies to you in terms of keeping the PR up.

@mclyk
Copy link
Contributor Author

mclyk commented Aug 6, 2022

@TarikGul reviewed the explaining in another PR, thanks for it, if it's possible to merge it if you think the PR is ok, because one of our cooperative partners relies on your repo, hope this will not block them for testing and developing.

Comment on lines 88 to 90
("heiko", _v) => V2(new_u128(inner)),
("parallel", _v) => V2(new_u128(inner)),

Copy link
Contributor

@Imod7 Imod7 Aug 6, 2022

Choose a reason for hiding this comment

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

@MrPai Thank you for your PR!
As already mentioned, if you would like this PR to be merged ASAP you can do the following :

  1. Revert/remove this change in calc_fee and
  2. Revert change in calc/pkg/calc_bg.wasm
  3. Change the title of the PR to something that it reflects the change in the chains config, e.g. "fix: add support (controllers) for Parallel and Heiko" (just an example).

Regarding the fee calculation, adding anything to calc_fee will not have any impact since we do not use it anymore. Please check the related comments here and here by Tarik and James.

Having said that, for fees we are advising to use the ?feeByEvent=true query param found in the /blocksrelated endpoints. Please check also the endpoints docs where it mentions more details on feeByEvent :

When set to true, extrinsics will have their fees retrieved by their events. This query param will use a fee estimation via rpc::payment::query_info to accurately identify which event has the corresponding fee information that is closest to the value provided by query_info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Imod7 thanks for your information, I have done what you mentioned, please take a look.

@mclyk mclyk changed the title fix: support calc fee for Parallel and Heiko fix: add support for Parallel and Heiko Aug 6, 2022
@mclyk mclyk requested a review from Imod7 August 6, 2022 08:10
@mclyk
Copy link
Contributor Author

mclyk commented Aug 8, 2022

Hi, @TarikGul @Imod7 do we have any updates about this pr?

@TarikGul
Copy link
Member

TarikGul commented Aug 8, 2022

@MrPai Hey LGTM, it'll go in tomorrow, then a release will happen on tuesday which will include these changes. It's still Sunday for the team so it will get another review tomorrow.

@mclyk
Copy link
Contributor Author

mclyk commented Aug 8, 2022

@TarikGul Thanks, have a nice day.

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

LGTM

@TarikGul TarikGul merged commit 8770f3a into paritytech:master Aug 8, 2022
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.

3 participants