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

helper macro to create storage types on the fly #8456

Merged
9 commits merged into from
Mar 30, 2021
Merged

helper macro to create storage types on the fly #8456

9 commits merged into from
Mar 30, 2021

Conversation

kianenigma
Copy link
Contributor

Of course, the codes that I changed now with this macro are all dead-codes, but we can use this in the future.

I also see a lot of potential use case for this in tests where we want to define a storage item on the fly, instead of doing a fake decl_storage, but didn't refactor them here.

cc @Lohann

@kianenigma kianenigma added 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. labels Mar 25, 2021
@kianenigma kianenigma mentioned this pull request Mar 25, 2021
6 tasks
Comment on lines +1042 to +1047
generate_storage_alias!(Staking, SnapshotValidators => Value<()>);
generate_storage_alias!(Staking, SnapshotNominators => Value<()>);
generate_storage_alias!(Staking, QueuedElected => Value<()>);
generate_storage_alias!(Staking, QueuedScore => Value<()>);
generate_storage_alias!(Staking, EraElectionStatus => Value<()>);
generate_storage_alias!(Staking, IsCurrentSessionFinal => Value<()>);
Copy link
Contributor

@Lohann Lohann Mar 25, 2021

Choose a reason for hiding this comment

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

[optional suggestion]

Once the pallet:ident is shared across many storages, what do you think about creating a pattern like this:

generate_storage_alias!(
    Staking,
    SnapshotValidators => Value<()>,
    SnapshotNominators => Value<()>,
    QueuedElected => Value<()>,
    QueuedScore => Value<()>,
    EraElectionStatus => Value<()>,
    IsCurrentSessionFinal => Value<()>,
);

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

IMHO having a macro to generate just:

struct __Voting;
impl frame_support::traits::StorageInstance for __Voting {
	fn pallet_prefix() -> &'static str { "PhragmenElection" }
	const STORAGE_PREFIX: &'static str = "Voting";
}

could be enough.

Also the macro is not usable for when pallet prefix is abstract. which is the case for all pallets using pallet macro.

@kianenigma
Copy link
Contributor Author

errr yeah the prefix being hardcoded is a bad assumption.

Not sure what to do yet. This is not a great approach, but on the other hand we're seeing the need to generate these dummy storage items both for testing and migration on #8414 #8197

@kianenigma
Copy link
Contributor Author

Of course, I am can make it support a non-string literal as well, but it might clutter the code a lot.

frame/support/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Copy link
Contributor

@apopiak apopiak 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 cool, will make writing migrations easier

@kianenigma
Copy link
Contributor Author

Of course, I am can make it support a non-string literal as well, but it might clutter the code a lot.

Not sure about this, will leave the decision to @thiolliere.

@gui1117
Copy link
Contributor

gui1117 commented Mar 29, 2021

Of course, I am can make it support a non-string literal as well, but it might clutter the code a lot.

Not sure about this, will leave the decision to @thiolliere.

I don't really know, maybe instead of ident we can take expression,

generate_storage_alias!("Staking", SnapshotValidators => Value<()>);

It would look like this, which isn't really good:

generate_storage_alias!(T::PalletInfo::name<Pallet<T>>().unwrap(), SnapshotValidators<T: Config> => Value<()>);

And the macro magic is difficult to grasp.

Maybe we can make something slightly more verbose but more understandable:

storage_prefix!(FooPrefix => "Staking", "Foo");
type Foo = StorageValue<FooPrefix, ()>);

storage_prefix!(FooPrefix<T> => <T as frame_system::Config>::PalletInfo::name<Pallet<T>>().unwrap(), "Foo");
type Foo<T> = StorageValue<FooPrefix<T>, ()>);

So I don't have any good idea

@apopiak
Copy link
Contributor

apopiak commented Mar 29, 2021

I would say merge as-is and iterate on the design later?

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

If ppl start using them in test and migration file, I would prefer the syntax to not change. Having to update migration and test code is annoying.

But if the design is too limiting we can create a new macro I guess, and deprecate the other one.

Also I hope we don't bring new magic which will make it hard to understand the code.

That said the code is correct so I don't mind.

@kianenigma kianenigma added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 29, 2021
@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 29, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Mar 29, 2021

Merge aborted: Checks failed for 237810b

@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 30, 2021

Waiting for commit status.

@ghost ghost merged commit aa33441 into master Mar 30, 2021
@ghost ghost deleted the kiz-storage-macro branch March 30, 2021 09:40
ordian added a commit that referenced this pull request Mar 31, 2021
* master: (84 commits)
  Duplicate logging to stdout (#8495)
  Fix sync restart (#8497)
  client: fix justifications migration (#8489)
  helper macro to create storage types on the fly (#8456)
  Make `BlockImport` and `Verifier` async (#8472)
  Get rid of `test-helpers` feature in sc-consensus-babe (#8486)
  Enhancement on Substrate Node Template (#8473)
  Add Social Network (#8065)
  Prepare UI tests for Rust 1.51 & new CI image (#8474)
  Benchmarking pallet-example (#8301)
  Use pathbuf for remote externalities (#8480)
  Bring back the on_finalize weight of staking. (#8463)
  Implement `fungible::*` for Balances (#8454)
  make types within `generate_solution_type` macro explicit (#8447)
  [pallet-staking] Refund unused weight for `payout_stakers` (#8458)
  Use `async_trait` in sc-consensus-slots (#8461)
  Repot frame_support::traits; introduce some new currency stuff (#8435)
  Fix &mut self -> &self in add_known_address (#8468)
  Add NetworkService::add_known_address (#8467)
  Fix companion check (#8464)
  ...
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* helper macro to create storage types on the fly

* Update frame/support/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* update lock

* fix test;

* Fix line width

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
shawntabrizi pushed a commit that referenced this pull request Jun 17, 2021
* helper macro to create storage types on the fly

* Update frame/support/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* update lock

* fix test;

* Fix line width

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
This pull request was closed.
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. 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
Development

Successfully merging this pull request may close these issues.

4 participants