Allow to set a worst case buy execution fee asset and weight#2962
Allow to set a worst case buy execution fee asset and weight#2962girazoki wants to merge 4 commits intoparitytech:masterfrom
Conversation
| let fee_asset = AssetId(Here.into()); | ||
| // The worst case we want for buy execution in terms of | ||
| // fee asset and weight | ||
| let (fee_asset, weight_limit) = T::worst_case_buy_execution()?; |
There was a problem hiding this comment.
I definitely like the idea of benchmarking with WeightLimit::Limited so we can hit more logic. There's now a fee_asset configuration item that is used to buying execution in the benchmark for refund_surplus, this could be merged with that to include the weight_limit.
We also have to make sure this is in holding, don't know if there's a good way to do it.
There was a problem hiding this comment.
ok I can try to do this!
There was a problem hiding this comment.
For holding, yes we rely on worst_case_holding having the fees, and I guess that if it is not the benchmark will fail. So I think this is good enough.
The other way we could do it is by injecting the fee_asset into holding directly, without using worst_case_holding at all
There was a problem hiding this comment.
@franciscoaguirre I dont see the fee_asset setup in refund_surplus. all I see is that the total_surplus is artificially set
| /// asset to trigger the Trader::buy_execution in the XCM executor | ||
| /// By default returns ((AssetId(Here.into()), 100_000_000u128), Unlimited) | ||
| fn worst_case_buy_execution() -> Result<(Asset, WeightLimit), BenchmarkError> { | ||
| Ok(((AssetId(Junctions::Here.into()), 100_000_000u128).into(), WeightLimit::Unlimited)) |
There was a problem hiding this comment.
I don't think we need a default for this. More often than not the worst case is Limited and we have many examples that don't buy fees with Here
There was a problem hiding this comment.
What should I configure in this PR for all the runtimes? Use the same values they had before? Because I dont think I know all the polkadot/cumulus runtimes as well to know what is the worst usecase for each of them..
|
@franciscoaguirre do you have any more input on this? If I dont provide the default implementation I dont think I know enough of the runtimes myself to implement the worst case scenario in each of the cumulus/relay runtimes |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
@girazoki is this still applicable? |
|
Closing as stale, please reopen if needed. |
|
I think it is still aplicable, I willl open a new PR. |
|
Sorry that I forgot about this and it became stale |
5 participants
Adds
worst_case_buy_executionto the Config trait ofpallet-xcm-benchmarkswith a default implementation that mimics the code that existed previous to this PR.Rationale: not allowing to set the
WeightLimitand theFeeAssetmight mean that we dont benchmark the worst case, as withWeightLimit::UnlimitedtheTraderdoes not even execute:polkadot-sdk/polkadot/xcm/xcm-executor/src/lib.rs
Line 833 in c01dbeb
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