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

[NFTs] Rework permissions model #13482

Merged
merged 28 commits into from
Mar 13, 2023
Merged

Conversation

jsidorenko
Copy link
Contributor

@jsidorenko jsidorenko commented Feb 27, 2023

This PR changes the permissions model in the pallet-nfts:

  1. an Admin account will no longer be able to transfer or burn items it doesn't own;
  2. to destroy the collection, there should be 0 items left;
  3. some actions were moved from one role to another to simplify accounts management.

For simple collections, the same account usually has all the roles.
But if we take complex collections, then each role here represents a separate service with its own responsibilities (e.g. items issuance), and it should be simple to rotate the keys of those services. At the same time, the main owner's account is usually used just to setup the collection and the private key is kept in cold storage afterwards.
Given all that, a bunch of frequently called actions were moved from the Owner role to Admin or Issuer.

So, here is the list of the main actions each role will be able to do:

Owner

  • destroy collection
  • redeposit
  • set_team
  • set_collection_max_supply
  • lock_collection (moved from Freezer) - (make non-transferable, fix max supply, lock collection metadata/attributes) -> one time call

Admin

  • set_attribute/set_metadata (in the CollectionOwner namespace) (moved from Owner)
  • set_attributes_pre_signed (moved from Owner)
  • lock_item_properties (moved from Owner) - (lock item metadata/attributes) -> one time call

Freezer

  • lock_item_transfer
  • unlock_item_transfer

Issuer

  • mint
  • force_mint (with custom item config)
  • mint_pre_signed (moved from Owner)
  • update_mint_settings (moved from Owner)

This PR closes: #13171

Cumulus Companion: paritytech/cumulus#2303

@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. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. labels Feb 27, 2023
@mustermeiszer
Copy link
Contributor

@jsidorenko that looks great! Thanks for adapting so quickly.

One question:

  • Why is this B0-silent although it contains E0-runtime_migration?

@jsidorenko jsidorenko added B1-note_worthy Changes should be noted in the release notes and removed B0-silent Changes should not be mentioned in any release notes labels Mar 2, 2023
@jsidorenko
Copy link
Contributor Author

@jsidorenko that looks great! Thanks for adapting so quickly.

One question:

  • Why is this B0-silent although it contains E0-runtime_migration?

It's because I'm still learning the labels science :)

@jsidorenko
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@jsidorenko
Copy link
Contributor Author

bot bench $ pallet dev pallet_nfts

@command-bot
Copy link

command-bot bot commented Mar 10, 2023

@jsidorenko https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2507636 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 35-88256996-026f-445e-a2ff-4f87c0c19640 to cancel this command or bot cancel to cancel all commands in this pull request.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just looking at the migration.

frame/nfts/src/migration.rs Outdated Show resolved Hide resolved
@command-bot
Copy link

command-bot bot commented Mar 10, 2023

@jsidorenko Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2507636 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2507636/artifacts/download.

@jsidorenko
Copy link
Contributor Author

bot bench $ pallet dev pallet_nfts

@command-bot
Copy link

command-bot bot commented Mar 10, 2023

@jsidorenko https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2508377 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 40-24f5b93a-3230-4f20-8b95-50edb54d8710 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 10, 2023

@jsidorenko Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2508377 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2508377/artifacts/download.

@jsidorenko
Copy link
Contributor Author

bot merge

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-40/2468/1

ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* Disallow admin to transfer or burn items he doesn't own

* lock_collection should be accessible by collection's owner only

* Allow admin to access lock_item_properties()

* Fix do_lock_item_properties

* Move update_mint_settings() to Issuer

* Rename check_owner to check_origin

* Typo

* Make admin to be in charge of managing the metadata

* Make admin the main attributes manager

* offchain mint should be signed by Issuer

* Remove the special case when the Issuer calls the mint() function

* Rework burn and destroy methods

* Return back item_metadatas

* Don't repatriate the deposit on transfer

* A bit more tests

* One more test

* Add migration

* Chore

* Clippy

* Rename to owned_item

* Address comments

* Replace .filter_map with .find_map

* Improve version validation in pre_upgrade()

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

---------

Co-authored-by: parity-processbot <>
@redzsina redzsina added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 14, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Disallow admin to transfer or burn items he doesn't own

* lock_collection should be accessible by collection's owner only

* Allow admin to access lock_item_properties()

* Fix do_lock_item_properties

* Move update_mint_settings() to Issuer

* Rename check_owner to check_origin

* Typo

* Make admin to be in charge of managing the metadata

* Make admin the main attributes manager

* offchain mint should be signed by Issuer

* Remove the special case when the Issuer calls the mint() function

* Rework burn and destroy methods

* Return back item_metadatas

* Don't repatriate the deposit on transfer

* A bit more tests

* One more test

* Add migration

* Chore

* Clippy

* Rename to owned_item

* Address comments

* Replace .filter_map with .find_map

* Improve version validation in pre_upgrade()

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

---------

Co-authored-by: parity-processbot <>
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Pallet-Uniques: Privileged User Actions
7 participants