Skip to content
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

Jump to bottom

decouple equip and unequip #190

Merged
merged 4 commits into from
Jul 28, 2022
Merged

decouple equip and unequip #190

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
deb0b4a
decouple equip and unequip
bmacer Jun 25, 2022
e138584
add helper methods and split ItemAlreadyEquipped
bmacer Jul 8, 2022
6a440e4
Merge branch 'main' into bug/147-decouple-equip-and-unequip
bmacer Jul 8, 2022
82d3f70
Merge 'main' into decouple-equip-and-unequip
ilionic Jul 28, 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
207 changes: 132 additions & 75 deletions pallets/rmrk-equip/src/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl<T: Config>
T::PartsLimit,
>,
BoundedVec<CollectionId, T::MaxCollectionsEquippablePerPart>,
BoundedVec<ThemeProperty<BoundedVec<u8, T::StringLimit>>, T::MaxPropertiesPerTheme>
BoundedVec<ThemeProperty<BoundedVec<u8, T::StringLimit>>, T::MaxPropertiesPerTheme>,
> for Pallet<T>
where
T: pallet_uniques::Config<CollectionId = CollectionId, ItemId = NftId>,
Expand Down Expand Up @@ -111,17 +111,14 @@ where

/// Implementation of the do_equip function for the Base trait
/// Called by the equip extrinsic to equip a child NFT's resource to a parent's slot, if all are
/// available. Also can be called to unequip, which can be successful if
/// - Item has beeen burned
/// - Item is equipped and extrinsic called by equipping item owner
/// - Item is equipped and extrinsic called by equipper NFT owner
/// available.
/// Equipping operations are maintained inside the Equippings storage.
/// Modeled after [equip interaction](https://github.com/rmrk-team/rmrk-spec/blob/master/standards/rmrk2.0.0/interactions/equip.md)
///
/// Parameters:
/// - issuer: The caller of the function, not necessarily anything else
/// - item: Child NFT being equipped (or unequipped)
/// - equipper: Parent NFT which will equip (or unequip) the item
/// - item: Child NFT being equipped
/// - equipper: Parent NFT which will equip the item
/// - base_id: ID of the base which the item and equipper must each have a resource referencing
/// - slot_id: ID of the slot which the item and equipper must each have a resource referencing
fn do_equip(
Expand All @@ -131,7 +128,7 @@ where
resource_id: ResourceId,
base_id: BaseId,
slot_id: SlotId,
) -> Result<(CollectionId, NftId, BaseId, SlotId, bool), DispatchError> {
) -> Result<(CollectionId, NftId, BaseId, SlotId), DispatchError> {
let item_collection_id = item.0;
let item_nft_id = item.1;
let equipper_collection_id = equipper.0;
Expand All @@ -147,85 +144,44 @@ where
pallet_uniques::Error::<T>::Locked
);

// If the item's equipped property is true, it is already equipped
ensure!(
!pallet_rmrk_core::Pallet::<T>::nfts(item_collection_id, item_nft_id)
.unwrap()
.equipped,
Error::<T>::AlreadyEquipped
);

// If the Equippings storage contains the Base/Slot for the Collection+NFT ID, the item is
// already equipped
let item_is_equipped =
Equippings::<T>::get(((equipper_collection_id, equipper_nft_id), base_id, slot_id))
.is_some();
ensure!(!item_is_equipped, Error::<T>::AlreadyEquipped);

// Item must exist
let item_exists =
pallet_rmrk_core::Pallet::<T>::nfts(item_collection_id, item_nft_id).is_some();
ensure!(item_exists, Error::<T>::ItemDoesntExist);

// If item doesn't exist, anyone can unequip it.
if !item_exists && item_is_equipped {
// Remove from Equippings nft/base/slot storage
Equippings::<T>::remove(((equipper_collection_id, equipper_nft_id), base_id, slot_id));

// Update item's equipped property
pallet_rmrk_core::Nfts::<T>::try_mutate_exists(
item_collection_id,
item_nft_id,
|nft| -> DispatchResult {
if let Some(nft) = nft {
nft.equipped = false;
}
Ok(())
},
)?;

// Return unequip event details
return Ok((item_collection_id, item_nft_id, base_id, slot_id, false))
}
// Equipper must exist
ensure!(
pallet_rmrk_core::Pallet::<T>::nfts(equipper_collection_id, equipper_nft_id).is_some(),
Error::<T>::EquipperDoesntExist
);

// Caller must root-own item
let item_owner =
pallet_rmrk_core::Pallet::<T>::lookup_root_owner(item_collection_id, item_nft_id)?;
ensure!(item_owner.0 == issuer, Error::<T>::PermissionError);

// Caller must root-own equipper
let equipper_owner = pallet_rmrk_core::Pallet::<T>::lookup_root_owner(
equipper_collection_id,
equipper_nft_id,
)?;

// If the item is equipped in this slot, and either the equipper or the item owner is the
// caller, it will be unequipped
if item_is_equipped && (item_owner.0 == issuer || equipper_owner.0 == issuer) {
// Remove from Equippings nft/base/slot storage
Equippings::<T>::remove(((equipper_collection_id, equipper_nft_id), base_id, slot_id));

// Update item's equipped property
pallet_rmrk_core::Nfts::<T>::try_mutate_exists(
item_collection_id,
item_nft_id,
|nft| -> DispatchResult {
if let Some(nft) = nft {
nft.equipped = false;
}
Ok(())
},
)?;

// Return unequip event details
return Ok((item_collection_id, item_nft_id, base_id, slot_id, false))
}

// Equipper NFT must exist
ensure!(
pallet_rmrk_core::Pallet::<T>::nfts(equipper_collection_id, equipper_nft_id).is_some(),
Error::<T>::EquipperDoesntExist
);

// Item must exist
ensure!(item_exists, Error::<T>::ItemDoesntExist);

// Is the item equipped anywhere?
ensure!(
!pallet_rmrk_core::Pallet::<T>::nfts(item_collection_id, item_nft_id)
.unwrap()
.equipped,
Error::<T>::AlreadyEquipped
);

// Caller must root-own equipper?
ensure!(equipper_owner.0 == issuer, Error::<T>::PermissionError);

// Caller must root-own item
ensure!(item_owner.0 == issuer, Error::<T>::PermissionError);

// Equipper must be direct parent of item
let equipper_owner = pallet_rmrk_core::Pallet::<T>::nfts(item_collection_id, item_nft_id)
.unwrap()
Expand Down Expand Up @@ -268,7 +224,7 @@ where
// Part must exist
ensure!(Self::parts(base_id, slot_id).is_some(), Error::<T>::PartDoesntExist);

// Returns Result
// Check the PartType stored for this Base + Slot
match Self::parts(base_id, slot_id).unwrap() {
PartType::FixedPart(_) => {
// Part must be SlotPart type
Expand Down Expand Up @@ -302,11 +258,109 @@ where
Ok(())
},
)?;
Ok((item_collection_id, item_nft_id, base_id, slot_id, true))
Ok((item_collection_id, item_nft_id, base_id, slot_id))
},
}
}

/// Implementation of the do_unequip function for the Base trait
/// Called by the equip extrinsic to unequip a child NFT's resource from a parent's slot, if it
/// is equipped. Unequip can be successful if
/// - Item has been burned
/// - Item is equipped and extrinsic called by equipping item owner
/// - Item is equipped and extrinsic called by equipper NFT owner
/// Equipping operations are maintained inside the Equippings storage.
/// Modeled after [equip interaction](https://github.com/rmrk-team/rmrk-spec/blob/master/standards/rmrk2.0.0/interactions/equip.md)
///
/// Parameters:
/// - issuer: The caller of the function, not necessarily anything else
/// - item: Child NFT being equipped (or unequipped)
/// - equipper: Parent NFT which will equip (or unequip) the item
/// - base_id: ID of the equipped item's base
/// - slot_id: ID of the equipped item's slot
fn do_unequip(
issuer: T::AccountId,
item: (CollectionId, NftId),
equipper: (CollectionId, NftId),
base_id: BaseId,
slot_id: SlotId,
) -> Result<(CollectionId, NftId, BaseId, SlotId), DispatchError> {
let item_collection_id = item.0;
let item_nft_id = item.1;
let equipper_collection_id = equipper.0;
let equipper_nft_id = equipper.1;
// Check item NFT lock status
ensure!(
!pallet_rmrk_core::Pallet::<T>::is_locked(item_collection_id, item_nft_id),
pallet_uniques::Error::<T>::Locked
);
// Check equipper NFT lock status
ensure!(
!pallet_rmrk_core::Pallet::<T>::is_locked(equipper_collection_id, equipper_nft_id),
pallet_uniques::Error::<T>::Locked
);

let item_is_equipped =
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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 👯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in e138584

Copy link
Contributor

Choose a reason for hiding this comment

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

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 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.

Equippings::<T>::get(((equipper_collection_id, equipper_nft_id), base_id, slot_id))
.is_some();
ensure!(item_is_equipped, Error::<T>::ItemNotEquipped);

let item_exists =
pallet_rmrk_core::Pallet::<T>::nfts(item_collection_id, item_nft_id).is_some();

// If item doesn't exist, anyone can unequip it. This can happen because burn_nft can
// happen in rmrk-core, which doesn't know about rmrk-equip.
if !item_exists {
// Remove from Equippings nft/base/slot storage
Equippings::<T>::remove(((equipper_collection_id, equipper_nft_id), base_id, slot_id));

// Update item's equipped property
pallet_rmrk_core::Nfts::<T>::try_mutate_exists(
item_collection_id,
item_nft_id,
|nft| -> DispatchResult {
if let Some(nft) = nft {
nft.equipped = false;
}
Ok(())
},
)?;

// Return unequip event details
return Ok((item_collection_id, item_nft_id, base_id, slot_id))
}

let item_owner =
pallet_rmrk_core::Pallet::<T>::lookup_root_owner(item_collection_id, item_nft_id)?;
let equipper_owner = pallet_rmrk_core::Pallet::<T>::lookup_root_owner(
equipper_collection_id,
equipper_nft_id,
)?;

let issuer_owns_either_equipper_or_item =
item_owner.0 == issuer || equipper_owner.0 == issuer;
ensure!(
issuer_owns_either_equipper_or_item,
Error::<T>::UnequipperMustOwnEitherItemOrEquipper
);

// Remove from Equippings nft/base/slot storage
Equippings::<T>::remove(((equipper_collection_id, equipper_nft_id), base_id, slot_id));

// Update item's equipped property
pallet_rmrk_core::Nfts::<T>::try_mutate_exists(
item_collection_id,
item_nft_id,
|nft| -> DispatchResult {
if let Some(nft) = nft {
nft.equipped = false;
}
Ok(())
},
)?;
Ok((item_collection_id, item_nft_id, base_id, slot_id))
}

/// Implementation of the equippable function for the Base trait
/// Called by the equippable extrinsic to update the array of Collections allowed
/// to be equipped to a Base's specified Slot Part.
Expand Down Expand Up @@ -366,7 +420,10 @@ where
fn add_theme(
issuer: T::AccountId,
base_id: BaseId,
theme: Theme<BoundedVec<u8, T::StringLimit>, BoundedVec<ThemeProperty<BoundedVec<u8, T::StringLimit>>, T::MaxPropertiesPerTheme>>
theme: Theme<
BoundedVec<u8, T::StringLimit>,
BoundedVec<ThemeProperty<BoundedVec<u8, T::StringLimit>>, T::MaxPropertiesPerTheme>,
>,
) -> Result<(), DispatchError> {
// Base must exist
ensure!(Bases::<T>::get(base_id).is_some(), Error::<T>::BaseDoesntExist);
Expand Down
81 changes: 55 additions & 26 deletions pallets/rmrk-equip/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ pub type BoundedThemeOf<T> = Theme<
BoundedVec<u8, <T as pallet_uniques::Config>::StringLimit>,
BoundedVec<
ThemeProperty<BoundedVec<u8, <T as pallet_uniques::Config>::StringLimit>>,
<T as Config>::MaxPropertiesPerTheme>
>;
<T as Config>::MaxPropertiesPerTheme,
>,
>;

#[frame_support::pallet]
pub mod pallet {
Expand Down Expand Up @@ -220,6 +221,10 @@ pub mod pallet {
// Attempting to define more Properties than capacity allows
// TODO confirm this is being used (after https://github.com/rmrk-team/rmrk-substrate/pull/95)
TooManyProperties,
// Cannot unequip an item that isn't equipped yet
ItemNotEquipped,
// Cannot unequip an item when caller owns neither the item nor equipper
UnequipperMustOwnEitherItemOrEquipper,
}

#[pallet::call]
Expand Down Expand Up @@ -256,17 +261,13 @@ pub mod pallet {
Ok(())
}
/// Equips a child NFT's resource to a parent's slot, if all are available.
/// Also can be called to unequip, which can be successful if
/// - Item has beeen burned
/// - Item is equipped and extrinsic called by equipping item owner
/// - Item is equipped and extrinsic called by equipper NFT owner
/// Equipping operations are maintained inside the Equippings storage.
/// Modeled after [equip interaction](https://github.com/rmrk-team/rmrk-spec/blob/master/standards/rmrk2.0.0/interactions/equip.md)
///
/// Parameters:
/// - origin: The caller of the function, not necessarily anything else
/// - item: Child NFT being equipped (or unequipped)
/// - equipper: Parent NFT which will equip (or unequip) the item
/// - item: Child NFT being equipped
/// - equipper: Parent NFT which will equip the item
/// - base: ID of the base which the item and equipper must each have a resource referencing
/// - slot: ID of the slot which the item and equipper must each have a resource referencing
#[pallet::weight(10_000 + T::DbWeight::get().reads_writes(1,1))]
Expand All @@ -280,26 +281,54 @@ pub mod pallet {
) -> DispatchResult {
let sender = ensure_signed(origin)?;

let (collection_id, nft_id, base_id, slot_id, equipped) =
let (collection_id, nft_id, base_id, slot_id) =
Self::do_equip(sender, item, equipper, resource_id, base, slot)?;

if equipped {
// Send Equip event
Self::deposit_event(Event::SlotEquipped {
item_collection: collection_id,
item_nft: nft_id,
base_id,
slot_id,
});
} else {
// Send Unequip event
Self::deposit_event(Event::SlotUnequipped {
item_collection: collection_id,
item_nft: nft_id,
base_id,
slot_id,
});
}
// Send Equip event
Self::deposit_event(Event::SlotEquipped {
item_collection: collection_id,
item_nft: nft_id,
base_id,
slot_id,
});

Ok(())
}

/// Unequips a child NFT's resource from its parent's slot.
/// Can be successful if
/// - Item has beeen burned
/// - Item is equipped and extrinsic called by equipping item owner
/// - Item is equipped and extrinsic called by equipper NFT owner
/// Equipping operations are maintained inside the Equippings storage.
/// Modeled after [equip interaction](https://github.com/rmrk-team/rmrk-spec/blob/master/standards/rmrk2.0.0/interactions/equip.md)
///
/// Parameters:
/// - origin: The caller of the function, not necessarily anything else
/// - item: Child NFT being unequipped
/// - unequipper: Parent NFT which will unequip the item
/// - base: ID of the base which the item and equipper must each have a resource referencing
/// - slot: ID of the slot which the item and equipper must each have a resource referencing
#[pallet::weight(10_000 + T::DbWeight::get().reads_writes(1,1))]
pub fn unequip(
origin: OriginFor<T>,
item: (CollectionId, NftId),
unequipper: (CollectionId, NftId),
base: BaseId,
slot: SlotId,
) -> DispatchResult {
let sender = ensure_signed(origin)?;

let (collection_id, nft_id, base_id, slot_id) =
Self::do_unequip(sender, item, unequipper, base, slot)?;

Self::deposit_event(Event::SlotUnequipped {
item_collection: collection_id,
item_nft: nft_id,
base_id,
slot_id,
});

Ok(())
}

Expand Down
Loading