-
Notifications
You must be signed in to change notification settings - Fork 41
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
Integrate --relay-chain-rpc-url flag #1037
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #1037 +/- ##
==========================================
- Coverage 94.46% 92.93% -1.53%
==========================================
Files 93 92 -1
Lines 21184 21589 +405
==========================================
+ Hits 20011 20064 +53
- Misses 1173 1525 +352
Flags with carried forward coverage won't be shown. Click here to find out more. |
// TODO `build_relay_chain_interface` will be included in polkadot-v0.9.36 in cumulus-client-service here: https://github.com/paritytech/cumulus/commit/babf73bbc6ade358db105580b68aacb91550faa5#diff-52df5aabcc0b97cfe43ce4b88b9dcf52de661934e481f6ae935435f4941b1639R224-R252 | ||
/// Build a relay chain interface. | ||
/// Will return a minimal relay chain node with RPC | ||
/// client or an inprocess node, based on the [`CollatorOptions`] passed in. |
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.
Do we have an issue for this?
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.
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.
Works as expected.
After the removal of the unused rpc flags in the standalone client this PR is ready to merge from my perspective.
Co-authored-by: Malte Kliemann <mail@maltekliemann.com>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
Co-authored-by: Harald Heckmann <mail@haraldheckmann.de>
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.
Seems fine to me except that the licensing question needs to be answered and formatting be fixed.
@@ -231,6 +236,32 @@ where | |||
}) | |||
} | |||
|
|||
// TODO(#1040) `build_relay_chain_interface` will be included in polkadot-v0.9.36 in cumulus-client-service here: https://github.com/paritytech/cumulus/commit/babf73bbc6ade358db105580b68aacb91550faa5#diff-52df5aabcc0b97cfe43ce4b88b9dcf52de661934e481f6ae935435f4941b1639R224-R252 |
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.
Wasn't this taken from PureStake, as well?
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.
No, I could find it in the cumulus repository at a later version.
PureStake just gave me a hint that cumulus already implemented it.
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 mean, isn't this just this: https://github.com/PureStake/moonbeam/blob/84c39610fcefbbfc57054ccbb8f5a10e9c2cb63d/node/service/src/lib.rs#L501-L528 with the let
statement replaced with a match
expression? Not sure if we need to add copyright here.
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.
Yes it is the same, but also from here.
What does it do?
Fixes #1010
Allows to specify a remote relay chain instead of the internal default relaychain.
What important points should reviewers know?
Bootnodes branch had to be used otherwise
cumulus-relay-chain-minimal-node
had the specified conflicting internal packages from paritytech instead of our fork.Is there something left for follow-up PRs?
From polkadot-v0.9.36 and on, the cumulus_client_service crate has
build_relay_chain_interface
internally. So we can remove ourbuild_relay_chain_interface
then.What alternative implementations were considered?
Are there relevant PRs or issues?
moonbeam-foundation/moonbeam#1388
moonbeam-foundation/moonbeam#2042
References