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

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jan 9, 2020

in the past the syntax for specifying the hasher for key1 was different from for key2.

Now key2 is same as key1: an optional hasher($hash) attribute that default to blake2_256 instead of $hash($MyKey2Type)

Maybe we can also rename this attribute with hasher1 and hasher2 if wanted.

Something not good is that the error message for the old syntax is not understandable because of the way decl_storage is parsing. maybe we should fix it before. Also this needs to be made before 2.0 otherwise we need to support both syntax.

@gui1117 gui1117 requested a review from kianenigma as a code owner January 9, 2020 12:16
@gui1117 gui1117 added the A0-please_review Pull request needs code review. label Jan 9, 2020
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

This is extremely sensitive code. Any mistakes will brick Kusama at the next update. What have you done to make absolutely certain that this changes no logic whatsoever?

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 9, 2020

This is extremely sensitive code. Any mistakes will brick Kusama at the next update. What have you done to make absolutely certain that this changes no logic whatsoever?

Indeed:

  • the old syntax cannot compile as $hash($type) can't be parsed as a type
  • logic in decl_storage hasn't changed, only the parsing.
  • to check how current doublemap usages has been updated, one must be careful that the $hash($type) is translated to hasher($hash) $type or just $type if hasher is blake2_256

@gavofyork
Copy link
Member

can we compare binaries to ensure the code generated is exactly the same?

@bkchr
Copy link
Member

bkchr commented Jan 10, 2020

So, the syntax changed from:
pub DataDM config(test_config): double_map hasher(twox_64_concat) u32, blake2_256(u32) => u64;

to:
pub DataDM config(test_config): double_map hasher(twox_64_concat) u32, u32 => u64;

or more explicit:
pub DataDM config(test_config): double_map hasher(twox_64_concat) u32, hasher(blake2_256) u32 => u64;

The first hasher was already optional and defaulted to blake2_256 when not given. So, we only need to look at newly introduced second hasher. If the second one is not given, it also defaults to blake2_256 now. We only need to convert the old syntax from twox_64_concat(u32) to hasher(twox_64_concat) u32 or blake2_256(u32) to u32 (as blake2_256 is the default value).

As @thiolliere already said, there are no more changes in the macro, just the parsing. This includes code generation, it is the same as before.

@gavofyork gavofyork merged commit ae9c6ea into master Jan 10, 2020
@gavofyork gavofyork deleted the gui-fix-doublemap-syntax branch January 10, 2020 10:52
@gui1117 gui1117 mentioned this pull request Jan 10, 2020
sorpaas pushed a commit that referenced this pull request Nov 20, 2020
* modify doublemap syntax

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <[email protected]>

Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants