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

tx fees diff support #1083

Merged
merged 33 commits into from Jul 19, 2023
Merged

tx fees diff support #1083

merged 33 commits into from Jul 19, 2023

Conversation

Dengjianping
Copy link
Contributor

@Dengjianping Dengjianping commented Apr 21, 2023

Description

Closes #807

With this pr merge, we will no need to look at new weights when they're updated.
Maybe it's not a smart implementation, at least, we can get rid of reviewing weight numbers.

  • The code about estimating all tx fees locates at diff_gas_fees.rs. All computations base on actual fee multiplier.
  • Whatever the extrinsic are open to community or not, I added them to diff_gas_fees.rs.
  • All tx fees history locates at tx-fees-data.
  • If any tx fees fluctuates over 10%, the test case diff_gas_fees::diff_gas_fees will fail.

Before we can approve this PR for merge, please make sure that all the following items have been checked off:

  • Connected to an issue with discussion and accepted design using zenhub "Connect issue" button below
  • Added one label out of the L- group to this PR
  • Added one or more labels from the A- and C- groups to this PR
  • Explicitly labelled A-calamari, A-dolphin and/or A-manta if your changes are meant for/impact either of these (CI depends on it)
  • Re-reviewed Files changed in the Github PR explorer.

Situational Notes:

  • If adding functionality, write unit tests!
  • If importing a new pallet, choose a proper module index for it, and allow it in BaseFilter. Ensure every extrinsic works from front-end. If there's corresponding tool, ensure both work for each other.
  • If needed, update our Javascript/Typescript APIs. These APIs are officially used by exchanges or community developers.
  • If modifying existing runtime storage items, make sure to implement storage migrations for the runtime and test them with try-runtime. This includes migrations inherited from upstream changes, and you can search the diffs for modifications of #[pallet::storage] items to check for any.

Signed-off-by: Dengjianping <djptux@gmail.com>
Signed-off-by: Dengjianping <djptux@gmail.com>
@Dengjianping Dengjianping self-assigned this Apr 24, 2023
@Dengjianping Dengjianping added C-enhancement Category: An issue proposing an enhancement or a PR with one A-weights Area: Issues and PRs related to Substrate Weights A-calamari Area: Issues and PRs related to the Calamari Runtime A-ci Area: Continuous Integration A-testing Area: Testing-related Issues and PRs L-added Log: Issues and PRs related to addition labels Apr 24, 2023
@Dengjianping Dengjianping marked this pull request as ready for review April 24, 2023 10:08
Signed-off-by: Dengjianping <djptux@gmail.com>
@github-actions
Copy link

github-actions bot commented Apr 24, 2023

⚠️ Congestion test: 1-day congestion cost (calamari) is NOT above target_daily_congestion_cost_kma

runtime/calamari/tx-fees-data/README.md Outdated Show resolved Hide resolved
runtime/calamari/src/diff_gas_fees.rs Outdated Show resolved Hide resolved
Signed-off-by: Dengjianping <djptux@gmail.com>
Signed-off-by: Dengjianping <djptux@gmail.com>
@Dengjianping
Copy link
Contributor Author

One more question, should I add it to manta now?

Copy link
Contributor

@Garandor Garandor left a comment

Choose a reason for hiding this comment

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

need to expand coverage of multiple extrinsic call params

@Dengjianping
Copy link
Contributor Author

need to expand coverage of multiple extrinsic call params

I would cover those extrinsics which are open to users, but like governance extrinsics, I will ignore them, just leave one case there.

Signed-off-by: Dengjianping <djptux@gmail.com>
Signed-off-by: Dengjianping <djptux@gmail.com>
Signed-off-by: Dengjianping <djptux@gmail.com>
@Dengjianping Dengjianping mentioned this pull request Jun 15, 2023
5 tasks
Signed-off-by: Dengjianping <djptux@gmail.com>
Signed-off-by: Dengjianping <djptux@gmail.com>
Signed-off-by: Dengjianping <djptux@gmail.com>
Copy link
Contributor

@ghzlatarev ghzlatarev left a comment

Choose a reason for hiding this comment

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

@ghzlatarev
Copy link
Contributor

Also the unit tests are failing after the latest manta-sbt change

@github-actions
Copy link

github-actions bot commented Jul 17, 2023

⚠️ Congestion test: 1-day congestion cost (calamari-runtime) is NOT above the target daily congestion cost

@github-actions
Copy link

github-actions bot commented Jul 17, 2023

✅ Congestion test: 1-day congestion cost (manta-runtime) is above the target daily congestion cost

Copy link
Contributor

@ghzlatarev ghzlatarev left a comment

Choose a reason for hiding this comment

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

Seems we need to bump the timeouts of the integration tests jobs from 180 to 240 or something like that.

@ghzlatarev ghzlatarev self-requested a review July 18, 2023 12:53
@ghzlatarev ghzlatarev added the A-manta Area: Issues and PRs related to the Manta Runtime label Jul 19, 2023
@ghzlatarev ghzlatarev merged commit 05841c6 into manta Jul 19, 2023
56 checks passed
@ghzlatarev ghzlatarev deleted the diff-gas-fees branch July 19, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-calamari Area: Issues and PRs related to the Calamari Runtime A-ci Area: Continuous Integration A-manta Area: Issues and PRs related to the Manta Runtime A-testing Area: Testing-related Issues and PRs A-weights Area: Issues and PRs related to Substrate Weights C-enhancement Category: An issue proposing an enhancement or a PR with one L-added Log: Issues and PRs related to addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate Weight Comparison previous->current release
4 participants