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

remove local-assets logic from assets-manager #2670

Merged
merged 7 commits into from
Apr 1, 2024

Conversation

RomarQ
Copy link
Contributor

@RomarQ RomarQ commented Feb 22, 2024

What does it do?

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@RomarQ RomarQ self-assigned this Feb 22, 2024
@RomarQ RomarQ 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. labels Feb 22, 2024
@RomarQ RomarQ added the breaking Needs to be mentioned in breaking changes label Feb 22, 2024
Copy link
Contributor

github-actions bot commented Feb 22, 2024

Coverage Report

@@                              Coverage Diff                              @@
##           master   rq/remove-local-assets-from-asset-manager      +/-   ##
=============================================================================
- Coverage   72.51%                                      72.50%   -0.01%     
  Files         229                                         229              
- Lines       70565                                       70401     -164     
=============================================================================
- Hits        51164                                       51039     -125     
- Misses      19401                                       19362      -39     
Files Changed Coverage
/pallets/asset-manager/src/lib.rs 73.12% (-3.14%) 🔽
/pallets/asset-manager/src/tests.rs 99.76% (-0.03%) 🔽

Coverage generated Wed Mar 27 11:13:24 UTC 2024

@RomarQ RomarQ marked this pull request as ready for review February 28, 2024 10:22
@@ -493,8 +493,6 @@ pub enum CurrencyId {
SelfReserve,
// Assets representing other chains native tokens
ForeignAsset(AssetId),
// Our local assets
DeprecatedLocalAssetReserve(AssetId),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the dummy variant to avoid breaking changes for CurrencyId scale encoding

@@ -485,8 +485,6 @@ pub enum CurrencyId {
SelfReserve,
// Assets representing other chains native tokens
ForeignAsset(AssetId),
// Our local assets
DeprecatedLocalAssetReserve(AssetId),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep a dummy variant here, youc an remove the (AssetId) parameter and use an empty varient named RemovedLocalAssetVariant

Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

Please keep a dummy variant for CurrencyId enum, otherwise you will break the scale encoding for the CurrencyId type, then it will break dapps beause dapps need to encode this type client side (at least the moonbeam dapp)

@RomarQ RomarQ merged commit bb42fd2 into master Apr 1, 2024
27 checks passed
@RomarQ RomarQ deleted the rq/remove-local-assets-from-asset-manager branch April 1, 2024 21:05
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 breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants