Implement a proper generic resolution in `decl_storage!` by bkchr · Pull Request #2913 · paritytech/substrate

bkchr · 2019-06-20T08:28:45Z

Until now, we are required to add the generic type to types generated by decl_storage! that do not require it.
The worst outcome of this is the requirement of _genesis_phantom_data in GenesisConfig.

All of this is now fixed by this pr. We do not require anymore a phantom field in a GenesisConfig, as we do a proper lookup if a generic type is required.

Fixes: #2219

* remove default hash, introduce twox_128 and blake2 * use blake2_128 & create ext_blake2_128 * refactor code * add benchmark * factorize generator * fix * parameterizable hasher * some fix * fix * fix * fix * metadata * fix * remove debug print * map -> blake2_256 * fix test * fix test * Apply suggestions from code review Co-Authored-By: thiolliere <gui.thiolliere@gmail.com> * impl twox 128 concat (#2353) * impl twox_128_concat * comment addressed * fix * impl twox_128->64_concat * fix test

…stuff

gui1117 · 2019-06-20T13:41:20Z

as a note the API change in the following way:

storage structs that doesn't need generics are now not generic: <CandidateCount<Test>>::put(1); is now CandidateCount::put(1); for some storages this breaks homogeneity of storages but make more sense with the underlying implementation.
GenesisConfig structs that doesn't need generics are now not generic as well, same as above this break homogeneity in favor of actual implementation
GenesisConfig no longer implements BuildStorage trait but only some methods which signatures differs depending on if GenesisConfig is generic of not:
system::GenesisConfig::<Test>::default().build_storage().unwrap().0.into() is now system::GenesisConfig::default().build_storage::<Test>().unwrap().0.into() for some config.

gui1117

partial review

srml/support/test/tests/issue2219.rs

srml/support/test/tests/instance.rs

srml/support/src/runtime.rs

bkchr · 2019-06-20T19:29:31Z

Thx for the extended summary :D

as a note the API change in the following way:

* storage structs that doesn't need generics are now not generic: `<CandidateCount<Test>>::put(1);` is now `CandidateCount::put(1);` for some storages this breaks homogeneity of storages but make more sense with the underlying implementation.

Yeah, I wanted to have it the same way, as it would be in a manual implementation.

* `GenesisConfig` structs that doesn't need generics are now not generic as well, same as above this break homogeneity in favor of actual implementation

This does not break homogeneity, we already could have GenesisConfigs without a generic argument. Now, we just do it the right way. Before, if no generic was required, but a build function was given, we still required the generic.

* `GenesisConfig` no longer implements BuildStorage trait but only some methods which signatures differs depending on if GenesisConfig is generic of not:
  `system::GenesisConfig::<Test>::default().build_storage().unwrap().0.into()` is now `system::GenesisConfig::default().build_storage::<Test>().unwrap().0.into()` for some config.

You could also have system::GenesisConfig::default().build_storage::<Test, Instance>().unwrap().0.into(). This is sadly required, as we can have build function that use some generics.

srml/support/procedural/src/storage/transformation.rs

gui1117

great PR :-) there is just assimilate_require_generic I have to review again but otherwise everything looks good

gui1117 · 2019-06-21T14:42:58Z

srml/support/src/origin.rs

+					($module::Origin < $( $generic, )? $( $module::$generic_instance )? > ),
+				)*
+				#[allow(dead_code)]
+				Void($crate::Void)


do you remember what this was for ?

I mean I removed it and all test compiles

Good question, but I don't really know it and would rather like to keep it for the moment.^^

gui1117 · 2019-06-21T14:59:03Z

srml/support/procedural/src/storage/impls.rs

@@ -634,14 +689,31 @@ impl<'a, I: Iterator<Item=syn::Meta>> Impls<'a, I> {
 			quote!{ #prefix.as_bytes() }
 		};


now that you generate a inherent instance you could make use of it to factorize this if condition like, maybe even renaming changing prefix and having such condition in transformation.rs but whatever I might take look after and all in all it is fine at the moment.

bkchr · 2019-06-21T20:22:19Z

@thiolliere addressed your feedback :)

gui1117 · 2019-06-21T20:40:26Z

Yes changes looks good I can approve or do a final review on tuesday.

Also we might want to update decl_storage doc, as the stucture is no longer always generic.

And I was thinking does it make sense to talk about assimilate storage stuff ? I mean it is for used when writing test for your modules, but I think it is something that ppl want to know, though I'm not sure and could added in a second time as it doesn't exist yet.

bkchr · 2019-06-22T06:27:54Z

Yes changes looks good I can approve or do a final review on tuesday.

Tuesday is okay.

Also we might want to update decl_storage doc, as the stucture is no longer always generic.

Good point.

And I was thinking does it make sense to talk about assimilate storage stuff ? I mean it is for used when writing test for your modules, but I think it is something that ppl want to know, though I'm not sure and could added in a second time as it doesn't exist yet.

You mean the function assimilate_storage?

gavofyork · 2019-06-26T18:34:35Z

did the final review happen?

bkchr · 2019-06-27T06:36:29Z

Not yet.

gui1117 · 2019-06-27T08:46:32Z

And I was thinking does it make sense to talk about assimilate storage stuff ? I mean it is for used when writing test for your modules, but I think it is something that ppl want to know, though I'm not sure and could added in a second time as it doesn't exist yet.

You mean the function assimilate_storage?

Yes I know construct_runtime handle that you, but someone that create a modules will make use of it for its testing so he wants to be aware of subtelties (though everything is in the generated doc)

…stuff

…s error: missing field `_genesis_phantom_data` in initializer of `groups::GenesisConfig<groups::tests::GroupsTest>` Probably related to this: paritytech/substrate#2913 and Issue #2219

…2913) * Add failing test case * move storage maps to blake2_128 (paritytech#2268) * remove default hash, introduce twox_128 and blake2 * use blake2_128 & create ext_blake2_128 * refactor code * add benchmark * factorize generator * fix * parameterizable hasher * some fix * fix * fix * fix * metadata * fix * remove debug print * map -> blake2_256 * fix test * fix test * Apply suggestions from code review Co-Authored-By: thiolliere <gui.thiolliere@gmail.com> * impl twox 128 concat (paritytech#2353) * impl twox_128_concat * comment addressed * fix * impl twox_128->64_concat * fix test * Fix compilation and cleanup some docs * Lol * Remove traits from storage types that are not generic * Get instance test almost working as wanted * Make `srml-support-test` compile again :) * Fixes test of srml-support * Fix compilation * Break some lines * Remove incorrect macro match arm * Integrates review feedback * Update documentation * Fix compilation

bkchr and others added 12 commits April 8, 2019 07:26

Add failing test case

1644e9c

Fix compilation and cleanup some docs

f683e1b

Merge branch 'bkchr-port-blake2-stuff' into bkchr-srml-support-stuff

9750c5f

Lol

09517d3

Remove traits from storage types that are not generic

1c3fff9

Get instance test almost working as wanted

015ae73

Make srml-support-test compile again :)

7368827

Merge remote-tracking branch 'origin/master' into bkchr-srml-support-…

bf55a76

…stuff

Merge branch 'master' into bkchr-srml-support-stuff

58aa5d5

Fixes test of srml-support

63b9ef8

Fix compilation

762b5af

bkchr added the A0-please_review Pull request needs code review. label Jun 20, 2019

bkchr requested a review from gui1117 June 20, 2019 08:28

devops-parity added the A4-gotissues label Jun 20, 2019

Break some lines

311181f

gui1117 reviewed Jun 20, 2019

View reviewed changes

srml/support/test/tests/issue2219.rs Show resolved Hide resolved

srml/support/test/tests/instance.rs Show resolved Hide resolved

srml/support/src/runtime.rs Outdated Show resolved Hide resolved

Remove incorrect macro match arm

69fe2dc

gui1117 reviewed Jun 21, 2019

View reviewed changes

srml/support/procedural/src/storage/transformation.rs Outdated Show resolved Hide resolved

srml/support/procedural/src/storage/transformation.rs Outdated Show resolved Hide resolved

srml/support/procedural/src/storage/transformation.rs Outdated Show resolved Hide resolved

gui1117 reviewed Jun 21, 2019

View reviewed changes

Integrates review feedback

23d13b3

Update documentation

8dd12b7

gui1117 approved these changes Jun 27, 2019

View reviewed changes

bkchr added 2 commits June 27, 2019 12:13

Merge remote-tracking branch 'origin/master' into bkchr-srml-support-…

2cdc4c5

…stuff

Fix compilation

0fba321

bkchr merged commit da91f05 into master Jun 27, 2019

bkchr deleted the bkchr-srml-support-stuff branch June 27, 2019 11:40

		@@ -634,14 +689,31 @@ impl<'a, I: Iterator<Item=syn::Meta>> Impls<'a, I> {
		quote!{ #prefix.as_bytes() }
		};

Conversation

bkchr commented Jun 20, 2019

Uh oh!

gui1117 commented Jun 20, 2019

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr commented Jun 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

gui1117 Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

gui1117 Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

gui1117 Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jun 21, 2019

Uh oh!

gui1117 commented Jun 21, 2019

Uh oh!

bkchr commented Jun 22, 2019

Uh oh!

gavofyork commented Jun 26, 2019

Uh oh!

bkchr commented Jun 27, 2019

Uh oh!

gui1117 commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gui1117 Jun 21, 2019 •

edited

Loading