Skip to content

Update to frame-metadata 22#8327

Merged
jsdw merged 11 commits into
masterfrom
jsdw-frame-metadata-22
Apr 30, 2025
Merged

Update to frame-metadata 22#8327
jsdw merged 11 commits into
masterfrom
jsdw-frame-metadata-22

Conversation

@jsdw
Copy link
Copy Markdown
Contributor

@jsdw jsdw commented Apr 24, 2025

The main change in the proposed frame-metadata@22 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:

/// 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.

@jsdw jsdw requested a review from a team as a code owner April 24, 2025 14:48
@jsdw jsdw changed the title Bump to frame-metadata 22 Update to frame-metadata 22 Apr 25, 2025
@jsdw jsdw added R0-no-crate-publish-required The change does not require any crates to be re-published. T4-runtime_API This PR/Issue is related to runtime APIs. labels Apr 25, 2025
Copy link
Copy Markdown
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Indeed, it sounds highly unlikely that an entire enum would be deprecated and since it also simplifies the code on the client side everything looks good, thanks🙏

I think our testing also needs a bit of adjusting to pass the CI (https://github.com/paritytech/polkadot-sdk/actions/runs/14663412489/job/41152740930?pr=8327)

@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/14665929676
Failed job name: build-runtimes-polkavm

Comment thread prdoc/pr_8327.prdoc
Copy link
Copy Markdown
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.

After this PR, a pallet author can still deprecate individual variants, but not the entire enum.

I agree that this is very rare to want to deprecate the whole thing. Even if needed, developers can still deprecate individual items.

Assuming this simplifies the internals of managing deprecation, it is a reasonable choice ✅

@jsdw jsdw enabled auto-merge April 30, 2025 11:01
@jsdw jsdw added this pull request to the merge queue Apr 30, 2025
Merged via the queue into master with commit 7d49f74 Apr 30, 2025
244 of 249 checks passed
@jsdw jsdw deleted the jsdw-frame-metadata-22 branch April 30, 2025 12:36
@ggwpez ggwpez removed the R0-no-crate-publish-required The change does not require any crates to be re-published. label May 2, 2025
github-merge-queue Bot pushed a commit that referenced this pull request May 6, 2025
The prdoc from #8327 did not reference all crates that were touched. I
added them here.
castillax pushed a commit 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.
castillax pushed a commit that referenced this pull request May 12, 2025
The prdoc from #8327 did not reference all crates that were touched. I
added them here.
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
The prdoc from #8327 did not reference all crates that were touched. I
added them here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T4-runtime_API This PR/Issue is related to runtime APIs.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants