-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
The PR is ready to be reviewed now. Still needs to be confirmed, but it seems the chess pallet is the responsable for the wasm exceed during execution. |
pallets/maintenance-mode/src/lib.rs
Outdated
#[pallet::genesis_build] | ||
impl<T: Config> GenesisBuild<T> for GenesisConfig { | ||
fn build(&self) { | ||
MaintenanceModeStatus::<T>::put(ACTIVATED); |
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.
what is the reason for having it activated at the start?
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.
so we can have the chance to check that everything is fine before making the operations available.
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.
I got it, but it doesn't seem to be responsibility of the pallet. This decision (on or off at the start) should be taken on a composition root level. My suggestion here is to add activated
into the genesis config
#[pallet::genesis_config]
/// Genesis config for lockdown mode pallet
pub struct GenesisConfig<T: Config> {
pub activated: bool,
pub _phantom: PhantomData<T>,
}
impl<T: Config> Default for GenesisConfig<T> {
fn default() -> Self {
Self { activated: ACTIVATED, _phantom: PhantomData }
}
}
#[pallet::genesis_build]
impl<T: Config> GenesisBuild<T> for GenesisConfig<T> {
fn build(&self) {
LockdownModeStatus::<T>::put(&self.activated);
}
}
this will also let you to set initial state for you tests
pub fn new_test_ext(activated: bool) -> sp_io::TestExternalities {
let mut storage = system::GenesisConfig::default().build_storage::<Test>().unwrap();
pallet_lockdown_mode::GenesisConfig::<Test> { activated, _phantom: PhantomData }
.assimilate_storage(&mut storage)
.unwrap();
storage.into()
}
So that you don't need to activate lockdown mode first to deactivate it in the next step.
You can also add a test to make sure that the storage is initialized properly
#[test]
fn initialized() {
[true, false].into_iter().for_each(|expected| {
new_test_ext(expected).execute_with(|| {
let lockdown_mode = LockdownModeStatus::<Test>::get();
assert_eq!(lockdown_mode, expected);
});
});
}
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.
@hbulgarini I think @kalaninja makes sense. If you can apply those changes, we should be good then.
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.
@hbulgarini I think calling this pallet |
@stiiifff @kalaninja @valentinfernandez1 , i have addressed all the comments. |
@kalaninja , i have to solve some conflicts on this branch because of the merge of the PR #165, could you please make sure that something was not incorrectly solved during the sync? Then if everything looks fine, you can just approve and merge. @stiiifff already approved it but due to the re-sync the approval was missed. |
@hbulgarini approved but I think it'd be nice to implement @kalaninja 's suggestion (see above). |
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. Let's create a new issue with @kalaninja's improvement suggestion.
This PR implements a lockdown mode pallet inspired in the Moonbeam maintenance pallet implementation.
Specs can be found here: #144 (comment)
Closes #144