Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Fix bridge hub router #140

Closed
wants to merge 2 commits into from
Closed

Fix bridge hub router #140

wants to merge 2 commits into from

Conversation

yrong
Copy link

@yrong yrong commented Apr 15, 2024

Context

Currently for sending Ethereum asset by reserve_transfer_assets to work it requires to explicitly set xcm version of Ethereum Location on Assethub, e.g.

AssetHubRococo::force_xcm_version(
Location::new(2, [GlobalConsensus(Ethereum { chain_id: CHAIN_ID })]),
XCM_VERSION,
);

Actually this is unnessessary, IMO setting xcm version of Ethereum only make sense on BridgeHub to construct different Command to execute on Ethereum when there is xcm version specific logic on solidity side.

It seems more like a bug introduced by bridge-hub-router, more details in #140 (comment)

Comment on lines +344 to +345
let destination_version =
T::DestinationVersion::get_version_for(dest_ref).ok_or(SendError::NotApplicable)?;
Copy link
Author

@yrong yrong Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not throw the DestinationUnsupported error here, instead as required by Tuple::validate logic

match Tuple::validate(destination, message) {
Err(SendError::NotApplicable) => None,
Err(e) => { return Err(e) },
Ok((v, c)) => {
maybe_cost = Some(c);
Some(v)
},
, the implementation of SendXcm should only throw the error NotApplicable to give chance to the next impl of the SendXcm Tuple to process.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@yrong
Copy link
Author

yrong commented Apr 16, 2024

From the log integration test send_xcm_through_opened_lane_with_different_xcm_version_on_hops_works failed so this PR may introduce some side effects.

So just stop here until the issue confirmed by Parity team.

@yrong
Copy link
Author

yrong commented Apr 17, 2024

Closed in favor of paritytech#4162

@yrong yrong closed this Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants