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

tx-pause and safe-mode pallets integration - Shibuya #1382

Conversation

sylvaincormier
Copy link

@sylvaincormier sylvaincormier commented Nov 7, 2024

Date: Thu Nov 7 08:10:27 2024 -0500

Integrate SafeMode pallet into Shibuya runtime
- Added pallet_tx_pause to runtime configuration.
- Added `pallet_safe_mode` to runtime configuration.
- Updated genesis configuration to include `safe_mode`.
- Configured `SafeModeWhitelistedCalls` to define calls that bypass SafeMode restrictions.
- Added parameter types for SafeMode configuration, including enter and extend durations, deposit amounts, and release delay.
- Updated `construct_runtime!` macro to include SafeMode pallet at index

Pull Request Summary

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • added benchmarks & weights for any modified runtime logics.

Date:   Thu Nov 7 08:10:27 2024 -0500

    Integrate SafeMode pallet into Shibuya runtime
    - Added pallet_tx_pause to runtime configuration.
    - Added `pallet_safe_mode` to runtime configuration.
    - Updated genesis configuration to include `safe_mode`.
    - Configured `SafeModeWhitelistedCalls` to define calls that bypass SafeMode restrictions.
    - Added parameter types for SafeMode configuration, including enter and extend durations, deposit amounts, and release delay.
    - Updated `construct_runtime!` macro to include SafeMode pallet at index
Copy link
Contributor

@ipapandinas ipapandinas left a comment

Choose a reason for hiding this comment

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

This first version is promising—good job! I've left comments on the config parameters and the master branch rebase.

Additionally, from my analysis of these pallets, it appears you forgot to extend the BaseCallFilter type in the frame_system config to include whitelisted calls. You can define a new type alias, SafeModeTxPauseFilter, and use it as follows:

type SafeModeTxPauseFilter = InsideBoth<SafeModeWhitelistedCalls, TxPauseWhitelistedCalls>;

type BaseCallFilter = InsideBoth<BaseFilter, SafeModeTxPauseFilter>;

@@ -1,6 +1,6 @@
[package]
name = "astar-collator"
version = "5.45.0"
version = "5.44.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to have the latest master branch rebased, otherwise it reverts existing work

@@ -153,6 +154,8 @@ substrate-wasm-builder = { workspace = true, optional = true }
[features]
default = ["std"]
std = [
"pallet-tx-pause/std",
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add them to runtime-benchmarks & try-runtime features below too

@@ -1522,7 +1526,13 @@ parameter_types! {
impl pallet_migrations::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
#[cfg(not(feature = "runtime-benchmarks"))]
type Migrations = ();
type Migrations = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Those migrations have already been executed on Shibuya, and will be deleted once you have rebased the master branch

@@ -1655,7 +1667,7 @@ pub type Executive = frame_executive::Executive<
pub type Migrations = (Unreleased, Permanent);

/// Unreleased migrations. Add new ones here:
pub type Unreleased = ();
pub type Unreleased = (cumulus_pallet_xcmp_queue::migration::v5::MigrateV4ToV5<Runtime>,);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, it has already been applied

@@ -1608,6 +1618,8 @@ construct_runtime!(
CollectiveProxy: pallet_collective_proxy = 109,

MultiBlockMigrations: pallet_migrations = 120,
TxPause: pallet_tx_pause = 200,
Copy link
Contributor

Choose a reason for hiding this comment

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

A small gap is ok, we can go with SafeMode first:

SafeMode: pallet_safe_mode = 130,
TxPause: pallet_tx_pause = 131,

type PauseOrigin = EnsureRoot<AccountId>; // Authority required to pause transactions.
type UnpauseOrigin = EnsureRoot<AccountId>; // Authority required to unpause transactions.
type WhitelistedCalls = ();
type MaxNameLen = ConstU32<32>; // Maximum length of name, adjust as needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

"256" is probably better here: type MaxNameLen = ConstU32<256>

type RuntimeEvent = RuntimeEvent;
type PauseOrigin = EnsureRoot<AccountId>; // Authority required to pause transactions.
type UnpauseOrigin = EnsureRoot<AccountId>; // Authority required to unpause transactions.
type WhitelistedCalls = ();
Copy link
Contributor

Choose a reason for hiding this comment

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

In the similar way you did for the pallet_safe_mode, can you create a TxPauseWhitelistedCalls type?

An example (but you can check if other pallets can be whitelisted):

/// Calls that can bypass the tx-pause pallet.
/// We always allow system calls and timestamp since it is required for block production
pub struct TxPauseWhitelistedCalls;
impl Contains<pallet_tx_pause::RuntimeCallNameOf<Runtime>> for TxPauseWhitelistedCalls {
	fn contains(full_name: &pallet_tx_pause::RuntimeCallNameOf<Runtime>) -> bool {
		matches!(full_name.0.as_slice(), b"Sudo" | b"System" | b"Timestamp" | b"TxPause")
	}
}

@@ -1194,7 +1194,13 @@ parameter_types! {
impl pallet_migrations::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
#[cfg(not(feature = "runtime-benchmarks"))]
type Migrations = ();
type Migrations = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -1333,7 +1339,11 @@ parameter_types! {
pub type Migrations = (Unreleased, Permanent);

/// Unreleased migrations. Add new ones here:
pub type Unreleased = ();
pub type Unreleased = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

impl Contains<RuntimeCall> for SafeModeWhitelistedCalls {
fn contains(call: &RuntimeCall) -> bool {
match call {
RuntimeCall::System(_) | RuntimeCall::SafeMode(_) | RuntimeCall::TxPause(_) => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add Sudo too

@ipapandinas ipapandinas added shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Nov 7, 2024
@ipapandinas ipapandinas linked an issue Nov 7, 2024 that may be closed by this pull request
5 tasks
@ipapandinas ipapandinas changed the title 1364-integrate-tx-pause-safe-mode tx-pause and safe-mode pallets integration - Shibuya Nov 7, 2024
- Added `SafeModeTxPauseFilter` combining SafeMode and TxPause whitelisted calls.
- Updated `BaseCallFilter` to include the combined filter for comprehensive runtime restrictions.
- Reordered pallet indices in `construct_runtime!` for SafeMode and TxPause (SafeMode now precedes TxPause).
- Implemented `TxPauseWhitelistedCalls` to allow specific calls to bypass TxPause restrictions (e.g., Sudo, System, Timestamp).
- Updated `SafeModeWhitelistedCalls` to include `Sudo`.
- Extended `MaxNameLen` from 32 to 256 in TxPause configuration.

Addresses comments on runtime configurations and ensures correct integration of SafeMode and TxPause functionalities.
@sylvaincormier
Copy link
Author

@ipapandinas, thank you for taking the time to provide such thorough feedback and detailed examples! I've addressed all the points you raised:

Updated BaseCallFilter to incorporate both SafeModeWhitelistedCalls and TxPauseWhitelistedCalls.
Created TxPauseWhitelistedCalls to ensure critical calls like Sudo, System, Timestamp, and TxPause are whitelisted.
Extended SafeModeWhitelistedCalls to include Sudo.
Reordered the SafeMode and TxPause pallets as suggested.
Increased MaxNameLen to 256.

I also rebased onto the latest master as you recommended. Let me know if there's anything else you need me to adjust. I appreciate your guidance throughout this process!

…igration setup

- Added `pallet-tx-pause` and `pallet-safe-mode` to runtime-benchmarks and try-runtime features in `Cargo.toml`.
- Imported `EnsureWithSuccess` and `EitherOfDiverse` to enhance the configuration of emergency authorities.
- Removed outdated lazy migrations for `pallet_dapp_staking` and `vesting_mbm` as they have already been executed.
- Cleared `Unreleased` and `Permanent` migrations, indicating their completion.
- Introduced `TxPauseWhitelistedCalls` struct for better granularity in whitelisted calls during transaction pause operations.
@ipapandinas
Copy link
Contributor

@sylvaincormier Thank you for your work on this contribution! For now, we’ll close this pull request, but we appreciate your effort and may revisit these ideas in the future. Thanks again for your understanding!

@sylvaincormier
Copy link
Author

sylvaincormier commented Nov 13, 2024

Hi Ipapandinas.
I was not done!
I just finished this:
Author: Silvereau [email protected]
Date: Wed Nov 13 06:02:18 2024 -0500

feat(runtime): add EmergencyAuthority origin types

Define origin types for emergency operations:
- MoreThanHalfCouncil for council majority actions
- EmergencyAuthority combining council and technical committee control
- EmergencyAuthorityWithSuccess for time-sensitive operations

These types enable proper governance control over emergency features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants