Use EncodeLike for storages traits #3676

gui1117 · 2019-09-24T12:13:15Z

Done:

this makes storage map, linked map and else use encodelike instead of there direct argument.

Critics:

There is usage of Ref::from in map and linkedmap, with some refactor we can avoid those and derive the final key only once.
I think we could do refactoring of this in a following PR
ppl have to bound FullCodec or EncodeLike in their associated type of their module.

bkchr · 2019-09-24T13:01:45Z

srml/session/src/lib.rs

 	}

 	fn clear_key_owner(id: KeyTypeId, key_data: &[u8]) {
-		<KeyOwner<T>>::remove(DEDUP_KEY_PREFIX, &(id, key_data.to_vec()));
+		<KeyOwner<T>>::remove(&DEDUP_KEY_PREFIX, &(id, key_data.to_vec()));


Suggested change

<KeyOwner<T>>::remove(&DEDUP_KEY_PREFIX, &(id, key_data.to_vec()));

<KeyOwner<T>>::remove(&DEDUP_KEY_PREFIX, &(id, key_data));

This file is actually the reason I started with EncodeLike :D

done, and tried to catch up most of them

…age2

gui1117 · 2019-09-24T15:42:40Z

also maybe I could check for compile time regression,

EDIT: I didn't make a proper full check but seems fine

…age2

bkchr · 2019-10-01T15:40:10Z

srml/democracy/src/lib.rs

@@ -929,7 +929,7 @@ impl<T: Trait> Module<T> {
 			} else {
 				<DispatchQueue<T>>::append_or_insert(
 					now + info.delay,
-					[Some((info.proposal, index))].into_iter()
+					&[Some((info.proposal, index))][..]


I think that makes no real difference, but okay.

no it will compile with something like:

the trait bound `Iter<'_, std::option::Option<(<T as Trait>::Proposal, u32)>>: EncodeLike<std::vec::Vec<std::option::Option<(<T as Trait>::Proposal, u32)>>>` is not satisfied

hmm at some point we should derive EncodeLike for standard iterator structures.

bkchr · 2019-10-01T15:41:07Z

srml/elections/src/lib.rs

@@ -816,7 +816,7 @@ impl<T: Trait> Module<T> {
 					if set_len + 1 == VOTER_SET_SIZE {
 						NextVoterSet::put(next + 1);
 					}
-					<Voters<T>>::append_or_insert(next, [Some(who.clone())].into_iter())
+					<Voters<T>>::append_or_insert(next, &[Some(who.clone())][..])


You are still cloning.

yes because satisfying both iterator constrained and EncodeLike<Vec<T>> is hard.

srml/support/src/storage/mod.rs

bkchr · 2019-10-01T16:15:03Z

srml/support/src/storage/storage_items.rs

@@ -839,13 +840,17 @@ mod test_append_and_len {
 	#[test]
 	fn append_or_put_works() {
 		with_externalities(&mut TestExternalities::default(), || {
-			let _ = MapVec::append_or_insert(1, [1, 2, 3].iter());
-			let _ = MapVec::append_or_insert(1, [4, 5].iter());


Shouldn't this work as well?

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

kianenigma · 2019-10-01T16:30:22Z

srml/assets/src/lib.rs

@@ -159,7 +159,7 @@ decl_module! {
 			let id = Self::next_asset_id();
 			<NextAssetId<T>>::mutate(|id| *id += One::one());

-			<Balances<T>>::insert((id, origin.clone()), total);
+			<Balances<T>>::insert((id, &origin), total);


cc @shawntabrizi in v2 for kitties tutorial: the original issue of the PR references the v2 tutorial. If keys of type tuple are still around they can be refactored now

kianenigma · 2019-10-01T16:32:01Z

srml/democracy/src/lib.rs

-			let new_prop = (index, (*proposal).clone(), who);
-			<PublicProps<T>>::append_or_put([new_prop].into_iter());
+			let new_prop = (index, proposal, who);
+			<PublicProps<T>>::append_or_put(&[Ref::from(&new_prop)][..]);


If can be refactored and you think it is worth it, would be good to have a small issue for it after the PR is merged 👍

Ref from inside srml-support/storage/generator like in the implementation of linkedmap and map.
here it is legit. no super beautiful indeed.

kianenigma · 2019-10-01T16:40:57Z

srml/support/src/storage/generator/linked_map.rs

+	// Next element key in storage (None for the last element)
+	next: Option<NKey>,
+	// The key of the linkage this type encode to
+	phantom: core::marker::PhantomData<Key>,


import from rstd for consistency?

kianenigma

I will revisit this at a later point since I could not read some parts in detail; looks to be already great.

gui1117 added 3 commits September 24, 2019 13:59

impl

dcfee51

patch

a932ddd

lock

2013b81

gui1117 requested review from andresilva, Demi-Marie and kianenigma as code owners September 24, 2019 12:13

gui1117 changed the title ~~Use EncodeLike for in our storages~~ Use EncodeLike for storages traits Sep 24, 2019

gui1117 requested a review from bkchr September 24, 2019 12:15

some refactor

09035c3

gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. 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 Sep 24, 2019

bkchr reviewed Sep 24, 2019

View reviewed changes

gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 24, 2019

gui1117 added 3 commits September 24, 2019 16:06

some avoided copy

36a97a5

new api without ref for doublemap

c58fddb

fix

4fadf1b

gui1117 requested review from jimpo and pepyakin as code owners September 24, 2019 14:34

gui1117 added 2 commits September 24, 2019 16:34

Merge remote-tracking branch 'origin/master' into gui-encodelike-stor…

a06f08e

…age2

version bump

767b306

gui1117 added 4 commits September 26, 2019 11:14

Merge remote-tracking branch 'origin/master' into gui-encodelike-stor…

e05776e

…age2

fix

c71c3c1

Merge remote-tracking branch 'origin/master' into gui-encodelike-stor…

ee9c5c6

…age2

point to incoming release

4103679

gui1117 mentioned this pull request Sep 26, 2019

New release for both codec and codec-derive paritytech/parity-scale-codec#168

Merged

gui1117 added 2 commits September 27, 2019 12:47

use codec latest

1800cfe

Merge remote-tracking branch 'origin/master' into gui-encodelike-stor…

e3081fc

…age2

bumpd impl version

a22ac94

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 Sep 27, 2019

gui1117 added 4 commits September 27, 2019 14:43

Merge remote-tracking branch 'origin/master' into gui-encodelike-stor…

bdd8260

…age2

fix unused

c58027b

Merge remote-tracking branch 'origin/master' into gui-encodelike-stor…

6dfe36f

…age2

fix

c5cce62

bkchr approved these changes Oct 1, 2019

View reviewed changes

Update srml/support/src/storage/mod.rs

ab00065

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

kianenigma reviewed Oct 1, 2019

View reviewed changes

kianenigma approved these changes Oct 1, 2019

View reviewed changes

bkchr merged commit db417ff into master Oct 1, 2019

bkchr deleted the gui-encodelike-storage2 branch October 1, 2019 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use EncodeLike for storages traits #3676

Use EncodeLike for storages traits #3676

gui1117 commented Sep 24, 2019 •

edited

Loading

bkchr Sep 24, 2019

gui1117 Sep 24, 2019

gui1117 commented Sep 24, 2019 •

edited

Loading

bkchr Oct 1, 2019

gui1117 Oct 1, 2019

gui1117 Oct 1, 2019

bkchr Oct 1, 2019

gui1117 Oct 1, 2019 •

edited

Loading

bkchr Oct 1, 2019

kianenigma Oct 1, 2019

shawntabrizi Oct 1, 2019

kianenigma Oct 1, 2019

gui1117 Oct 1, 2019

kianenigma Oct 1, 2019

kianenigma left a comment

	<KeyOwner<T>>::remove(&DEDUP_KEY_PREFIX, &(id, key_data.to_vec()));
	<KeyOwner<T>>::remove(&DEDUP_KEY_PREFIX, &(id, key_data));

Use EncodeLike for storages traits #3676

Use EncodeLike for storages traits #3676

Conversation

gui1117 commented Sep 24, 2019 • edited Loading

Done:

Critics:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 commented Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

gui1117 commented Sep 24, 2019 •

edited

Loading

gui1117 commented Sep 24, 2019 •

edited

Loading

gui1117 Oct 1, 2019 •

edited

Loading