-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
someone fixed that :). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
impl<T: Config<I>, I: 'static> GenesisBuild<T, I> for GenesisConfig<T, I> { | ||
fn build(&self) { | ||
Pot::<T, I>::put(self.pot); | ||
MaxMembers::<T, I>::put(self.max_members); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a warning if self.max_members >= self.members.len()
? cc @thiolliere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and we should even panic during genesis, this will happen when we implement the bounding of storages.
But as it wasn't done before I think we shouldn't block on this.
@@ -552,8 +713,8 @@ decl_module! { | |||
/// | |||
/// Total Complexity: O(M + B + C + logM + logB + X) | |||
/// # </weight> | |||
#[weight = T::BlockWeights::get().max_block / 10] | |||
pub fn bid(origin, value: BalanceOf<T, I>) -> DispatchResult { | |||
#[pallet::weight(T::BlockWeights::get().max_block / 10)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope of this PR, but these weight seems strange - anyone know if this pallet be properly benchmarked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it should be done, but the weight is probably an upper bound for now, at least for our usage in kusama I guess, so maybe not the priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
few small comments/questions but I think they are out of scope of this PR
bot merge |
Trying merge. |
relates: #7882
Following the upgrade guidelines here: https://crates.parity.io/frame_support/attr.pallet.html#upgrade-guidelines.
From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines
So users of the
Society
pallet must be careful about the name they used inconstruct_runtime!
. Hence the runtime-migration label, which might not be needed depending on the configuration of theSociety
pallet.only kusama uses the pallet
Society
kusama uses the name
Society
inconstruct-runtime!
so no need for migration