New NFT traits: granular and abstract interface #5620
Conversation
|
Thank you for tackling the issue of generic, granular traits for non-fungible tokens! I have a few proposals I think would improve these traits:
We use this pattern quite a lot of first validating that an action can be done and then doing it. trait Transfer {
type Ticket;
fn can_transfer(id: _) -> Self::Ticket;
fn transfer(id: _, ticket: Self::Ticket);
}You can see this pattern in the This
trait Inspect {
type Key;
type Value;
fn owner(id: _) -> Option<Self::AccountId>;
// ...other common attributes we expect every NFT engine could have...
fn attribute(id: _, key: Self::Key) -> Result<Value>;
}This could allow an NFT engine to define a schema of attributes it supports by setting What do you think? |
|
@franciscoaguirre Thanks for the feedback! About the
|
|
@franciscoaguirre, a follow-up to my previous comment. There is room for simplification. Although I believe multiple strategies are a good thing (as per the reasons provided in the previous comment), it seems there is no need for a notion of "asset kinds." The For example, if an XCM adapter or a pallet's config requires In this design, the implementor of the traits "defines" an asset kind. For example: // Assume we have an NFT pallet `Pallet<T: Config, I: 'static>`
pub struct Collection<PalletInstance>(PhantomData<PalletInstance>);
impl<T: Config<I>, I: 'static> AssetDefinition for Collection<Pallet<T, I>> {
type Id = /* the collection ID type */;
}
// Collection "from-to" transfer
// Note that there is NO `AssetKind` parameter
impl<T: Config<I>, I: 'static> Transfer<FromTo<AccountId>> for Collection<Pallet<T, I>> {
fn transfer(collection_id: &Self::Id, strategy: FromTo<AccountId>) -> DispatchResult {
let FromTo(from, to) = strategy;
todo!("check if `from` is the current owner using Pallet<T, I>");
// Reuse `Transfer<JustDo<AccountId>>` impl (it is assumed in this example)
Self::transfer(collection_id, JustDo(to))
}
}
pub struct Nft<PalletInstance>(PhantomData<PalletInstance>);
impl<T: Config<I>, I: 'static> AssetDefinition for Nft<Pallet<T, I>> {
type Id = /* the *full* NFT ID type */;
}
// The NFT "from-to" transfer
// Note that there is NO `AssetKind` parameter
impl<T: Config<I>, I: 'static> Transfer<FromTo<AccountId>> for Nft<Pallet<T, I>> {
fn transfer(nft_id: &Self::Id, strategy: FromTo<AccountId>) -> DispatchResult {
let FromTo(from, to) = strategy;
todo!("check if `from` is the current owner using Pallet<T, I>");
// Reuse `Transfer<JustDo<AccountId>>` impl (it is assumed in this example)
Self::transfer(nft_id, JustDo(to))
}
} |
|
@franciscoaguirre I simplified the traits. Only one generic parameter is used by operations. |
|
The asset-ops tests added into pallet-uniques |
| /// | ||
| /// The common transfer strategies are: | ||
| /// * [`JustDo`](common_strategies::JustDo) | ||
| /// * [`FromTo`](common_strategies::FromTo) |
There was a problem hiding this comment.
Maybe hard to make a reusable common strategy from this, but it might be useful to have one that checks metadata first. Something like MetadataCheck that will first inspect the metadata for some value like can_transfer: bool. That way we have a common strategy for doing these sort of checks.
There was a problem hiding this comment.
Refactored the strategies that fall into the "check-then-do" pattern. There are now CheckState and CheckOrigin strategies (the last one was renamed for consistency). Also, the JustDo was renamed to Unchecked and stripped from its parameters because their meaning wasn't obvious from the strategy itself.
The CheckState is defined as follows:
/// It is meant to be used when the asset state check should be performed
/// in addition to the `Inner` strategy.
/// The inspected state must be equal to the provided value.
pub struct CheckState<Inspect: InspectStrategy, Inner = Unchecked>(
pub Inspect::Value,
pub Inner
);The FromTo is removed in favor of IfOwnedBy (which got generalized) and To.
Here is the current IfOwnedBy definition:
/// The operation implementation must check
/// if the given account owns the asset and acts according to the inner strategy.
pub type IfOwnedBy<Owner, Inner = Unchecked> = CheckState<Ownership<Owner>, Inner>;So, the "from-to" transfers can be used like this (see the examples in tests as well):
Item::transfer(&(collection_id, item_id), IfOwnedBy::new(alice, To(bob)));The "just to" transfers look like this:
Item::transfer(&(collection_id, item_id), To(bob));The "stash" operation can be done like this (note the same IfOwnedBy strategy):
Item::stash(&(collection_id, item_id), IfOwnedBy::expect(collection_owner))The similar thing is for CheckOrigin:
Item::stash(&(collection_id, item_id), CheckOrigin::expect(RuntimeOrigin::root()))Item::transfer(&(collection_id, item_id), CheckOrigin::new(RuntimeOrigin::root(), To(bob)))Here is the Unchecked example:
Item::stash(&(collection_id, item_id), Unchecked))IfOwnedByWithWitness is also removed because it can now be expressed as IfOwnedBy<Account, WithWitness<W>>
There was a problem hiding this comment.
Awesome! Those examples look much better
substrate/frame/support/src/traits/tokens/asset_ops/common_strategies.rs
Outdated
Show resolved
Hide resolved
| /// implements. | ||
| pub struct CheckState<Inspect: InspectStrategy, Inner = Unchecked>(pub Inspect::Value, pub Inner); | ||
| impl<Inspect: InspectStrategy> CheckState<Inspect, Unchecked> { | ||
| pub fn expect(expected: Inspect::Value) -> Self { |
There was a problem hiding this comment.
I don't really understand this name much. What about new_unchecked?
There was a problem hiding this comment.
I don't know :)
I find it confusing to write CheckState::new_unchecked (do we check or not? :D)
I imagined that the expect name suggests that we do check the state (and "expect" it to be equal to the supplied value) and then "just do" the desired operation.
I admit, however, that it also can be confusing.
For now, no alternative comes to mind
There was a problem hiding this comment.
Could be new and new_with_inner but usually you want the shorter name to be the one you use most of the time, and I guess that's the one that has the inner variant
There was a problem hiding this comment.
How about new and with_inner?
The new will be about "just checking the state before the action," while with_inner will be about "checking the state and then performing the action with the inner strategy."
…ategies.rs Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
This PR introduces a new set of traits that represent different asset operations in a granular and abstract way.
The new abstractions provide an interface for collections and tokens for use in general and XCM contexts.
To make the review easier and the point clearer, this PR's code was extracted from #4300 (which contains the new XCM adapters). The #4300 is now meant to become a follow-up to this one.
Note: Thanks to @franciscoaguirre for a very productive discussion in Matrix. His questions are used in the Q&A notes.
Motivation: issues of the existing traits v1 and v2
This PR is meant to solve several issues and limitations of the existing frame-support nonfungible traits (both v1 and v2).
Derivative NFTs limitations
The existing v1 and v2 nonfungible traits (both collection-less—"nonfungible.rs", singular; and in-collection—"nonfungibles.rs", plural) can create a new token only if its ID is already known.
Combined with the corresponding XCM adapters implementation for v1 collection-less, in-collection (and the unfinished one for v2), this means that, in general, the only supported derivative NFTs are those whose chain-local IDs can be derived by the
Matcherand the NFT engine can mint the token with the provided ID. It is presumed the chain-local ID is derived without the use of storage (i.e., statelessly) because all the standard matcher's implementations aren't meant to look into the storage.To implement an alternative approach where chain-local derivative IDs are derived statefully, workarounds are needed. In this case, a custom stateful Matcher is required, or the NFT engine must be modified if it doesn't support predefined IDs for new tokens.
It is a valid use case if a chain has exactly one NFT engine, and its team wants to provide NFT derivatives in a way consistent with the rest of the NFTs on this chain.
Usually, if a chain already supports NFTs (Unique Network, Acala, Aventus, Moonbeam, etc.), they use their own chain-local NFT IDs.
Of course, it is possible to introduce a separate NFT engine just for derivatives and use XCM IDs as chain-local IDs there.
However, if the chain has a related logic to the NFT engine (e.g., fractionalizing), introducing a separate NFT engine for derivatives would require changing the related logic or limiting it to originals.
Also, in this case, the clients would need to treat originals and derivatives differently, increasing their maintenance burden.
The more related logic for a given NFT engine exists on-chain, the more changes will be required to support another instance of the NFT engine for derivatives.
Q&A: AssetHub uses the two pallets approach local and foreign assets. Why is this not an issue there?
Q&A: New traits open an opportunity for keeping derivatives on the same pallet. Thus, things like NFT fractionalization are reused without effort. Does it make sense to fractionalize a derivative?
Another thing about chain-local NFT IDs is that an NFT engine could provide some guarantees about its NFT IDs, such as that they are always sequential or convey some information. The chain's team might want to do the same for derivatives. In this case, it might be impossible to derive the derivative ID from the XCM ID statelessly (so the workarounds would be needed).
The existing adapters and traits don't directly support all of these cases. Workarounds could exist, but using them will increase the integration cost, the review process, and maintenance efforts.
The Polkadot SDK tries to provide general interfaces and tools, so it would be good to provide NFT interfaces/tools that are consistent and easily cover more use cases.
Design issues
Lack of generality
The existing traits (v1 and v2) are too concrete, leading to code duplication and inconvenience.
For example, two distinct sets of traits exist for collection-less and in-collection NFTs. The two sets are nearly the same. However, having two sets of traits necessitates providing two different XCM adapters. For instance, this PR introduced the
NonFungibleAdapter(collection-less). The description states that theNonFungibleAdapter"will be useful for enabling cross-chain Coretime region transfers, as the existingNonFungiblesAdapter1 is unsuitable for this purpose", which is true. It is unsuitable (without workarounds, at least).The same will happen with any on-chain entity that wants to use NFTs via these interfaces. Hence, the very structure of the interfaces makes using NFTs as first-class citizens harder (due to code duplication). This is sad since NFTs could be utility objects similar to CoreTime regions. For instance, they could be various capability tokens, on-chain shared variables, in-game characters and objects, and all of that could interoperate.
Another example of this issue is the methods of collections, which are very similar to the corresponding methods of tokens:
create_collection/mint_into,collection_attribute/attribute, and so on. In many ways, a collection could be considered a variant of a non-fungible token, so it shouldn't be surprising that the methods are analogous. Therefore, there could be a universal interface for these things.Q&A: there's a lot of duplication between nonfungible and nonfungibles. The SDK has the same with fungible and fungibles. Is this also a problem with fungible tokens?
Q&A: If it is not an issue for fungibles, why would this be an issue for NFTs?
An opinionated interface
Both v1 and v2 trait sets are opinionated.
The v1 set is less opinionated than v2, yet it also has some issues. For instance, why does the
burnmethod provide a way to check if the operation is permitted, buttransferandset_attributedo not? In thetransfercase, there is already an induced mistake in the XCM adapter. Even if we add an ownership check to all the methods, why should it be only the ownership check? There could be different permission checks. Even in this trait set, we can see that, for example, thedestroymethod for a collection takes a witness parameter additional to the ownership check.The same goes for v2 and even more.
For instance, the v2
mint_into, among other things, takesdeposit_collection_owner, which is an implementation detail of pallet-nfts that shouldn't be part of a general interface.It also introduces four different attribute kinds: metadata, regular attributes, custom attributes, and system attributes.
The motivation of why these particular attribute kinds are selected to be included in the general interface is unclear.
Moreover, it is unclear why not all attribute kinds are mutable (not all have the corresponding methods in the
Mutatetrait). And even those that can be modified (attributeandmetadata) have inconsistent interfaces:set_attributesets the attribute without any permission checks.set_metadatasets the metadata using thewho: AccountIdparameter for a permission check.set_metadatais a collection-less variant ofset_item_metadata, whileset_attributehas the same name in both trait sets.set_metadata, other methods (even theset_item_metadata!) that do the permission check useOption<AccountId>instead ofAccountId.clear_*methods.This is all very confusing. I believe this confusion has already led to many inconsistencies in implementation and may one day lead to bugs.
For example, if you look at the implementation of v2 traits in pallet-nfts, you can see that
attributereturns an attribute fromCollectionOwnernamespace or metadata, butset_attributesets an attribute inPalletnamespace (i.e., it sets a system attribute!).Future-proofing
Similar to how the pallet-nfts introduced new kinds of attributes, other NFT engines could also introduce different kinds of NFT operations. Or have sophisticated permission checks. Instead of bloating the general interface with the concrete use cases, I believe it would be great to make it granular and flexible, which this PR aspires to achieve. This way, we can preserve the consistency of the interface, make its implementation for an NFT engine more straightforward (since the NFT engine will implement only what it needs), and the pallets like pallet-nft-fractionalization that use NFT engines would work with more NFT engines, increasing the interoperability between NFT engines and other on-chain mechanisms.
New frame-support traits
The new
asset_opsmodule is added toframe_support::traits::tokens.It defines several "asset operations".
We avoid duplicating the interfaces with the same idea by providing the possibility to implement them on different structures representing different asset kinds. For example, similar operations can be performed on Collections and NFTs, such as creating Collections/NFTs, transferring their ownership, managing their metadata, etc.
The following "operations" are defined:
Q&A: What do InspectMetadata and UpdateMetadata operations mean?
Q&A: What do Stash/Restore operations mean?
Each operation can be implemented multiple times using different strategies associated with this operation.
This PR provides the implementation of the new traits for pallet-uniques.
Usage example: pallet-nft-fractionalization
In this in-fork draft PR, you can check out how these new traits are used in the pallet-nft-fractionalization.
A generic example: operations and strategies
Let's illustrate how we can implement the new traits for an NFT engine.
Imagine we have an NftEngine pallet (or a Smart Contract accessible from Rust; it doesn't matter), and we need to expose the following to other on-chain mechanisms:
Here is how this will look:
For further examples, see how pallet-uniques implements these operations for collections and items.
Footnotes
Don't confuse
NonFungibleAdapter(collection-less) andNonFungiblesAdapter(in-collection; see "s" in the name). ↩