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

• Convert the Proofs struct into an enum. (See Implement proof verification functions for Tendermint client informalsystems/hermes#1583 (comment))

 // 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), 
    // ...
}

• 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. (ICS24 Path variants as types informalsystems/hermes#1760)
• Follow RFC-356-no-module-prefixes. 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 structs should either have public fields or setters/getters (if required), but not both. e.g.

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>;
    // ...
}

• 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.
• 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?

• Disallow empty CommitmentPrefix and CommitmentProofBytes, i.e. empty values for these types should not be constructible. (Disallow empty CommitmentPrefix and CommitmentProofBytes informalsystems/hermes#1761)
• Implement Capability{Reader,Keeper} traits #58
• Remove all unwrap()s.
• Init ClientState metadata (https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/07-tendermint/types/client_state.go#L187). (Initialize consensus metadata during client create informalsystems/hermes#1763)
• Pass ChannelId by value since it impls Copy. Model ChannelId as newtype u64 and improve validation informalsystems/hermes#2071 (comment)

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