feat: safe XCM version migration + extend SafeCallFilter #1526

wischli · 2023-09-04T12:24:41Z

For both the Centrifuge as well as the Altair runtimes adds:

Migration to set safe XCM version for Centrifuge and Altair chains to v2 (mirroring behaviour of relay and other parachains)
pallet_liquidity_pools_gateway::Call::process_msg to SafeCallFilter which enables this to be executed by xcm transact

NunoAlexandre

LGTM 👌

mustermeiszer · 2023-09-04T12:33:24Z

runtime/altair/src/migrations.rs

+	impl OnRuntimeUpgrade for SetSafeXcmVersion {
+		fn on_runtime_upgrade() -> Weight {
+			if VERSION.spec_version > 1030 {
+				return Weight::zero();
+			}
+
+			// Unfortunately, SafeXcmVersion storage is not leaked to runtime
+			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().reads_writes(1, 1)
+		}
+	}


No before and after?

See the comment before setting:

Unfortunately, SafeXcmVersion storage is not leaked to runtime

So we can't check this storage in pre and post upgrade unfortunately. Since we have no PolkadotXcm storage at the moment, we can't batch the runtime upgrade with the execution of force_default_xcm_version either unfortunately.

Either we live with the fact it cannot be checked with try-runtime or need to create a post-upgrade referendum to set it. Given that we only do a single write here, I think it is sane to keep the migration. I have tested it locally against centrifuge-local @ 1019.

Sorry, yeah, thats true. Lets keep it as is

mustermeiszer · 2023-09-04T12:44:28Z

runtime/altair/src/xcm.rs

@@ -87,6 +87,9 @@ impl frame_support::traits::Contains<RuntimeCall> for SafeCallFilter {
 			) | RuntimeCall::XcmpQueue(..)
 				| RuntimeCall::DmpQueue(..)
 				| RuntimeCall::Proxy(..)
+				| RuntimeCall::LiquidityPoolsGateway(
+					pallet_liquidity_pools_gateway::Call::process_msg { .. }


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? ^^

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!

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.

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.

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?

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.

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.

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

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!

NunoAlexandre · 2023-09-04T12:47:46Z

runtime/altair/src/migrations.rs

+			if VERSION.spec_version > 1030 {
+				return Weight::zero();
+			}


Actually, to avoid the annoyance of having to either comment this out or bump the spec_version to test this with try-runtime, and given that this migration is idempotent, I'd say we are better off without this check. It would only be unwise to drop it if setting this xcm safe version was time-sensitive, which it is not.

* centrifuge: Enable LP gateway XCM origin converter * centrifuge: Update LP pallet config to use Quantity for BalanceRatio * runtime: Update XCM origin converter, use Quantity in LP message, add OriginRecovery to LP gateway

mustermeiszer

Will reaprove if lints are in. But I would also be fine with merging and fixing lints locally and merging directly.

mustermeiszer

Re-approve

* wip xcm v3 asset-registry migrations * fix: xcm v3 altair, centrifuge * feat: add defensive SafeCallFilter for transact * clean up asset addition for catalyst & centrifuge * add Altair + Algol migration * Add Liquidity Pools related pallets to centrifuge runtime (#1523) * centrifuge: Add liquidity pools pallets to runtime * centrifuge: Add LP OutboundQueue wrapper to runtime * taplo: Obey * runtime: Move OutboundQueue import * centrifuge: Allow both root and half of council as admin origins for LP pallets * centrifuge: Add Axelar Gateway precompile * centrifuge: Use correct HRMP encoder, update HRMP fee to 1 DOT, use XCM V3 imports * centrifuge: Update error message for InboundQueue * clippy: Fix warnings * centrifuge: Rename outbound queue, fix migration import for orml asset registry * Upgrade1020 LP follow-ups (#1525) * centrifuge: Enable LP gateway XCM origin converter * centrifuge: Update LP pallet config to use Quantity for BalanceRatio * runtime: Update XCM origin converter, use Quantity in LP message, add OriginRecovery to LP gateway * feat: safe XCM version migration + extend SafeCallFilter (#1526) * fix: allow gateway process msg for xcm * feat: add safe xcm version migration * fix: Allow process_msg for xcm transact * refactor: remove spec_version check * refactor: stricter SafeCallFilter * Upgrade1020 LP follow-ups (#1525) * centrifuge: Enable LP gateway XCM origin converter * centrifuge: Update LP pallet config to use Quantity for BalanceRatio * runtime: Update XCM origin converter, use Quantity in LP message, add OriginRecovery to LP gateway * fix: rm OrderBook safe call cause missing * fix: OrderBook missing in both runtimes * fix: clippy --------- Co-authored-by: Cosmin Damian <17934949+cdamian@users.noreply.github.com> * Feat/upgrade1020 misc 1 (#1529) * fix: asset metadata xcm v3 migration cleanup other migrations (#1528) * fix: centrifuge asset migration * Apply suggestions from code review Co-authored-by: Frederik Gartenmeister <mustermeiszer@posteo.de> * chore: append cfg assets to catalyst * fix: asset registry migration improvements * feat: apply asset metadata migration to altair runtime * fix: cleanup altair/algol migrations * fix: less aggressive nuke pre-upgrade * fix: finalize migrations, add context * fix: clippy + cleanup imports * fix: native currency altair, algol * fix: Algol native currency name --------- Co-authored-by: Frederik Gartenmeister <mustermeiszer@posteo.de> * fmt * fixup * Upgrade1020 gateway changes (#1531) * gateway: Use BoundedVec instead of EVMChain enum * gateway: Add Sender type to LP gateway config * gateway: Rename EVM chain size const, emit event when submitting message, adjust gateway sender provider * Feat: upgrade1020 misc 2 (#1532) * feat: account ensuring origin * feat: bumb runtime version develpment * fix: allow council to control gatway on Altair based chains * fix: use EnsureSigned, make use of type and add admin * feat: overestimated weights for lp logic * fix: taplooo * fix: tests and comment * chore: bump weights for 1020 (#1533) * chore: update centrifuge, altair weights * feat: bump and improve dev weights * fix: transfer_allowlist weights * feat: normalize SafeCallFilter across runtimes * fix: native asset key in migration --------- Co-authored-by: William Freudenberger <w.freude@icloud.com> Co-authored-by: Cosmin Damian <17934949+cdamian@users.noreply.github.com> Co-authored-by: Frederik Gartenmeister <mustermeiszer@posteo.de> Co-authored-by: nuno <hi@nunoalexandre.com>

wischli added 3 commits September 4, 2023 14:11

fix: allow gateway process msg for xcm

08c7cbc

feat: add safe xcm version migration

7492614

fix: Allow process_msg for xcm transact

3909880

wischli marked this pull request as ready for review September 4, 2023 12:25

wischli requested review from mikiquantum, mustermeiszer, NunoAlexandre, lemunozm and cdamian as code owners September 4, 2023 12:25

NunoAlexandre approved these changes Sep 4, 2023

View reviewed changes

cdamian approved these changes Sep 4, 2023

View reviewed changes

mustermeiszer reviewed Sep 4, 2023

View reviewed changes

NunoAlexandre reviewed Sep 4, 2023

View reviewed changes

wischli changed the title ~~feat: safe XCM version mgiration~~ feat: safe XCM version migration + extend SafeFallFilter Sep 4, 2023

wischli changed the title ~~feat: safe XCM version migration + extend SafeFallFilter~~ feat: safe XCM version migration + extend SafeCallFilter Sep 4, 2023

wischli added the D8-migration Pull request touches storage and needs migration code. label Sep 4, 2023

wischli self-assigned this Sep 4, 2023

wischli and others added 5 commits September 4, 2023 16:44

refactor: remove spec_version check

deb9fa3

refactor: stricter SafeCallFilter

cea48d4

Upgrade1020 LP follow-ups (#1525)

741dcca

* centrifuge: Enable LP gateway XCM origin converter * centrifuge: Update LP pallet config to use Quantity for BalanceRatio * runtime: Update XCM origin converter, use Quantity in LP message, add OriginRecovery to LP gateway

fix: rm OrderBook safe call cause missing

eb2b02f

fix: OrderBook missing in both runtimes

e5a89e8

wischli requested a review from mustermeiszer September 4, 2023 16:45

mustermeiszer approved these changes Sep 4, 2023

View reviewed changes

fix: clippy

418acde

mustermeiszer approved these changes Sep 4, 2023

View reviewed changes

wischli merged commit 6244e22 into upgrade1020 Sep 4, 2023
11 checks passed

wischli deleted the wf/upgrade-1020 branch September 4, 2023 20:02

wischli mentioned this pull request Sep 5, 2023

fix: asset metadata xcm v3 migration cleanup other migrations #1528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: safe XCM version migration + extend SafeCallFilter #1526

feat: safe XCM version migration + extend SafeCallFilter #1526

wischli commented Sep 4, 2023

NunoAlexandre left a comment

mustermeiszer Sep 4, 2023

wischli Sep 4, 2023

mustermeiszer Sep 4, 2023

mustermeiszer Sep 4, 2023

wischli Sep 4, 2023 •

edited

mustermeiszer Sep 4, 2023

wischli Sep 4, 2023

mustermeiszer Sep 4, 2023

mustermeiszer Sep 4, 2023

wischli Sep 4, 2023

wischli Sep 4, 2023

wischli Sep 4, 2023 •

edited

mustermeiszer Sep 4, 2023

NunoAlexandre Sep 4, 2023

wischli Sep 4, 2023

mustermeiszer left a comment

mustermeiszer left a comment

feat: safe XCM version migration + extend SafeCallFilter #1526

feat: safe XCM version migration + extend SafeCallFilter #1526

Conversation

wischli commented Sep 4, 2023

NunoAlexandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli Sep 4, 2023 •

edited

wischli Sep 4, 2023 •

edited