Add migration for ForeignAssetCreator by ParthDesai · Pull Request #581 · moondance-labs/tanssi

ParthDesai · 2024-06-06T09:38:58Z

No description provided.

girazoki · 2024-06-06T09:44:08Z

runtime/common/src/migrations.rs

+        // Write to the new storage with removed and added fields
+        for (asset_id, old_location) in asset_id_to_foreign_asset_data {
+            if let Ok(new_location) = Runtime::ForeignAsset::try_from(old_location) {
+                AssetIdToForeignAsset::<Runtime>::insert(asset_id, new_location);


YOu can probably do both migrations here and not iterate twice

Hmm. But, that would mean I am assuming the semantics of these two storage items in runtime.

girazoki · 2024-06-06T09:45:49Z

runtime/common/src/migrations.rs

+        Runtime::DbWeight::get()
+            .reads(migrated_count as u64)
+            .saturating_add(Runtime::DbWeight::get().writes(migrated_count as u64 + 3u64))
+            .saturating_add(Runtime::DbWeight::get().writes(foreign_asset_to_asset_id_count as u64))


Dont you need to multiply by 2 here? (as you are writing twice per foreign_asset_to_asset_id_count

The other write is calculated above in migrated_count

tmpolaczyk · 2024-06-06T09:54:00Z

runtime/common/src/migrations.rs

+        // Removing older entries with old location as key
+        let mut removal_result = storage::unhashed::clear_prefix(
+            &ForeignAssetToAssetId::<Runtime>::final_prefix(),
+            None,
+            None,
+        );
+        while removal_result.maybe_cursor.is_some() {
+            removal_result = storage::unhashed::clear_prefix(
+                &ForeignAssetToAssetId::<Runtime>::final_prefix(),
+                None,
+                removal_result.maybe_cursor.as_deref(),
+            );
+        }


This may not be needed because storage_key_iter().drain() above already removes from storage

Oh. I did not know that drain also removes the item from storage.

Me neither, I had to look at docs: https://paritytech.github.io/polkadot-sdk/master/frame_support/storage/struct.PrefixIterator.html#method.drain

Removed this call in next commit.

tmpolaczyk

Looks good so far!

github-actions · 2024-06-06T10:50:55Z

Coverage Report

(master)

@@                                  Coverage Diff                                  @@
##           tanssi-update-1.11.0   add-foreign-asset-creator-migration      +/-   ##
=====================================================================================
+ Coverage                 69.32%                                69.53%   +0.21%     
  Files                       224                                   224              
+ Lines                     39347                                 39741     +394     
=====================================================================================
+ Hits                      27274                                 27632     +358     
+ Misses                    12073                                 12109      +36

Files Changed	Coverage
/client/manual-xcm/src/lib.rs	40.43% (+0.85%)	🔼
/client/node-common/src/service.rs	48.16% (+0.63%)	🔼
/client/services-payment/src/lib.rs	74.19% (+4.49%)	🔼
/container-chains/nodes/frontier/src/command.rs	35.04% (-0.10%)	🔽
/container-chains/nodes/frontier/src/service.rs	79.50% (+1.10%)	🔼
/container-chains/nodes/simple/src/command.rs	30.96% (-0.08%)	🔽
/container-chains/nodes/simple/src/service.rs	86.43% (+2.08%)	🔼
/container-chains/runtime-templates/frontier/src/lib.rs	60.82% (+0.60%)	🔼
/container-chains/runtime-templates/frontier/src/migrations.rs	89.29% (-10.71%)	🔽
/container-chains/runtime-templates/frontier/src/weights/xcm/mod.rs	16.39% (+1.31%)	🔼
/container-chains/runtime-templates/simple/src/lib.rs	73.43% (+0.52%)	🔼
/container-chains/runtime-templates/simple/src/migrations.rs	89.29% (-10.71%)	🔽
/container-chains/runtime-templates/simple/src/weights/xcm/mod.rs	16.39% (+1.31%)	🔼
/node/src/command.rs	27.96% (-0.05%)	🔽
/node/src/service.rs	21.63% (-0.02%)	🔽
/pallets/author-noting/src/lib.rs	87.96% (+1.39%)	🔼
/pallets/registrar/src/weights.rs	25.00% (+0.83%)	🔼
/pallets/services-payment/src/lib.rs	94.27% (+1.59%)	🔼
/primitives/author-noting-inherent/src/client_side.rs	87.50% (+8.01%)	🔼
/runtime/common/src/migrations.rs	87.73% (+41.61%)	🔼
/runtime/dancebox/src/lib.rs	89.17% (-0.15%)	🔽
/runtime/dancebox/src/weights/pallet_balances.rs	21.95% (-2.05%)	🔽
/runtime/dancebox/src/weights/pallet_xcm.rs	10.47% (-0.64%)	🔽
/runtime/dancebox/src/weights/xcm/mod.rs	43.17% (-1.55%)	🔽
/runtime/dancebox/src/xcm_config.rs	84.54% (+0.87%)	🔼
/runtime/dancebox/tests/integration_test.rs	99.70% (+0.01%)	🔼
/runtime/flashbox/src/lib.rs	45.52% (-0.21%)	🔽

Coverage generated Thu Jun 6 17:34:22 UTC 2024

tmpolaczyk · 2024-06-06T16:10:35Z

runtime/common/src/migrations.rs

 //! This module acts as a registry where each migration is defined. Each migration should implement
 //! the "Migration" trait declared in the pallet-migrations crate.

+use frame_support::__private::log;


Suggested change

use frame_support::__private::log;

tmpolaczyk · 2024-06-06T16:10:58Z

runtime/common/src/migrations.rs

+        }
+
+        // One db read and one db write per element, plus the on-chain storage
+        Runtime::DbWeight::get().reads_writes(migrated_count as u64, 2 * migrated_count as u64)


I see, 1 write when using drain, and another write when using insert...

* first branches * removfe try runtime and start updating * start working on fixes * advances * fixes before rt * dancebox compiles * flashbox compiles * fix a million things * node changes * update lock * fixes2 * fix xcm-primtiives * remove from dancebox * add primitive-types to cargo.toml * fix cargo.lock * add primitive-types/std to dancebox * Add PoV size host function to runtimes * Fix zombienet config * let's see if now tomls are fixed * fix currencyAdapter * Update mocks * Rest of mocks and unit tests fixes * fix dev tests execution * remove build output * FMT * chopsticks * fix custom policy test * fix scripts typescript api gen * base fee * update packageS * fmt * use ResolveTo, although we lose the event * resolveto * fmt * Update polkadot-sdk commit, solving dup import * fix different message when invalid params returned from rpc * Fix XCMv4 types in tests * XCM * FMT * Update lock * Config get * fmt * VersionedXcmV4 * different fixes * fmt * brings back event for on demand * left overs from merge * Fix xcm-core-buyer tests * start putting migrations in * lock file update Signed-off-by: girazoki <gorka.irazoki@gmail.com> * fix zombienet upgrade jobs * Add migration for ForeignAssetCreator (#581) * Add migration for ForeignAssetCreator * use accurate weight --------- Signed-off-by: girazoki <gorka.irazoki@gmail.com> Co-authored-by: Agusrodri <agusrodriguez2456@gmail.com> Co-authored-by: Tomasz Polaczyk <tmpolaczyk@gmail.com> Co-authored-by: Francisco Gamundi <francisco@moonsonglabs.com> Co-authored-by: Parth <desaiparth08@gmail.com>

girazoki reviewed Jun 6, 2024

View reviewed changes

Add migration for ForeignAssetCreator

076476e

ParthDesai force-pushed the add-foreign-asset-creator-migration branch from 9f08bab to 076476e Compare June 6, 2024 09:46

tmpolaczyk reviewed Jun 6, 2024

View reviewed changes

remove clear_prefix call as it is not required

cb15a21

ParthDesai force-pushed the add-foreign-asset-creator-migration branch from 8ef4430 to cb15a21 Compare June 6, 2024 10:00

tmpolaczyk reviewed Jun 6, 2024

View reviewed changes

girazoki added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes labels Jun 6, 2024

ParthDesai added 2 commits June 6, 2024 20:04

use accurate weight

e476249

add test for migration

8d094a0

tmpolaczyk reviewed Jun 6, 2024

View reviewed changes

tmpolaczyk approved these changes Jun 6, 2024

View reviewed changes

remove un-necessary import

454d71f

girazoki approved these changes Jun 6, 2024

View reviewed changes

fix clippy

5952aaf

ParthDesai merged commit 88af7e5 into tanssi-update-1.11.0 Jun 6, 2024

ParthDesai deleted the add-foreign-asset-creator-migration branch June 6, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migration for ForeignAssetCreator#581

Add migration for ForeignAssetCreator#581
ParthDesai merged 6 commits intotanssi-update-1.11.0from
add-foreign-asset-creator-migration

ParthDesai commented Jun 6, 2024

Uh oh!

girazoki Jun 6, 2024

Uh oh!

ParthDesai Jun 6, 2024

Uh oh!

girazoki Jun 6, 2024

Uh oh!

ParthDesai Jun 6, 2024

Uh oh!

tmpolaczyk Jun 6, 2024

Uh oh!

ParthDesai Jun 6, 2024

Uh oh!

tmpolaczyk Jun 6, 2024

Uh oh!

ParthDesai Jun 6, 2024

Uh oh!

tmpolaczyk left a comment

Uh oh!

github-actions bot commented Jun 6, 2024 •

edited

Loading

Uh oh!

tmpolaczyk Jun 6, 2024

Uh oh!

tmpolaczyk Jun 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ParthDesai commented Jun 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmpolaczyk left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jun 6, 2024 •

edited

Loading