-
Notifications
You must be signed in to change notification settings - Fork 369
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 (
|
pallets/xcm-weight-trader/src/lib.rs
Outdated
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.
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.
Just replace amount
by amount_to_refund
lines 439 and 441.
The amount
is not what we refund, it's what the user actually have to pay
WASM runtime size check:Compared to target branchMoonbase runtime: 2348 KB (-8 KB) ✅ Moonbeam runtime: 2500 KB (no changes) 🚨 Moonriver runtime: 2504 KB (+4 KB) 🚨 Compared to latest release (runtime-3800)Moonbase runtime: 2348 KB (+304 KB compared to latest release) Moonbeam runtime: 2500 KB (+336 KB compared to latest release) 🚨 Moonriver runtime: 2504 KB (+340 KB compared to latest release) 🚨 |
Moonbase Weight Difference Report
Moonriver Weight Difference Report
Moonbeam Weight Difference Report
|
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.
About the very high cost for some XCM instructions, like WithdrawAsset:
I think it’s expected because these instructions can read an EVM contract and therefore add up to 24 KB of proof size—but in 99% of cases, that cost is too high for the user.
The ideal solution would be to have dynamic weight computation for XCM instructions that can read AccountCodesMetadata and charge accordingly.
If we can’t have dynamic weight computation, we could add a check in the pallet-ethereum-xcm to read AccountCodesMetadata and fail—without reading the actual bytecode—if the size exceeds a threshold. That threshold should be high enough to accommodate any legitimate ERC-20 (probably around 12 KB), so the cost will still be high for users.
If pallet-xcm benchmarks don’t support dynamic weight computation for XCM instructions, it may be a good reason to maintain a custom benchmarking pallet on our side.
Xcm instructions are now benchmarked and as a result the wights are now different than before