Decouple Equip Logic

[HashWarlock]

Currently the Equip Pallet's equip function handles the following scenarios:

1. Removes NFT to equip if the NFT has been burned & is present in Equippings storage

• Mutates Nfts field equipped sets to false
• Removes from Equippings then triggers a SlotUnequipped

2. Removes NFT to equip if NFT is equipped and called by the root_owner or equipper_owner

• Mutates Nfts field equipped sets to false
• Removes from Equippings then triggers a SlotUnequipped

3. Equips NFT if the direct parent of NFT is the NFT to be equipped to with a valid BaseId & SlotId

• If Resource already exists in BaseId & SlotId then it will replace the previous Resource.
• If no Resource exists and valid BaseId & SlotId then the new Resource is equipped.

I recommend creating 2 separate functions:

• equip(origin, nft_to_equip: (CollectionId, NftId), parent_nft: (CollectionId, NftId), parent_resource_id: ResourceId, resource_id_to_equip: ResourceId, base: BaseId, slot: SlotId)
  • Handles equipping Resources to a NFT with open BaseId and SlotId. Instead of iterating through the Resources of the NFT to be equipped, we can pass in the ResourceId as well since UI can handle finding the right ResourceId
  • Replaces NFT equipped with the compatible ResourceId
• unequip(origin, nft_to_equip: (CollectionId, NftId), parent_nft: (CollectionId, NftId), parent_resource_id: ResourceId, resource_id_to_unequip: ResourceId, base: BaseId, slot: SlotId)
  • Handles points 1) and 2) and removes the equipped NFT

Since all the information will be on the blockchain, we can leave iterating through storage to find the right ResourceId to be equipped to the parent ResourceId w/ a compatible BaseId and SlotId to UI development to query and ensure they have correct info for parameters to be able to equip the NFT.

Another suggestion would be restructuring the ResourceInfo to have an enum ResourceType to differentiate between the different resources that a resource can be. From my understanding, there can be 3 types: Basic Media, Composable Base & Composable Parts. Separating these types can make the struct for ResourceInfo better to understand & can help with detecting the ResourceType quicker to ensure the equip functions can better handle logic.

Any thoughts on this and if there are other optimizations I may be missing?

[bmacer]

I'm happy to see your suggestion going in the direction of including the ResourceId in the equip/unequip operations. I agree that equip and unequip should be decoupled. I can work on that.

As for the enum ResourceType, this is implemented in #145, unless you disagree?

rmrk-substrate/traits/src/resource.rs

Lines 111 to 128 in 20235f9

	#[derive(Encode, Decode, Eq, PartialEq, Clone, RuntimeDebug, TypeInfo, MaxEncodedLen)]
	#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
	pub struct ResourceInfo<BoundedString, BoundedParts> {
	/// id is a 5-character string of reasonable uniqueness.
	/// The combination of base ID and resource id should be unique across the entire RMRK
	/// ecosystem which
	pub id: ResourceId,
	/// Resource
	pub resource: ResourceTypes<BoundedString, BoundedParts>,
	/// If resource is sent to non-rootowned NFT, pending will be false and need to be accepted
	pub pending: bool,
	/// If resource removal request is sent by non-rootowned NFT, pending will be true and need to
	/// be accepted
	pub pending_removal: bool,
	}

ResourceInfo becomes just a shell for allowing pending actions (even the id I think might be disposable if we accept the additional changes in commit 20235f9. But the important part is the reference to this enum ResourceTypes, which does exactly as you're describing.

rmrk-substrate/traits/src/resource.rs

Lines 103 to 109 in 20235f9

	#[derive(Encode, Decode, Eq, PartialEq, Clone, RuntimeDebug, TypeInfo, MaxEncodedLen)]
	#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
	pub enum ResourceTypes<BoundedString, BoundedParts> {
	Basic(BasicResource<BoundedString>),
	Composable(ComposableResource<BoundedString, BoundedParts>),
	Slot(SlotResource<BoundedString>),
	}

I'd recommend we consider if #145 is good to merge (especially with regards to the new commit 20235f9), then just treat this issue as a decoupling of equip/unequip, unless I'm misunderstanding your suggestion. We could also add removing ID from the struct to this issue.

[HashWarlock]

Yeah, I agree. I'll make some comments on PR if I have anymore, but I think this is a good direction and we can optimize anything else we may run into.