ADR for redesigning the modules' API

[hu55a1n1]

Summary

Create an ADR for redesigning the modules' API in a way that is more robust and usable for our customers.

Requirements

• async - The API must be usable in an async context.
• Usability - The API must be usable with a variety of execution models and must avoid making assumptions about the host
  blockchain env.
• Composability - The API must be flexible and extensible by its users. It should additionally provide building blocks
  that are composable.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[hu55a1n1]

IMHO, to ensure that we have a low barrier of entry/usability, we must provide two APIs - a low-level bare-bones API
that is easy to use/compose, and a high-level API (that builds on the low-level API and) that is robust and extensible.
Here's my proposal ->

Phases in a handler's lifecycle

From accepting an input message to executing it and generating events and logs, a handler's lifecycle can be broadly
divided into 4 phases ->

• Stateless validation - This typically includes conversion of a raw message to a domain type message.
• Stateful validation - Involves checks that need access to state. May additionally include proof verification.
• Message processing - Includes execution of the message resulting in the generation of events and state changes.
• State modification - This step applies the results of the previous phase to the state.

Note that the above phases constitute a state machine where each phase must be executed in order and depends on the
result of the previous phase.
The above phases can be expressed in Rust as follows ->

Stateless validation ('Validate' phase)

Stateless validation can be expressed as a simple impl TryFrom<RawMsg> for Msg, since a handler's lifecycle begins
with a message and no other context is required at this stage.

fn validate(msg: Self::RawMessage) -> Result<Self::Message, Self::Error> {
    msg.try_into()
}

Stateful validation ('Check' phase)

Here, we need a context (from the host and the light client) to validate the contents of the message against.
We could return an intermediate result that would be used by the next phase (similar to the type-state concept) - this
ensures that these functions are executed in order as long as the intermediate result is not constructible by users.

fn check<ContextRead, Message, CheckResult>(
    _ctx: &ContextRead,
    _msg: Message,
) -> Result<CheckResult, Error> {
    todo!()
}

Note that msg is consumed here.

Message processing ('Process' phase)

This can be modelled like so ->

fn process<ContextRead, CheckResult, ProcessResult>(
    _ctx: &ContextRead,
    _check_result: CheckResult,
) -> Result<ProcessResult, Error> {
    todo!()
}

ProcessResult must contain all state modifications that must be written. Note that so far, the state hasn't been
modified.

State modification ('Write' phase)

This step writes the state modifications using the result from the previous phase and completes the lifecycle of the
handler. The result is a MsgReceipt that contains events and logs that were generated by the handler.

fn write<ContextWrite, ProcessResult, Event>(
    ctx: &mut ContextWrite,
    process_result: ProcessResult,
) -> Result<MsgReceipt<Event>, Error> {
    todo!()
}

Note: The library could guarantee that write() will not fail as long as the host implementation can guarantee
that ContextWrite methods are infallible.

Events and logs

Although a handler itself is mostly stateless as demonstrated by above functions, it must provide facilities for logging
and emitting events. Technically speaking, handlers can generate logs and events during any phase of their lifecycle.
This can be expressed using the following traits ->

pub trait Logger: Into<Vec<String>> {
    /// Return the logs generated so-far
    fn logs(&self) -> &[String];

    /// Log a message
    fn log_message(&mut self, _msg: impl ToString);
}

pub trait EventEmitter: Into<Vec<Self::Event>> {
    /// Event type
    type Event;

    /// Return the events generated so-far
    fn events(&self) -> &[Self::Event];

    /// Emit an event
    fn emit_event(&mut self, _event: Self::Event);
}

Low level API

Ideally, the library must provide a low-level API that contains core logic and is easy to use. It should provide one
freestanding function per phase, for e.g. ->

pub fn update_client_check<Handler: Logger + EventEmitter, CtxRead: ClientReader>(
    _handler: &mut Handler,
    _ctx: &CtxRead,
    _msg: MsgUpdateClient,
) -> Result<UpdateClientCheckResult, Error> {
    /* ... */
}

The rationale for providing this low level API is to lower the barrier for entry and provide users with well-audited
core handler logic in the form of building-blocks that they can compose according to their host design.

Specifying dependencies

The degree of specificity of what the ClientReader trait must define is debatable. We could have it contain all
client related host methods and reuse it with all client handler functions, or let it contain just the ones that
update_client_check() uses.
We could go even further and directly specify dependencies as function parameters or as closures ->

pub fn update_client_check<CS: ConsensusState, Handler: Logger + EventEmitter>(
    _handler: &mut Handler,
    _client_state: impl FnOnce(&ClientId) -> Result<Box<dyn ClientState>, Error>,
    _consensus_state: CS,
    _msg: MsgUpdateClient,
) -> Result<UpdateClientCheckResult, Error> {
    /* ... */
}

The benefit of using closures is that they are easier to satisfy, they're more specific and self-contained, and they
support lazy evaluation.

async handlers

Handler functions can be made async to allow them to be used in async contexts. Context trait methods can be
made async and closures can be made to return Futures if that is desirable.

pub async fn update_client_check<Handler: Logger + EventEmitter>(
    _handler: &mut Handler,
    _client_state: impl FnOnce(&ClientId) -> Result<Future<Box<dyn ClientState>>, Error>,
    _msg: MsgUpdateClient,
) -> Result<UpdateClientCheckResult, Error> {
    /* ... */
}

High level API and the Handler trait

A higher level API maybe provided and can be expressed in the form of a Handler trait ->

pub trait Handler {
    /// Error and (intermediate) results
    type Error;
    type CheckResult;
    type ProcessResult;

    /// Message types
    /// Conversion from raw-type to domain-type encodes stateless validation
    type RawMessage;
    type Message: TryFrom<Self::RawMessage, Error=Self::Error>;

    /// Subset of IbcEvents that this handler could emit
    type Event: Into<IbcEvent>;

    /// Context from host (includes light client related context)
    type ContextRead;
    type ContextWrite;

    /// Other facilities
    type Logger: Logger;
    type EventEmitter: EventEmitter<Event=Self::Event>;

    /// Stateless validation
    fn validate(&self, msg: Self::RawMessage) -> Result<Self::Message, Self::Error> {
        msg.try_into()
    }

    /// Stateful validation of the message (includes proof verification)
    /// `&mut self` is required for writing logs and emitting events
    fn check(
        &mut self,
        ctx: &Self::ContextRead,
        msg: Self::Message,
    ) -> Result<Self::CheckResult, Self::Error>;

    /// Process/Execute the message
    /// `&mut self` is required for writing logs and emitting events
    fn process(
        &mut self,
        ctx: &Self::ContextRead,
        check_result: Self::CheckResult,
    ) -> Result<Self::ProcessResult, Self::Error>;

    /// Write the result
    /// Consumes `self` and returns generated logs and events
    fn write(
        self,
        ctx: &mut Self::ContextWrite,
        process_result: Self::ProcessResult,
    ) -> Result<MsgReceipt<Self::Event>, Self::Error>;
}

Such a trait will aid us in implementing handler functions in an orderly manner.

Example implementation

Here's an implementation of the update-client handler with the above trait ->

#[derive(Clone, Debug)]
pub struct UpdateClientHandler<
    CtxRead,
    CtxWrite,
    Logger = DefaultLogger,
    EventEmitter = DefaultEventEmitter<UpdateClientEvent>,
> {
    logger: Logger,
    event_emitter: EventEmitter,
    _ctx: PhantomData<(CtxRead, CtxWrite)>,
}

impl<
    CtxRead: ClientReader,
    CtxWrite: ClientKeeper,
    L: Logger,
    E: EventEmitter<Event=UpdateClientEvent>,
> Handler for UpdateClientHandler<CtxRead, CtxWrite, L, E>
{
    type Error = Error;
    type CheckResult = UpdateClientCheckResult;
    type ProcessResult = UpdateClientResult;
    type RawMessage = RawMsgUpdateClient;
    type Message = MsgUpdateClient;
    type Event = UpdateClientEvent;
    type ContextRead = CtxRead;
    type ContextWrite = CtxWrite;
    type Logger = L;
    type EventEmitter = E;

    fn check(
        &mut self,
        ctx: &Self::ContextRead,
        msg: Self::Message,
    ) -> Result<Self::CheckResult, Self::Error> {
        update_client_check(|client_id| ctx.client_state(client_id), msg)
    }

    fn process(
        &mut self,
        ctx: &Self::ContextRead,
        check_result: Self::CheckResult,
    ) -> Result<Self::ProcessResult, Self::Error> {
        update_client_process(&mut self.event_emitter, ctx, check_result)
    }

    fn write(
        self,
        ctx: &mut Self::ContextWrite,
        process_result: Self::ProcessResult,
    ) -> Result<MsgReceipt<Self::Event>, Self::Error> {
        update_client_write(ctx, process_result)?;
        Ok(MsgReceipt {
            events: self.event_emitter.into(),
            log: self.logger.into(),
        })
    }
}

Composition

The power of the high level API lies in its ability of composition. For example, ICS04 handlers must provide the ability
to register and call application callbacks. This can be implemented easily by defining
a ModuleCallbackHandler<Handler> that provides callback functionality and wrapping the core ICS04 handler, for
e.g. CoreChanOpenTryHandler, in it.

/// ModuleCallbackHandler that implements support for module callbacks using a specialized `CheckResult` and `ProcessResult`
pub struct ModuleCallbackHandler<Handler> {
    inner: Handler,
}

impl<H: Handler> Handler for ModuleCallbackHandler<H> { /* ... */ }

/// Core ICS04 ChanOpenTry handler
pub struct CoreChanOpenTryHandler<
    CtxRead,
    CtxWrite,
    Logger = DefaultLogger,
    EventEmitter = DefaultEventEmitter<IbcEvent>,
> {
    logger: Logger,
    event_emitter: EventEmitter,
    _ctx: PhantomData<(CtxRead, CtxWrite)>,
}

impl</* ... */> Handler for CoreChanOpenTryHandler</* ... */> { /* ... */ }

pub type ChanOpenTryHandler</* ... */> = ModuleCallbackHandler<CoreChanOpenTryHandler</* ... */>>;

ICS26 routing implementation

The same interface can then be used to provide an entry-level API for the library. An Ics26Handler can be defined as
an enum over all handlers that also implements the Handler trait by simply delegating to them.

pub trait Ics26Reader: ClientReader + ConnectionReader + ChannelReader + PortReader {}

impl<T: Ics26Context> Ics26Reader for T {}

pub trait Ics26Keeper: ClientKeeper + ConnectionKeeper + ChannelKeeper {}

impl<T: Ics26Context> Ics26Keeper for T {}

pub enum Ics26CheckResult {
    ClientUpdate(UpdateClientCheckResult),
}

pub enum Ics26ProcessResult {
    ClientUpdate(UpdateClientResult),
}

pub enum Ics26Handler<CtxRead, CtxWrite> {
    ClientUpdate(UpdateClientHandler<CtxRead, CtxWrite>),
}

impl<CtxRead: Ics26Reader, CtxWrite: Ics26Keeper> Handler for Ics26Handler<CtxRead, CtxWrite> {
    type Error = Error;
    type CheckResult = Ics26CheckResult;
    type ProcessResult = Ics26ProcessResult;
    type RawMessage = Any;
    type Message = Ics26Envelope;
    type Event = IbcEvent;
    type ContextRead = CtxRead;
    type ContextWrite = CtxWrite;
    type Logger = DefaultLogger;
    type EventEmitter = DefaultEventEmitter<Self::Event>;

    fn check(
        &mut self,
        ctx: &Self::ContextRead,
        msg: Self::Message,
    ) -> Result<Self::CheckResult, Self::Error> {
        match (msg, self) {
            (
                Ics26Envelope::Ics2Msg(ClientMsg::UpdateClient(msg)),
                Ics26Handler::ClientUpdate(handler),
            ) => handler
                .check(ctx, msg)
                .map(Ics26CheckResult::ClientUpdate)
                .map_err(Error::ics02_client),
            _ => todo!(),
        }
    }

    fn process(
        &mut self,
        ctx: &Self::ContextRead,
        check_result: Self::CheckResult,
    ) -> Result<Self::ProcessResult, Self::Error> {
        match (check_result, self) {
            (
                Ics26CheckResult::ClientUpdate(check_result),
                Ics26Handler::ClientUpdate(handler),
            ) => handler
                .process(ctx, check_result)
                .map(Ics26ProcessResult::ClientUpdate)
                .map_err(Error::ics02_client),
        }
    }

    fn write(
        self,
        ctx: &mut Self::ContextWrite,
        process_result: Self::ProcessResult,
    ) -> Result<MsgReceipt<Self::Event>, Self::Error> {
        match (process_result, self) {
            (
                Ics26ProcessResult::ClientUpdate(process_result),
                Ics26Handler::ClientUpdate(handler),
            ) => handler
                .write(ctx, process_result)
                .map(|receipt| MsgReceipt {
                    events: receipt.events.into_iter().map(Into::into).collect(),
                    log: receipt.log,
                })
                .map_err(Error::ics02_client),
        }
    }
}

The Ics26Handler could provide a ctor like so ->

impl<CtxRead, CtxWrite> Ics26Handler<CtxRead, CtxWrite> {
    pub fn new(msg: &Ics26Envelope) -> Self {
        match msg {
            Ics26Envelope::Ics2Msg(ClientMsg::UpdateClient(_)) => {
                Self::ClientUpdate(UpdateClientHandler::default())
            }
            _ => todo!(),
        }
    }
}

Example usage

fn test() -> Result<(), Error> {
    let msg = Ics26Envelope::Ics2Msg(ClientMsg::UpdateClient(MsgUpdateClient {
        client_id: client_id.clone(),
        header: block.into(),
        signer,
    }));

    let mut ics26_handler = Ics26Handler::new(&msg);
    let check_result = ics26_handler.check(&ctx, msg)?;
    let process_result = ics26_handler.process(&ctx, check_result)?;
    let receipt = ics26_handler.write(&mut ctx, process_result);
    assert!(receipt.is_ok());
}

[hu55a1n1]

async support

It appears that using an async API in a non-async context is easier that using a blocking API in an async context. For
this reason it might be better if the API was made async by default.

I see two options here - either we transition to an async API completely by making all trait methods, handler
functions, etc. async or we use a proc-macro based crate like maybe_async (thanks to @romac for the suggestion!).

With maybe_async, all we would need to do on our side would be to make trait methods and other functions async and
mark them with the #[maybe_async] attribute.
Sync implementations can continue to use the API as before by setting the is_sync feature (ideally we enable this by
default). Async implementations will have to mark traits using the #[async_trait] attribute.

Users seem to run into problems sometimes with maybe_async as discussed here, but I feel
our use-case is simple, and it shouldn't be a problem for us. We should look into it in more detail.

[plafer]

Based on my analysis of Namada's and Penumbra's codebases as concrete use cases, I came to different conclusions from what is proposed here. I will first comment on your suggestions, and then propose an alternative design.

About the 4 phases

From accepting an input message to executing it and generating events and logs, a handler's lifecycle can be broadly divided into 4 phases
...
Note that the above phases constitute a state machine where each phase must be executed in order and depends on the result of the previous phase.

I disagree with this model. For one, I don't think each phase should feed into one another; instead, they should be independent. Looking at Namada and Penumbra, neither could work with this proposal since feeding the result of one phase into the other doesn't work with their framework. Concretely, for example, Namada first runs "Message processing" and "State modification", and then runs both validations in parallel (i.e. their validity predicates).

I also don't see the value in separating "Message processing" from "state modification". Making writes generic (i.e. using our "Keepers") handles that use case: we are implicitly telling chains to provide an implementation where writes can be reverted until committed. FWIW, both Penumbra and Namada use a "revertible store" which can be reverted until it is committed, and so will always apply state modifications right after message processing. And I expect all chains to have that kind of functionality.

Hence, I think there should only be 3 independent phases that don't feed into one another:

1. stateless validation
2. stateful validation
3. execution (where state modifications are written using the provided Context)

By "independent", I mean that they all they all take as input the transaction, and either succeed or fail with an error.

About the low vs high level APIs

I don't see the benefit of this separation, because I see the abovementioned 3 phases as atomic. That is, you cannot separate them further without breaking IBC. Also, I find that the low-level API forces us to create more Context traits than necessary; I will touch on that again when I write out my proposal. But the gist is that I don't think we need more than 2 or 3 context traits: StatelessValidationContext, StatefulValidationContext, and ExecutionContext.

About async

One thing seems clear: we will need separate context traits and handlers for sync and async. Whether or not we use something like maybe_async requires further investigation. In any case, we could limit the scope of this ADR to just the sync case, knowing that the async case that comes later would be very similar (potentially even "generic over async" if we use something like maybe_async).

Proposal

I am still fleshing out the details of the design I have in mind, and will post later.

[hu55a1n1]

Simplifying host state access

Currently, we define an entry-point function which requires an Ics26Context ->

pub fn deliver<Ctx: Ics26Context>(_ctx: &mut Ctx, _message: Any) -> Result<MsgReceipt, Error> { /* ... */ }

And Ics26Context is defined as having multiple Reader/Keeper supertraits, such that individual handlers usually
depend on one of these supertraits for host state access ->

pub trait Ics26Context:
ClientReader
+ ClientKeeper
+ ConnectionReader
+ ConnectionKeeper
+ ChannelKeeper
+ ChannelReader
+ PortReader
{}

The problem with this design is that there is a duplication of methods in these supertraits and not all handlers use all
methods from a Reader/Keeper trait.

I propose we define one context/state trait per handler function that only defines the methods that are relevant for
that handler function. (this is similar to @soareschen's proposal in the relayer v1.5 design)
For e.g., the update_client_check() handler would define a trait containing just one method to access ClientState ->

trait UpdateClientCheckCtx {
    fn client_state(&self, client_id: &ClientId) -> Result<Box<dyn ClientState>, Error>;
}

And the Ics26Context would not have any supertraits and would instead define such methods in itself. And a blanket
impl for UpdateClientCheckCtx would be provided for any type that impls Ics26Context. e.g. ->

pub trait Ics26Context {
    fn client_state(&self, client_id: &ClientId) -> Result<Box<dyn ClientState>, Error>;

    // Followed by all methods from all handler specific contexts 
}

// blanket impl of all handler function context traits for any type that implements the Ics26Context
impl<T: Ics26Context> UpdateClientCheckCtx for T {
    fn client_state(&self, client_id: &ClientId) -> Result<Box<dyn ClientState>, Error> {
        Ics26Context::client_state(self, client_id)
    }
}

Modelling the store directly in the API

Maybe we should consider modelling the store directly in the API. ICS24 requires hosts to have some form of a KV store.
It also defines specific paths/keys that handlers are permitted to write to.
We can levarage this requirement and define a Store trait that would replace the handler Context traits I introduced
previously.

pub trait Store: Send + Sync + Clone {
    type Error;
    type Path: Into<IbcPath>;

    fn set(&mut self, path: Path, value: Vec<u8>) -> Result<Option<Vec<u8>>, Self::Error>;

    fn get(&self, height: Height, path: &Path) -> Option<Vec<u8>>;

    fn delete(&mut self, path: &Path);
}

We can then define ScopedStores that provide access only to certain specified paths ->

pub struct ScopedStore<S, P> {
    inner: S,
    _path: PhantomData<P>,
}

impl<S: Store, P> Store for ScopedStore<S, P> {
    /* ... */
}

And handler functions would use this instead of Reader/Keeper contexts ->

pub fn update_client_check<S: Store, UpdateClientStore: ScopedStore<S, UpdateClientPath>>(
    _store: &UpdateClientStore,
    _msg: MsgUpdateClient,
) -> Result<UpdateClientCheckResult, Error> {
    /* ... */
}

This API provides static guarantees about the paths that a handler can access and would make it easier to apply formal
verification techniques (e.g. prusti, MBT, etc.).

ScopedStores can additionally provide methods to inspect written paths that can be used to figure out which paths were
modified, and might be useful for validity predicate based execution models ->

impl<S, P> ScopedStore<S, P> {
    fn written_paths(&self) -> Vec<P> {
        /* ... */
    }
}

[plafer]

Below is my proposal. The idea is to provide 2 entry points: one for validation, and one for execution, each with their corresponding context trait. I prefer this to the idea of having handler-specific context traits, as I think no implementation will ever need that level of control, and therefore will only increase the complexity of the library with no clear benefit.

As for your "modeling the store API" idea, I borrowed a few ideas from there. I don't like explicitly modeling the store where values are encoded Vec<u8>, as it forces the implementation to store serialized objects. Instead, I keep similar methods to our current Keeper objects, but replace the ad-hoc key parameters with the proper Ibc path.

Note that Ibc mandates all stores to store values that "look serialized", but they don't have to be. All objects stored have an encode_vec() method, which makes them easily convertible to their serialized form whenever necessary.

Proposal

We have a ValidationContext, which exposes a validate() method, and an ExecutionContext which exposes an execute() method.

trait ValidationContext {
    fn validate(&self, message: Any) -> Result<(), Error> {
        /* validation entrypoint implemented here */
    }

    /* Below are all the methods needed for validation */

    /// Returns the ClientState for the given identifier `client_id`.
    fn client_state(&self, client_id: &ClientId) -> Result<Box<dyn ClientState>, Error>;

    /// Tries to decode the given `client_state` into a concrete light client state.
    fn decode_client_state(&self, client_state: Any) -> Result<Box<dyn ClientState>, Error>;

    /// Retrieve the consensus state for the given client ID at the specified
    /// height.
    ///
    /// Returns an error if no such state exists.
    fn consensus_state(
        &self,
        client_id: &ClientId,
        height: Height,
    ) -> Result<Box<dyn ConsensusState>, Error>;

    /* ... */
}

trait ExecutionContext {
    fn execute(&mut self, message: Any) -> Result<(), Error> {
        /* Execution entrypoint implemented here */
    }

    /// Called upon successful client creation
    fn store_client_type(
        &mut self,
        client_type_path: ClientTypePath,
        client_type: ClientType,
    ) -> Result<(), Error>;

    /// Called upon successful client creation and update
    fn store_client_state(
        &mut self,
        client_state_path: ClientStatePath,
        client_state: Box<dyn ClientState>,
    ) -> Result<(), Error>;

    /// Called upon successful client creation and update
    fn store_consensus_state(
        &mut self,
        consensus_state_path: ClientConsensusStatePath,
        consensus_state: Box<dyn ConsensusState>,
    ) -> Result<(), Error>;

    /// Ibc events
    fn emit_ibc_event(event: IbcEvent);
   
    /* ... */
   
    // Note: there will be some methods that read from the store as well, 
    // duplicated from `ValidationContext`
}

We can also provide a convenience deliver() function:

fn deliver<V, E>(val_ctx: &V, exec_ctx: &mut E, message: Any) -> Result<(), Error> {
    let _ = val_ctx.validate(message)?;
    exec_ctx.execute(message)
}

[plafer]

Note about being generic over async: The rust-lang team has started an initiative to work on that. See blog post.

[hu55a1n1]

Case study: Client Update handler

I thought it would be helpful to look at the proposal from the perspective of a concrete and specific handler impl for
Namada.

Note: I chose update client as that's what I used initially and I think it makes for a good example. Implementations
have been simplified for the sake of brevity.

// Source -> https://github.com/anoma/namada/blob/main/shared/src/ledger/ibc/vp/client.rs#L227
fn validate(&self, msg: Any) -> Result<(), Error> {
    // 1. Decode message
    // 2. Get `client_state` from host using `msg.client_id`
    // 3. Construct `new_client_state` and `new_consensus_state` (with verification)
    // 4. Construct event and check it matches the event emitted in `execute()` 
}

// Source ->
// https://github.com/anoma/namada/blob/main/shared/src/ledger/ibc/handler.rs#L153
// https://github.com/anoma/namada/blob/main/shared/src/ledger/ibc/handler.rs#L241
fn execute(&mut self, msg: Any) -> Result<(), Error> {
    // 1. Decode message
    // 2. Get `client_state` from host using `msg.client_id`
    // 3. Construct `new_client_state` and `new_consensus_state` (without verification)
    // 4. Write `new_client_state` and `new_consensus_state` to store
    // 5. Construct event and emit it
}

Some key observations

• Steps 1 & 2 from both validate() and execute() are the same.
• Step 3 from execute() (i.e. construct new_client_state and new_consensus_state without verification) is not
  possible with the current API.
• Note that events are constructed in both validate() and execute().
• execute() needs to decode the message, run stateful validations and construct the results before they can be
  written.

Takeaways

• Since steps 1 & 2 from both validate() and execute() are the same, it would make sense to extract them out. A
  closer look reveals that they're essentially the same two phases that I specified in my proposal, i.e. stateless and
  stateful validation (validate() and check()).

• Our idea of validation (stateless/ful) is distinct from what Namada calls validation (which is more like verification).

• Implementing deliver() as proposed, would mean forcing our users to decode the message and perform stateful
  validations twice.

  fn deliver<V, E>(val_ctx: &V, exec_ctx: &mut E, message: Any) -> Result<(), Error> {
      let _ = val_ctx.validate(message)?;
      exec_ctx.execute(message)
  }

  This can be avoided by organizing the code into smaller functions much like I previously proposed ->

  /// Entry points
  
  /// VP style
  fn validate(&self, msg: Any) -> Result<(), Error> {
      let msg = msg.try_into()?;
      let check_result = check(msg)?;
      let process_result = process(check_result)?;
      verify(process_result)
  }
  
  fn execute(&mut self, msg: Any) -> Result<(), Error> {
      let msg = msg.try_into()?;
      let check_result = check(msg)?;
      let process_result = process(msg)?;
      write(process_result)
  }
  
  /// Tendermint deliverTx style
  fn deliver<V, E>(val_ctx: &V, exec_ctx: &mut E, message: Any) -> Result<(), Error> {
      let msg = msg.try_into()?;
      let check_result = check(msg)?;
      let process_result = process(msg)?;
      verify(process_result)?;
      write(process_result)
  }

Addressing prior criticism

I don't think each phase should feed into one another. (...) I mean that they all take as input the transaction, and
either succeed or fail with an error.

Generally, designing the system in a way such that the output of a previous phase feeds into the current phase makes the
API easier to use correctly and difficult to use incorrectly.
If each of these functions accepted the raw message as input, we would end up performing validations multiple times for
the same message as demonstrated above. Furthermore, each of these phases relies on the output of the previous phase,
for e.g. to perform stateful validation we need a domain type message as that is what the API (ClientReader, etc.)
rightly expects, and to write the result, we need to execute the message and generate the result first.

I also don't see the value in separating "Message processing" from "state modification". Making writes generic (i.e.
using our "Keepers") handles that use case: we are implicitly telling chains to provide an implementation where writes
can be reverted until committed.

Separating process() from write() allows us to compose them. To me, process() is message execution, which
generates the result that must be written and events/logs. As demonstrated above, process() is required for
both verify() and write().

I don't see the benefit of this separation, because I see the abovementioned 3 phases as atomic. That is, you cannot
separate them further without breaking IBC.

I think the right way to think about this is in terms of modularity and (single) responsibility rather than atomicity.
I look at these functions as building blocks. Regardless of whether external users will want to use them or not,
organizing code this way allows us to build more resilient higher-level APIs and multiple versions of them (like VP or
deliverTx style APIs).
We could even have the low-level API in a separate crate (say ibc-handlers) with a semver than can stay unstable for a
long time and provide high-level entry point functions in the ibc crate that provides a stable API.

Conclusion

I still haven't touched on my other ideas including the high-level API and simplified state access as this post is
already quite long and I don't think there's an appetite for that, and it's easy to ignore details in such posts. 😉

All that said, I would still prefer the low-level API I initially proposed with the addition of a verify() phase,
based on the observations/justifications provided above, but happy to be proven wrong.

[plafer]

During today's sync call, @hu55a1n1 and I agreed on the following.

We will expose the API that I proposed (with modifications); that is, a ValidationContext and a ExecuteContext. We decided not to expose a lower-level API, as it will not be needed by our current customers. Keeping it private there has the following benefits:

• No need to document it with the same level of detail that a public API would require
• The library is simpler; there are fewer ways to use it incorrectly
• We can modify the lower-level API without requiring breaking changes
• We can always expose the lower-level API at a later date if we see a clear use case for it.
  • Note also that exposing it later is not API breaking.

However, fleshing out the private lower-level API is in scope for the ADR. The general ADR structure would look something like:

1. Context for the ADR
2. public API (ValidationContext and ExecutionContext)
3. private API

Customers which we run the ADR by can focus on (1) and (2) only, and skip the internal details which aren't relevant to them.

We will investigate further the Store idea, where most methods of the ValidationContext and ExecutionContext would have a default implementation which accesses this abstract Store. @hu55a1n1 has an idea on how to address the concern that the currently proposed Store forces users to serialize their objects. Note that some methods which don't use standard store paths, such as emit_ibc_event(), will need to be implemented by the user (no default implementation possible).

Async is out of scope for this ADR.

[hu55a1n1]

Host based API

Based on ICS24, we could require hosts to implement a Host trait as defined below. This trait would provide all
facilities that a ICS24 complaint host is expected to provide, namely, a key-value store, an event logging system,
timestamp access, etc.

pub trait Host {
    type Error: StoreError;
    type KvStore: IbcStore<Self::Error>;
    type EventEmitter: EventEmitter<Event=Event<DefaultIbcTypes>>;

    fn store(&self) -> &Self::KvStore;

    fn store_mut(&mut self) -> &mut Self::KvStore;

    fn event_emitter(&self) -> &Self::EventEmitter;

    fn event_emitter_mut(&mut self) -> &mut Self::EventEmitter;

    fn current_timestamp(&self) -> IbcTimestamp;

    fn current_height(&self) -> IbcHeight;

    /* ... */
    /* see section below on 'Host methods that cannot be obtained from the `Store`' for a complete list of methods */
}

This Host trait impl will be used to derive blanket impls for other contexts such as ValidationContext
, ExecutionContext, etc.

pub struct IbcHost<H> {
    host: H,
}

impl<H: Host> ValidationContext for IbcHost<H> { /* ... */ }

impl<H: Host> ExecutionContext for IbcHost<H> { /* ... */ }

(click to expand):

Host methods that cannot be obtained from the `Store`.

Here's an exhaustive list of methods whose implementation cannot be derived from the host's key-value Store and
therefore must be part of the Host trait. Note that some blockchains could opt to store much of this data on
their Store using predefined custom paths (this is the case with Namada).

pub trait ClientReader {
    fn decode_client_state(&self, client_state: Any) -> Result<Box<dyn ClientState>, Error>;
    fn host_height(&self) -> Height;
    fn host_timestamp(&self) -> Timestamp;
    fn host_consensus_state(&self, height: Height) -> Result<Box<dyn ConsensusState>, Error>;
    fn client_counter(&self) -> Result<u64, Error>;
}

pub trait ClientKeeper {
    fn increase_client_counter(&mut self);
    fn store_update_time(
        &mut self,
        client_id: ClientId,
        height: Height,
        timestamp: Timestamp,
    ) -> Result<(), Error>;
    fn store_update_height(
        &mut self,
        client_id: ClientId,
        height: Height,
        host_height: Height,
    ) -> Result<(), Error>;
}

pub trait ConnectionReader {
    fn host_oldest_height(&self) -> Height;
    fn commitment_prefix(&self) -> CommitmentPrefix;
    fn connection_counter(&self) -> Result<u64, Error>;
}

pub trait ConnectionKeeper {
    fn increase_connection_counter(&mut self);
}

pub trait ChannelReader {
    fn connection_channels(&self, cid: &ConnectionId) -> Result<Vec<(PortId, ChannelId)>, Error>;
    fn hash(&self, value: Vec<u8>) -> Vec<u8>;
    fn client_update_time(&self, client_id: &ClientId, height: Height) -> Result<Timestamp, Error>;
    fn client_update_height(&self, client_id: &ClientId, height: Height) -> Result<Height, Error>;
    fn channel_counter(&self) -> Result<u64, Error>;
    fn max_expected_time_per_block(&self) -> Duration;
    fn block_delay(&self, delay_period_time: Duration) -> u64;
}

pub trait ChannelKeeper {
    fn increase_channel_counter(&mut self);
}

---

Store

We define a generic TypedStore trait that must be implemented by the host system for each IBC path/value combination.

Note, depending on data-availability requirements some clients may need access to earlier state, so we might need
a get_at_height() in the future. For now a get_pre() method should suffice for Namada's execution model.

pub trait TypedStore<K, V> {
    type Error;

    fn set(&mut self, key: K, value: V) -> Result<(), Self::Error>;

    fn get(&self, key: K) -> Result<Option<V>, Self::Error>;

    /// Optional: only required for certain host types, for e.g. hosts with a validity predicate based execution model
    fn get_pre(&self, _key: K) -> Result<Option<V>, Self::Error> {
        unimplemented!()
    }

    fn delete(&mut self, key: K) -> Result<(), Self::Error>;
}

Internally, the library will only access the Store via an IbcStore -

pub trait IbcStore<Error>:
IbcTypedStore<ClientTypePath, Error>
+ IbcTypedStore<ClientStatePath, Error>
+ IbcTypedStore<ClientConsensusStatePath, Error>
/* + other IbcTypedStore */
{}

(click to expand):

Store implementation details

To be able to specify such trait-bounds, we must tie an IBC path to the value it is expected to hold at the type-level.
This can be done using a sealed IbcValueForPath trait as below and implementing it for all IBC paths -

pub trait IbcValueForPath: private::Sealed {
    type Value;
}

impl IbcValueForPath for ClientTypePath {
    type Value = IbcClientType;
}

impl IbcValueForPath for ClientStatePath {
    type Value = Box<dyn ClientState>;
}

impl IbcValueForPath for ClientConsensusStatePath {
    type Value = Box<dyn ConsensusState>;
}

This code can later be moved to a derive macro on the Paths themselves.

Once that's done, we define an IbcTypedStore and provide a blanket impl -

pub trait IbcTypedStore<Path, Error>:
TypedStore<Path, <Path as IbcValueForPath>::Value, Error=Error>
    where
        Path: IbcValueForPath,
{}

impl<Path, Value, Error, T> IbcTypedStore<Path, Error> for T
    where
        T: TypedStore<Path, Value, Error=Error>,
        Path: IbcValueForPath<Value=Value>,
{}

We could also provide a IbcSerde trait for better UX. This trait provides canonical serde for IBC values.

pub trait IbcSerde {
    fn serialize(self) -> Vec<u8>;

    fn deserialize(value: &[u8]) -> Self;
}

---

(click to expand):

Typical host store implementation

struct HostStore(BTreeMap<Path, Vec<u8>>);

impl<K, V> TypedStore<K, V> for HostStore
    where
        K: Into<Path> + IbcValueForPath<Value=V>,
        V: IbcSerde,
{
    type Error = Ics02Error;

    fn set(&mut self, key: K, value: V) -> Result<(), Self::Error> {
        let key = key.into();
        let value = <<K as IbcValueForPath>::Value as IbcSerde>::serialize(value);
        self.0.insert(key, value).map(|_| ()).unwrap();
        Ok(())
    }

    fn get(&self, key: K) -> Result<Option<V>, Self::Error> {
        let key = key.into();
        Ok(self
            .0
            .get(&key)
            .map(|bytes| <<K as IbcValueForPath>::Value as IbcSerde>::deserialize(bytes)))
    }

    fn delete(&mut self, key: K) -> Result<(), Self::Error> {
        let key = key.into();
        self.0.remove(&key).map(|_| ()).unwrap();
        Ok(())
    }
}

---

Event subsystem

IIUC, ICS24 requires hosts to store a succinct commitment to the event logs on chain/store. I think this can be
leveraged to implement the event validation in Namada, but we might need to discuss this with them. For now,
the EventEmitter interface described earlier should suffice.

Other ICS24 requirements

Other requirements including Consensus state introspection, Client state validation, Commitment path introspection,
Timestamp access, etc. should be covered by the methods listed in
the Host methods that cannot be obtained from the Store. section above.

[hu55a1n1]

Low level implementation

I think we can continue to use the low-level phase based functions described before and integrate them with the context
based high level traits and host based APIs.
The idea would be to require hosts to implement the Host trait, which will be used to generate the context traits (
e.g. ValidationContext, ExecutionContext, etc.). These contexts can then be used to generate blanket impls for the
low-level function context traits.

Context generic programming techniques

@soareschen was kind enough to implement a PoC framework to show how these techniques can be used with the current modules
code while still allowing it to deal with trait objects.
The code also demonstrates how we can move to a completely generic approach in the future (if that is desired) and can
be found here -> https://github.com/informalsystems/ibc-rs/tree/soares/ibc-next

An implementation of the update client handler using the same framework but integrated with the Host based API
described above can be found here -> https://github.com/informalsystems/ibc-rs/tree/hu55a1n1/adr-12-playground