Abstract components to query connection end #167
Conversation
| async fn query_connection_end( | ||
| chain: &Chain, | ||
| connection_id: &Chain::ConnectionId, | ||
| height: Option<&Chain::Height>, |
There was a problem hiding this comment.
Let's always require a height, and have the caller retrieve the latest height via CanQueryChainHeight if necessary.
IMO making the height optional complicates the implementation without much benefit.
There was a problem hiding this comment.
Isn't this the simplest way to get the connection end at the latest height without incurring another redundant query at the call site when the user is only interested in the latest height?
I would vote for either keeping the optional height parameter or define our own enum QueryHeight<Height> { Latest, Specific(Height) } type and take a QueryHeight<Chain::Height> as an argument.
There was a problem hiding this comment.
The chain height can easily be cached every few hundred milliseconds, so that the query do not become costly.
If we really want to make it optional, perhaps we can add an abstract HeightParam as follows:
pub trait HasHeightParamType: HasHeightType {
type HeightParam: Async + From<Self::Height>;
}We would also add WithLatestHeight variants of the trait, and allow implementations to choose different strategies to handle the latest height parameter. For example:
impl<Chain> ClientStateWithLatestHeightQuerier<Chain>
for QueryLatestClientStateWithDefaultHeightParam
where
Chain::HeightParam: Default,
and
impl<Chain> ClientStateWithLatestHeightQuerier<Chain>
for QueryLatestHeightAndClientState
where
Chain: CanQueryChainHeight,
Anyway, it is not too important for the current case of querying connection end, as it is only used once in the CLI. Let's keep this for now, and continue our discussion somewhere else.
| let query_height = if let Some(height) = height { | ||
| let specified_height = Height::new(chain_handle.id().version(), height) | ||
| .map_err(|e| RelayerError::generic(eyre!("Failed to create Height with revision number `{}` and revision height `{height}`. Error: {e}", chain_handle.id().version())))?; | ||
| let height = self.height.map(|h| Height::new(chain.chain_id().version(), h).map_err(|e| RelayerError::generic(eyre!("Failed to create Height with revision number `{}` and revision height `{h}`. Error: {e}", chain.chain_id().version()))).unwrap()); |
There was a problem hiding this comment.
There's something weird going on with the formatting here
There was a problem hiding this comment.
Yes I've had odd behaviour with cargo fmt and the CI doesn't seem to complain either. I don't know what's going on
There was a problem hiding this comment.
As last resort you can mark the block with #[rustfmt::skip] and format it manually.
There was a problem hiding this comment.
There is a .map_err() followed by an .unwrap(), which doesn't really make sense. And if you want to do map_err(), perhaps it is better to do it right after Height::new().
There was a problem hiding this comment.
As last resort you can mark the block with #[rustfmt::skip] and format it manually.
The lines inside the .map don't seem to be impacted by cargo fmt, I can format it manually without disabling rustfmt
There is a .map_err() followed by an .unwrap(), which doesn't really make sense. And if you want to do map_err(), perhaps it is better to do it right after Height::new().
Oh good catch. I will update this using transpose() and ? instead of the unwrap, see a1878ee
Closes: #153
Description
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.