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 path check #1298

Merged
merged 12 commits into from
Aug 9, 2024
Merged

Conversation

yito88
Copy link
Contributor

@yito88 yito88 commented Aug 5, 2024

Closes: #1297
Closes: #1303

Description

  • Get and use the first upgrade path

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

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.

Hey @yito88, thanks for the PR. While reviewing it, I came across another issue. If you'd like to address that as part of your PR, feel free to do so. Otherwise, we're happy to take care of it right after and ensure everything is fixed for your use case.

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 39.47368% with 69 lines in your changes missing coverage. Please review.

Project coverage is 66.99%. Comparing base (ccf6ce6) to head (1dd9be8).
Report is 3 commits behind head on main.

Files Patch % Lines
...lients/ics07-tendermint/src/client_state/common.rs 5.88% 32 Missing ⚠️
ibc-query/src/core/client/query.rs 0.00% 18 Missing ⚠️
ibc-core/ics24-host/types/src/path.rs 72.88% 16 Missing ⚠️
ibc-query/src/core/client/types/request.rs 0.00% 2 Missing ⚠️
.../ics24-host/cosmos/src/upgrade_proposal/handler.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1298    +/-   ##
========================================
  Coverage   66.99%   66.99%            
========================================
  Files         226      227     +1     
  Lines       23267    23451   +184     
========================================
+ Hits        15588    15712   +124     
- Misses       7679     7739    +60     

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

@Farhad-Shabani Farhad-Shabani mentioned this pull request Aug 6, 2024
15 tasks
@yito88
Copy link
Contributor Author

yito88 commented Aug 6, 2024

@Farhad-Shabani Thank you for sharing the issue. I'll try to fix it in this PR because I want to try the fixes with Namada.

@Farhad-Shabani
Copy link
Member

I'll try to fix it in this PR because I want to try the fixes with Namada.

Awesome. Thank you, @yito88.
You can update your branch by merging the main, so that won't be getting the test-msrv job failure.

@yito88 yito88 force-pushed the yuji/fix-upgrade-path-check branch from a5b0d24 to 30fa654 Compare August 7, 2024 08:58
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for taking care of this @yito88!

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.

Thank you for the fixes. 🙏

@Farhad-Shabani Farhad-Shabani added this to the 0.54.0 milestone Aug 8, 2024
@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Aug 8, 2024

Just noticed the changelog missed. Do you mind @yito88 add the following. I don't have the permission.

- [ibc-client-tendermint] Fix client verification panic on upgrades when the
  `upgrade_path` size is 1.
  ([\#1297](https://github.com/cosmos/ibc-rs/issues/1297))
- [ibc-client-tendermint] Use the user-defined upgrade path for client upgrades,
instead of defaulting to `UPGRADED_IBC_STATE`.
([\#1303](https://github.com/cosmos/ibc-rs/issues/1303))

@yito88
Copy link
Contributor Author

yito88 commented Aug 9, 2024

Thank you for reviewing! I've added change logs.

@yito88
Copy link
Contributor Author

yito88 commented Aug 9, 2024

FYI: I've confirmed that an IBC client on Namada can be upgraded.

@seanchen1991 seanchen1991 added this pull request to the merge queue Aug 9, 2024
Merged via the queue into cosmos:main with commit 9d19cc9 Aug 9, 2024
14 checks passed
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* fix upgrade_path prefix

* fix conversion

* add more check

* user-defined upgrade path

* fix a test

* Update ibc-clients/ics07-tendermint/src/client_state/common.rs

Co-authored-by: Sean Chen <[email protected]>
Signed-off-by: Yuji Ito <[email protected]>

* Update ibc-clients/ics07-tendermint/src/client_state/common.rs

Co-authored-by: Sean Chen <[email protected]>
Signed-off-by: Yuji Ito <[email protected]>

* fmt

* disallow empty upgrade_path

* remove unused import

* add InvalidUpgradePath

* add changelogs

---------

Signed-off-by: Yuji Ito <[email protected]>
Co-authored-by: Sean Chen <[email protected]>
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.

upgrade client bypasses the user-defined upgrade path upgraded_path in ics07-tendermint was removed
3 participants