Skip to content

Make resource_id / nft_id required #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ilionic merged 9 commits into from
Aug 11, 2022
Merged

Make resource_id / nft_id required #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a0318a0
make resource id required
ilionic Aug 4, 2022
5de41cc
make nft_id required, don't auto increment
ilionic Aug 5, 2022
609befa
Merge branch 'main' into add-required-resource-id
ilionic Aug 5, 2022
3fac4f5
compatibility updates
ilionic Aug 5, 2022
ae1b9e9
cleanup unused next id storages / logic
ilionic Aug 5, 2022
7a17ac6
use nft_exists fn, add a test
ilionic Aug 5, 2022
e08f452
fix integration tests
ilionic Aug 5, 2022
e004e3e
fix resources handling in integration tests
ilionic Aug 5, 2022
d5c290d
integration tests: use unique ids for mint nft
ilionic Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,016 changes: 449 additions & 567 deletions Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

64 changes: 33 additions & 31 deletions pallets/rmrk-core/src/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,14 @@ where
nft_id: NftId,
resource: ResourceTypes<BoundedVec<u8, T::StringLimit>, BoundedVec<PartId, T::PartsLimit>>,
adding_on_mint: bool,
resource_id: ResourceId,
) -> Result<ResourceId, DispatchError> {
ensure!(
Resources::<T>::get((collection_id, nft_id, resource_id)).is_none(),
Error::<T>::ResourceAlreadyExists
);

let collection = Self::collections(collection_id).ok_or(Error::<T>::CollectionUnknown)?;
let resource_id = Self::get_next_resource_id(collection_id, nft_id)?;

ensure!(collection.issuer == sender, Error::<T>::NoPermission);
let (root_owner, _) = Pallet::<T>::lookup_root_owner(collection_id, nft_id)?;
Expand Down Expand Up @@ -321,7 +326,7 @@ where
}
}

impl<T: Config> Nft<T::AccountId, StringLimitOf<T>, BoundedResourceTypeOf<T>> for Pallet<T>
impl<T: Config> Nft<T::AccountId, StringLimitOf<T>, BoundedResourceInfoTypeOf<T>> for Pallet<T>
where
T: pallet_uniques::Config<CollectionId = CollectionId, ItemId = NftId>,
{
Expand All @@ -330,19 +335,20 @@ where
fn nft_mint(
sender: T::AccountId,
owner: T::AccountId,
nft_id: NftId,
collection_id: CollectionId,
royalty_recipient: Option<T::AccountId>,
royalty_amount: Option<Permill>,
metadata: StringLimitOf<T>,
transferable: bool,
resources: Option<BoundedResourceTypeOf<T>>,
resources: Option<BoundedResourceInfoTypeOf<T>>,
) -> sp_std::result::Result<(CollectionId, NftId), DispatchError> {
let nft_id = Self::get_next_nft_id(collection_id)?;
ensure!(!Self::nft_exists((collection_id, nft_id)), Error::<T>::NftAlreadyExists);
let collection = Self::collections(collection_id).ok_or(Error::<T>::CollectionUnknown)?;

// Prevent minting when next NFT id is greater than the collection max.
// 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);
}

// NFT should be pending if minting to another account
Expand Down Expand Up @@ -389,7 +395,14 @@ where
// Add all at-mint resources
if let Some(resources) = resources {
for res in resources {
Self::resource_add(sender.clone(), collection_id, nft_id, res, true)?;
Self::resource_add(
sender.clone(),
collection_id,
nft_id,
res.resource,
true,
res.id,
)?;
}
}

Expand All @@ -405,19 +418,20 @@ where
fn nft_mint_directly_to_nft(
sender: T::AccountId,
owner: (CollectionId, NftId),
nft_id: NftId,
collection_id: CollectionId,
royalty_recipient: Option<T::AccountId>,
royalty_amount: Option<Permill>,
metadata: StringLimitOf<T>,
transferable: bool,
resources: Option<BoundedResourceTypeOf<T>>,
resources: Option<BoundedResourceInfoTypeOf<T>>,
) -> sp_std::result::Result<(CollectionId, NftId), DispatchError> {
let nft_id = Self::get_next_nft_id(collection_id)?;
ensure!(!Self::nft_exists((collection_id, nft_id)), Error::<T>::NftAlreadyExists);
let collection = Self::collections(collection_id).ok_or(Error::<T>::CollectionUnknown)?;

// Prevent minting when next NFT id is greater than the collection max.
// 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

@HashWarlock HashWarlock Aug 6, 2022

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

@HashWarlock HashWarlock Aug 9, 2022

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

@HashWarlock HashWarlock Aug 9, 2022

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

@ilionic ilionic Aug 11, 2022

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 )

}

// Calculate the rootowner of the intended owner of the minted NFT
Expand Down Expand Up @@ -468,7 +482,14 @@ where
// Add all at-mint resources
if let Some(resources) = resources {
for res in resources {
Self::resource_add(sender.clone(), collection_id, nft_id, res, true)?;
Self::resource_add(
sender.clone(),
collection_id,
nft_id,
res.resource,
true,
res.id,
)?;
}
}

Expand Down Expand Up @@ -881,25 +902,6 @@ where
}
}

pub fn get_next_nft_id(collection_id: CollectionId) -> Result<NftId, Error<T>> {
NextNftId::<T>::try_mutate(collection_id, |id| {
let current_id = *id;
*id = id.checked_add(1).ok_or(Error::<T>::NoAvailableNftId)?;
Ok(current_id)
})
}

pub fn get_next_resource_id(
collection_id: CollectionId,
nft_id: NftId,
) -> Result<ResourceId, Error<T>> {
NextResourceId::<T>::try_mutate(collection_id, nft_id, |id| {
let current_id = *id;
*id = id.checked_add(1).ok_or(Error::<T>::NoAvailableResourceId)?;
Ok(current_id)
})
}

pub fn set_lock(nft: (CollectionId, NftId), lock_status: bool) -> bool {
Lock::<T>::mutate(nft, |lock| {
*lock = lock_status;
Expand Down
48 changes: 25 additions & 23 deletions pallets/rmrk-core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use sp_std::convert::TryInto;
use rmrk_traits::{
primitives::*, AccountIdOrCollectionNftTuple, BasicResource, Collection, CollectionInfo,
ComposableResource, Nft, NftChild, NftInfo, PhantomType, Priority, Property, PropertyInfo,
Resource, ResourceInfo, ResourceTypes, RoyaltyInfo, SlotResource,
Resource, ResourceInfo, ResourceInfoMin, ResourceTypes, RoyaltyInfo, SlotResource,
};
use sp_std::result::Result;

Expand Down Expand Up @@ -63,6 +63,14 @@ pub type BoundedResourceTypeOf<T> = BoundedVec<
<T as Config>::MaxResourcesOnMint,
>;

pub type BoundedResourceInfoTypeOf<T> = BoundedVec<
ResourceInfoMin<
BoundedVec<u8, <T as pallet_uniques::Config>::StringLimit>,
BoundedVec<PartId, <T as Config>::PartsLimit>,
>,
<T as Config>::MaxResourcesOnMint,
>;

pub type PropertyInfoOf<T> = PropertyInfo<KeyLimitOf<T>, ValueLimitOf<T>>;

pub mod types;
Expand Down Expand Up @@ -101,27 +109,10 @@ pub mod pallet {
type MaxResourcesOnMint: Get<u32>;
}

#[pallet::storage]
#[pallet::getter(fn next_nft_id)]
pub type NextNftId<T: Config> = StorageMap<_, Twox64Concat, CollectionId, NftId, ValueQuery>;

#[pallet::storage]
#[pallet::getter(fn collection_index)]
pub type CollectionIndex<T: Config> = StorageValue<_, CollectionId, ValueQuery>;

/// Next available Resource ID.
#[pallet::storage]
#[pallet::getter(fn next_resource_id)]
pub type NextResourceId<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
CollectionId,
Twox64Concat,
NftId,
ResourceId,
ValueQuery,
>;

#[pallet::storage]
#[pallet::getter(fn collections)]
/// Stores collections info
Expand Down Expand Up @@ -348,6 +339,7 @@ pub mod pallet {
CollectionFullOrLocked,
CannotSendToDescendentOrSelf,
ResourceAlreadyExists,
NftAlreadyExists,
EmptyResource,
TooManyRecursions,
NftIsLocked,
Expand Down Expand Up @@ -382,12 +374,13 @@ pub mod pallet {
pub fn mint_nft(
origin: OriginFor<T>,
owner: Option<T::AccountId>,
nft_id: NftId,
collection_id: CollectionId,
royalty_recipient: Option<T::AccountId>,
royalty: Option<Permill>,
metadata: BoundedVec<u8, T::StringLimit>,
transferable: bool,
resources: Option<BoundedResourceTypeOf<T>>,
resources: Option<BoundedResourceInfoTypeOf<T>>,
) -> DispatchResult {
let sender = ensure_signed(origin)?;
if let Some(collection_issuer) =
Expand All @@ -408,6 +401,7 @@ pub mod pallet {
Self::nft_mint(
sender,
nft_owner,
nft_id,
collection_id,
royalty_recipient,
royalty,
Expand All @@ -433,12 +427,13 @@ pub mod pallet {
pub fn mint_nft_directly_to_nft(
origin: OriginFor<T>,
owner: (CollectionId, NftId),
nft_id: NftId,
collection_id: CollectionId,
royalty_recipient: Option<T::AccountId>,
royalty: Option<Permill>,
metadata: BoundedVec<u8, T::StringLimit>,
transferable: bool,
resources: Option<BoundedResourceTypeOf<T>>,
resources: Option<BoundedResourceInfoTypeOf<T>>,
) -> DispatchResult {
let sender = ensure_signed(origin.clone())?;

Expand All @@ -455,6 +450,7 @@ pub mod pallet {
Self::nft_mint_directly_to_nft(
sender,
owner,
nft_id,
collection_id,
royalty_recipient,
royalty,
Expand Down Expand Up @@ -702,15 +698,17 @@ pub mod pallet {
collection_id: CollectionId,
nft_id: NftId,
resource: BasicResource<StringLimitOf<T>>,
resource_id: ResourceId,
) -> DispatchResult {
let sender = ensure_signed(origin)?;

let resource_id = Self::resource_add(
Self::resource_add(
sender,
collection_id,
nft_id,
ResourceTypes::Basic(resource),
false,
resource_id,
)?;

Self::deposit_event(Event::ResourceAdded { nft_id, resource_id });
Expand All @@ -725,15 +723,17 @@ pub mod pallet {
collection_id: CollectionId,
nft_id: NftId,
resource: ComposableResource<StringLimitOf<T>, BoundedVec<PartId, T::PartsLimit>>,
resource_id: ResourceId,
) -> DispatchResult {
let sender = ensure_signed(origin)?;

let resource_id = Self::resource_add(
Self::resource_add(
sender,
collection_id,
nft_id,
ResourceTypes::Composable(resource),
false,
resource_id,
)?;

Self::deposit_event(Event::ResourceAdded { nft_id, resource_id });
Expand All @@ -748,15 +748,17 @@ pub mod pallet {
collection_id: CollectionId,
nft_id: NftId,
resource: SlotResource<StringLimitOf<T>>,
resource_id: ResourceId,
) -> DispatchResult {
let sender = ensure_signed(origin)?;

let resource_id = Self::resource_add(
Self::resource_add(
sender,
collection_id,
nft_id,
ResourceTypes::Slot(resource),
false,
resource_id,
)?;

Self::deposit_event(Event::ResourceAdded { nft_id, resource_id });
Expand Down
Loading