feat(mainnet): add XCM runtime APIs, config tests, and refactor to API mod [DON'T MERGE] #426

peterwht · 2025-01-12T19:06:29Z

DO NOT SQUASH MERGE -- REMOVE THIS MESSAGE BEFORE MERGE
Prereq: #425

This PR has various improvements to the mainnet runtime. Please review the individual commits:

test(mainnet): more configuration tests
refactor(mainnet): runtime APIs to apis module
feat(mainnet): add XCM runtime APIs

Co-authored-by: Daanvdplas <daanvdplas@live.nl> Co-authored-by: Daan van der Plas <93204684+Daanvdplas@users.noreply.github.com>

…nstead of then_some https://github.com/paritytech/polkadot-sdk/pull/6156/files#diff-7ab065c73b2237dff7c6eca18162b7f674e5e7f9e330af668cc5d35de6a597bd https://github.com/paritytech/polkadot-sdk/pull/5919/files#diff-7b805930c8caa346a498254e233963501862a29b4ae5633a00efb96b9c96258a paritytech/polkadot-sdk#5693 paritytech/polkadot-sdk@b76e91a

… well

peterwht · 2025-01-12T19:51:46Z

[sc-2219]

Daanvdplas

CI is failing

runtime/mainnet/src/lib.rs

Daanvdplas · 2025-01-14T14:36:53Z

runtime/mainnet/src/apis.rs

@@ -257,4 +267,71 @@ impl_runtime_apis! {
 			Default::default()
 		}
 	}
+
+	impl xcm_runtime_apis::fees::XcmPaymentApi<Block> for Runtime {


Curious to hear whether we want to add them to testnet and devnet as well.

I don't see why not!

In follow up PRs, I think it would make sense. However, it seems a distraction right now. Our current mainnet runtime will be deployed as its own testnet. Which means our current testnet and devnet don't factor into the initial mainnet launch.

We do need to resolve what will happen to the current testnet and how the two testnets will live together (if at all)

That means that our current testnet becomes our devnet, and testnet should become a copy of mainnet?

Still believe that any feature on mainnet (thus testnet) should be on devnet

Consider that certain things like spec name cannot be changed and that protocol id should be unique. Deploying the mainnet runtime as-is to Paseo may only make sense if it is going to be removed completely to make way for the Polkadot deployment rather than upgraded in place to become a testnet.

Fine to do it later, my view is that it only creates more work for later and it is a simple add now.

It is still unclear to me what the plan for this new testnet exactly is so getting that done would prevent further confusions.

Happy to address this myself in a different PR to get this closed one sorted.

al3mart

Overall pretty good!

Have left a few comments, but mainly things worth discussing rather than change requests.

al3mart · 2025-01-14T18:05:54Z

runtime/mainnet/src/lib.rs

@@ -16,6 +16,8 @@ use alloc::{borrow::Cow, vec::Vec};
 use config::xcm::{RelayLocation, XcmOriginToTransactDispatchOrigin};
 use cumulus_pallet_parachain_system::RelayNumberMonotonicallyIncreases;
 use cumulus_primitives_core::{AggregateMessageOrigin, ParaId};
+use cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim;


Looks like this is being deprecated and substituted by a new implementation.

For para chains StorageWeightReclaim in cumulus-primitives-storage-weight-reclaim is deprecated.
A new transaction extension StorageWeightReclaim in cumulus-pallet-weight-reclaim is introduced.
StorageWeightReclaim is meant to be used as a wrapping of the whole transaction extension pipeline, and will take into account all proof size accurately.

source

Let's see if this change is backported into 2412

good catch. Hopefully they release the new release soon.

al3mart · 2025-01-14T19:11:11Z

runtime/mainnet/src/apis.rs

+		}
+	}
+
+	impl xcm_runtime_apis::dry_run::DryRunApi<Block, RuntimeCall, RuntimeEvent, OriginCaller> for Runtime {


These dry_run api are delicate in the sense that if someone has access to a node can make the node work a lot for free.

While there are ways to rate-limit the requests to the node:

either via integrated middleware: rpc server: add rate limiting middleware paritytech/polkadot-sdk#3301

or extra deployments: https://github.com/AcalaNetwork/subway

Ideally we should be capable of dropping a dry-run that is bigger than some threshold independently of spam. A weight-rate, instead of a requests one. However I'm not sure how could we do that from the top of my head.

These are very useful apis, that's for sure! Whether we are going to benefit from them from the start or not is questionable. Having native XCM support on ink! is a vote in favor for them.

My take would be keeping them for now and do some extra research on them.

Would these not be limited to RPCs only? If collators only expose P2P, is you concern then a non-issue?

Exactly, these apis are only accessible if one can reach out the node.

runtime/mainnet/src/apis.rs

codecov-commenter · 2025-01-15T00:18:52Z

Codecov Report

Attention: Patch coverage is 14.20613% with 308 lines in your changes missing coverage. Please review.

Project coverage is 68.03%. Comparing base (e89b670) to head (ad7cee7).

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

@@                       Coverage Diff                        @@
##           peterwht/chore-polkadot-2412     #426      +/-   ##
================================================================
- Coverage                         68.31%   68.03%   -0.28%     
================================================================
  Files                                70       71       +1     
  Lines                             11777    11899     +122     
  Branches                          11777    11899     +122     
================================================================
+ Hits                               8045     8096      +51     
- Misses                             3480     3551      +71     
  Partials                            252      252

Files with missing lines	Coverage Δ
runtime/mainnet/src/lib.rs	`73.98% <100.00%> (+52.53%)`	⬆️
runtime/mainnet/src/apis.rs	`0.00% <0.00%> (ø)`

Cargo.toml

al3mart

I believe the changes look great. Happy to proceed once CI is happy too.

evilrobot-01

Seems the transaction payment pallet is missing from define_benchmarks! along with the inclusion of the new transaction extension benchmarks across the various runtimes.

evilrobot-01 · 2025-01-17T13:36:16Z

Cargo.toml

 tokio = { version = "1.36", features = [ "macros", "rt-multi-thread", "time" ] }
 tracing-subscriber = { version = "0.3.18", default-features = false }

 # Build
-substrate-build-script-utils = "11.0.0"
-substrate-wasm-builder = "24.0.1"
+substrate-build-script-utils = { git = "https://github.com/r0gue-io/polkadot-sdk", branch = "stable2412" }


Suggested change

substrate-build-script-utils = { git = "https://github.com/r0gue-io/polkadot-sdk", branch = "stable2412" }

substrate-build-script-utils = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2412" }

Suggest a quick find and replace and see if it builds and everything works. Some fixes are already pushed there ahead of the first patch release due next week. Can then get rid of the fork and switch back to the crates.io versions separately with next release.

fixed in #425

evilrobot-01 · 2025-01-17T13:38:06Z

Cargo.toml

-paseo-runtime-constants = { git = "https://github.com/paseo-network/runtimes/", tag = "v1.3.3", default-features = false }
-westend-runtime = { version = "18.0.1", default-features = false }
-westend-runtime-constants = { version = "17.0.0", default-features = false }
+asset-hub-paseo-runtime = { git = "https://github.com/paseo-network/runtimes", default-features = false, tag = "v1.3.3" }


Suggest we remove these other runtimes as a follow up PR. I dont see them being used anytime soon due to the lag between a SDK release and runtime release.

evilrobot-01 · 2025-01-17T13:41:16Z

integration-tests/Cargo.toml

@@ -60,74 +60,9 @@ paseo = [
 	"asset-hub-paseo-runtime",
 	"paseo-runtime",
 	"paseo-runtime-constants",
-	"std",
-]
-runtime-benchmarks = [


Justification here is that none of these features are used/required to run integration tests. These were included historically to work around build issues when trying to use a single command to run all tests/benchmarks in the repo. Can be revisited again separately.

evilrobot-01 · 2025-01-17T13:43:14Z

integration-tests/src/chains/relay/genesis.rs

 };
 use polkadot_primitives::{AssignmentId, Balance, ValidatorId, LOWEST_PUBLIC_ID};
 use sp_authority_discovery::AuthorityId as AuthorityDiscoveryId;
 use sp_consensus_babe::AuthorityId as BabeId;
 use sp_consensus_beefy::ecdsa_crypto::AuthorityId as BeefyId;
+use sp_consensus_beefy::test_utils::Keyring;


No change required, but single usage imports seem unnecessary to me, provided the usage doesnt cause a line wrap. Based on the level of indentation below, I assume this would be the case.

I simply prefer as little code as possible, less to write, less to review.

evilrobot-01 · 2025-01-17T13:46:16Z

node/src/service.rs

-		config.prometheus_registry(),
-		task_manager.spawn_essential_handle(),
-		client.clone(),
+	// TOOD: double check


Can more insight be provided here? Not sure we should be reviewing/approving/merging if there are todos.

resolved in #425, dependent on this convo: #425 (comment)

primitives/src/lib.rs

runtime/devnet/src/config/api/versioning.rs

evilrobot-01 · 2025-01-17T13:52:51Z

runtime/devnet/src/lib.rs

@@ -186,15 +187,15 @@ impl_opaque_keys! {

 #[sp_version::runtime_version]
 pub const VERSION: RuntimeVersion = RuntimeVersion {
-	spec_name: create_runtime_str!("pop"),
-	impl_name: create_runtime_str!("pop"),
+	spec_name: Cow::Borrowed("pop"),


No action required, but as there is an import for Cow, could have just been Cow::Borrowed imported to avoid the duplication here. I figure you may prefer it having an explicit prefix tho.

evilrobot-01 · 2025-01-17T13:57:19Z

runtime/mainnet/Cargo.toml

@@ -166,6 +169,7 @@ runtime-benchmarks = [
 	"pallet-scheduler/runtime-benchmarks",
 	"pallet-sudo/runtime-benchmarks",
 	"pallet-timestamp/runtime-benchmarks",
+	"pallet-transaction-payment/runtime-benchmarks",


I dont see that this has been added to define_benchmarks in any of the runtimes. Likewise there is no addition of the benchmarks for transaction extensions (frame_system_extensions)

this was an intentional decision (should have been documented). Our define_benchmarks! in all of our runtimes does not include every pallet. I will create an issue to resolve this in a separate PR once the 2412 uplift is merged

evilrobot-01 · 2025-01-17T14:01:08Z

runtime/testnet/src/lib.rs

@@ -984,7 +988,7 @@ impl_runtime_apis! {

 		fn dispatch_benchmark(
 			config: frame_benchmarking::BenchmarkConfig
-		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
+		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {


Missing use frame_system_benchmarking::extensions::Pallet as SystemExtensionsBench; in the benchmarks trait implementation. Closest I could place the comment.

resolved in #447

peterwht · 2025-01-22T21:51:59Z

superseded by #447

chungquantin and others added 18 commits January 3, 2025 12:24

chore: upgrade to stable2409 (and stable2407) (#234)

545a095

Co-authored-by: Daanvdplas <daanvdplas@live.nl> Co-authored-by: Daan van der Plas <93204684+Daanvdplas@users.noreply.github.com>

chore: initial uplift to 2412 -- checkpoint

af593ef

chore: update p-sdk to r0gue-io, exclude integration tests

c18b35e

fix(tests): use total_weight(), update balances in mock

09765b5

chore: use ISMP deps from github

326430a

style: cargo & taplo fmt

4e187a2

fix: cargo.lock had to be force added

5da3697

refactor(chain-spec): update postfix from _config to _chain_spec

de891df

refactor(extension): use call_weight instead of total_weight()

3a4d188

refactor: CheckEra -> CheckMortality

0fec866

refactor(node): use sp-keyring for dev accounts

44d4a3c

chore(deps): add transaction-payment/runtime-benchmarks. Taplo fmt as…

6381572

… well

fix(integration-tests): use XCM v5 and Keyring

7bb4014

style( sorry ): editor reset fmt to use default instead of nightly

e89b670

test(mainnet): more configuration tests

26592f9

refactor(mainnet): runtime APIs to apis module

b04efa2

feat(mainnet): add XCM runtime APIs

4d18a39

peterwht requested a review from Daanvdplas January 12, 2025 19:06

peterwht added the ready-for-final-review label Jan 12, 2025

peterwht mentioned this pull request Jan 12, 2025

feat(mainnet): add revive [DON'T MERGE] #427

Open

peterwht changed the title ~~[DON'T MERGE] mainnet improvements~~ feat,refactor,test(mainnet): various improvements [DON'T MERGE] Jan 12, 2025

peterwht mentioned this pull request Jan 12, 2025

chore: uplift to polkadot 2412 #425

Merged

4 tasks

peterwht changed the title ~~feat,refactor,test(mainnet): various improvements [DON'T MERGE]~~ feat(mainnet): add XCM runtime APIs, config tests, and refactor to API mod [DON'T MERGE] Jan 12, 2025

peterwht requested a review from al3mart January 12, 2025 19:17

Daanvdplas reviewed Jan 14, 2025

View reviewed changes

fix(mainnet): missing runtime benchmarks dependencies

16e58c7

al3mart reviewed Jan 14, 2025

View reviewed changes

chore(mainnet): better sentence format for comments in test

ad7cee7

chungquantin added the not-merged label Jan 15, 2025

peterwht force-pushed the peterwht/chore-polkadot-2412 branch from 029a3a9 to 937ca11 Compare January 17, 2025 02:50

al3mart reviewed Jan 17, 2025

View reviewed changes

Cargo.toml Show resolved Hide resolved

al3mart reviewed Jan 17, 2025

View reviewed changes

evilrobot-01 requested changes Jan 17, 2025

View reviewed changes

peterwht removed the ready-for-final-review label Jan 17, 2025

Base automatically changed from peterwht/chore-polkadot-2412 to chore-upgrade-pop-net-to-stable2412/sc-1940 January 21, 2025 14:59

Base automatically changed from chore-upgrade-pop-net-to-stable2412/sc-1940 to main January 21, 2025 20:45

peterwht mentioned this pull request Jan 22, 2025

feat(mainnet): add XCM APIs, config tests, refactor mod #447

Merged

peterwht closed this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mainnet): add XCM runtime APIs, config tests, and refactor to API mod [DON'T MERGE] #426

feat(mainnet): add XCM runtime APIs, config tests, and refactor to API mod [DON'T MERGE] #426

peterwht commented Jan 12, 2025 •

edited

Loading

peterwht commented Jan 12, 2025

Daanvdplas left a comment

Daanvdplas Jan 14, 2025

al3mart Jan 14, 2025

peterwht Jan 15, 2025

Daanvdplas Jan 15, 2025 •

edited

Loading

evilrobot-01 Jan 15, 2025

Daanvdplas Jan 17, 2025 •

edited

Loading

al3mart Jan 17, 2025

al3mart left a comment •

edited

Loading

al3mart Jan 14, 2025 •

edited

Loading

peterwht Jan 15, 2025

al3mart Jan 14, 2025 •

edited

Loading

evilrobot-01 Jan 15, 2025

al3mart Jan 17, 2025

codecov-commenter commented Jan 15, 2025

al3mart left a comment

evilrobot-01 left a comment

evilrobot-01 Jan 17, 2025

peterwht Jan 17, 2025

evilrobot-01 Jan 17, 2025

evilrobot-01 Jan 17, 2025

evilrobot-01 Jan 17, 2025

evilrobot-01 Jan 17, 2025

peterwht Jan 17, 2025

evilrobot-01 Jan 17, 2025

evilrobot-01 Jan 17, 2025

peterwht Jan 17, 2025

evilrobot-01 Jan 17, 2025

peterwht Jan 22, 2025

peterwht commented Jan 22, 2025

	substrate-build-script-utils = { git = "https://github.com/r0gue-io/polkadot-sdk", branch = "stable2412" }
	substrate-build-script-utils = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2412" }

feat(mainnet): add XCM runtime APIs, config tests, and refactor to API mod [DON'T MERGE] #426

feat(mainnet): add XCM runtime APIs, config tests, and refactor to API mod [DON'T MERGE] #426

Conversation

peterwht commented Jan 12, 2025 • edited Loading

peterwht commented Jan 12, 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

Daanvdplas Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al3mart left a comment • edited Loading

Choose a reason for hiding this comment

al3mart Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al3mart Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 15, 2025

Codecov Report

al3mart left a comment

Choose a reason for hiding this comment

evilrobot-01 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwht commented Jan 22, 2025

peterwht commented Jan 12, 2025 •

edited

Loading

Daanvdplas Jan 15, 2025 •

edited

Loading

Daanvdplas Jan 17, 2025 •

edited

Loading

al3mart left a comment •

edited

Loading

al3mart Jan 14, 2025 •

edited

Loading

al3mart Jan 14, 2025 •

edited

Loading