Add NonFungibleAdapter#2924
Merged
franciscoaguirre merged 9 commits intoparitytech:masterfrom Jan 26, 2024
Merged
Conversation
franciscoaguirre
approved these changes
Jan 17, 2024
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Contributor
Author
|
@joepetrowski Could you take a look at this? |
Contributor
|
Yeah I can review, but if it's for Coretime then I think @seadanda knows this better. |
seadanda
reviewed
Jan 22, 2024
Contributor
seadanda
left a comment
seadanda
left a comment
There was a problem hiding this comment.
Looks good, just needs an update to XCM v4 syntax.
Comment on lines
+89
to
+96
| fn accrue_checked(checking_account: AccountId, instance: Asset::ItemId) { | ||
| let ok = Asset::mint_into(&instance, &checking_account).is_ok(); | ||
| debug_assert!(ok, "`mint_into` cannot generally fail; qed"); | ||
| } | ||
| fn reduce_checked(instance: Asset::ItemId) { | ||
| let ok = Asset::burn(&instance, None).is_ok(); | ||
| debug_assert!(ok, "`can_check_in` must have returned `true` immediately prior; qed"); | ||
| } |
Contributor
There was a problem hiding this comment.
Perhaps these should be proper log messages and not debug_asserts. You are assuming that the user has e.g. called can_check_in prior to calling this function, but there is no guarantee. If the trait doesn't allow you to return an error here, should probably log it.
Contributor
Author
There was a problem hiding this comment.
I used debug_assert here to keep consistent with other adapter implementations.
Co-authored-by: Dónal Murray <donalm@seadanda.dev>
|
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. |
seadanda
approved these changes
Jan 24, 2024
Contributor
|
@Szegoo seems like after fixing a conflict this can be merged |
Contributor
Author
|
@franciscoaguirre Resolved the conflict, should be ready for merging now. |
This was referenced Apr 3, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 24, 2025
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—"nonfungible**s**.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](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/polkadot/xcm/xcm-builder/src/nonfungible_adapter.rs#L208-L212), [in-collection](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/polkadot/xcm/xcm-builder/src/nonfungibles_adapter.rs#L225-L229) (and [the unfinished one for v2](https://github.com/paritytech/polkadot-sdk/blob/3b401b02115e08f38f33ce8f3b3825e773a6113e/polkadot/xcm/xcm-builder/src/nonfungibles_v2_adapter.rs#L249-L254)), this means that, in general, _**the only**_ supported derivative NFTs are those whose chain-local IDs can be derived by the `Matcher` and 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.** <details> <summary><i>Q&A: AssetHub uses the two pallets approach local and foreign assets. Why is this not an issue there?</i></summary> >Since the primary goal of AssetHub (as far as I understand) is to host assets and not provide rich functionality around them (which is the task of other parachains), having a specialized NFT engine instance for derivatives is okay. Even if AssetHub would provide NFT-related operations (e.g., fractionalization), I think the number of different kinds of such operations would be limited, so it would be pretty easy to maintain them for two NFT engines. I even believe that supporting chain-local derivative IDs on AssetHub would be needlessly more complicated than having two NFT engines. </details> <details> <summary><i>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?</i></summary> >I think it makes sense. Moreover, it could be one of the reasons for employing reserve-based transfer for an NFT. Imagine a chain with no such functionality, and you have an NFT on that chain. And you want to fractionalize that NFT. You can transfer the NFT to another chain that provides NFT fractionalization. This way, you can model shared ownership of the original asset via its derivative. The same would be true for any NFT operation not provided by the chain where the NFT is located, while another chain can provide the needed functionality. </details> 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](#2924) introduced the `NonFungibleAdapter` (collection-less). The description states that the `NonFungibleAdapter` "will be useful for enabling cross-chain Coretime region transfers, as the existing `NonFungiblesAdapter`[^1] 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. <details> <summary><i>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?</i></summary> >I could argue that it is also a problem for fungibles, but I believe they are okay as they are. Firstly, fungible tokens are a simpler concept since, in one way or another, they represent the money-like value abstraction. It seems the number of different kinds of related operations is bound (in contrast to NFTs, which could be various utility objects with different related operations, just like objects in OOP). >Also, not all things that induce duplication apply to fungible(s) traits. For example, "a fungible collection" can not be viewed as a "fungible asset"—that's impossible, so having additional methods for "fungible collections" is okay. But at the same time, any collection (fungible or not) **can** be viewed as an NFT. It's not a "token" in the strict sense, but it is a unique object. This is precisely what NFTs represent. An NFT collection often has a similar interface to NFTs: create/transfer/destroy/metadata-related operations, etc. Of course, collections can have more methods that make sense only for collections but not their tokens, but this doesn't cancel the fact that collections can be viewed as another "kind" of NFTs. >Secondly, the fungible(s) trait sets are already granular. For example, multiple Inspect and Mutate traits are categorized by operation kind. Here is the Inspect/Mutate for [metadata](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungibles/metadata.rs) and here is the separate traits for [holds](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungibles/hold.rs). For comparison, the nonfungible(_v2)(s) trait sets have all the kinds of operations in uncategorized Inspect/Mutate/Transfer traits. >The fungible(s) traits are granular but not too abstract. I believe it is a good thing. Using the abstract traits from this PR, even for fungibles, is possible, but I see no reason to do so. A more concrete interface for fungibles seems even better because the very notion of fungibles outlines the possible related operations. </details> <details> <summary><i>Q&A: If it is not an issue for fungibles, why would this be an issue for NFTs?</i></summary> >Unlike fungibles, different NFTs could represent any object-like thing. Just like with objects in OOP, it is natural to expect them to have different inherent operations (e.g., different kinds of attributes, permission-based/role-based modification, etc.). The more abstract traits should help maintain interoperability between any NFT engine and other pallets. Even if we'd need some "adapters," they could be made easily because of the abstract traits. </details> #### 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 `burn` method provide a way to check if the operation is permitted, but `transfer` and `set_attribute` do not? In the `transfer` case, there is already an induced [mistake](#4073) 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, the `destroy` method for a collection [takes](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/substrate/frame/support/src/traits/tokens/nonfungibles.rs#L161-L165) 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, [takes](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L249) `deposit_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 `Mutate` trait). And even those that can be modified (`attribute` and `metadata`) have inconsistent interfaces: * `set_attribute` sets the attribute without any permission checks. * [in-collection](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L265-L273) * [collection-less](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungible_v2.rs#L143-L146) * [`set_metadata`](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungible_v2.rs#L161-L164) sets the metadata using the `who: AccountId` parameter for a permission check. * `set_metadata` is a collection-less variant of [`set_item_metadata`](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L313-L316), while `set_attribute` has the same name in both trait sets. * In contrast to `set_metadata`, other methods (even the `set_item_metadata`!) that do the permission check use `Option<AccountId>` instead of `AccountId`. * The same goes for the corresponding `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 `attribute` [returns](https://github.com/paritytech/polkadot-sdk/blob/8d81f1e648a21d7d14f94bc86503d3c77ead5807/substrate/frame/nfts/src/impl_nonfungibles.rs#L44-L62) an attribute from `CollectionOwner` namespace or metadata, but `set_attribute` [sets](https://github.com/paritytech/polkadot-sdk/blob/8d81f1e648a21d7d14f94bc86503d3c77ead5807/substrate/frame/nfts/src/impl_nonfungibles.rs#L266-L280) an attribute in `Pallet` namespace (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_ops` module is added to `frame_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: * Create * Inspect * Update * Destroy * Stash * Restore <details> <summary>Q&A: What do Inspect and Update operations mean?</summary> >Inspect is an interface meant to inspect any information about an asset. This information could be 1) asset owner, 2) attribute bytes, 3) a flag representing the asset's ability to be transferred, or 4) any other "feature" of the asset. >The Update is the corresponding interface for updating this information. </details> <details> <summary>Q&A: What do Stash/Restore operations mean?</summary> >This can be considered a variant of "Locking," but I decided to call it "Stash" because the actual "lock" operation is represented by the `CanUpdate<Owner<AccountId>>` update strategy. "Stash" implies losing ownership of the token to the chain itself. The symmetrical "Restore" operation may restore the token to any location, not just the before-stash owner. It depends on the particular chain business logic. </details> 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. ### 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: * Collection "from-to" transfer and a transfer without a check. * The similar transfers for NFTs * NFT force-transfers * A flag representing the ability of a collection to be transferred * The same flag for NFTs * NFT byte data * NFT attributes like in the pallet-uniques (byte data under a byte key) Here is how this will look: ```rust pub struct Collection<PalletInstance>(PhantomData<PalletInstance>); pub struct Token<PalletInstance>(PhantomData<PalletInstance>); impl AssetDefinition for Collection<NftEngine> { type Id = /* the collection ID type */; } impl AssetDefinition for Token<NftEngine> { type Id = /* the *full* NFT ID type */; } // --- Collection operations --- // The collection transfer without checks impl Update<Owner<AccountId>> for Collection<NftEngine> { fn update( class_id: &Self::Id, _strategy: Owner<AccountId>, new_owner: &AccountId, ) -> DispatchResult { todo!("use NftEngine internals to perform the collection transfer to the `new_owner`") } } // The collection "from-to" transfer impl Update<ChangeOwnerFrom<AccountId>> for Collection<NftEngine> { fn update( class_id: &Self::Id, strategy: ChangeOwnerFrom<AccountId>, new_owner: &AccountId, ) -> DispatchResult { let CheckState(from, ..) = strategy; todo!("check if `from` is the current owner"); // Reuse the previous impl Self::update(class_id, Owner::default(), new_owner) } } // A flag representing the ability of a collection to be transferred impl Inspect<CanUpdate<Owner<AccountId>>> for Collection<NftEngine> { fn inspect( class_id: &Self::Id, _can_transfer: CanUpdate<Owner<AccountId>>, ) -> Result<bool, DispatchError> { todo!("use NftEngine internals to learn if the collection can be transferred") } } // --- NFT operations --- // The NFT transfer implementation is similar in structure. // The NFT transfer without checks impl Update<Owner<AccountId>> for Token<NftEngine> { fn update( instance_id: &Self::Id, _strategy: Owner<AccountId>, new_owner: &AccountId, ) -> DispatchResult { todo!("use NftEngine internals to perform the NFT transfer") } } // The NFT "from-to" transfer impl Update<ChangeOwnerFrom<AccountId>> for Token<NftEngine> { fn update( instance_id: &Self::Id, strategy: ChangeOwnerFrom<AccountId><AccountId>, new_owner: &AccountId, ) -> DispatchResult { let CheckState(from, ..) = strategy; todo!("check if `from` is the current owner"); // Reuse the previous impl Self::transfer(instance_id, Owner::default(), new_owner) } } // There are meta-strategies like CheckOrigin, which carries an Origin and any internal strategy. // It abstracts origin checks for any possible operation. // For example, we can do this to implement NFT force-transfers impl Update<CheckOrigin<RuntimeOrigin, Owner<AccountId>>> for Token<NftEngine> { fn update( instance_id: &Self::Id, strategy: CheckOrigin<RuntimeOrigin, Owner<AccountId>>, new_owner: &AccountId, ) -> DispatchResult { let CheckOrigin(origin, owner_strategy) = strategy; ensure_root(origin)?; Self::transfer(instance_id, owner_strategy, new_owner) } } // A flag representing the ability of an NFT to be transferred impl Inspect<CanUpdate<Owner<AccountId>>> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, _can_transfer: CanUpdate<Owner<AccountId>>, ) -> Result<bool, DispatchError> { todo!("use NftEngine internals to learn if the NFT can be transferred") } } // The NFT bytes (notice that we have a different return type because of the "Bytes" strategy). impl Inspect<Bytes> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, _bytes: Bytes, ) -> Result<Vec<u8>, DispatchError> { todo!("use NftEngine internals to get the NFT bytes") } } // Some strategies like Bytes and CanUpdate are generic so that they can have different "parameters". // We can add a custom byte request called "Attribute" to make the attribute logic for NFTs. Its parameter carries the key. // Note: in this PR, pallet-uniques provides the Attribute request: https://github.com/UniqueNetwork/polkadot-sdk/blob/fb55a66a657b9e357a0b0a9490773221d3ef03bf/substrate/frame/uniques/src/types.rs#L151. // For self-containment, let's declare the pallet-uniques' `Attribute` here. pub struct Attribute<'a>(pub &'a [u8]); // The NFT attributes implementation impl<'a> Inspect<Bytes<Attribute<'a>>> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, strategy: Bytes<Attribute>, ) -> Result<Vec<u8>, DispatchError> { let Bytes(Attribute(attribute_key)) = strategy; todo!("use NftEngine internals to get the attribute bytes") } } ``` For further examples, see how pallet-uniques implements these operations for [collections](https://github.com/UniqueNetwork/polkadot-sdk/blob/feature/asset-ops-traits-only/substrate/frame/uniques/src/asset_ops/collection.rs) and [items](https://github.com/UniqueNetwork/polkadot-sdk/blob/feature/asset-ops-traits-only/substrate/frame/uniques/src/asset_ops/item.rs). The usage examples can be found in the `asset_ops` module docs (which is based on [this comment](#5620 (comment))) and in the pallet-uniques tests. [^1]: Don't confuse `NonFungibleAdapter` (collection-less) and `NonFungiblesAdapter` (in-collection; see "s" in the name). --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
wassimans
pushed a commit
to wassimans/polkadot-sdk
that referenced
this pull request
Apr 27, 2025
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 paritytech#4300 (which contains the new XCM adapters). The paritytech#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—"nonfungible**s**.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](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/polkadot/xcm/xcm-builder/src/nonfungible_adapter.rs#L208-L212), [in-collection](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/polkadot/xcm/xcm-builder/src/nonfungibles_adapter.rs#L225-L229) (and [the unfinished one for v2](https://github.com/paritytech/polkadot-sdk/blob/3b401b02115e08f38f33ce8f3b3825e773a6113e/polkadot/xcm/xcm-builder/src/nonfungibles_v2_adapter.rs#L249-L254)), this means that, in general, _**the only**_ supported derivative NFTs are those whose chain-local IDs can be derived by the `Matcher` and 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.** <details> <summary><i>Q&A: AssetHub uses the two pallets approach local and foreign assets. Why is this not an issue there?</i></summary> >Since the primary goal of AssetHub (as far as I understand) is to host assets and not provide rich functionality around them (which is the task of other parachains), having a specialized NFT engine instance for derivatives is okay. Even if AssetHub would provide NFT-related operations (e.g., fractionalization), I think the number of different kinds of such operations would be limited, so it would be pretty easy to maintain them for two NFT engines. I even believe that supporting chain-local derivative IDs on AssetHub would be needlessly more complicated than having two NFT engines. </details> <details> <summary><i>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?</i></summary> >I think it makes sense. Moreover, it could be one of the reasons for employing reserve-based transfer for an NFT. Imagine a chain with no such functionality, and you have an NFT on that chain. And you want to fractionalize that NFT. You can transfer the NFT to another chain that provides NFT fractionalization. This way, you can model shared ownership of the original asset via its derivative. The same would be true for any NFT operation not provided by the chain where the NFT is located, while another chain can provide the needed functionality. </details> 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](paritytech#2924) introduced the `NonFungibleAdapter` (collection-less). The description states that the `NonFungibleAdapter` "will be useful for enabling cross-chain Coretime region transfers, as the existing `NonFungiblesAdapter`[^1] 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. <details> <summary><i>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?</i></summary> >I could argue that it is also a problem for fungibles, but I believe they are okay as they are. Firstly, fungible tokens are a simpler concept since, in one way or another, they represent the money-like value abstraction. It seems the number of different kinds of related operations is bound (in contrast to NFTs, which could be various utility objects with different related operations, just like objects in OOP). >Also, not all things that induce duplication apply to fungible(s) traits. For example, "a fungible collection" can not be viewed as a "fungible asset"—that's impossible, so having additional methods for "fungible collections" is okay. But at the same time, any collection (fungible or not) **can** be viewed as an NFT. It's not a "token" in the strict sense, but it is a unique object. This is precisely what NFTs represent. An NFT collection often has a similar interface to NFTs: create/transfer/destroy/metadata-related operations, etc. Of course, collections can have more methods that make sense only for collections but not their tokens, but this doesn't cancel the fact that collections can be viewed as another "kind" of NFTs. >Secondly, the fungible(s) trait sets are already granular. For example, multiple Inspect and Mutate traits are categorized by operation kind. Here is the Inspect/Mutate for [metadata](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungibles/metadata.rs) and here is the separate traits for [holds](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungibles/hold.rs). For comparison, the nonfungible(_v2)(s) trait sets have all the kinds of operations in uncategorized Inspect/Mutate/Transfer traits. >The fungible(s) traits are granular but not too abstract. I believe it is a good thing. Using the abstract traits from this PR, even for fungibles, is possible, but I see no reason to do so. A more concrete interface for fungibles seems even better because the very notion of fungibles outlines the possible related operations. </details> <details> <summary><i>Q&A: If it is not an issue for fungibles, why would this be an issue for NFTs?</i></summary> >Unlike fungibles, different NFTs could represent any object-like thing. Just like with objects in OOP, it is natural to expect them to have different inherent operations (e.g., different kinds of attributes, permission-based/role-based modification, etc.). The more abstract traits should help maintain interoperability between any NFT engine and other pallets. Even if we'd need some "adapters," they could be made easily because of the abstract traits. </details> #### 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 `burn` method provide a way to check if the operation is permitted, but `transfer` and `set_attribute` do not? In the `transfer` case, there is already an induced [mistake](paritytech#4073) 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, the `destroy` method for a collection [takes](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/substrate/frame/support/src/traits/tokens/nonfungibles.rs#L161-L165) 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, [takes](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L249) `deposit_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 `Mutate` trait). And even those that can be modified (`attribute` and `metadata`) have inconsistent interfaces: * `set_attribute` sets the attribute without any permission checks. * [in-collection](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L265-L273) * [collection-less](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungible_v2.rs#L143-L146) * [`set_metadata`](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungible_v2.rs#L161-L164) sets the metadata using the `who: AccountId` parameter for a permission check. * `set_metadata` is a collection-less variant of [`set_item_metadata`](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L313-L316), while `set_attribute` has the same name in both trait sets. * In contrast to `set_metadata`, other methods (even the `set_item_metadata`!) that do the permission check use `Option<AccountId>` instead of `AccountId`. * The same goes for the corresponding `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 `attribute` [returns](https://github.com/paritytech/polkadot-sdk/blob/8d81f1e648a21d7d14f94bc86503d3c77ead5807/substrate/frame/nfts/src/impl_nonfungibles.rs#L44-L62) an attribute from `CollectionOwner` namespace or metadata, but `set_attribute` [sets](https://github.com/paritytech/polkadot-sdk/blob/8d81f1e648a21d7d14f94bc86503d3c77ead5807/substrate/frame/nfts/src/impl_nonfungibles.rs#L266-L280) an attribute in `Pallet` namespace (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_ops` module is added to `frame_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: * Create * Inspect * Update * Destroy * Stash * Restore <details> <summary>Q&A: What do Inspect and Update operations mean?</summary> >Inspect is an interface meant to inspect any information about an asset. This information could be 1) asset owner, 2) attribute bytes, 3) a flag representing the asset's ability to be transferred, or 4) any other "feature" of the asset. >The Update is the corresponding interface for updating this information. </details> <details> <summary>Q&A: What do Stash/Restore operations mean?</summary> >This can be considered a variant of "Locking," but I decided to call it "Stash" because the actual "lock" operation is represented by the `CanUpdate<Owner<AccountId>>` update strategy. "Stash" implies losing ownership of the token to the chain itself. The symmetrical "Restore" operation may restore the token to any location, not just the before-stash owner. It depends on the particular chain business logic. </details> 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. ### 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: * Collection "from-to" transfer and a transfer without a check. * The similar transfers for NFTs * NFT force-transfers * A flag representing the ability of a collection to be transferred * The same flag for NFTs * NFT byte data * NFT attributes like in the pallet-uniques (byte data under a byte key) Here is how this will look: ```rust pub struct Collection<PalletInstance>(PhantomData<PalletInstance>); pub struct Token<PalletInstance>(PhantomData<PalletInstance>); impl AssetDefinition for Collection<NftEngine> { type Id = /* the collection ID type */; } impl AssetDefinition for Token<NftEngine> { type Id = /* the *full* NFT ID type */; } // --- Collection operations --- // The collection transfer without checks impl Update<Owner<AccountId>> for Collection<NftEngine> { fn update( class_id: &Self::Id, _strategy: Owner<AccountId>, new_owner: &AccountId, ) -> DispatchResult { todo!("use NftEngine internals to perform the collection transfer to the `new_owner`") } } // The collection "from-to" transfer impl Update<ChangeOwnerFrom<AccountId>> for Collection<NftEngine> { fn update( class_id: &Self::Id, strategy: ChangeOwnerFrom<AccountId>, new_owner: &AccountId, ) -> DispatchResult { let CheckState(from, ..) = strategy; todo!("check if `from` is the current owner"); // Reuse the previous impl Self::update(class_id, Owner::default(), new_owner) } } // A flag representing the ability of a collection to be transferred impl Inspect<CanUpdate<Owner<AccountId>>> for Collection<NftEngine> { fn inspect( class_id: &Self::Id, _can_transfer: CanUpdate<Owner<AccountId>>, ) -> Result<bool, DispatchError> { todo!("use NftEngine internals to learn if the collection can be transferred") } } // --- NFT operations --- // The NFT transfer implementation is similar in structure. // The NFT transfer without checks impl Update<Owner<AccountId>> for Token<NftEngine> { fn update( instance_id: &Self::Id, _strategy: Owner<AccountId>, new_owner: &AccountId, ) -> DispatchResult { todo!("use NftEngine internals to perform the NFT transfer") } } // The NFT "from-to" transfer impl Update<ChangeOwnerFrom<AccountId>> for Token<NftEngine> { fn update( instance_id: &Self::Id, strategy: ChangeOwnerFrom<AccountId><AccountId>, new_owner: &AccountId, ) -> DispatchResult { let CheckState(from, ..) = strategy; todo!("check if `from` is the current owner"); // Reuse the previous impl Self::transfer(instance_id, Owner::default(), new_owner) } } // There are meta-strategies like CheckOrigin, which carries an Origin and any internal strategy. // It abstracts origin checks for any possible operation. // For example, we can do this to implement NFT force-transfers impl Update<CheckOrigin<RuntimeOrigin, Owner<AccountId>>> for Token<NftEngine> { fn update( instance_id: &Self::Id, strategy: CheckOrigin<RuntimeOrigin, Owner<AccountId>>, new_owner: &AccountId, ) -> DispatchResult { let CheckOrigin(origin, owner_strategy) = strategy; ensure_root(origin)?; Self::transfer(instance_id, owner_strategy, new_owner) } } // A flag representing the ability of an NFT to be transferred impl Inspect<CanUpdate<Owner<AccountId>>> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, _can_transfer: CanUpdate<Owner<AccountId>>, ) -> Result<bool, DispatchError> { todo!("use NftEngine internals to learn if the NFT can be transferred") } } // The NFT bytes (notice that we have a different return type because of the "Bytes" strategy). impl Inspect<Bytes> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, _bytes: Bytes, ) -> Result<Vec<u8>, DispatchError> { todo!("use NftEngine internals to get the NFT bytes") } } // Some strategies like Bytes and CanUpdate are generic so that they can have different "parameters". // We can add a custom byte request called "Attribute" to make the attribute logic for NFTs. Its parameter carries the key. // Note: in this PR, pallet-uniques provides the Attribute request: https://github.com/UniqueNetwork/polkadot-sdk/blob/fb55a66a657b9e357a0b0a9490773221d3ef03bf/substrate/frame/uniques/src/types.rs#L151. // For self-containment, let's declare the pallet-uniques' `Attribute` here. pub struct Attribute<'a>(pub &'a [u8]); // The NFT attributes implementation impl<'a> Inspect<Bytes<Attribute<'a>>> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, strategy: Bytes<Attribute>, ) -> Result<Vec<u8>, DispatchError> { let Bytes(Attribute(attribute_key)) = strategy; todo!("use NftEngine internals to get the attribute bytes") } } ``` For further examples, see how pallet-uniques implements these operations for [collections](https://github.com/UniqueNetwork/polkadot-sdk/blob/feature/asset-ops-traits-only/substrate/frame/uniques/src/asset_ops/collection.rs) and [items](https://github.com/UniqueNetwork/polkadot-sdk/blob/feature/asset-ops-traits-only/substrate/frame/uniques/src/asset_ops/item.rs). The usage examples can be found in the `asset_ops` module docs (which is based on [this comment](paritytech#5620 (comment))) and in the pallet-uniques tests. [^1]: Don't confuse `NonFungibleAdapter` (collection-less) and `NonFungiblesAdapter` (in-collection; see "s" in the name). --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
castillax
pushed a commit
that referenced
this pull request
May 12, 2025
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—"nonfungible**s**.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](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/polkadot/xcm/xcm-builder/src/nonfungible_adapter.rs#L208-L212), [in-collection](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/polkadot/xcm/xcm-builder/src/nonfungibles_adapter.rs#L225-L229) (and [the unfinished one for v2](https://github.com/paritytech/polkadot-sdk/blob/3b401b02115e08f38f33ce8f3b3825e773a6113e/polkadot/xcm/xcm-builder/src/nonfungibles_v2_adapter.rs#L249-L254)), this means that, in general, _**the only**_ supported derivative NFTs are those whose chain-local IDs can be derived by the `Matcher` and 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.** <details> <summary><i>Q&A: AssetHub uses the two pallets approach local and foreign assets. Why is this not an issue there?</i></summary> >Since the primary goal of AssetHub (as far as I understand) is to host assets and not provide rich functionality around them (which is the task of other parachains), having a specialized NFT engine instance for derivatives is okay. Even if AssetHub would provide NFT-related operations (e.g., fractionalization), I think the number of different kinds of such operations would be limited, so it would be pretty easy to maintain them for two NFT engines. I even believe that supporting chain-local derivative IDs on AssetHub would be needlessly more complicated than having two NFT engines. </details> <details> <summary><i>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?</i></summary> >I think it makes sense. Moreover, it could be one of the reasons for employing reserve-based transfer for an NFT. Imagine a chain with no such functionality, and you have an NFT on that chain. And you want to fractionalize that NFT. You can transfer the NFT to another chain that provides NFT fractionalization. This way, you can model shared ownership of the original asset via its derivative. The same would be true for any NFT operation not provided by the chain where the NFT is located, while another chain can provide the needed functionality. </details> 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](#2924) introduced the `NonFungibleAdapter` (collection-less). The description states that the `NonFungibleAdapter` "will be useful for enabling cross-chain Coretime region transfers, as the existing `NonFungiblesAdapter`[^1] 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. <details> <summary><i>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?</i></summary> >I could argue that it is also a problem for fungibles, but I believe they are okay as they are. Firstly, fungible tokens are a simpler concept since, in one way or another, they represent the money-like value abstraction. It seems the number of different kinds of related operations is bound (in contrast to NFTs, which could be various utility objects with different related operations, just like objects in OOP). >Also, not all things that induce duplication apply to fungible(s) traits. For example, "a fungible collection" can not be viewed as a "fungible asset"—that's impossible, so having additional methods for "fungible collections" is okay. But at the same time, any collection (fungible or not) **can** be viewed as an NFT. It's not a "token" in the strict sense, but it is a unique object. This is precisely what NFTs represent. An NFT collection often has a similar interface to NFTs: create/transfer/destroy/metadata-related operations, etc. Of course, collections can have more methods that make sense only for collections but not their tokens, but this doesn't cancel the fact that collections can be viewed as another "kind" of NFTs. >Secondly, the fungible(s) trait sets are already granular. For example, multiple Inspect and Mutate traits are categorized by operation kind. Here is the Inspect/Mutate for [metadata](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungibles/metadata.rs) and here is the separate traits for [holds](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungibles/hold.rs). For comparison, the nonfungible(_v2)(s) trait sets have all the kinds of operations in uncategorized Inspect/Mutate/Transfer traits. >The fungible(s) traits are granular but not too abstract. I believe it is a good thing. Using the abstract traits from this PR, even for fungibles, is possible, but I see no reason to do so. A more concrete interface for fungibles seems even better because the very notion of fungibles outlines the possible related operations. </details> <details> <summary><i>Q&A: If it is not an issue for fungibles, why would this be an issue for NFTs?</i></summary> >Unlike fungibles, different NFTs could represent any object-like thing. Just like with objects in OOP, it is natural to expect them to have different inherent operations (e.g., different kinds of attributes, permission-based/role-based modification, etc.). The more abstract traits should help maintain interoperability between any NFT engine and other pallets. Even if we'd need some "adapters," they could be made easily because of the abstract traits. </details> #### 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 `burn` method provide a way to check if the operation is permitted, but `transfer` and `set_attribute` do not? In the `transfer` case, there is already an induced [mistake](#4073) 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, the `destroy` method for a collection [takes](https://github.com/paritytech/polkadot-sdk/blob/4057ccd7a37396bc1c6d1742f418415af61b2787/substrate/frame/support/src/traits/tokens/nonfungibles.rs#L161-L165) 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, [takes](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L249) `deposit_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 `Mutate` trait). And even those that can be modified (`attribute` and `metadata`) have inconsistent interfaces: * `set_attribute` sets the attribute without any permission checks. * [in-collection](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L265-L273) * [collection-less](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungible_v2.rs#L143-L146) * [`set_metadata`](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungible_v2.rs#L161-L164) sets the metadata using the `who: AccountId` parameter for a permission check. * `set_metadata` is a collection-less variant of [`set_item_metadata`](https://github.com/paritytech/polkadot-sdk/blob/7e7c33453eeb14f47c6c4d0f98cc982e485edc77/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs#L313-L316), while `set_attribute` has the same name in both trait sets. * In contrast to `set_metadata`, other methods (even the `set_item_metadata`!) that do the permission check use `Option<AccountId>` instead of `AccountId`. * The same goes for the corresponding `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 `attribute` [returns](https://github.com/paritytech/polkadot-sdk/blob/8d81f1e648a21d7d14f94bc86503d3c77ead5807/substrate/frame/nfts/src/impl_nonfungibles.rs#L44-L62) an attribute from `CollectionOwner` namespace or metadata, but `set_attribute` [sets](https://github.com/paritytech/polkadot-sdk/blob/8d81f1e648a21d7d14f94bc86503d3c77ead5807/substrate/frame/nfts/src/impl_nonfungibles.rs#L266-L280) an attribute in `Pallet` namespace (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_ops` module is added to `frame_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: * Create * Inspect * Update * Destroy * Stash * Restore <details> <summary>Q&A: What do Inspect and Update operations mean?</summary> >Inspect is an interface meant to inspect any information about an asset. This information could be 1) asset owner, 2) attribute bytes, 3) a flag representing the asset's ability to be transferred, or 4) any other "feature" of the asset. >The Update is the corresponding interface for updating this information. </details> <details> <summary>Q&A: What do Stash/Restore operations mean?</summary> >This can be considered a variant of "Locking," but I decided to call it "Stash" because the actual "lock" operation is represented by the `CanUpdate<Owner<AccountId>>` update strategy. "Stash" implies losing ownership of the token to the chain itself. The symmetrical "Restore" operation may restore the token to any location, not just the before-stash owner. It depends on the particular chain business logic. </details> 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. ### 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: * Collection "from-to" transfer and a transfer without a check. * The similar transfers for NFTs * NFT force-transfers * A flag representing the ability of a collection to be transferred * The same flag for NFTs * NFT byte data * NFT attributes like in the pallet-uniques (byte data under a byte key) Here is how this will look: ```rust pub struct Collection<PalletInstance>(PhantomData<PalletInstance>); pub struct Token<PalletInstance>(PhantomData<PalletInstance>); impl AssetDefinition for Collection<NftEngine> { type Id = /* the collection ID type */; } impl AssetDefinition for Token<NftEngine> { type Id = /* the *full* NFT ID type */; } // --- Collection operations --- // The collection transfer without checks impl Update<Owner<AccountId>> for Collection<NftEngine> { fn update( class_id: &Self::Id, _strategy: Owner<AccountId>, new_owner: &AccountId, ) -> DispatchResult { todo!("use NftEngine internals to perform the collection transfer to the `new_owner`") } } // The collection "from-to" transfer impl Update<ChangeOwnerFrom<AccountId>> for Collection<NftEngine> { fn update( class_id: &Self::Id, strategy: ChangeOwnerFrom<AccountId>, new_owner: &AccountId, ) -> DispatchResult { let CheckState(from, ..) = strategy; todo!("check if `from` is the current owner"); // Reuse the previous impl Self::update(class_id, Owner::default(), new_owner) } } // A flag representing the ability of a collection to be transferred impl Inspect<CanUpdate<Owner<AccountId>>> for Collection<NftEngine> { fn inspect( class_id: &Self::Id, _can_transfer: CanUpdate<Owner<AccountId>>, ) -> Result<bool, DispatchError> { todo!("use NftEngine internals to learn if the collection can be transferred") } } // --- NFT operations --- // The NFT transfer implementation is similar in structure. // The NFT transfer without checks impl Update<Owner<AccountId>> for Token<NftEngine> { fn update( instance_id: &Self::Id, _strategy: Owner<AccountId>, new_owner: &AccountId, ) -> DispatchResult { todo!("use NftEngine internals to perform the NFT transfer") } } // The NFT "from-to" transfer impl Update<ChangeOwnerFrom<AccountId>> for Token<NftEngine> { fn update( instance_id: &Self::Id, strategy: ChangeOwnerFrom<AccountId><AccountId>, new_owner: &AccountId, ) -> DispatchResult { let CheckState(from, ..) = strategy; todo!("check if `from` is the current owner"); // Reuse the previous impl Self::transfer(instance_id, Owner::default(), new_owner) } } // There are meta-strategies like CheckOrigin, which carries an Origin and any internal strategy. // It abstracts origin checks for any possible operation. // For example, we can do this to implement NFT force-transfers impl Update<CheckOrigin<RuntimeOrigin, Owner<AccountId>>> for Token<NftEngine> { fn update( instance_id: &Self::Id, strategy: CheckOrigin<RuntimeOrigin, Owner<AccountId>>, new_owner: &AccountId, ) -> DispatchResult { let CheckOrigin(origin, owner_strategy) = strategy; ensure_root(origin)?; Self::transfer(instance_id, owner_strategy, new_owner) } } // A flag representing the ability of an NFT to be transferred impl Inspect<CanUpdate<Owner<AccountId>>> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, _can_transfer: CanUpdate<Owner<AccountId>>, ) -> Result<bool, DispatchError> { todo!("use NftEngine internals to learn if the NFT can be transferred") } } // The NFT bytes (notice that we have a different return type because of the "Bytes" strategy). impl Inspect<Bytes> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, _bytes: Bytes, ) -> Result<Vec<u8>, DispatchError> { todo!("use NftEngine internals to get the NFT bytes") } } // Some strategies like Bytes and CanUpdate are generic so that they can have different "parameters". // We can add a custom byte request called "Attribute" to make the attribute logic for NFTs. Its parameter carries the key. // Note: in this PR, pallet-uniques provides the Attribute request: https://github.com/UniqueNetwork/polkadot-sdk/blob/fb55a66a657b9e357a0b0a9490773221d3ef03bf/substrate/frame/uniques/src/types.rs#L151. // For self-containment, let's declare the pallet-uniques' `Attribute` here. pub struct Attribute<'a>(pub &'a [u8]); // The NFT attributes implementation impl<'a> Inspect<Bytes<Attribute<'a>>> for Token<NftEngine> { fn inspect( instance_id: &Self::Id, strategy: Bytes<Attribute>, ) -> Result<Vec<u8>, DispatchError> { let Bytes(Attribute(attribute_key)) = strategy; todo!("use NftEngine internals to get the attribute bytes") } } ``` For further examples, see how pallet-uniques implements these operations for [collections](https://github.com/UniqueNetwork/polkadot-sdk/blob/feature/asset-ops-traits-only/substrate/frame/uniques/src/asset_ops/collection.rs) and [items](https://github.com/UniqueNetwork/polkadot-sdk/blob/feature/asset-ops-traits-only/substrate/frame/uniques/src/asset_ops/item.rs). The usage examples can be found in the `asset_ops` module docs (which is based on [this comment](#5620 (comment))) and in the pallet-uniques tests. [^1]: Don't confuse `NonFungibleAdapter` (collection-less) and `NonFungiblesAdapter` (in-collection; see "s" in the name). --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a new
NonFungibleAdapter.It will be useful for enabling cross-chain Coretime region transfers, as the existing
NonFungiblesAdapteris unsuitable for this purpose. This is due to the fact that there is only one class of items within thepallet-broker, i.e., the Coretime regions.