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

Implement ClientState::status() #774

merged 18 commits into from
Aug 4, 2023

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Jul 20, 2023

Closes: #536

@plafer plafer requested a review from Farhad-Shabani July 20, 2023 17:56
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 80.88% and project coverage change: +3.59% 🎉

Comparison is base (79cb166) 67.90% compared to head (4a69b49) 71.49%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
+ Coverage   67.90%   71.49%   +3.59%     
==========================================
  Files         124      124              
  Lines       14806    14960     +154     
==========================================
+ Hits        10054    10696     +642     
+ Misses       4752     4264     -488     
Files Changed Coverage Δ
...ive/src/client_state/traits/client_state_common.rs 100.00% <ø> (+100.00%) ⬆️
crates/ibc/src/core/ics02_client/error.rs 24.32% <0.00%> (-3.81%) ⬇️
...s/ibc/src/hosts/tendermint/validate_self_client.rs 0.00% <0.00%> (ø)
crates/ibc/src/mock/context.rs 74.55% <33.33%> (-0.23%) ⬇️
crates/ibc/src/core/ics02_client/client_state.rs 50.00% <53.84%> (+50.00%) ⬆️
...bc/src/core/ics02_client/handler/upgrade_client.rs 94.44% <80.00%> (+3.69%) ⬆️
...rc/core/ics03_connection/handler/conn_open_init.rs 97.27% <80.00%> (-0.85%) ⬇️
...src/core/ics03_connection/handler/conn_open_ack.rs 96.28% <83.33%> (-0.35%) ⬇️
...core/ics03_connection/handler/conn_open_confirm.rs 98.27% <83.33%> (-0.55%) ⬇️
...src/core/ics03_connection/handler/conn_open_try.rs 93.68% <83.33%> (-0.35%) ⬇️
... and 18 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@plafer plafer marked this pull request as draft July 20, 2023 18:04
@plafer plafer marked this pull request as ready for review July 26, 2023 13:55
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

Left a few suggestions. Otherwise LGTM. Thanks 🙏

@@ -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

Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

a unclog update and we are good. Thanks 🌷

@plafer
Copy link
Contributor Author

plafer commented Aug 4, 2023

Updated changelog; is this what you had in mind? Basically now we have only 1 entry that points to ibc-derive being changed, even though both ibc and ibc-derive were changed. Since this will always be the case (i.e. ibc-derive only changes when ibc changes), I figured it was self-explanatory?

@Farhad-Shabani
Copy link
Member

Updated changelog; is this what you had in mind? Basically now we have only 1 entry that points to ibc-derive being changed, even though both ibc and ibc-derive were changed. Since this will always be the case (i.e. ibc-derive only changes when ibc changes), I figured it was self-explanatory?

Yup, this works.

@plafer plafer merged commit c9ebc26 into main Aug 4, 2023
@plafer plafer deleted the plafer/536-clientstate-status branch August 4, 2023 16:07
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* Status enum

* implement status API

* replace most confirm_not_frozen

* validate_self_client no longer uses confirm_not_frozen

* remove confirm_not_frozen

* clippy

* fix test

* remove expired()

* changelog

* Remove `Status::Unknown`

* don't consider the client expired if consensus state is in the future

* Status methods

* remove redundant check

* Remove error variant

* update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ClientState: consider replacing confirm_frozen and expired methods with a "status" instead
2 participants