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

[FRAME] Pallet storage items should be public #3787

Closed
ggwpez opened this issue Mar 21, 2024 · 6 comments
Closed

[FRAME] Pallet storage items should be public #3787

ggwpez opened this issue Mar 21, 2024 · 6 comments
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@ggwpez
Copy link
Member

ggwpez commented Mar 21, 2024

Since we are removing getters, it would be useful to make the storage items of all pallets public.
Otherwise it is not easily possible to test stuff where accessing another pallets' storage would be useful.

I guess we could issue a warning on private storage items, but maybe that is too opinionated for everyone.

@ggwpez ggwpez added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Mar 21, 2024
@liamaharon
Copy link
Contributor

Good pick up, yeah I can't think of a good reason for storage to ever be private/inaccessible.

@gupnik
Copy link
Contributor

gupnik commented Mar 25, 2024

There might be a few instances where we want to restrict direct access to the underlying storage and instead only allow the public APIs to modify it. For example, something like bags-list should only be modified via these APIs to ensure that the linked storage items stay in sync.

One of the motivations for pallet splitting work was to make this encapsulation better, but making these public by default would take us even farther.

Can we only make these public for tests if that's the primary reason here?

@ggwpez
Copy link
Member Author

ggwpez commented Mar 25, 2024

Can we only make these public for tests if that's the primary reason here?

Tests and benchmarks, yes. I guess we could just modify the visibility for those features?

@rainbow-promise
Copy link

Hello @ggwpez is there an instance of this?

@ggwpez
Copy link
Member Author

ggwpez commented Sep 20, 2024

I think we went with just making all storage items pub, no need to shield them.
Most MRs that did this also removed the getters, should be worthwhile to do both at once: #3326

@ggwpez
Copy link
Member Author

ggwpez commented Sep 20, 2024

Actually going to close in favour of #3326

@ggwpez ggwpez closed this as completed Sep 20, 2024
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
Status: Done
Development

No branches or pull requests

4 participants