Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events and ParachainSystem::HostConfiguration #3454

Merged
merged 18 commits into from
Feb 28, 2024
Merged

Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events and ParachainSystem::HostConfiguration #3454

merged 18 commits into from
Feb 28, 2024

Conversation

liamaharon
Copy link
Contributor

Closes #2560

Allows marking storage items with #[disable_try_decode_storage], and uses it with System::Events.

Question: what's the recommended way to write a test for this? I couldn't find a test for similar existing macro #[whitelist_storage].

@liamaharon liamaharon added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Feb 23, 2024
@liamaharon liamaharon requested a review from a team as a code owner February 23, 2024 02:55
liamaharon
liamaharon commented Feb 23, 2024
View reviewed changes
substrate/frame/support/procedural/src/pallet/expand/storage.rs
@@ -834,7 +834,7 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
.storages
.iter()
.filter_map(|storage| {
if storage.cfg_attrs.is_empty() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma do you remember why we check if storage.cfg_attrs.is_empty() here?

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 seems it's to avoid this compile error https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5317738

I wonder if there's a better way to handle that.

Copy link
Member

Choose a reason for hiding this comment

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

Yea we should probably re-expand the attributes here.
It seems to be only used only for testing.

@liamaharon liamaharon changed the title Add new storage attr macro #[disable_try_decode_storage] and set it on System::Events Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events Feb 23, 2024
franciscoaguirre
franciscoaguirre approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

The whole macro changes look good

@kianenigma
Copy link
Contributor

kianenigma commented Feb 23, 2024 •
edited by liamaharon
Loading

I thought an off-band discussion concluded that we don't want to expose this to the end-user facing API, and just hardcode somewhere that System::events is an exception.

This PR is not doing that, but it is neatly done.

I would like to see:

  • the docs for the new macro being fixed
  • the docs for #[pallet::storage] updated with just one line that references this new attribute, namely here.

Before merge tho

@liamaharon
Copy link
Contributor Author

liamaharon commented Feb 24, 2024 •
edited
Loading

I thought an off-band discussion concluded that we don't want to expose this to the end-user facing API, and just hardcode somewhere that System::events is an exception.

Basti requested this to be done as an attribute macro rather than a hardcoded exception for System::Events here.

@xlc
Copy link
Contributor

xlc commented Feb 24, 2024

@liamaharon can you quote it instead of private link

@bkchr
Copy link
Member

bkchr commented Feb 24, 2024

My point basically was/is that the same thing can happen to downstream users. Like some storage item that is always deleted at the beginning of the next block. Thus, you don't really care if you can still decode it.

@xlc
Copy link
Contributor

xlc commented Feb 25, 2024

In that case it is the try runtime tool should call initialize hook before validate storage entries.

@xlc xlc mentioned this pull request Feb 26, 2024
Closed
@liamaharon liamaharon force-pushed the liam-disable-try-decode-storage-attr-macro branch from 83c339a to 06ff03b Compare February 26, 2024 04:31
@liamaharon liamaharon added this pull request to the merge queue Feb 26, 2024
@liamaharon liamaharon removed this pull request from the merge queue due to a manual request Feb 27, 2024
@liamaharon liamaharon changed the title Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events and ParachainSystem::HostConfiguration Feb 27, 2024
xlc
xlc suggested changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

It will be great if you can help address my concerns before merge #2560 (comment)

@liamaharon liamaharon added this pull request to the merge queue Feb 28, 2024
Merged via the queue into master with commit 95da658 Feb 28, 2024
129 of 130 checks passed
@liamaharon liamaharon deleted the liam-disable-try-decode-storage-attr-macro branch February 28, 2024 02:38
ordian added a commit that referenced this pull request Feb 28, 2024
@ordian
Merge remote-tracking branch 'origin/master' into ao-collator-parent-…
8b70d0f 
…head-data

* origin/master: (51 commits)
  Runtime Upgrade ref docs and Single Block Migration example pallet  (#1554)
  Collator overseer builder unification (#3335)
  Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454)
  Add Polkadotters bootnoders per IBP application (#3423)
  Add documentation around FRAME Origin (#3362)
  Bridge zombienet tests: Check amount received at destination (#3490)
  Snowbridge benchmark tests fix (#3424)
  fix(zombienet): increase timeout in download artifacts (#3376)
  Cleanup String::from_utf8 (#3446)
  [prdoc] Validate crate names (#3467)
  Limit max execution time for `test-linux-stable` CI jobs (#3483)
  Introduce Notification block pinning limit (#2935)
  frame-support: Improve error reporting when having too many pallets (#3478)
  add Encointer as trusted teleporter for Westend (#3411)
  [pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (#3464)
  Add more debug logs to understand if statement-distribution misbehaving (#3419)
  Remove redundant parachains assigner pallet. (#3457)
  Use generic hash for runtime wasm in resolve_state_version_from_wasm (#3447)
  Runtime: allow backing multiple candidates of same parachain on different cores  (#3231)
  Bridge zombienet tests: move all "framework" files under one folder (#3462)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@liamaharon
Introduce storage attr macro #[disable_try_decode_storage] and set …
9c96053 
…it on `System::Events` and `ParachainSystem::HostConfiguration` (paritytech#3454)

Closes paritytech#2560

Allows marking storage items with `#[disable_try_decode_storage]`, and
uses it with `System::Events`.

Question: what's the recommended way to write a test for this? I
couldn't find a test for similar existing macro `#[whitelist_storage]`.
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xlc xlc xlc requested changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@kianenigma kianenigma kianenigma approved these changes

@ggwpez ggwpez ggwpez approved these changes

Assignees
No one assigned
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skip System::Events in TryDecodeEntireState
6 participants
@liamaharon @kianenigma @xlc @bkchr @franciscoaguirre @ggwpez