Add XCM Precompile to pallet-xcm

[tiagobndr]

This PR adds the XCM precompile (with xcmSend, xcmExecute and weightMessage functionalities) to pallet-xcm.

This follows the discussion we had on the now closed PR, which attempted to add the precompile to pallet-revive, but that approach would have introduced unwanted cyclic dependencies. That's why we decided to migrate the precompile to pallet-xcm, avoiding adding unnecessary dependencies to pallet-revive.

This PR should also encapsulate unit tests in precompiles.rs as well as integration tests under cumulus/parachains/integration-tests/emulated/tests.

See tracking parent issue

[tiagobndr]

/cmd fmt

[franciscoaguirre]

We don't return anything from either xcmExecute or xcmSend, should we at least let the caller know if it was successful? Or we just revert?

[sphamjoli]

What about emitting an event @franciscoaguirre

[tiagobndr]

What about emitting an event @franciscoaguirre

We get that out of the box from calling pallet_xcm::send() or pallet_xcm::execute() - e.g. here

[tiagobndr]

We don't return anything from either xcmExecute or xcmSend, should we at least let the caller know if it was successful? Or we just revert?

From the call to the precompile, we get back whatever gets returned from pallet_xcm::send() or pallet_xcm::execute(). It's just that we don't explicitly set it in the interface because we don't know its type in advance

[xermicus]

I don't like this interface. In practice, as @franciscoaguirre pointed out, how would you even know if it succeeded or not?

I suggest to version the interface in one way or the other, such that it will remain stable (then return values are possible - even if they are just untyped bytes[] the caller at least get's a chance to decode them, can be added later).

[xermicus]

FWIW if the precompile reverts on errors, callers can at least check the call success variable in Solidity. But versioning it explicitly still seems like a good idea to me. Unless to whatever this is calling into is absolutely stable forever? I can't really imagine that.

[xermicus]

And those things should be documented in the interface as well.

[xermicus]

And pleases also consider this: Given the precompile reverts on error, any change in the conditions when reverts happen is a breaking change. Unless I am missing things I don't yet see how this will work out unless we get a mapping of exactly one (versioned) interface to one precompile contract address.

[sphamjoli]

@tiagobndr @franciscoaguirre should precompile.rs not be named precompile.rs

[sphamjoli]

/cmd fmt

[tiagobndr]

@tiagobndr @franciscoaguirre should precompile.rs not be named precompile.rs

I followed the same convention (precompiles.rs) used in the ERC20 precompile (PR#8554). I don't have a strong opinion on it but one reason this approach could make sense is that we might add additional XCM precompiles to the same file in the future

[acatangiu]

Looks good. Left a couple of comments.

[tiagobndr]

/cmd fmt

[github-actions]

Review required! Latest push from author must always be reviewed

[athei]

1. XCM is already versioned. We are just routing through the XCM as-is which preserves those properties. Please note that we read a VersionedXcm struct from the contract. So we should be fine here.
2. Please note that contracts can match on the revert strings here. So they can never change. That might be really annoying for a string. I suggest just returning an Error which traps without an error to catch on.

[athei]

Sorry have to block until my comment is addressed. When adding the dispatchable weight to the pre-charged weight it also needs to be added to the actual weight. Otherwise it is only included once. Or am I wrong here?

[franciscoaguirre]

@athei I commited in faceac5 a small fix regarding weights. The execute call has to take in the weight passed in as a parameter to the precompile, not what I called weight_to_charge which is that weight + the benchmark for execute.

The benchmarked weight for execute doesn't need to be added to the post dispatch info because the call itself adds it here in the case of a simple pre-validation error and here in the case of a partial execution of the XCM.

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/15578396319
Failed job name: test-linux-stable-no-try-runtime

[athei]

Ahh yes I get it now. I missed that you are calling into a dispatch able that already includes its own weight. Thought you were calling into the executor directly.

[xermicus]

@tiagobndr @Brianspha my review comments were dismissed entirely. Why? Developer experience wise, this is a disaster. This isn't done yet, please fix with a follow up PR. Also don't forget to document it externally on our docs (here / here).

[athei]

But wait. Didn't we discuss that you detect whether this failed by catching the panic? But I guess this is really bad because in Solidity panics are silently ignored by default (lol).

[tiagobndr]

@tiagobndr @Brianspha my review comments were dismissed entirely. Why? Developer experience wise, this is a disaster. This isn't done yet, please fix with a follow up PR.

@xermicus you’re right that the DevEx could be improved. We opened PR #9023 to address this — would really appreciate your feedback on it when you have a moment

[xermicus]

Ah that's great, thanks! Didn't see 9023, happy to take a look once it's ready.

Just think about it from this perspective: You are a Solidity dev (know nothing about Polkadot) and try to do XCM. I was already asked exactly this a couple times. Ideally our interfaces together with doc comments and broader documentation are written well enough so we don't need to hand hold them at all.