From 9b319d982d54d8a27b6800ffa1185fb02c24b32a Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 19 Aug 2020 20:12:03 +0200 Subject: [PATCH] ADR 003: Handler implementation (#197) * 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 Co-authored-by: Anca Zamfir --- .../adr-003-handler-implementation.md | 679 ++++++++++++++++++ relayer-cli/src/commands/query/channel.rs | 2 +- relayer-cli/src/commands/query/client.rs | 8 +- relayer-cli/src/commands/query/connection.rs | 6 +- relayer-cli/tests/acceptance.rs | 2 +- relayer-cli/tests/integration.rs | 2 +- 6 files changed, 689 insertions(+), 10 deletions(-) create mode 100644 docs/architecture/adr-003-handler-implementation.md diff --git a/docs/architecture/adr-003-handler-implementation.md b/docs/architecture/adr-003-handler-implementation.md new file mode 100644 index 0000000000..f0f5ada59a --- /dev/null +++ b/docs/architecture/adr-003-handler-implementation.md @@ -0,0 +1,679 @@ +# ADR 003: IBC protocol implementation + +## Changelog +* 2020-08-06: Initial proposal +* 2020-08-10: Rename Handler to Message Processor +* 2020-08-14: Revamp definition of chain-specific messages, readers and keepers + +## Reader + +> This section contains all the context one needs to understand the current state, and why there is a problem. It should be as succinct as possible and introduce the high level idea behind the solution. + +TODO + +## Decision + +In this ADR, we provide recommendations for implementing the IBC message processing logic within the `ibc-rs` crate. +Concepts are introduced in the order given by a topological sort of their dependencies on each other. + +### Events + +IBC message processors must be able to emit events which will then be broadcasted via the node's pub/sub mechanism, +and eventually picked up by the IBC relayer. + +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. + +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, +each IBC submodule should defined its own event type and associated variants. + +[events]: https://github.com/informalsystems/ibc-rs/blob/bf84a73ef7b3d5e9a434c9af96165997382dcc9d/modules/src/events.rs#L15-L43 + +```rust +pub struct Attribute { + key: String, + value: String, +} + +impl Attribute { + pub fn new(key: String, value: String) -> Self; +} + +pub enum EventType { + Message, + Custom(String), +} + +pub struct Event { + typ: EventType, + attributes: Vec, +} + +impl Event { + pub fn new(typ: EventType, attrs: Vec<(String, String)>) -> Self; +} +``` + +### Logging + +IBC message processors must be able to log information for introspectability and ease of debugging. +A message processor can output multiple log records, which are expressed as a pair of a status and a +log line. The interface for emitting log records is described in the next section. + +```rust +pub enum LogStatus { + Success, + Info, + Warning, + Error, +} + +pub struct Log { + status: LogStatus, + body: String, +} + +impl Log { + fn success(msg: impl Display) -> Self; + fn info(msg: impl Display) -> Self; + fn warning(msg: impl Display) -> Self; + fn error(msg: impl Display) -> Self; +} +``` + +### Message processor output + +IBC message processors must be able to return arbitrary data, together with events and log records, as descibed above. +As a message processor may fail, it is necessary to keep track of errors. + +To this end, we introduce a type for the return value of a message processor: + +```rust +pub type HandlerResult = Result, E>; + +pub struct HandlerOutput { + pub result: T, + pub log: Vec, + pub events: Vec, +} +``` + +We introduce a builder interface to be used within the message processor implementation to incrementally build a `HandlerOutput` value. + +```rust +impl HandlerOutput { + pub fn builder() -> HandlerOutputBuilder { + HandlerOutputBuilder::new() + } +} + +pub struct HandlerOutputBuilder { + log: Vec, + events: Vec, + marker: PhantomData, +} + +impl HandlerOutputBuilder { + pub fn log(&mut self, log: impl Into); + pub fn emit(&mut self, event: impl Into); + pub fn with_result(self, result: T) -> HandlerOutput; +} +``` + +We provide below an example usage of the builder API: + +```rust +fn some_ibc_handler() -> HandlerResult { + let mut output = HandlerOutput::builder(); + + // ... + + output.log(Log::info("did something")) + + // ... + + output.log(Log::success("all good")); + output.emit(SomeEvent::AllGood); + + Ok(output.with_result(42)); +} +``` + +### IBC Submodule + +The various IBC messages and their processing logic, as described in the IBC specification, +are split into a collection of submodules, each pertaining to a specific aspect of +the IBC protocol, eg. client lifecycle management, connection lifecycle management, +packet relay, etc. + +In this section we propose a general approach to implement the message processors for a submodule. +As a running example we will use a dummy submodule that deals with connections, which should not +be mistaken for the actual ICS 003 Connection submodule. + +#### Events + +The events which may be emitted by the message processors of a submodule should be defined +as an enumeration, while a way of converting those into the generic `Event` type +defined in a previous section should be provided via the `From` trait. + +```rust +pub enum ConnectionEvent { + ConnectionOpenInit(ConnectionId), + ConnectionOpenTry(ConnectionId), +} + +impl From for Event { + fn from(ce: ConnectionEvent) -> Event { + match ce { + ConnectionEvent::ConnectionOpenInit(connection_id) => Event::new( + EventType::Custom("ConnectionOpenInit".to_string()), + vec![("connection_id".to_string(), connection_id.to_string())], + ), + ConnectionEvent::ConnectionOpenTry(connection_id) => Event::new( + EventType::Custom("ConnectionOpenTry".to_string()), + vec![("connection_id".to_string(), connection_id.to_string())], + ), + } + } +} +``` + +#### Reader + +A typical message processor will need to read data from the chain state at the current height, +via the private and provable stores. + +To avoid coupling between the message processor interface and the store API, we introduce an interface +for accessing this data. This interface, called a `Reader`, is shared between all message processors +in a submodule, as those typically access the same data. + +Having a high-level interface for this purpose helps avoiding coupling which makes +writing unit tests for the message processors easier, as one does not need to provide a concrete +store, or to mock one. + +```rust +pub trait ConnectionReader +{ + fn connection_end(&self, connection_id: &ConnectionId) -> Option; +} +``` + +A production implementation of this `Reader` would hold references to both the private and provable +store at the current height where the message processor executes, but we omit the actual implementation as +the store interfaces are yet to be defined, as is the general IBC top-level module machinery. + +A mock implementation of the `ConnectionReader` trait could look as follows: + +```rust +struct MockConnectionReader { + connection_id: ConnectionId, + connection_end: Option, + client_reader: MockClientReader, +} + +impl ConnectionReader for MockConnectionReader { + fn connection_end(&self, connection_id: &ConnectionId) -> Option { + if connection_id == &self.connection_id { + self.connection_end.clone() + } else { + None + } + } +} +``` + +#### Keeper + +Once a message processor executes successfully, some data will typically need to be persisted in the chain state +via the private/provable store interfaces. In the same vein as for the reader defined in the previous section, +a submodule should define a trait which provides operations to persist such data. +The same considerations w.r.t. to coupling and unit-testing apply here as well. + +```rust +pub trait ConnectionKeeper { + fn store_connection( + &mut self, + client_id: ConnectionId, + client_type: ConnectionType, + ) -> Result<(), Error>; + + fn add_connection_to_client( + &mut self, + client_id: ClientId, + connection_id: ConnectionId, + ) -> Result<(), Error>; +} +``` + +#### Submodule implementation + +We now come to the actual definition of a message processor for a submodule. + +We recommend each message processor to be defined within its own Rust module, named +after the message processor itself. For example, the "Create Client" message processor of ICS 002 would +be defined in `ibc_modules::ics02_client::handler::create_client`. + +##### Message type + +Each message processor must define a datatype which represent the message it can process. + +```rust +pub struct MsgConnectionOpenInit { + connection_id: ConnectionId, + client_id: ClientId, + counterparty: Counterparty, +} +``` + +##### Message processor implementation + +In this section we provide guidelines for implementating an actual message processor. + +We divide the message processor in two parts: processing and persistance. + +###### Processing + +The actual logic of the message processor is expressed as a pure function, typically named +`process`, which takes as arguments a `Reader` and the corresponding message, and returns +a `HandlerOutput`, where `T` is a concrete datatype and `E` is an error type which defines +all potential errors yielded by the message processors of the current submodule. + +```rust +pub struct ConnectionMsgProcessingResult { + connection_id: ConnectionId, + connection_end: ConnectionEnd, +} +``` + +The `process` function will typically read data via the `Reader`, perform checks and validation, construct new +datatypes, emit log records and events, and eventually return some data together with objects to be persisted. + +To this end, this `process` function will create and manipulate a `HandlerOutput` value like described in +the corresponding section. + +```rust +pub fn process( + reader: &dyn ConnectionReader, + msg: MsgConnectionOpenInit, +) -> HandlerResult +{ + let mut output = HandlerOutput::builder(); + + let MsgConnectionOpenInit { connection_id, client_id, counterparty, } = msg; + + if reader.connection_end(&connection_id).is_some() { + return Err(Kind::ConnectionAlreadyExists(connection_id).into()); + } + + output.log("success: no connection state found"); + + if reader.client_reader.client_state(&client_id).is_none() { + return Err(Kind::ClientForConnectionMissing(client_id).into()); + } + + output.log("success: client found"); + + output.emit(ConnectionEvent::ConnectionOpenInit(connection_id.clone())); + + Ok(output.with_result(ConnectionMsgProcessingResult { + connection_id, + client_id, + counterparty, + })) +} +``` + +###### Persistence + +If the `process` function specified above succeeds, the result value it yielded is then +passed to a function named `keep`, which is responsible for persisting the objects constructed +by the processing function. This `keep` function takes the submodule's `Keeper` and the result +type defined above, and performs side-effecting calls to the keeper's methods to persist the result. + +Below is given an implementation of the `keep` function for the "Create Connection" message processors: + +```rust +pub fn keep( + keeper: &mut dyn ConnectionKeeper, + result: ConnectionMsgProcessingResult, +) -> Result<(), Error> +{ + keeper.store_connection(result.connection_id.clone(), result.connection_end)?; + keeper.add_connection_to_client(result.client_id, result.connection_id)?; + + Ok(()) +} +``` + +##### Submodule dispatcher + +> This section is very much a work in progress, as further investigation into what +> a production-ready implementation of the `ctx` parameter of the top-level dispatcher +> is required. As such, implementors should feel free to disregard the recommendations +> below, and are encouraged to come up with amendments to this ADR to better capture +> the actual requirements. + +Each submodule is responsible for dispatching the messages it is given to the appropriate +message processing function and, if successful, pass the resulting data to the persistance +function defined in the previous section. + +To this end, the submodule should define an enumeration of all messages, in order +for the top-level submodule dispatcher to forward them to the appropriate processor. +Such a definition for the ICS 003 Connection submodule is given below. + +```rust +pub enum ConnectionMsg { + ConnectionOpenInit(MsgConnectionOpenInit), + ConnectionOpenTry(MsgConnectionOpenTry), + ... +} +``` +The actual implementation of a submodule dispatcher is quite straightforward and unlikely to vary +much in substance between submodules. We give an implementation for the ICS 003 Connection module below. + +```rust +pub fn dispatch(ctx: &mut Ctx, msg: Msg) -> Result, Error> +where + Ctx: ConnectionReader + ConnectionKeeper, +{ + match msg { + Msg::ConnectionOpenInit(msg) => { + let HandlerOutput { + result, + log, + events, + } = connection_open_init::process(ctx, msg)?; + + connection::keep(ctx, result)?; + + Ok(HandlerOutput::builder() + .with_log(log) + .with_events(events) + .with_result(())) + } + + Msg::ConnectionOpenTry(msg) => // omitted + } +} +``` + +In essence, a top-level dispatcher is a function of a message wrapped in the enumeration introduced above, +and a "context" which implements both the `Reader` and `Keeper` interfaces. + +### Dealing with chain-specific datatypes + +The ICS 002 Client submodule stands out from the other submodules as it needs +to deal with chain-specific datatypes, such as `Header`, `ClientState`, and +`ConsensusState`. + +To abstract over chain-specific datatypes, we introduce a trait which specifies +both which types we need to abstract over, and their interface. + +For the ICS 002 Client submodule, this trait looks as follow: + +```rust +pub trait ClientDef { + type Header: Header; + type ClientState: ClientState; + type ConsensusState: ConsensusState; +} +``` + +The `ClientDef` trait specifies three datatypes, and their corresponding interface, which is provided +via a trait defined in the same submodule. + +A production implementation of this interface would instantiate these types with the concrete +types used by the chain, eg. Tendermint datatypes. Each concrete datatype must be provided +with a `From` instance to lift it into its corresponding `Any...` enumeration. + +For the purpose of unit-testing, a mock implementation of the `ClientDef` trait could look as follows: + +```rust +struct MockHeader(u32); + +impl Header for MockHeader { + // omitted +} + +impl From for AnyHeader { + fn from(mh: MockHeader) -> Self { + Self::Mock(mh) + } +} + +struct MockClientState(u32); + +impl ClientState for MockClientState { + // omitted +} + +impl From for AnyClientState { + fn from(mcs: MockClientState) -> Self { + Self::Mock(mcs) + } +} + +struct MockConsensusState(u32); + +impl ConsensusState for MockConsensusState { + // omitted +} + +impl From for AnyConsensusState { + fn from(mcs: MockConsensusState) -> Self { + Self::Mock(mcs) + } +} + +struct MockClient; + +impl ClientDef for MockClient { + type Header = MockHeader; + type ClientState = MockClientState; + type ConsensusState = MockConsensusState; +} +``` + +Since the actual type of client can only be determined at runtime, we cannot encode +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 +which are agnostic to the actual type of client being used. + +We could alternatively model all chain-specific datatypes as boxed trait objects (`Box`), +but this approach runs into a lot of limitations of trait objects, such as the inability to easily +require such trait objects to be Clonable, or Serializable, or to define an equality relation on them. +Some support for such functionality can be found in third-party libraries, but the overall experience +for the developer is too subpar. + +We thus settle on a different strategy: lifting chain-specific data into an `enum` over all +possible chain types. + +For example, to model a chain-specific `Header` type, we would define an enumeration in the following +way: + +```rust +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] // TODO: Add Eq +pub enum AnyHeader { + Mock(mocks::MockHeader), + Tendermint(tendermint::header::Header), +} + +impl Header for AnyHeader { + fn height(&self) -> Height { + match self { + Self::Mock(header) => header.height(), + Self::Tendermint(header) => header.height(), + } + } + + fn client_type(&self) -> ClientType { + match self { + Self::Mock(header) => header.client_type(), + Self::Tendermint(header) => header.client_type(), + } + } +} +``` + +This enumeration dispatches method calls to the underlying datatype at runtime, while +hiding the latter, and is thus akin to a proper existential type without running +into any limitations of the Rust type system (`impl Header` bounds not being allowed +everywhere, `Header` not being able to be treated as a trait objects because of `Clone`, +`PartialEq` and `Serialize`, `Deserialize` bounds, etc.) + +Other chain-specific datatypes, such as `ClientState` and `ConsensusState` require their own +enumeration over all possible implementations. + +On top of that, we also need to lift the specific client definitions (`ClientDef` instances), +into their own enumeration, as follows: + +```rust +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum AnyClient { + Mock(mocks::MockClient), + Tendermint(tendermint::TendermintClient), +} + +impl ClientDef for AnyClient { + type Header = AnyHeader; + type ClientState = AnyClientState; + type ConsensusState = AnyConsensusState; +} +``` + +Messages can now be defined generically over the `ClientDef` instance: + + +```rust +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub struct MsgCreateClient { + pub client_id: ClientId, + pub client_type: ClientType, + pub consensus_state: CD::ConsensusState, +} + +pub struct MsgUpdateClient { + pub client_id: ClientId, + pub header: CD::Header, +} +``` + +The `Keeper` and `Reader` traits are defined for any client: + +```rust +pub trait ClientReader { + fn client_type(&self, client_id: &ClientId) -> Option; + fn client_state(&self, client_id: &ClientId) -> Option; + fn consensus_state(&self, client_id: &ClientId, height: Height) -> Option; +} + +pub trait ClientKeeper { + fn store_client_type( + &mut self, + client_id: ClientId, + client_type: ClientType, + ) -> Result<(), Error>; + + fn store_client_state( + &mut self, + client_id: ClientId, + client_state: AnyClientState, + ) -> Result<(), Error>; + + fn store_consensus_state( + &mut self, + client_id: ClientId, + consensus_state: AnyConsensusState, + ) -> Result<(), Error>; +} +``` + +This way, only one implementation of the `ClientReader` and `ClientKeeper` trait is required, +as it can delegate eg. the serialization of the underlying datatypes to the `Serialize` bound +of the `Any...` wrappper. + +Both the `process` and `keep` function are defined to take a message generic over +the actual client type: + +```rust +pub fn process( + ctx: &dyn ClientReader, + msg: MsgCreateClient, +) -> HandlerResult, Error>; + +pub fn keep( + keeper: &mut dyn ClientKeeper, + result: CreateClientResult, +) -> Result<(), Error>; +``` + +Same for the top-level dispatcher: + +```rust +pub fn dispatch(ctx: &mut Ctx, msg: ClientMsg) -> Result, Error> +where + Ctx: ClientReader + ClientKeeper; +``` + +With this boilerplate out of way, one can write tests using a mock client, and associated mock datatypes +in a fairly straightforward way, taking advantage of the `From` instance to lift concerete mock datatypes +into the `Any...` enumeration: + +```rust + #[test] + fn test_create_client_ok() { + let client_id: ClientId = "mockclient".parse().unwrap(); + + let reader = MockClientReader { + client_id: client_id.clone(), + client_type: None, + client_state: None, + consensus_state: None, + }; + + let msg = MsgCreateClient { + client_id, + client_type: ClientType::Tendermint, + consensus_state: MockConsensusState(42).into(), // lift into `AnyConsensusState` + }; + + let output = process(&reader, msg.clone()); + + match output { + Ok(HandlerOutput { + result, + events, + log, + }) => { + // snip + } + Err(err) => { + panic!("unexpected error: {}", err); + } + } + } +``` + +## Status + +Proposed + +## Consequences + +> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. + +### Positive + +### Negative + +### Neutral + +## References + +> Are there any relevant PR comments, issues that led up to this, or articles refernced for why we made the given design choice? If so link them here! + +* {reference link} diff --git a/relayer-cli/src/commands/query/channel.rs b/relayer-cli/src/commands/query/channel.rs index 7c26452aaa..71b4609258 100644 --- a/relayer-cli/src/commands/query/channel.rs +++ b/relayer-cli/src/commands/query/channel.rs @@ -189,7 +189,7 @@ mod tests { name: "Bad channel, name too short".to_string(), params: QueryChannelEndCmd { channel_id: Some("chshort".to_string()), - ..default_params.clone() + ..default_params }, want_pass: false, }, diff --git a/relayer-cli/src/commands/query/client.rs b/relayer-cli/src/commands/query/client.rs index a951b6ba80..80aee8facc 100644 --- a/relayer-cli/src/commands/query/client.rs +++ b/relayer-cli/src/commands/query/client.rs @@ -348,7 +348,7 @@ mod tests { name: "No client id specified".to_string(), params: QueryClientStateCmd { client_id: None, - ..default_params.clone() + ..default_params }, want_pass: false, }, @@ -356,7 +356,7 @@ mod tests { name: "Bad client id, non-alpha".to_string(), params: QueryClientStateCmd { client_id: Some("p34".to_string()), - ..default_params.clone() + ..default_params }, want_pass: false, }, @@ -434,7 +434,7 @@ mod tests { name: "No client id specified".to_string(), params: QueryClientConnectionsCmd { client_id: None, - ..default_params.clone() + ..default_params }, want_pass: false, }, @@ -442,7 +442,7 @@ mod tests { name: "Bad client id, non-alpha".to_string(), params: QueryClientConnectionsCmd { client_id: Some("p34".to_string()), - ..default_params.clone() + ..default_params }, want_pass: false, }, diff --git a/relayer-cli/src/commands/query/connection.rs b/relayer-cli/src/commands/query/connection.rs index 74c46f0694..e03e6871d7 100644 --- a/relayer-cli/src/commands/query/connection.rs +++ b/relayer-cli/src/commands/query/connection.rs @@ -141,7 +141,7 @@ mod tests { name: "No connection id specified".to_string(), params: QueryConnectionEndCmd { connection_id: None, - ..default_params.clone() + ..default_params }, want_pass: false, }, @@ -149,7 +149,7 @@ mod tests { name: "Bad connection, non-alpha".to_string(), params: QueryConnectionEndCmd { connection_id: Some("conn01".to_string()), - ..default_params.clone() + ..default_params }, want_pass: false, }, @@ -157,7 +157,7 @@ mod tests { name: "Bad connection, name too short".to_string(), params: QueryConnectionEndCmd { connection_id: Some("connshort".to_string()), - ..default_params.clone() + ..default_params }, want_pass: false, }, diff --git a/relayer-cli/tests/acceptance.rs b/relayer-cli/tests/acceptance.rs index 2da38d04ca..9063229a18 100644 --- a/relayer-cli/tests/acceptance.rs +++ b/relayer-cli/tests/acceptance.rs @@ -27,7 +27,7 @@ use once_cell::sync::Lazy; /// the runner acquire a mutex when executing commands and inspecting /// exit statuses, serializing what would otherwise be multithreaded /// invocations as `cargo test` executes tests in parallel by default. -pub static RUNNER: Lazy = Lazy::new(|| CmdRunner::default()); +pub static RUNNER: Lazy = Lazy::new(CmdRunner::default); /// Use `Config::default()` value if no config or args #[test] diff --git a/relayer-cli/tests/integration.rs b/relayer-cli/tests/integration.rs index ac6efe2704..16de6a4001 100644 --- a/relayer-cli/tests/integration.rs +++ b/relayer-cli/tests/integration.rs @@ -63,7 +63,7 @@ fn query_connection_id() { assert_eq!(query.counterparty().connection_id(), "connectionidtwo"); assert_eq!( query.counterparty().prefix(), - &CommitmentPrefix::new("prefix".as_bytes().to_vec()) + &CommitmentPrefix::new(b"prefix".to_vec()) ); assert_eq!( query.versions(),