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

NFTs 2.0 on Statemine #2314

Merged
merged 4 commits into from
Mar 21, 2023
Merged

NFTs 2.0 on Statemine #2314

merged 4 commits into from
Mar 21, 2023

Conversation

jsidorenko
Copy link
Contributor

This PR adds the new nfts pallet to statemine config

@jsidorenko jsidorenko added B0-silent Changes should not be mentioned in any release notes A0-please_review Pull request needs code review. 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 14, 2023
@paritytech-ci paritytech-ci requested review from a team March 14, 2023 11:12
@jsidorenko
Copy link
Contributor Author

bench $ pallet statemine-dev pallet_nfts

type CollectionId = u32;
type ItemId = u32;
type Currency = Balances;
type CreateOrigin = AsEnsureOriginWithArg<EnsureSigned<AccountId>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should allow foreign origins as well, so that collectives can create collections.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be an upgrade.

Copy link
Contributor Author

@jsidorenko jsidorenko Mar 16, 2023

Choose a reason for hiding this comment

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

let's do this as an upgrade once we get to the integration with collectives

Comment on lines +231 to +239
pallet_nfts::Call::set_attribute { .. } |
pallet_nfts::Call::force_set_attribute { .. } |
pallet_nfts::Call::clear_attribute { .. } |
pallet_nfts::Call::approve_item_attributes { .. } |
pallet_nfts::Call::cancel_item_attributes_approval { .. } |
pallet_nfts::Call::set_metadata { .. } |
pallet_nfts::Call::clear_metadata { .. } |
pallet_nfts::Call::set_collection_metadata { .. } |
pallet_nfts::Call::clear_collection_metadata { .. } |
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these have bounds on their encoded size?

Copy link
Contributor Author

@jsidorenko jsidorenko Mar 16, 2023

Choose a reason for hiding this comment

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

yep, none of those methods accept unbounded vecs, only the bounded ones

@paritytech-ci paritytech-ci requested a review from a team March 15, 2023 19:03
@@ -632,6 +686,7 @@ construct_runtime!(
// The main stage.
Assets: pallet_assets::<Instance1>::{Pallet, Call, Storage, Event<T>} = 50,
Uniques: pallet_uniques::{Pallet, Call, Storage, Event<T>} = 51,
Copy link
Contributor

Choose a reason for hiding this comment

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

so Uniques: pallet_uniques stays or any migration to nfts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's going to stay for now until we get the UI support for the new pallet and make a decision on the proper migration path

Comment on lines +634 to +636
type StringLimit = ConstU32<256>;
type KeyLimit = ConstU32<64>;
type ValueLimit = ConstU32<256>;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a rationale how those numbers were chosen and why they are different between pallet_uniques and pallet_nfts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I didn't want to change anything in pallet uniques
  2. in real apps it's very hard to fit anything in 32/64 bytes. Since users will put a deposit as per byte, I think it's safe to allow them to store up to 256 bytes of data in one attribute

@jsidorenko
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit a2e5508 into master Mar 21, 2023
@paritytech-processbot paritytech-processbot bot deleted the js/nfts-statemine branch March 21, 2023 11:25
EgorPopelyaev pushed a commit that referenced this pull request Mar 21, 2023
* Add nfts pallet to statemine

* Add missing trait

* Add nfts pallet to SafeCallFilter

* Re-use uniques deposits
ordian added a commit that referenced this pull request Mar 21, 2023
* master:
  Companion for #13624 (#2354)
  Introduce Fellowship into Collectives (#2186)
  NFTs 2.0 on Statemine (#2314)
  Bump assert_cmd from 2.0.8 to 2.0.10 (#2341)
  Bump clap from 4.1.8 to 4.1.11 (#2352)
  Companion for substrate #13312: Rename `Deterministic` to `Enforce` (#2350)
  [Companion #13634] keystore overhaul (iter) (#2345)
  Revert #2304 (#2349)
  Deprecate Currency: Companion for #12951 (#2334)
  Bump ci-linux image for rust 1.68
  Always pass port to jsonrpsee WebSocket client (#2339)
  bump zombienet to v1.3.40 (#2348)
  Improve build times by disabling wasm-builder in `no_std` (#2308)
  Bump toml from 0.7.2 to 0.7.3 (#2340)
  Bump serde from 1.0.152 to 1.0.156 (#2329)
  Parachains should charge for proof size weight (#2326)
  dmp-queue: Store messages if already processed more than the maximum (#2343)
  [Companion  #13615] Keystore overhaul (#2336)
  Bump quote from 1.0.23 to 1.0.26 (#2331)
@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-41-v0-9-42/2828/1

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. 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.

6 participants