Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

[MOON-1460] fix migration for author-slot-filter #43

Merged
merged 4 commits into from
Apr 6, 2022

Conversation

nbaztec
Copy link
Contributor

@nbaztec nbaztec commented Apr 6, 2022

This PR changes the following in author-slot-filter pallet:

  • Fixes migration so the EligibilityValue is written even if old value was missing from storage (previously it would have ignored writing it).
  • Adds try-runtime feature so the migration can be tested
  • Adds default() method (for no-std in runtime) instead of name constant
  • Adds migration tests

@nbaztec nbaztec marked this pull request as ready for review April 6, 2022 14:26
impl EligibilityValue {
/// Default total number of eligible authors, must NOT be 0.
pub fn default() -> Self {
NonZeroU32::new_unchecked(50)
Copy link
Contributor

@notlesh notlesh Apr 6, 2022

Choose a reason for hiding this comment

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

In what cases is this value expected to be relevant? If it's mostly supposed to be a safe number, I'd set it to 100%. Nevermind, I forgot the whole point of this was to move away from using a percentage :)

}
let old_value =
migration::get_storage_value::<Percent>(PALLET_NAME, ELIGIBLE_RATIO_ITEM_NAME, &[]);
let new_value = old_value
Copy link
Contributor

Choose a reason for hiding this comment

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

It's more readable here to use an if let syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I prefer to avoid having to specify defaults in multiple places, hence went with the functional approach, but if the team prefers the procedural style, I can change it to.

               let new_value = if let Some(value) = old_value {
			let total_authors = <T as Config>::PotentialAuthors::get().len();
			let new_value = percent_of_num(value, total_authors as u32);
			NonZeroU32::new(new_value).unwrap_or(EligibilityValue::default())
		} else {
			EligibilityValue::default()
		};

Only disadvantage being a default specified in each branch. Alternatively we can remove the flatten (forgot about and_then), and specify default only once with:

               let new_value = old_value
			.and_then(|value| {
				let total_authors = <T as Config>::PotentialAuthors::get().len();
				let new_value = percent_of_num(value, total_authors as u32);
				NonZeroU32::new(new_value)
			})
			.unwrap_or(EligibilityValue::default());

thoughts?

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 prefer the functional approach, and_then is more readable here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do prefer it but only due to not having to define a default in each branch (which would then need to be tested to not diverge).
In general I'd like to go with whatever the majority of the team is comfortable with :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have always thought that the succinctness of if let some is lost when there is an else branch attached to it...

let old_value =
migration::get_storage_value::<Percent>(PALLET_NAME, ELIGIBLE_RATIO_ITEM_NAME, &[]);

let expected_value = old_value
Copy link
Contributor

Choose a reason for hiding this comment

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

It's more readable here to use an if let syntax

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants