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

Implement ClientState::status() #774

Merged
merged 18 commits into from
Aug 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- [ibc-derive] Replace `ClientState::{confirm_not_frozen, expired}()` with `ClientState::status()`
([#536](https://github.com/cosmos/ibc-rs/issues/536))
22 changes: 0 additions & 22 deletions crates/ibc-derive/src/client_state/traits/client_state_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,6 @@ pub(crate) fn impl_ClientStateCommon(
enum_variants.iter(),
quote! {validate_proof_height(cs, proof_height)},
);
let confirm_not_frozen_impl = delegate_call_in_match(
client_state_enum_name,
enum_variants.iter(),
quote! {confirm_not_frozen(cs)},
);
let expired_impl = delegate_call_in_match(
client_state_enum_name,
enum_variants.iter(),
quote! {expired(cs, elapsed)},
);
let verify_upgrade_client_impl = delegate_call_in_match(
client_state_enum_name,
enum_variants.iter(),
Expand Down Expand Up @@ -95,18 +85,6 @@ pub(crate) fn impl_ClientStateCommon(
}
}

fn confirm_not_frozen(&self) -> core::result::Result<(), #ClientError> {
match self {
#(#confirm_not_frozen_impl),*
}
}

fn expired(&self, elapsed: core::time::Duration) -> bool {
match self {
#(#expired_impl),*
}
}

fn verify_upgrade_client(
&self,
upgraded_client_state: #Any,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,21 @@ pub(crate) fn impl_ClientStateValidation(
quote! { check_for_misbehaviour(cs, ctx, client_id, client_message, update_kind) },
);

let status_impl = delegate_call_in_match(
client_state_enum_name,
enum_variants.iter(),
opts,
quote! { status(cs, ctx, client_id) },
);

let HostClientState = client_state_enum_name;
let ClientValidationContext = &opts.client_validation_context;

let Any = Imports::Any();
let ClientId = Imports::ClientId();
let ClientError = Imports::ClientError();
let ClientStateValidation = Imports::ClientStateValidation();
let Status = Imports::Status();
let UpdateKind = Imports::UpdateKind();

quote! {
Expand Down Expand Up @@ -64,6 +72,17 @@ pub(crate) fn impl_ClientStateValidation(
#(#check_for_misbehaviour_impl),*
}
}

fn status(
&self,
ctx: &#ClientValidationContext,
client_id: &#ClientId,
) -> core::result::Result<#Status, #ClientError> {
match self {
#(#status_impl),*
}

}
}

}
Expand Down
4 changes: 4 additions & 0 deletions crates/ibc-derive/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ impl Imports {
pub fn UpdateKind() -> TokenStream {
quote! {ibc::core::ics02_client::client_state::UpdateKind}
}

pub fn Status() -> TokenStream {
quote! {ibc::core::ics02_client::client_state::Status}
}
}

/// Retrieves the field of a given enum variant. Outputs an error message if the enum variant
Expand Down
2 changes: 1 addition & 1 deletion crates/ibc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ scale-info = { version = "2.1.2", default-features = false, features = ["derive"
borsh = {version = "0.9.0", default-features = false, optional = true }
parking_lot = { version = "0.12.1", default-features = false, optional = true }

ibc-derive = "0.2.0"
ibc-derive = { version ="0.2.0", path = "../ibc-derive" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I had to reincorporate the path because otherwise whenever we make changes to ibc-derive, ibc doesn't see the changes, and it fails to compile

Copy link
Member

Choose a reason for hiding this comment

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

Therefore, good to add a [ibc-drive] tag for unclog. Like how this handled e.g. in tendermint-rs

Copy link
Member

Choose a reason for hiding this comment

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

Was thinking of adding this to the contributing.md. I see we already have it here


schemars = { version = "0.8.12", optional = true }

Expand Down
59 changes: 45 additions & 14 deletions crates/ibc/src/clients/ics07_tendermint/client_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::clients::ics07_tendermint::error::Error;
use crate::clients::ics07_tendermint::header::Header as TmHeader;
use crate::clients::ics07_tendermint::misbehaviour::Misbehaviour as TmMisbehaviour;
use crate::core::ics02_client::client_state::{
ClientStateCommon, ClientStateExecution, ClientStateValidation, UpdateKind,
ClientStateCommon, ClientStateExecution, ClientStateValidation, Status, UpdateKind,
};
use crate::core::ics02_client::client_type::ClientType;
use crate::core::ics02_client::consensus_state::ConsensusState;
Expand Down Expand Up @@ -248,6 +248,10 @@ impl ClientState {
self.chain_id.clone()
}

pub fn is_frozen(&self) -> bool {
self.frozen_height.is_some()
}

// Resets custom fields to zero values (used in `update_client`)
pub fn zero_custom_fields(&mut self) {
self.trusting_period = ZERO_DURATION;
Expand Down Expand Up @@ -289,19 +293,6 @@ impl ClientStateCommon for ClientState {
Ok(())
}

fn confirm_not_frozen(&self) -> Result<(), ClientError> {
if let Some(frozen_height) = self.frozen_height {
return Err(ClientError::ClientFrozen {
description: format!("the client is frozen at height {frozen_height}"),
});
}
Ok(())
}

fn expired(&self, elapsed: Duration) -> bool {
elapsed > self.trusting_period
}

/// Perform client-specific verifications and check all data in the new
/// client state to be the same across all valid Tendermint clients for the
/// new chain.
Expand Down Expand Up @@ -422,6 +413,9 @@ impl ClientStateCommon for ClientState {
impl<ClientValidationContext> ClientStateValidation<ClientValidationContext> for ClientState
where
ClientValidationContext: TmValidationContext,
ClientValidationContext::AnyConsensusState: TryInto<TmConsensusState>,
ClientError:
From<<ClientValidationContext::AnyConsensusState as TryInto<TmConsensusState>>::Error>,
{
fn verify_client_message(
&self,
Expand Down Expand Up @@ -460,6 +454,43 @@ where
}
}
}

fn status(
&self,
ctx: &ClientValidationContext,
client_id: &ClientId,
) -> Result<Status, ClientError> {
if self.is_frozen() {
return Ok(Status::Frozen);
}

let latest_consensus_state: TmConsensusState = {
let any_latest_consensus_state = match ctx.consensus_state(
&ClientConsensusStatePath::new(client_id, &self.latest_height),
) {
Ok(cs) => cs,
// if the client state does not have an associated consensus state for its latest height
// then it must be expired
Err(_) => return Ok(Status::Expired),
};

any_latest_consensus_state.try_into()?
};

// Note: if the `duration_since()` is `None`, indicating that the latest
// consensus state is in the future, then we don't consider the client
// to be expired.
let now = ctx.host_timestamp()?;
if let Some(elapsed_since_latest_consensus_state) =
now.duration_since(&latest_consensus_state.timestamp())
{
if elapsed_since_latest_consensus_state > self.trusting_period {
return Ok(Status::Expired);
}
}

Ok(Status::Active)
}
}

impl<E> ClientStateExecution<E> for ClientState
Expand Down
50 changes: 41 additions & 9 deletions crates/ibc/src/core/ics02_client/client_state.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
//! Defines `ClientState`, the core type to be implemented by light clients

use core::fmt::Debug;
use core::fmt::{Debug, Display, Formatter};
use core::marker::{Send, Sync};
use core::time::Duration;

use ibc_proto::google::protobuf::Any;

Expand Down Expand Up @@ -30,6 +29,39 @@ pub enum UpdateKind {
SubmitMisbehaviour,
}

/// Represents the status of a client
#[derive(Debug, PartialEq, Eq)]
pub enum Status {
/// The client is active and allowed to be used
Active,
/// The client is frozen and not allowed to be used
Frozen,
/// The client is expired and not allowed to be used
Expired,
/// Unauthorized indicates that the client type is not registered as an allowed client type.
Unauthorized,
}

impl Status {
pub fn is_active(&self) -> bool {
*self == Status::Active
}

pub fn is_frozen(&self) -> bool {
*self == Status::Frozen
}

pub fn is_expired(&self) -> bool {
*self == Status::Expired
}
}

impl Display for Status {
fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
write!(f, "{self:?}")
}
}

/// `ClientState` methods needed in both validation and execution.
///
/// They do not require access to a client `ValidationContext` nor
Expand All @@ -50,13 +82,6 @@ pub trait ClientStateCommon {
/// Validate that the client is at a sufficient height
fn validate_proof_height(&self, proof_height: Height) -> Result<(), ClientError>;

/// Assert that the client is not frozen
fn confirm_not_frozen(&self) -> Result<(), ClientError>;

/// Check if the state is expired when `elapsed` time has passed since the latest consensus
/// state timestamp
fn expired(&self, elapsed: Duration) -> bool;

/// Verify the upgraded client and consensus states and validate proofs
/// against the given root.
///
Expand Down Expand Up @@ -140,6 +165,13 @@ pub trait ClientStateValidation<ClientValidationContext> {
client_message: Any,
update_kind: &UpdateKind,
) -> Result<bool, ClientError>;

/// Returns the status of the client. Only Active clients are allowed to process packets.
fn status(
&self,
ctx: &ClientValidationContext,
client_id: &ClientId,
) -> Result<Status, ClientError>;
}

/// `ClientState` methods which require access to the client's
Expand Down
17 changes: 12 additions & 5 deletions crates/ibc/src/core/ics02_client/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ use crate::core::timestamp::Timestamp;
use crate::core::ContextError;
use crate::Height;

use super::client_state::Status;

/// Encodes all the possible client errors
#[derive(Debug, Display)]
pub enum ClientError {
Expand All @@ -25,6 +27,8 @@ pub enum ClientError {
},
/// client is frozen with description: `{description}`
ClientFrozen { description: String },
/// client is not active. Status=`{status}`
ClientNotActive { status: Status },
/// client state not found: `{client_id}`
ClientStateNotFound { client_id: ClientId },
/// client state already exists: `{client_id}`
Expand Down Expand Up @@ -91,11 +95,6 @@ pub enum ClientError {
},
/// timestamp is invalid or missing, timestamp=`{time1}`, now=`{time2}`
InvalidConsensusStateTimestamp { time1: Timestamp, time2: Timestamp },
/// header not within trusting period: expires_at=`{latest_time}` now=`{update_time}`
HeaderNotWithinTrustPeriod {
latest_time: Timestamp,
update_time: Timestamp,
},
/// the local consensus state could not be retrieved for height `{height}`
MissingLocalConsensusState { height: Height },
/// invalid signer error: `{reason}`
Expand All @@ -121,6 +120,14 @@ impl From<ContextError> for ClientError {
}
}

impl From<&'static str> for ClientError {
fn from(s: &'static str) -> Self {
Self::Other {
description: s.to_string(),
}
}
}

#[cfg(feature = "std")]
impl std::error::Error for ClientError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
Expand Down
25 changes: 20 additions & 5 deletions crates/ibc/src/core/ics02_client/handler/update_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ where
// Read client state from the host chain store. The client should already exist.
let client_state = ctx.client_state(&client_id)?;

client_state.confirm_not_frozen()?;
{
let status = client_state.status(ctx.get_client_validation_context(), &client_id)?;
if !status.is_active() {
return Err(ClientError::ClientNotActive { status }.into());
}
}

let client_message = msg.client_message();

Expand Down Expand Up @@ -245,7 +250,10 @@ mod tests {
assert!(res.is_ok(), "result: {res:?}");

let client_state = ctx.client_state(&msg.client_id).unwrap();
assert!(client_state.confirm_not_frozen().is_ok());
assert!(client_state
.status(&ctx, &msg.client_id)
.unwrap()
.is_active());
assert_eq!(client_state.latest_height(), latest_header_height);
}

Expand Down Expand Up @@ -292,7 +300,10 @@ mod tests {
assert!(res.is_ok(), "result: {res:?}");

let client_state = ctx.client_state(&msg.client_id).unwrap();
assert!(client_state.confirm_not_frozen().is_ok());
assert!(client_state
.status(&ctx, &msg.client_id)
.unwrap()
.is_active());
assert_eq!(client_state.latest_height(), latest_header_height);
}

Expand Down Expand Up @@ -413,7 +424,10 @@ mod tests {
assert!(res.is_ok(), "result: {res:?}");

let client_state = ctx_a.client_state(&msg.client_id).unwrap();
assert!(client_state.confirm_not_frozen().is_ok());
assert!(client_state
.status(&ctx_a, &msg.client_id)
.unwrap()
.is_active());
assert_eq!(client_state.latest_height(), latest_header_height);
assert_eq!(client_state, ctx_a.latest_client_states(&msg.client_id));
}
Expand Down Expand Up @@ -496,7 +510,8 @@ mod tests {
fn ensure_misbehaviour(ctx: &MockContext, client_id: &ClientId, client_type: &ClientType) {
let client_state = ctx.client_state(client_id).unwrap();

assert!(client_state.confirm_not_frozen().is_err());
let status = client_state.status(ctx, client_id).unwrap();
assert!(status.is_frozen(), "client_state status: {status}");

// check events
assert_eq!(ctx.events.len(), 2);
Expand Down
Loading