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

XCM coretime region transfers #3455

Merged
merged 46 commits into from
Apr 17, 2024
Merged

XCM coretime region transfers #3455

merged 46 commits into from
Apr 17, 2024

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Feb 23, 2024

This PR introduces changes enabling the transfer of coretime regions via XCM.

TL;DR: There are two primary issues that are resolved in this PR:

  1. The mint and burn functions were not implemented for coretime regions. These operations are essential for moving assets to and from the XCM holding register.
  2. The transfer of non-fungible assets through XCM was previously disallowed. This was due to incorrectly benchmarking non-fungible asset transfers via XCM, which led to assigning it a weight of Weight::Max, effectively preventing its execution.

mint_into and burn implementation

This PR addresses the issue with cross-chain transferring regions back to the Coretime chain. Remote reserve transfers are performed by withdrawing and depositing the asset to and from the holding registry. This requires the asset to support burning and minting functionality.

This PR adds burning and minting; however, they work a bit differently than usual so that the associated region record is not lost when burning. Instead of removing all the data, burning will set the owner of the region to None, and when minting it back, it will set it to an actual value. So, when cross-chain transferring, withdrawing into the registry will remove the region from its original owner, and when depositing it from the registry, it will set its owner to another account

This was originally implemented in this PR: #3455, however we decided to move all of it to this single PR (#3455 (comment))

Fixes made in this PR

  • Update the XcmReserveTransferFilter on coretime chain since it is meant as a reserve chain for coretime regions.
  • Update the XCM benchmark to use AssetTransactor instead of assuming pallet-balances for fungible transfers.
  • Update the XCM benchmark to properly measure weight consumption for nonfungible reserve asset transfers. ATM reserve transfers via the extrinsic do not work since the weight for it is set to Weight::max().

Closes: #865

@Szegoo Szegoo marked this pull request as ready for review February 23, 2024 16:27
@Szegoo Szegoo requested a review from a team as a code owner February 23, 2024 16:27
@bkchr bkchr requested a review from bkontur February 24, 2024 15:28
@Szegoo Szegoo marked this pull request as draft February 27, 2024 08:57
@Szegoo Szegoo marked this pull request as ready for review February 27, 2024 12:49
@Szegoo
Copy link
Contributor Author

Szegoo commented Feb 27, 2024

The benchmark for the coretime chain should be re-run since the weight is currently set to Weight::max().

@seadanda seadanda added T6-XCM This PR/Issue is related to XCM. T2-pallets This PR/Issue is related to a particular pallet. labels Apr 8, 2024
@github-actions github-actions bot requested a review from bkontur April 16, 2024 10:19
Copy link

Review required! Latest push from author must always be reviewed

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 16, 2024

Would be great if we could get this merged soon :)

Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

Thanks!

@seadanda seadanda added this pull request to the merge queue Apr 17, 2024
Merged via the queue into paritytech:master with commit e6f3106 Apr 17, 2024
133 of 137 checks passed
@Szegoo
Copy link
Contributor Author

Szegoo commented May 5, 2024

Seems like we forgot to update the weights in this PR. I don't have the permission to run the bench bot so could someone run it and submit the changes in separate PR?

@seadanda @bkontur

@bkontur
Copy link
Contributor

bkontur commented May 7, 2024

Seems like we forgot to update the weights in this PR. I don't have the permission to run the bench bot so could someone run it and submit the changes in separate PR?

@seadanda @bkontur

@Szegoo @seadanda waiting here: #4396 (comment)

github-merge-queue bot pushed a commit that referenced this pull request May 7, 2024
Addressing comment:
#3455 (comment)

---------

Co-authored-by: command-bot <>
paritytech-ci pushed a commit that referenced this pull request May 8, 2024
Addressing comment:
#3455 (comment)

---------

Co-authored-by: command-bot <>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
Addressing comment:
paritytech#3455 (comment)

---------

Co-authored-by: command-bot <>
@seadanda seadanda self-assigned this Jul 24, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 26, 2024
The problem with the current implementation is that minting will cause
the region coremask to be set to `Coremask::complete` regardless of the
actual coremask.

This PR fixes that.

More details about the nonfungible implementation can be found here:
#3455

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Addressing comment:
paritytech#3455 (comment)

---------

Co-authored-by: command-bot <>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
The problem with the current implementation is that minting will cause
the region coremask to be set to `Coremask::complete` regardless of the
actual coremask.

This PR fixes that.

More details about the nonfungible implementation can be found here:
paritytech#3455

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet. T6-XCM This PR/Issue is related to XCM.
Projects
Status: Audited
Development

Successfully merging this pull request may close these issues.

XCM v3: Integrate NonFungibleAsset traits
5 participants