-
Notifications
You must be signed in to change notification settings - Fork 378
Transfer asset via bridge pallet xcm with dynamic fees and back-pressure #2997
base: bko-transfer-asset-via-bridge-pallet-xcm
Are you sure you want to change the base?
Transfer asset via bridge pallet xcm with dynamic fees and back-pressure #2997
Conversation
8f86ec78b7 ".git/.scripts/commands/fmt/fmt.sh" ccf2f9483b Merge remote-tracking branch 'origin/polkadot-staging' into dynamic-fees-v1 f822ebc450 Dynamic fees v1: report congestion status to sending chain (#2318) add9fb1d53 added/fixed some docs 569a80f233 Rename LocalXcmChannel to XcmChannelStatusProvider (#2319) dc3618a4a5 Clippy e7cab6ab49 (Suggestion) Ability to externalize configuration for `ExporterFor` (#2313) c68467beff fmt 5d76f25311 use saturated_len where possible 7cc1470528 Update modules/xcm-bridge-hub-router/src/lib.rs 8d7a38a409 change log target for xcm bridge router pallet 773f93209f Revert "trigger CI" 48f1ba0323 trigger CI b26aa98d1e fixing spellcheck, clippy and rustdoc c467911a37 add new pallet to verify-pallets-build.sh ed72ebe62b get rid of redundant storage value 522bbc7ec4 benchmarks for pallet-xcm-bridge-hub-router 958243564d extension_reject_call_when_dispatcher_is_inactive 38cd8f3df3 fix other tests in the bridge-runtime-common b75e64fdf7 tests for new logic in the XcmBlobHaulerAdapter 4c741714cb tests for LocalXcmQueueMessageProcessor d99420e14c tests for LocalXcmQueueSuspender 084f551bb6 new tests for logic changes in messages pallet d9515f7317 use LocalXcmChannel in XcmBlobMessageDispatch d9a0c2e468 improvements and tests for palle-xcm-bridge-router c24301374a removed commented code eea610a875 pallet-xcm-bridge-hub-router 1fdac85a14 forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended 3c98c245ac OnMessageDelviered callback 65787da038 LocalXcmQueueManager + more adapters 74b48e2cc3 impl backpressure in the XcmBlobHaulerAdapter git-subtree-dir: bridges git-subtree-split: 8f86ec78b7747ba32807e8691f022edb4ad3040d
…sfer-asset-via-bridge-pallet-xcm-with-dynamic-fees
Adjust test for paid or unpaid xcm matcher
f7a5fbc
to
c725400
Compare
af0a0c3
to
62f2878
Compare
62f2878
to
d48d27e
Compare
pub type XcmRouter = WithUniqueTopic<( | ||
LocalXcmRouter, | ||
// Router which wraps and sends xcm to BridgeHub to be delivered to the Polkadot GlobalConsensus | ||
ToPolkadotXcmRouter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkontur Just some random thought. Can you, please, confirm that noone can use the XCM pallet send (or something like that) to send the program with ExportMessage
instruction from AH to sibling BH? Otherwise he could avoid paying the proper fee for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svyatonik do you mean something like if any other sibling parachain send xcm::Transact(pallet_xcm::send(to_bridge_hub, ExportMessage(...)))
?
all AssetHubs have disabled pallet_xcm::send
:
// We want to disallow users sending (arbitrary) XCMs from this chain.
type SendXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, ()>;
...
type XcmExecuteFilter = Nothing;
but I think I got your good point,
if above stuff will be allowed, we would charge origin by XcmpQueue
router fee and not by ToPolkadotXcmRouter
fee, if XcmpQueue fee will be enough and lower than ToPolkadotXcmRouter
, then yes we could have a problem
I think also there is a simple solution for this:
pallet_xcm::send_xcm
adds DescendOrigin
, so if we adjust xcm filter on BridgeHubs e.g. do not allow DescendOrigin
or allow only MultiLocation {parents: 1, interior: X1(Parachain(para_id))}
pattern it should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust your judgement here :) If we do not allow sending arbitrary messages to sibling BH now, then probably it is OK - just wanted to be sure. For v2 we won't need it (I assume), because we will be accepting messages from non-system parachains and (I guess) we don't want to force them to use something that is not strictly necessary (like disallowing programs with DescendOrigin
instruction). Because in any case, we'll either need to support fishermens, or impl something like https://github.com/paritytech/parity-bridges-common/issues/2324 for activating backpressure
parachains/runtimes/assets/asset-hub-polkadot/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
…dge_status)` for report congestion
bot bench cumulus-assets --subcommand=pallet --pallet=pallet_xcm_bridge_hub_router --runtime=asset-hub-kusama |
@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3400837 was started for your command Comment |
@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3400838 was started for your command Comment |
@bkontur Command |
@bkontur Command |
…llet-xcm' into bko-transfer-asset-via-bridge-pallet-xcm-with-dynamic-fees
bot rebase |
Error: Command 'Command { std: cd "/storage/repositories/cumulus" && "git" "merge" "origin/bko-transfer-asset-via-bridge-pallet-xcm" "--no-ff" "--no-edit", kill_on_drop: false }' failed with status Some(1); output: no output |
…llet-xcm' into bko-transfer-asset-via-bridge-pallet-xcm-with-dynamic-fees
…llet-xcm' into bko-transfer-asset-via-bridge-pallet-xcm-with-dynamic-fees
bot bench cumulus-assets --subcommand=pallet --pallet=pallet_xcm_bridge_hub_router --runtime=asset-hub-kusama |
@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3406271 was started for your command Comment |
@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3406272 was started for your command Comment |
…=asset-hub-kusama --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_bridge_hub_router
@bkontur Command |
@bkontur Command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good except couple of comments
@@ -769,6 +796,9 @@ construct_runtime!( | |||
Multisig: pallet_multisig::{Pallet, Call, Storage, Event<T>} = 41, | |||
Proxy: pallet_proxy::{Pallet, Call, Storage, Event<T>} = 42, | |||
|
|||
// Bridge utilities. | |||
ToPolkadotXcmRouter: pallet_xcm_bridge_hub_router::<Instance1>::{Pallet, Storage, Call} = 43, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
could leave room for a couple more generic utilities;
how many bridging pallets do we expect in AH? there will probably be some Snowfork-specific ones too?
ToPolkadotXcmRouter: pallet_xcm_bridge_hub_router::<Instance1>::{Pallet, Storage, Call} = 43, | |
ToPolkadotXcmRouter: pallet_xcm_bridge_hub_router::<Instance1>::{Pallet, Storage, Call} = 45, |
This PR extends asset transfer with
pallet_xcm
PR, adding dynamic fees and back-preassure mechanism.You can review
bridges
subdirectory either here or in its dedicated PR in parity-bridges-common where it was originally developed.TODO:
BridgeTable
(according to ExportMessage calculation from BridgeHub)XcmBridgeHubRouterByteFee
(actaully isTransactionByteFee
) or setup governance call?XcmBridgeHubRouterByteFee
via governance + testWeightInfo
report_congestion
+ testFeeManager
charged fees in test when merged Introduce XcmFeesToAccount fee manager polkadot#7005// TODO:check-parameter: change and assert in tests when (https://github.com/paritytech/polkadot/pull/7005) merged