Skip to content

Conversation

@ilionic
Copy link
Contributor

@ilionic ilionic commented Aug 4, 2022

Fixes #169

  • make resource_id required ( remove auto increment )
  • make nft_id required ( remove auto increment )
  • make Basic resource metadata required
  • integration tests

@ilionic ilionic changed the title Make resource_id a Make resource_id a required field Aug 4, 2022
@ilionic ilionic changed the title Make resource_id a required field Make resource_id required field Aug 4, 2022
@ilionic ilionic changed the title Make resource_id required field Make resource_id / nft_id required Aug 5, 2022
@ilionic ilionic marked this pull request as ready for review August 5, 2022 16:35
@ilionic ilionic requested review from HashWarlock and bmacer August 6, 2022 07:58
// Prevent minting when nfts_count is greater than the collection max.
if let Some(max) = collection.max {
ensure!(nft_id < max, Error::<T>::CollectionFullOrLocked);
ensure!(collection.nfts_count < max, Error::<T>::CollectionFullOrLocked);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a helper function that gets the Collection's NFT count?

Copy link
Contributor

Choose a reason for hiding this comment

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

pub fn get_collection_nfts_count(collection_info: CollectionInfoOf<T>) -> u32 {
		collection_info.nfts_count
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts? It's pretty simple, but we could not do this, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure actually, seems a bit unneeded abstraction of just accessing property? But happy to update if you think worth pursuing. ( Usually I would extract a function with more logic )

@HashWarlock
Copy link
Contributor

Should we think about adding BoundedVec for ResourceId and NftId now? This makes more sense now that we are not auto-incrementing the NftId and ResourceId. Do you think we should also propose this for the Uniques v2 implementation as well?

@ilionic
Copy link
Contributor Author

ilionic commented Aug 11, 2022

Should we think about adding BoundedVec for ResourceId and NftId now? This makes more sense now that we are not auto-incrementing the NftId and ResourceId. Do you think we should also propose this for the Uniques v2 implementation as well?

Do you mean using a 'stringy' version instead of u32? There was another point in favour of u32 - to keep it compatible with EVM. We also discussed id's strategy extensively with Parity Uniques devs and basically agreed to keep u32 ( just align with auto inc strategies across the board, hence there was recent update in Uniques. So we are pretty much in line now )

Copy link
Contributor

@HashWarlock HashWarlock left a comment

Choose a reason for hiding this comment

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

LGTM

@ilionic ilionic merged commit 405af24 into main Aug 11, 2022
@ilionic ilionic deleted the add-required-resource-id branch August 11, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resources should have assignable (not auto-incrementing) IDs, at least optionally

3 participants