Fix DryRunApi client-facing XCM versions #7438

mrshiposha · 2025-02-03T15:30:40Z

Description

Integration

This PR makes breaking changes to the DryRunApi. The signature of the dry_run_call is changed, and the XCM version of the return values of dry_run_xcm now follows the version of the input XCM program.

If needed, this PR can be reworked so that the Runtime API is assigned a new version by adding a changed_in attribute to the dry_run_call.

Review Notes

The DryRunApi is modified
Added the Router::clear_messages to dry_run_xcm common implementation
The common implementation in the pallet-xcm is modified accordingly
The DryRunApi tests are modified to account for testing old XCM versions
The implementation from the pallet-xcm is used where it was not used (including the DryRunApi tests)
All the runtime implementations are modified according to the Runtime API change

polkadot/xcm/pallet-xcm/src/lib.rs

polkadot/xcm/xcm-runtime-apis/tests/fee_estimation.rs

bkchr · 2025-02-07T21:46:20Z

This PR makes breaking changes to the DryRunApi. The signature of the dry_run_call is changed, and the XCM version of the return values of dry_run_xcm now follows the version of the input XCM program.

Can you not just determine the XCM version of input xcm program and use this for the output? Otherwise you should at least bump the version of the runtime api.

mrshiposha · 2025-02-07T21:52:34Z

@bkchr

Can you not just determine the XCM version of input xcm program and use this for the output?

I did this for the dry_run_xcm. It was possible there. As for the dry_run_call, there is no input XCM program, just an arbitrary RuntimeCall.

Otherwise you should at least bump the version of the runtime API.

Sure, will do.

bkchr · 2025-02-07T22:06:18Z

As for the dry_run_call, there is no input XCM program, just an arbitrary RuntimeCall.

Somewhere in this RuntimeCall there will be a XCM? If not, you could just assume the minimum XCM version?

mrshiposha · 2025-02-07T22:20:25Z

We could examine the call and see if it is, say, transfer_assets from pallet-xcm. It seems to be a good enough default for common implementation.

I only worried about alternative XCM frontends like ORML pallet-xtokens. If the common implementation only considers pallet-xcm, the pallet-xtokens users will always receive the minimum XCM version.

CC @xlc

acatangiu · 2025-02-10T07:52:33Z

Assuming minimum is brittle, some new instructions do not have equivalents in older versions and conversion will fail. You could start with minimum and go through them until one successfully converts, but feels like overkill.

I'm personally happy with the current PR state where an explicit version is requested by the caller.

acatangiu · 2025-02-10T07:56:20Z

polkadot/xcm/xcm-runtime-apis/src/dry_run.rs

@@ -64,7 +64,7 @@ sp_api::decl_runtime_apis! {
 		OriginCaller: Encode
 	{
 		/// Dry run call.
-		fn dry_run_call(origin: OriginCaller, call: Call) -> Result<CallDryRunEffects<Event>, Error>;
+		fn dry_run_call(origin: OriginCaller, xcms_version: XcmVersion, call: Call) -> Result<CallDryRunEffects<Event>, Error>;


Having this param between origin and call makes one think it is somehow related to the call.

I would move it at the back, and also rename it to something more explicit like result_xcm_version/output_xcm_version/desired_xcm_version.

Suggested change

fn dry_run_call(origin: OriginCaller, xcms_version: XcmVersion, call: Call) -> Result<CallDryRunEffects<Event>, Error>;

fn dry_run_call(origin: OriginCaller, call: Call, result_xcms_version: XcmVersion) -> Result<CallDryRunEffects<Event>, Error>;

acatangiu · 2025-02-10T07:59:09Z

polkadot/xcm/xcm-runtime-apis/tests/fee_estimation.rs

@@ -354,6 +389,15 @@ fn dry_run_xcm() {
 	});
 }

+#[test]
+fn dry_run_xcm_versions() {
+	let tested_versions = [XCM_VERSION, 5, 4, 3];


this will have to be manually updated for new xcm versions, pls make it automatic

Suggested change

let tested_versions = [XCM_VERSION, 5, 4, 3];

let tested_versions = [XCM_VERSION, XCM_VERSION - 1, XCM_VERSION - 2, XCM_VERSION - 3];

XCM_VERSION - 3 would fail since V2 was removed.

Maybe we could introduce MIN_XCM_VERSION = 3 next to XCM_VERSION?
So that these test things would look like this:

let xcm_version_range = MIN_XCM_VERSION..=XCM_VERSION; for version in xcm_version_range { /* test for the version */ }

So that this will always be up-to-date

acatangiu · 2025-02-10T08:04:13Z

polkadot/xcm/pallet-xcm/src/lib.rs

-		frame_system::Pallet::<Runtime>::reset_events(); // To make sure we only record events from current call.
+
+		// To make sure we only record events from current call.
+		Router::clear_messages();


nice! I see this was done for dry_run_call but not for dry_run_xcm 🤦‍♂️

Yet, this fix breaks penpal runtime somehow...

It seems the xcmp-queue's clear_messages doesn't fix the page indices in the OutboundXcmpStatus and this causes penpal to break when using the common DryRunApi::dry_run_xcm implementation from pallet-xcm (see CI, broken tests::xcm_fee_estimation::multi_hop_works and tests::xcm_fee_estimation::multi_hop_pay_fees_works).

Does it influence prod, though? I have a feeling that these storages should be empty either way when running Runtime API properly.

Maybe the issue is with the test itself?

Does it influence prod, though? I have a feeling that these storages should be empty either way when running Runtime API properly.

Why should it be empty? Calling clear_messages here is correct, as otherwise the function will return invalid messages.

(That got send when building block the runtime api function is called on)

Yes, calling clear_messages() here is correct. Can you look into the tests and find out why exactly they are failing? Is it penpal runtime config issue or test issue?

Why should it be empty?

I just thought that maybe Runtime API runs with the state of the last finalized block. And I remembered that ParachainSystem's on_finalize clears the XCMP storages. However, it seems it does it partially anyway. So yeah, manual clearing before dry-running is needed anyway.

I will look into that.

mrshiposha added 4 commits February 3, 2025 16:17

fix: DryRunApi honors client's XCM version

047fb16

fix(test): DryRunApi tests old XCM versions

d66e18e

fix: DryRunApi runtime implementations

6804c15

fix: fmt

b2f2f06

mrshiposha requested a review from a team as a code owner February 3, 2025 15:30

mrshiposha added 5 commits February 3, 2025 17:57

fix: clippy

4bda5a7

fix: clippy

709a39f

fix: clippy

ba1b1bf

fix: integration tests

9ba3c46

fix: integration tests

84a29b9

acatangiu requested review from karolk91, x3c41a and raymondkfcheung February 4, 2025 09:24

x3c41a reviewed Feb 4, 2025

View reviewed changes

polkadot/xcm/pallet-xcm/src/lib.rs Show resolved Hide resolved

x3c41a reviewed Feb 4, 2025

View reviewed changes

polkadot/xcm/xcm-runtime-apis/tests/fee_estimation.rs Outdated Show resolved Hide resolved

x3c41a approved these changes Feb 4, 2025

View reviewed changes

mrshiposha added 2 commits February 5, 2025 13:53

refactor: runtime api version tests

f21fc2b

fix: clear messages before xcm dry run

1d77746

x3c41a approved these changes Feb 5, 2025

View reviewed changes

acatangiu approved these changes Feb 10, 2025

View reviewed changes

acatangiu added T6-XCM This PR/Issue is related to XCM. T4-runtime_API This PR/Issue is related to runtime APIs. labels Feb 10, 2025

raymondkfcheung approved these changes Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DryRunApi client-facing XCM versions #7438

Fix DryRunApi client-facing XCM versions #7438

mrshiposha commented Feb 3, 2025 •

edited

Loading

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

acatangiu commented Feb 10, 2025

acatangiu Feb 10, 2025

acatangiu Feb 10, 2025

mrshiposha Feb 10, 2025

acatangiu Feb 10, 2025

mrshiposha Feb 10, 2025

mrshiposha Feb 10, 2025

mrshiposha Feb 10, 2025

bkchr Feb 10, 2025

bkchr Feb 10, 2025

acatangiu Feb 11, 2025

mrshiposha Feb 11, 2025

	fn dry_run_call(origin: OriginCaller, xcms_version: XcmVersion, call: Call) -> Result<CallDryRunEffects<Event>, Error>;
	fn dry_run_call(origin: OriginCaller, call: Call, result_xcms_version: XcmVersion) -> Result<CallDryRunEffects<Event>, Error>;

	let tested_versions = [XCM_VERSION, 5, 4, 3];
	let tested_versions = [XCM_VERSION, XCM_VERSION - 1, XCM_VERSION - 2, XCM_VERSION - 3];

Fix DryRunApi client-facing XCM versions #7438

Are you sure you want to change the base?

Fix DryRunApi client-facing XCM versions #7438

Conversation

mrshiposha commented Feb 3, 2025 • edited Loading

Description

Integration

Review Notes

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

acatangiu commented Feb 10, 2025

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

mrshiposha commented Feb 3, 2025 •

edited

Loading