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

Allow assets managed by AssetManager and EvmForeignAssets to use the pallet-xcm precompile #3136

Merged
merged 3 commits into from
Jan 19, 2025

Conversation

RomarQ
Copy link
Contributor

@RomarQ RomarQ commented Jan 17, 2025

What does it do?

Followup of #3107

This change allows assets managed by AssetManager and EvmForeignAssets to use the pallet-xcm precompile.

@RomarQ RomarQ self-assigned this Jan 17, 2025
Copy link
Contributor

github-actions bot commented Jan 17, 2025

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2284 KB (no changes) ✅

Moonbeam runtime: 2264 KB (no changes) ✅

Moonriver runtime: 2252 KB (no changes) ✅

Compared to latest release (runtime-3400)

Moonbase runtime: 2284 KB (+256 KB compared to latest release) ⚠️

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

Moonriver runtime: 2252 KB (+240 KB compared to latest release) ⚠️

@RomarQ RomarQ added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Jan 17, 2025
@RomarQ RomarQ changed the title update AssetIdToLocationManager to support both EvmForeignAssets and AssetManager as asset managers Allow assets managed by AssetManager and EvmForeignAssets to use the pallet-xcm precompile Jan 17, 2025
@RomarQ RomarQ added not-breaking Does not need to be mentioned in breaking changes evm-native-foreign-assets D3-trivial PR contains trivial changes in a runtime directory that do not require an audit labels Jan 17, 2025
Copy link
Contributor

github-actions bot commented Jan 17, 2025

Coverage Report

@@                          Coverage Diff                          @@
##           master   rq/update-AssetIdToLocationManager     +/-   ##
=====================================================================
  Coverage   74.47%                               74.47%   0.00%     
  Files         377                                  377             
  Lines       95720                                95720             
=====================================================================
  Hits        71283                                71283             
  Misses      24437                                24437             
Files Changed Coverage

Coverage generated Fri Jan 17 16:05:17 UTC 2025

@RomarQ RomarQ marked this pull request as ready for review January 17, 2025 14:16
manuelmauro
manuelmauro previously approved these changes Jan 17, 2025
Copy link
Contributor

@manuelmauro manuelmauro left a comment

Choose a reason for hiding this comment

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

LGTM! Double-check: using this tuple type we'll have the runtime trying to convert the AssetId into a Location using fist AsAssetType<...> and, if it fails, falling back on EvmForignAsset. Correct?

TarekkMA
TarekkMA previously approved these changes Jan 17, 2025
Copy link
Contributor

@TarekkMA TarekkMA left a comment

Choose a reason for hiding this comment

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

LGTM; should we add an integration test?

@RomarQ
Copy link
Contributor Author

RomarQ commented Jan 17, 2025

LGTM! Double-check: using this tuple type we'll have the runtime trying to convert the AssetId into a Location using fist AsAssetType<...> and, if it fails, falling back on EvmForignAsset. Correct?

Yes, exactly.

@RomarQ RomarQ dismissed stale reviews from TarekkMA and manuelmauro via fd2b083 January 17, 2025 15:30
@RomarQ
Copy link
Contributor Author

RomarQ commented Jan 17, 2025

LGTM; should we add an integration test?

Added a test with the old approach, the assets manager pallet is going to be removed in future upgrades.

@RomarQ RomarQ merged commit a322ae9 into master Jan 19, 2025
38 checks passed
@RomarQ RomarQ deleted the rq/update-AssetIdToLocationManager branch January 19, 2025 14:49
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 evm-native-foreign-assets 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