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 from pallet-staking #6184

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Zebedeusz
Copy link
Contributor

@Zebedeusz Zebedeusz commented Oct 23, 2024

Description

Part of #3326
Removes all pallet::getter occurrences from pallet-staking and replaces them with explicit implementations.
Adds tests to verify that retrieval of affected entities works as expected so via storage::getter.

Review Notes

  1. Traits added to the derive attribute are used in tests (either directly or indirectly).
  2. The getters had to be placed in a separate impl block since the other one is annotated with #[pallet::call] and that requires #[pallet::call_index(0)] annotation on each function in that block. So I thought it's better to separate them.

@Zebedeusz Zebedeusz added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 23, 2024
@Zebedeusz Zebedeusz marked this pull request as ready for review October 23, 2024 08:44
@Zebedeusz Zebedeusz requested a review from a team October 23, 2024 09:34
@@ -575,7 +575,7 @@ impl<T: Config> Pallet<T> {

let era_duration = (now_as_millis_u64.defensive_saturating_sub(active_era_start))
.saturated_into::<u64>();
let staked = Self::eras_total_stake(&active_era.index);
let staked = Self::eras_total_stake(active_era.index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let staked = Self::eras_total_stake(active_era.index);
let staked = ErasTotalStake::<T>::get(&active_era.index);

Part of the aim here is to replace any usage of Self::storage() with Storage::<T>::get() in the sdk codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it's done. I replaced these usages everywhere I was able to spot them.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 25, 2024 13:48
}

/// Get the preferences of a given validator.
pub fn validators<EncodedAccountId>(account_id: EncodedAccountId) -> ValidatorPrefs
Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma cargo expand shows that FRAME macros actually expand to these (seemingly over-) generic function signatures, is this just to allow args to be passed as a ref or value, or is there another reason? I guess we also need them here to not be a breaking change, or can we make assumptions about usage here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it was the goal, it is mostly useful for types like tuples where you can pass a tuple of reference.

Here we can probably take a reference but it would be a breaking change indeed. Some people might be giving a reference some other might be giving the type itself.

The generic should be name EncodeLikeAccountId, because it is not yet encoded.

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.

3 participants