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

Allow ChainEndpoint implementations to fetch any types of clients and consensus states #1625

Merged
merged 16 commits into from
Jan 20, 2022

Conversation

romac
Copy link
Member

@romac romac commented Nov 30, 2021

Closes: #1481

TODO

  • Test that client upgrades still work

For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

This method was previously returning `Self::ConsensusState` instead of `AnyConsensusState`
which made it impossible to implement it for non-Cosmos chains since it would force
the implementation to return a `Self::ConsensusState` even when the given client id
corresponds to another type of client with a different consensus state.
@romac romac marked this pull request as draft November 30, 2021 11:31
@romac romac changed the title Romac/1481 any consensus state Allow ChainEndpoint implementations to fetch any types of clients and consensus states Nov 30, 2021
@adizere
Copy link
Member

adizere commented Jan 14, 2022

@lightyear15 and @hu55a1n1, can you help with reviewing this, once it's marked ready for review? I think the PR is almost there. We don't have any automated tests to cover the upgrade CLIs & functionality, so we may need to test that manually.

@romac romac marked this pull request as ready for review January 14, 2022 08:57
Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

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

Thanks, @romac! Nicely done! 👌 Just a couple of comments related to the separation of Tendermint specific code. I am trying to test the upgrade procedure manually and will follow up with the results soon.

modules/src/core/ics02_client/client_state.rs Outdated Show resolved Hide resolved
modules/src/core/ics02_client/client_state.rs Outdated Show resolved Hide resolved
modules/src/clients/ics07_tendermint/consensus_state.rs Outdated Show resolved Hide resolved
@hu55a1n1
Copy link
Member

Tested the upgrade functionality manually with gaiad v6.0.0 and it seems to work as expected. 👍

@romac romac requested a review from hu55a1n1 January 19, 2022 10:10
Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

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

❤️

@romac romac merged commit ba43f88 into master Jan 20, 2022
@romac romac deleted the romac/1481-any-consensus-state branch January 20, 2022 08:12
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChainEndpoint trait: the role of associated type ConsensusState is ambiguous
4 participants