Skip to content
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

Removed relayer_account: &AccountId from MessageDispatch #2080

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Removed relayer_account: &AccountId from MessageDispatch #2080

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc824ad
SourceBridgeHubChain/TargetBridgeHubChain replaced with RelayerAccoun…
bkontur Apr 26, 2023
17e7152
Fix
bkontur Apr 26, 2023
cfbf7c0
Removed `relayer_account: &AccountId` from `MessageDispatch`
bkontur Apr 26, 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
1 change: 0 additions & 1 deletion bin/millau/runtime/src/rialto_messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ pub type ToRialtoMessagesDeliveryProof =
pub type FromRialtoMessageDispatch =
bridge_runtime_common::messages_xcm_extension::XcmBlobMessageDispatch<
bp_millau::Millau,
bp_rialto::Rialto,
crate::xcm_config::OnMillauBlobDispatcher,
(),
>;
Expand Down
1 change: 0 additions & 1 deletion bin/millau/runtime/src/rialto_parachain_messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ pub type FromRialtoParachainMessagePayload = messages::target::FromBridgedChainM
pub type FromRialtoParachainMessageDispatch =
bridge_runtime_common::messages_xcm_extension::XcmBlobMessageDispatch<
bp_millau::Millau,
bp_rialto::Rialto,
crate::xcm_config::OnMillauBlobDispatcher,
(),
>;
Expand Down
1 change: 0 additions & 1 deletion bin/rialto-parachain/runtime/src/millau_messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ pub type FromMillauMessagePayload = messages::target::FromBridgedChainMessagePay
pub type FromMillauMessageDispatch =
bridge_runtime_common::messages_xcm_extension::XcmBlobMessageDispatch<
bp_rialto_parachain::RialtoParachain,
bp_millau::Millau,
crate::OnRialtoParachainBlobDispatcher,
(),
>;
Expand Down
1 change: 0 additions & 1 deletion bin/rialto/runtime/src/millau_messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ pub type FromMillauMessagePayload = messages::target::FromBridgedChainMessagePay
pub type FromMillauMessageDispatch =
bridge_runtime_common::messages_xcm_extension::XcmBlobMessageDispatch<
bp_rialto::Rialto,
bp_millau::Millau,
crate::xcm_config::OnRialtoBlobDispatcher,
(),
>;
Expand Down
22 changes: 6 additions & 16 deletions bin/runtime-common/src/messages_xcm_extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,13 @@ pub enum XcmBlobMessageDispatchResult {
}

/// [`XcmBlobMessageDispatch`] is responsible for dispatching received messages
pub struct XcmBlobMessageDispatch<SourceBridgeHubChain, TargetBridgeHubChain, DispatchBlob, Weights>
{
_marker: sp_std::marker::PhantomData<(
SourceBridgeHubChain,
TargetBridgeHubChain,
DispatchBlob,
Weights,
)>,
pub struct XcmBlobMessageDispatch<RelayerAccountChain, DispatchBlob, Weights> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Relayer has accounts on both chains, so imo SourceChain or BridgedChain would be better. We also may remove this argument (_relayer_account) at all, if it isn't required for XCM dispatch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry - I mean TargetChain or ThisChain, not the SourceChain/BridgedChain :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, we dont use it here
and here is writtern:

        /// If your configuration allows paying dispatch fee at the target chain, then
	/// it must be paid inside this method to the `relayer_account`.
	fn dispatch(
		relayer_account: &AccountId,

but we call this in extrinsic, where relayer_account pays for that and after dispatch we do rewards
T::DeliveryPayments::pay_reward(

so I am not sure, can we remove relayer_account: &AccountId, from dispatch trait?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so - our rewards mechanism has changed significantly and now we're going to support XCM messaging only. So let's remove imo (as well as this obsolete comment). Feel free to merge as is - we may remove later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is target chain chain here ? is it BridgeHub who does routing (ump, hrmp)?

BridgeBlobDispatcher does not count with paying anything let _ = send_xcm::<Router>

Copy link
Contributor

Choose a reason for hiding this comment

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

what is target chain chain here ? is it BridgeHub who does routing (ump, hrmp)?

Target chain is the chain where dispatch method is called. In current architecture it is the target bridge hub (who's receiving messages).

BridgeBlobDispatcher does not count with paying anything let _ = send_xcm::

Yes, that's why I'm proposing to remove this argument and comment at all

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, it'd be better to be specific which chain is used - relayer has account on both chains (both ends of the bridge) so RelayerAccountChain is ambiguous.

_marker: sp_std::marker::PhantomData<(RelayerAccountChain, DispatchBlob, Weights)>,
}

impl<
SourceBridgeHubChain: Chain,
TargetBridgeHubChain: Chain,
BlobDispatcher: DispatchBlob,
Weights: MessagesPalletWeights,
> MessageDispatch<AccountIdOf<SourceBridgeHubChain>>
for XcmBlobMessageDispatch<SourceBridgeHubChain, TargetBridgeHubChain, BlobDispatcher, Weights>
impl<RelayerAccountChain: Chain, BlobDispatcher: DispatchBlob, Weights: MessagesPalletWeights>
MessageDispatch<AccountIdOf<RelayerAccountChain>>
for XcmBlobMessageDispatch<RelayerAccountChain, BlobDispatcher, Weights>
{
type DispatchPayload = XcmAsPlainPayload;
type DispatchLevelResult = XcmBlobMessageDispatchResult;
Expand All @@ -78,7 +68,7 @@ impl<
}

fn dispatch(
_relayer_account: &AccountIdOf<SourceBridgeHubChain>,
_relayer_account: &AccountIdOf<RelayerAccountChain>,
message: DispatchMessage<Self::DispatchPayload>,
) -> MessageDispatchResult<Self::DispatchLevelResult> {
let payload = match message.data.payload {
Expand Down
6 changes: 5 additions & 1 deletion modules/grandpa/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,7 @@ mod tests {
run_test(|| {
assert_eq!(BestFinalized::<TestRuntime>::get(), None,);
assert_eq!(Pallet::<TestRuntime>::best_finalized(), None);
assert_eq!(PalletOperatingMode::<TestRuntime>::try_get(), Err(()));

let init_data = init_with_origin(RuntimeOrigin::root()).unwrap();

Expand All @@ -843,7 +844,10 @@ mod tests {
CurrentAuthoritySet::<TestRuntime>::get().authorities,
init_data.authority_list
);
assert_eq!(PalletOperatingMode::<TestRuntime>::get(), BasicOperatingMode::Normal);
assert_eq!(
PalletOperatingMode::<TestRuntime>::try_get(),
Ok(BasicOperatingMode::Normal)
);
})
}

Expand Down