Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

WIP: [NFTs] Simplify collection creation and item mint #13572

Closed
wants to merge 7 commits into from

Conversation

jsidorenko
Copy link
Contributor

@jsidorenko jsidorenko commented Mar 9, 2023

When trying to create a collection via traits, it requires providing the CollectionConfig param. This requires adding a lot of boilerplate code, which annoys when dealing with simple use cases.
This PR adds a new CreateSimplified trait that doesn't have the config param and puts the default collection's config.
Additionally, I made the item's config param optional in the mint_into() method

@jsidorenko jsidorenko added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 9, 2023
@jasl
Copy link
Contributor

jasl commented Mar 9, 2023

What about make the config optional? I'm concerning that in my use-case, I need customize the config when creating the collection, separates create and set may introduce avoidable state read and write?

@jsidorenko
Copy link
Contributor Author

Making it optional, unfortunately, doesn't remove the requirement to have a lot of boilerplate code

@jsidorenko
Copy link
Contributor Author

another possible solution would be to have two traits: Create and CreateConfigurable

@jasl
Copy link
Contributor

jasl commented Mar 9, 2023

Making it optional, unfortunately, doesn't remove the requirement to have a lot of boilerplate code

Understood, I think it is acceptable because create and mutate collections ususally not high-frequent operations

@jasl
Copy link
Contributor

jasl commented Mar 9, 2023

another possible solution would be to have two traits: Create and CreateConfigurable

Sounds cool!

@jsidorenko jsidorenko marked this pull request as draft March 10, 2023 08:44
@jsidorenko jsidorenko changed the title [NFTs] Simplify collection creation [NFTs] Simplify collection creation and item mint Mar 10, 2023
@jasl
Copy link
Contributor

jasl commented Mar 10, 2023

Thank you for help! I see you're working on improving mint_into here

I've refactored my pallet to nonfungibles_v2 interface again, I have few feedbacks:

nonfungibles_v2 interface mint_into can only deposit by none or the collection owner, but pallet-nfts do_mint can support specify depositor, and mint extrinsic call will set the caller as depositor which is make sense
So, I'd propose change mint_into that accept depositor: Option<T::AccountId> instead of deposit_collection_owner: bool

mint_into lacking MintWitness, but destroy has witness: Self::DestroyWitness,

set_attribute and set_typed_attribute (and collection version) pass set_as: None so no one can be the depositor of the attribute (or metadata), and it seems doesn't support Smart Attributes

set_collection_attribute given empty key Vec::<u8>::default() not set metadata, but an attribute

typed_attribute() delegates to attribute() which hard-coded namespace to CollectionOwner, but set_typed_attribute() hard-coded namespace to AttributeNamespace::Pallet

Off-topic: I propose separate read and write metadata from set_attribute and set_typed_attribute (and collection version), this would help to make more clear and simplify R&W metadata (avoid to construct a default Vec), making key optional is a solution but I still think separate them is better

@jasl
Copy link
Contributor

jasl commented Mar 10, 2023

Off-topic: just my opinion, nonfungibles_v2 is used by pallet which 100% controlled by developers, so I'm thinking all of maybe_check_* argument can be simplified, developer can do the permission check before call nft api
and, feature gate check should move out from features implementations, as developer, I call set_attribute is I must to use the feature, configure to enable it on pallet-nfts is a little bit odd... instead, the feature gates should be used to restrict pallet-nfts extrinsic, that's more security than filter extrinsic calls on Runtime because pallet-nfts has too much extrinsics and may change in the future, besides like my WIP pallet, my pallet will do minting NFTs so I want disable pallet-nft mint extrinic

@jasl
Copy link
Contributor

jasl commented Mar 10, 2023

I would love for help if you can give me some directions, those changes I think shouldn't difficult to me

@jsidorenko jsidorenko changed the title [NFTs] Simplify collection creation and item mint WIP: [NFTs] Simplify collection creation and item mint Mar 14, 2023
@stale
Copy link

stale bot commented Apr 14, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Apr 14, 2023
@stale stale bot closed this Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A3-stale B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants