-
Notifications
You must be signed in to change notification settings - Fork 366
Enable benchmarks for pallet_xcm_benchmarks #3330
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
base: master
Are you sure you want to change the base?
Conversation
…pallet_xcm_benchmarks::generic`
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
let weight = actual_weight.min(self.0); | ||
let amount: u128 = | ||
Self::compute_amount_to_charge(&weight, &location).unwrap_or(u128::MAX); | ||
let amount_to_refund = initial_amount.saturating_sub(amount); | ||
self.0 -= weight; | ||
self.1 = Some(Asset { | ||
fun: Fungibility::Fungible(amount_to_refund), | ||
id: XcmAssetId(location.clone()), | ||
}); | ||
log::trace!( | ||
target: "xcm-weight-trader", | ||
"refund_weight amount to refund: {:?}", | ||
amount_to_refund | ||
); | ||
if amount > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the implementation the same as https://paritytech.github.io/polkadot-sdk/master/staging_xcm_builder/struct.UsingComponents.html
This resulted in fixing refund_surplus instruction benchmark test which was failing before.
// 50 fungibles | ||
const HOLDING_FUNGIBLES: u32 = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bring down the number of assets in the holding register to 50 to avoid ExceedsMaxMessageSize
when running report_holding benchmark.
@@ -986,14 +986,39 @@ macro_rules! impl_runtime_apis_plus_common { | |||
); | |||
XcmWeightTrader::set_asset_price( | |||
location.clone(), | |||
1u128.pow(18) | |||
10u128.pow(18) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This typo resulted in miscalculation in the fee price price = (native_price * 10^18) / relative_price
Sorry, something went wrong.
All reactions
-
👍 1 reaction
@@ -1112,9 +1144,6 @@ macro_rules! impl_runtime_apis_plus_common { | |||
|
|||
add_benchmarks!(params, batches); | |||
|
|||
if batches.is_empty() { | |||
return Err("Benchmark not found for this pallet.".into()); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines would cause benchmark to be reported as failed if it was skipped
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: polkadot-fellows/runtimes#446
Sorry, something went wrong.
All reactions
-
❤️ 1 reaction
runtime/moonbase/src/xcm_config.rs
Outdated
@@ -113,10 +115,30 @@ parameter_types! { | |||
}; | |||
} | |||
|
|||
#[cfg(feature = "runtime-benchmarks")] | |||
pub struct BenchAccountId32Aliases<AccountId>(PhantomData<AccountId>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A workaround to pass this line https://github.com/paritytech/polkadot-sdk/blob/45cc86a4588fdae2929f9829f3a1d196a985afb5/polkadot/xcm/pallet-xcm-benchmarks/src/lib.rs#L111-L112
Since we don't use 32 byte accounts.
Sorry, something went wrong.
All reactions
// 100 fungibles | ||
const HOLDING_FUNGIBLES: u32 = 100; | ||
// 50 fungibles | ||
const HOLDING_FUNGIBLES: u32 = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could even do:
const HOLDING_FUNGIBLES: u32 = 50; | |
const HOLDING_FUNGIBLES: u32 = MaxAssetsIntoHolding::get(); |
Sorry, something went wrong.
All reactions
-
👍 1 reaction
WASM runtime size check:Compared to target branchMoonbase runtime: 2372 KB (-8 KB) ✅ Moonbeam runtime: 2356 KB (-16 KB) ✅ Moonriver runtime: 2360 KB (-12 KB) ✅ Compared to latest release (runtime-3701)Moonbase runtime: 2372 KB (+316 KB compared to latest release) Moonbeam runtime: 2356 KB (+308 KB compared to latest release) Moonriver runtime: 2360 KB (+308 KB compared to latest release) |
All reactions
Sorry, something went wrong.
RomarQ
At least 1 approving review is required to merge this pull request.
Successfully merging this pull request may close these issues.
Xcm instructions are now benchmarked and as a result the wights are now different than before