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

Allow renaming storage item prefixes #9016

Merged
9 commits merged into from
Jun 14, 2021
3 changes: 1 addition & 2 deletions frame/support/procedural/src/pallet/expand/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ use std::collections::HashSet;
/// prefix_ident is used for the prefix struct to be given to storage as first generic param.
fn prefix_ident(storage: &StorageDef) -> syn::Ident {
let storage_ident = &storage.ident;
let ident = storage.prefix();
syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", ident), storage_ident.span())
syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", storage_ident), storage_ident.span())
}

/// Check for duplicated storage prefixes. This step is necessary since users can specify an
Expand Down
10 changes: 9 additions & 1 deletion frame/support/procedural/src/pallet/parse/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,15 @@ impl syn::parse::Parse for PalletStorageAttr {
content.parse::<keyword::storage_prefix>()?;
content.parse::<syn::Token![=]>()?;

Ok(Self::StorageName(content.parse::<syn::LitStr>()?, attr_span))
let renamed_prefix = content.parse::<syn::LitStr>()?;
// Ensure the renamed prefix is a proper Rust identifier
syn::parse_str::<syn::Ident>(&renamed_prefix.value())
.map_err(|_| {
let msg = format!("`{}` is not a valid identifier", renamed_prefix.value());
syn::Error::new(renamed_prefix.span(), msg)
})?;

Ok(Self::StorageName(renamed_prefix, attr_span))
Copy link
Contributor

Choose a reason for hiding this comment

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

just a sidenote, now that we don't use renamed_prefix as ident, we don't need to force it to be a valid ident anymore AFAICT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want that though? This would mean that it's possible to create storage prefixes that does not follow Rust identifier naming conventions. I guess that kind of makes sense, but something tells me that it's a bad idea, like there might be a footgun lying there somewhere if we're too permissive...

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, we can leverage the constraint when wanted.

} else {
Err(lookahead.error())
}
Expand Down
18 changes: 18 additions & 0 deletions frame/support/test/tests/pallet_ui/storage_invalid_rename_value.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#[frame_support::pallet]
mod pallet {
use frame_support::pallet_prelude::Hooks;
use frame_system::pallet_prelude::BlockNumberFor;

#[pallet::config]
pub trait Config: frame_system::Config {}

#[pallet::pallet]
pub struct Pallet<T>(core::marker::PhantomData<T>);

#[pallet::storage]
#[pallet::storage_prefix = "pub"]
type Foo<T> = StorageValue<_, u8>;
}

fn main() {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: `pub` is not a valid identifier
--> $DIR/storage_invalid_rename_value.rs:13:29
|
13 | #[pallet::storage_prefix = "pub"]
| ^^^^^