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

[Cumulus] Add GeneralKey variant to AggregateMessageOrigin #2338

Closed

Conversation

vgeddes
Copy link
Contributor

@vgeddes vgeddes commented Nov 15, 2023

Adds GeneralKey variant to AggregateMessageOrigin.

This change is needed for the system BridgeHub runtimes, so that Snowbridge can also make use of the MessageQueue pallet. See previous discussion at #2146 (comment).

We have reworked Snowbridge to utilize the more generic approach introduced by this PR.
This avoids the need for BridgeHub runtimes to define their own AggregateMessageOrigin and associated helper types.

Other parachain teams may also benefit from this.

@ggwpez
Copy link
Member

ggwpez commented Nov 15, 2023

This avoids the need for BridgeHub runtimes to define their own AggregateMessageOrigin and associated helper types.

But that is cleaner? If we only ever need those variants in the bridgehub runtimes, then only the bridge hub runtimes should expose them.
Otherwise all other parachain like asset-hub will now have the case in their code that a message from GeneralKey comes in, although they never use it, which means that we have an unreachable that could easily be prevented by the type system.

You even wrote in that comment:

That makes very good sense, narrowing the change down to the BridgeHub level. Let me try that.

Other parachain teams may also benefit from this.

Any downstream team is strongly encouraged to create their own origin types. AFAIK no downstream team needs to use this type at all. We cannot add some variants here, since we then force all system chains to also have them. It is much better to wrap the type when possible.

Another issue with this is that Explorers and UIs loose all introspection for which queue is currently doing something. When they are listed in an enum, then PolkadotJS and wallets can correctly display this.
It should be possible to do something like this:

enum BridgeOrigin {
    Core(AggregateMessageOrigin),
    Abridged(...),
}

Without any need to update the AggregateMessageOrigin itself.

@vgeddes
Copy link
Contributor Author

vgeddes commented Nov 16, 2023

Those are good points - I thought the generalized approach would have been the lightest touch approach. Though as you say, its not very type safe or introspectable.

I appreciate the time you've spent reviewing this and previous drafts. Thanks Oliver.

Closing this off. Instead, will submit a PR providing the specialized AggregateMessageOrigin together with all other Snowbridge stuff we intend to put into the BridgeHub runtimes.

@vgeddes vgeddes closed this Nov 16, 2023
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
…aritytech#2341)

* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Move finality Engine to finality_base folder

* Define SubstrateFinalityPipeline

Extract basic parts of SubstrateFinalitySyncPipeline into
SubstrateFinalityPipeline

* Add equivocation detection pipeline

* Fix comment
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.

2 participants