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

Remove pallet::getter usage from the bounties and child-bounties pallets #4392

Merged

Conversation

PolkadotDom
Copy link
Contributor

As per #3326, removes pallet::getter usage from the bounties and child-bounties pallets. The syntax StorageItem::<T, I>::get() should be used instead.

Changes to one pallet involved changes in the other, so I figured it'd be best to combine these two.

cc @muraca

@PolkadotDom PolkadotDom requested a review from a team as a code owner May 6, 2024 15:03
@bkchr bkchr requested a review from ggwpez May 10, 2024 21:42
@bkchr bkchr added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label May 10, 2024
@bkchr bkchr enabled auto-merge May 10, 2024 21:42
@bkchr bkchr requested a review from bkontur May 10, 2024 21:43
Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

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

I would prefer in such case to provide a pub getters for storage items and have a minor bump for the crate.
These pallets are old, if someone using those public getters to read data, this will be a breaking change, and it will take some time for maintainers to get those getters back. It wont be a nice experience.
@bkchr @ggwpez what you think?

@muharem
Copy link
Contributor

muharem commented May 12, 2024

Also storage items are part of public API of a pallet, even if there are no getter for them. So I believe additional getters do not add much to maintenance.

@PolkadotDom
Copy link
Contributor Author

I would prefer in such case to provide a pub getters for storage items and have a minor bump for the crate. These pallets are old, if someone using those public getters to read data, this will be a breaking change, and it will take some time for maintainers to get those getters back. It wont be a nice experience. @bkchr @ggwpez what you think?

Is there some way that introducing these changes breaks systems in production? I was thinking people would just need to update their codebase if they depended on the getters, but not familiar with every part of this.

@bkchr
Copy link
Member

bkchr commented May 15, 2024

I was thinking people would just need to update their codebase

Yeah that is the case.

Generally I don't know if anyone build something on top of these crates and used these getters, but also the release notes are quite clear. Still a better way would have been to first deprecate these getters and then to remove them.

@PolkadotDom
Copy link
Contributor Author

Happy to deprecate first moving forward if that is preferred. Just let me know 👍

@bkchr
Copy link
Member

bkchr commented May 15, 2024

For me it is fine to remove them. This is not the first PR doing this, so it would be weird to stop now 🙈

@muharem
Copy link
Contributor

muharem commented May 16, 2024

The storage items actually are public. They can be accessed from outside of the pallet without getter. Sorry for confusion.

@bkchr bkchr added this pull request to the merge queue May 16, 2024
Merged via the queue into paritytech:master with commit 6487ac1 May 16, 2024
154 of 156 checks passed
@PolkadotDom PolkadotDom deleted the dom/remove-getter-pallet-bounties branch May 16, 2024 15:15
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this pull request May 27, 2024
…ets (paritytech#4392)

As per paritytech#3326, removes pallet::getter usage from the bounties and
child-bounties pallets. The syntax `StorageItem::<T, I>::get()` should
be used instead.

Changes to one pallet involved changes in the other, so I figured it'd
be best to combine these two.

cc @muraca

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants