decouple equip and unequip #190
Conversation
pallets/rmrk-equip/src/functions.rs
Outdated
| pallet_uniques::Error::<T>::Locked | ||
| ); | ||
|
|
||
| let item_is_equipped = |
There was a problem hiding this comment.
Since we repeat these 2 steps, should we create a helper functions like is_nft_equipped((collection_id, nft_id), base_id, slot_id) and nft_exists(collection_id, nft_id)? Could be useful for projects downstream in the future that use RMRK pallet as well. What do you think?
There was a problem hiding this comment.
i went ahead and did this. initially i included returning as a Result, but that didn't work out, because...in the case of nft_exists, there was more action i needed to take; and in the case of is_nft_equipped, i couldn't throw an error because the two times we use it are in opposite cases, so the errors would be different (AlreadyEquipped in the case of equip, and NotEquipped in the case of unequip). But anyway, the more helper methods the better. Always appreciate the feedback and suggestions, as I'm just making it up as I go 👯
There was a problem hiding this comment.
i went ahead and did this. initially i included returning as a Result, but that didn't work out, because...in the case of nft_exists, there was more action i needed to take; and in the case of
is_nft_equipped, i couldn't throw an error because the two times we use it are in opposite cases, so the errors would be different (AlreadyEquipped in the case ofequip, and NotEquipped in the case ofunequip). But anyway, the more helper methods the better. Always appreciate the feedback and suggestions, as I'm just making it up as I go dancers
I see, I think returning a bool is the right solution & this allows for others to return the relevant error based on their downstream implementation.
|
in e138584 i also added a splitting of AlreadyEquipped error into SlotAlreadyEquipped and ItemAlreadyEquipped, as these should be distinct. also added some test for this. another quick peek would be appreciated. |
|
|
||
| // Item must exist | ||
| let item_exists = | ||
| pallet_rmrk_core::Pallet::<T>::nfts(item_collection_id, item_nft_id).is_some(); | ||
| pallet_rmrk_core::Pallet::<T>::nft_exists((item_collection_id, item_nft_id)); |
There was a problem hiding this comment.
We do what you did on line 166 for this logic here:
ensure!(pallet_rmrk_core::Pallet::<T>::nft_exists((item_collection_id, item_nft_id)), Error::<T>::ItemDoesntExist);
should fix #147,
i think it solves #185 as well.