Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create custom rollkit ClientState type #22

Merged

Conversation

crodriguezvega
Copy link

@crodriguezvega crodriguezvega commented Apr 19, 2024

No description provided.

@rnbguy
Copy link
Collaborator

rnbguy commented May 6, 2024

Hey, cosmos/ibc-rs#1204 relaxes the TryFrom<Error = ClientError> restriction. If you are using the latest branch from ibc-rs, you can just do this

use ibc_client_cw::api::ClientType;
use ibc_client_tendermint::consensus_state::ConsensusState as TendermintConsensusState;

use crate::client_state::ClientState as RollkitClientState;

pub struct RollkitClient;

impl<'a> ClientType<'a> for RollkitClient {
    type ClientState = RollkitClientState;
    type ConsensusState = TendermintConsensusState;
}

You can refer to the current implementation of the Tendermint CW client.

PS: We reversed the direction of Convertible and dropped Error generic. So,

V::ConsensusStateRef: Convertible<TendermintConsensusStateType, ClientError>,

becomes

TendermintConsensusStateType: Convertible<V::ConsensusStateRef>

Sometimes you may need to add

ClientError: From<<TendermintConsensusStateType as TryFrom<V::ConsensusStateRef>>::Error>

for ClientError conversion.

@crodriguezvega
Copy link
Author

crodriguezvega commented May 10, 2024

Hey, cosmos/ibc-rs#1204 relaxes the TryFrom<Error = ClientError> restriction. If you are using the latest branch from ibc-rs, you can just do this

use ibc_client_cw::api::ClientType;
use ibc_client_tendermint::consensus_state::ConsensusState as TendermintConsensusState;

use crate::client_state::ClientState as RollkitClientState;

pub struct RollkitClient;

impl<'a> ClientType<'a> for RollkitClient {
    type ClientState = RollkitClientState;
    type ConsensusState = TendermintConsensusState;
}

You can refer to the current implementation of the Tendermint CW client.

PS: We reversed the direction of Convertible and dropped Error generic. So,

V::ConsensusStateRef: Convertible<TendermintConsensusStateType, ClientError>,

becomes

TendermintConsensusStateType: Convertible<V::ConsensusStateRef>

Sometimes you may need to add

ClientError: From<<TendermintConsensusStateType as TryFrom<V::ConsensusStateRef>>::Error>

for ClientError conversion.

Thank you for the heads up, @rnbguy. I have updated to the tip of ibc-rs and implemented the suggested changes.

Comment on lines 92 to 97
let tendermint_consensus_state: TendermintConsensusStateType = consensus_state
.clone()
.try_into()
.map_err(|_: ClientError| ClientError::UnknownConsensusStateType {
consensus_state_type: consensus_state.type_url,
})?;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rnbguy Would you happen to know if this can be simplified? Doing just consensus_state.try_into() was giving me errors about the ClientError, and this is what I came up with to work around them...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting find. Looks like the Rust compiler is getting confused between the implementations for TryFrom<Any> and TryFrom<E::ConsensusStateRef>.

For this particular code, you can remove this trait bound, and that allows .try_into()?.

If we really needed that trait bound, you could have done:

let tendermint_consensus_state =
  <TendermintConsensusStateType as TryFrom<Any>>::try_from(consensus_state.clone())?;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit for this 31babdb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, @rnbguy! I will try the removal of the trait bound for now. :)

@crodriguezvega
Copy link
Author

This looks good to me for now. No tests (yet), but we'll figure out how to best do that later on. Merging now...

@crodriguezvega crodriguezvega merged commit b9e09d7 into feat/rollkit May 13, 2024
5 checks passed
@crodriguezvega crodriguezvega deleted the carlos/17-create-custom-rollkit-clientstate-type branch May 13, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants