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

pallet macro: allow to declare individual unbounded storage for those who cannot go into PoV #9670

Merged
9 commits merged into from
Sep 27, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Sep 1, 2021

This PR introduce a new attribute in the pallet macro for storage: pallet::unbounded.

This attribute allow to declare that a storage doesn't have bounds on its keys and value types and thus only generate partial storage information.

For storage who cannot go into PoV this is useful. Like system event for instance.

Limitation: Maybe the some storage might want to express the bounds on the key types but not the bound on the value. Currently the storage info doesn't support this. I think we can implement more fine grained description when the need comes.

@gui1117 gui1117 added A0-please_review Pull request needs code review. B3-apinoteworthy 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 Sep 1, 2021
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM, why is the system::event not already tagged?

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@gui1117
Copy link
Contributor Author

gui1117 commented Sep 14, 2021

for now event goes into PoV, and frame_system doesn't declare the storage info bounds, so no storage are bounded.

for attr in attrs {
match attr {
PalletStorageAttr::Getter(ident, ..) if getter.is_none() => getter = Some(ident),
PalletStorageAttr::StorageName(name, ..) if rename_as.is_none() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a change in api: before we errored if there was more than 1 getter or name. Now we simply skip anything after the first one.

for example we had

		if names.len() > 1 {
			let msg = "Invalid pallet::storage, multiple argument pallet::storage_prefix found";
			return Err(syn::Error::new(names[1].attr_span(), msg))
		}

Copy link
Contributor

Choose a reason for hiding this comment

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

looking at the UI tests though it seems like it still complains about a duplicate attribute, is this just now an error we can rely on the compiler for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the duplicate attribute is not from the compiler it is from the last matching arm below, in case rename_as is already some, then the last matching are is branched and the error is raised

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

My understanding of the proc-macro libs is really not great, but from my level of understanding it looks ok.

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 27, 2021

bot merge

@ghost
Copy link

ghost commented Sep 27, 2021

Waiting for commit status.

@ghost ghost merged commit be28ab0 into master Sep 27, 2021
@ghost ghost deleted the gui-individual-unbounded branch September 27, 2021 08:25
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. 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.

3 participants