ADR 11: Light client extraction

[hu55a1n1]

Description

An ADR for light client extraction by removal of Any* enums and replacing them with trait objects.

Rendered

---

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).

[seanchen1991]

Left some nits.

[hu55a1n1]

A cool crate that might be helpful in implementing such a registry -> https://github.com/dtolnay/inventory. (Thanks to @romac for suggesting it).

[plafer]

Instead of using Option<Any> for header, why not implement Deserialize ourselves, and apply the transformation encode_any() in deserialize()?

serde might also even provide an attribute that we could tag header with that specifies to apply this transformation before deserializing. Not sure though.

[hu55a1n1]

I think the problem with that approach is that the deserialize impl would depend on Header::decode() which must have a Self: Sized requirement. This is because we must be aware of the concrete type of the header at the time of deserialization but that's not possible.

[hu55a1n1]

Seems like there are some ready-made solutions like erased_serde but I am not sure if they can work for our specific use case.

[hu55a1n1]

Here's my quick attempt at implementing this using the #[serde(with = "/* ... */")] attribute -> https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=de9aa123fcec49b410fec8bb23d5f53e

[plafer]

This is because we must be aware of the concrete type of the header at the time of deserialization but that's not possible.

This is a good point. To address this issue, we could modify the IntoRef trait suggested here to inject some context into the conversion:

trait IntoRef<T: ?Sized> { 
    // Same applies to `try_into_ref()`
    fn into_ref(&self, ctx: &ConversionContext) -> Box<T>;
}

ConversionContext (or any context more appropriate) would be implemented by the chain (along with all other Readers and Keepers). It would look something like:

trait ConversionContext {
    fn decode_header(&self, raw_header: &Any) -> Result<Box<dyn Header>, Error>;
    fn decode_client_state(&self, raw_cs: &Any) -> Result<Box<dyn ClientState>, Error>;
    fn decode_consensus_state(&self, raw_cs: &Any) -> Result<Box<dyn ConsensusState>, Error>;
    ...
}

Note that we could facilitate the implementation of this trait with the light client registry crate proposed in this ADR (for all known light clients known to the registry).

However, the downside of this approach then is that the ibc library doesn't know how to convert Raw -> Domain for types who use, for example, dyn Header. This is because by design, we want the chains to tell us how to decode the Header (and ClientState, etc) for the light clients that it knows about.

We then need to introduce an additional trait TryFromContext:

trait TryFromContext<T> {
    type Error;
    
    fn try_from_ctx(value: T, ctx: ConversionContext) -> Result<Self, Self::Error>;
}

which would be used with, say, MsgUpdateClient:

struct MsgUpdateClient {
    pub client_id: ClientId,
    pub header: Box<dyn Header>,
    pub signer: Signer,
}

impl TryFromContext<RawMsgUpdateClient> for MsgUpdateClient {
    type Error = Error;

    // We need to use `TryFromContext` instead of `TryFrom` so that 
    // the `ConversionContext` can be used to decode the raw `Header`
    fn try_from_ctx(raw: RawMsgUpdateClient, ctx: &ConversionContext) -> Result<Self, Self::Error> {
        Ok(MsgUpdateClient {
            client_id: ...,
            header: ctx.decode_header(&raw)?,
            signer: ...,
        })
    }

Ultimately, I still prefer this solution to using the raw Any for Header et al, as it allows us to have proper domain types (where all fields are also domain types), at the cost of more complexity in the conversion code.

WDYT?

[romac]

I believe we should indeed aim for a better typed solution here rather than rely on Any types everywhere. That said, if going with Any for the moment makes it easier to move forward rapidly with this, we can delay the properly-typed solution to a later stage.

I would still at least try to gauge if @plafer's proposal above works and if so how much more complex it would make the code before going with either solution.

[hu55a1n1]

I like the idea of grouping the decoding functions into a ConversionContext and using that in a custom TryFromContext impl. 👌 But I don't think it solves our deserialization problem because we would still need access to the ConversionContext impl in the deserialize impl. Additionally, TryFromContext isn't object safe. Looking into how erased_serde works (as Romain suggested).

[hu55a1n1]

erased_serde uses a cool trick explained here -> https://github.com/dtolnay/erased-serde/blob/master/explanation/main.rs. It allows one to have a separate object safe version of a trait that can operate with the original (non object safe version) seamlessly.
In our case, I don't think we need to support interoperability with the tendermint_proto::Protobuf trait so we can define our own trait that is object safe and just use that instead. We can still use erased_serde's blanket impl trick to keep the original tendermint_proto::Protobuf API on the implementation side.
See experimental PR where Protobuf is object safe and requires no other changes on the implementation side -> https://github.com/informalsystems/ibc-rs/pull/2412/files#diff-2b95cbfaf034ca5aa2b9a3f02436e8bd7a7a937df7c5ffb6dba81a0237141941R70 =>

mod erased {
  use core::convert::{Into as CoreInto, TryFrom as CoreTryFrom};
  
  mod sealed {
      use super::*;
  
      pub trait SealedInto<T: ?Sized> {}
      impl<T, U: Clone + CoreInto<T>> SealedInto<T> for U {}
      pub trait SealedTryFrom<T> {}
      impl<T, U: CoreTryFrom<T>> SealedTryFrom<T> for U {}
  }
  
  pub trait Into<T: ?Sized>: sealed::SealedInto<T> {
      fn into(&self) -> Box<T>;
  }
  
  impl<T, U: Clone + CoreInto<T>> Into<T> for U {
      fn into(&self) -> Box<T> {
          Box::new(self.clone().into())
      }
  }
  
  pub trait TryFrom<T>: sealed::SealedTryFrom<T> {
      type Error;
  
      fn try_from(t: T) -> Result<Self, Self::Error>
      where
          Self: Sized;
  }
  
  impl<T, U: CoreTryFrom<T>> TryFrom<T> for U {
      type Error = <Self as CoreTryFrom<T>>::Error;
  
      fn try_from(t: T) -> Result<Self, Self::Error>
      where
          Self: Sized,
      {
          <Self as CoreTryFrom<T>>::try_from(t)
      }
  }
}

pub trait Protobuf<T: Message + Default>
where
    Self: erased::TryFrom<T> + erased::Into<T>,
    <Self as erased::TryFrom<T>>::Error: Display,
{ /* ... */ }

[hu55a1n1]

TryFromContext isn't object safe because the trait function is static (and doesn't have a self parameter) and it returns Self. This is the same problem we faced with the From trait which we solved using the IntoRef solution, but that doesn't work here with TryFromContext, that's because an equivalent TryIntoContext trait bound would look like T: TryIntoContext<Self> and that isn't allowed due object safety restrictions.

Furthermore, to be able to decode using TryFromContext we would need to specify the additional context at the point of decoding which may or may not be accessible there, for e.g. Rust's serde deserialization is stateless and so it is not straightforward to access host context at the time of deserialization.

For these reasons, I believe it is better to not use the TryFromContext trait and instead stick to the TryFrom trait with a where Self: Sized bound.

[romac]

Amazing work, @hu55a1n1 and @plafer! 💐

The restriction introduced under Domain types containing light client specific types is unfortunate but does seem indeed like the best way forward for now.

[plafer]

Why would we ever need to downcast to a specific concrete type? Isn't the interface provided by, say, Header enough?

[hu55a1n1]

I believe this will be required (for example) in cases where the light client wants to access its state stored on the host - the host is oblivious to the light client state's concrete type so it would return a trait object (such as &dyn ConsensusState) and the light client would need to downcast it to the concrete light client type (e.g. TmConsensusState) before it can use light client specific fields from it (such as next_validators_hash).

[hu55a1n1]

Already merged via #2423 and impld in #2483.