[Deprecation] remove storage getters in FRAME

[kianenigma]

... or for now mark them as deprecated, and just remove all usage in substrate.

Tracking issue: #3326

[bkchr]

Which storage getters? You mean the get(fn something) attribute?

[bkchr]

If yes, what you expect as replacement? Just a normal function?

[kianenigma]

Yes, manually implemented if needed. getters are one of the last pieces of "magic code" that FRAME macros generate and the less of that we have, the better.

[kianenigma]

@muraca will work on this.

[shawntabrizi]

I have also found it easier to audit storage access by searching for StorageName::<T>::. This getter pattern can obfuscate that.

I think the world would be better without it.

[ruseinov]

I have also found it easier to audit storage access by searching for StorageName::<T>::. This getter pattern can obfuscate that.

I think the world would be better without it.

Arguably it's just a matter of adding #[pallet::getter( pattern into the mix, but tbh I totally agree that the micro-convenience of this is probably not worth it. If there's too much bloat - implementing this is quite easy. The cool thing about this macro though is that allows for not supplying the type, because even if we implement this by hand, it'd still need to be something like get_blah::<T>. That said, visibility is more important.

[muraca]

Hi folks! I have two questions:

1. What should the deprecation message look like? At the moment, I put "#[pallet::getter] is deprecated and will be unavailable in future versions.".
2. Should I manually implement getters for all the Storage items which don't have a pub identifier? I think the auto-generated getters are set as public to be reachable from other pallets. Inside the pallet itself, I'll use the StorageName::<T>:: syntax anyway.
   edit: There are lots of storage items which have getters used by other pallets. At this point, I think it would make sense to manually implement all the getters I'm removing, because giving pub access to storage items implies that external pallets could also write on them.

[bkchr]

I have also found it easier to audit storage access by searching for StorageName::<T>::

That will not bring you that much as you can also do <StorageName<T>>::get() :P

[kianenigma]

I have also found it easier to audit storage access by searching for StorageName::<T>::

That will not bring you that much as you can also do <StorageName<T>>::get() :P

Needing to keep an eye for 2 patterns is still less than 3 🙈

[bkchr]

I don't wanted to argue against this issue :P

[kianenigma]

I have been having a lot of second doubt about this given some recent discussions elsewhere.

I can see two arguments against getters:

1. Code that is generated, which you don't write yourself, going against "explicitness". This also means more things that we have to "teach".
2. Harder audit. I think it is a mental help for reading FRAME code to actually distinguish between a function that reads storage with a slightly different syntax, directly referring the storage struct rather than a function.

The former is not a particularly strong argument, and if you really want to go down that rabbit whole, there is a lot of such code in FRAME and it is futile to pursue explicit-ness in an absolutist manner.

The latter is a stronger argument, but I am having a hard time convincing myself that is is the right fit for everyone. If I/some Parity developers don't like getters, we can simply not use them internally. Reviewing external code would still be a pain for me, but that would be my problem. But, IFF we could gather the data to argue that majority of FRAME developers think similar to me, then we could argue that the readability of code improves across the ecosystem if we agree on one paradigm.

[gavofyork]

FWIW I originally introduced this and I no longer use it prefering instead consistency of the ::get() API. I'm happy to mark as deprecated and remove usages.

[KiChjang]

I believe that there is a use case for getters, and that is that of publicity -- you may have a storage item that is private or crate-public, but you also want people to be able to publicly get the value under the storage item, so you expose the getter. In short, the getter attribute is useful when you only want to expose the read functionality, but nothing else.

[muraca]

@KiChjang for that you can manually implement a getter function.

[xlc]

@KiChjang for that you can manually implement a getter function.

That's not a good argement. Many languages offers syntax sugar to generate getter and you can't just say those features should be removed because it is possible to manually write the equivalent code. I don't want to write those code.

The fact that this feature is used in many places means people wants to use them. So why be the bad guy taking away a feature used by people?

[gavofyork]

@xlc Having the feature does not come for free. Compared to using the Item::get() API (rather than Self::get_item()) It's extra code both in Substrate (to support the macro) and in pallets (the attribute macro).

So the question is not "people are using it; why remove it?" (though admittedly this can reasonably place a time limit on the deprecation timeline). It's instead "what is the benefit of the feature, and what cost does it bring?"; in this case the only real benefit appears to be the ability to have a public getter with all other access of the item (e.g. mutating) being restricted.

The cost is code inconsistency:

• the same operation can be done in two ways - a pallet might use Item::<T>::get(), Self::get_item() or a mix of the two;
• you might assume that Self::get_item() is exactly equivalent to Item::<T>::get() however this is not necessarily the case. This might lead to errors in review or audit.

As well as additional complexity in the pallet and substrate to support the feature.

Personally, I'm unconvinced that the benefit outweighs the cost. Perhaps there are other unspecified benefits (if so it would be good to bring these up). If the feature is determined to be not worth the cost, then the points of deprecation and final removal (as well as a possible compatibility path like perhaps a frame_compat crate with a macro to do something similar) should be properly discussed and have a timeline of length corresponding to the community's dependence on the feature.

[xlc]

Compatibility path will be very helpful.

I can live without this feature. It is mostly I prefer to not touch code that aren’t broken. Proper deprecation is a must and more discussion with wider audiences will be great.

[kianenigma]

I can live without this feature. It is mostly I prefer to not touch code that aren’t broken. Proper deprecation is a must and more discussion with wider audiences will be great.

We can make this be a pilot for learning a mature deprecation path. I know that this has also been discussed internally recently, @ggwpez might be able to chime in. At the moment, our communication is solely based on PRs and the note_worthy label. Usually, we mark a feature as deprecated in substrate and remove usage within our codebase to satisfy the CI. Other teams will see the warning and will have time to remove it. Within a few months, we actually remove the code. At the moment, I can see two issues in this pipeline:

• [Deprecation] deprecate trait Store in FRAME storage #222
• [Deprecation] Deprecate decl_* macros substrate#12248

What else we can do?

One thing that comes to mind is making sure such PRs make it into the Substate Developer Newsletter, maintained by @sacha-l, although I am sure he already takes all the PRs that are note-worthy labeled.

---

Moreover, while we have a few experts in this thread, it would be good to get a few more opinions. This could be a good task for @juangirini and @aaronbassett to take over: find the right means to do some kind of soft-voting within FRAME developers in the ecosystem to see what the sentiment is.

[ggwpez]

Yea I think we can move the creation of this deprecation-process into its own issue @kianenigma, or?

[juangirini]

Yea I think we can move the creation of this deprecation-process into its own issue @kianenigma, or?

@ggwpez @kianenigma @aaronbassett I moved this conversation to a separate issue #182

[ggwpez]

If you mark something as deprecated, then you also have to remove all usage - otherwise the CI will complain.

[muraca]

I'm fine with not following the deprecation checklist, due to the wide impact of this change

[ggwpez]

Yea we can just start with our pallets one-by-one or doing rarely modified ones first and then adding the deprecation once we removed it here.

[ggwpez]

Am not so convinced of this anymore. We are doing breaking changes to all our pallets for no obvious gain.

[xlc]

Am not so convinced of this anymore. We are doing breaking changes to all our pallets for no obvious gain.

100%

we can do this is there are no other work left, which is clearly not the case. there are many CRITICAL open issues and people choose to break code and add more workload to everyone else instead

[shawntabrizi]

• There is nothing breaking about these changes if done right.
  • Just as a pub fn get_variable in impl Pallet
• It prevents more macro magic and needing to explain people how frame does stuff
• It prevents confusion for many people in the contract space which think these getters are needed to "read storage"
• It is a great open issue for brand new contributors to make their first pr
  • so it think it is not in conflict at all with people who actually work on critical open issues

AFAIK, we took the path of deprecation notice for this feature. I don't see how any pallet takes more than 15 min to update.

[xlc]

yeah sure except people are making breaking changes and the breaking changes are merged

[shawntabrizi]

Sounds like a problem of developers and reviewers. Let's point out those problems, not point fingers at a good initiative.

[kianenigma]

@xlc

Going forward, we should merge these without breaking change. The new CI warning you about it being a breaking change is enough deterrant to not want to break things needlessly.

There is no one working on this who could have solved anything more important. This initiaitve is a simple one for juniors to get their first contributions into Polkadot, and our support and review of it is value. Putting a stop gap on this is also not going to solve anything. I spend a few minutes every week reviewing this, and other should do the same.

In fact, the same CI system and semver-based release is one of those CRITICAL things that we have indeed solved ;)