Remove moonbeam-xcm-benchmarks and only use pallet-xcm-benchmarks#3318
Remove moonbeam-xcm-benchmarks and only use pallet-xcm-benchmarks#3318
moonbeam-xcm-benchmarks and only use pallet-xcm-benchmarks#3318Conversation
|
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 WalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime
participant WeightsModule
participant XCMInstruction
Runtime->>WeightsModule: Request weight for XCM instruction (e.g., transfer_asset)
WeightsModule->>WeightsModule: Select appropriate weight function (fungible/generic)
WeightsModule->>XCMInstruction: Aggregate per-asset or per-instruction weights
WeightsModule-->>Runtime: Return computed weight
Suggested labels
Poem
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 (
|
Moonbase Weight Difference Report
Moonriver Weight Difference Report
Moonbeam Weight Difference Report
|
TarekkMA
added
B7-runtimenoteworthy
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
runtime/moonriver/src/weights/xcm/generic.rs (1)
405-412: Fix identical execution time overflow in export_message function.Same issue as in the Moonbeam version - the execution time appears to be a u64 overflow.
This is the same critical issue found in
runtime/moonbeam/src/weights/xcm/generic.rsat lines 405-412. The minimum execution time needs to be corrected.runtime/moonriver/src/weights/xcm/mod.rs (1)
139-148: Address commented logic (duplicate of Moonbeam issue).Same issue as in the Moonbeam version regarding uncertain weight calculation for
initiate_reserve_withdraw.This is the same issue found in
runtime/moonbeam/src/weights/xcm/mod.rsat lines 139-148. The commented code needs clarification.
🧹 Nitpick comments (2)
runtime/moonbeam/src/xcm_config.rs (1)
63-63: Remove unused import
The importuse pallet_evm::Call::create;does not appear to be used in this file. Please remove it to avoid dead code and satisfy linting rules.runtime/moonbeam/src/weights/xcm/mod.rs (1)
105-120: Consider more nuanced weight handling for unsupported HRMP operations.While returning
Weight::MAXis safe for unsupported HRMP channel operations, this might be overly conservative and could prevent legitimate XCM programs from executing efficiently.Consider returning a more measured weight that reflects the actual cost of rejecting the operation:
fn hrmp_new_channel_open_request( _sender: &u32, _max_message_size: &u32, _max_capacity: &u32, ) -> XCMWeight { - // XCM Executor does not currently support HRMP channel operations - Weight::MAX + // XCM Executor does not currently support HRMP channel operations + // Return a reasonable weight for operation rejection + XcmGeneric::<Runtime>::trap() }
📜 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 (31)
Cargo.toml(0 hunks)pallets/moonbeam-xcm-benchmarks/Cargo.toml(0 hunks)pallets/moonbeam-xcm-benchmarks/src/generic/benchmarking.rs(0 hunks)pallets/moonbeam-xcm-benchmarks/src/generic/mock.rs(0 hunks)pallets/moonbeam-xcm-benchmarks/src/generic/mod.rs(0 hunks)pallets/moonbeam-xcm-benchmarks/src/lib.rs(0 hunks)pallets/moonbeam-xcm-benchmarks/src/mock.rs(0 hunks)runtime/common/Cargo.toml(0 hunks)runtime/common/src/apis.rs(2 hunks)runtime/moonbase/Cargo.toml(0 hunks)runtime/moonbase/src/lib.rs(1 hunks)runtime/moonbase/src/weights/mod.rs(1 hunks)runtime/moonbase/src/weights/xcm/mod.rs(1 hunks)runtime/moonbase/src/xcm_config.rs(1 hunks)runtime/moonbase/tests/integration_test.rs(2 hunks)runtime/moonbeam/Cargo.toml(0 hunks)runtime/moonbeam/src/lib.rs(1 hunks)runtime/moonbeam/src/weights/mod.rs(1 hunks)runtime/moonbeam/src/weights/xcm/fungible.rs(1 hunks)runtime/moonbeam/src/weights/xcm/generic.rs(1 hunks)runtime/moonbeam/src/weights/xcm/mod.rs(1 hunks)runtime/moonbeam/src/xcm_config.rs(2 hunks)runtime/moonbeam/tests/integration_test.rs(1 hunks)runtime/moonriver/Cargo.toml(0 hunks)runtime/moonriver/src/lib.rs(1 hunks)runtime/moonriver/src/weights/mod.rs(1 hunks)runtime/moonriver/src/weights/xcm/fungible.rs(1 hunks)runtime/moonriver/src/weights/xcm/generic.rs(1 hunks)runtime/moonriver/src/weights/xcm/mod.rs(1 hunks)runtime/moonriver/src/xcm_config.rs(1 hunks)runtime/moonriver/tests/integration_test.rs(1 hunks)
💤 Files with no reviewable changes (11)
- runtime/moonbeam/Cargo.toml
- runtime/common/Cargo.toml
- runtime/moonriver/Cargo.toml
- Cargo.toml
- runtime/moonbase/Cargo.toml
- pallets/moonbeam-xcm-benchmarks/src/lib.rs
- pallets/moonbeam-xcm-benchmarks/src/generic/mod.rs
- pallets/moonbeam-xcm-benchmarks/Cargo.toml
- pallets/moonbeam-xcm-benchmarks/src/mock.rs
- pallets/moonbeam-xcm-benchmarks/src/generic/mock.rs
- pallets/moonbeam-xcm-benchmarks/src/generic/benchmarking.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-coverage
- GitHub Check: build
- GitHub Check: rust-test
🔇 Additional comments (26)
runtime/moonbase/src/weights/mod.rs (1)
60-60: Publicly export new XCM weights module
Thexcmmodule is now exposed to integrate the local XCM weight calculations replacing the removed external benchmarks.runtime/moonbeam/src/weights/mod.rs (1)
59-59: Publicly export new XCM weights module
Addingpub mod xcm;aligns Moonbeam runtime weights with the new internal XCM benchmarks.runtime/moonriver/src/weights/mod.rs (1)
59-59: Publicly export new XCM weights module
Thexcmexport enables the new Moonriver-local XCM weight implementations.runtime/moonbeam/tests/integration_test.rs (2)
44-44: Use localmoonbeam_xcm_weightsre-export
Replaces the external benchmark crate import with the runtime’smoonbeam_xcm_weightsre-export.
52-52: ImportXcmWeightfrom local module
Brings in the newXcmWeighttype for test assertions.runtime/moonriver/tests/integration_test.rs (1)
44-44: LGTM! Clean migration to local XCM weight definitions.The import changes correctly replace the external
moonbeam-xcm-benchmarksdependency with the new localmoonriver_xcm_weightsmodule. The interface remains compatible as evidenced by the existing test usage at line 2479, ensuring a seamless transition to the locally maintained weight calculations.Also applies to: 51-51
runtime/moonbase/src/lib.rs (1)
146-146: LGTM! Clean public re-export for local XCM weights.This change correctly exposes the local XCM weight module under a clear, runtime-specific alias, facilitating the migration away from the external
moonbeam-xcm-benchmarkscrate.runtime/moonbase/src/weights/xcm/mod.rs (1)
24-24: LGTM! Proper re-export of local WeightInfo.This change correctly re-exports the
WeightInfofrom the localgenericmodule, completing the migration from external benchmarking dependencies to local weight calculation modules.runtime/moonriver/src/lib.rs (1)
147-147: Expose XCM weights publicly under a clear alias
This addspub use weights::xcm as moonriver_xcm_weights;. Verify that all references to XCM weights (e.g., inxcm_config.rsand integration tests) now use this alias, and ensure naming consistency with other chain runtimes.runtime/moonbeam/src/lib.rs (1)
140-142: Ensure the new XCM weights re-export is correct
The addedpub use weights::xcm as moonbeam_xcm_weights;re-exports the local XCM weight module. Confirm thatweights::xcmcontains all required types (e.g.,XcmWeight) and that other runtimes follow the same alias to maintain consistency.runtime/moonbeam/src/xcm_config.rs (1)
199-204: Validate the new XcmWeigher alias configuration
TheXcmWeigheris now bound to the localcrate::weights::xcm::XcmWeight<Runtime, RuntimeCall>. Ensure this change aligns with the removal of the external benchmarking crate and that all XCM executors referenceXcmWeighercorrectly.runtime/moonbase/tests/integration_test.rs (2)
41-45: Update imports to use the local weights module
The change correctly replaces references to the removed external benchmark crate by pulling inmoonbase_xcm_weightsalongside existing runtime types. Ensure all downstream uses (e.g., inxcm_configand integration tests) reference this new module.
60-60: ImportXcmWeightfrom the new local crate
Bringing inXcmWeightfrommoonbase_xcm_weightsaligns with the weight‐calculation calls later in this file. Confirm that the runtime’slib.rspublicly re-exports this module so the tests compile cleanly.runtime/moonriver/src/xcm_config.rs (1)
208-208: LGTM! Transitioning to local XCM weight implementation.The change successfully updates the
XcmWeighertype alias to use the local runtime-specific weight implementation instead of the externalmoonbeam-xcm-benchmarkscrate. This aligns with the PR objectives to consolidate benchmarking efforts and use locally defined weight calculations.runtime/moonbase/src/xcm_config.rs (1)
214-214: LGTM! Transitioning to local XCM weight implementation.The change successfully updates the
XcmWeighertype alias to use the local runtime-specific weight implementation instead of the externalmoonbeam-xcm-benchmarkscrate. This change is consistent with the moonriver runtime and aligns with the PR objectives.runtime/common/src/apis.rs (2)
818-818: LGTM! Adding required import for updated API.The
WeightLimitimport is needed for the updatedworst_case_for_trader()method signature in the benchmarking configuration.
1039-1041: LGTM! Updated to match upstream pallet-xcm-benchmarks API.The method signature change from
fee_asset()toworst_case_for_trader()with the new return typeResult<(Asset, WeightLimit), BenchmarkError>aligns with the upstream Polkadot SDK changes mentioned in the PR objectives. The functionality remains the same as it still returnsErr(BenchmarkError::Skip).runtime/moonriver/src/weights/xcm/fungible.rs (3)
25-27: Verify proof size constants are appropriate for Moonriver.The proof size constants are copied from statemint benchmarks. Please verify these values are appropriate for the Moonriver runtime, as different runtimes may have different storage layouts and proof requirements.
31-71: LGTM! Well-structured weight calculation implementation.The implementation correctly:
- Uses trait bounds to ensure required pallets are available
- Differentiates between ERC20 and foreign assets using appropriate pallet methods
- Delegates to pallet-specific weight functions for accurate calculations
- Disables unsupported operations (teleport) by returning
Weight::MAX- Provides reasonable fallback weights for reserve operations
The asset type detection and weight delegation approach is appropriate for the Moonbeam ecosystem.
50-50: Verify hardcoded weight values are appropriate.The hardcoded weight values of 200_000_000 should be verified to ensure they're appropriate for the Moonriver runtime operations. Consider documenting the rationale for these specific values or deriving them from benchmarks.
Also applies to: 61-61, 69-69
runtime/moonbeam/src/weights/xcm/fungible.rs (3)
25-27: Verify proof size constants are appropriate for Moonbeam.The proof size constants are copied from statemint benchmarks. Please verify these values are appropriate for the Moonbeam runtime, as different runtimes may have different storage layouts and proof requirements.
31-71: LGTM! Consistent implementation with Moonriver.The implementation follows the same well-structured approach as the Moonriver version:
- Proper trait bounds for required pallets
- Asset type differentiation using appropriate pallet methods
- Delegation to pallet-specific weight functions
- Appropriate handling of unsupported operations
The consistency between Moonbeam and Moonriver implementations is appropriate given their similar architectures.
50-50: Verify hardcoded weight values are appropriate.The hardcoded weight values of 200_000_000 match the Moonriver implementation. Please verify these values are appropriate for both runtimes or if runtime-specific values should be used based on their respective characteristics.
Also applies to: 61-61, 69-69
runtime/moonbeam/src/weights/xcm/mod.rs (2)
41-53: LGTM: Well-implemented asset filter weight calculation.The
WeighMultiAssetsFilterimplementation correctly handles different asset filter variants with proper bounds checking usingmin(MAX_ASSETS, *count)to prevent overflow.
139-148: Address the commented logic and improve documentation.The commented code and explanation suggest uncertainty about the correct weight calculation for
initiate_reserve_withdraw. This needs clarification.Please verify whether the current implementation correctly reflects the actual computational cost of
initiate_reserve_withdraw. The comment suggests it should scale with number of assets, but the current implementation uses a fixed weight.#!/bin/bash # Search for other initiate_reserve_withdraw implementations to understand expected behavior ast-grep --pattern $'fn initiate_reserve_withdraw($$$) { $$$ }'runtime/moonbeam/src/weights/xcm/generic.rs (1)
387-393: Verify placeholder weight values for unsupported operations.Multiple functions return uniform weights of
500_000_000_000picoseconds, suggesting these may be placeholder values rather than actual benchmarked results.Please verify that these weight values are appropriate for the actual cost of these operations:
#!/bin/bash # Check if these are actual benchmark results or placeholders rg "500_000_000_000" --type rust -A 2 -B 2Also applies to: 421-427, 431-437, 439-445, 447-455
WASM runtime size check:Compared to target branchMoonbase runtime: 2372 KB (-8 KB) ✅ Moonbeam runtime: 2356 KB (+8 KB) Moonriver runtime: 2356 KB (-4 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: 2356 KB (+304 KB compared to latest release) |
Coverage Report@@ Coverage Diff @@
## master tarekkma/remove-moonbeam-xcm-bench +/- ##
======================================================================
- Coverage 75.39% 74.28% -1.11%
+ Files 407 412 +5
+ Lines 101610 103214 +1604
======================================================================
+ Hits 76604 76671 +67
+ Misses 25006 26543 +1537
|
TarekkMA
requested review from
RomarQ,
librelois,
manuelmauro,
snowmead and
stiiifff
Co-authored-by: Rodrigo Quelhas <[email protected]>
RomarQ
added
D3-trivial
|
My previous remarks will be addressed in a follow-up PR. |
2 participants
Removing
moonbeam-xcm-benchmarksand only usingpallet-xcm-benchmarks.This PR rely on 3 cherry picks on the polkasdk fork to include paritytech/polkadot-sdk#7944:
Summary by CodeRabbit
New Features
Refactor
Chores