Skip to content
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

Fix weight generation for pallet-asset-manager #3078

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

ahmadkaouk
Copy link
Contributor

@ahmadkaouk ahmadkaouk commented Dec 5, 2024

What does it do?

This PR fix an issue with weight generation for the pallet-asset-manager caused by changes introduced in #2908.

The functions change_existing_asset_type and remove_existing_asset_type in the WeightInfo trait are not supposed to accept arguments, as indicated by their signatures:

pub trait WeightInfo {
	fn register_foreign_asset() -> Weight;
	fn change_existing_asset_type() -> Weight;
	fn remove_existing_asset_type() -> Weight;
}

But since we're using this syntax le x in 5..100 in the benchmark scenarios, this generates the following code:

    /// Storage: `AssetManager::AssetIdType` (r:1 w:1)
    /// Proof: `AssetManager::AssetIdType` (`max_values`: None, `max_size`: None, mode: `Measured`)
    /// Storage: `AssetManager::AssetTypeId` (r:0 w:2)
    /// Proof: `AssetManager::AssetTypeId` (`max_values`: None, `max_size`: None, mode: `Measured`)
    /// The range of component `x` is `[5, 100]`.
    fn change_existing_asset_type(x: u32, ) -> Weight {
        // Proof Size summary in bytes:
        //  Measured:  `447 + x * (4 ±0)`
        //  Estimated: `3909 + x * (5 ±0)`
        // Minimum execution time: 16_455_000 picoseconds.
        Weight::from_parts(18_175_493, 3909)
            // Standard Error: 1_387
            .saturating_add(Weight::from_parts(77_450, 0).saturating_mul(x.into()))
            .saturating_add(T::DbWeight::get().reads(1_u64))
            .saturating_add(T::DbWeight::get().writes(3_u64))
            .saturating_add(Weight::from_parts(0, 5).saturating_mul(x.into()))
    }
    /// Storage: `AssetManager::AssetIdType` (r:1 w:1)
    /// Proof: `AssetManager::AssetIdType` (`max_values`: None, `max_size`: None, mode: `Measured`)
    /// Storage: `AssetManager::AssetTypeId` (r:0 w:1)
    /// Proof: `AssetManager::AssetTypeId` (`max_values`: None, `max_size`: None, mode: `Measured`)
    /// The range of component `x` is `[5, 100]`.
    fn remove_existing_asset_type(x: u32, ) -> Weight {
        // Proof Size summary in bytes:
        //  Measured:  `447 + x * (4 ±0)`
        //  Estimated: `3909 + x * (5 ±0)`
        // Minimum execution time: 14_125_000 picoseconds.
        Weight::from_parts(15_904_692, 3909)
            // Standard Error: 1_352
            .saturating_add(Weight::from_parts(70_484, 0).saturating_mul(x.into()))
            .saturating_add(T::DbWeight::get().reads(1_u64))
            .saturating_add(T::DbWeight::get().writes(2_u64))
            .saturating_add(Weight::from_parts(0, 5).saturating_mul(x.into()))
    }

Which does not match the signature in the trait.

@ahmadkaouk ahmadkaouk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes labels Dec 5, 2024
Copy link
Contributor

github-actions bot commented Dec 5, 2024

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2260 KB (no changes) ✅

Moonbeam runtime: 2248 KB (no changes) ✅

Moonriver runtime: 2240 KB (no changes) ✅

Compared to latest release (runtime-3300)

Moonbase runtime: 2260 KB (+232 KB compared to latest release) ⚠️

Moonbeam runtime: 2248 KB (+252 KB compared to latest release) ⚠️

Moonriver runtime: 2240 KB (+248 KB compared to latest release) ⚠️

Copy link
Contributor

github-actions bot commented Dec 5, 2024

Coverage Report

@@                        Coverage Diff                         @@
##           master   ahmad-fix-weights-asset-manager     +/-   ##
==================================================================
  Coverage   74.58%                            74.58%   0.00%     
  Files         375                               375             
  Lines       95682                             95682             
==================================================================
  Hits        71356                             71356             
  Misses      24326                             24326             
Files Changed Coverage

Coverage generated Thu Dec 5 11:01:22 UTC 2024

Copy link
Contributor

@RomarQ RomarQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmadkaouk can you explain the issue? The CI benchmarks check was passing.

@ahmadkaouk
Copy link
Contributor Author

@RomarQ I’ve added a description of the issue in the PR. The problem is that the compilation fails after the weights are generated. To address this, we should consider adding an extra step in the CI benchmark check to compile the runtime with the generated weights.

Copy link
Contributor

@gonzamontiel gonzamontiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this is the right approach to remove the loop, but it looks in sync with the definition and fix the benchmark.

pallets/asset-manager/src/benchmarks.rs Show resolved Hide resolved
Copy link
Contributor

@RomarQ RomarQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine since we are no longer using num_assets_weight_hint;

@ahmadkaouk ahmadkaouk merged commit 9344aea into master Dec 6, 2024
52 checks passed
@ahmadkaouk ahmadkaouk deleted the ahmad-fix-weights-asset-manager branch December 6, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants