-
Notifications
You must be signed in to change notification settings - Fork 321
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
ibc: add grpc queries for connections, channels, and clients #2954
Conversation
All of the previously implemented ibc queries are implemented now, this just needs to be hooked up to the grpc api |
Hooked up the grpc api |
Looks like this is just blocked on fixing build flags for wasm? |
34ffb5c
to
bf8343b
Compare
Discussed at sprint planning today: we need to land these necessary-but-not-sufficient queries to unblock a Hermes relaying spike. However, further queries are required. For Testnet 60 #2984, we won't have full support merged, but we'll press on with support. |
&self, | ||
_request: tonic::Request<QueryClientConnectionsRequest>, | ||
) -> std::result::Result<tonic::Response<QueryClientConnectionsResponse>, tonic::Status> { | ||
todo!() |
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.
This will make the node panic if we get one of these requests, can we search/replace these todo!()
impls with a snippet that returns an Ok
with Status::unimplemented
?
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.
Let's avoid the todo!()
s but otherwise this LGTM!
.add_service(tonic_web::enable(ClientQueryServer::new(ibc.clone()))) | ||
.add_service(tonic_web::enable(ChannelQueryServer::new(ibc.clone()))) | ||
.add_service(tonic_web::enable(ConnectionQueryServer::new(ibc.clone()))) |
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.
Non-blocking: going forward, we want to split up the RPCs and move them into the component crates. When we do that, we'll have LOTS of different services to stitch together, and we should figure out how to cleanly add them all. Maybe tower::Layer
s would be helpful there.
We want this in for 60 (#2984). @avahowell is aiming for merge by EOD. |
44fbf3a
to
1a84797
Compare
Matches changes in penumbra-zone/penumbra#2954 Closes #64
Matches changes in penumbra-zone/penumbra#2954 Closes #64
No description provided.