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

Removed pallet::getter usage from Polkadot Runtime pallets #3660

Merged
merged 23 commits into from
Apr 10, 2024

Conversation

muraca
Copy link
Contributor

@muraca muraca commented Mar 12, 2024

Part of #3326

@kianenigma @ggwpez

polkadot address: 12poSUQPtcF1HUPQGY3zZu2P8emuW9YnsPduA4XG3oCEfJVp

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
…nfiguration, disputes, paras_inherent}`

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
…heduler, session_info}`

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
…red`

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
@muraca muraca marked this pull request as ready for review March 12, 2024 20:08
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
@muraca muraca requested a review from liamaharon March 19, 2024 16:17
@@ -1018,8 +1018,8 @@ impl<T: Config> Pallet<T> {
let recipient_deposit = if system_channel { 0 } else { config.hrmp_recipient_deposit };

if request.confirmed {
if <paras::Pallet<T>>::is_valid_para(channel_id.sender) &&
<paras::Pallet<T>>::is_valid_para(channel_id.recipient)
if paras::Pallet::<T>::is_valid_para(channel_id.sender) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

If you ever get to explore https://github.com/trailofbits/dylint for our preferred syntax, I'd be very curious to hear about it :)

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

A bit of a painful review, but did it :D If need be, could have been broken down into smaller PRs.

@ordian @eskimor can you also do a review, or at least approve the direction, as it affects a lot of your code base.

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @kianenigma. I'm fine with this change assuming this has no effect on visibility of chain state on polkadot js, is that right?
Screenshot 2024-04-01 at 21 50 16

prdoc/pr_3660.prdoc Outdated Show resolved Hide resolved
@@ -512,8 +512,7 @@ pub mod pallet {
/// The active configuration for the current session.
#[pallet::storage]
#[pallet::whitelist_storage]
#[pallet::getter(fn config)]
pub(crate) type ActiveConfig<T: Config> =
pub type ActiveConfig<T: Config> =
Copy link
Member

Choose a reason for hiding this comment

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

could you clarify why the change in visibility (pub(crate) -> pub) is needed and what are the implications? AFAIU, pub(crate) provides a guarantee that external pallets won't be able to write into this storage (checked at compile time) or even read?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing previously we could specify different visibility for getters and setters and we'd lose this ability now.

Copy link
Member

Choose a reason for hiding this comment

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

I think getters are always 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.

Getters are always pub indeed, this change is required as it is the only way we currently have, other than writing manually all getters.

@kianenigma
Copy link
Contributor

Thanks for the ping @kianenigma. I'm fine with this change assuming this has no effect on visibility of chain state on polkadot js, is that right? Screenshot 2024-04-01 at 21 50 16

Yes, removing the getters is a 100% noop in the metadata and the external API.

@kianenigma
Copy link
Contributor

@muraca ready for last upstream sync and then we can merge.

@paritytech-cicd-pr
Copy link

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

muraca and others added 2 commits April 6, 2024 14:42
@ordian ordian added the T2-pallets This PR/Issue is related to a particular pallet. label Apr 10, 2024
@ordian ordian added this pull request to the merge queue Apr 10, 2024
Merged via the queue into paritytech:master with commit 92e1425 Apr 10, 2024
133 of 138 checks passed
@muraca
Copy link
Contributor Author

muraca commented Apr 10, 2024

tip?

@kianenigma
Copy link
Contributor

/tip medium

Copy link

@kianenigma A referendum for a medium (80 DOT) tip was successfully submitted for @muraca (12poSUQPtcF1HUPQGY3zZu2P8emuW9YnsPduA4XG3oCEfJVp on polkadot).

Referendum number: 666.
tip

Copy link

The referendum has appeared on Polkassembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants