-
Notifications
You must be signed in to change notification settings - Fork 710
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
Granular NFT traits and new XCM NFT types #4300
base: master
Are you sure you want to change the base?
Granular NFT traits and new XCM NFT types #4300
Conversation
polkadot/xcm/xcm-builder/src/unique_instances/backed_derivative.rs
Outdated
Show resolved
Hide resolved
@franciscoaguirre @xlc This PR adds new granular NFT traits to frame-support and provides new XCM types, giving us the instruments to implement XCM NFT in any chain regardless of differences in NFT solutions used in the ecosystem (different pallets, which represent NFTs differently or incompatible with each other, or smart contract-based solutions). This PR could undoubtedly be divided into at least two. However, I decided to provide all the pieces at once so we can discuss the complete picture. I can divide the PR if needed. |
The CI pipeline was cancelled due to failure one of the required jobs. |
The CI pipeline was cancelled due to failure one of the required jobs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a shallow pass on the adapter, ops and strategies, I'll make more review passes
polkadot/xcm/pallet-xnft/src/lib.rs
Outdated
|
||
#[pallet::storage] | ||
#[pallet::getter(fn foreign_asset_to_derivative_id_params)] | ||
pub type ForeignAssetToDerivativeIdParams<T: Config<I>, I: 'static = ()> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to map AssetId
to a DerivativeId
. Is it possible to just use AssetId
? As we do with ForeignAssets
in asset hub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not strictly necessary. In fact, chains like AssetHub can use AssetId
everywhere.
However, I introduced this optional pallet that can map AssetId
to a different DerivativeId
to support the use case of mapping foreign NFTs to the already existing NFT engine on a given chain. For instance, we at Unique want a unified interface for working with all kinds of NFTs on the chain, both ours and foreign ones. E.g., we want to use the existing methods for nesting and fractionalization.
This way, our clients can think about a single interface without worrying about the kind of token they are working with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the pallet's name should be refined to reflect its optionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good showcase of what can be done, I don't think it belongs in this repository then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it belongs in this repository then
Providing support for derivatives of a general form seems like a good thing. Also, the pallet is minimal, so maintaining it will be easy.
/// | ||
/// The implemented [`Destroy`] operation can be used in the | ||
/// [`UniqueInstancesAdapter`](super::UniqueInstancesAdapter) via the [`UniqueInstancesOps`]. | ||
pub struct StashOnDestroy<InstanceOps>(PhantomData<InstanceOps>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of implementing destroy using a stash instead of burning the asset. However, should stash be its own operation? I also have a hard time wrapping my head around strategies, some seem to make sense, but some seem like they're more implementation details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the Stash
/ Restore
operations could be reasonably frequent. For instance, the fractionalization operation implies "stashing" the NFT and depositing associated fungibles (and symmetrically, burning fungibles and "restoring" the NFT).
The non-fungibles could be stashed during other, more high-level operations (like fractionalization). For instance, an NFT could be used as an entry ticket to a dedicated on-chain logic, and this ticket is considered "active" when the NFT is stashed. I feel that more use cases could be derived around stashing/restoring.
Nonetheless, I agree that these two strategies are more semantically complex than Transfer
or Destroy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that sense, isn't Stash
be some sort of lock? Like how with fungibles you lock and you gain a benefit, it could be the same with an nft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be. To me, the Stash
seems more general than a hypothetical Lock
. Maybe I'm wrong, but it looks like the Lock
should preserve ownership (the Unlock
should return the token to the before-lock-owner). Stash
isn't obliged to provide such guarantees; hence, it is more general.
} | ||
|
||
pub trait MatchesInstance<Id> { | ||
fn matches_instance(a: &Asset) -> result::Result<Id, Error>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting addition. Some NFTs do indeed come without a collection, like coretime regions. Do they use the same AssetId
and an index or do they use different AssetId
s and the Undefined
instance, which is supposed to be used for NFTs with only one instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on the chain that provides such NFTs :)
Yet, I'd like to see the following principle. Suppose we have collection-less NFTs on a chain. In that case, in XCM, they should be identified by the same AssetId
and varying AssetInstance
if these NFTs are different instances of the same "type" (where "type" is defined by the operations you can do with its objects).
So, applying this principle, the combination of AssetId
and AssetInstance::Undefined
should identify a singleton object with associated operations unique to this particular asset.
In short, I'd say this:
AssetId
should identify the NFT engine (and possibly some interior "group" of NFTs in that engine, like a collection)AssetInstance
should identify the particular NFT instance inside the specified NFT engine.
Note: I assume the chain can host multiple NFT engines that provide different NFT-associated operations.
Self(PhantomData) | ||
} | ||
} | ||
impl<Owner> MetadataInspectStrategy for Ownership<Owner> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean by "strategy" here, it's more of a view inside the metadata. You specify some values that are reasonable for the metadata to have, and a catch-all bytes
one for custom stuff. It's maybe more of a Field
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It indeed feels like a Field
or something. Maybe rename it to MetadataField
?
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/persistent-parachains-for-a-long-term-testnet-on-v1-14/9886/1 |
@franciscoaguirre, I opened a separate PR #5620 containing only the new NFT traits. Later, I will refactor the current PR #4300 description and code to make it solely XCM NFT-focused. |
Overview
This PR is meant to become a follow-up to #5620 that defined new NFT traits. Currently, this PR includes all the changes from #5620.
This PR provides new XCM types and tools for building NFT Asset Transactors.
The new types use general and granular NFT traits from #5620.
The new XCM adapters and utility types to work with NFTs can be considered the main deliverable of the XCM NFT proposal. The new types use a more general approach, making integrating into any chain with various NFT implementations easier.
For instance, different implementations could use:
With new types, these differences can be either abstracted away or worked around (see the example with pallet-nfts below).
Moreover, the new types provide greater flexibility for supporting derivative NFTs, allowing several possible approaches depending on the given chain's team's goals. See the section about derivatives below.
Also, this is the PR I mentioned in the #4073 issue, as it can be viewed as the solution. In particular, the new adapter (
UniqueInstancesAdapter
) requires theTransfer
operation with theFromTo
strategy. This brings the attention of both a developer and a reviewer to theFromTo
strategy (meaning that the transfer checks if the asset can be transferred from a given account to another account) both at trait bound and at the call site without sacrificing the flexibility of the NFT traits.New types for xcm-builder and xcm-executor
This PR introduces several new XCM types.
UniqueInstancesAdapter
is a newTransactAsset
adapter that supersedes bothNonFungibleAdapter
andNonFungiblesAdapter
(for reserve-based transfers only, as teleports can't be implemented appropriately without transferring the NFT metadata alongside it; no standard solution exists for that yet. Hopefully, the Felloweship RFC 125 (PR, text) will help with that).Thanks to the new Matcher types, the new adapter can be used instead of both
NonFungibleAdapter
andNonFungiblesAdapter
:MatchesInstance
(a trait)MatchInClassInstances
MatchClasslessInstances
Superseding the old adapters for pallet-uniques
Here is how the new
UniqueInstancesAdapter
in Westend Asset Hub replaces theNonFungiblesAdapter
:MatchInClassInstances
allows us to reuse the already existingUniquesConvertedConcreteId
matcher.The
pallet_uniques::asset_ops::Item<Uniques>
already implements the needed traits and can destroy and re-create NFT items without losing their data.So, migrating from the old adapter to the new one regarding runtime config changes is easy.
Declarative modification of an NFT engine
This PR doesn't include the implementation of the new traits for pallet-nfts.
However, you can find the implementation for it on this branch: https://github.com/UniqueNetwork/polkadot-sdk/tree/xcm-nft-dev-env.
The pallet-nfts is a good example of an NFT engine that can't destroy and then re-create an NFT without losing its data in general (as is ORML/Acala, Unique Network, Aventus, and possibly more).
Yet, the
UniqueInstancesAdapter
requires an NFT engine to be able to create and destroy NFTs.We will avoid implementing the new functionality for the NFT engine directly. Instead, we will illustrate how to model the "create" and "destroy" operations using other operations. Essentially, we will declaratively create a new NFT engine right in the runtime config.
Here is how this could look.
The main actor here is
UniqueInstancesOps
.It accepts implementations of the "create", "transfer", and "destroy" operations and merges them into one "NFT engine" that implements all three.
The
RestoreOnCreate
predictably uses theRestore
operation when asked to create an NFT.StashOnDestroy
works similarly by using theStash
operation.The
pallet_nfts::asset_ops::Item<Nfts>
doesn't implement theRestore
andStash
operations, however. So, we utilizeSimpleStash
, which takes a stash account and an NFT engine capable of transferring assets using theFromTo
strategy. It transfers an NFT to the stash account when stashing and from the stash account when restoring.To sum things up, this example illustrates how the existing NFT engine can be declaratively modified to fit into the XCM adapter if needed.
Supporting derivative NFTs (reserve-based model)
Derivative NFTs can be implemented differently depending on a given chain's team's goals.
For instance, if a team wants to separate the interaction with their chain's original NFTs and derivative NFTs, they could create a separate instance of an NFT engine that works solely with derivatives (if the NFT engine allows that).
For example,
pallet-uniques
is such an engine. A team could create another instance of this pallet, configuring it to useAssetId
as a collection ID andAssetInstance
as an item ID. In that case, the configuration of an NFT transactor is pretty much the same as in the example we've seen for pallet-uniques above, except for the matcher, which should reject NFT originals (because the originals are handled separately).Alternatively, a team might want to interact uniformly with originals and derivatives, ensuring that any clients working with the given chain's NFTs can work with the derivatives without any additional effort (or with a minimal one). Also, there could be complex NFT-related on-chain logic. By choosing the uniform interaction approach, the team can ensure that derivatives can participate in the same logic as the originals.
The uniform approach implies that derivatives will share the chain-local NFT IDs. So, to support this use scenario, this PR provides a minimal separate adapter called
UniqueInstancesDepositAdapter
to "register" derivatives (i.e., establish the mapping between the(AssetId, AssetInstance)
and chain-local NFT ID). This adapter requires the usualAccountId
andLocationToAccountId
generic parameters and only one additional parameter that implements theCreate
operation, which takes the beneficiary account and theNonFungibleAsset = (AssetId, AssetInstance)
as parameters to create a new derivative NFT.This is what its usage looks like:
When a derivative is registered, and the mapping between chain-local NFT ID and XCM NFT ID is saved, we need a matcher to transact known derivatives. Assuming the given chain provides an implementation for the
DerivativesRegistry
trait (which provides the info about the chain-local NFT ID <-> XCM NFT ID mapping), the team can use theMatchDerivativeInstances
, which will match only the derivatives found in the registry.Finally, this PR provides the pallet-derivatives that can facilitate the creation of such registries. This pallet implements the needed traits and can optionally provide extrinsics for registering derivatives (for instance, for registering derivative collections). Yet, this pallet is optional. The chain's team can use its own tools to store information about derivatives.