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.

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

Extend metadata with deprecation #4098

Open
2 tasks done
Tracked by #4520
rzadp opened this issue Apr 12, 2024 · 5 comments
Open
2 tasks done
Tracked by #4520

Extend metadata with deprecation #4098

rzadp opened this issue Apr 12, 2024 · 5 comments
Assignees
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T13-deprecation The current issue/pr is, or should be, part of a deprecation process.

Comments

@rzadp
Copy link
Contributor

rzadp commented Apr 12, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

As a dApp/tooling developer interacting with the blockchain through JS libraries and RPCs, there are no warnings or any information letting you know that a method you're using is deprecated.

No such information is included in metadata or RPC responses, so there is no way to programmatically read this information.

The only way to realize you're using a deprecated method is to follow the chain:

  • A polkadot-sdk release including a PR that marks a given method as deprecated.
  • A runtimes release that upgrades polkadot-sdk to this polkadot-sdk release.
  • An OpenGov referendum that upgrades the runtime with this runtimes release.
  • Finally, the approval and enactment of this referendum.

Only following this chain of releases you can realize that a method you're using without any warning or issues starts being deprecated.

It can fail for many reasons (either from insufficiently propagating deprecation/breaking-changes information down this list, or from developer failing to keep himself up to date). I posted some more thoughts here.

Because of that, I think that there is a lack of deprecation information that can be consumed programmatically when running a program and interacting with the chain.

Request

I'm requesting for deprecation information to be included in the metadata.
With that, the libraries such as PJS could read it and warn the developer that he's using a deprecated method automatically.

Solution

Solution M (for minimal)

In the metadata, there is a text field docs. It could be enforced to have a deprecated string in this text, if a method is marked as deprecated in Rust. That could be a start.

Solution A

Add a dedicated deprecated field to metadata for for calls, events, errors and storage items - as posted here by @kianenigma.

Solution B

Add a custom metadata field, which could also be used for this purpose with a specified custom metadata key - as posted here by @xlc.

Are you willing to help with this request?

Yes!

@rzadp rzadp added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T13-deprecation The current issue/pr is, or should be, part of a deprecation process. labels Apr 12, 2024
@kianenigma
Copy link
Contributor

Solution B sounds more reasonable to me, but before doing this I would like to discuss two things:

  1. We might not need a custom metadata attribute and instead stick to the normal docs. The docs for all Items (event, error, storage, call) are already in the metadata. If the keyword deprecated is present in them, we assume they are deprecated?

  2. No approach here is useful without a thumbs up from @TarikGul and co. that they will implement this in PJS-Apps + @jsdw that they will implement it in subxt.

@TarikGul
Copy link
Member

TarikGul commented May 7, 2024

So in regards to PJS, the docs provided in the metadata are augmented as comments as part of the call when its generated for the API. Example. So if there is a deprecation in the docs it will appear in the comments.

We might not need a custom metadata attribute and instead stick to the normal docs. The docs for all Items (event, error, storage, call) are already in the metadata. If the keyword deprecated is present in them, we assume they are deprecated?

I think this would need very specific formatting. For example, prefixing the deprecated string with some sort of symbol like !deprecated, this way we can differentiate between a sentence like This call will be deprecated in favor of x in y amount of time.

In regards to the other proposed solutions:

  • Solution A seems like the most straightforward way for tooling to adapt to the addition. But requires so the most work on the SDK side since it will affect the metadata format.

  • Solution B seems reasonable as well. But I am a bit confused on how the custom metadata would be read, and would there be a standard for a deprecation notice?

@jsdw
Copy link
Contributor

jsdw commented May 7, 2024

For 1, we could borrow from a format a bit like TSDoc or whatever and have @deprecated and other "documentation-like" things there (eg https://tsdoc.org/pages/tags/deprecated/)

@xlc
Copy link
Contributor

xlc commented May 7, 2024

we remove all the comments from metadata to reduce wasm size so M is not an option

@jsdw
Copy link
Contributor

jsdw commented May 8, 2024

Sorry; I was somewhat distracted yesterday and didn't elaborate much.

Overall, I'm generally in favour of adding deprecation tags to things in the metadata so that you get the nice tooling warnings, and in the long term would prob aim for A, eg adding a deprecated field to each call/tx/storage/constant.

A couple of questions if we were to go for A:

  1. Should we add a deprecrated field for entire pallets too? Probably?

  2. Is there a consensus on what exactly should be the value of such a field? We could add a simple bool, or we could provide more information eg:

    enum DeprecatedStatus {
        NotDeprecrated,
        DeprecatedSince { version: String, reason: String }
    }

    In the "not deprecated" case this is only 1 SCALE encoded byte per call/tx/storage/constant anyway, like a bool.

If we want to add this then we'll need to work towards a metadata V16. Since we don't want to release new metadata versions too often (IMO), we'd also want to gather up what else should make it into V16 (eg maybe also information to help reduce the config needed in tools like Subxt).

So if we want this sooner rather than later then maybe this is a good chance to use the custom metadata field first to add some information without needing to update to V16. For that, we could add a type something like this:

enum DeprecatedIdentifier {
    Pallet { index: u8 },
    Tx { pallet_index: u8, call_index: u8 },
    // .. enough info to point to the thing that's deprecated
}

struct DeprecatedStatuses {
    statuses: BTreeMap<DeprecatedIdentifier, DeprecatedStatus>
}

And then tooling can look up the relevant value for a given tx etc to see what its deprecation status is.

I'm expecting that we'll be looking at V16 metadata in Q3/Q4 and what to add etc this year, so perhaps it won't be tooo long!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T13-deprecation The current issue/pr is, or should be, part of a deprecation process.
Projects
Status: Draft
Development

No branches or pull requests

6 participants