Avoid code duplication in implementation of ValidationContext and ExecutionContext

[plafer]

Originally posted by @hu55a1n1 in #257 (comment)

I have been thinking about how we can avoid the code duplication we currently have and it seems roughly speaking each of these handlers are composed of the following steps -

validate:
* get relevant state from host
* check current state is as expected (based on message)
* construct expected state
* verify proofs

execute:
* get relevant state from host
* emit event/log
* construct new state
* write state

process:
* get relevant state from host
* check current state is as expected (based on message)
* construct expected state
* verify proofs
* emit event/log
* construct new state
* (write state later on in store_connection_result)

So there is some degree of overlap between them. Here's a potential way to avoid the duplication by defining a separate 'handler' trait and providing blanket impls for types that implement the ValidationContext and ConnectionReader ->

trait ConnOpenConfirmHandler {
    fn verify(&self, msg: &MsgConnectionOpenConfirm) -> Result<(), Error> {
        let conn_end_on_b = self.connection_end(&msg.conn_id_on_b)?;
        if !conn_end_on_b.state_matches(&State::TryOpen) {
            return Err(Error::connection_mismatch(msg.conn_id_on_b.clone()));
        }

        let client_id_on_a = conn_end_on_b.counterparty().client_id();
        let client_id_on_b = conn_end_on_b.client_id();
        let conn_id_on_a = conn_end_on_b
            .counterparty()
            .connection_id()
            .ok_or_else(Error::invalid_counterparty)?;

        // Verify proofs
        {
            let client_state_of_a_on_b = self
                .client_state(client_id_on_b)
                .map_err(|_| Error::other("failed to fetch client state".to_string()))?;
            let consensus_state_of_a_on_b = self
                .consensus_state(client_id_on_b, msg.proof_height_on_a)
                .map_err(|_| Error::other("failed to fetch client consensus state".to_string()))?;

            let prefix_on_a = conn_end_on_b.counterparty().prefix();
            let prefix_on_b = self.commitment_prefix();

            let expected_conn_end_on_a = ConnectionEnd::new(
                State::Open,
                client_id_on_a.clone(),
                Counterparty::new(
                    client_id_on_b.clone(),
                    Some(msg.conn_id_on_b.clone()),
                    prefix_on_b,
                ),
                conn_end_on_b.versions().to_vec(),
                conn_end_on_b.delay_period(),
            );

            client_state_of_a_on_b
                .verify_connection_state(
                    msg.proof_height_on_a,
                    prefix_on_a,
                    &msg.proof_conn_end_on_a,
                    consensus_state_of_a_on_b.root(),
                    conn_id_on_a,
                    &expected_conn_end_on_a,
                )
                .map_err(Error::verify_connection_state)?;
        }

        Ok(())
    }

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

    fn connection_end(&self, conn_id: &ConnectionId) -> Result<ConnectionEnd, Error>;

    fn consensus_state(
        &self,
        client_id: &ClientId,
        height: Height,
    ) -> Result<Box<dyn ConsensusState>, Error>;

    fn commitment_prefix(&self) -> CommitmentPrefix;
}

struct ValidationCtx<'a, T>(&'a T);

impl<T> ConnOpenConfirmHandler for ValidationCtx<'_, T> where T: ValidationContext {
    fn client_state(&self, client_id: &ClientId) -> Result<Box<dyn ClientState>, Error> {
        self.0.client_state(client_id)
            .map_err(|_| Error::other("failed to fetch client state".to_string()))
    }

    fn connection_end(&self, conn_id: &ConnectionId) -> Result<ConnectionEnd, Error> {
        self.0.connection_end(conn_id)
    }

    fn consensus_state(&self, client_id: &ClientId, height: Height) -> Result<Box<dyn ConsensusState>, Error> {
        self.0.consensus_state(client_id, height)
            .map_err(|_| Error::other("failed to fetch consensus state".to_string()))
    }

    fn commitment_prefix(&self) -> CommitmentPrefix {
        self.0.commitment_prefix()
    }
}

struct Ics26Ctx<'a, T>(&'a T);

impl<T> ConnOpenConfirmHandler for Ics26Ctx<'_, T> where T: ConnectionReader {
    fn client_state(&self, client_id: &ClientId) -> Result<Box<dyn ClientState>, Error> {
        self.0.client_state(client_id)
    }

    fn connection_end(&self, conn_id: &ConnectionId) -> Result<ConnectionEnd, Error> {
        self.0.connection_end(conn_id)
    }

    fn consensus_state(&self, client_id: &ClientId, height: Height) -> Result<Box<dyn ConsensusState>, Error> {
        self.0.client_consensus_state(client_id, height)
    }

    fn commitment_prefix(&self) -> CommitmentPrefix {
        self.0.commitment_prefix()
    }
}

[plafer]

I based myself off of this to come up with the following, which I believe addresses all our problems:

1. No duplication of variable definition code between validate() and execute()
2. No duplication of logic between validate()/execute() and process()
3. process() is as efficient as before; no additional runtime cost was added

Note that I don't worry about ConnectionReader because it's going away.

If you like it, I can implement this directly in #257.

/// Abstracts all common variables between `validate()` and `execute()`
struct LocalVars {
    conn_end_on_b: ConnectionEnd,
}

impl LocalVars {
    fn new<Ctx>(ctx_b: &Ctx, msg: &MsgConnectionOpenConfirm) -> Result<Self, Error>
    where
        Ctx: ValidationContext,
    {
        Ok(Self {
            conn_end_on_b: ctx_b.connection_end(&msg.conn_id_on_b)?,
        })
    }

    fn conn_end_on_b(&self) -> &ConnectionEnd {
        &self.conn_end_on_b
    }

    fn client_id_on_a(&self) -> &ClientId {
        self.conn_end_on_b.counterparty().client_id()
    }

    fn client_id_on_b(&self) -> &ClientId {
        self.conn_end_on_b.client_id()
    }

    fn conn_id_on_a(&self) -> Result<&ConnectionId, Error> {
        self.conn_end_on_b
            .counterparty()
            .connection_id()
            .ok_or_else(Error::invalid_counterparty)
    }
}

pub(crate) fn validate<Ctx>(ctx_b: &Ctx, msg: MsgConnectionOpenConfirm) -> Result<(), Error>
where
    Ctx: ValidationContext,
{
    let vars = LocalVars::new(ctx_b, &msg)?;
    validate_impl(ctx_b, msg, &vars)
}

fn validate_impl<Ctx>(
    ctx_b: &Ctx,
    msg: MsgConnectionOpenConfirm,
    vars: &LocalVars,
) -> Result<(), Error>
where
    Ctx: ValidationContext,
{
    let conn_end_on_b = vars.conn_end_on_b();
    if !conn_end_on_b.state_matches(&State::TryOpen) {
        return Err(Error::connection_mismatch(msg.conn_id_on_b));
    }

    let client_id_on_a = vars.client_id_on_a();
    let client_id_on_b = vars.client_id_on_b();
    let conn_id_on_a = vars.conn_id_on_a()?;

    // Verify proofs
    {
        let client_state_of_a_on_b = ctx_b
            .client_state(client_id_on_b)
            .map_err(|_| Error::other("failed to fetch client state".to_string()))?;
        let consensus_state_of_a_on_b = ctx_b
            .consensus_state(client_id_on_b, msg.proof_height_on_a)
            .map_err(|_| Error::other("failed to fetch client consensus state".to_string()))?;

        let prefix_on_a = conn_end_on_b.counterparty().prefix();
        let prefix_on_b = ctx_b.commitment_prefix();

        let expected_conn_end_on_a = ConnectionEnd::new(
            State::Open,
            client_id_on_a.clone(),
            Counterparty::new(
                client_id_on_b.clone(),
                Some(msg.conn_id_on_b.clone()),
                prefix_on_b,
            ),
            conn_end_on_b.versions().to_vec(),
            conn_end_on_b.delay_period(),
        );

        client_state_of_a_on_b
            .verify_connection_state(
                msg.proof_height_on_a,
                prefix_on_a,
                &msg.proof_conn_end_on_a,
                consensus_state_of_a_on_b.root(),
                conn_id_on_a,
                &expected_conn_end_on_a,
            )
            .map_err(Error::verify_connection_state)?;
    }

    Ok(())
}

pub(crate) fn execute<Ctx>(ctx_b: &mut Ctx, msg: MsgConnectionOpenConfirm) -> Result<(), Error>
where
    Ctx: ExecutionContext,
{
    let vars = LocalVars::new(ctx_b, &msg)?;
    execute_impl(ctx_b, msg, vars)
}

fn execute_impl<Ctx>(
    ctx_b: &mut Ctx,
    msg: MsgConnectionOpenConfirm,
    vars: LocalVars,
) -> Result<(), Error>
where
    Ctx: ExecutionContext,
{
    let client_id_on_a = vars.client_id_on_a();
    let client_id_on_b = vars.client_id_on_b();
    let conn_id_on_a = vars.conn_id_on_a()?;

    ctx_b.emit_ibc_event(IbcEvent::OpenConfirmConnection(OpenConfirm::new(
        msg.conn_id_on_b.clone(),
        client_id_on_b.clone(),
        conn_id_on_a.clone(),
        client_id_on_a.clone(),
    )));
    ctx_b.log_message("success: conn_open_confirm verification passed".to_string());

    {
        let new_conn_end_on_b = {
            let mut new_conn_end_on_b = vars.conn_end_on_b;

            new_conn_end_on_b.set_state(State::Open);
            new_conn_end_on_b
        };

        ctx_b.store_connection(ConnectionsPath(msg.conn_id_on_b), &new_conn_end_on_b)?;
    }

    Ok(())
}

pub(crate) fn process<Ctx>(ctx_b: &mut Ctx, msg: MsgConnectionOpenConfirm) -> Result<(), Error>
where
    Ctx: ExecutionContext,
{
    // Note  that this is a "zero-cost" refactor, since common variables are only built
    // once in `LocalVars` 
    let vars = LocalVars::new(ctx_b, &msg)?;
    // we can get rid of clone when we fix our `Error` structs
    validate_impl(ctx_b, msg.clone(), &vars)?;
    execute_impl(ctx_b, msg, vars)
}

[hu55a1n1]

I like the idea to use LocalVars! But I see that the process() requires the Ctx: ExecutionContext bound now and not the reader/keeper traits, so is the plan to do away with them? I fear removing the reader/keeper traits might break the Ics20 app impl.

[plafer]

Right; I was thinking we can keep process() that need the old keepers as copy/paste for now, and use a different name (handle()?) for the one that needs an ExecutionContext.

[hu55a1n1]

I think this makes sense as an intermediary solution. 👍 We can remove the readers/keepers once we migrate the apps to use the new API.