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

Allow configuration of worst case buy execution weight #7944

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

girazoki
Copy link
Contributor

Adds worst_case_buy_execution to the Config trait of pallet-xcm-benchmarks with a default implementation that mimics the code that existed previous to this PR.

Rationale: not allowing to set the WeightLimit and the FeeAsset might mean that we dont benchmark the worst case, as with WeightLimit::Unlimited the Trader does not even execute:

if let Some(weight) = Option::<Weight>::from(weight_limit) {

The new configurable function allows projects to customize the parameters with which the benchmark is run to make sure they account for the worst-case scenario

This is very likely the case of the assethub system chain, with several traders being analyzed and possibly several reads being made:

cumulus_primitives_utility::TakeFirstAssetTrader<

@girazoki girazoki requested a review from a team as a code owner March 17, 2025 12:48
@girazoki
Copy link
Contributor Author

CC I am re-opening this #2962. I think it is still valid as right now the buy_execution instruction is being benchmarked with the (Here) asset, which might not include the worst case.

CC @bkchr sorry for not answering before on the closed PR, I somehow missed it

@girazoki
Copy link
Contributor Author

I remember back in that PR there were a couple of things that needed to be analyzed:

  • do we want a default impl for the new helper?
  • what is the worst case for assethub? (in which case, I can add it)

@girazoki
Copy link
Contributor Author

CC @RomarQ

@acatangiu
Copy link
Contributor

cc @franciscoaguirre @bkontur

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.

2 participants