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

chore: add into_inner() for ICS07 ConsensusState #1156

Merged
merged 5 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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-client-tendermint] Add `into_inner()` method to ICS07 `ConsensusState`
([\#1156](https://github.com/cosmos/ibc-rs/pull/1156))
18 changes: 8 additions & 10 deletions docs/architecture/adr-007-light-client-contexts.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ This ADR is meant to address the main limitation of our current light client API
+ By giving the light clients access to `ValidationContext` and `ExecutionContext`, we're effectively giving them the same capabilities as the core handlers.
+ Although our current model is that all code is trusted (including light clients we didn't write), restraining the capabilities we give to light clients at the very least eliminates a class of bugs (e.g. calling the wrong method), and serves as documentation for exactly which methods the light client needs.

This ADR is all about fixing this issue; namely, to enable light clients to define their own `ValidationContext` and `ExecutionContext` traits for the host to implement.
This ADR is all about fixing this issue; namely, to enable light clients to define their own `ValidationContext` and `ExecutionContext` traits for the host to implement.

[ADR 4]: ../architecture/adr-004-light-client-crates-extraction.md
[later improved]: https://github.com/cosmos/ibc-rs/pull/584
Expand All @@ -26,9 +26,9 @@ This ADR is all about fixing this issue; namely, to enable light clients to defi

### Changes to `ClientState`

The `ClientState` functionality is split into 3 traits:
+ `ClientStateCommon`,
+ `ClientStateValidation<ClientValidationContext>`, and
The `ClientState` functionality is split into 3 traits:
+ `ClientStateCommon`,
+ `ClientStateValidation<ClientValidationContext>`, and
+ `ClientStateExecution<ClientExecutionContext>`

Then, `ClientState` is defined as
Expand Down Expand Up @@ -68,7 +68,7 @@ where
&self,
ctx: &ClientValidationContext,
// ...
) -> Result<(), ClientError> {
) -> Result<(), ClientError> {
// `get_resource_Y()` accessible through `ctx`
}

Expand All @@ -93,7 +93,7 @@ where `ClientExecutionContext` is defined as (simplified)

```rust
pub trait ClientExecutionContext: Sized {
// ... a few associated types
// ... a few associated types

/// Called upon successful client creation and update
fn store_client_state(
Expand Down Expand Up @@ -214,9 +214,7 @@ trait ClientState<ClientValidationContext, ClientExecutionContext>

The problem with defining all methods directly under `ClientState` is that it would force users to use fully qualified notation to call any method.

This arises from the fact that no method uses both generic parameters. [This playground] provides an explanatory example. Hence, our solution is to have all methods in a trait use every generic parameter of the trait to avoid this problem.

[This playground]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=da65c22f1532cecc9f92a2b7cb2d1360
Farhad-Shabani marked this conversation as resolved.
Show resolved Hide resolved
This arises from the fact that no method uses both generic parameters. Hence, our solution is to have all methods in a trait use every generic parameter of the trait to avoid this problem.

### Why did you write custom `ClientState` and `ConsensusState` derive macros? Why not use `enum_dispatch` or `enum_delegate`?
We ended up having to write our own custom derive macros because existing crates that offer similar functionality had shortcomings that prevented us from using them:
Expand All @@ -235,7 +233,7 @@ We ended up having to write our own custom derive macros because existing crates

### Negative
+ Increased complexity.
+ Harder to document.
+ Harder to document.
+ Specifically, we do not write any trait bounds on the `Client{Validation, Execution}Context` generic parameters. The effective trait bounds are spread across all light client implementations that a given host uses.


Expand Down
12 changes: 6 additions & 6 deletions ibc-clients/ics07-tendermint/src/client_state/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@
max_clock_drift: subject_max_clock_drift,
proof_specs: subject_proof_specs,
upgrade_path: subject_upgrade_path,
} = subject_client_state.clone();
} = subject_client_state;

Check warning on line 235 in ibc-clients/ics07-tendermint/src/client_state/validation.rs

View check run for this annotation

Codecov / codecov/patch

ibc-clients/ics07-tendermint/src/client_state/validation.rs#L235

Added line #L235 was not covered by tests

let substitute_client_state = ClientStateType::try_from(substitute_client_state)?;

Expand All @@ -249,11 +249,11 @@
upgrade_path: substitute_upgrade_path,
} = substitute_client_state;

(subject_trust_level == substitute_trust_level
&& subject_unbonding_period == substitute_unbonding_period
&& subject_max_clock_drift == substitute_max_clock_drift
&& subject_proof_specs == substitute_proof_specs
&& subject_upgrade_path == substitute_upgrade_path)
(subject_trust_level == &substitute_trust_level
Farhad-Shabani marked this conversation as resolved.
Show resolved Hide resolved
&& subject_unbonding_period == &substitute_unbonding_period
&& subject_max_clock_drift == &substitute_max_clock_drift
&& subject_proof_specs == &substitute_proof_specs
&& subject_upgrade_path == &substitute_upgrade_path)

Check warning on line 256 in ibc-clients/ics07-tendermint/src/client_state/validation.rs

View check run for this annotation

Codecov / codecov/patch

ibc-clients/ics07-tendermint/src/client_state/validation.rs#L252-L256

Added lines #L252 - L256 were not covered by tests
.then_some(())
.ok_or(ClientError::ClientRecoveryStateMismatch)
}
4 changes: 4 additions & 0 deletions ibc-clients/ics07-tendermint/src/consensus_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
&self.0
}

pub fn into_inner(self) -> ConsensusStateType {
self.0
}

Check warning on line 35 in ibc-clients/ics07-tendermint/src/consensus_state.rs

View check run for this annotation

Codecov / codecov/patch

ibc-clients/ics07-tendermint/src/consensus_state.rs#L33-L35

Added lines #L33 - L35 were not covered by tests

pub fn timestamp(&self) -> Time {
self.0.timestamp
}
Expand Down
Loading