[MOON-1460] change eligibility ratio to be a number #38
Conversation
| #[pallet::getter(fn eligible_ratio)] | ||
| pub type EligibleRatio<T: Config> = StorageValue<_, Percent, ValueQuery, Half<T>>; | ||
| #[pallet::getter(fn eligible_count)] | ||
| pub type EligibleCount<T: Config> = StorageValue<_, EligibilityValue, ValueQuery, Half<T>>; |
There was a problem hiding this comment.
This implies that a migration is necessary, it must also be decided how the percentage is translated into number during the migration, @alan do you have an idea on this?
There was a problem hiding this comment.
Since this might affect block production I suggest we keep the previous storage, and add the new EligibleCount in a new storage. We can in the future remove EligibleRatio
There was a problem hiding this comment.
Why do we want to keep the previous storage? How is it helping ?
There was a problem hiding this comment.
In general I am hesitant of migrating a storage prefix involved in block production without keeping the previous storage. I think that has been our approach so far with every data migration we have performed if we suspected it could affect block production
There was a problem hiding this comment.
Ok, we could mark it as deprecated. But I don't think it makes anything safer in terms of block production
|
I am having trouble figuring out why this brings and advantage over what was before. I dont understand why the previous approach could not handle more than 100 authors, would you mind explaining? |
|
I think it was mentioned that if the total author count was |
|
Ok got it thanks to @librelois, it is a precision issue right? |
| Origin::root(), | ||
| value.clone() | ||
| )); | ||
| assert_eq!(AuthorSlotFilter::eligible_count(), value) | ||
| }); |
There was a problem hiding this comment.
Can you add one more test which tests the migration? You can take https:/PureStake/moonbeam/blob/c582e2f34a2271a22e60a73dbc6d4ffde90de837/pallets/xcm-transactor/src/tests.rs#L319 as a reference
There was a problem hiding this comment.
added, please review.
Updates eligibility ratio to be a number to handle cases of more than
100authors