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

Makes storage hashers optional in dev mode #13815

Merged
merged 6 commits into from
Apr 11, 2023
Merged

Makes storage hashers optional in dev mode #13815

merged 6 commits into from
Apr 11, 2023

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Apr 4, 2023

As discussed in #13263, this PR adds the ability to make storage hashers as _ in dev_mode. All code without dev_mode continues to work in dev_mode.

@gupnik gupnik added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2023
@sam0x17
Copy link
Contributor

sam0x17 commented Apr 4, 2023

This looks great! I would definitely add a UI test covering when someone tries to use this syntax without dev mode enabled

@gupnik gupnik changed the title [WIP]: Makes storage hashers optional in dev mode Makes storage hashers optional in dev mode Apr 4, 2023
@gupnik gupnik marked this pull request as ready for review April 4, 2023 16:23
@gupnik gupnik 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 Apr 4, 2023
@gupnik gupnik requested a review from sam0x17 April 4, 2023 16:23
@gupnik gupnik added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 4, 2023
@gupnik gupnik requested a review from kianenigma April 4, 2023 16:26
@gupnik
Copy link
Contributor Author

gupnik commented Apr 4, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Apr 4, 2023 •
edited
Loading

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2641655 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-3874f62f-44ec-4ea7-be83-dec95bfff3db to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Apr 4, 2023

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2641655 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2641655/artifacts/download.

@gupnik gupnik requested review from ggwpez and KiChjang April 10, 2023 15:27
ggwpez
ggwpez reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I am not that familiar with the code but looks good.

frame/support/test/tests/pallet_ui/pass/dev_mode_valid.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/parse/storage.rs Show resolved Hide resolved
frame/support/procedural/src/pallet/parse/storage.rs Show resolved Hide resolved
ggwpez
ggwpez approved these changes Apr 11, 2023
View reviewed changes
frame/support/test/tests/pallet_ui/pass/dev_mode_valid.rs
pallet::MyStorageMap::<Runtime>::insert(1, 2);
let mut k = [twox_128(b"Example"), twox_128(b"MyStorageMap")].concat();
k.extend(1u32.using_encoded(blake2_128_concat));
assert_eq!(unhashed::get::<u64>(&k), Some(2u64));
Copy link
Member

Choose a reason for hiding this comment

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

Yes thanks for the test!

Normally we only test UI behaviour in the UI tests and not logic, but i think it is fine.

@gupnik gupnik merged commit da9f88d into master Apr 11, 2023
@gupnik gupnik deleted the gupnik/optional_hashers_dev branch April 11, 2023 15:10
bkchr
bkchr reviewed Apr 11, 2023
View reviewed changes
frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.stderr
Comment on lines +1 to +5
error[E0121]: the placeholder `_` is not allowed within types on item signatures for type aliases
--> tests/pallet_ui/dev_mode_without_arg_default_hasher.rs:21:47
|
21 | type MyStorageMap<T: Config> = StorageMap<_, _, u32, u64>;
| ^ not allowed in type signatures
Copy link
Member

Choose a reason for hiding this comment

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

This will confuse people when they disable dev_mode. They will not get why it doesn't work. I would still be in favor of having Blake2128Concat as default hasher, even in "normal mode". However, the bare minimum should be a proper error message that tells the user on what to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, will fix the error in another PR

@gupnik gupnik mentioned this pull request Apr 12, 2023
Merged
@kianenigma
Copy link
Contributor

kianenigma commented Apr 17, 2023 •
edited
Loading

I think the fact that this PR has not updated any docs is not good IMO. This is exactly the type of change that does affect the end users of FRAME, and should be better documented going forward -- Do we have any docs for dev_mode in the code?

  • If no, we should, including an pallet-example-dev-mode of some sort.
  • If yes, they should have been updated.

@gupnik can you make a follow-up?

@gupnik
Copy link
Contributor Author

gupnik commented Apr 18, 2023 •
edited
Loading

@kianenigma Got it. Will raise a follow-up.

@gupnik gupnik mentioned this pull request Apr 18, 2023
Merged
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@codekitz @nathanwhit
Makes storage hashers optional in dev mode (paritytech#13815)
b1b86cc 
* Initial changes

* Adds UI test for error when _ is used without dev_mode

* Minor

* ".git/.scripts/commands/fmt/fmt.sh"

* Adds test to verify hasher

---------

Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr left review comments

@ggwpez ggwpez ggwpez approved these changes

@sam0x17 sam0x17 sam0x17 approved these changes

@kianenigma kianenigma Awaiting requested review from kianenigma

@KiChjang KiChjang Awaiting requested review from KiChjang

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gupnik @sam0x17 @kianenigma @bkchr @ggwpez @codekitz