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.

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

Closed

Conversation

vgeddes
Copy link
Contributor

@vgeddes vgeddes commented Nov 1, 2023

The AggregateMessageOrigin enum now 2 levels of hierarchy, with two top-level variants, Xcm and Other, 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 aggregate origin of an inbound message.
#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
pub enum AggregateMessageOrigin {
	/// The message came from the DMP or HRMP subsystem
	Xcm(MessageOrigin),
	/// The message came from some other pallet
	Other(MessageOrigin),
}

/// The origin of an inbound message.
#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
pub enum MessageOrigin {
	/// 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),
}

The other change is to register the MessageQueue pallet last in the runtime definition for bridge-hub-rococo. This is because we need our bridge on_initialize handler to run before the MessageQueue on_initialize handler.

@paritytech-review-bot paritytech-review-bot bot requested review from a team November 1, 2023 13:49
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4165881

#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
pub enum AggregateMessageOrigin {
/// The message came from the DMP or HRMP subsystem
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Contributor Author

@vgeddes vgeddes Nov 2, 2023

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)
}

@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2023 15:25
@ggwpez ggwpez deleted the branch paritytech:oty-message-queue November 2, 2023 14:31
@ggwpez ggwpez closed this Nov 2, 2023
@acatangiu
Copy link
Contributor

@vgeddes this was automatically closed when oty-message-queue branch was deleted, to reopen rebase over master

@vgeddes
Copy link
Contributor Author

vgeddes commented Nov 3, 2023

@vgeddes this was automatically closed when oty-message-queue branch was deleted, to reopen rebase over master

Oopsie! ok, I've opened this PR: #2146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants