-
Notifications
You must be signed in to change notification settings - Fork 823
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.
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
RFC: Reorganise AggregateMessageOrigin to support Snowbridge #2117
RFC: Reorganise AggregateMessageOrigin to support Snowbridge #2117
Conversation
The CI pipeline was cancelled due to failure one of the required jobs. |
#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)] | ||
pub enum AggregateMessageOrigin { | ||
/// The message came from the DMP or HRMP subsystem |
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 dont think we can just change this enum, since its use by the MQ pallet as queue ID.
That would mean that we need a lazy migration across all parachain which is a huge pain.
It would be better to just add a variant to it like Abridged
or Other
and keeping the others compatible.
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.
Unless this change goes in directly after #1246 in the same release, but its difficult.
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.
Also this doesn't make sense, XCM
is not an origin or transport protocol. I can't tell if you are indicating the format of the message or its origin with this type? If the latter, why not just add a new variant to the AggregateMessageOrigin
enum, like ExternalConsensus(Junction)
, where the Junction
is expected to be of the variant GlobalConsensus(NetworkId)
?
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 dont think we can just change this enum, since its use by the MQ pallet as queue ID.
Good point, didn't think of that. I'll will just add another variant to the enum then.
Here's a new design, inline with the above suggested changes.
Since the NetworkId
and Junction
types are not stable across XCM versions (and because they are only meant to be used at higher protocol layers), I have not used them here.
/// The origin of an inbound message.
#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
pub enum AggregateMessageOrigin {
/// The message came from the para-chain itself.
Here,
/// The message came from the relay-chain.
///
/// This is used by the DMP queue.
Parent,
/// The message came from a sibling para-chain.
///
/// This is used by the HRMP queue.
Sibling(ParaId),
/// External consensus
ExternalConsensus {
origin: ExternalConsensusOrigin,
dest: ExternalConsensusDestination
}
}
/// External consensus destinations
#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
pub enum ExternalConsensusDestination {
// This network is reachable via Snowbridge on BridgeHub
Ethereum { chain_id: u64 }
}
/// The origin of a message destined for an external consensus system
#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
pub enum ExternalConsensusOrigin {
Here,
Sibling(ParaId)
}
@vgeddes this was automatically closed when |
The
AggregateMessageOrigin
enum now 2 levels of hierarchy, with two top-level variants,Xcm
andOther
, to be used by (dmp-queue, hrmp-queue) and snowbridge respectively.This PR only changes the
bridge-hub-rococo
runtime as a demonstration of the refactor. It also demonstrates how we intend to modify BridgeHub so that our bridge can also use MessageQueue.The other change is to register the
MessageQueue
pallet last in the runtime definition forbridge-hub-rococo
. This is because we need our bridgeon_initialize
handler to run before the MessageQueueon_initialize
handler.