Static `ClientState` API #683

plafer · 2023-05-17T20:51:39Z

Closes: #296
Closes: #534
Closes: #614

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

…-client-interface

codecov · 2023-05-17T20:58:29Z

Codecov Report

Patch coverage: 83.86% and project coverage change: +0.88 🎉

Comparison is base (5ceaef3) 72.32% compared to head (0cb3657) 73.21%.

❗ Current head 0cb3657 differs from pull request most recent head ee4e888. Consider uploading reports for the commit ee4e888 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
+ Coverage   72.32%   73.21%   +0.88%     
==========================================
  Files         116      124       +8     
  Lines       15527    16101     +574     
==========================================
+ Hits        11230    11788     +558     
- Misses       4297     4313      +16

Impacted Files	Coverage Δ
crates/ibc/src/core/handler.rs	`88.60% <ø> (-1.54%)`	⬇️
crates/ibc/src/core/ics02_client/client_state.rs	`0.00% <0.00%> (-78.95%)`	⬇️
crates/ibc/src/core/ics03_connection/error.rs	`7.69% <ø> (ø)`
...src/core/ics04_channel/events/packet_attributes.rs	`10.82% <ø> (+0.13%)`	⬆️
.../src/core/ics04_channel/handler/acknowledgement.rs	`94.39% <ø> (ø)`
...c/core/ics04_channel/handler/chan_close_confirm.rs	`97.57% <ø> (ø)`
.../src/core/ics04_channel/handler/chan_close_init.rs	`97.19% <ø> (ø)`
...bc/src/core/ics04_channel/handler/chan_open_ack.rs	`97.50% <ø> (ø)`
...rc/core/ics04_channel/handler/chan_open_confirm.rs	`97.70% <ø> (ø)`
...c/src/core/ics04_channel/handler/chan_open_init.rs	`96.34% <ø> (ø)`
... and 37 more

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Farhad-Shabani

Further to our discussions on this PR, I went over the changes once again and left a few questions to ensure some other parts of the design.

crates/ibc/src/core/context.rs

crates/ibc/src/core/ics02_client/client_state.rs

crates/ibc/src/core/context.rs

crates/ibc/src/core/ics02_client/consensus_state.rs

Farhad-Shabani · 2023-06-20T02:57:35Z

crates/ibc/src/core/ics02_client/consensus_state.rs

+    ///
+    /// Note that the `Protobuf` trait in `tendermint-proto` provides convenience methods
+    /// to do this automatically.
+    fn encode_vec(&self) -> Result<Vec<u8>, tendermint_proto::Error>;


Couldn't we go with Protobuf trait from ibc-proto-rs instead? by which encode_vec can return Vec<u8> instead of Result<Vec<u8, tendermint_proto::Error>.

The Protobuf trait in ibc-proto-rs is trait object safe, which it trades off with being more complex, such as having Self: erased::TryFrom<Raw> instead of just Self: TryFrom<Raw>. Now that we no longer need trait object safety, we no longer need this additional complexity.

We can just bring the same changes and make tendermint_proto::Protobuf::encode_vec() return Vec<u8>?

Hmm. We can do so but also was thinking of not being dependent on tendermint-specific things in the "core" module.

Indeed that would be desirable, but we should do this in another PR

crates/ibc/src/hosts/tendermint/upgrade_proposal/context.rs

Farhad-Shabani · 2023-06-28T17:55:13Z

Reminder to add unclogs :)
Also, I think #614 is resolved through this PR.

Farhad-Shabani

From my end, nothing else remains. Looks solid. Excellent work on this PR 👌🏻

* StaticClientState * Static{Validation,Execution}Context * improve static clientstate & contexts * StaticTmClientState scaffold * tentative separation of `ClientState` * remove chain_id * move `zero_custom_fields()` * adjust comment * changelog * Remove methods from staticclientstate * static client state for tendermint WIP * `ClientStateValidation` for tendermint `ClientState` * `StaticClientStateExecution` tendermint * use static contexts in handlers * `StaticConsensusState` trait for tm `ConsensusState` * Move `MockClientRecord` to context file * mock: make records use enums instead of dyn * mock context works * bunch of errors fixed * fix tm consensus state * clippy * fix todos * use derive_more * Remove old types * Remove old `ClientState` * move things around * Rename tendermint's ClientState * Rename ValidationContext * Rename ExecutionContext * rename ClientState traits * fix doc * Rename ConsensusState * ClientState derive macro scaffold * macro improvements + test setup * fmt * remove ibc dev-dependency * implement validate_proof_height * fix previous merge from main * crate-level `allow(non_snake_case)` * `Imports` struct * move ClientStateBase impl to a module * extern crate trick * outdated comments * latest_height * implement more `ClientStateBase` methods * finish `ClientStateBase` impl * rustdoc ignore * comment line * introduce darling * ClientStateInitializer impl * export attribute * ClientStateInitializer done * fmt * add mock mode * use `ClientState` macro in tests * no long path * clippy rust 1.70 * remove useless cruft * comment * impl ClientStateValidation * use core::result::Result * ClientStateExecution impl in macro * Supported -> Any * host -> generics * rename derive macro attrs * Remove `PartialEq` from `ClientState` traits * Remove `Debug` supertrait from `ClientState` * Remove `Clone` requirement on `ClientState` traits * Remove `Send + Sync` from `ClientStateBase` * Remove `Debug + Clone` from `ConsensusState` * Remove `EncodeError` associated type (ConsensusState) * client-state-derive: move to submodule client_state * client-state-derive -> ibc-derive * `ConsensusState` macro * move `ClientState` re-export * Fix `ConsensusState` macro * add `ConsensuState` to ibc-derive-test * remove ibc-derive-test * Remove `ClientStateInitializer` * Finish removing `ClientStateInitializer` * docs * Remove `store_update_{height,time}` from `ExecutionContext` * Revert "Remove `store_update_{height,time}` from `ExecutionContext`" This reverts commit 282424f. * update client: store update time/height in core * create client: store update height/time in core * Remove store_update_time/height from Tm Client Execution context * remove `store_{client,consensus}_state` from `ExecutionContext` * docs * tm client: `context` module * Rename tm client types * Rename TmConsensusState * Remove `dyn-clone` dependency * Remove erased-serde dependency * Remove `ErasedPartialEq` from `Header` * ClientStateCommon * `ClientExecutionContext` trait * Complete ClientExecutionContext * Rename Host{Consensus,Client}State * Move `ClientExecutionContext` * Use `ContextError` in `ClientExecutionContext` methods * mock: remove stale time/height update * mock: clients submodule * mock: application impls submodule * mock context: file reorg * ClientExecutionContext docs * `ClientState` derive macro docs * `ConsensusState` docs * `ClientState` docstring * docs * Remove unused method * tm docs * core context traits docs * refine tm client's contexts * move `get_client_execution_context` * Move `verify_consensus_state` * Remove useless match in recv_packet * fix typo in `UpgradeValidationContext` * blanket impl for TmExecutionContext * ClientState derive macro: document generic limitation * Upgrade Contexts associated types * fix * add ClientType test * chore: organize import calls * changelog * Add `CommonContext::ConversionError` --------- Co-authored-by: Farhad Shabani <farhad.shabani@gmail.com>

plafer added 14 commits May 17, 2023 10:38

StaticClientState

c3b8585

Static{Validation,Execution}Context

167671a

improve static clientstate & contexts

af1047a

StaticTmClientState scaffold

1c97a11

tentative separation of ClientState

d0608e3

remove chain_id

a521754

move zero_custom_fields()

c4edfc2

adjust comment

d7fce36

changelog

2e5a771

Merge branch 'plafer/681-client-state-methods' into plafer/296-static…

3791150

…-client-interface

Remove methods from staticclientstate

edd4212

Merge branch 'main' into plafer/296-static-client-interface

93d797e

static client state for tendermint WIP

be379e7

ClientStateValidation for tendermint ClientState

a54cae8

plafer added 15 commits May 22, 2023 13:16

StaticClientStateExecution tendermint

212a3e0

use static contexts in handlers

0cfe195

StaticConsensusState trait for tm ConsensusState

faed5df

Move MockClientRecord to context file

9a9b8bf

mock: make records use enums instead of dyn

b039c80

mock context works

e657f5e

bunch of errors fixed

cbc30f7

fix tm consensus state

500ee80

clippy

b385463

fix todos

e5740d0

use derive_more

b2845aa

Remove old types

bfa224d

Remove old ClientState

1e18e9c

Merge branch 'main' into plafer/296-static-client-interface

c72afe5

move things around

4f6e380

plafer marked this pull request as ready for review June 14, 2023 13:28

plafer requested a review from Farhad-Shabani June 14, 2023 13:28

Farhad-Shabani reviewed Jun 20, 2023

View reviewed changes

move get_client_execution_context

ce64959

plafer mentioned this pull request Jun 20, 2023

Investigate removing ConsensusState::encode_vec with a proper trait bound #724

Open

plafer added 6 commits June 21, 2023 13:33

Merge branch 'main' into plafer/296-static-client-interface

c1fcbf0

Move verify_consensus_state

a157110

Remove useless match in recv_packet

084fb86

Merge branch 'main' into plafer/296-static-client-interface

8602543

fix typo in UpgradeValidationContext

996030d

blanket impl for TmExecutionContext

29142ed

plafer mentioned this pull request Jun 26, 2023

Track ibc-rs PR 683 informalsystems/basecoin-rs#111

Merged

ClientState derive macro: document generic limitation

58ac179

Farhad-Shabani reviewed Jun 26, 2023

View reviewed changes

crates/ibc/src/hosts/tendermint/upgrade_proposal/context.rs Show resolved Hide resolved

plafer and others added 6 commits June 26, 2023 14:13

Upgrade Contexts associated types

fbd3ccc

fix

e044556

Merge branch 'main' into plafer/296-static-client-interface

7e921a4

add ClientType test

934ad76

Merge branch 'main' into plafer/296-static-client-interface

fcd45ba

chore: organize import calls

9d2dc7b

plafer added 2 commits June 28, 2023 14:41

changelog

bf69139

Add CommonContext::ConversionError

0cb3657

Farhad-Shabani approved these changes Jul 1, 2023

View reviewed changes

Merge branch 'main' into plafer/296-static-client-interface

ee4e888

plafer merged commit cd3a266 into main Jul 5, 2023

plafer deleted the plafer/296-static-client-interface branch July 5, 2023 14:49

Farhad-Shabani mentioned this pull request Jul 11, 2023

Investigate: streamline Protobuf abstraction or re-export from tendermint-proto cosmos/ibc-proto-rs#116

Closed

Farhad-Shabani mentioned this pull request Aug 1, 2023

Investigate using static dispatch for Module look-up in Router #804

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static `ClientState` API #683

Static `ClientState` API #683

plafer commented May 17, 2023 •

edited

Loading

codecov bot commented May 17, 2023 •

edited

Loading

Farhad-Shabani left a comment

Farhad-Shabani Jun 20, 2023

plafer Jun 20, 2023

Farhad-Shabani Jun 21, 2023

plafer Jun 28, 2023

Farhad-Shabani commented Jun 28, 2023

Farhad-Shabani left a comment

Static ClientState API #683

Static ClientState API #683

Conversation

plafer commented May 17, 2023 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented May 17, 2023 • edited Loading

Codecov Report

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Farhad-Shabani Jun 20, 2023

Choose a reason for hiding this comment

plafer Jun 20, 2023

Choose a reason for hiding this comment

Farhad-Shabani Jun 21, 2023

Choose a reason for hiding this comment

plafer Jun 28, 2023

Choose a reason for hiding this comment

Farhad-Shabani commented Jun 28, 2023

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Static `ClientState` API #683

Static `ClientState` API #683

plafer commented May 17, 2023 •

edited

Loading

codecov bot commented May 17, 2023 •

edited

Loading