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

bug(core): client creation only checks if the client is frozen #1088

Open
rnbguy opened this issue Feb 13, 2024 · 4 comments · May be fixed by #1098
Open

bug(core): client creation only checks if the client is frozen #1088

rnbguy opened this issue Feb 13, 2024 · 4 comments · May be fixed by #1098
Labels
A: blocked Admin: blocked by another (internal/external) issue or PR

Comments

@rnbguy
Copy link
Collaborator

rnbguy commented Feb 13, 2024

Bug Summary

only non-frozen status is checked at client creation. we should just check if the client is active.

Details

should be refactored to if !status.is_active() { ... }.

The status() is the status of the stored client state and its corresponding stored consensus state. So this doesn't include the provided ConsensusState. So its status needs to be checked locally.

IBC-go also checks for active status, instead of just non-frozen status.

Version

v0.50.0

@hoank101
Copy link

hi @Farhad-Shabani i have a PR fix this issues, please verify it
#1098

@hoank101
Copy link

cc @rnbguy

@rnbguy rnbguy linked a pull request Feb 22, 2024 that will close this issue
7 tasks
@tropicaldog
Copy link
Contributor

Hi @rnbguy, I'd like to continue working on this issue. Could you please provide some guidance on how I should approach resolving it? I'd really appreciate any insights or suggestions you have. Thanks!

The status() is the status of the stored client state and its corresponding stored consensus state. So this doesn't include the provided ConsensusState. So its status needs to be checked locally.

Does this means we need to also store the ConsensusState to the validation context?

@rnbguy
Copy link
Collaborator Author

rnbguy commented May 10, 2024

Hey @tropicaldog, thanks for your interest.

Does this means we need to also store the ConsensusState to the validation context?

We don't modify the store in the validation step. The Context is passed as an immutable reference in the validation calls. This is a convention in ibc-rs.

The way I see it, we may have to modify the status method to take the latest ConsensusState. Since this is an API-breaking change, we need to decide on this internally.

@rnbguy rnbguy added the A: blocked Admin: blocked by another (internal/external) issue or PR label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: blocked Admin: blocked by another (internal/external) issue or PR
Projects
Status: 📥 To Do
Development

Successfully merging a pull request may close this issue.

3 participants