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

Merge IterableStorageMap into StorageMap and IterableStorageDoubleMap into StorageDoubleMap #5335

Closed
wants to merge 10 commits into from

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Mar 20, 2020

FIRST NOTE

to make this PR easiy to review, I'll split it into multiple small ones:
1 - make iteration consistent and translate consistent #5470
2 - refactor all iterators to make use of a single PrefixIterator
3 - refactor all traits to only StorageMap and StorageDoubleMap

4 - add iter to StorageDoubleMap which decodes key when both hasher are reversible #5504 (merge)
(considering the current iter will be renamed to iter_prefix or iter_sub). This point can be made after or before reorganization.

Context

for now DoubleMap allows to iterate with the keys only when iterating over a prefix (i.e over a first key).
This PR adds iter on all key1, key2, value for DoubleMap.

This PR also reorganize storage traits to include everything in one Traits, this allow to see all available functionality easily.
It also remove the duplicate of IterableDoubleMap::translate and PrefixedStorage::translate which were both available for a double_map but with a slightly different behavior.

DONE

  • StoragePrefixedMap is removed and merged into StorageDoubleMap and StorageMap.
    so fn iter_values fn translate_values fn remove_all are now available in both traits.
    (for doublemap translate_values is actually removed in favor of IterableDoubleMap::translate which has been renamed to translate_values, see consistency section)

  • IterableStorageMap is removed and merged into StorageMap: so iter drain and translate are now in StorageMap under some condition on the hasher.

  • IterableStorageDoubleMap is removed and merged into StorageDoubleMap:

    • iter is renamed iter_prefix to be consistent with other function name (though we can discuss changing it to iter_sub and rename remove_prefix to remove_sub etc..)
    • drain is renamed drain_prefix for same reason
    • translate is renamed to translate_values see consistency section
  • behavior of iter_values has changed as it no longer stop on the first value that failed to decode, instead if continues until the last value inside the given prefix.

TODO

  • change map::translate_values behavior to be consistent with map::translate (in later PR)
  • maybe change name *_prefix to *_sub in double_map

Note

I think the biggest advantages are:

  • method are easy to find as they are all in one trait
  • adding a method under some constraint about the reverse hasher is very easy you just have to add a where clause on your new method.

Traits consistency

  • now StorageDoubleMap has:

    • iter and iter_prefix available only when both hasher are reversible
    • iter_values (previously in PrefixedMap) and iter_prefix_values always available. (we can remove if useless)
    • translate_values always available
    • no translate yet, we can implement it if needed.
  • now StorageMap has:

    • iter and iter_prefix and translate available only when both hasher are reversible
    • iter_values (previously in PrefixedMap) and iter_prefix_values and translate_values always available. (we can remove if useless)
    • translate and translate_values have slightly different signature and behave differently when some keys fail to decode. I think I'll fix it in a following PR.

@gui1117 gui1117 changed the title extend DoubleMap iterations extend DoubleMap iterations and merge all functionality into StorageDoubleMap and StorageMap Mar 20, 2020
@gui1117 gui1117 marked this pull request as ready for review March 23, 2020 11:25
@gui1117 gui1117 requested a review from kianenigma as a code owner March 23, 2020 11:25
@gavofyork
Copy link
Member

I really don't like this PR. The point of having an IterableStorageMap is to ensure that only the hasher types whose keys are actually iterable have a .iter() function. This PR actually works against Rust's type safety and tries to cover up the fundamentally different nature of hashers making it seem as though both are iterable but then giving a useless key when its not. This is a dumb API.

@gavofyork gavofyork closed this Mar 23, 2020
@bkchr bkchr deleted the gui-doublemap branch March 23, 2020 12:57
@gui1117
Copy link
Contributor Author

gui1117 commented Mar 23, 2020

I reimplemented without making use of StorageHasherInfo.

I'm still unsure why such traits is agains't rust principle: the trait basically implement what kind of information the encoding of a key using the hasher has.
Both kind of hasher can implement such decode_hash_and_then_info they are not different in this regard. even if they are still different in the kind of info they can provide from the hash.

/// Trait to retrieve some info from hash of type `Key` encoded.
pub trait StorageHasherInfo<Key> {
	/// Some info contained in the hash of type `Key` encoded.
	type Info;

	/// Decode the hash and then decode the info from the decoded hash.
	///
	/// # WARNING
	///
	/// Even if info is (), input must be modified to have read the entire encoded hash.
	fn decode_hash_and_then_info<I: codec::Input>(input: &mut I)
		-> Result<Self::Info, codec::Error>;
}

But I removed it and introduced FixedLengthHasher instead.

Note that we could generalize FixedLengthHasher into SkipHasher<Key> which would skip the hashed key by by skipping the hash part and then decoding the key to skip it (or by adding features to codec something equivalent to be able to skip it).
Such generalization would allow

DoubleMap: doublemap hasher(reversible) u32, hasher(reversible) u32 => u32

to be able to call iter_key2_value() (currently you can only call iter_key1_value() or iter_key1_key2_value() as the second hasher doesn't implement FixedLengthHasher)

it is still in progress I want to verify this design but this is the direction.

@gui1117 gui1117 reopened this Mar 23, 2020
@gui1117 gui1117 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 23, 2020
@gui1117
Copy link
Contributor Author

gui1117 commented Mar 23, 2020

This PR actually works against Rust's type safety and tries to cover up the fundamentally different nature of hashers making it seem as though both are iterable but then giving a useless key when its not

Also the traits StorageMap and StorageDoubleMap didn't expose StorageHasherInfo::Info in their API, they just make use of it in the where clause in order to make sure that the iter_key1_value() is only callable when the first hasher is reversible.

So the iter_key1_key2_value function was only available when both hasher were reversibe. Same as before.

} else {
Err(errors)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function has the old signature and implementation of PrefixedStorage::translate_values.
Change is planned in a following PR

@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 25, 2020
@gui1117
Copy link
Contributor Author

gui1117 commented Mar 25, 2020

I removed the superfluous iter functions, now the only new function introduced is StorageDoubleMap::iter which iter on all (key1, key2, values), this function is callable only if both hasher are reversible.

Additionnally I updated the top message for clarification about the reorganization and consistency.

This is ready for review

@gui1117 gui1117 added A1-onice and removed A0-please_review Pull request needs code review. labels Apr 15, 2020
@gui1117 gui1117 changed the title extend DoubleMap iterations and merge all functionality into StorageDoubleMap and StorageMap Merge IterableStorageMap into StorageMap and IterableStorageDoubleMap into StorageDoubleMap Apr 15, 2020
@gnunicorn
Copy link
Contributor

closing because of inactivity.

@gnunicorn gnunicorn closed this Sep 9, 2020
@gui1117
Copy link
Contributor Author

gui1117 commented Sep 9, 2020

@shawntabrizi I guess we can do this reorganization later for me it allow more discoverability.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants