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.

Sign up for GitHub

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

Jump to bottom

Supersede ClientState proof verification methods with generic interfaces #531

Merged
merged 18 commits into from
Mar 16, 2023
Merged

Supersede ClientState proof verification methods with generic interfaces #531

merged 18 commits into from
Mar 16, 2023

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Mar 14, 2023 •
edited by plafer
Loading

Closes: #530
Closes: #545

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).

@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Mar 14, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.33.0 milestone Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 80.10% and project coverage change: +0.84 🎉

Comparison is base (8fe5d08) 71.84% compared to head (3a4c9e4) 72.68%.

❗ Current head 3a4c9e4 differs from pull request most recent head 828b40a. Consider uploading reports for the commit 828b40a to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
+ Coverage   71.84%   72.68%   +0.84%     
==========================================
  Files         126      126              
  Lines       15836    15540     -296     
==========================================
- Hits        11377    11296      -81     
+ Misses       4459     4244     -215
Impacted Files Coverage Δ
crates/ibc/src/clients/ics07_tendermint/error.rs 19.04% <ø> (ø)
crates/ibc/src/core/ics02_client/client_state.rs 71.42% <ø> (-3.58%) ⬇️
crates/ibc/src/core/ics02_client/error.rs 5.55% <ø> (+0.79%) ⬆️
crates/ibc/src/core/ics03_connection/error.rs 7.14% <ø> (ø)
crates/ibc/src/core/ics04_channel/error.rs 0.00% <ø> (ø)
crates/ibc/src/hosts/tendermint.rs 0.00% <0.00%> (ø)
...s/ibc/src/clients/ics07_tendermint/client_state.rs 64.28% <37.50%> (+10.98%) ⬆️
...src/core/ics04_channel/handler/timeout_on_close.rs 86.56% <66.66%> (+0.39%) ⬆️
crates/ibc/src/mock/client_state.rs 86.45% <80.00%> (-4.82%) ⬇️
crates/ibc/src/core/ics04_channel/channel.rs 54.07% <87.50%> (+0.82%) ⬆️
... and 18 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review March 14, 2023 00:16
plafer
plafer reviewed Mar 14, 2023
View reviewed changes
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

review still WIP, but got this so far!

crates/ibc/src/clients/ics07_tendermint/client_state.rs Outdated Show resolved Hide resolved
crates/ibc/src/clients/ics07_tendermint/client_state.rs Outdated Show resolved Hide resolved
crates/ibc/src/clients/ics07_tendermint/client_state.rs Outdated Show resolved Hide resolved
@plafer
Copy link
Contributor

plafer commented Mar 14, 2023

could you also run the integration tests on basecoin-rs, given that this is a critical change?

@Farhad-Shabani
Copy link
Member Author

Let's see how things will go for the basecoin-rs
Surprisingly, I only changed the branch for the ibc-rs dependency there!

@Farhad-Shabani Farhad-Shabani mentioned this pull request Mar 14, 2023
Merged
1 task
@Farhad-Shabani Farhad-Shabani mentioned this pull request Mar 15, 2023
Closed
plafer
plafer approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

🎉

@plafer plafer merged commit 22fe103 into main Mar 16, 2023
@plafer plafer deleted the farhad/generic-proof-verification branch March 16, 2023 17:54
@Farhad-Shabani Farhad-Shabani mentioned this pull request Mar 17, 2023
Closed
Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
@Farhad-Shabani @plafer
Supersede ClientState proof verification methods with generic inter…
4bd246a 
…faces (#531)

* Supersede ClientState proof verification methods with generic interfaces

* add changelog entry

* Inline tendermint verify_(non)_membership impls

* Move height verifications to appropriate places

* Fix verify_* comments

* Fix tests

* Remove Seq & ConsState conversions

* Fix no-std

* Fix ChannelEnd error comment

* Add client_state frozen check for chan_open/close_init

* Rename assert_not_frozen and make it abstract

* Fix chan_open_init unit tests

* remove extra block

* changelog 545

* Remove frozen_height interface

* Add missed client frozen checks

* Add proto_encode_vec

---------

Co-authored-by: Philippe Laferriere <plafer@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plafer plafer plafer approved these changes

Assignees
No one assigned
Labels
A: breaking Admin: breaking change that may impact operators
Projects
ibc-rs
Status: ✅ Done
Milestone
v0.33.0
2 participants
@Farhad-Shabani @plafer