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

Integrate --relay-chain-rpc-url flag #1037

Merged
merged 16 commits into from
Jul 17, 2023
Merged

Integrate --relay-chain-rpc-url flag #1037

merged 16 commits into from
Jul 17, 2023

Conversation

Chralt98
Copy link
Member

@Chralt98 Chralt98 commented Jul 6, 2023

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 our build_relay_chain_interface then.

What alternative implementations were considered?

Are there relevant PRs or issues?

moonbeam-foundation/moonbeam#1388

moonbeam-foundation/moonbeam#2042

References

@Chralt98 Chralt98 requested a review from sea212 as a code owner July 6, 2023 08:32
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2023

Codecov Report

Merging #1037 (5f6f1ad) into main (46c142b) will decrease coverage by 1.53%.
The diff coverage is n/a.

❗ 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     
Flag Coverage Δ
tests 92.93% <ø> (-1.53%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 24 files with indirect coverage changes

@Chralt98 Chralt98 self-assigned this Jul 6, 2023
@Chralt98 Chralt98 added the s:review-needed The pull request requires reviews label Jul 6, 2023
@sea212 sea212 added this to the v0.4.0 milestone Jul 7, 2023
maltekliemann
maltekliemann previously approved these changes Jul 13, 2023
node/src/cli.rs Outdated Show resolved Hide resolved
Comment on lines 239 to 242
// 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.
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@sea212 sea212 left a 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.

node/src/service/service_standalone.rs Outdated Show resolved Hide resolved
node/src/service/service_standalone.rs Outdated Show resolved Hide resolved
node/src/service/service_standalone.rs Show resolved Hide resolved
node/src/command.rs Outdated Show resolved Hide resolved
node/src/command.rs Outdated Show resolved Hide resolved
node/src/command.rs Outdated Show resolved Hide resolved
node/src/command.rs Outdated Show resolved Hide resolved
node/src/command.rs Outdated Show resolved Hide resolved
node/src/cli.rs Show resolved Hide resolved
@sea212 sea212 added s:revision-needed The pull requests must be revised and removed s:review-needed The pull request requires reviews labels Jul 13, 2023
Co-authored-by: Malte Kliemann <mail@maltekliemann.com>
Chralt98 and others added 10 commits July 14, 2023 09:01
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>
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:revision-needed The pull requests must be revised labels Jul 14, 2023
@Chralt98 Chralt98 requested a review from sea212 July 14, 2023 09:26
maltekliemann
maltekliemann previously approved these changes Jul 14, 2023
Copy link
Member

@maltekliemann maltekliemann left a 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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jul 17, 2023
@Chralt98 Chralt98 merged commit fa1b839 into main Jul 17, 2023
12 checks passed
@Chralt98 Chralt98 deleted the chralt98-remote-relay-rpc branch July 17, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add remote relay chain support via RPC
4 participants