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
32 changes: 26 additions & 6 deletions frame/support/procedural/src/pallet/expand/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,30 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::Def;
use crate::pallet::{Def, parse::storage::StorageDef};
use crate::pallet::parse::storage::{Metadata, QueryKind, StorageGenerics};
use frame_support_procedural_tools::clean_type_string;

fn literal_to_string(lit: &syn::Lit) -> String {
match lit {
syn::Lit::Str(s) => s.value(),
syn::Lit::ByteStr(s) =>
String::from_utf8(s.value()).expect("All byte strings are valid UTF-8 strings; qed"),
syn::Lit::Byte(b) => char::from(b.value()).to_string(),
syn::Lit::Char(c) => c.value().to_string(),
syn::Lit::Int(i) => i.base10_digits().to_owned(),
syn::Lit::Float(f) => f.base10_digits().to_owned(),
syn::Lit::Bool(b) => b.value.to_string(),
syn::Lit::Verbatim(l) => l.to_string(),
}
}
KiChjang marked this conversation as resolved.
Show resolved Hide resolved

/// Generate the prefix_ident related the the storage.
/// prefix_ident is used for the prefix struct to be given to storage as first generic param.
fn prefix_ident(storage_ident: &syn::Ident) -> syn::Ident {
syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", storage_ident), storage_ident.span())
fn prefix_ident(storage: &StorageDef) -> syn::Ident {
let storage_ident = &storage.ident;
let ident = storage.rename_as.as_ref().map(literal_to_string).unwrap_or(storage_ident.to_string());
Copy link
Member

Choose a reason for hiding this comment

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

So this is all the magic right?

We either take the storage rename, or we fallback to the old ident

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually not here, but on the prefix_struct_const code, because that's the value that ultimately gets assigned to StorageItem::STORAGE_PREFIX.

syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", ident), storage_ident.span())
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
}

/// * if generics are unnamed: replace the first generic `_` by the generated prefix structure
Expand Down Expand Up @@ -50,7 +66,7 @@ pub fn process_generics(def: &mut Def) {
_ => unreachable!("Checked by def"),
};

KiChjang marked this conversation as resolved.
Show resolved Hide resolved
let prefix_ident = prefix_ident(&storage_def.ident);
let prefix_ident = prefix_ident(&storage_def);
let type_use_gen = if def.config.has_instance {
quote::quote_spanned!(storage_def.attr_span => T, I)
} else {
Expand Down Expand Up @@ -344,9 +360,13 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
let prefix_structs = def.storages.iter().map(|storage_def| {
let type_impl_gen = &def.type_impl_generics(storage_def.attr_span);
let type_use_gen = &def.type_use_generics(storage_def.attr_span);
let prefix_struct_ident = prefix_ident(&storage_def.ident);
let prefix_struct_ident = prefix_ident(&storage_def);
let prefix_struct_vis = &storage_def.vis;
let prefix_struct_const = storage_def.ident.to_string();
let prefix_struct_const = storage_def
.rename_as
.as_ref()
.map(literal_to_string)
.unwrap_or(storage_def.ident.to_string());
let config_where_clause = &def.config.where_clause;

let cfg_attrs = &storage_def.cfg_attrs;
Expand Down
69 changes: 57 additions & 12 deletions frame/support/procedural/src/pallet/parse/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,26 @@ mod keyword {
syn::custom_keyword!(Error);
syn::custom_keyword!(pallet);
syn::custom_keyword!(getter);
syn::custom_keyword!(storage_name);
syn::custom_keyword!(OptionQuery);
syn::custom_keyword!(ValueQuery);
}

/// Parse for `#[pallet::getter(fn dummy)]`
pub struct PalletStorageAttr {
getter: syn::Ident,
/// Parse for one of the following:
/// * `#[pallet::getter(fn dummy)]`
/// * `#[pallet::storage_name = "CustomName"]`
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
pub enum PalletStorageAttr {
Getter(syn::Ident),
StorageName(syn::Lit),
}
KiChjang marked this conversation as resolved.
Show resolved Hide resolved

impl PalletStorageAttr {
fn span(&self) -> proc_macro2::Span {
match self {
Self::Getter(ident) => ident.span(),
Self::StorageName(lit) => lit.span(),
}
}
}

impl syn::parse::Parse for PalletStorageAttr {
Expand All @@ -41,12 +54,23 @@ impl syn::parse::Parse for PalletStorageAttr {
syn::bracketed!(content in input);
content.parse::<keyword::pallet>()?;
content.parse::<syn::Token![::]>()?;
content.parse::<keyword::getter>()?;

let generate_content;
syn::parenthesized!(generate_content in content);
generate_content.parse::<syn::Token![fn]>()?;
Ok(Self { getter: generate_content.parse::<syn::Ident>()? })
let lookahead = content.lookahead1();
if lookahead.peek(keyword::getter) {
content.parse::<keyword::getter>()?;

let generate_content;
syn::parenthesized!(generate_content in content);
generate_content.parse::<syn::Token![fn]>()?;
Ok(Self::Getter(generate_content.parse::<syn::Ident>()?))
} else if lookahead.peek(keyword::storage_name) {
content.parse::<keyword::storage_name>()?;
content.parse::<syn::Token![=]>()?;

Ok(Self::StorageName(content.parse::<syn::Lit>()?))
} else {
Err(lookahead.error())
}
}
}

Expand Down Expand Up @@ -89,6 +113,8 @@ pub struct StorageDef {
pub instances: Vec<helper::InstanceUsage>,
/// Optional getter to generate. If some then query_kind is ensured to be some as well.
pub getter: Option<syn::Ident>,
/// Optional expression that evaluates to a type that can be used as StoragePrefix instead of ident.
pub rename_as: Option<syn::Lit>,
/// Whereas the querytype of the storage is OptionQuery or ValueQuery.
/// Note that this is best effort as it can't be determined when QueryKind is generic, and
/// result can be false if user do some unexpected type alias.
Expand Down Expand Up @@ -552,12 +578,30 @@ impl StorageDef {
return Err(syn::Error::new(item.span(), "Invalid pallet::storage, expect item type."));
};

let mut attrs: Vec<PalletStorageAttr> = helper::take_item_pallet_attrs(&mut item.attrs)?;
if attrs.len() > 1 {
let attrs: Vec<PalletStorageAttr> = helper::take_item_pallet_attrs(&mut item.attrs)?;
let (mut getters, mut names) = attrs
.into_iter()
.partition::<Vec<_>, _>(|attr| matches!(attr, PalletStorageAttr::Getter(_)));
if getters.len() > 1 {
let msg = "Invalid pallet::storage, multiple argument pallet::getter found";
return Err(syn::Error::new(attrs[1].getter.span(), msg));
return Err(syn::Error::new(getters[1].span(), msg));
}
if names.len() > 1 {
let msg = "Invalid pallet::storage, multiple argument pallet::storage_name found";
return Err(syn::Error::new(names[1].span(), msg));
}
let getter = attrs.pop().map(|attr| attr.getter);
let getter = getters.pop().map(|attr| {
match attr {
PalletStorageAttr::Getter(ident) => ident,
_ => unreachable!(),
}
});
let rename_as = names.pop().map(|attr| {
match attr {
PalletStorageAttr::StorageName(lit) => lit,
_ => unreachable!(),
}
});

let cfg_attrs = helper::get_item_cfg_attrs(&item.attrs);

Expand Down Expand Up @@ -609,6 +653,7 @@ impl StorageDef {
metadata,
docs,
getter,
rename_as,
query_kind,
where_clause,
cfg_attrs,
Expand Down
20 changes: 20 additions & 0 deletions frame/support/test/tests/pallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ pub mod pallet {
#[pallet::storage]
pub type Value<T> = StorageValue<Value = u32>;

#[pallet::storage]
#[pallet::storage_name = "Value2"]
pub type RenamedValue<T> = StorageValue<Value = u64>;

#[pallet::type_value]
pub fn MyDefault<T: Config>() -> u16
where T::AccountId: From<SomeType7> + From<SomeType1> + SomeAssociation1
Expand Down Expand Up @@ -577,6 +581,10 @@ fn storage_expand() {
let k = [twox_128(b"Example"), twox_128(b"Value")].concat();
assert_eq!(unhashed::get::<u32>(&k), Some(1u32));

pallet::RenamedValue::<Runtime>::put(2);
let k = [twox_128(b"Example"), twox_128(b"Value2")].concat();
assert_eq!(unhashed::get::<u64>(&k), Some(2));

pallet::Map::<Runtime>::insert(1, 2);
let mut k = [twox_128(b"Example"), twox_128(b"Map")].concat();
k.extend(1u8.using_encoded(blake2_128_concat));
Expand Down Expand Up @@ -697,6 +705,13 @@ fn metadata() {
default: DecodeDifferent::Decoded(vec![0]),
documentation: DecodeDifferent::Decoded(vec![]),
},
StorageEntryMetadata {
name: DecodeDifferent::Decoded("Value2".to_string()),
Copy link
Member

Choose a reason for hiding this comment

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

I think we might actually want to break the metadata here...

Maybe not right in this PR, but you can imagine someone will want to expose the name as "Balance" but use a custom prefix like "1".

As such, there would be a name property in the metadata, and a prefix property.

But this is something that would be much more annoying to merge in, and so probably should be an issue and we save it for when we already will break metadata.

modifier: StorageEntryModifier::Optional,
ty: StorageEntryType::Plain(DecodeDifferent::Decoded("u64".to_string())),
default: DecodeDifferent::Decoded(vec![0]),
documentation: DecodeDifferent::Decoded(vec![]),
},
StorageEntryMetadata {
name: DecodeDifferent::Decoded("Map".to_string()),
modifier: StorageEntryModifier::Default,
Expand Down Expand Up @@ -993,6 +1008,11 @@ fn test_storage_info() {
max_values: Some(1),
max_size: Some(4),
},
StorageInfo {
prefix: prefix(b"Example", b"Value2"),
max_values: Some(1),
max_size: Some(8),
},
StorageInfo {
prefix: prefix(b"Example", b"Map"),
max_values: None,
Expand Down
24 changes: 24 additions & 0 deletions frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#[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::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}

#[pallet::call]
impl<T: Config> Pallet<T> {}

#[pallet::storage]
#[pallet::generate_store(pub trait Store)]
type Foo<T> = StorageValue<u8, u8>;
}

fn main() {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: expected `getter` or `storage_name`
--> $DIR/storage_invalid_attribute.rs:19:15
|
19 | #[pallet::generate_store(pub trait Store)]
| ^^^^^^^^^^^^^^
25 changes: 25 additions & 0 deletions frame/support/test/tests/pallet_ui/storage_multiple_getters.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#[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::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}

#[pallet::call]
impl<T: Config> Pallet<T> {}

#[pallet::storage]
#[pallet::getter(fn get_foo)]
#[pallet::getter(fn foo_error)]
type Foo<T> = StorageValue<_, u8>;
}

fn main() {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Invalid pallet::storage, multiple argument pallet::getter found
--> $DIR/storage_multiple_getters.rs:20:25
|
20 | #[pallet::getter(fn foo_error)]
| ^^^^^^^^^
25 changes: 25 additions & 0 deletions frame/support/test/tests/pallet_ui/storage_multiple_renames.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#[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::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}

#[pallet::call]
impl<T: Config> Pallet<T> {}

#[pallet::storage]
#[pallet::storage_name = "Bar"]
#[pallet::storage_name = "Baz"]
type Foo<T> = StorageValue<_, u8>;
}

fn main() {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Invalid pallet::storage, multiple argument pallet::storage_name found
--> $DIR/storage_multiple_renames.rs:20:30
|
20 | #[pallet::storage_name = "Baz"]
| ^^^^^