remove storage getters in FRAME

[muraca]

resolves paritytech/polkadot-sdk#223

I added the deprecation message, removed every usage of #[pallet::getter] and refactored everything accordingly. Since a few getters for non-public maps were used in runtime or other situations, I manually wrote those functions and removed the getter. I generally used the pallet_name::StorageItem::<T>::get() syntax, I don't like the use pallet_name::StorageItem; because it's less explicit. I removed the getters from the examples, because since we don't want users to use them anymore, I think it's a better UX if we just don't mention them.

Companions: Cumulus paritytech/substrate#2216 Polkadot TBD

[muraca]

I'm sure I missed something, I'll let the tests run to see if there's any errors in pallets. I also think I need to add some info about the deprecation in the docs of the procedural macro. Any hints?

@shawntabrizi @kianenigma

[xlc]

Do we really want to do this like this? This is a big breaking change and will add significantly amount of work to downstream projects for no benefits.

It makes some sense to deprecate the getter syntax, but please don't remove the getters from pallets immediately.

[kianenigma]

Do we really want to do this like this? This is a big breaking change and will add significantly amount of work to downstream projects for no benefits.

It makes some sense to deprecate the getter syntax, but please don't remove the getters from pallets immediately.

Indeed, as per the original issue:

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

[muraca]

No problem then, I'll revert those changes!

[muraca]

Polkadot and Cumulus tests fail because of a missing SetMembersOrigin in pallet collective. It was added yesterday in PR paritytech/substrate#13159. I rebased and will let the checks tell me if something has broken.

[shawntabrizi]

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

I think this is how @muraca already approached this problem, but better to just manually implement all the getters on existing pallets, and no need to really mark anything as deprecated. Basically, this is just some legacy debt that probably will stick around, but has little impact.

[kianenigma]

Please don't close the linked issue with this PR, as we still need to revisit this and make sure we remove the code after some time has passed.

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/less-magic-in-frame-good-or-bad/2287/1

[kianenigma]

@muraca rebase please?

[altaua]

this needs to be rebased on current master, there have been breaking changes to the check-crate-publishing job.

[kianenigma]

@liamaharon / @gupnik if @muraca does not show up, can either of you push this to the finish line? Seems like it is mostly done.

We can mark PR as "made with ❤️ by PBA students" :))

[liamaharon]

@liamaharon / @gupnik if @muraca does not show up, can either of you push this to the finish line? Seems like it is mostly done.

We can mark PR as "made with ❤️ by PBA students" :))

I just spoke with @gupnik, I can take it on if we don't hear from @muraca before next week

[xlc]

why is this removing getters instead of deprecate them?

[muraca]

@xlc as per request, I added a deprecation message and removed every usage of the auto-generated functions

[xlc]

But the getter is still removed? There shouldn't be any companions PR required for this PR.

[bkchr]

But the getter is still removed? There shouldn't be any companions PR required for this PR.

Ahh yeah good point, we should first deprecate the usage before removing the actual calls.

[muraca]

But the getter is still removed?

@xlc I'm not sure I understood what you're asking. I'm not actually removing any of the getter functions, it just shows a warning message if you try to use one.

I was told I should not leave any warning behind, so I removed all the calls to said auto-generated functions in the FRAME pallets, and I did the same for the Cumulus repository. For the same reason, I had to replace some of the auto-generated getters, for example Aura's fn authorities, because it is used somewhere you can't call pallet_aura::Authorities::<T>::get().

[bkchr]

Ahh yeah, thank you @muraca!

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs. Job name: test-linux-stable Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2702142

[stale[bot]]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[juangirini]

@muraca unfortunately this got behind master massively, would you take this over?

[muraca]

Hey @juangirini as far as I remember it was decided not to proceed with this deprecation at the moment. If you're sure that this should be done right now, please let me know, and I'll go through the code again in the next few days.

[stale[bot]]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.