Conversation
non-fungible-token/src/lib.rs
Outdated
| impl<T: Trait> Module<T> { | ||
| /// Create NFT(non fungible token) class | ||
| pub fn create_class(owner: &T::AccountId, metadata: CID, data: T::ClassData) -> Result<T::ClassId, DispatchError> { | ||
| let class_id = Self::next_class_id(); |
There was a problem hiding this comment.
let class_id = NextClassId::<T>::try_mutate(|id| {
let currentId = *id;
*id = id.checked_add(One::one()).ok_or(Error::<T>::NoAvailableClassId);
currentId
})?;
non-fungible-token/src/lib.rs
Outdated
| info.owner = to.clone(); | ||
| } | ||
| }); | ||
| TokensByOwner::<T>::remove(from.clone(), token); |
There was a problem hiding this comment.
| TokensByOwner::<T>::remove(from.clone(), token); | |
| TokensByOwner::<T>::remove(from, token); |
the key can be a reference so never need to clone the key. take a reference if needed.
non-fungible-token/src/lib.rs
Outdated
| } | ||
| }); | ||
| TokensByOwner::<T>::remove(from.clone(), token); | ||
| TokensByOwner::<T>::insert(to.clone(), token, ()); |
There was a problem hiding this comment.
| TokensByOwner::<T>::insert(to.clone(), token, ()); | |
| TokensByOwner::<T>::insert(to, token, ()); |
non-fungible-token/src/lib.rs
Outdated
|
|
||
| /// Transfer NFT(non fungible token) from `from` account to `to` account | ||
| pub fn transfer(from: &T::AccountId, to: &T::AccountId, token: (T::ClassId, T::TokenId)) -> DispatchResult { | ||
| ensure!(Tokens::<T>::contains_key(token.0, token.1), Error::<T>::TokenNotFound); |
There was a problem hiding this comment.
should be able to combine the contains_key and mutate with try_mutate_exists, same for TokensByOwner
One example
open-runtime-module-library/auction/src/lib.rs
Lines 127 to 128 in 27d55f6
non-fungible-token/src/lib.rs
Outdated
| Ok(()) | ||
| })?; | ||
| Tokens::<T>::insert(class_id, token_id, token_info); | ||
| TokensByOwner::<T>::insert(owner.clone(), (class_id, token_id), ()); |
There was a problem hiding this comment.
| TokensByOwner::<T>::insert(owner.clone(), (class_id, token_id), ()); | |
| TokensByOwner::<T>::insert(owner, (class_id, token_id), ()); |
non-fungible-token/src/lib.rs
Outdated
| data: T::TokenData, | ||
| ) -> Result<T::TokenId, DispatchError> { | ||
| let token_id = Self::next_token_id(); | ||
| ensure!(token_id != T::TokenId::max_value(), Error::<T>::NoAvailableTokenId); |
non-fungible-token/src/lib.rs
Outdated
| if let Some(info) = class_info { | ||
| info.total_issuance = info | ||
| .total_issuance | ||
| .checked_sub(&1.into()) |
There was a problem hiding this comment.
use One::one() over 1.into(). Learn from Gav paritytech/substrate#5715 (comment)
non-fungible-token/src/lib.rs
Outdated
| } | ||
| Ok(()) | ||
| })?; | ||
| Tokens::<T>::remove(token.0, token.1); |
There was a problem hiding this comment.
Should be able to use take().ok_or(Error::<T>::TokenNotFound) to combine remove and check.
non-fungible-token/src/lib.rs
Outdated
|
|
||
| /// Destroy NFT(non fungible token) class | ||
| pub fn destroy_class(owner: &T::AccountId, class_id: T::ClassId) -> DispatchResult { | ||
| ensure!(Classes::<T>::contains_key(class_id), Error::<T>::ClassNotFound); |
There was a problem hiding this comment.
use try_mutate_exists to avoid multiple access
|
Looks good. Just needs some small optimizations and improvements. |
non-fungible-token/src/lib.rs
Outdated
| Ok(current_id) | ||
| })?; | ||
|
|
||
| Classes::<T>::try_mutate(class_id, |class_info| -> DispatchResult { |
There was a problem hiding this comment.
This could fail and NextTokenId still get updated.
Should put rest of the method into the NextTokenId::try_mutate closure
non-fungible-token/src/lib.rs
Outdated
| ensure!(token_info.take().is_some(), Error::<T>::TokenNotFound); | ||
| Ok(()) | ||
| })?; | ||
| TokensByOwner::<T>::try_mutate_exists(owner, token, |info| -> DispatchResult { |
There was a problem hiding this comment.
This could fail and Tokens still get updates.
Should put rest of the method into the Tokens:: try_mutate_exists closure
2 participants
No description provided.