refactor(mainnet): xcm config #455

al3mart · 2025-01-30T22:40:54Z

Extends xcm config module including:

pallet_message_queue
cumulus_pallet_xcm
cumulus_pallet_xcmp_queue

And adding configuration unit tests for:

xcm_executor
pallet_xcm

TODOs:

Charges delivery fees: fix(xcm): charge delivery fees #457
Removes WaivedLocations: fix(xcm): remove waived locations #456
XcmReserveTransferFilter should be configured to only allow reserve transfers of relay native asset. refactor(xcm): Use NativeAssetFrom<T> as XcmReserveTransferFilter #459

Notable configuration items are:

pallet_message_queue

type HeapSize = ConstU32<{ 64 * 1024 }>;
type IdleMaxServiceWeight = Perbill::from_percent(20) * RuntimeBlockWeights::get().max_block;
type ServiceWeight = Perbill::from_percent(35) * RuntimeBlockWeights::get().max_block;

cumulus_pallet_xcmp_queue

type ControllerOrigin = EnsureRoot<AccountId>;
type MaxActiveOutboundChannels = ConstU32<128>;
type MaxInboundSuspended = ConstU32<128>;
type MaxPageSize = ConstU32<{ 103 * 1024 }>;

PriceForSiblingDelivery is addressed in: #457

xcm_executor

// One XCM operation is 1_000_000_000 weight - almost certainly a conservative estimate.
pub UnitWeightCost: Weight = Weight::from_parts(1_000_000_000, 64 * 1024);

WaivedLocations are removed from FeeManager config in: #456

pallet_xcm

type AdminOrigin = EnsureRoot<AccountId>;
type ExecuteXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
type SendXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
type XcmExecuteFilter = Everything;

[sc-2205]

codecov-commenter · 2025-01-31T00:59:54Z

Codecov Report

Attention: Patch coverage is 99.83444% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.78%. Comparing base (702b7f2) to head (25cd9f9).

Files with missing lines	Patch %	Lines
runtime/mainnet/src/apis.rs	0.00%	1 Missing ⚠️

@@                         Coverage Diff                         @@
##           al3mart/refactor-mainnet-config     #455      +/-   ##
===================================================================
+ Coverage                            74.94%   75.78%   +0.84%     
===================================================================
  Files                                   83       83              
  Lines                                16550    17127     +577     
  Branches                             16550    17127     +577     
===================================================================
+ Hits                                 12403    12980     +577     
  Misses                                3884     3884              
  Partials                               263      263

Files with missing lines	Coverage Δ
runtime/mainnet/src/config/xcm.rs	`100.00% <100.00%> (ø)`
runtime/mainnet/src/lib.rs	`66.23% <ø> (ø)`
runtime/mainnet/src/apis.rs	`0.00% <0.00%> (ø)`

al3mart · 2025-01-31T11:55:14Z

runtime/mainnet/src/config/xcm.rs

+	type ControllerOriginConverter = XcmOriginToTransactDispatchOrigin;
+	type MaxActiveOutboundChannels = ConstU32<128>;
+	type MaxInboundSuspended = ConstU32<128>;
+	type MaxPageSize = ConstU32<{ 103 * 1024 }>;


https://github.com/polkadot-fellows/runtimes/blob/76d1fa680d00c3e447e40199e7b2250862ad4bfa/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L692C2-L693C90

Max channel size in Polkadot:

hrmpChannelMaxTotalSize: 102,400

Daanvdplas

Looking good, left some comments.

will help with the PR that is blocking this PR as fast as I can

runtime/mainnet/src/config/xcm.rs

Daanvdplas · 2025-02-03T13:53:22Z

runtime/mainnet/src/config/xcm.rs

@@ -34,6 +44,31 @@ parameter_types! {
 	pub const RelayNetwork: Option<NetworkId> = Some(Polkadot);
 	pub RelayChainOrigin: RuntimeOrigin = cumulus_pallet_xcm::Origin::Relay.into();
 	pub UniversalLocation: InteriorLocation = [GlobalConsensus(RelayNetwork::get().unwrap()), Parachain(ParachainInfo::parachain_id().into())].into();
+	pub MessageQueueIdleServiceWeight: Weight = Perbill::from_percent(20) * RuntimeBlockWeights::get().max_block;
+	pub MessageQueueServiceWeight: Weight = Perbill::from_percent(35) * RuntimeBlockWeights::get().max_block;
+	pub TreasuryAccount: AccountId = PalletId(*b"treasury").into_account_truncating();


Should this not be the one from pallet treasury?

Yes, needs changing with the rebase. I should have left a comment here in GH. The description of the base PR notes that there are relevant changes out of the base branch, and that it needs a rebase to include those.

Leaving the rebase to do last so I can address all at once. Treasury's account, for instance, is not only used here.

Now that you rebased pallet treasury into the base branch, can this not be immediately resolved?

Nice, I did amend this during the rebase. Looks like I rolled back the changed along the way.

runtime/mainnet/src/config/xcm.rs

peterwht

looks good! Left a few tiny nitpicks and then good to approve (once the next PRs are merged in)

runtime/mainnet/src/config/xcm.rs

Daanvdplas · 2025-02-05T13:04:58Z

FYI: I wanted to see if I could improve the integration tests changes but it couldn't compile locally.

integration-tests/src/lib.rs

runtime/mainnet/src/config/xcm.rs

typos AssetFilter

lint

unnecessary Fungibles

* refactor: integration tests mainnet xcm * fix: remove unused ED rebase fmt fmt

Daanvdplas

PR looking really good! A few last things that need addressing but then good to go!

Daanvdplas · 2025-02-07T14:11:05Z

integration-tests/src/lib.rs

@@ -326,6 +327,18 @@ fn reserve_transfer_native_asset_from_para_to_system_para() {
 	let sender_balance_before = test.sender.balance;
 	let receiver_balance_before = test.receiver.balance;

+	let delivery_fees = PopNetworkPara::execute_with(|| {


Could we move those to where it was initially positioned and check if the clones can be removed? I thought I did this but apparently made a mistake

Daanvdplas · 2025-02-07T14:12:19Z

runtime/mainnet/src/config/xcm.rs

@@ -34,6 +44,31 @@ parameter_types! {
 	pub const RelayNetwork: Option<NetworkId> = Some(Polkadot);
 	pub RelayChainOrigin: RuntimeOrigin = cumulus_pallet_xcm::Origin::Relay.into();
 	pub UniversalLocation: InteriorLocation = [GlobalConsensus(RelayNetwork::get().unwrap()), Parachain(ParachainInfo::parachain_id().into())].into();
+	pub MessageQueueIdleServiceWeight: Weight = Perbill::from_percent(20) * RuntimeBlockWeights::get().max_block;
+	pub MessageQueueServiceWeight: Weight = Perbill::from_percent(35) * RuntimeBlockWeights::get().max_block;
+	pub TreasuryAccount: AccountId = PalletId(*b"treasury").into_account_truncating();


Now that you rebased pallet treasury into the base branch, can this not be immediately resolved?

runtime/mainnet/src/config/xcm.rs

Daanvdplas · 2025-02-07T14:16:32Z

runtime/mainnet/src/config/xcm.rs

-	fn relay_as_relay_asset_reserve_fails() {
-		let relay_asset = Asset::from((AssetId::from(Parent), Fungibility::from(100u128)));
-		assert!(!TrustedReserves::contains(&relay_asset, &Parent.into()));
+	mod xcm_configuration {


Since those tests do not belong to a pallet directly I would suggest to remove this submodule

runtime/mainnet/src/config/xcm.rs

Daanvdplas · 2025-02-07T14:18:05Z

runtime/mainnet/src/config/xcm.rs

+	mod xcm_executor_configuration {
+		use super::*;
+
+		mod reserves_config {


Suggested change

mod reserves_config {

mod reserves {

runtime/mainnet/src/config/xcm.rs

Daanvdplas · 2025-02-07T14:22:31Z

runtime/mainnet/src/config/xcm.rs

+		}
+
+		#[test]
+		fn reserve_transfer_filter_only_allows_relay_asset_from_ah() {


Would like to see this test refactored into two tests. One for the filter, one passing.

Moreover, like I have said elsewhere too, I find the tests that use the Location::Parent confusing. This should not be possible on our chain and thus causes confusion to me. Especially the passing test of an asset with location Location::Parent on line 972

Still don't get it why we are testing for parent location. Also the second test now contains logic that is not specific to that test which only confused or distracts from what actually needs to be tested.

Rule of thumb I learned, tests should be as specific and simple as it can be to reduce the amount work required, as well as maintaining it. In addition, you want the tests to be specific so that it is super clear what is tested and why.

Co-authored-by: Daan van der Plas <[email protected]>

…ra_to_system

runtime/mainnet/src/config/xcm.rs

integration-tests/src/lib.rs

Daanvdplas

Final suggestion to simplify the unit tests then happy to approve!

runtime/mainnet/src/config/xcm.rs

Daanvdplas · 2025-02-10T15:47:19Z

runtime/mainnet/src/config/xcm.rs

+		}
+
+		#[test]
+		fn reserve_transfer_filter_contains_parent_asset() {


Co-authored-by: Daan van der Plas <[email protected]>

Daanvdplas

Good job and thanks for the patience!

peterwht

LGTM! Thanks

al3mart mentioned this pull request Jan 31, 2025

refactor(mainnet): ready runtime config #458

Draft

12 tasks

al3mart commented Jan 31, 2025

View reviewed changes

al3mart requested review from Daanvdplas and peterwht January 31, 2025 12:04

al3mart marked this pull request as ready for review January 31, 2025 12:13

al3mart added ready-for-final-review and removed ready-for-final-review labels Jan 31, 2025

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch from 5b70d8c to 9333fe6 Compare January 31, 2025 13:29

Daanvdplas requested changes Feb 3, 2025

View reviewed changes

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch from 9333fe6 to 5fa78c6 Compare February 4, 2025 13:08

peterwht requested changes Feb 4, 2025

View reviewed changes

runtime/mainnet/src/config/xcm.rs Outdated Show resolved Hide resolved

runtime/mainnet/src/config/xcm.rs Outdated Show resolved Hide resolved

runtime/mainnet/src/config/xcm.rs Outdated Show resolved Hide resolved

runtime/mainnet/src/config/xcm.rs Outdated Show resolved Hide resolved

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch from 7b7df0f to 95745b5 Compare February 5, 2025 01:39

al3mart requested review from peterwht and Daanvdplas February 5, 2025 01:39

al3mart added the ready-for-final-review label Feb 5, 2025

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch from 95745b5 to 73926d9 Compare February 5, 2025 08:32

al3mart force-pushed the al3mart/refactor-mainnet-config branch from 809c784 to 9b1f3e2 Compare February 5, 2025 09:08

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch 3 times, most recently from d644888 to 1610f34 Compare February 5, 2025 13:00

Daanvdplas reviewed Feb 5, 2025

View reviewed changes

runtime/mainnet/src/config/xcm.rs Show resolved Hide resolved

runtime/mainnet/src/config/xcm.rs Show resolved Hide resolved

runtime/mainnet/src/config/xcm.rs Outdated Show resolved Hide resolved

Daanvdplas reviewed Feb 5, 2025

View reviewed changes

integration-tests/src/lib.rs Outdated Show resolved Hide resolved

Daanvdplas reviewed Feb 5, 2025

View reviewed changes

integration-tests/src/lib.rs Outdated Show resolved Hide resolved

Daanvdplas reviewed Feb 5, 2025

View reviewed changes

integration-tests/src/lib.rs Outdated Show resolved Hide resolved

Daanvdplas reviewed Feb 5, 2025

View reviewed changes

runtime/mainnet/src/config/xcm.rs Show resolved Hide resolved

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch 3 times, most recently from 8700383 to d63b78b Compare February 6, 2025 16:35

al3mart added 3 commits February 7, 2025 11:46

test(xcm): address feedback from #459

cd079dc

typos AssetFilter

style(xcm): locate reserve tests within executor

d5427d8

lint

fix(integration-test): improve tests & remove feature gated compilation

5193dac

unnecessary Fungibles

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch 2 times, most recently from 475d40e to d9ec3db Compare February 7, 2025 11:36

al3mart requested a review from Daanvdplas February 7, 2025 11:36

refactor: integration tests mainnet xcm (#466)

8969363

* refactor: integration tests mainnet xcm * fix: remove unused ED rebase fmt fmt

al3mart force-pushed the al3mart/refactor-mainnet-xcm branch from d9ec3db to 8969363 Compare February 7, 2025 12:16

Daanvdplas requested changes Feb 7, 2025

View reviewed changes

al3mart and others added 7 commits February 7, 2025 15:37

Update runtime/mainnet/src/config/xcm.rs

46b18d1

Co-authored-by: Daan van der Plas <[email protected]>

refactor(xcm): use TreasuryAccount from monetary

4424eb8

chore(xcm): unused imports

1fa7160

chore(xcm): amend test modules

4695c2a

chore(integration-tests): amend reserve_transfer_native_asset_from_pa…

b0902c5

…ra_to_system

chore(xcm): divive reserve filter test in two

4b9bb82

chore(xcm): fmt

57b6c5c

Daanvdplas reviewed Feb 10, 2025

View reviewed changes

runtime/mainnet/src/config/xcm.rs Outdated Show resolved Hide resolved

Daanvdplas reviewed Feb 10, 2025

View reviewed changes

integration-tests/src/lib.rs Outdated Show resolved Hide resolved

style(integration-tests): fix unwanted indentation

7071e44

al3mart requested a review from Daanvdplas February 10, 2025 11:24

test(xcm): more focused unit tests

84f535c

Daanvdplas reviewed Feb 10, 2025

View reviewed changes

al3mart and others added 3 commits February 10, 2025 17:11

Update runtime/mainnet/src/config/xcm.rs

554554b

Co-authored-by: Daan van der Plas <[email protected]>

Update runtime/mainnet/src/config/xcm.rs

4638711

Co-authored-by: Daan van der Plas <[email protected]>

fix(test): amend suggestions

25cd9f9

Daanvdplas self-requested a review February 10, 2025 17:36

Daanvdplas approved these changes Feb 10, 2025

View reviewed changes

peterwht approved these changes Feb 10, 2025

View reviewed changes

al3mart merged commit cae5b19 into al3mart/refactor-mainnet-config Feb 10, 2025
15 checks passed

al3mart deleted the al3mart/refactor-mainnet-xcm branch February 10, 2025 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(mainnet): xcm config #455

refactor(mainnet): xcm config #455

al3mart commented Jan 30, 2025 •

edited

Loading

codecov-commenter commented Jan 31, 2025 •

edited

Loading

al3mart Jan 31, 2025 •

edited

Loading

Daanvdplas left a comment

Daanvdplas Feb 3, 2025

al3mart Feb 4, 2025 •

edited

Loading

Daanvdplas Feb 7, 2025

al3mart Feb 7, 2025

al3mart Feb 7, 2025

peterwht left a comment

Daanvdplas commented Feb 5, 2025

Daanvdplas left a comment

Daanvdplas Feb 7, 2025

al3mart Feb 7, 2025

Daanvdplas Feb 7, 2025

Daanvdplas Feb 7, 2025

al3mart Feb 7, 2025

Daanvdplas Feb 7, 2025

al3mart Feb 7, 2025

Daanvdplas Feb 7, 2025

al3mart Feb 7, 2025

Daanvdplas Feb 10, 2025

Daanvdplas left a comment

Daanvdplas Feb 10, 2025

Daanvdplas left a comment

peterwht left a comment

refactor(mainnet): xcm config #455

refactor(mainnet): xcm config #455

Conversation

al3mart commented Jan 30, 2025 • edited Loading

codecov-commenter commented Jan 31, 2025 • edited Loading

Codecov Report

al3mart Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al3mart Feb 4, 2025 • edited Loading

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

peterwht left a comment

Choose a reason for hiding this comment

Daanvdplas commented Feb 5, 2025

Daanvdplas 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

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

Daanvdplas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

peterwht left a comment

Choose a reason for hiding this comment

al3mart commented Jan 30, 2025 •

edited

Loading

codecov-commenter commented Jan 31, 2025 •

edited

Loading

al3mart Jan 31, 2025 •

edited

Loading

al3mart Feb 4, 2025 •

edited

Loading