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.

Sign up for GitHub

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

Jump to bottom

Polkadot-1.7.2: restricted transfer location changes #1853

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Polkadot-1.7.2: restricted transfer location changes #1853

merged 8 commits into from
Jun 6, 2024

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jun 3, 2024

Description

Modify how RestrictedTransferLocation stores XCM information and do the consequent migration. Based on this #1833 (comment)

@lemunozm lemunozm added Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. P5-soon Issue should be addressed soon. D8-migration Pull request touches storage and needs migration code. labels Jun 3, 2024
@lemunozm lemunozm requested review from wischli and mustermeiszer June 3, 2024 09:20
@lemunozm lemunozm self-assigned this Jun 3, 2024
@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 3, 2024

Still WIP, we need to figure out how to create the same v3::Locationused by apps: https://github.com/centrifuge/apps/blob/b59bdd34561a4ccd90e0d803c14a3729fc2f3a6d/centrifuge-app/src/utils/usePermissions.tsx#L386

@lemunozm lemunozm requested a review from hieronx as a code owner June 3, 2024 12:29
lemunozm
lemunozm commented Jun 3, 2024
View reviewed changes
Copy link
Contributor Author

@lemunozm lemunozm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It works migrating the expected Location with the correct hash.

2 points left:

  • Do we want to migrate also some possible accounts in demo? @wischli? If new Locations are added before RU they will not be migrated, but nothing will be broken
  • Add some pre/post checks

libs/types/src/locations.rs Outdated Show resolved Hide resolved
runtime/common/src/migrations/restricted_location.rs Outdated Show resolved Hide resolved
runtime/common/src/migrations/restricted_location.rs Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 82 lines in your changes missing coverage. Please review.

Please upload report for BASE (polkadot-v1.7.2@fb52bc4). Learn more about missing BASE report.

Files Patch % Lines
...ntime/common/src/migrations/restricted_location.rs 0.00% 73 Missing ⚠️
pallets/restricted-xtokens/src/lib.rs 0.00% 6 Missing ⚠️
runtime/common/src/transfer_filter.rs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@                Coverage Diff                 @@
##             polkadot-v1.7.2    #1853   +/-   ##
==================================================
  Coverage                   ?   46.61%           
==================================================
  Files                      ?      167           
  Lines                      ?    13065           
  Branches                   ?        0           
==================================================
  Hits                       ?     6090           
  Misses                     ?     6975           
  Partials                   ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wischli wischli mentioned this pull request Jun 4, 2024
Merged
4 tasks
@lemunozm lemunozm mentioned this pull request Jun 5, 2024
Merged
52 tasks
lemunozm
lemunozm commented Jun 5, 2024
View reviewed changes
libs/types/src/locations.rs Outdated
/// blake2 hashing on storage map keys, and we don't want the extra overhead
XCM(H256),
/// XCM Location sending destinations.
Xcm(Location),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@onnovisser from this, you do not need to create the hash, just put the location (will be probably a VersionedLocation)

@lemunozm lemunozm added the D5-breaks-api Pull request changes public api. Document changes publicly. label Jun 5, 2024
wischli and others added 3 commits June 5, 2024 16:24
@wischli
chore: extend v1.7.2 transfer location migration (#1858)
f457f22 
* feat: add pre + post-upgrade

* feat: add nuking migration for dev and demo

* fix: remove account list for demo
@wischli
refactor: Use VersionedLocation in RestrictedTransferLocation (#1861
3d97a95 
)

* feat: add pre + post-upgrade

* feat: add nuking migration for dev and demo

* fix: remove account list for demo

* refactor: use VersionedLocation

* fix: transfer allowlist benchmarks + tests

* fix: clippy

* fix: benchmark cli

* fix: revert tight coupling

* fmt
@lemunozm
fix IT compilation
26d42bd
@lemunozm lemunozm merged commit 43f7c48 into polkadot-v1.7.2 Jun 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wischli wischli Awaiting requested review from wischli wischli is a code owner

@mustermeiszer mustermeiszer Awaiting requested review from mustermeiszer mustermeiszer is a code owner

@hieronx hieronx Awaiting requested review from hieronx hieronx is a code owner

Assignees

@lemunozm lemunozm

Labels
D5-breaks-api Pull request changes public api. Document changes publicly. D8-migration Pull request touches storage and needs migration code. P5-soon Issue should be addressed soon. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lemunozm @wischli