Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement InspectEnumerable for Uniques #9117

Merged
6 commits merged into from
Jul 1, 2021
Merged

Implement InspectEnumerable for Uniques #9117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
589e28f
implement InspectEnumerable in pallet_uniques
apopiak Jun 15, 2021
8692664
Merge branch 'master' into apopiak/impl-enumerable
shawntabrizi Jul 1, 2021
e6b7a27
use `iter_keys` and `iter_key_prefix`
shawntabrizi Jul 1, 2021
8bdd9e1
return an iterator instead of constructing a vec
shawntabrizi Jul 1, 2021
ce84d9b
update comments
shawntabrizi Jul 1, 2021
1d31ab9
additional warning about storage reads
shawntabrizi Jul 1, 2021
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
14 changes: 6 additions & 8 deletions frame/support/src/traits/tokens/nonfungible.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,12 @@ pub trait Inspect<AccountId> {

/// Interface for enumerating assets in existence or owned by a given account over a collection
/// of NFTs.
///
/// WARNING: These may be a heavy operations. Do not use when execution time is limited.
pub trait InspectEnumerable<AccountId>: Inspect<AccountId> {
/// Returns the instances of an asset `class` in existence.
fn instances() -> Vec<Self::InstanceId>;
/// Returns an iterator of the instances of an asset `class` in existence.
fn instances() -> Box<dyn Iterator<Item = Self::InstanceId>>;

/// Returns the asset instances of all classes owned by `who`.
fn owned(who: &AccountId) -> Vec<Self::InstanceId>;
/// Returns an iterator of the asset instances of all classes owned by `who`.
fn owned(who: &AccountId) -> Box<dyn Iterator<Item = Self::InstanceId>>;
}

/// Trait for providing an interface for NFT-like assets which may be minted, burned and/or have
Expand Down Expand Up @@ -148,10 +146,10 @@ impl<
A: Get<<F as nonfungibles::Inspect<AccountId>>::ClassId>,
AccountId,
> InspectEnumerable<AccountId> for ItemOf<F, A, AccountId> {
fn instances() -> Vec<Self::InstanceId> {
fn instances() -> Box<dyn Iterator<Item = Self::InstanceId>> {
<F as nonfungibles::InspectEnumerable<AccountId>>::instances(&A::get())
}
fn owned(who: &AccountId) -> Vec<Self::InstanceId> {
fn owned(who: &AccountId) -> Box<dyn Iterator<Item = Self::InstanceId>> {
<F as nonfungibles::InspectEnumerable<AccountId>>::owned_in_class(&A::get(), who)
}
}
Expand Down
18 changes: 8 additions & 10 deletions frame/support/src/traits/tokens/nonfungibles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,20 +95,18 @@ pub trait Inspect<AccountId> {

/// Interface for enumerating assets in existence or owned by a given account over many collections
/// of NFTs.
///
/// WARNING: These may be a heavy operations. Do not use when execution time is limited.
pub trait InspectEnumerable<AccountId>: Inspect<AccountId> {
/// Returns the asset classes in existence.
fn classes() -> Vec<Self::ClassId>;
/// Returns an iterator of the asset classes in existence.
fn classes() -> Box<dyn Iterator<Item = Self::ClassId>>;

/// Returns the instances of an asset `class` in existence.
fn instances(class: &Self::ClassId) -> Vec<Self::InstanceId>;
/// Returns an iterator of the instances of an asset `class` in existence.
fn instances(class: &Self::ClassId) -> Box<dyn Iterator<Item = Self::InstanceId>>;

/// Returns the asset instances of all classes owned by `who`.
fn owned(who: &AccountId) -> Vec<(Self::ClassId, Self::InstanceId)>;
/// Returns an iterator of the asset instances of all classes owned by `who`.
fn owned(who: &AccountId) -> Box<dyn Iterator<Item = (Self::ClassId, Self::InstanceId)>>;

/// Returns the asset instances of `class` owned by `who`.
fn owned_in_class(class: &Self::ClassId, who: &AccountId) -> Vec<Self::InstanceId>;
/// Returns an iterator of the asset instances of `class` owned by `who`.
fn owned_in_class(class: &Self::ClassId, who: &AccountId) -> Box<dyn Iterator<Item = Self::InstanceId>>;
}

/// Trait for providing an interface for multiple classes of NFT-like assets which may be minted,
Expand Down
32 changes: 31 additions & 1 deletion frame/uniques/src/impl_nonfungibles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

use super::*;
use sp_std::convert::TryFrom;
use frame_support::traits::tokens::nonfungibles::{Inspect, Mutate, Transfer};
use frame_support::traits::tokens::nonfungibles::{Inspect, InspectEnumerable, Mutate, Transfer};
use frame_support::BoundedSlice;
use sp_runtime::DispatchResult;

Expand Down Expand Up @@ -106,3 +106,33 @@ impl<T: Config<I>, I: 'static> Transfer<T::AccountId> for Pallet<T, I> {
Self::do_transfer(class.clone(), instance.clone(), destination.clone(), |_, _| Ok(()))
}
}

impl<T: Config<I>, I: 'static> InspectEnumerable<T::AccountId> for Pallet<T, I> {
/// Returns an iterator of the asset classes in existence.
///
/// NOTE: iterating this list invokes a storage read per item.
fn classes() -> Box<dyn Iterator<Item = Self::ClassId>> {
Box::new(ClassMetadataOf::<T, I>::iter_keys())
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!! I didn't know that was possible to Box an Iterator

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this is a trait object. One day if Rust supports impl Trait in trait method return types, we'd use that here instead, but until then, Box<dyn Trait> is the only workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering if is secure (or possible) to use this iterator as a pagination for multi-step migration, ex: migrate all uniques using on_initialize hook but without repeating the same key.. but I don't think it is possible.

Copy link
Contributor

@KiChjang KiChjang Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Entirely possible. The Iterator API has a rich set of operations that allow you to do a lot of things. In this particular case, I am looking at the take method which allows me to iterate and return the first n elements that I choose.

So since the storage API ultimately relies on sp_io::storage::next_key to iterate over to the next key, and that the way it works is by passing the previous storage key as an argument, what you can do is take say 10 keys in on_initialize, and then grab the previous key from the iterator and store it, so that on the next block's on_initialize, you fetch the previous key and recreate an iterator from it.

I know this is a little bit hard to digest, but it's really easy to understand if I write some sample code:

impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
	fn on_initialize(_: T::BlockNumber) -> Weight {
		if start_of_migration() {
			let mut iterator = <Self as InspectEnumberable<T::AccountId>>::classes();
			let classes = iterator.take(10);
			let previous_key = iterator.previous_key;
			store_iteration_key(previous_key);

			do_something_with_classes(classes);
		} else {
			let previous_key = fetch_iteration_key();
			let mut iterator = create_iterator_from_key(previous_key);
			let classes = iterator.take(10);

			do_something_with_classes(classes);
		}
	}
}

The caveat that I can think of here is about storage. We can't really allow anything to do a DB write while migration is in progress, otherwise it might change the storage root and hence invalidate our previous key. This also entails that we can't store the previous key into storage either, and I can think of 2 ways to work around this limitation: 1) use in-memory storage, but it's a bit unclear to me how that works; or 2) use off-chain storage.

Copy link
Contributor

@Lohann Lohann Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Genius! I have no idea how memory storage works either, but if is possible to persist the iterator, theoretically is possible to keep the storage keys write/delete working by including some extra logic before each StorageMap::remove operation, ex:

Example using uniques/src/lib.rs

pub fn destroy(
	origin: OriginFor<T>,
	#[pallet::compact] class: T::ClassId,
	witness: DestroyWitness,
) -> DispatchResult {
		// snip ...
		let previous_key = fetch_iteration_key();
		if let Some(previous_class) = previous_key {
			if previous_class == class {
				// skip current key
				let mut iterator = create_iterator_from_key(previous_class);
				store_iteration_key(iterator.next());
			}
		}
		ClassMetadataOf::<T, I>::remove(&class);
		// snip ...
	})
}

Of course it increases the weight of each operation, but the other alternative I see is migrate everything to another storage.. which doesn't seems interesting either as it duplicates every existing unique.

}

/// Returns an iterator of the instances of an asset `class` in existence.
///
/// NOTE: iterating this list invokes a storage read per item.
fn instances(class: &Self::ClassId) -> Box<dyn Iterator<Item = Self::InstanceId>> {
Box::new(InstanceMetadataOf::<T, I>::iter_key_prefix(class))
}

/// Returns an iterator of the asset instances of all classes owned by `who`.
///
/// NOTE: iterating this list invokes a storage read per item.
fn owned(who: &T::AccountId) -> Box<dyn Iterator<Item = (Self::ClassId, Self::InstanceId)>> {
Box::new(Account::<T, I>::iter_key_prefix((who,)))
}

/// Returns an iterator of the asset instances of `class` owned by `who`.
///
/// NOTE: iterating this list invokes a storage read per item.
fn owned_in_class(class: &Self::ClassId, who: &T::AccountId) -> Box<dyn Iterator<Item = Self::InstanceId>> {
Box::new(Account::<T, I>::iter_key_prefix((who, class)))
}
}