ADR 009: ChainEndpoint/ChainHandle Consolidation

[plafer]

Description

This PR proposes ADR 8, which aims to reduce the code duplication between the ChainEndpoint and ChainHandle traits.

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests: integration (for Hermes) or unit/mock tests (for modules).
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[plafer]

Please let me know if you can think of better names for ChainBase and unroll_template.

[mzabaluev]

In my understanding, ChainEndpoint is the extension point to enable runtime backends for different chains (in the future).
ChainHandle is used to build utility on the API client side, such as mocks for testing, statistics middleware, and the like.

One potential concern with this change is that it may introduce unnecessary coupling between the client side and the runtime mechanics. For example, in a future async rewrite we might choose to convert components to async piecemeal or even leave some of them synchronous. Use of a common trait would make that process unnecessarily complicated.

That said, I'm all for reducing the surface area of the traits by deduplicating methods, using auxiliary data types, etc.

[adizere]

In my understanding, ChainEndpoint is the extension point to enable runtime backends for different chains (in the future).
ChainHandle is used to build utility on the API client side, such as mocks for testing, statistics middleware, and the like.

This is a very good point. It is highlighted also in our of older ADRs (005). The runtime backends are described here. The API client side can also be seen as a "relayer application" which uses the ChainHandle.

One advantage of this 2-sided architecture is that we can customize handlers (relayer application logic) independently of runtimes. A concrete example is in the caching layer implementation, which was added as a custom chain handle.

[adizere]

One more input we should take into account in the redesign of Runtime/Handlers is the need for supporting parallel queries in the runtime. The problem is as follows:

Whenever a relayer object (ForeignClient, Connection, RelayPath, etc) wants to assemble a message (such as a client update message), it needs to do certain queries on both the source and destination chain. Currently, each such query is serialized via the Handle -> crossbeam -> Runtime (blocking) interface. This enforces that all relayer objects must wait on each other when they want to query a certain chain, a limitation that likely affects performance.

Can we fulfil this requirement with the proposed redesign, or can we accommodate it?

[plafer]

Good point @mzabaluev. How I see it, nothing needs to change in the ADR other than having both ChainEndpoint and ChainHandle have some ChainBase as supertrait. This will facilitate further changes we expect that would have these traits' interface diverge.

@adizere IIUC, you're referring to the problem that ChainRuntime::run() takes one message at a time out of the channel, delegates the network request to the inner ChainEndpoint, and waits until it gets the response back until it starts processing the next item in the channel. That to me calls for refactoring the Runtime to use async features, and falls out of the scope of this ADR. I will say though that I don't foresee the changes I'm proposing here to make this future async refactor more difficult.

[mzabaluev]

@adizere When we get an async tendermint-rs, we could start with converting the runtime (and ChainEndpoint) to async. If we choose to not do the same for the ChainHandle APIs at that point, the common base trait would be a hindrance.

[romac]

While I agree that implementing these query_ methods is fairly repetitive and boilerplaty, I would rather not introduce a macro-based solution at this stage. Let's keep the code simple and readable and only fallback on macros if absolutely necessary.

[adizere]

Meta: removed this PR from milestone 0.14 (we're using milestones for issues, not PRs). It's OK to push this work into next release train (0.15).

[adizere]

I think most of the places where we're taking a request object is because the underlying RPC method relies on gRPC. Where we lack Request* it's often because we rely on ABCI (called rcp_client) underlying transport.

[plafer]

As discussed with @romac, this ADR would have each method have a domain Request* type, irrespective of if it ends up using RPC, gRPC, etc, effectively abstracting that away.

[adizere]

Feel free to merge Philippe. Nice consolidation!