Proposal for redesigning parts of the ibc-rs modules' API

[hu55a1n1]

Crate

ibc

Proposal

Following is a list of ideas for redesigning parts of our ibc-rs modules' API accompanied with some reasoning for why these changes are required. (Some of these have already been implemented in PR informalsystems/hermes#1583.) This is meant to serve as a meta issue for tracking these design changes. Might make sense to defer this until we start implementing another client (i.e. substrate, etc.) ->

• [x] Convert the Proofs struct into an enum. (See https://github.com/informalsystems/ibc-rs/pull/1583#discussion_r766923695)

  
  // change this ->
  pub struct Proofs {
  object_proof: CommitmentProofBytes,
  client_proof: Option<CommitmentProofBytes>,
  consensus_proof: Option<ConsensusProof>,
  other_proof: Option<CommitmentProofBytes>,
  height: Height,
  }

// to something like this -> pub enum Proofs { ChannelOpenTry(ChannelOpenTryProof), // ... }

// or maybe this -> pub struct SimpleProof(CommitmentProofBytes); pub struct ComplexProof { pub proof: CommitmentProofBytes, pub consensus_proof: ConsensusProof, // ... }

pub enum Proofs { ConnectionOpenTry(ComplexProof), ChannelOpenTry(SimpleProof), // ... }

- [x] `ics24_host::Path` variants as types - Since Rust doesn't treat enum variants as types, it might be a good idea to extract these variants as distinct types and use those types as the actual enum variants. This will allow us to enforce that certain functions accept only a certain type of path. eg. `verify_client_consensus_state()` should only accept a `Path::ClientConsensusState`. (https://github.com/informalsystems/ibc-rs/pull/1760)
- [x] Follow [RFC-356-no-module-prefixes](https://github.com/rust-lang/rfcs/blob/master/text/0356-no-module-prefixes.md). This basically means we rename `ibc::core::ics04_channel::msgs::MsgChannelCloseConfirm` to `ibc::core::ics04_channel::msgs::ChannelCloseConfirm`, and either re-export it as `MsgChannelCloseConfirm` in `msgs.rs` or add it to the prelude.
- [ ] Public `struct`s should either have public fields or setters/getters (if required), but not both. e.g.
```rust
pub struct MsgChannelCloseInit {
    pub port_id: PortId, // <- public
    // ...
}

impl MsgChannelCloseInit {
    pub fn port_id(&self) -> &PortId {
        &self.port_id
    } // <- also has a getter
}

• [ ] Add ParamsReader trait to allow users to get host genesis params like MaxExpectedTimePerBlock. Prefer composition over inheritance (where possible) ->

  fn handler<CR: ConnectionReader, PR: ParamsReader>(
  connection_reader: CR,
  params_reader: PR,
  ...
  ) {}

• [ ] Remove &dyn in trait methods.
• [ ] Errors returned by *Reader trait methods must match API. e.g. ChannelReader::client_state() is usually just a proxy to ClientReader::client_state(), so it doesn't make sense for it to return a ChannelError ->

  pub trait ChannelReader {
  fn client_state(&self, client_id: &ClientId) -> Result<AnyClientState, ics04_channel::error::Error>;
  // ...
  }

• [x] Super-traits - There is significant overlap in the various *Reader trait methods, so it might make sense to use inheritance here ->

  pub trait ClientReader {}
  pub trait ConnectionReader: ClientReader {}
  pub trait ChannelReader: ConnectionReader {}

• [ ] Provide verification functions with everything they need and don't give them access to the *Reader contexts. (this may not be feasible due to methods like check_header_and_update_state() requiring access to ctx.next_consensus_state()) The other extreme is to only give verification functions such contexts and allow them to extract everything they need them.

• [x] ChannelReader::hash() accepts input as (and returns) a String.

  pub trait ChannelReader {
  fn hash(&self, value: String) -> String;
  }

  The method should ideally take a Vec<u8> as input and return a Digest type that is fixed per client. So maybe something like ->

  pub trait ChannelReader {
  type Digest: AsRef<&[u8]>;
  
  fn hash(&self, value: Vec<u8>) -> Self::Digest;
  }

  Or maybe this should be part of ClientReader?

• [x] Disallow empty CommitmentPrefix and CommitmentProofBytes, i.e. empty values for these types should not be constructible. (https://github.com/informalsystems/ibc-rs/pull/1761)
• [x] informalsystems/ibc-rs-tmp#58
• [x] Remove all unwrap()s.
• [x] Init ClientState metadata (https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/07-tendermint/types/client_state.go#L187). (https://github.com/informalsystems/ibc-rs/pull/1763)
• [ ] Pass ChannelId by value since it impls Copy. https://github.com/informalsystems/ibc-rs/pull/2071#discussion_r845063824

PS: Thanks to @romac for his valuable insights and suggestions!

---

For Admin Use

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

[mzabaluev]

pub enum Proofs {
    ChannelOpenTry(ChannelOpenTryProof), 
    // ...
}

Bikeshedding: Does this type mean "all proofs supplied with (necessary for?) this request"? The variant names suggest that each case can be thought of a singular "proof", and in this case the enum should probably be named Proof.

// or maybe this -> 
pub struct SimpleProof(CommitmentProofBytes);
pub struct ComplexProof {
    pub proof: CommitmentProofBytes, 
    pub consensus_proof: ConsensusProof,
    // ...
}

pub enum Proofs {
    ConnectionOpenTry(ComplexProof), 
    ChannelOpenTry(SimpleProof), 
    // ...
}

Could be, but then a convenience accessor may be needed to get at the commitment proof in all variants where it's available:

impl Proofs {
    pub fn commitment(&self) -> Option<&CommitmentProofBytes> {
        todo!()
    }
}

If there may be benefit in type-checking the proof variants in particular places where only such a variant is meaningful, they each should get their own dedicated type, to be used instead of matching a dynamically multiplexed Proofs value and asserting. This is similar to @hu55a1n1's suggestion for ics24_host::Path

[Farhad-Shabani]

For the remained tasks in this ticket, issues that still make sense are opened (#532, #534, #537). This is done as a pre-work before preparing the "Road to IBC-rs V1". @plafer Can you please recheck the list to make sure I didn't miss anything before closing the issue?

[plafer]

LGTM