Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Refactor: Decouple *Client types from Any* types #60

Merged
merged 28 commits into from
Sep 16, 2022
Merged

Conversation

vmarkushin
Copy link

@vmarkushin vmarkushin commented Sep 7, 2022

The basic reason for this refactor is to be able to separate different IBC clients (Beefy, Near, etc.) from the ibc-rs crate itself. This wasn't possible due to the usage of Any* (AnyClient, AnyConsensusState, etc.) types highly coupled to the particular client implementation, so in order to do this one would need to abstract the clients on some global definition of the Any* types. This is done by introducing AnyHeader, AnyClientState, etc. types in the ClientKeeper trait and passing it as a generic Ctx to most of the IBC and clients' functions.

@vmarkushin vmarkushin added the enhancement New feature or request label Sep 7, 2022
@vmarkushin vmarkushin self-assigned this Sep 7, 2022
@vmarkushin vmarkushin marked this pull request as ready for review September 9, 2022 08:28
@vmarkushin vmarkushin changed the title Decouple *Client types from Any* types Refactor: Decouple *Client types from Any* types Sep 9, 2022
// Upgrade the client state
self.latest_height = upgrade_height;
self.unbonding_period = upgrade_options.unbonding_period;
self.chain_id = chain_id;

Choose a reason for hiding this comment

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

does chain id change every time?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know. @Wizdave97 ?

Choose a reason for hiding this comment

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

Yeah it could, if the chain updates it's consensus mechanism or has a fork maybe.

@blasrodri
Copy link

Can you provide a high level description of the refactor? @vmarkushin

@seunlanlege
Copy link

Before i review vlad, can you delete the modules/src/clients folder? Since they'll be maintained outside of this repo

modules/src/core/ics02_client/client_state.rs Outdated Show resolved Hide resolved
modules/src/core/ics02_client/msgs/update_client.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link

The final step for this pr will be to remove the Any* types and introduce the macro we talked about @vmarkushin


fn update_state_on_misbehaviour(
&self,
client_state: Self::ClientState,
header: Self::Header,
) -> Result<Self::ClientState, Error>;

fn check_for_misbehaviour(
fn check_for_misbehaviour<Ctx: ReaderContext>(

Choose a reason for hiding this comment

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

This function is meant to be used when handling misbehaviour messages, we should refactor it to accept take header_1 && header_2, not just one header.

Choose a reason for hiding this comment

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

don't think this necessary, because of how 02-client uses it:

let found_misbehaviour = client_def
.check_for_misbehaviour(ctx, client_id.clone(), client_state.clone(), header.clone())
.map_err(|e| Error::header_verification_failure(e.to_string()))?;
let event_attributes = Attributes {
client_id: client_id.clone(),
height: ctx.host_height(),
client_type,
consensus_height: client_state.latest_height(),
};
if found_misbehaviour {
let client_state = client_def.update_state_on_misbehaviour(client_state, header)?;
let result = ClientResult::Update(Result {
client_id,
client_state,
consensus_state: None,
processed_time: ctx.host_timestamp(),
processed_height: ctx.host_height(),
});
output.emit(IbcEvent::ClientMisbehaviour(event_attributes.into()));
return Ok(output.with_result(result));
}

Choose a reason for hiding this comment

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

Yeah, it's not.

Choose a reason for hiding this comment

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

Copy link

@seunlanlege seunlanlege Sep 16, 2022

Choose a reason for hiding this comment

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

so it means it shouldn't be ClientDef::verify_header but instead, it should be ClientDef::verify_client_message

https://github.com/cosmos/ibc-go/blob/80ea89a356da6606b351f0e00fc82786a5c13fad/modules/light-clients/07-tendermint/update.go#L21-L33

Choose a reason for hiding this comment

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

@Wizdave97 can you make the necessary refactor on this PR?

@seunlanlege seunlanlege merged commit af5bb83 into master Sep 16, 2022
@seunlanlege seunlanlege deleted the trait-refactor branch September 16, 2022 11:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

4 participants