Skip to content

(Suggestion) Ability to externalize configuration for ExporterFor #2313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
svyatonik merged 7 commits into from
Aug 3, 2023
Merged

(Suggestion) Ability to externalize configuration for ExporterFor #2313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f8db47c
Ability to externalize configuration for `ExporterFor`
bkontur Aug 1, 2023
6876de7
Fix millau
bkontur Aug 1, 2023
6033f69
Compile fix
bkontur Aug 1, 2023
b3ee5dc
Merge remote-tracking branch 'origin/dynamic-fees-v1' into bko-dynami…
bkontur Aug 2, 2023
a3fa48f
Return back `BridgedNetworkId` but as optional filter
bkontur Aug 2, 2023
623c9c0
Replaced `BaseFee` with fees from inner `Bridges: ExporterFor`
bkontur Aug 2, 2023
0119a43
typo
bkontur Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions bin/millau/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ use sp_std::prelude::*;
#[cfg(feature = "std")]
use sp_version::NativeVersion;
use sp_version::RuntimeVersion;
use xcm_builder::NetworkExportTable;

// to be able to use Millau runtime in `bridge-runtime-common` tests
pub use bridge_runtime_common;
Expand Down Expand Up @@ -546,17 +547,20 @@ impl pallet_utility::Config for Runtime {

// this config is totally incorrect - the pallet is not actually used at this runtime. We need
// it only to be able to run benchmarks and make required traits (and default weights for tests).
parameter_types! {
pub BridgeTable: Vec<(xcm::prelude::NetworkId, xcm::prelude::MultiLocation, Option<xcm::prelude::MultiAsset>)>
= vec![(xcm_config::RialtoNetwork::get(), xcm_config::TokenLocation::get(), Some((xcm_config::TokenAssetId::get(), 1_000_000_000_u128).into()))];
}
impl pallet_xcm_bridge_hub_router::Config for Runtime {
type WeightInfo = ();

type UniversalLocation = xcm_config::UniversalLocation;
type SiblingBridgeHubLocation = xcm_config::TokenLocation;
type BridgedNetworkId = xcm_config::RialtoNetwork;
type Bridges = NetworkExportTable<BridgeTable>;

type ToBridgeHubSender = xcm_config::XcmRouter;
type WithBridgeHubChannel = xcm_config::EmulatedSiblingXcmpChannel;

type BaseFee = ConstU128<1_000_000_000>;
type ByteFee = ConstU128<1_000>;
type FeeAsset = xcm_config::TokenAssetId;
}
Expand Down
75 changes: 60 additions & 15 deletions modules/xcm-bridge-hub-router/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,21 @@ pub mod pallet {

/// Universal location of this runtime.
type UniversalLocation: Get<InteriorMultiLocation>;
/// Relative location of the sibling bridge hub.
type SiblingBridgeHubLocation: Get<MultiLocation>;
/// The bridged network that this config is for.
type BridgedNetworkId: Get<NetworkId>;
/// The bridged network that this config is for if specified.
/// Also used for filtering `Bridges` by `BridgedNetworkId`.
/// If not specified, allows all networks pass through.
type BridgedNetworkId: Get<Option<NetworkId>>;
/// Configuration for supported **bridged networks/locations** with **bridge location** and
/// **possible fee**. Allows to externalize better control over allowed **bridged
/// networks/locations**.
type Bridges: ExporterFor;

/// Actual message sender (`HRMP` or `DMP`) to the sibling bridge hub location.
type ToBridgeHubSender: SendXcm;
/// Underlying channel with the sibling bridge hub. It must match the channel, used
/// by the `Self::ToBridgeHubSender`.
type WithBridgeHubChannel: LocalXcmChannel;

/// Base bridge fee that is paid for every outbound message.
type BaseFee: Get<u128>;
/// Additional fee that is paid for every byte of the outbound message.
type ByteFee: Get<u128>;
/// Asset that is used to paid bridge fee.
Expand Down Expand Up @@ -177,32 +179,75 @@ type ViaBridgeHubExporter<T, I> = SovereignPaidRemoteExporter<
impl<T: Config<I>, I: 'static> ExporterFor for Pallet<T, I> {
fn exporter_for(
network: &NetworkId,
_remote_location: &InteriorMultiLocation,
remote_location: &InteriorMultiLocation,
message: &Xcm<()>,
) -> Option<(MultiLocation, Option<MultiAsset>)> {
// ensure that the message is sent to the expected bridged network
if *network != T::BridgedNetworkId::get() {
return None
// ensure that the message is sent to the expected bridged network (if specified).
if let Some(bridged_network) = T::BridgedNetworkId::get() {
if *network != bridged_network {
log::trace!(
target: LOG_TARGET,
"Router with bridged_network_id {:?} does not support bridging to network {:?}!",
bridged_network,
network,
);
return None
}
}

// ensure that the message is sent to the expected bridged network and location.
let Some((bridge_hub_location, maybe_payment)) = T::Bridges::exporter_for(network, remote_location, message) else {
Copy link
Copy Markdown
Contributor

@svyatonik svyatonik Aug 2, 2023

Choose a reason for hiding this comment

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

What concerns me here is exactly: "another small positive side-effect is that for bridging to ethereum or other consensus, we dont need to add another xcm-bridge-hub-router instance, we just change Bridges cfg". IDK what will be the cost of with-Ethereum bridge, but my guess it'll be a bit more expensive than ours. So maybe they'll be using different BaseFee and ByteFee and we'll need to have a different instance here, or use something more sophisticated configuration trait than ExporterFor.

Also - I admit that our current dynamic fees architecture will work fine with a single remote consensus. But I have not been thinking about multiple remote consensus systems. If with-Polkadot bridge is congested (e.g. because relayer is not running), shall we also pause the with-Ethereum bridge? It might be a problem and I have no answer right now. Thanks for spotting that. Please share your thoughts here.

Copy link
Copy Markdown
Contributor

@svyatonik svyatonik Aug 2, 2023

Choose a reason for hiding this comment

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

But I have not been thinking about multiple remote consensus systems

Maybe we should do some hybrid solution for v2. I.e. if one of bridges is congested, then instead of blocking the XCM channel with sending chain, it shall send XCM message to that chain with the congested bridge id. And then, if it keeps receiving messages to be sent over this bridge, suspend the XCM channel. Anyway - imo that's related to v2, right? We shall track this in some issue

Copy link
Copy Markdown
Contributor

@svyatonik svyatonik Aug 2, 2023

Choose a reason for hiding this comment

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

I've filed #2315 to track that

Copy link
Copy Markdown
Collaborator Author

@bkontur bkontur Aug 2, 2023

Choose a reason for hiding this comment

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

ok, thank you, you're right with your concerns, I totally got it now,
I returned back BridgedNetwotkId, but as optional, that user can decide, if he needs separate router for bridged network or not, with consequences of the same BaseFee/ByteFee and backpressure #2315,
or do you thing we should be better strict here with type BridgedNetworkId: Get<NetworkId>;?

Copy link
Copy Markdown
Contributor

@svyatonik svyatonik Aug 2, 2023

Choose a reason for hiding this comment

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

Let's keep as is. For v1 (hopefully) we'll deal with the single remote network only, right? For v2 - let's see if/how we'll implement the #2315

Copy link
Copy Markdown
Collaborator Author

@bkontur bkontur Aug 2, 2023

Choose a reason for hiding this comment

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

If with-Polkadot bridge is congested (e.g. because relayer is not running), shall we also pause the with-Ethereum bridge? It might be a problem and I have no answer right now. Thanks for spotting that. Please share your thoughts here.

some thoughts here

Copy link
Copy Markdown
Collaborator Author

@bkontur bkontur Aug 2, 2023

Choose a reason for hiding this comment

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

Let's keep as is. For v1 (hopefully) we'll deal with the single remote network only, right? For v2 - let's see if/how we'll implement the #2315

well, hard to say, for example, there is a snowfork ethereum bridge comming, not sure if it will be part of v1 or v2?
other use-case is bridging polkadot-collectives to kusama-somewhere (propagating governance origins for technical fellowship), but thats probably v2 (at least after asset transfer)

log::trace!(
target: LOG_TARGET,
"Router with bridged_network_id {:?} does not support bridging to network {:?} and remote_location {:?}!",
T::BridgedNetworkId::get(),
network,
remote_location,
);
return None
};

// take `base_fee` from `T::Brides`, but it has to be the same `T::FeeAsset`
let base_fee = match maybe_payment {
Some(payment) => match payment {
MultiAsset { fun: Fungible(amount), id } if id.eq(&T::FeeAsset::get()) => amount,
invalid_asset @ _ => {
log::error!(
target: LOG_TARGET,
"Router with bridged_network_id {:?} is configured for `T::FeeAsset` {:?} which is not compatible with {:?} for bridge_hub_location: {:?} for bridging to {:?}/{:?}!",
T::BridgedNetworkId::get(),
T::FeeAsset::get(),
invalid_asset,
bridge_hub_location,
network,
remote_location,
);
return None
},
},
None => 0,
};

// compute fee amount. Keep in mind that this is only the bridge fee. The fee for sending
// message from this chain to child/sibling bridge hub is determined by the
// `Config::ToBridgeHubSender`
let message_size = message.encoded_size();
let message_fee = (message_size as u128).saturating_mul(T::ByteFee::get());
let fee_sum = T::BaseFee::get().saturating_add(message_fee);
let fee_sum = base_fee.saturating_add(message_fee);
let fee_factor = Self::delivery_fee_factor();
let fee = fee_factor.saturating_mul_int(fee_sum);

let fee = if fee > 0 { Some((T::FeeAsset::get(), fee).into()) } else { None };

log::info!(
target: LOG_TARGET,
"Going to send message ({} bytes) over bridge. Computed bridge fee {} using fee factor {}",
fee,
fee_factor,
"Going to send message ({} bytes) over bridge. Computed bridge fee {:?} using fee factor {}",
message_size,
fee,
fee_factor
);

Some((T::SiblingBridgeHubLocation::get(), Some((T::FeeAsset::get(), fee).into())))
Some((bridge_hub_location, fee))
}
}

Expand Down
6 changes: 4 additions & 2 deletions modules/xcm-bridge-hub-router/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use sp_runtime::{
BuildStorage,
};
use xcm::prelude::*;
use xcm_builder::NetworkExportTable;

pub type AccountId = u64;
type Block = frame_system::mocking::MockBlock<TestRuntime>;
Expand All @@ -51,6 +52,8 @@ parameter_types! {
pub UniversalLocation: InteriorMultiLocation = X2(GlobalConsensus(ThisNetworkId::get()), Parachain(1000));
pub SiblingBridgeHubLocation: MultiLocation = ParentThen(X1(Parachain(1002))).into();
pub BridgeFeeAsset: AssetId = MultiLocation::parent().into();
pub BridgeTable: Vec<(NetworkId, MultiLocation, Option<MultiAsset>)>
= vec![(BridgedNetworkId::get(), SiblingBridgeHubLocation::get(), Some((BridgeFeeAsset::get(), BASE_FEE).into()))];
}

impl frame_system::Config for TestRuntime {
Expand Down Expand Up @@ -83,13 +86,12 @@ impl pallet_xcm_bridge_hub_router::Config<()> for TestRuntime {
type WeightInfo = ();

type UniversalLocation = UniversalLocation;
type SiblingBridgeHubLocation = SiblingBridgeHubLocation;
type BridgedNetworkId = BridgedNetworkId;
type Bridges = NetworkExportTable<BridgeTable>;

type ToBridgeHubSender = TestToBridgeHubSender;
type WithBridgeHubChannel = TestWithBridgeHubChannel;

type BaseFee = ConstU128<BASE_FEE>;
type ByteFee = ConstU128<BYTE_FEE>;
type FeeAsset = BridgeFeeAsset;
}
Expand Down