Skip to content

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

Merged
liamaharon merged 18 commits intomasterfrom
liam-disable-try-decode-storage-attr-macro
Feb 28, 2024
Merged

Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events and ParachainSystem::HostConfiguration#3454
liamaharon merged 18 commits intomasterfrom
liam-disable-try-decode-storage-attr-macro

Conversation

@liamaharon
Copy link
Copy Markdown
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
.storages
.iter()
.filter_map(|storage| {
if storage.cfg_attrs.is_empty() {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kianenigma commented Feb 23, 2024

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
Copy Markdown
Contributor Author

liamaharon commented Feb 24, 2024

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
Copy Markdown
Contributor

xlc commented Feb 24, 2024

@liamaharon can you quote it instead of private link

@bkchr
Copy link
Copy Markdown
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
Copy Markdown
Contributor

xlc commented Feb 25, 2024

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

@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 enabled auto-merge February 26, 2024 05:00
@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
@liamaharon liamaharon enabled auto-merge February 27, 2024 00:18
Copy link
Copy Markdown
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 disabled auto-merge February 27, 2024 06:16
@liamaharon liamaharon enabled auto-merge February 28, 2024 01:45
@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
@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
…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
…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]`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T1-FRAME This PR/Issue is related to core FRAME, the framework.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Skip System::Events in TryDecodeEntireState

6 participants