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

feat(spike): cross-chain nonfungible implementation #400

Open
wants to merge 21 commits into
base: chungquantin/feat-nfts
Choose a base branch
from

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Dec 5, 2024

Description

Blocked by: #387

Context: Why can't we use the default NonFungiblesTransactor provided by the xcm crate? Current asset transactor for nonfungible implementation is currently only for nonfungibles, not nonfungibles_v2. If we want it to support latest the version of the pallet-nfts crate (collection management functionalities), we need to reconstruct the NonFungiblesTransaction.

The pull request implements the NonFungiblesTransactor using the ForeignNfts pallet nfts instance. This makes changes to the pallet-nfts CollectionId type to remove the Copy required parameter trait as xcm::latest::Location is not implemented with Copy. Replace all the copies of CollectionId in the pallet-nfts code to use Clone. Performance benchmarking should be considered to see if there is a huge change in the memory efficiency.

  • MultiLocationCollectionId implemented with xcm::latest::Location
  • NonFungiblesTransactor
  • ForeignNfts instance and runtime configuration
  • XCM simulator integration tests

Outcome

Screenshot 2024-12-09 at 13 02 11

@chungquantin chungquantin self-assigned this Dec 6, 2024
@chungquantin chungquantin force-pushed the chungquantin/feat-nfts_cross_chain branch from 46d374a to 05e552e Compare December 6, 2024 09:03
@chungquantin chungquantin changed the title feat(xcm): cross-chain nonfungible implementation feat(spike): cross-chain nonfungible implementation Dec 9, 2024
@chungquantin chungquantin force-pushed the chungquantin/feat-nfts branch from b76a626 to c76f66b Compare December 9, 2024 08:47
@@ -148,7 +148,7 @@ pub mod pallet {
/// the `create_collection_with_id` function. However, if the `Incrementable` trait
/// implementation has an incremental order, the `create_collection_with_id` function
/// should not be used as it can claim a value in the ID sequence.
type CollectionId: Member + Parameter + MaxEncodedLen + Copy + Incrementable;
type CollectionId: Member + Parameter + MaxEncodedLen + Clone + Incrementable;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove Copy, add Clone. xcm::v4::Location type is not implemented with Copy.

@Daanvdplas
Copy link
Collaborator

The Incrementable trait is implemented in the non_fungibles_v2.rs file. Could we check whether removing this bound from the CollectionId type causes a lot of issues.

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 68.70540% with 307 lines in your changes missing coverage. Please review.

Project coverage is 68.75%. Comparing base (6ef9b32) to head (1aa59c3).

Files with missing lines Patch % Lines
runtime/common/src/xcm/nonfungibles_adapter.rs 0.00% 206 Missing ⚠️
pallets/nfts/src/impl_nonfungibles.rs 33.33% 20 Missing ⚠️
pallets/nfts/src/weights.rs 0.00% 16 Missing ⚠️
runtime/devnet/src/config/xcm.rs 0.00% 15 Missing ⚠️
runtime/devnet/src/config/assets.rs 0.00% 13 Missing ⚠️
pallets/nfts/src/lib.rs 66.66% 1 Missing and 6 partials ⚠️
pallets/nfts/src/features/create_delete_item.rs 88.67% 0 Missing and 6 partials ⚠️
pallets/nfts/src/features/attributes.rs 72.22% 0 Missing and 5 partials ⚠️
pallets/nfts/src/features/transfer.rs 70.58% 2 Missing and 3 partials ⚠️
pallets/nfts/src/features/metadata.rs 66.66% 0 Missing and 4 partials ⚠️
... and 6 more
@@                    Coverage Diff                     @@
##           chungquantin/feat-nfts     #400      +/-   ##
==========================================================
- Coverage                   70.44%   68.75%   -1.70%     
==========================================================
  Files                          70       72       +2     
  Lines                       13109    13232     +123     
  Branches                    13109    13232     +123     
==========================================================
- Hits                         9234     9097     -137     
- Misses                       3603     3856     +253     
- Partials                      272      279       +7     
Files with missing lines Coverage Δ
pallets/nfts/src/benchmarking.rs 85.79% <100.00%> (ø)
pallets/nfts/src/common_functions.rs 71.42% <100.00%> (ø)
pallets/nfts/src/features/roles.rs 96.25% <100.00%> (ø)
pallets/nfts/src/tests.rs 99.90% <100.00%> (-0.01%) ⬇️
runtime/common/src/lib.rs 0.00% <ø> (ø)
runtime/devnet/src/config/proxy.rs 0.00% <ø> (ø)
runtime/devnet/src/lib.rs 5.53% <ø> (ø)
pallets/nfts/src/features/atomic_swap.rs 90.71% <94.44%> (+0.20%) ⬆️
...lets/nfts/src/features/create_delete_collection.rs 85.39% <93.33%> (+0.50%) ⬆️
pallets/nfts/src/features/lock.rs 90.36% <85.71%> (+0.23%) ⬆️
... and 13 more

@chungquantin
Copy link
Collaborator Author

The Incrementable trait is implemented in the non_fungibles_v2.rs file. Could we check whether removing this bound from the CollectionId type causes a lot of issues.

Any reason why we need to remove it?

@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Dec 10, 2024

Because if the incrementable can be removed it would not be necessary to implement the MultiLocation(Location) type

@chungquantin chungquantin force-pushed the chungquantin/feat-nfts branch from 769f50a to fafb988 Compare December 12, 2024 02:39
@chungquantin
Copy link
Collaborator Author

[sc-1734]

@chungquantin chungquantin force-pushed the chungquantin/feat-nfts_cross_chain branch from eba2458 to 7332079 Compare December 13, 2024 03:38
@chungquantin chungquantin force-pushed the chungquantin/feat-nfts_cross_chain branch from 76a0d8e to e0d13f2 Compare December 13, 2024 04:07
@chungquantin
Copy link
Collaborator Author

Because if the incrementable can be removed it would not be necessary to implement the MultiLocation(Location) type

You can view the changes if we remove the incrementable trait here #406. Imo, the current approach in this PR is cleaner as it does not remove any thing related to functionality in the pallet-nfts, mainly changes in the CollectionId trait and data ownership.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants