ADR 003: Handler implementation #197
Conversation
|
This PR also provides an implementation of the "Create Client" handler together with unit tests for it. |
|
|
||
| ## Decision | ||
|
|
||
| In this chapter, we provide recommendation for implementing IBC handlers within the `ibc-rs` crate. |
|
|
||
| A generic interface for events is provided below, where an event is represented | ||
| as a pair of an event type and a list of attributes. An attribute is simply a pair | ||
| of a key and a value, both represented as strings. TODO: Describe event type. |
There was a problem hiding this comment.
Instead of this TODO here, maybe we can link to here for an overview (though they are not precisely the same)?
https://github.com/informalsystems/ibc-rs/blob/master/modules/src/events.rs#L15
|
|
||
| ### Logging | ||
|
|
||
| IBC handlers must be able to log information for introspectability and ease of debugging. |
There was a problem hiding this comment.
Not sure if introspectability is right (does that mean self-examination?). Maybe we mean here to say that the handlers log information for exporting it in a simple/plain format?
| @@ -0,0 +1,552 @@ | |||
| # ADR 003: Handler implementation | |||
There was a problem hiding this comment.
I propose we use the term "protocol" to replace "handlers".
The "message processing logic" for a given codebase is effectively implementing the "protocol".
The term "protocol" is still imperfect, but easier to talk about than "message processing logic" and less contentious than "handler". A protocol consists of processing functions process_msg_x, process_msg_y etc. so that any input message is processed by such a function.
There was a problem hiding this comment.
Loved reading this ADR and code! Looks great! I haven't checked all the details of the actual message processing logic, that will be easy to fix if needed. My doubts are still around the context and keeper in general, thinking also about the context for connection, channel and packets. Left a comment against the tests for create client. Wondering if it would be easier to have separate test infrastructure to help with the setup of a mock chain/ app state.
|
|
||
| #[test] | ||
| fn test_create_client_ok() { | ||
| let mock = MockClientContext { |
There was a problem hiding this comment.
As shown below, this is missing the client_id.
What is the expectation on the caller that needs to give a context to process() in addition to the message? Specifically for clients, if the client does not exist, the context should still include a valid client_id and this test passes as long as one is given. Outside tests, is the caller expected to check if this is a MsgCreateClient create a context with None for state and type if the client doesn't exist or return existing if it does? We briefly talked about this last week and this is still an aspect we should clarify. For connections the context/ keeper passed to process() needs both client and connection context, for channels we need client, connection and channel, etc. Things will become more complex when setting up all the mocks for tests.
But the bigger concern is that the IBC handler (as per ICS) logic is split somewhat between handle() and the caller.
There was a problem hiding this comment.
The expectations are captured by the CientContext trait:
pub trait ClientContext<CD>
where
CD: ClientDef,
{
fn client_type(&self, client_id: &ClientId) -> Option<ClientType>;
fn client_state(&self, client_id: &ClientId) -> Option<CD::ClientState>;
fn consensus_state(&self, client_id: &ClientId, height: Height) -> Option<CD::ConsensusState>;
}The fact that the mock requires a client id is an implementation detail of this particular mock struct.
One could imagine implementing the ClientContext trait in a different way, but the way it's done here is imho pretty straightforward as one can exercise all code paths in the process function just by constructing different values of the mock struct.
But the bigger concern is that the IBC handler (as per ICS) logic is split somewhat between handle() and the caller.
Could you expand on that, in the context of what I wrote above, if that still makes sense?
Agreed, definitely something we need to take a closer look at, by eg. writing tests for the module-level handler/dispatcher. |
modules/src/ics02_client/msgs.rs
Outdated
| where | ||
| Self: Msg, | ||
| { | ||
| type Header: Header; | ||
| fn client_id(&self) -> &ClientId; | ||
| fn header(&self) -> &Self::Header; | ||
| /// A type of message that triggers the creation of a new on-chain (IBC) client. | ||
| #[derive(Clone, Debug)] | ||
| pub struct MsgCreateClient<CD: ClientDef> { | ||
| pub client_id: ClientId, | ||
| pub client_type: ClientType, | ||
| pub consensus_state: CD::ConsensusState, | ||
| } | ||
|
|
||
| pub trait MsgCreateClient | ||
| where | ||
| Self: Msg, | ||
| { | ||
| type ConsensusState: ConsensusState; | ||
|
|
||
| fn client_id(&self) -> &ClientId; | ||
| fn client_type(&self) -> ClientType; | ||
| fn consensus_state(&self) -> Self::ConsensusState; | ||
| /// A type of message that triggers the update of an on-chain (IBC) client with new headers. | ||
| #[derive(Clone, Debug)] | ||
| pub struct MsgUpdateClient<CD: ClientDef> { | ||
| pub client_id: ClientId, | ||
| pub header: CD::Header, | ||
| } |
There was a problem hiding this comment.
The structure of MsgCreateClient is different depending on the client type. Same for MsgUpdateClient. For example for tendermint client, the MsgCreateClient includes fields like trusting_period, unbonding_period, etc. While for loopback (not implemented) or other client types, these are not present while others might be. Not sure how we can implement the ics02 messages without traits.
There was a problem hiding this comment.
Actually, I think this will work if we create the concrete ClientState, ConsensusState, etc, from the wire/ raw message and then we call dispatch -> process. So some of the logic will be in this pre-processing step. I will give it a try.
Added client state to MsgCreateAnyClient, result, keeper, etc Test for concrete tendermint MsgCreateClient Updated TM client and consensus states, MsgCreateClient Extract client and consensus states from MsgCreateClient for TM Extract consensus state from MsgUpdateClient for TM
| as a pair of an event type and a list of attributes. An attribute is simply a pair | ||
| of a key and a value, both represented as strings. | ||
|
|
||
| Here is the [list of all IBB-related events][events], as seen by the relayer. |
There was a problem hiding this comment.
| Here is the [list of all IBB-related events][events], as seen by the relayer. | |
| Here is the [list of all IBC-related events][events], as seen by the relayer. |
| of a key and a value, both represented as strings. | ||
|
|
||
| Here is the [list of all IBB-related events][events], as seen by the relayer. | ||
| Because the structure of these events do not match the ones which are emitted by the IBC message processors, |
| impl<T> HandlerOutputBuilder<T> { | ||
| pub fn log(&mut self, log: impl Into<Log>); | ||
| pub fn emit(&mut self, event: impl Into<Event>); | ||
| pub fn with_result(self, result: T) -> HandlerOutput<T>; |
There was a problem hiding this comment.
Why not just result like the others? Otherwise it makes one wonder why it's not with_event? Just a minor nit maybe fine as is
| struct MockConnectionReader { | ||
| connection_id: ConnectionId, | ||
| connection_end: Option<ConnectionEnd>, | ||
| client_reader: MockClientReader, |
|
|
||
| #### Keeper | ||
|
|
||
| Once a message processor executes successfully, some data will typically need to be persisted in the chain state |
There was a problem hiding this comment.
Should mention the need for atomicity here
| the type of client within the message itself. | ||
|
|
||
| Because of some limitations of the Rust type system, namely the lack of proper support | ||
| for existential types, it is currently impossible to define `Reader` and `Keeper` traits |
There was a problem hiding this comment.
Can I read more about this somewhere?
* Add doc comments and error types to ICS 02 module * Rename ics02_client::client module to ics02_client::raw * Replace message traits with structs * Fix formatting error * Add implementation specific error kind * Fixup client message structs definitions * Fix clippy warnings * Add basic handler definitions * Add initial implementation of CreateClient handler * Add implementation of ClientDef for Tendermint * Re-use existing traits * Add valid test for create_client handler * Add tests for case where client already exists * WIP: Update client handler * Add initial proposal for ADR 003 * Rename file to adr-003-handler-implementation.md * Replace "handler" with "message processor" in plain text * Formatting * Fix create client handler tests * Rename module handler to dispatch * Formatting * Add sketch of update client processor * Move mocks into their own module * Remove genericity over client def in client submodule * Lift chain specific data into an enum * Update ADR to match new handling of chain-specific data * Fix the connection specifics in ADR * Added Tendermint to the new "Any*" enums Added client state to MsgCreateAnyClient, result, keeper, etc Test for concrete tendermint MsgCreateClient Updated TM client and consensus states, MsgCreateClient Extract client and consensus states from MsgCreateClient for TM Extract consensus state from MsgUpdateClient for TM Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
Closes: cosmos/ibc-rs#118
Description
This PR introduces a initial version of ADR 003 which provides recommendations for the implementation of IBC handlers.
Rendered
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer