Skip to content

removed pallet::getter from pallet-sudo #3370

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.

Sign up for GitHub

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

Jump to bottom
Merged
ggwpez merged 3 commits into from
Feb 19, 2024
Merged

removed pallet::getter from pallet-sudo #3370

ggwpez merged 3 commits into from
Feb 19, 2024

Conversation

@muraca
Copy link
Contributor

@muraca muraca commented Feb 17, 2024

@muraca muraca requested a review from a team as a code owner February 17, 2024 10:16
@muraca muraca mentioned this pull request Feb 17, 2024
Open
64 tasks
gupnik
gupnik approved these changes Feb 18, 2024
View reviewed changes
kianenigma
kianenigma reviewed Feb 19, 2024
View reviewed changes
@kianenigma kianenigma enabled auto-merge February 19, 2024 10:16
@kianenigma kianenigma added R0-no-crate-publish-required The change does not require any crates to be re-published. T1-FRAME This PR/Issue is related to core FRAME, the framework. T13-deprecation The current issue/pr is, or should be, part of a deprecation process. and removed T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Feb 19, 2024
@kianenigma kianenigma added this pull request to the merge queue Feb 19, 2024
@kianenigma kianenigma removed this pull request from the merge queue due to a manual request Feb 19, 2024
@kianenigma kianenigma removed the R0-no-crate-publish-required The change does not require any crates to be re-published. label Feb 19, 2024
@ggwpez ggwpez added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Feb 19, 2024
@ggwpez ggwpez added this pull request to the merge queue Feb 19, 2024
Merged via the queue into paritytech:master with commit 435e339 Feb 19, 2024
ordian added a commit that referenced this pull request Feb 19, 2024
@ordian
Merge branch 'master' into ao-collator-parent-head-data
739e63c 
* master: (41 commits)
  Add Coretime to Westend (#3319)
  removed `pallet::getter` from `pallet-sudo` (#3370)
  gossip-support: add unittests for update authorities (#3258)
  [FRAME Core] remove unnecessary overrides while using derive_impl for frame_system (#3317)
  Update coretime-westend bootnodes (#3380)
  `im-online` removal cleanup: remove off-chain storage (#2290)
  Bump the known_good_semver group with 1 update (#3379)
  Fix documentation dead link (#3372)
  Make `sp-keystore` `no_std`-compatible and fix the `build-runtimes-polkavm` CI job (#3363)
  Remove unused `im-online` weights (#3373)
  Ensure referenda `TracksInfo` is sorted (#3325)
  rpc server: add rate limiting middleware (#3301)
  do not block finality for "disabled" disputes (#3358)
  fix(zombienet): docker `img` version to use in merge queues for bridges (#3337)
  Various nits and alignments for SP testnets found during bumping `polkadot-fellows` repo (#3359)
  Add broker pallet to `coretime-westend` (#3272)
  remove recursion limit (#3348)
  Update subkey README.md (#3355)
  Bump the known_good_semver group with 6 updates (#3347)
  Document LocalKeystore insert method (#3336)
  ...
@muraca
Copy link
Contributor Author

muraca commented Feb 19, 2024

@ggwpez the silent label was removed as I had to write a prdoc because these are breaking changes

@ggwpez
Copy link
Member

ggwpez commented Feb 19, 2024 •
edited
Loading

How is this a breaking change?
Is the key function public or what?

@muraca
Copy link
Contributor Author

muraca commented Feb 20, 2024

All getters visibility is pub, while the storage items left behind might not have this visibility, for example here we have pub(super).
This is probably due to the fact that the key is managed by pallet logic and we don't want other runtime logic to modify it.

pallet_sudo::Key::<T>::get() is not accessible from other pallets.

@kianenigma
Copy link
Contributor

kianenigma commented Feb 20, 2024

Yeah, so I also first added Silent to these and added to the queue, but then removed them.

If we are to be pedantic, they are all breaking changes.

@muraca
Copy link
Contributor Author

muraca commented Feb 20, 2024

pallet_sudo::Key::<T>::get() is not accessible from other pallets.

what should we do with this issue?

@kianenigma
Copy link
Contributor

kianenigma commented Feb 20, 2024

One solution would be to just remove the usage of all getters in pallets one by one, but keep the getters.

This will not be breaking change in any way, and do 99% of the work.

Then, in one breaking PR, we mark the getters as deprecated and/or remove them.

@ggwpez ggwpez mentioned this pull request Feb 20, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2024
@ggwpez
nit: add prdoc for 3370 (#3401)
60563b8 
Did not realize that
#3370 was indeed
breaking. cc @muraca

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@muraca
Copy link
Contributor Author

muraca commented Feb 23, 2024

One solution would be to just remove the usage of all getters in pallets one by one, but keep the getters.

Keeping the getter would not solve the issue, if you want to not use it.
Say that a pallet is using the sudo key, for whatever reason. If not with a getter, how will this other pallet access such data?

The solutions I see are either pub for all the storage items where we remove a getter, or we manually implement getters for items that must not be changed by other logic, for example the sudo key.

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@muraca @kianenigma
removed pallet::getter from pallet-sudo (paritytech#3370)
1400ad5 
part of paritytech#3326 

@ggwpez @kianenigma @shawntabrizi

---------

Signed-off-by: Matteo Muraca <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
@nazar-pc nazar-pc mentioned this pull request Apr 5, 2024
Merged
1 task
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
@LayNath242 LayNath242 mentioned this pull request Dec 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kianenigma kianenigma kianenigma left review comments

@ggwpez ggwpez ggwpez approved these changes

+1 more reviewer

@gupnik gupnik gupnik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T13-deprecation The current issue/pr is, or should be, part of a deprecation process.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@muraca @ggwpez @kianenigma @gupnik