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

Extend the Chain trait to enable listening for IBC events from non-Cosmos SDK chains, eg. Substrate #1456

Closed
5 tasks done
DaviRain-Su opened this issue Oct 13, 2021 · 6 comments
Assignees
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews E: non-cosmos External: related to non-Cosmos chains E: substrate External: substrate integration, requirements & problems
Milestone

Comments

@DaviRain-Su
Copy link
Contributor

DaviRain-Su commented Oct 13, 2021

Crate

None

Summary

init_event_monitor Support multiply chain to event mointor

https://github.com/informalsystems/ibc-rs/blob/8944f94815c35b2d3297971aa437900d259dc75a/relayer/src/chain.rs#L99

/// Initializes and returns the event monitor (if any) associated with this chain.
    fn init_event_monitor(
        &self,
        rt: Arc<TokioRuntime>,
    ) -> Result<(EventReceiver, TxMonitorCmd), Error>;

https://github.com/informalsystems/ibc-rs/blob/8944f94815c35b2d3297971aa437900d259dc75a/relayer/src/event/monitor.rs#L132

pub struct EventMonitor {
    chain_id: ChainId,
    /// WebSocket to collect events from
    client: WebSocketClient,
    /// Async task handle for the WebSocket client's driver
    driver_handle: JoinHandle<()>,
    /// Channel to handler where the monitor for this chain sends the events
    tx_batch: channel::Sender<Result<EventBatch>>,
    /// Channel where to receive client driver errors
    rx_err: mpsc::UnboundedReceiver<tendermint_rpc::Error>,
    /// Channel where to send client driver errors
    tx_err: mpsc::UnboundedSender<tendermint_rpc::Error>,
    /// Channel where to receive commands
    rx_cmd: channel::Receiver<MonitorCmd>,
    /// Node Address
    node_addr: Url,
    /// Queries
    event_queries: Vec<Query>,
    /// All subscriptions combined in a single stream
    subscriptions: Box<SubscriptionStream>,
    /// Tokio runtime
    rt: Arc<TokioRuntime>,
}

Problem Definition

Recently I am doing support for substrate relayer support, for event listening here, it seems that there is no support for multiple chains like ChainEndpoint defines an interface, here for EventMonitor to listen to events suitable for tendermint-rpc strong binding, no implementation and different chains between the loose coupling. Is there a good implementation to achieve this part of the function, because I found that this EventMointor in the implementation of start and listen command is related.

Proposal

None

Acceptance Criteria

None


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@romac romac changed the title About init_event_monitor and EventMointor Support multiply chain to event mointor Extend the Chain abstraction to allow for non-Cosmos EventMonitor Oct 13, 2021
@romac
Copy link
Member

romac commented Oct 13, 2021

Hi @DaviRain-Su, thanks for opening this issue! If I understand correctly, you are rightfully pointing out that the EventMonitor (which Hermes uses to listen for events from a chain) only works for Cosmos SDK based chains and that we should extend the Chain trait to allow non-Cosmos chains to implement their own version of the EventMonitor.

What do you suggest to fix this?

Would extending the Chain trait with a new method

    fn init_event_monitor(
        &self,
        rt: Arc<TokioRuntime>,
    ) -> Result<(EventReceiver, TxMonitorCmd), Error> {

work for you?

At the moment, the EventReceiver type is defined as follows:

pub type EventReceiver = channel::Receiver<Result<EventBatch>>;

where EventBatch is defined as

pub struct EventBatch {
    pub chain_id: ChainId,
    pub height: Height,
    pub events: Vec<IbcEvent>,
}

Do you think this would work? To be more clear, do you think it would be possible for Substrate to emit events such that we can feed them into a channel that emits EventBatches?

If you had some example code from your implementation, for example your implementation of the Chain trait for Substrate, it would be very useful to us to get a better understanding of what the requirements are.

@romac romac added the E: non-cosmos External: related to non-Cosmos chains label Oct 13, 2021
@romac romac added this to the 12.2021 milestone Oct 13, 2021
@romac romac changed the title Extend the Chain abstraction to allow for non-Cosmos EventMonitor Extend the Chain trait to enable listening for IBC events from non-Cosmos SDK chains, eg. Substrate Oct 13, 2021
@faddat
Copy link
Contributor

faddat commented Oct 14, 2021

this is super exciting.

<3

@DaviRain-Su
Copy link
Contributor Author

@romac Thank you for your reply, do you see the logic of my modification here, my understanding for Newblock this event is the event of generating a new block on the node. For substrate is to generate SystemEventSuccess event. There may be some problems here. And for the chain request to substrate rpc request is used substrate-subxt. The last question is, for start this command also seems to be related to Eventmointor, I would like to know where are some difficulties in adapting this command here.

This is my code: https://github.com/octopus-network/ibc-rs/blob/b7d1556aae68262504115115330571795b5ec038/relayer/src/event/substrate_mointor.rs#L347

@romac romac added the A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews label Nov 17, 2021
@romac romac added the E: substrate External: substrate integration, requirements & problems label Feb 9, 2022
@adizere adizere modified the milestones: v1.0.0, v1.1 May 27, 2022
@romac romac modified the milestones: Refactor sink, v1.2 Nov 22, 2022
@romac
Copy link
Member

romac commented Nov 22, 2022

@DaviRain-Su What do you think of the refactoring in #2865? Does this help with your use case?

@DaviRain-Su
Copy link
Contributor Author

It looks good, but there are still some components that are strongly related to tendermint that I wish could be abstracted

@romac
Copy link
Member

romac commented Nov 30, 2022

Closed by #2865: #2865 (comment)

@romac romac closed this as completed Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews E: non-cosmos External: related to non-Cosmos chains E: substrate External: substrate integration, requirements & problems
Projects
None yet
Development

No branches or pull requests

4 participants