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

Fix upgrade-client issues and refactor relevant tests #673

Merged
merged 5 commits into from
May 11, 2023

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented May 10, 2023

Closes: #385
Closes: #671
Closes: #672

Description

Check out Basecoin-rs PR99 for the new integration test covering client upgrades.

Note:

  • With the current implementation, all comet-based chains can upgrade their clients on hosts using IBC-rs, but hosts for handling an upgrade proposal must figure out the implementation suits their architecture, but can use the newly defined APIs here for setting/getting upgraded_client/cons_state.

  • The new APIs haven't been implemented for MockContext yet because both it and its storage will be undergoing changes. Additionally, the methods aren't callable anywhere for testing purposes due to the lack of an upgrade proposal handler.


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 mentioned this pull request May 10, 2023
7 tasks
@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review May 10, 2023 21:11
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Patch coverage: 87.67% and project coverage change: +0.12 🎉

Comparison is base (ccd6a97) 72.86% compared to head (736a7d2) 72.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #673      +/-   ##
==========================================
+ Coverage   72.86%   72.99%   +0.12%     
==========================================
  Files         113      113              
  Lines       15258    15304      +46     
==========================================
+ Hits        11118    11171      +53     
+ Misses       4140     4133       -7     
Impacted Files Coverage Δ
...bc/src/clients/ics07_tendermint/consensus_state.rs 85.71% <ø> (+5.50%) ⬆️
crates/ibc/src/core/handler.rs 88.60% <ø> (ø)
crates/ibc/src/core/ics02_client/error.rs 20.83% <ø> (ø)
crates/ibc/src/hosts/tendermint.rs 0.00% <0.00%> (ø)
...s/ibc/src/core/ics02_client/msgs/upgrade_client.rs 80.26% <73.33%> (-4.28%) ⬇️
...s/ibc/src/clients/ics07_tendermint/client_state.rs 69.34% <82.97%> (+1.36%) ⬆️
...bc/src/core/ics02_client/handler/upgrade_client.rs 90.64% <93.68%> (+0.98%) ⬆️
crates/ibc/src/core/ics23_commitment/commitment.rs 73.56% <100.00%> (+0.94%) ⬆️
crates/ibc/src/core/ics24_host/path.rs 81.19% <100.00%> (+0.04%) ⬆️
crates/ibc/src/mock/context.rs 73.98% <100.00%> (+0.02%) ⬆️

... and 5 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.

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.

👌

@Farhad-Shabani Farhad-Shabani merged commit af477d4 into main May 11, 2023
@Farhad-Shabani Farhad-Shabani deleted the farhad/finalize-upgrade-client branch May 11, 2023 19:11
@Farhad-Shabani Farhad-Shabani added this to the v0.41.0 milestone May 11, 2023
Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
* Fix upgrade-client bugs and refactor relevant tests

* Update ADR06

* Add APIs for storing/retrieving upgraded client/cons states

* Add unclog

* Remove upgrade APIs to be added in a later PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
2 participants