Skip to content

Commit

Permalink
More fixes and do_try_state_for_bridge
Browse files Browse the repository at this point in the history
  • Loading branch information
@bkontur
bkontur committed Jul 29, 2024
1 parent 2431b1a commit c54ae92
Show file tree
Hide file tree
Showing 4 changed files with 203 additions and 25 deletions.
38 changes: 28 additions & 10 deletions bridges/modules/xcm-bridge-hub/src/dispatcher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,27 +124,45 @@ mod tests {
use crate::{mock::*, Bridges, LaneToBridge};

use bp_messages::{target_chain::DispatchMessageData, MessageKey};
use bp_xcm_bridge_hub::{Bridge, BridgeId, BridgeState};
use bp_xcm_bridge_hub::{Bridge, BridgeLocations, BridgeState};
use frame_support::assert_ok;
use sp_core::H256;

fn bridge() -> (BridgeId, LaneId) {
(BridgeId::from_inner(H256::from([1u8; 32])), LaneId::new(1, 2))
use xcm_executor::traits::ConvertLocation;

fn bridge() -> (Box<BridgeLocations>, LaneId) {
let origin = OpenBridgeOrigin::sibling_parachain_origin();
let with = bridged_asset_hub_universal_location();
let locations =
XcmOverBridge::bridge_locations_from_origin(origin, Box::new(with.into())).unwrap();
let lane_id = locations.calculate_lane_id(xcm::latest::VERSION).unwrap();
(locations, lane_id)
}

fn run_test_with_opened_bridge(test: impl FnOnce()) {
run_test(|| {
let (bridge, lane_id) = bridge();

Bridges::<TestRuntime, ()>::insert(
bridge().0,
bridge.bridge_id(),
Bridge {
bridge_origin_relative_location: Box::new(Location::new(0, Here).into()),
bridge_origin_relative_location: Box::new(
bridge.bridge_origin_relative_location().clone().into(),
),
bridge_origin_universal_location: Box::new(
bridge.bridge_origin_universal_location().clone().into(),
),
bridge_destination_universal_location: Box::new(
bridge.bridge_destination_universal_location().clone().into(),
),
state: BridgeState::Opened,
bridge_owner_account: [0u8; 32].into(),
bridge_owner_account: LocationToAccountId::convert_location(
bridge.bridge_origin_relative_location(),
)
.expect("valid accountId"),
reserve: 0,
lane_id: bridge().1,
lane_id,
},
);
LaneToBridge::<TestRuntime, ()>::insert(bridge().1, bridge().0);
LaneToBridge::<TestRuntime, ()>::insert(lane_id, bridge.bridge_id());
assert_ok!(XcmOverBridge::do_try_state());

test();
Expand Down
162 changes: 158 additions & 4 deletions bridges/modules/xcm-bridge-hub/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,12 @@ pub mod pallet {
bridge_origin_relative_location: Box::new(
locations.bridge_origin_relative_location().clone().into(),
),
bridge_origin_universal_location: Box::new(
locations.bridge_origin_universal_location().clone().into(),
),
bridge_destination_universal_location: Box::new(
locations.bridge_destination_universal_location().clone().into(),
),
state: BridgeState::Opened,
bridge_owner_account,
reserve: deposit,
Expand Down Expand Up @@ -524,6 +530,35 @@ pub mod pallet {
bridge_id: BridgeId,
bridge: BridgeOf<T, I>,
) -> Result<LaneId, sp_runtime::TryRuntimeError> {
// check `BridgeId` points to the same `LaneId` and vice versa.
ensure!(
Some(bridge_id) == Self::lane_to_bridge(bridge.lane_id),
"Found `LaneToBridge` inconsistency for bridge_id - missing mapping!"
);

// check that `locations` are convertible to the `latest` XCM.
let bridge_origin_relative_location_as_latest: &Location =
bridge.bridge_origin_relative_location.try_as().map_err(|_| {
"`bridge.bridge_origin_relative_location` cannot be converted to the `latest` XCM, needs migration!"
})?;
let bridge_origin_universal_location_as_latest: &InteriorLocation = bridge.bridge_origin_universal_location
.try_as()
.map_err(|_| "`bridge.bridge_origin_universal_location` cannot be converted to the `latest` XCM, needs migration!")?;
let bridge_destination_universal_location_as_latest: &InteriorLocation = bridge.bridge_destination_universal_location
.try_as()
.map_err(|_| "`bridge.bridge_destination_universal_location` cannot be converted to the `latest` XCM, needs migration!")?;

// check `BridgeId` does not change
ensure!(
bridge_id == BridgeId::new(bridge_origin_universal_location_as_latest, bridge_destination_universal_location_as_latest),
"`bridge_id` is different than calculated from `bridge_origin_universal_location_as_latest` and `bridge_destination_universal_location_as_latest`, needs migration!"
);

// check bridge account owner
ensure!(
T::BridgeOriginAccountIdConverter::convert_location(bridge_origin_relative_location_as_latest) == Some(bridge.bridge_owner_account),
"`bridge.bridge_owner_account` is different than calculated from `bridge.bridge_origin_relative_location`, needs migration!"
);

Ok(bridge.lane_id)
}
Expand Down Expand Up @@ -580,6 +615,12 @@ pub mod pallet {
bridge_origin_relative_location: Box::new(
locations.bridge_origin_relative_location().clone().into(),
),
bridge_origin_universal_location: Box::new(
locations.bridge_origin_universal_location().clone().into(),
),
bridge_destination_universal_location: Box::new(
locations.bridge_destination_universal_location().clone().into(),
),
state: BridgeState::Opened,
bridge_owner_account,
reserve: Zero::zero(),
Expand Down Expand Up @@ -699,6 +740,12 @@ mod tests {
bridge_origin_relative_location: Box::new(
locations.bridge_origin_relative_location().clone().into(),
),
bridge_origin_universal_location: Box::new(
locations.bridge_origin_universal_location().clone().into(),
),
bridge_destination_universal_location: Box::new(
locations.bridge_destination_universal_location().clone().into(),
),
state: BridgeState::Opened,
bridge_owner_account,
reserve,
Expand Down Expand Up @@ -852,6 +899,12 @@ mod tests {
bridge_origin_relative_location: Box::new(
locations.bridge_origin_relative_location().clone().into(),
),
bridge_origin_universal_location: Box::new(
locations.bridge_origin_universal_location().clone().into(),
),
bridge_destination_universal_location: Box::new(
locations.bridge_destination_universal_location().clone().into(),
),
state: BridgeState::Opened,
bridge_owner_account: [0u8; 32].into(),
reserve: 0,
Expand Down Expand Up @@ -977,6 +1030,12 @@ mod tests {
bridge_origin_relative_location: Box::new(
locations.bridge_origin_relative_location().clone().into()
),
bridge_origin_universal_location: Box::new(
locations.bridge_origin_universal_location().clone().into(),
),
bridge_destination_universal_location: Box::new(
locations.bridge_destination_universal_location().clone().into(),
),
state: BridgeState::Opened,
bridge_owner_account: bridge_owner_account.clone(),
reserve: expected_reserve,
Expand Down Expand Up @@ -1254,12 +1313,19 @@ mod tests {
fn do_try_state_works() {
use sp_runtime::Either;

let older_xcm_version = xcm::latest::VERSION - 1;
let bridge_origin_relative_location = Location::new(1, [Parachain(2025)]);
let bridge_origin_relative_location = SiblingLocation::get();
let bridge_origin_universal_location = SiblingUniversalLocation::get();
let bridge_destination_universal_location = BridgedUniversalDestination::get();
let bridge_owner_account =
LocationToAccountId::convert_location(&bridge_origin_relative_location)
.expect("valid accountId");
let bridge_id = BridgeId::from_inner(H256::from([1u8; 32]));
let bridge_owner_account_mismatch =
LocationToAccountId::convert_location(&Location::parent()).expect("valid accountId");
let bridge_id = BridgeId::new(
&bridge_origin_universal_location,
&bridge_destination_universal_location,
);
let bridge_id_mismatch = BridgeId::new(&InteriorLocation::Here, &InteriorLocation::Here);
let lane_id = LaneId::from_inner(Either::Left(H256::default()));

let test_bridge_state =
Expand All @@ -1269,11 +1335,16 @@ mod tests {

let result = XcmOverBridge::do_try_state();
if let Some(e) = expected_error {
assert_err!(XcmOverBridge::do_try_state(), e);
assert_err!(result, e);
} else {
assert_ok!(result);
}
};
let cleanup = |bridge_id, lane_id| {
Bridges::<TestRuntime, ()>::remove(bridge_id);
LaneToBridge::<TestRuntime, ()>::remove(lane_id);
assert_ok!(XcmOverBridge::do_try_state());
};

run_test(|| {
// ok state
Expand All @@ -1283,6 +1354,14 @@ mod tests {
bridge_origin_relative_location: Box::new(VersionedLocation::from(
bridge_origin_relative_location.clone(),
)),
bridge_origin_universal_location: Box::new(VersionedInteriorLocation::from(
bridge_origin_universal_location.clone(),
)),
bridge_destination_universal_location: Box::new(
VersionedInteriorLocation::from(
bridge_destination_universal_location.clone(),
),
),
state: BridgeState::Opened,
bridge_owner_account: bridge_owner_account.clone(),
reserve: Zero::zero(),
Expand All @@ -1291,6 +1370,81 @@ mod tests {
(lane_id, bridge_id),
None,
);
cleanup(bridge_id, lane_id);

// error - missing `LaneToBridge` mapping
test_bridge_state(
bridge_id,
Bridge {
bridge_origin_relative_location: Box::new(VersionedLocation::from(
bridge_origin_relative_location.clone(),
)),
bridge_origin_universal_location: Box::new(VersionedInteriorLocation::from(
bridge_origin_universal_location.clone(),
)),
bridge_destination_universal_location: Box::new(
VersionedInteriorLocation::from(
bridge_destination_universal_location.clone(),
),
),
state: BridgeState::Opened,
bridge_owner_account: bridge_owner_account.clone(),
reserve: Zero::zero(),
lane_id,
},
(lane_id, bridge_id_mismatch),
Some(TryRuntimeError::Other(
"Found `LaneToBridge` inconsistency for bridge_id - missing mapping!",
)),
);
cleanup(bridge_id, lane_id);

// error bridge owner account cannot be calculated
test_bridge_state(
bridge_id,
Bridge {
bridge_origin_relative_location: Box::new(VersionedLocation::from(
bridge_origin_relative_location.clone(),
)),
bridge_origin_universal_location: Box::new(VersionedInteriorLocation::from(
bridge_origin_universal_location.clone(),
)),
bridge_destination_universal_location: Box::new(VersionedInteriorLocation::from(
bridge_destination_universal_location.clone(),
)),
state: BridgeState::Opened,
bridge_owner_account: bridge_owner_account_mismatch.clone(),
reserve: Zero::zero(),
lane_id,
},
(lane_id, bridge_id),
Some(TryRuntimeError::Other("`bridge.bridge_owner_account` is different than calculated from `bridge.bridge_origin_relative_location`, needs migration!")),
);
cleanup(bridge_id, lane_id);

// error when (bridge_origin_universal_location + bridge_destination_universal_location)
// produces different `BridgeId`
test_bridge_state(
bridge_id_mismatch,
Bridge {
bridge_origin_relative_location: Box::new(VersionedLocation::from(
bridge_origin_relative_location.clone(),
)),
bridge_origin_universal_location: Box::new(VersionedInteriorLocation::from(
bridge_origin_universal_location.clone(),
)),
bridge_destination_universal_location: Box::new(VersionedInteriorLocation::from(
bridge_destination_universal_location.clone(),
)),
state: BridgeState::Opened,
bridge_owner_account: bridge_owner_account_mismatch.clone(),
reserve: Zero::zero(),
lane_id,
},
(lane_id, bridge_id_mismatch),
Some(TryRuntimeError::Other("`bridge_id` is different than calculated from `bridge_origin_universal_location_as_latest` and `bridge_destination_universal_location_as_latest`, needs migration!")),
);
cleanup(bridge_id_mismatch, lane_id);
});
}
}
1 change: 1 addition & 0 deletions bridges/modules/xcm-bridge-hub/src/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ parameter_types! {
Parachain(THIS_BRIDGE_HUB_ID),
].into();
pub SiblingLocation: Location = Location::new(1, [Parachain(SIBLING_ASSET_HUB_ID)]);
pub SiblingUniversalLocation: InteriorLocation = [GlobalConsensus(RelayNetwork::get()), Parachain(SIBLING_ASSET_HUB_ID)].into();

pub const BridgedRelayNetwork: NetworkId = NetworkId::Polkadot;
pub BridgedRelayNetworkLocation: Location = (Parent, GlobalConsensus(BridgedRelayNetwork::get())).into();
Expand Down
27 changes: 16 additions & 11 deletions bridges/primitives/xcm-bridge-hub/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ impl BridgeId {
///
/// Note: The `BridgeId` is constructed from `latest` XCM, so if stored, you need to ensure
/// compatibility with newer XCM versions.
fn new(universal_source: &InteriorLocation, universal_destination: &InteriorLocation) -> Self {
pub fn new(
universal_source: &InteriorLocation,
universal_destination: &InteriorLocation,
) -> Self {
const VALUES_SEPARATOR: [u8; 33] = *b"bridges-bridge-id-value-separator";

BridgeId(
Expand All @@ -82,13 +85,6 @@ impl BridgeId {
.into(),
)
}

/// Create bridge identifier from given hash.
/// (for testing purposes)
#[cfg(any(feature = "std", test))]
pub const fn from_inner(inner: H256) -> Self {
BridgeId(inner)
}
}

/// Local XCM channel manager.
Expand Down Expand Up @@ -152,8 +148,17 @@ pub enum BridgeState {
)]
#[scale_info(skip_type_params(ThisChain))]
pub struct Bridge<ThisChain: Chain> {
/// Relative location of the bridge origin chain.
/// Relative location of the bridge origin chain. This is expected to be **convertible** to the
/// `latest` XCM, so the check and migration needs to be ensured.
pub bridge_origin_relative_location: Box<VersionedLocation>,

/// See [`BridgeLocations::bridge_origin_universal_location`].
/// Stored for `BridgeId` sanity check.
pub bridge_origin_universal_location: Box<VersionedInteriorLocation>,
/// See [`BridgeLocations::bridge_destination_universal_location`].
/// Stored for `BridgeId` sanity check.
pub bridge_destination_universal_location: Box<VersionedInteriorLocation>,

/// Current bridge state.
pub state: BridgeState,
/// Account with the reserved funds. Derived from `self.bridge_origin_relative_location`.
Expand Down Expand Up @@ -374,8 +379,8 @@ mod tests {
.bridge_destination_universal_location
.clone(),
bridge_id: BridgeId::new(
&test.bridge_origin_universal_location.into(),
&test.bridge_destination_universal_location.into(),
&test.bridge_origin_universal_location,
&test.bridge_destination_universal_location,
),
})),
);
Expand Down

0 comments on commit c54ae92

Please sign in to comment.