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: use upgrade_ctx in upgraded_client/consensus_state for querying proof #1161

Merged

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Apr 8, 2024

Description

In PR #1153, we've made it so that client upgrade query endpoints now give us proof of inclusion along with the response. But when we ran integration tests in basecoin-rs, I found out we have to call the get_proof on upgrade_ctx to get that proof. This PR fixes this issue.


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 marked this pull request as ready for review April 8, 2024 20:57
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 63.79%. Comparing base (f7a3c2b) to head (dcb7c91).

Files Patch % Lines
ibc-query/src/core/client/query.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1161   +/-   ##
=======================================
  Coverage   63.79%   63.79%           
=======================================
  Files         219      219           
  Lines       21391    21391           
=======================================
  Hits        13647    13647           
  Misses       7744     7744           

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

Copy link
Collaborator

@rnbguy rnbguy left a comment

Choose a reason for hiding this comment

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

Ah nice catch ! 🙏 Thanks for the fix 👍

@Farhad-Shabani Farhad-Shabani added this pull request to the merge queue Apr 13, 2024
Merged via the queue into main with commit c659e21 Apr 13, 2024
16 checks passed
@Farhad-Shabani Farhad-Shabani deleted the farhad/use-upgrade-ctx-for-query-upgraded-states branch April 13, 2024 00:02
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.

2 participants