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

feat(server): start grpc server and register services in standalone mode #18110

Merged

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Oct 13, 2023

Description

Closes: #16277
Starts the gRPC server even when standalone mode is used.
Also registers services to expose over the gRPC server:
The CometBFT rpc endpoin t is assumed to be exposed at the rpc.laddr (which is where the cometBFT server started by the SDK would also expose the rpc server).

I tried this locally with CometMock and it fixes #16277: I tried grpcurl to call endpoints from cosmos.tx.v1beta1.Service, cosmos.base.node.v1beta1.Service/Status and cosmos.base.tendermint.v1beta1.Service

Is there any testing infrastructure set up to test out-of-process comet?
Then tests for this could hook in there.
If there are no tests for that anyways, I think this change isn't necessarily worth adding them for,
since it's relatively minimal.

This should be backported to v0.47 and v0.50. There is already a PR that aims to backport to v0.47 here:
#18109


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules N/A
  • included the necessary unit and integration tests N/A I think neither the standalone nor the in-process versions of start are tested in those tests. Please correct me if that's wrong
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed: I think all the failures are things that don't have anything to do with the PR, but I might be mistaken

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt julienrbrt changed the title Start grpc server and register services in standalone mode feat(server): start grpc server and register services in standalone mode Oct 13, 2023
@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Oct 13, 2023
@p-offtermatt p-offtermatt marked this pull request as ready for review October 13, 2023 12:38
@p-offtermatt p-offtermatt requested a review from a team as a code owner October 13, 2023 12:38
@github-prbot github-prbot requested review from a team, alexanderbez and samricotta and removed request for a team October 13, 2023 12:39
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

utACK!

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

do you need the api server here as well?

@p-offtermatt
Copy link
Contributor Author

Currently I don't. I'll let you decide whether you think this should still be started - I think the alternative is to have an issue for it and pick it back up if anyone asks for it at some point

@tac0turtle
Copy link
Member

can we add a startAPI call, everything seems registered, for consistency and to avoid have another issue sitting for a while

@p-offtermatt
Copy link
Contributor Author

Done, it's also starting the api server now

CHANGELOG.md Outdated Show resolved Hide resolved
@tac0turtle tac0turtle enabled auto-merge October 18, 2023 10:21
@tac0turtle tac0turtle added this pull request to the merge queue Oct 18, 2023
Merged via the queue into cosmos:main with commit 31f5bc0 Oct 18, 2023
46 of 48 checks passed
mergify bot pushed a commit that referenced this pull request Oct 18, 2023
…ode (#18110)

Co-authored-by: Marko <[email protected]>
(cherry picked from commit 31f5bc0)

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standalone mode --with-tendermint false does not work with GRPC and transaction simulation
3 participants