Follow-up on #1518

[serban300]

Addressing the comments in #1518

The last commit is something that we didn't discuss in the comments, but it seemed like a good idea. It felt easier to me to work with the CustomNetworkId. I can drop it if it doesn't make sense.

[svyatonik]

I wouldn't add this function, because this code will be (I hope) used in the Rococo/Wococo/Kusama/Polkadot bridge hubs and (again - that' how I understand that) we'll be able to send XCM message from e.g. some Kusama parachain (Westmine) to other Polkadot parachain (e.g. Westmint). So the final destination isn't always the other side of the bridge - it may be any destination, which is routed by this router (verify_destination)

[serban300]

Sure, I can remove if it doesn't make sense. But I'm not sure if I understand what this scenario would suppose. So I have a couple of questions.

I added this function to deduplicate some code in verify_destination() and encode_send_xcm() (maybe absolute_destination() is not the right name also). For example

impl XcmBridge for ToRialtoParachainBridge {
        ...
	fn verify_destination(dest: &MultiLocation) -> bool {
		matches!(*dest, MultiLocation { parents: 1, interior: X2(GlobalConsensus(r), Parachain(RIALTO_PARACHAIN_ID)) } if r == RialtoNetwork::get())
	}
        ...
}

vs

impl CliEncodeMessage for Millau {
	fn encode_send_xcm(
		message: xcm::VersionedXcm<()>,
		bridge_instance_index: u8,
	) -> anyhow::Result<EncodedOrDecodedCall<Self::Call>> {
		let dest = match bridge_instance_index {
			...
			bridge::MILLAU_TO_RIALTO_PARACHAIN_INDEX => (
				Parent,
				X2(
					GlobalConsensus(millau_runtime::xcm_config::RialtoNetwork::get()),
					Parachain(RIALTO_PARACHAIN_ID),
				),
			),
			...
		};

		...
	}
       ...
}

How will these 2 pieces of code look like in the scenario that you mentioned ? Will there be any resemblance between them ? Or anything that we could reuse ?

[svyatonik]

The idea (again - that's my impression in how bridge would work) is that we need to build bridge between two bridge hubs, e.g. KusamaBridgeHub and PolkadotBridgeHub and then all Polkadot parachains will be able to "talk" to all Kusama parachains. So destination may be e.g. MultiLocation { parents: 1, interior: X2(GlobalConsensus(r), Parachain(WESTMINT_PARACHAIN_ID)) instead of MultiLocation { parents: 1, interior: X2(GlobalConsensus(r), Parachain(RIALTO_PARACHAIN_ID)) and still the message will be sent through the bridge and reach RIALTO_PARACHAIN_ID first. In some extreme cases we may even talk to other top-level chains using our bridge - e.g. (just an imaginary example, don't take it as a real world example) - we may bridge Bitcoin with Polkadot and Kusama with Ethereum. Then we may be able to send XCM messages from Bitcoin to Ethereum using Kusama <> Polkadot bridge.

TLDR: there's no single XCM destination for the bridge

[svyatonik]

If you want to keep it, then, please, change fn absolute_destination documentation, so that it states that it will return XCM destination of the other side of the bridge + probably change the function/name/documentation of the verify_absolute_destination method - it may handle any destinations, not only destinations "covered" by what the "absolute_destination" returns. It shall work :)

[serban300]

Oh ok, I think I understood. And in this case verify_destination() will become:

matches!(*dest, MultiLocation { parents: 1, interior: X2(GlobalConsensus(r), Parachain(_)) } if r == RialtoNetwork::get())

while encode_send_xcm() will stay the same.

So the code can't be reused. Makes sesnse. I will remove the function.

[serban300]

Done. I reverted that commit.

[svyatonik]

One tiny comment (may be opionated - feel free to ignore), otherwise seems good