(Suggestion) Ability to externalize configuration for `ExporterFor` #2313

bkontur · 2023-08-01T15:01:52Z

(Replaced BridgedNetworkId/SiblingBridgeHubLocation with Bridges: ExporterFor)

actual ExportFor impl for Pallet matches only bridged newtork, which is not enough,
e.g. if we use pallet_xcm::reserve_transfer_assets on AssetHubKusama and we set destination as polkadot-collectives MultiLocation { parents: 2, X2(GlobalConsensus(Polkadot), Parachain(1001))},
nothing stops to send this message over bridge to polkadot-collectives,
so for AssetHub we need to control exactly where we allow to send assets,

so for AssetHubs few days ago I added implementation for ExportFor which matches also remote destination:
paritytech/cumulus@5103de8

TODO:

how to handle maybe_payment? subsume with (T::FeeAsset::get(), fee)?
fix compiling other runtimes (millau, ...) :)

(Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`)

bkontur · 2023-08-01T15:32:31Z

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

svyatonik

LGTM, thanks for spotting that! But please check out comments first. My suggestions:

to change how we handle fee, returned by ExporterFor;
and keep theBridgedNetwotkId check to avoid using the same pallet instance for different consensus systems.

svyatonik · 2023-08-02T05:39:53Z

modules/xcm-bridge-hub-router/src/lib.rs

@@ -194,6 +194,8 @@ impl<T: Config<I>, I: 'static> ExporterFor for Pallet<T, I> {
 		let fee_factor = Self::delivery_fee_factor();
 		let fee = fee_factor.saturating_mul_int(fee_sum);

+		// TODO: how to handle `maybe_payment`? subsume with `(T::FeeAsset::get(), fee)`?


I think we can subsume and use it instead of Config;:BaseFee.

@svyatonik
I tried to fix maybe_payment, please, check 623c9c0

svyatonik · 2023-08-02T05:48:04Z

modules/xcm-bridge-hub-router/src/lib.rs

-		// ensure that the message is sent to the expected bridged network
-		if *network != T::BridgedNetworkId::get() {
+		// ensure that the message is sent to the expected bridged network/location.
+		let Some((bridge_hub_location, maybe_payment)) = T::Bridges::exporter_for(network, remote_location, message) else {


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.

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

I've filed #2315 to track that

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>;?

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

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

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)

bkontur · 2023-08-02T10:27:56Z

bot rebase

…c-fees-v1

svyatonik

Looks good,m thank you for the help

* impl backpressure in the XcmBlobHaulerAdapter * LocalXcmQueueManager + more adapters * OnMessageDelviered callback * forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended * pallet-xcm-bridge-hub-router * removed commented code * improvements and tests for palle-xcm-bridge-router * use LocalXcmChannel in XcmBlobMessageDispatch * new tests for logic changes in messages pallet * tests for LocalXcmQueueSuspender * tests for LocalXcmQueueMessageProcessor * tests for new logic in the XcmBlobHaulerAdapter * fix other tests in the bridge-runtime-common * extension_reject_call_when_dispatcher_is_inactive * benchmarks for pallet-xcm-bridge-hub-router * get rid of redundant storage value * add new pallet to verify-pallets-build.sh * fixing spellcheck, clippy and rustdoc * trigger CI * Revert "trigger CI" This reverts commit 48f1ba0. * change log target for xcm bridge router pallet * Update modules/xcm-bridge-hub-router/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * use saturated_len where possible * fmt * (Suggestion) Ability to externalize configuration for `ExporterFor` (#2313) * Ability to externalize configuration for `ExporterFor` (Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`) * Fix millau * Compile fix * Return back `BridgedNetworkId` but as optional filter * Replaced `BaseFee` with fees from inner `Bridges: ExporterFor` * typo * Clippy * Rename LocalXcmChannel to XcmChannelStatusProvider (#2319) * Rename LocalXcmChannel to XcmChannelStatusProvider * fmt * added/fixed some docs * Dynamic fees v1: report congestion status to sending chain (#2318) * report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions * ".git/.scripts/commands/fmt/fmt.sh" * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P (#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks --------- Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: command-bot <>

* impl backpressure in the XcmBlobHaulerAdapter * LocalXcmQueueManager + more adapters * OnMessageDelviered callback * forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended * pallet-xcm-bridge-hub-router * removed commented code * improvements and tests for palle-xcm-bridge-router * use LocalXcmChannel in XcmBlobMessageDispatch * new tests for logic changes in messages pallet * tests for LocalXcmQueueSuspender * tests for LocalXcmQueueMessageProcessor * tests for new logic in the XcmBlobHaulerAdapter * fix other tests in the bridge-runtime-common * extension_reject_call_when_dispatcher_is_inactive * benchmarks for pallet-xcm-bridge-hub-router * get rid of redundant storage value * add new pallet to verify-pallets-build.sh * fixing spellcheck, clippy and rustdoc * trigger CI * Revert "trigger CI" This reverts commit 48f1ba0. * change log target for xcm bridge router pallet * Update modules/xcm-bridge-hub-router/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * use saturated_len where possible * fmt * (Suggestion) Ability to externalize configuration for `ExporterFor` (paritytech#2313) * Ability to externalize configuration for `ExporterFor` (Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`) * Fix millau * Compile fix * Return back `BridgedNetworkId` but as optional filter * Replaced `BaseFee` with fees from inner `Bridges: ExporterFor` * typo * Clippy * Rename LocalXcmChannel to XcmChannelStatusProvider (paritytech#2319) * Rename LocalXcmChannel to XcmChannelStatusProvider * fmt * added/fixed some docs * Dynamic fees v1: report congestion status to sending chain (paritytech#2318) * report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions * ".git/.scripts/commands/fmt/fmt.sh" * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P (paritytech#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks --------- Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: command-bot <>

Ability to externalize configuration for ExporterFor

f8db47c

(Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`)

bkontur requested a review from svyatonik August 1, 2023 15:01

Fix millau

6876de7

Compile fix

6033f69

svyatonik approved these changes Aug 2, 2023

View reviewed changes

This was referenced Aug 2, 2023

Dynamic fees for bridges-v1 #2294

Merged

Think of more sophisticated algorithm than blocking XCM channel between sending chain and bridge hub #2315

Closed

bkontur added 4 commits August 2, 2023 12:32

Merge remote-tracking branch 'origin/dynamic-fees-v1' into bko-dynami…

b3ee5dc

…c-fees-v1

Return back BridgedNetworkId but as optional filter

a3fa48f

Replaced BaseFee with fees from inner Bridges: ExporterFor

623c9c0

typo

0119a43

svyatonik approved these changes Aug 2, 2023

View reviewed changes

svyatonik merged commit e7cab6a into dynamic-fees-v1 Aug 3, 2023

svyatonik deleted the bko-dynamic-fees-v1 branch August 3, 2023 04:45

svyatonik mentioned this pull request Aug 21, 2023

Transfer asset via bridge pallet xcm with dynamic fees and back-pressure paritytech/cumulus#2997

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Suggestion) Ability to externalize configuration for `ExporterFor` #2313

(Suggestion) Ability to externalize configuration for `ExporterFor` #2313

bkontur commented Aug 1, 2023 •

edited

Loading

bkontur commented Aug 1, 2023

svyatonik left a comment

svyatonik Aug 2, 2023

bkontur Aug 2, 2023

svyatonik Aug 2, 2023

svyatonik Aug 2, 2023

svyatonik Aug 2, 2023

bkontur Aug 2, 2023

svyatonik Aug 2, 2023

bkontur Aug 2, 2023

bkontur Aug 2, 2023

bkontur commented Aug 2, 2023

svyatonik left a comment

(Suggestion) Ability to externalize configuration for ExporterFor #2313

(Suggestion) Ability to externalize configuration for ExporterFor #2313

Conversation

bkontur commented Aug 1, 2023 • edited Loading

bkontur commented Aug 1, 2023

svyatonik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkontur commented Aug 2, 2023

svyatonik left a comment

Choose a reason for hiding this comment

(Suggestion) Ability to externalize configuration for `ExporterFor` #2313

(Suggestion) Ability to externalize configuration for `ExporterFor` #2313

bkontur commented Aug 1, 2023 •

edited

Loading