chore(sequencer-relayer): add timeout to gRPCs to Celestia app#1191
chore(sequencer-relayer): add timeout to gRPCs to Celestia app#1191
Conversation
github-actions
bot
added
the
sequencer-relayer
There was a problem hiding this comment.
I agree with the patch, but I am not a fan of the test (described in a comment on the code).
What would make for a useful test is externally observable behavior, such as a request timing out and a second request being issued. This would test relayer's expected behavior in light of failure.
I don't see a reason to block this PR on these grounds (because adding this blackbox test requires more work on the mock grpc server), but I would be fine if the unit test was removed.
| use super::*; | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn get_tx_should_timeout() { |
There was a problem hiding this comment.
At its core, this tests the timeout inside Client::get_tx and thus if the underlying timeout logic of tonic's channel works. This limits the usefulness of this test (because we should always be able to rely on the correct behavior of our building blocks).
There was a problem hiding this comment.
As discussed, I agree on removing this test, with a view to add black box tests in the near future for sad path cases like gRPC errors, including timeouts.
* main: chore(bridge-withdrawer): add missing errors and clean up names (#1178) feat(sequencer): add ttl and invalid cache to app mempool (#1138) chore(astria-merkle): add benchmarks (#1179) chore(sequencer-relayer): add timeout to gRPCs to Celestia app (#1191) refactor(core): parse ics20 denoms as ibc or trace prefixed variants (#1181) Mycodecrafting/sequencer seed node (#1188) chore: register all metrics during startup (#1144) feat(charts): option to purge geth mempool (#1182)
2 participants
Summary
Added a five second timeout to all gRPCs targeting the Celestia app.
Background
Currently if the Celestia app doesn't respond to any gRPC in a timely manner, the relayer's submission flow is blocked by this. We want to avoid this by not waiting indefinitely for any gRPC.
Changes
Testing
time::advanceto avoid the test running for the actual duration of the timeout, and we have a better chance to inspect the actual error returned by the RPC to ensure it's a timeout variant.Related Issues
Closes #1187.