Tidy Deprecation Information and prep 22.0.0 release#101
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the deprecation information in the metadata to eliminate ambiguous states and improve clarity without worsening memory usage. Key changes include renaming deprecation types across various metadata structs, updating the associated enums for item and enum deprecation, and introducing a new enum for variant deprecation.
Comments suppressed due to low confidence (3)
frame-metadata/src/v16.rs:108
- Ensure that 'ItemDeprecationInfo' clearly indicates its role as representing item-level deprecation and that the updated naming is consistent with related documentation.
pub deprecation_info: ItemDeprecationInfo<T>,
frame-metadata/src/v16.rs:572
- [nitpick] Verify that 'EnumDeprecationInfo' effectively differentiates between full deprecation and variant-specific deprecation; ensure the inline documentation reflects this distinction clearly.
pub enum EnumDeprecationInfo<T: Form = MetaForm> {
frame-metadata/src/v16.rs:618
- [nitpick] Consider enriching the documentation for 'VariantDeprecationInfo' so its purpose in handling variant deprecation is unmistakably clear.
pub enum VariantDeprecationInfo<T: Form = MetaForm> {
niklasad1
reviewed
Apr 23, 2025
niklasad1
approved these changes
Apr 23, 2025
pkhry
approved these changes
Apr 23, 2025
Contributor
Author
|
Will make a Substrate PR to confirm that removing the "enum deprecated" variants works ok w.r.t whatever the attrs etc are before merging this. |
jsdw
changed the title
github-merge-queue Bot
pushed a commit
to paritytech/polkadot-sdk
that referenced
this pull request
Apr 30, 2025
The main change in [the proposed `frame-metadata@22`](paritytech/frame-metadata#101) is that the deprecation information was tidied to remove some possible but confusing states, which simplifies making use of it in client code. One change made in the process is that **the ability to deprecate the entire of the Call/Error/Event enum was removed**. In other words, prior to this change a pallet author could do either of these: ```rust /// Deprecate the entire enum (but this is ignored if any variants are deprecated): #[deprecated(note = "This is deprecated")] #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event<T: Config<I>, I: 'static = ()> { /// An account was created with some free balance. Endowed { account: T::AccountId, free_balance: T::Balance }, /// An account was removed whose balance was non-zero but below ExistentialDeposit, /// resulting in an outright loss. DustLost { account: T::AccountId, amount: T::Balance }, } /// Deprecate individual variants: #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event<T: Config<I>, I: 'static = ()> { /// An account was created with some free balance. #[deprecated(note = "This is deprecated")] Endowed { account: T::AccountId, free_balance: T::Balance }, /// An account was removed whose balance was non-zero but below ExistentialDeposit, /// resulting in an outright loss. #[deprecated(note = "This is deprecated too")] DustLost { account: T::AccountId, amount: T::Balance }, } ``` After this PR, **a pallet author can still deprecate individual variants, but not the entire enum**. Some reasoning behind this change: 1. It's unlikely that the entire of these enums would ever be entirely deprecated. 2. In the case of calls, there is no obvious place to put such a warning anyway. 3. Sticking to just deprecating variants makes it a little simpler for client code; fewer cases to handle during codegen or whatever. 4. Avoid a confusing state where `#[deprecation]` can be put on the entire enum _and_ some variants, which leads to the enum-wide deprecation warning being ignored. See paritytech/frame-metadata#100 for some more context on the changes. # Notes This PR currently pulls `frame-metadata` via a github branch, for testing purposes. If we are happy here, I'll merge that PR, release `frame-metadata` and remove the github dependency before we merge this.
castillax
pushed a commit
to paritytech/polkadot-sdk
that referenced
this pull request
May 12, 2025
The main change in [the proposed `frame-metadata@22`](paritytech/frame-metadata#101) is that the deprecation information was tidied to remove some possible but confusing states, which simplifies making use of it in client code. One change made in the process is that **the ability to deprecate the entire of the Call/Error/Event enum was removed**. In other words, prior to this change a pallet author could do either of these: ```rust /// Deprecate the entire enum (but this is ignored if any variants are deprecated): #[deprecated(note = "This is deprecated")] #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event<T: Config<I>, I: 'static = ()> { /// An account was created with some free balance. Endowed { account: T::AccountId, free_balance: T::Balance }, /// An account was removed whose balance was non-zero but below ExistentialDeposit, /// resulting in an outright loss. DustLost { account: T::AccountId, amount: T::Balance }, } /// Deprecate individual variants: #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event<T: Config<I>, I: 'static = ()> { /// An account was created with some free balance. #[deprecated(note = "This is deprecated")] Endowed { account: T::AccountId, free_balance: T::Balance }, /// An account was removed whose balance was non-zero but below ExistentialDeposit, /// resulting in an outright loss. #[deprecated(note = "This is deprecated too")] DustLost { account: T::AccountId, amount: T::Balance }, } ``` After this PR, **a pallet author can still deprecate individual variants, but not the entire enum**. Some reasoning behind this change: 1. It's unlikely that the entire of these enums would ever be entirely deprecated. 2. In the case of calls, there is no obvious place to put such a warning anyway. 3. Sticking to just deprecating variants makes it a little simpler for client code; fewer cases to handle during codegen or whatever. 4. Avoid a confusing state where `#[deprecation]` can be put on the entire enum _and_ some variants, which leads to the enum-wide deprecation warning being ignored. See paritytech/frame-metadata#100 for some more context on the changes. # Notes This PR currently pulls `frame-metadata` via a github branch, for testing purposes. If we are happy here, I'll merge that PR, release `frame-metadata` and remove the github dependency before we merge this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The deprecation information allowed a few "confusing" / duplicate states which are all semantically equivalent to
DeprecationInfo::NotDeprecated:DeprecationInfo::ItemDeprecated(DeprecationStatus::NotDeprecated)DeprecationInfo::VariantsDeprecated(BTreeMap::from_iter([(1u8, DeprecationStatus::NotDeprecated)]))DeprecationInfo::VariantsDeprecated(BTreeMap::from_iter([])).These possibilities make it harder to be sure whether something is deprecated or not (even though in practice we only expect to see the "obvious" state for "not deprecated").
Additionally, the
DeprecationInfoandDeprecationStatusnames were a bit confusing to me and felt like synonyms.One possibility to resolve this is to remove the
NotDeprecatedvariants and wrapDeprecationInfo/DeprecationStatusin anOptioninstead. The downside there is that we end up using an extra byte for any deprecated item.Instead, I duplicate a couple of the variants and such so that we don't use any additional bytes (and actually save a few), while removing any confusing states and tweaking the names to be clearer (to me, at least..).
Note: The ability to deprecate the entire of a Call/Error/Event enum was removed here. Reason being that it seemed pointless to support; if such an enum has one variant left and you wish to remove the enum, then you would deprecate that variant. Users interact with the specific errors/calls/events anyway more than with any variant, and libraries can generate (or not) such variants as they please from the metadata.
Closes #100
Polkadot-sdk PR here: paritytech/polkadot-sdk#8327