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

pd: 📬 create tonic router in penumbra-app #3679

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

cratelyn
Copy link
Contributor

this addresses a todo comment found in the pd entrypoint. we are now using tonic 0.10 so we can have the penumbra-app crate assemble a Routes, sparing us the recitation of every service.

@cratelyn cratelyn added A-node Area: System design and implementation for node software E-day C-chore Codebase maintenance that doesn't fix bugs or add features, and isn't urgent or blocking. labels Jan 26, 2024
@cratelyn cratelyn self-assigned this Jan 26, 2024
@cratelyn cratelyn force-pushed the katie/merging-tonic-routers branch from e8ab3aa to 28c406c Compare January 26, 2024 18:47
this list has grown to include other external dependencies, not just
crates in the penumbra-zone organization. change this comment to reflect
that.
@cratelyn cratelyn force-pushed the katie/merging-tonic-routers branch from 28c406c to 495ecd2 Compare January 26, 2024 19:26
@cratelyn cratelyn force-pushed the katie/merging-tonic-routers branch from 495ecd2 to c25ff39 Compare January 26, 2024 19:27
@cratelyn cratelyn marked this pull request as draft January 26, 2024 19:43
@cratelyn cratelyn force-pushed the katie/merging-tonic-routers branch from c25ff39 to b43cf7b Compare January 26, 2024 22:24
@cratelyn
Copy link
Contributor Author

NB: this includes #3687.

@cratelyn cratelyn marked this pull request as ready for review January 26, 2024 22:48
@cratelyn
Copy link
Contributor Author

for the sake of github cross-referencing, this was a quick follow-on to #3652/#3627.

@conorsch conorsch merged commit 7b0458c into main Jan 29, 2024
7 checks passed
@conorsch conorsch deleted the katie/merging-tonic-routers branch January 29, 2024 16:23
cratelyn added a commit that referenced this pull request Mar 12, 2024
see #3913, #3973 and #3588. this is a second attempt, following up on
#3980.

#### 🔭 background

NB: the difference between this and #3679 is that the latter (_which ran
afoul of a regression_) would have `penumbra-app` create a `Routes`,
that we would
[add](https://github.com/penumbra-zone/penumbra/pull/3679/files#diff-fbc4204ceb976c8cb30ed06168e2476700bae21bfd803e26281b2d026194d430R204)
to the builder (_which stays in `pd`_). here, i'm not trying to make
that cut between `Router` and `Routes`, and am attempting to hoist the
whole thing out of `pd`, without making changes to how we interact with
`tonic`. my aim is for us to be able to move this, without running into
that bug (#3697) again.

NB: after running into problems in #3980, i found a way to easily
reproduce the issue locally. my belief was that something related to our
dependencies' cargo features was at play. rather than isolate the issue,
it was easier to rewrite this (_it's just code motion, after all_) while
running some of the network integration tests in a loop.

unlike #3980, this moves the rpc server into `penumbra-app`, per
#3980 (comment)

#### 👁️ overview

we would like to use the rust view server in mock consensus tests. in
order to run the `penumbra_view::ViewServer` however, we need to spin up
the corresponding grpc endpoint for it to connect to.

this branch performs a bit of code motion, moving the `grpc_server` out
of `pd` and into `penumbra-app`. there will likely be other functional
changes to the code in question before we can use it in those tests, but
this PR is interested in moving that code into a place where our tests
can rely upon it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-node Area: System design and implementation for node software C-chore Codebase maintenance that doesn't fix bugs or add features, and isn't urgent or blocking. E-day
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants