From 472040a43f315fa816953f4e8c95f8bc4d9ec8fc Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 Apr 2024 04:18:43 -0700 Subject: [PATCH 1/5] chore: add into_inner() for TmConsensusState + remove redundant cloning in check_substitute --- .../ics07-tendermint/src/client_state/validation.rs | 12 ++++++------ ibc-clients/ics07-tendermint/src/consensus_state.rs | 4 ++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/validation.rs b/ibc-clients/ics07-tendermint/src/client_state/validation.rs index 2510a8ade..d3d53f018 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/validation.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/validation.rs @@ -232,7 +232,7 @@ where max_clock_drift: subject_max_clock_drift, proof_specs: subject_proof_specs, upgrade_path: subject_upgrade_path, - } = subject_client_state.clone(); + } = subject_client_state; let substitute_client_state = ClientStateType::try_from(substitute_client_state)?; @@ -249,11 +249,11 @@ where 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 + && 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) .then_some(()) .ok_or(ClientError::ClientRecoveryStateMismatch) } diff --git a/ibc-clients/ics07-tendermint/src/consensus_state.rs b/ibc-clients/ics07-tendermint/src/consensus_state.rs index 242c82be4..f2473d63a 100644 --- a/ibc-clients/ics07-tendermint/src/consensus_state.rs +++ b/ibc-clients/ics07-tendermint/src/consensus_state.rs @@ -30,6 +30,10 @@ impl ConsensusState { &self.0 } + pub fn into_inner(self) -> ConsensusStateType { + self.0 + } + pub fn timestamp(&self) -> Time { self.0.timestamp } From e5466738bc8138ed2e209426ebd1797051c7e3fc Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 Apr 2024 04:30:12 -0700 Subject: [PATCH 2/5] fix: markdown-lint-check --- .../adr-007-light-client-contexts.md | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/docs/architecture/adr-007-light-client-contexts.md b/docs/architecture/adr-007-light-client-contexts.md index 834a74bfb..65406ffc1 100644 --- a/docs/architecture/adr-007-light-client-contexts.md +++ b/docs/architecture/adr-007-light-client-contexts.md @@ -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 @@ -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`, and +The `ClientState` functionality is split into 3 traits: ++ `ClientStateCommon`, ++ `ClientStateValidation`, and + `ClientStateExecution` Then, `ClientState` is defined as @@ -68,7 +68,7 @@ where &self, ctx: &ClientValidationContext, // ... - ) -> Result<(), ClientError> { + ) -> Result<(), ClientError> { // `get_resource_Y()` accessible through `ctx` } @@ -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( @@ -214,9 +214,7 @@ trait ClientState 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 +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: @@ -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. From 16d3ecfbf65379b9681705227aadd11efcf2c1f1 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 Apr 2024 04:32:38 -0700 Subject: [PATCH 3/5] chore: add unclog --- .../1156-add-into-inner-for-ics07-consensus-state.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md diff --git a/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md b/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md new file mode 100644 index 000000000..ae1771938 --- /dev/null +++ b/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md @@ -0,0 +1,2 @@ +- [ibc-client-tendermint] Add `ino_inner()` method to IC07 `ConsensusState` + ([\#1156](https://github.com/cosmos/ibc-rs/pull/1156)) From 51eab5cf12e24893f08aed33224197fc02fed040 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 Apr 2024 04:34:19 -0700 Subject: [PATCH 4/5] nit --- .../1156-add-into-inner-for-ics07-consensus-state.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md b/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md index ae1771938..08c42925b 100644 --- a/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md +++ b/.changelog/unreleased/improvements/1156-add-into-inner-for-ics07-consensus-state.md @@ -1,2 +1,2 @@ -- [ibc-client-tendermint] Add `ino_inner()` method to IC07 `ConsensusState` +- [ibc-client-tendermint] Add `into_inner()` method to ICS07 `ConsensusState` ([\#1156](https://github.com/cosmos/ibc-rs/pull/1156)) From c579628c672bdb384ae4ef3cb02a7455c9d03386 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 Apr 2024 11:01:08 -0700 Subject: [PATCH 5/5] fix: revert adr007 playground --- docs/architecture/adr-007-light-client-contexts.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/architecture/adr-007-light-client-contexts.md b/docs/architecture/adr-007-light-client-contexts.md index 65406ffc1..b0caabdd5 100644 --- a/docs/architecture/adr-007-light-client-contexts.md +++ b/docs/architecture/adr-007-light-client-contexts.md @@ -214,7 +214,9 @@ trait ClientState 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. Hence, our solution is to have all methods in a trait use every generic parameter of the trait to avoid this problem. +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 ### 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: