paritytech
[Deprecation] remove storage getters in FRAME
... or for now mark them as deprecated, and just remove all usage in substrate.
Tracking issue: https://github.com/paritytech/polkadot-sdk/issues/3326
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.
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.
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.
Hi folks! I have two questions:
- What should the deprecation message look like? At the moment, I put "#[pallet::getter] is deprecated and will be unavailable in future versions.".
- Should I manually implement getters for all the Storage items which don't have a
pubidentifier? I think the auto-generated getters are set as public to be reachable from other pallets. Inside the pallet itself, I'll use theStorageName::<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.
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
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 🙈
I have been having a lot of second doubt about this given some recent discussions elsewhere.
I can see two arguments against getters:
- Code that is generated, which you don't write yourself, going against "explicitness". This also means more things that we have to "teach".
- 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
structrather 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.
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.
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.
@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?
@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 toItem::<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.
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.
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:
- https://github.com/paritytech/polkadot-sdk/issues/222
- https://github.com/paritytech/substrate/issues/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.
Yea I think we can move the creation of this deprecation-process into its own issue @kianenigma, or?
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 paritytech/polkadot-sdk#182
fwiw I think this is still worth doing, and worthy of a large tip. But it will be a hard PR to review, so one should brace themselves for a lot of merge conflicts :p
I might still be onboard to do this, if it is actually confirmed and non-controversial, and if we have a well defined way on how to proceed. I suggest to have an initial PR to add the deprecation message and allow deprecated in all the existing FRAME pallets. Then add a tracking issue for all pallets subject to the change, and handle them one by one in different PRs, to minimize the size of merge conflicts.
I think a simple starting point is just to get rid of it within the polkadot-sdk codebase.
Most people use getters because it is in the various templates and other pallets.
If we remove them, then people will likely use these things less.
It may not be that we even need to fully deprecate it if it is going to cause a huge community headache (although i would prefer that).
I just opened a new issue to track the work in all the various pallets.
According to the deprecation checklist I should add the deprecation message first. Is "pallet::getter will be removed after August 2024. Please use StorageItem::get instead." good? Should the PR for the deprecation message close this issue?
If you mark something as deprecated, then you also have to remove all usage - otherwise the CI will complain.
I'm fine with not following the deprecation checklist, due to the wide impact of this change
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.
Am not so convinced of this anymore. We are doing breaking changes to all our pallets for no obvious gain.
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