Remove ChainHandle dependency for *Querier
#367
Conversation
thaodt
force-pushed
the
queriers
branch
2 times, most recently
from
2d8713e
to
a25a88a
Compare
|
hi @soareschen can you please help me do a quick review on |
There was a problem hiding this comment.
@thaodt thanks for your contribution! Looks pretty good. I have added some suggestions on how to improve the code. Your branch also needs to be updated before we can merge.
| + CanRaiseError<Ics04Error> | ||
| + CanRaiseError<Ics02Error> | ||
| + CanRaiseError<tonic::transport::Error> | ||
| + CanRaiseError<ibc_relayer::error::Error>, |
There was a problem hiding this comment.
The following constraints can be safely removed without compilation error:
HasErrorType(because we haveCanRaiseErrorwhich includes it as a supertrait)HasGrpcAddressHasComponentsCanRaiseError<Ics04Error>CanRaiseError<tonic::transport::Error>
In general, a constraint can be removed if it does not introduce any compile error.
|
|
||
| pub struct QueryReceivedPacketWithChainHandle; | ||
|
|
||
| impl<Chain, Counterparty> ReceivedPacketQuerier<Chain, Counterparty> | ||
| for QueryReceivedPacketWithChainHandle | ||
| where | ||
| Chain: HasIbcChainTypes<Counterparty, ChannelId = ChannelId, PortId = PortId> | ||
| + HasBlockingChainHandle, | ||
| + HasGrpcAddress | ||
| + CanRaiseError<eyre::Report>, |
There was a problem hiding this comment.
We can check what error is returned from calls like ChannelQueryClient::connect, and add the CanRaiseError constraint for that specific error here.
There was a problem hiding this comment.
it actually returns a tonic::transport::Error, so I guess should CanRaiseError<eyre::Report> be enough?
There was a problem hiding this comment.
In principle, yes. However, eyre::Report is too general and obscures details about what kind of error it is. If we add the constraint CanRaiseError<tonic::transport::Error>, the chain context can easily choose to handle it in different way, e.g. marking the error as retryable.
In addition, the use of CGP and the CanRaiseError trait makes it easy to raise any custom error to the chain context. Hence, we prefer to raise errors that are as specific as possible.
| use ibc_relayer::chain::requests::QueryUnreceivedPacketsRequest; | ||
| use ibc_relayer_types::core::ics04_channel::packet::Sequence; | ||
| use ibc_relayer_types::core::ics24_host::identifier::{ChannelId, PortId}; | ||
|
|
||
| use crate::traits::chain_handle::HasBlockingChainHandle; | ||
| use crate::traits::grpc_address::HasGrpcAddress; | ||
|
|
||
| pub struct QueryReceivedPacketWithChainHandle; |
There was a problem hiding this comment.
This component can be renamed too.
| source_channel_id: packet.destination_channel.clone(), | ||
| source_port_id: packet.destination_port.clone(), | ||
| destination_channel_id: packet.source_channel.clone(), | ||
| destination_port_id: packet.source_port.clone(), |
There was a problem hiding this comment.
i'm still not sure which one is the source and which one is the dest data.
Hmm you are right. It looks like the source should match the packet source, and vice versa for destination.
Actually looking at the code, it looks like this component is not used anywhere in the relayer code, other than the mock relayer. So I think we can remove this component (QueryWriteAckEventFromAbci) altogether, if it does not introduce any compile error.
|
I've pushed the fix from your previous review @soareschen. And I have some questions while doing the last one - which later called to this fn I tried extracting the code from there and follow the abci queries's methods to get necessary data, but very soon after that I discovered that i didn't have the The expected output of this function is the chain ID of the counterparty chain and requires that the client connection channel be open so that the chain id can be obtained from the client state of that connection. |
|
If you trace the original call, you can find that it is querying from the latest height. So you can add the constraint For now, you can check whether the channel state condition is open, and if not, we can raise error strings for now. To do that, simply add |
Apart of #214.
Description
Following the guidance in the issue and refactoring items below:
CounterpartyChainIdQuerierReceivedPacketQuerierWriteAckQuerierPR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.