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.

Sign up for GitHub

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

Jump to bottom

feat: safe XCM version migration + extend SafeCallFilter #1526

Merged
merged 9 commits into from
Sep 4, 2023
Merged

feat: safe XCM version migration + extend SafeCallFilter #1526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
08c7cbc
fix: allow gateway process msg for xcm
wischli Sep 4, 2023
7492614
feat: add safe xcm version migration
wischli Sep 4, 2023
3909880
fix: Allow process_msg for xcm transact
wischli Sep 4, 2023
deb9fa3
refactor: remove spec_version check
wischli Sep 4, 2023
cea48d4
refactor: stricter SafeCallFilter
wischli Sep 4, 2023
741dcca
Upgrade1020 LP follow-ups (#1525)
cdamian Sep 4, 2023
eb2b02f
fix: rm OrderBook safe call cause missing
wischli Sep 4, 2023
e5a89e8
fix: OrderBook missing in both runtimes
wischli Sep 4, 2023
418acde
fix: clippy
wischli Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion libs/primitives/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@ frame-support = { git = "https://github.com/paritytech/substrate", default-featu
frame-system = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "polkadot-v0.9.38" }
pallet-collective = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "polkadot-v0.9.38" }

# cumuluse primitives dependencies
# cumulus primitives dependencies
cumulus-primitives-core = { git = "https://github.com/paritytech/cumulus", default-features = false, branch = "polkadot-v0.9.38" }

# XCM primitives dependencies
xcm = { git = "https://github.com/paritytech/polkadot", default-features = false, branch = "release-v0.9.38" }

[features]
default = ["std"]
runtime-benchmarks = [
Expand All @@ -51,6 +54,7 @@ std = [
"pallet-collective/std",
"sp-consensus-aura/std",
"cumulus-primitives-core/std",
"xcm/std",
]
try-runtime = [
"frame-support/try-runtime",
Expand Down
3 changes: 3 additions & 0 deletions libs/primitives/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,9 @@ pub mod constants {
/// Transaction pallet. As per:
/// <https://github.com/PureStake/moonbeam/blob/fb63014a5e487f17e31283776e4f6b0befd009a2/primitives/xcm/src/ethereum_xcm.rs#L167>
pub const TRANSACTION_RECOVERY_ID: u64 = 42;

/// The safe XCM version of pallet-xcm, same as on relay chain
pub const SAFE_XCM_VERSION: u32 = xcm::opaque::v2::VERSION;
}

/// Listing of parachains we integrate with.
Expand Down
23 changes: 23 additions & 0 deletions runtime/altair/src/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ pub type UpgradeAltair1031 = (
crate::XcmpQueue,
// Low weight, bumps uninitialized storage version from v0 to v1
pallet_xcm::migration::v1::MigrateToV1<crate::Runtime>,
// Sets currently unset safe XCM version to v2
xcm_v2_to_v3::SetSafeXcmVersion,
);

/// The Upgrade set for Algol - it excludes the migrations already executed in
Expand Down Expand Up @@ -890,3 +892,24 @@ mod pool_system {
}
}
}

mod xcm_v2_to_v3 {
use super::*;
use crate::{PolkadotXcm, RuntimeOrigin};

pub struct SetSafeXcmVersion;

impl OnRuntimeUpgrade for SetSafeXcmVersion {
fn on_runtime_upgrade() -> Weight {
// Unfortunately, SafeXcmVersion storage is not leaked to runtime, so we can't
// do any pre- or post-upgrade checks
PolkadotXcm::force_default_xcm_version(
RuntimeOrigin::root(),
Some(cfg_primitives::SAFE_XCM_VERSION),
)
.unwrap_or_else(|_| log::error!("Failed to set safe XCM version on runtime upgrade, requires manual call via governance"));

RocksDbWeight::get().writes(1)
}
}
}
17 changes: 5 additions & 12 deletions runtime/altair/src/xcm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,17 @@ impl frame_support::traits::Contains<RuntimeCall> for SafeCallFilter {
fn contains(call: &RuntimeCall) -> bool {
matches!(
call,
RuntimeCall::System(
frame_system::Call::kill_prefix { .. } | frame_system::Call::set_heap_pages { .. },
) | RuntimeCall::Timestamp(..)
RuntimeCall::Timestamp(..)
| RuntimeCall::Balances(..)
| RuntimeCall::Session(pallet_session::Call::purge_keys { .. })
| RuntimeCall::Treasury(..)
| RuntimeCall::Utility(pallet_utility::Call::as_derivative { .. })
| RuntimeCall::Vesting(..)
| RuntimeCall::PolkadotXcm(
pallet_xcm::Call::limited_reserve_transfer_assets { .. }
) | RuntimeCall::CollatorSelection(
pallet_collator_selection::Call::set_desired_candidates { .. }
| pallet_collator_selection::Call::set_candidacy_bond { .. }
| pallet_collator_selection::Call::register_as_candidate { .. }
| pallet_collator_selection::Call::leave_intent { .. },
) | RuntimeCall::XcmpQueue(..)
) | RuntimeCall::XcmpQueue(..)
| RuntimeCall::DmpQueue(..)
| RuntimeCall::Proxy(..)
| RuntimeCall::LiquidityPoolsGateway(
pallet_liquidity_pools_gateway::Call::process_msg { .. }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure what we are filtering here. All these calls will be allowed, right?

  • We need to allow pallet_liquidity_pools_gateway::Call::process_msg { .. }
  • But I would rather not allow all of the above calls. Only some.

Why do we have them here and why did we choose them? ^^

Copy link
Contributor Author

@wischli wischli Sep 4, 2023 •
edited

Choose a reason for hiding this comment

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

These are the allowed calls, exactly. The above ones are mirroring the allowed ones on Polkadot and System parachains which also exist on our parachain(s). We can definitely be more restrictive!

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. As we are not a system parachain and do not want to be "controlled" by the relay I think we should revisit that. But lets keep it as is for now.

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 fully agree. In the end, it is much more restrictive than the current setting of allowing everything. I am happy to make another proposal and cut even more. My approach was to make the most open proposal first and then adjust based on your input.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still would opt for changing this. I would say remove everything and only add

  • calls of pallet-investments
  • batch and batch-all of pallet-utility
  • calls of pallet-order-book

Do see anything else that would be needed to generate an xcm message that would allow somebody to invest natively from other parachains?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. Then lets just allow the one that are safe, add the order book. But we must add pallet-investments soon in order to allow somebody from asset hub to invest directly.

I still would like to remove the system extrinsics though. Although, the origin is not correct. Not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we must add pallet-investments soon in order to allow somebody from asset hub to invest directly.

We can do this ones we have reasonable upper PoV bounds, most ideally real benchmarks. Beforehand, we are vulnerable to PoV excess and thus stalling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cea48d4

Copy link
Contributor Author

@wischli wischli Sep 4, 2023 •
edited

Choose a reason for hiding this comment

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

Had to remove the OrderBook call again as its not implemented for Altair or Centrifuge Runtime but marked it as TODO

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do this ones we have reasonable upper PoV bounds, most ideally real benchmarks. Beforehand, we are vulnerable to PoV excess and thus stalling.

Yeah, good catch. Thanks for being so thorough!

) // TODO: Enable later: | RuntimeCall::OrderBook(..)
)
}
}
Expand Down
7 changes: 4 additions & 3 deletions runtime/centrifuge/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,14 +438,14 @@ impl pallet_liquidity_pools::Config for Runtime {
type AdminOrigin = EnsureRootOr<TwoThirdOfCouncil>;
type AssetRegistry = OrmlAssetRegistry;
type Balance = Balance;
type BalanceRatio = Quantity;
type CurrencyId = CurrencyId;
type ForeignInvestment = Investments;
type GeneralCurrencyPrefix = cfg_primitives::liquidity_pools::GeneralCurrencyPrefix;
type OutboundQueue = FilteredOutboundQueue;
type Permission = Permissions;
type PoolId = PoolId;
type PoolInspect = PoolSystem;
type Rate = Rate;
type RuntimeEvent = RuntimeEvent;
type Time = Timestamp;
type Tokens = Tokens;
Expand All @@ -457,7 +457,7 @@ impl pallet_liquidity_pools::Config for Runtime {
}

type LiquidityPoolsMessage =
pallet_liquidity_pools::Message<Domain, PoolId, TrancheId, Balance, Rate>;
pallet_liquidity_pools::Message<Domain, PoolId, TrancheId, Balance, Quantity>;

/// The FilteredOutboundQueue serves as a filter for outbound LP messages that
/// we want to allow initially.
Expand Down Expand Up @@ -496,6 +496,7 @@ impl pallet_liquidity_pools_gateway::Config for Runtime {
type LocalEVMOrigin = pallet_liquidity_pools_gateway::EnsureLocal;
type MaxIncomingMessageSize = MaxIncomingMessageSize;
type Message = LiquidityPoolsMessage;
type OriginRecovery = crate::LiquidityPoolsAxelarGateway;
type Router = liquidity_pools_gateway_routers::DomainRouter<Runtime>;
type RuntimeEvent = RuntimeEvent;
type RuntimeOrigin = RuntimeOrigin;
Expand All @@ -507,7 +508,7 @@ impl pallet_liquidity_pools_gateway::Config for Runtime {
pub struct DummyInboundQueue;

impl InboundQueue for DummyInboundQueue {
type Message = pallet_liquidity_pools::Message<Domain, PoolId, TrancheId, Balance, Rate>;
type Message = LiquidityPoolsMessage;
type Sender = DomainAddress;

fn submit(_: Self::Sender, _: Self::Message) -> DispatchResult {
Expand Down
23 changes: 23 additions & 0 deletions runtime/centrifuge/src/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ pub type UpgradeCentrifuge1020 = (
crate::XcmpQueue,
// Low weight, bumps uninitialized storage version from v0 to v1
pallet_xcm::migration::v1::MigrateToV1<crate::Runtime>,
// Sets currently unset safe XCM version to v2
xcm_v2_to_v3::SetSafeXcmVersion,
);

mod asset_registry {
Expand Down Expand Up @@ -596,3 +598,24 @@ pub fn get_centrifuge_assets() -> Vec<(
),
]
}

mod xcm_v2_to_v3 {
use super::*;
use crate::{PolkadotXcm, RuntimeOrigin};

pub struct SetSafeXcmVersion;

impl OnRuntimeUpgrade for SetSafeXcmVersion {
fn on_runtime_upgrade() -> Weight {
// Unfortunately, SafeXcmVersion storage is not leaked to runtime, so we can't
// do any pre- or post-upgrade checks
PolkadotXcm::force_default_xcm_version(
RuntimeOrigin::root(),
Some(cfg_primitives::SAFE_XCM_VERSION),
)
.unwrap_or_else(|_| log::error!("Failed to set safe XCM version on runtime upgrade, requires manual call via governance"));

RocksDbWeight::get().writes(1)
}
}
}
22 changes: 7 additions & 15 deletions runtime/centrifuge/src/xcm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use orml_xcm_support::MultiNativeAsset;
use pallet_xcm::XcmPassthrough;
use polkadot_parachain::primitives::Sibling;
use runtime_common::{
xcm::{general_key, AccountIdToMultiLocation, FixedConversionRateProvider},
xcm::{general_key, AccountIdToMultiLocation, FixedConversionRateProvider, LpInstanceRelayer},
xcm_fees::{default_per_second, native_per_second},
};
use sp_core::ConstU32;
Expand Down Expand Up @@ -71,24 +71,17 @@ impl frame_support::traits::Contains<RuntimeCall> for SafeCallFilter {
fn contains(call: &RuntimeCall) -> bool {
matches!(
call,
RuntimeCall::System(
frame_system::Call::kill_prefix { .. } | frame_system::Call::set_heap_pages { .. },
) | RuntimeCall::Timestamp(..)
RuntimeCall::Timestamp(..)
| RuntimeCall::Balances(..)
| RuntimeCall::Session(pallet_session::Call::purge_keys { .. })
| RuntimeCall::Treasury(..)
| RuntimeCall::Utility(pallet_utility::Call::as_derivative { .. })
| RuntimeCall::Vesting(..)
| RuntimeCall::PolkadotXcm(
pallet_xcm::Call::limited_reserve_transfer_assets { .. }
) | RuntimeCall::CollatorSelection(
pallet_collator_selection::Call::set_desired_candidates { .. }
| pallet_collator_selection::Call::set_candidacy_bond { .. }
| pallet_collator_selection::Call::register_as_candidate { .. }
| pallet_collator_selection::Call::leave_intent { .. },
) | RuntimeCall::XcmpQueue(..)
) | RuntimeCall::XcmpQueue(..)
| RuntimeCall::DmpQueue(..)
| RuntimeCall::Proxy(..)
| RuntimeCall::LiquidityPoolsGateway(
pallet_liquidity_pools_gateway::Call::process_msg { .. }
) // TODO: Enable later: | RuntimeCall::OrderBook(..)
)
}
}
Expand Down Expand Up @@ -376,9 +369,8 @@ impl TryConvert<cfg_types::ParaId, EVMChainId> for ParaToEvm {
/// `Transact`. There is an `OriginKind` which can biases the kind of local
/// `Origin` it will become.
pub type XcmOriginToTransactDispatchOrigin = (
// TODO: Activate once gateway is in here.
// A matcher that catches all Moonbeam relaying contracts to generate the right Origin
// LpGatewayInstance<ParaToEvm, Runtime>,
LpInstanceRelayer<ParaToEvm, Runtime>,
// Sovereign account converter; this attempts to derive an `AccountId` from the origin location
// using `LocationToAccountId` and then turn that into the usual `Signed` origin. Useful for
// foreign chains who want to have a local sovereign account on this chain which they control.
Expand Down
7 changes: 3 additions & 4 deletions src/chain_spec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
#![allow(clippy::derive_partial_eq_without_eq)]

use altair_runtime::constants::currency::{AIR, MILLI_AIR};
use cfg_primitives::{currency_decimals, parachains, Balance, BlockNumber, CFG, MILLI_CFG};
use cfg_primitives::{
currency_decimals, parachains, Balance, BlockNumber, CFG, MILLI_CFG, SAFE_XCM_VERSION,
};
use cfg_types::{
fee_keys::FeeKey,
tokens::{
Expand Down Expand Up @@ -52,9 +54,6 @@ pub type CentrifugeChainSpec =
pub type DevelopmentChainSpec =
sc_service::GenericChainSpec<development_runtime::GenesisConfig, Extensions>;

/// The default XCM version to set in genesis config.
const SAFE_XCM_VERSION: u32 = xcm::prelude::XCM_VERSION;

/// Helper function to generate a crypto pair from seed
pub fn get_from_seed<TPublic: Public>(seed: &str) -> <TPublic::Pair as Pair>::Public {
TPublic::Pair::from_string(&format!("//{seed}"), None)
Expand Down