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

🔗 integration test coverage for grpc-web compatibility #3704

Open
cratelyn opened this issue Jan 30, 2024 · 4 comments
Open

🔗 integration test coverage for grpc-web compatibility #3704

cratelyn opened this issue Jan 30, 2024 · 4 comments
Labels
A-CI/CD Relates to continuous integration & deployment of Penumbra A-node Area: System design and implementation for node software _P-low Priority: low

Comments

@cratelyn
Copy link
Contributor

it is an important requirement of pd that we have maintain compatibility with gRPC-web clients. the https://github.com/penumbra-zone/web project depends upon connect-es to communicate with nodes via the gRPC-web protocol.

to address #3627, we tested that #3652 maintained compatibility by spinning up a live validator and connecting to it via the web wallet. this is a time-consuming process that we can't perform on every PR, so we nonetheless observed a regression after landing other seemingly innocuous changes to pd's router. (see #3697, #3679)

we should introduce some integration test coverage that demonstrates that a gRPC-web client can properly communicate with a fullnode.

i would prefer if the solution to this does not entail depending on the web repository as a submodule. perhaps we could spin up a test client that also uses the same connect-es library, to show that transport works properly.

@cratelyn cratelyn added A-node Area: System design and implementation for node software A-CI/CD Relates to continuous integration & deployment of Penumbra labels Jan 30, 2024
@hdevalence
Copy link
Member

Should this be considered a dup of penumbra-zone/web#415 ?

@cratelyn
Copy link
Contributor Author

Should this be considered a dup of penumbra-zone/web#415 ?

i don't believe so. that is discussing the web project's CI; i'd like if we had something in the penumbra monorepo to exercise this.

i'm not privy to how they are planning on wiring up playwright though, presumably it would be running against the preview network? in that case, we would still be in a position where our test suite would not catch regressions that could break the web extension until after changes have been merged.

i'd like something that, while perhaps not exhaustive, gives us some confidence that grpc-web clients can talk to pd. does that sound fair to you, @hdevalence?

@hdevalence
Copy link
Member

Yes, the issue is just that we aren't putting any web tooling in this repo, to insulate the core protocol development from the churn of web tooling. So unless we have a Rust grpc-web client handy we'll need to get delayed notifications via a CI run triggered in another repo. I think that's fine, since those breaks should be rare — this is the first one in more than a year.

@cratelyn cratelyn added the _P-low Priority: low label Jan 30, 2024
@conorsch
Copy link
Contributor

conorsch commented May 8, 2024

Not having this coverage in place was painful: #4351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI/CD Relates to continuous integration & deployment of Penumbra A-node Area: System design and implementation for node software _P-low Priority: low
Projects
Status: Backlog
Status: No status
Development

No branches or pull requests

3 participants