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.

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

Mutable properties #269

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 2 additions & 2 deletions pallets/rmrk-core/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::Pallet as RmrkCore;
use frame_benchmarking::{account, benchmarks, whitelisted_caller};
use frame_support::traits::{Currency, Get};
use frame_system::RawOrigin;
use rmrk_traits::{AccountIdOrCollectionNftTuple, BasicResource};
use rmrk_traits::{property::PropertyValue, AccountIdOrCollectionNftTuple, BasicResource};
use sp_runtime::traits::Bounded;
use sp_std::vec;

Expand Down Expand Up @@ -374,7 +374,7 @@ benchmarks! {
let alice: T::AccountId = whitelisted_caller();

let key = stbk::<T>("test-key");
let value = stb::<T>("test-value");
let value = PropertyValue { mutable: true, value: stb::<T>("test-value") };

let collection_index = 1;
let collection_id = create_test_collection::<T>(alice.clone(), collection_index);
Expand Down
77 changes: 56 additions & 21 deletions pallets/rmrk-core/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use sp_runtime::{
ArithmeticError,
};

use rmrk_traits::{budget::Budget, misc::TransferHooks};
use rmrk_traits::{budget::Budget, misc::TransferHooks, property::PropertyValue};
use sp_std::collections::btree_set::BTreeSet;

// Randomness to generate NFT virtual accounts
Expand Down Expand Up @@ -46,7 +46,11 @@ impl<T: Config>
priority_index += 1;
}
Self::deposit_event(Event::PrioritySet { collection_id, nft_id });
Ok(Some(<T as pallet::Config>::WeightInfo::set_priority(priority_index, T::NestingBudget::get())).into())
Ok(Some(<T as pallet::Config>::WeightInfo::set_priority(
priority_index,
T::NestingBudget::get(),
))
.into())
}
}

Expand All @@ -58,31 +62,61 @@ impl<T: Config> Property<KeyLimitOf<T>, ValueLimitOf<T>, T::AccountId, T::Collec
collection_id: T::CollectionId,
maybe_nft_id: Option<T::ItemId>,
key: KeyLimitOf<T>,
value: ValueLimitOf<T>,
) -> DispatchResult {
value: PropertyValue<ValueLimitOf<T>>,
) -> Result<PropertyValue<ValueLimitOf<T>>, DispatchError> {
let collection =
Collections::<T>::get(&collection_id).ok_or(Error::<T>::CollectionUnknown)?;
ensure!(collection.issuer == sender, Error::<T>::NoPermission);
if let Some(nft_id) = &maybe_nft_id {
// Check NFT lock status
ensure!(
!Pallet::<T>::is_locked(collection_id, *nft_id),
pallet_uniques::Error::<T>::Locked
);
let budget = budget::Value::new(T::NestingBudget::get());
let (root_owner, _) = Pallet::<T>::lookup_root_owner(collection_id, *nft_id, &budget)?;
ensure!(root_owner == collection.issuer, Error::<T>::NoPermission);
}
Properties::<T>::insert((&collection_id, maybe_nft_id, &key), &value);
Ok(())
Collections::<T>::get(collection_id).ok_or(Error::<T>::CollectionUnknown)?;

Properties::<T>::try_mutate((&collection_id, maybe_nft_id, &key), |property| {
let new_value = if let Some(nft_id) = &maybe_nft_id {
// Check NFT lock status
ensure!(
!Pallet::<T>::is_locked(collection_id, *nft_id),
pallet_uniques::Error::<T>::Locked
);

let budget = budget::Value::new(T::NestingBudget::get());

let (root_owner, _) =
Pallet::<T>::lookup_root_owner(collection_id, *nft_id, &budget)?;

let new_value = match (property.take(), sender == collection.issuer) {
(Some(PropertyValue { mutable, .. }), false) => {
ensure!(root_owner == sender && mutable, Error::<T>::NoPermission);

PropertyValue { mutable: true, value: value.value }
},
(None, _) | (Some(_), true) => {
ensure!(
root_owner == collection.issuer && root_owner == sender,
Error::<T>::NoPermission
);

value
},
};

*property = Some(new_value.clone());

new_value
} else {
ensure!(collection.issuer == sender, Error::<T>::NoPermission);

*property = Some(value.clone());

value
};

Ok(new_value)
})
}

// Internal function to set a property for downstream `Origin::root()` calls.
fn do_set_property(
collection_id: T::CollectionId,
maybe_nft_id: Option<T::ItemId>,
key: KeyLimitOf<T>,
value: ValueLimitOf<T>,
value: PropertyValue<ValueLimitOf<T>>,
) -> sp_runtime::DispatchResult {
// Ensure collection exists
Collections::<T>::get(&collection_id).ok_or(Error::<T>::CollectionUnknown)?;
Expand Down Expand Up @@ -723,8 +757,9 @@ impl<T: Config>
ensure!(Pallet::<T>::nft_exists((collection_id, nft_id)), Error::<T>::NoAvailableNftId);

let owner_account = match new_owner.clone() {
AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid) =>
Pallet::<T>::nft_to_account_id(cid, nid),
AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid) => {
Pallet::<T>::nft_to_account_id(cid, nid)
},
AccountIdOrCollectionNftTuple::AccountId(owner_account) => owner_account,
};

Expand Down
33 changes: 23 additions & 10 deletions pallets/rmrk-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ pub mod pallet {
use super::*;
use frame_support::{dispatch::DispatchResult, pallet_prelude::*};
use frame_system::pallet_prelude::*;
use rmrk_traits::property::PropertyValue;

/// Configure the pallet by specifying the parameters and types on which it depends.
#[pallet::config]
Expand Down Expand Up @@ -258,7 +259,7 @@ pub mod pallet {
NMapKey<Blake2_128Concat, Option<T::ItemId>>,
NMapKey<Blake2_128Concat, KeyLimitOf<T>>,
),
ValueLimitOf<T>,
PropertyValue<ValueLimitOf<T>>,
OptionQuery,
>;

Expand Down Expand Up @@ -334,7 +335,7 @@ pub mod pallet {
collection_id: T::CollectionId,
maybe_nft_id: Option<T::ItemId>,
key: KeyLimitOf<T>,
value: ValueLimitOf<T>,
value: PropertyValue<ValueLimitOf<T>>,
},
PropertyRemoved {
collection_id: T::CollectionId,
Expand Down Expand Up @@ -449,7 +450,7 @@ pub mod pallet {
{
ensure!(collection_issuer == sender, Error::<T>::NoPermission);
} else {
return Err(Error::<T>::CollectionUnknown.into())
return Err(Error::<T>::CollectionUnknown.into());
}

// Extract intended owner or default to sender
Expand Down Expand Up @@ -483,7 +484,7 @@ pub mod pallet {
/// - `recipient`: Receiver of the royalty
/// - `royalty`: Permillage reward from each trade for the Recipient
/// - `metadata`: Arbitrary data about an nft, e.g. IPFS hash
#[pallet::call_index(1)]
#[pallet::call_index(1)]
#[pallet::weight(<T as pallet::Config>::WeightInfo::mint_nft_directly_to_nft(T::NestingBudget::get()))]
#[transactional]
pub fn mint_nft_directly_to_nft(
Expand All @@ -505,7 +506,7 @@ pub mod pallet {
{
ensure!(collection_issuer == sender, Error::<T>::NoPermission);
} else {
return Err(Error::<T>::CollectionUnknown.into())
return Err(Error::<T>::CollectionUnknown.into());
}

// Mint NFT for RMRK storage
Expand Down Expand Up @@ -628,8 +629,9 @@ pub mod pallet {
Some(owner) => {
let owner_account =
match Pallet::<T>::decode_nft_account_id::<T::AccountId>(owner.clone()) {
Some((cid, nid)) =>
AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid),
Some((cid, nid)) => {
AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid)
},
None => AccountIdOrCollectionNftTuple::AccountId(owner),
};
ensure!(new_owner == owner_account, Error::<T>::CannotAcceptToNewOwner)
Expand Down Expand Up @@ -708,13 +710,24 @@ pub mod pallet {
collection_id: T::CollectionId,
maybe_nft_id: Option<T::ItemId>,
key: KeyLimitOf<T>,
value: ValueLimitOf<T>,
value: PropertyValue<ValueLimitOf<T>>,
) -> DispatchResult {
let sender = ensure_signed(origin)?;

Self::property_set(sender, collection_id, maybe_nft_id, key.clone(), value.clone())?;
let actual_value = Self::property_set(
sender,
collection_id,
maybe_nft_id,
key.clone(),
value.clone(),
)?;

Self::deposit_event(Event::PropertySet { collection_id, maybe_nft_id, key, value });
Self::deposit_event(Event::PropertySet {
collection_id,
maybe_nft_id,
key,
value: actual_value,
});
Ok(())
}
/// lock collection
Expand Down
112 changes: 102 additions & 10 deletions pallets/rmrk-core/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use sp_runtime::Permill;
use super::*;
use mock::{RuntimeEvent as MockEvent, RuntimeOrigin as Origin, *};
use pallet_uniques as UNQ;
use rmrk_traits::budget::Budget;
use rmrk_traits::{budget::Budget, property::PropertyValue};
use sp_std::convert::TryInto;

type RMRKCore = Pallet<Test>;
Expand Down Expand Up @@ -1797,11 +1797,18 @@ fn set_property_works() {
ExtBuilder::build().execute_with(|| {
// Define property key
let key = stbk("test-key");
// Define property value
let value = stb("test-value");
// Define property value (immutable because collection issue should skip the check)
let value_immutable = PropertyValue { mutable: false, value: stb("test-value") };

// set_property fails without a collection (CollectionUnknown)
assert_noop!(
RMRKCore::set_property(Origin::signed(ALICE), 0, Some(0), key.clone(), value.clone()),
RMRKCore::set_property(
Origin::signed(ALICE),
0,
Some(0),
key.clone(),
value_immutable.clone()
),
Error::<Test>::CollectionUnknown
);
// Create a basic collection
Expand All @@ -1814,32 +1821,117 @@ fn set_property_works() {
0,
Some(0),
key.clone(),
value.clone()
value_immutable.clone()
));
// Successful property setting should trigger a PropertySet event
System::assert_last_event(MockEvent::RmrkCore(crate::Event::PropertySet {
collection_id: 0,
maybe_nft_id: Some(0),
key: key.clone(),
value: value.clone(),
value: value_immutable.clone(),
}));
// Property value now exists
assert_eq!(RMRKCore::properties((0, Some(0), key.clone())).unwrap(), value.clone());
assert_eq!(
RMRKCore::properties((0, Some(0), key.clone())).unwrap(),
value_immutable.clone()
);
// BOB does not own NFT so attempt to set property should fail
assert_noop!(
RMRKCore::set_property(Origin::signed(BOB), 0, Some(0), key.clone(), value.clone()),
RMRKCore::set_property(
Origin::signed(BOB),
0,
Some(0),
key.clone(),
value_immutable.clone()
),
Error::<Test>::NoPermission
);
});
}

#[test]
fn set_mutable_property_works() {
ExtBuilder::build().execute_with(|| {
// Define property key
let key = stbk("test-key");
// Define property value (immutable because collection issue should skip the check)
let value_immutable = PropertyValue { mutable: false, value: stb("test-value0") };
let value_mutable = PropertyValue { mutable: true, value: stb("test-value1") };

// Create a basic collection
assert_ok!(basic_collection());
// Mint NFT
assert_ok!(basic_mint(0));
// ALICE sets property on NFT
assert_ok!(RMRKCore::set_property(
Origin::signed(ALICE),
0,
Some(0),
key.clone(),
value_immutable.clone()
));
// Successful property setting should trigger a PropertySet event
System::assert_last_event(MockEvent::RmrkCore(crate::Event::PropertySet {
collection_id: 0,
maybe_nft_id: Some(0),
key: key.clone(),
value: value_immutable.clone(),
}));
// Property value now exists
assert_eq!(
RMRKCore::properties((0, Some(0), key.clone())).unwrap(),
value_immutable.clone()
);
// ALICE sets property on NFT
assert_ok!(RMRKCore::set_property(
Origin::signed(ALICE),
0,
Some(0),
key.clone(),
value_mutable.clone()
));
// Successful property setting should trigger a PropertySet event
System::assert_last_event(MockEvent::RmrkCore(crate::Event::PropertySet {
collection_id: 0,
maybe_nft_id: Some(0),
key: key.clone(),
value: value_mutable.clone(),
}));
// Property value now exists
assert_eq!(RMRKCore::properties((0, Some(0), key.clone())).unwrap(), value_mutable.clone());
// ALICE sends NFT to BOB
assert_ok!(RMRKCore::nft_send(ALICE, 0, 0, AccountIdOrCollectionNftTuple::AccountId(BOB)));
// BOB owns the NFT, so he should be able to change the value, but not the mutability
assert_ok!(RMRKCore::set_property(
Origin::signed(BOB),
0,
Some(0),
key.clone(),
value_immutable.clone()
));
eprintln!("last event");
arrudagates marked this conversation as resolved.
Show resolved Hide resolved
// Successful property setting should trigger a PropertySet event
System::assert_last_event(MockEvent::RmrkCore(crate::Event::PropertySet {
collection_id: 0,
maybe_nft_id: Some(0),
key: key.clone(),
value: PropertyValue { mutable: true, value: value_immutable.value.clone() },
}));
// Property value now be changed
assert_eq!(
RMRKCore::properties((0, Some(0), key.clone())).unwrap(),
PropertyValue { mutable: true, value: value_immutable.value.clone() }
);
});
}

arrudagates marked this conversation as resolved.
Show resolved Hide resolved
#[test]
fn set_property_with_internal_works() {
ExtBuilder::build().execute_with(|| {
// Define property key
let key = stbk("test-key");
// Define property value
let value = stb("test-value");
let value = PropertyValue { mutable: false, value: stb("test-value") };
// set_property fails without a collection (CollectionUnknown)
assert_noop!(
RMRKCore::do_set_property(0, Some(0), key.clone(), value.clone()),
Expand Down Expand Up @@ -1869,7 +1961,7 @@ fn remove_property_with_internal_works() {
// Define property key
let key = stbk("test-key");
// Define property value
let value = stb("test-value");
let value = PropertyValue { mutable: false, value: stb("test-value") };
// Create a basic collection
assert_ok!(basic_collection());
// Mint NFT
Expand Down
Loading