Send messages using xcm pallet by svyatonik · Pull Request #1518 · paritytech/parity-bridges-common

svyatonik · 2022-07-20T13:05:54Z

TODOs:

do we still need an ability to send messages using messages pallet directly?;
add RialtoParachain to Millau XCM configuration (and vice versa);
impl encode_send_xcm for remaining chains;
tests;
use in deployments/

svyatonik · 2022-07-27T12:12:05Z

One important note Rialto -> Millau messages are still being sent using messages pallet. The reason is that the rational relayer is unable to deliver XCM messages if they're sent without conversion rate override (and when the on-chain rate is obsolete). Let it be here for some time - I have a feeling that we may still need rational relayer in the future.

serban300 · 2022-08-10T12:18:00Z

+	/// Encode a send XCM call of the XCM pallet.
+	fn encode_send_xcm(
+		message: xcm::VersionedXcm<()>,
+		bridge_instance_index: u8,


Nit: Could we use the FullBridge enum here directly ?

serban300 · 2022-08-10T12:55:45Z

+		Ok(match bridge_instance_index {
+			bridge::MILLAU_TO_RIALTO_INDEX => {
+				let dest =
+					(Parent, X1(GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get())));
+				millau_runtime::Call::XcmPallet(millau_runtime::XcmCall::send {
+					dest: Box::new(dest.into()),
+					message: Box::new(message),
+				})
+				.into()
+			},
+			bridge::MILLAU_TO_RIALTO_PARACHAIN_INDEX => {
+				let dest = (
+					Parent,
+					X2(
+						GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get()),
+						Parachain(RIALTO_PARACHAIN_ID),
+					),
+				);
+				millau_runtime::Call::XcmPallet(millau_runtime::XcmCall::send {
+					dest: Box::new(dest.into()),
+					message: Box::new(message),
+				})
+				.into()
+			},
+			_ => anyhow::bail!(
+				"Unsupported target bridge pallet with instance index: {}",
+				bridge_instance_index
+			),
+		})


Since the message is the same for both cases we could de-duplicate this a bit by transforming it into:

let dest = match bridge_instance_index { bridge::MILLAU_TO_RIALTO_INDEX => (Parent, X1(GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get()))), bridge::MILLAU_TO_RIALTO_PARACHAIN_INDEX => ( Parent, X2( GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get()), Parachain(RIALTO_PARACHAIN_ID), ), ), _ => anyhow::bail!( "Unsupported target bridge pallet with instance index: {}", bridge_instance_index ), }; Ok(millau_runtime::Call::XcmPallet(millau_runtime::XcmCall::send { dest: Box::new(dest.into()), message: Box::new(message), }) .into())

serban300 · 2022-08-10T13:17:24Z

+			bridge::MILLAU_TO_RIALTO_INDEX => {
+				let dest =
+					(Parent, X1(GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get())));
+				millau_runtime::Call::XcmPallet(millau_runtime::XcmCall::send {
+					dest: Box::new(dest.into()),
+					message: Box::new(message),
+				})
+				.into()
+			},
+			bridge::MILLAU_TO_RIALTO_PARACHAIN_INDEX => {
+				let dest = (
+					Parent,
+					X2(
+						GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get()),
+						Parachain(RIALTO_PARACHAIN_ID),
+					),
+				);


This destinations seem very similar to what we check in XcmBridge::verify_destination() (ToRialtoBridge, ToRialtoParachainBridge) and I was wondering if we could have something that we could use in both places.

Yeah, that's a good idea

serban300 · 2022-08-10T14:01:00Z

+			}
+
+			let route = T::build_destination();
+			let msg = (route, msg.take().unwrap()).encode();


Should we check the msg first and return MissingArgument if it is none ? Instead of panicking.

Yeah - that's obviously my fault. Can you, please, fix it? Thank you

Sure, thanks for the answers. I will implement them in a follow-up PR.

serban300 · 2022-08-10T14:52:44Z

-			fee.cast().into(),
-			data.bridge.bridge_instance_index(),
-		)?;
+		let send_message_call = if data.use_xcm_pallet {


Nits, not related to this PR:

I think that we can modify estimate_message_delivery_and_dispatch_fee to receive a reference to the payload in order to avoid cloning it. If we handle very large messages, this should help.

On the line above we might be able to use payload.encoded_size(), which should be more efficient.

serban300 · 2022-08-10T16:00:07Z


 # Polkadot Dependencies

+#pallet-xcm = { git = "https://github.com/paritytech/polkadot", branch = "gav-xcm-v3", default-features = false }


Nit: Is this line needed ?

Nope, thanks for spotting!

serban300 · 2022-08-10T16:07:10Z

+	/// The Millau network ID, associated with Kusama.
+	pub const MillauNetwork: NetworkId = Kusama;
+	/// The RialtoParachain network ID, associated with Westend.
+	pub const ThisNetwork: NetworkId = Westend;


Sorry, it might be something simple, but I couldn't udnerstand it so far. Why is MillauNetwork == Kusama and ThisNetwork == Westend ?

It's just an association - there's this existing NetworkId enum in XCM, but obviously Rialto and Millau are not there. We have also associated Rialto/Millau tokens with Polakdot/Kusama tokens, so this association is kept. For RialtoParachain it's just new association

* Adjustments for the xcm messages sending logic Signed-off-by: Serban Iorga <serban@parity.io> * Deduplicate XCM destination Signed-off-by: Serban Iorga <serban@parity.io> * [send_message] small changes Signed-off-by: Serban Iorga <serban@parity.io> * Define CustomNetworkId Right now we use some associations between Rialto, RialtoParachain and Millau chains and chains defined in the NetworkId enum. But if we are not carreful we might do mistakes like: In Millau: pub const ThisNetwork: NetworkId = Kusama; pub const RialtoNetwork: NetworkId = Polkadot; In Rialto: pub const ThisNetwork: NetworkId = Kusama; pub const MillauNetwork: NetworkId = Polkadot; We're introducing CustomNetworkId to have a centralized mapping between NetworkId chains and our custom chains. Signed-off-by: Serban Iorga <serban@parity.io> * Revert "Deduplicate XCM destination" This reverts commit 3a0a950. Signed-off-by: Serban Iorga <serban@parity.io>

serban300 · 2022-08-12T09:25:23Z

The comments were addressed in #1546

Marking this PR as reviewed.

* changes to read json spec in test binary * add zombienet tests * fmt * use {{COL_IMAGE}} and clean config * add comment and use relay image from env * use test-parachain image from pr * fix warns * fix warns * fmt * typo * fix ci to use zombienet image * fix spawn nodes for test * reorg test * add within to test * remove check for full node collators is up * add tests for pov, mirate solo to para, sync blocks * bump zombienet image * add job dep with artifacts * add sleep for test * fix after merge * fmt * bump zombienet version * changes from clap * use base/shared params * fmt * debug ci * add upgrade test * update js test for debug * less debug in test * print assertion * fix upgrade test * Collator key only needed if we run as collator * [Fix] Benchmark build artifact folder creation (#1518) * Trivial networking changes for Substrate PR #11940 (#1486) * Trivial networking changes for Substrate PR paritytech/substrate#11940 * Apply formatting rules * update lockfile for {"polkadot", "substrate"} Co-authored-by: parity-processbot <> * bump zombienet version * update network def for test * typo Co-authored-by: Sebastian Kunert <skunert49@gmail.com> Co-authored-by: Roman Useinov <roman.useinov@gmail.com> Co-authored-by: Nazar Mokrynskyi <nazar@mokrynskyi.com>

* send messages using xcm pallet * XcmBridge && XcmBridgeAdapter + (untested) config in RialtoParachain * impl encode_send_xcm for the rest * remove duplicate code * some fixes * cleanup * some more tests * cleanup * cleanup * send Rialto -> Millau messages using bridge-messages pallet * fmt * some clippy fixes * more clippy

* Adjustments for the xcm messages sending logic Signed-off-by: Serban Iorga <serban@parity.io> * Deduplicate XCM destination Signed-off-by: Serban Iorga <serban@parity.io> * [send_message] small changes Signed-off-by: Serban Iorga <serban@parity.io> * Define CustomNetworkId Right now we use some associations between Rialto, RialtoParachain and Millau chains and chains defined in the NetworkId enum. But if we are not carreful we might do mistakes like: In Millau: pub const ThisNetwork: NetworkId = Kusama; pub const RialtoNetwork: NetworkId = Polkadot; In Rialto: pub const ThisNetwork: NetworkId = Kusama; pub const MillauNetwork: NetworkId = Polkadot; We're introducing CustomNetworkId to have a centralized mapping between NetworkId chains and our custom chains. Signed-off-by: Serban Iorga <serban@parity.io> * Revert "Deduplicate XCM destination" This reverts commit 3a0a950. Signed-off-by: Serban Iorga <serban@parity.io>

* send messages using xcm pallet * XcmBridge && XcmBridgeAdapter + (untested) config in RialtoParachain * impl encode_send_xcm for the rest * remove duplicate code * some fixes * cleanup * some more tests * cleanup * cleanup * send Rialto -> Millau messages using bridge-messages pallet * fmt * some clippy fixes * more clippy

* Adjustments for the xcm messages sending logic Signed-off-by: Serban Iorga <serban@parity.io> * Deduplicate XCM destination Signed-off-by: Serban Iorga <serban@parity.io> * [send_message] small changes Signed-off-by: Serban Iorga <serban@parity.io> * Define CustomNetworkId Right now we use some associations between Rialto, RialtoParachain and Millau chains and chains defined in the NetworkId enum. But if we are not carreful we might do mistakes like: In Millau: pub const ThisNetwork: NetworkId = Kusama; pub const RialtoNetwork: NetworkId = Polkadot; In Rialto: pub const ThisNetwork: NetworkId = Kusama; pub const MillauNetwork: NetworkId = Polkadot; We're introducing CustomNetworkId to have a centralized mapping between NetworkId chains and our custom chains. Signed-off-by: Serban Iorga <serban@parity.io> * Revert "Deduplicate XCM destination" This reverts commit 3a0a950. Signed-off-by: Serban Iorga <serban@parity.io>

send messages using xcm pallet

1a923cc

svyatonik added the P-Relay label Jul 20, 2022

svyatonik added 8 commits July 25, 2022 14:15

XcmBridge && XcmBridgeAdapter + (untested) config in RialtoParachain

26adc31

impl encode_send_xcm for the rest

be4da3f

remove duplicate code

f74c48e

some fixes

b39b769

cleanup

aa02892

some more tests

98df05c

cleanup

69621bc

cleanup

138a49b

svyatonik mentioned this pull request Jul 27, 2022

Consider adding losing-funds alert for relayer running in altruistic mode #1522

Closed

svyatonik added 4 commits July 27, 2022 14:24

send Rialto -> Millau messages using bridge-messages pallet

9f31119

fmt

f8afc12

some clippy fixes

3fd150a

more clippy

c458516

svyatonik marked this pull request as ready for review July 27, 2022 11:57

svyatonik enabled auto-merge (squash) July 27, 2022 12:14

svyatonik merged commit 9ad43b6 into master Jul 27, 2022

svyatonik deleted the send-messages-using-xcm-pallet branch July 27, 2022 12:15

serban300 reviewed Aug 10, 2022

View reviewed changes

serban300 mentioned this pull request Aug 11, 2022

Follow-up on #1518 #1546

Merged

boundless-forest mentioned this pull request Sep 9, 2022

Sync upstream darwinia-network/darwinia-messages-substrate#174

Closed

30 tasks

jiguantong mentioned this pull request Oct 26, 2022

LCMP support XCM darwinia-network/darwinia-messages-substrate#218

Closed

10 tasks

jiguantong added a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Oct 27, 2022

Companion for paritytech/parity-bridges-common#1518

2673e9c

jiguantong added a commit to darwinia-network/darwinia-parachain that referenced this pull request Oct 31, 2022

Companion for paritytech/parity-bridges-common#1518

47fc92e

jiguantong pushed a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Apr 13, 2023

Companion for paritytech/parity-bridges-common#1518

0df6204

svyatonik pushed a commit that referenced this pull request Jul 17, 2023

[Fix] Benchmark build artifact folder creation (#1518)

ed57c8a


		# Polkadot Dependencies

		#pallet-xcm = { git = "https://github.com/paritytech/polkadot", branch = "gav-xcm-v3", default-features = false }

Conversation

svyatonik commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svyatonik commented Jul 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serban300 commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

svyatonik commented Jul 20, 2022 •

edited

Loading