Implement InspectEnumerable for Uniques

[apopiak]

This PR implements the InspectEnumerable trait for the Uniques pallet, thus allowing on-chain iteration of e.g. a user's non-fungible assets.

[apopiak]

cc @Lohann

[shawntabrizi]

Not for this PR, but maybe we should think about an iter_keys function which would be slightly more optimal when we dont care about the value and dont need to read or decode it.

[apopiak]

I've thought the same, I'll create an issue.

[Lohann]

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

[KiChjang]

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.

[Lohann]

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.

[KiChjang]

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.

[Lohann]

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.

[KiChjang]

bot merge

[ghost]

Waiting for commit status.