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

[XNFT PoC] xnft module - XCM asset transactor for module nft #2649

Merged
merged 10 commits into from
Nov 30, 2023

Conversation

mrshiposha
Copy link
Contributor

This PR adds a new xnft module.
This module serves as a PoC of the XCM NFT Asset Transactor on Karura. This code will be used as a base to implement a more general Asset Transactor for Substrate chains.

Foreign Collection Registration

  • To keep things simple in the PoC, a foreign NFT collection can be registered manually by a technical committee member invoking the xnft.registerAsset extrinsic. In the future, we need to come up with a mechanism that will allow an owner of an original collection on a remote chain to register the corresponding foreign collection themselves without involving a governance body.

Notes:

  • xnft holds the mapping between the XCM identification of an NFT asset and the local identification in the nft module.
    The mapping is bidirectional, though only one side is used now. The backward mapping can be used in the future for extrinsics like xtokens.transfer where we identify assets using parachain-local IDs.
  • xnft pallet account serves as the location for NFTs when they are located in the Holding Register to avoid burning an NFT and losing the associated data.
  • Also, the xnft pallet account will hold a derivative of an NFT when the original NFT is transferred elsewhere instead of burning the derivative. This serves two goals:
    • If the original NFT returns, it will correspond to the same derivative NFT with the same Karura-local ID.
    • Suppose the reserve chain erroneously sends the ReserveAssetDeposited containing an NFT that was already present on Karura. In that case, it will fail due to the failed transfer from the xnft module account to the beneficiary because the derivative NFT will belong to someone else's account, avoiding creating a duplicate.

Tests

The common tests are located in the xnft-tests repository.

@mrshiposha mrshiposha changed the title Xnft poc [XNFT PoC] xnft module - XCM asset transactor for module nft Nov 23, 2023
modules/xnft/src/lib.rs Show resolved Hide resolved
modules/nft/src/lib.rs Outdated Show resolved Hide resolved
modules/xnft/src/lib.rs Outdated Show resolved Hide resolved
modules/xnft/src/xcm_helpers.rs Outdated Show resolved Hide resolved
modules/xnft/src/xcm_helpers.rs Outdated Show resolved Hide resolved
xlc
xlc previously approved these changes Nov 28, 2023
Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

Mostly good except the token deposit. But should be fine for an MVP with very limited usage.

@mrshiposha
Copy link
Contributor Author

Strangely, it didn't build in the CI. I tested it before the last push. Will fix.

@xlc
Copy link
Member

xlc commented Nov 28, 2023

not 100% sure why the e2e-tests are failing. I will take a look

@xlc
Copy link
Member

xlc commented Nov 29, 2023

can you do a merge master to fix the e2e-tests issue

@mrshiposha
Copy link
Contributor Author

mrshiposha commented Nov 29, 2023

@xlc

Local NFT also shouldn't hold any extra data, at least those data should transfer back to origin chain so it should be safe to delete them then the NFT is transferred back.

We can't drop local NFTs' data if we use reserve-based transfers. Because Karura is the reserve location for these NFTs, Karura is the only source of truth of any aspect of these assets. We had a long thread about this on the Polkadot forum. Here is Keith's post.

Karura, as the reserve location, can't trust another chain with Karura's own NFTs' data.


Our team has been thinking about the NFT metadata issue, and we concluded that metadata operations and transfers are separate classes of operations (at least concerning reserve-based transfers). The result of the R&D is the presentation at the Sub0 conference where I explained this in detail with several examples.

At the conference, I mentioned only these new hypothetical XCM instructions:

  • ApproveMetadataModification
  • MetadataModificationApproved
  • ModifyMetadata

Yet we need one more: QueryMetadata and the corresponding response. So that smart contracts on a non-reserve chain could access a foreign NFT's data. Not all chains will support metadata modification and the approval mechanism.

@xlc xlc merged commit c7fde71 into AcalaNetwork:master Nov 30, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants