Conversation
WalkthroughThis PR introduces pallet_xcm weight benchmarking support by adding conditional WeightInfo trait implementations in the template, wiring pallet_xcm configuration in the runtime, creating autogenerated weight modules, and extending benchmark dependencies and scripts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
runtime/battery-station/src/lib.rs (1)
21-21: Verify necessity of broad lint suppression.The
#![allow(non_local_definitions)]attribute suppresses warnings crate-wide. This is often needed for macro-generated code in Substrate runtimes, but consider documenting why this suppression is necessary (e.g., which macros trigger it) to help future maintainers understand the rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
misc/frame_weight_template.hbs(1 hunks)runtime/battery-station/src/lib.rs(1 hunks)runtime/common/Cargo.toml(5 hunks)runtime/common/src/lib.rs(6 hunks)runtime/common/src/weights/mod.rs(1 hunks)runtime/common/src/weights/pallet_xcm.rs(1 hunks)runtime/zeitgeist/src/lib.rs(1 hunks)scripts/benchmarks/configuration.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Coverage
- GitHub Check: Test parachain build
- GitHub Check: Fuzz
- GitHub Check: Quick check benchmarks
- GitHub Check: Test standalone build
- GitHub Check: Format
- GitHub Check: Checks (parachain)
- GitHub Check: Checks (clippy)
- GitHub Check: Checks (standalone)
- GitHub Check: Summary
🔇 Additional comments (10)
scripts/benchmarks/configuration.sh (1)
16-20: LGTM!The addition of
pallet_xcmto the parachain pallets benchmarking list is correct and aligns with the PR objectives.runtime/zeitgeist/src/lib.rs (1)
21-21: Same lint suppression as battery-station runtime.See the comment on
runtime/battery-station/src/lib.rs:21regarding this lint suppression.runtime/common/Cargo.toml (1)
1-108: LGTM!The addition of benchmarking and XCM dependencies is well-structured with proper optional dependency handling and feature flags. The use of
?syntax correctly handles conditional feature enablement.runtime/common/src/lib.rs (3)
616-619: LGTM - Appropriate security configuration.The conditional
XcmReserveTransferFilteris correctly configured:
Everythingduring benchmarks allows testing all code pathsNothingin production is a safe default that restricts reserve transfersThis is a security-sensitive setting and the conservative production default is appropriate.
628-628: LGTM - Switches to production weights.Correctly replaces
TestWeightInfowith the autogeneratedweights::pallet_xcm::WeightInfo<Runtime>. This is essential for proper weight accounting in production.
1783-2012: LGTM - Comprehensive benchmarking configuration.The pallet_xcm benchmarking setup is well-implemented:
- Properly gated with
#[cfg(feature = "parachain")]TestDeliveryHelpercorrectly funds accounts for benchmark executionbenchmarking::Configimplementation provides all required methods- Asset and location handling appears correct
runtime/common/src/weights/mod.rs (1)
27-27: LGTM!The export of
pallet_xcmweights module is correctly placed under theparachainfeature guard, consistent with other parachain-specific weight modules.misc/frame_weight_template.hbs (1)
23-23: LGTM - Handles pallet_xcm's different module structure.The conditional logic correctly accounts for
pallet_xcmhaving itsWeightInfotrait directly in the pallet module (pallet_xcm::WeightInfo) rather than in a submodule (pallet_xcm::weights::WeightInfo) like other pallets.runtime/common/src/weights/pallet_xcm.rs (2)
72-80: Verify intentionally disabled benchmarks.The
teleport_assetsfunction shows a weight of18_446_744_073_709_551_000(u64::MAX) and referencesBenchmark::Overridestorage, suggesting this benchmark is intentionally disabled or not supported. Please confirm this is intentional and consider adding a comment explaining why these operations are not benchmarked (e.g., if teleport assets are not supported in this runtime configuration).The same pattern appears for the
executefunction at lines 123-130.
56-320: LGTM - Autogenerated weights look correct.The autogenerated weight implementations follow the expected pattern with proper storage access annotations and reasonable weight calculations. The file is well-structured and includes all necessary pallet_xcm extrinsics.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1450 +/- ##
=======================================
Coverage 93.32% 93.32%
=======================================
Files 181 181
Lines 34769 34769
=======================================
Hits 32448 32448
Misses 2321 2321
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Chralt98
added
s:accepted
3 participants
What does it do?
Fixes #1425
What important points should reviewers know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues?
References
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.