Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

remove storage getters in FRAME #13365

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

muraca
Copy link
Contributor

@muraca muraca commented Feb 11, 2023

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
Copy link
Contributor Author

muraca commented Feb 11, 2023

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

@muraca muraca force-pushed the issue/13259 branch 7 times, most recently from 5538376 to e66328e Compare February 11, 2023 21:01
@xlc
Copy link
Contributor

xlc commented Feb 11, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

muraca commented Feb 12, 2023

No problem then, I'll revert those changes!

@muraca
Copy link
Contributor Author

muraca commented Feb 13, 2023

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

@muraca muraca force-pushed the issue/13259 branch 2 times, most recently from ef77421 to 2450e79 Compare February 20, 2023 20:59
@shawntabrizi shawntabrizi added C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit D2-breaksapi B1-note_worthy Changes should be noted in the release notes F3-breaks_API This PR changes public API; next release should be major. labels Feb 20, 2023
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks like the right direction to me

@shawntabrizi
Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi for the pallet-sudo, the definition for the Key is under super scope limitation.
Maybe you forget to remove the getter for the key, but all in all, if you plan to remove getter for the storage Key, please at least make the Key to be pub. Because in many projects including our, we use the sudo key to check something in other pallets. So that for this pr modification, we need to let the sudo key can be accessed from outside, I advise you to choose the following two way:

  1. let the storage Key to be public;
  2. write a function key() to return this storage, impl same logic as getter function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key function is not going to be removed as of now, but the auto-generated getter is being deprecated.
I can speculate about what might happen in the future, I think that somebody will "manually" implement such getters, and remove the pallet::getter from the pallets, but from what I understood it's going to take a while.
cc @kianenigma

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I mean when remove the getter for Key, please do not forget to let it be pub or implement the getter manually, otherwise we need to create another pr to add it.
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@muraca I see that this pallet-sudo already remove this getter, so please do not forget to add this getter by manual or let the Key be pub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not removed the #[pallet::getter(fn key)] line. See here.

Copy link
Contributor

@atenjin atenjin Apr 17, 2023

Choose a reason for hiding this comment

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

so can you add this function in this pallet-sudo? otherwise it will breaking many thing in our projects ? @muraca

pub fn key() -> Option<T::AccountId>{
    Key::<T>::get()
}

I think it's very necessary! otherwise there is no way to get the sudo key from this pallet!

@xlc
Copy link
Contributor

xlc commented Apr 12, 2023

why is this removing getters instead of deprecate them?

@muraca
Copy link
Contributor Author

muraca commented Apr 12, 2023

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

@xlc
Copy link
Contributor

xlc commented Apr 12, 2023

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

@bkchr
Copy link
Member

bkchr commented Apr 12, 2023

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
Copy link
Contributor Author

muraca commented Apr 12, 2023

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
Copy link
Member

bkchr commented Apr 12, 2023

Ahh yeah, thank you @muraca!

Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

ok I think you do the correct thing for most of the pallets but may have missed a few others.

My suggestion: always avoid hugh PR if possible. This is no reason that you need to update every pallet in a single PR. This makes review impossible (Github struggling to render diff and very time comsuming for reviews).

@@ -362,7 +362,6 @@ pub mod pallet {

/// The total units issued in the system.
#[pallet::storage]
#[pallet::getter(fn total_issuance)]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is still removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A method with the same name and return value is implemented in Currency and fungible::Inspect

frame/alliance/src/tests.rs Outdated Show resolved Hide resolved
frame/aura/src/lib.rs Show resolved Hide resolved
frame/aura/src/lib.rs Show resolved Hide resolved
frame/babe/src/lib.rs Show resolved Hide resolved
frame/babe/src/lib.rs Show resolved Hide resolved
frame/system/src/lib.rs Show resolved Hide resolved
frame/system/src/lib.rs Show resolved Hide resolved
frame/system/src/lib.rs Show resolved Hide resolved
frame/treasury/src/lib.rs Show resolved Hide resolved
frame/treasury/src/lib.rs Show resolved Hide resolved
@paritytech-cicd-pr
Copy link

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

@juangirini juangirini assigned juangirini and muraca and unassigned juangirini May 22, 2023
@juangirini juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jun 12, 2023
@stale
Copy link

stale bot commented Jul 12, 2023

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.

@stale stale bot added the A3-stale label Jul 12, 2023
@juangirini
Copy link
Contributor

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

@stale stale bot removed the A3-stale label Jul 12, 2023
@muraca
Copy link
Contributor Author

muraca commented Jul 12, 2023

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
Copy link

stale bot commented Aug 11, 2023

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.

@stale stale bot added the A3-stale label Aug 11, 2023
@stale stale bot removed the A3-stale label Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit F3-breaks_API This PR changes public API; next release should be major. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[Deprecation] remove storage getters in FRAME