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

*_update_time and *_update_height should live under client contexts #914

Closed
Farhad-Shabani opened this issue Oct 11, 2023 · 0 comments · Fixed by #915
Closed

*_update_time and *_update_height should live under client contexts #914

Farhad-Shabani opened this issue Oct 11, 2023 · 0 comments · Fixed by #915
Assignees
Labels
A: breaking Admin: breaking change that may impact operators O: logic Objective: aims for better implementation logic
Milestone

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Oct 11, 2023

Background

There are different places we handle a client state transition. That transition might be because of regular update clients, misbehavior, or due to the chain upgrade, and in all of them, part of the execution process (setting the IBC states within a store) is done at the level of our ClientState trait (e.g. here for a client update) and part of that within our handlers (e.g. here for a client update)

For the same reason we gave this flexibility to light client developers to set client and consensus states during those transitions according to their preferences, we should allow consensus state metadata (storing client's update time and height) to be stored in the same level within the implementation of client state methods.

This became clear when working on adding the pruning feature (this issue) which enables hosts to get rid of expired consensus states. For that, we have to define delete_update_time and delete_update_height which couldn’t be placed under our primary Validation/Execution contexts as they won’t be in access of clients for performing the pruning.

Also, I noticed client_update_time and client_update_height later should be used for performing checks for recovering an expired client (related to this issue)

So, it’s reasonable to relocate *_update_time and *_update_height under the client contexts' traits.

@Farhad-Shabani Farhad-Shabani added the O: logic Objective: aims for better implementation logic label Oct 11, 2023
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Oct 11, 2023
@Farhad-Shabani Farhad-Shabani self-assigned this Oct 11, 2023
@Farhad-Shabani Farhad-Shabani changed the title *_update_time and *_update_height` methods should live under client contexts *_update_time and *_update_height methods should live under client contexts Oct 11, 2023
@Farhad-Shabani Farhad-Shabani changed the title *_update_time and *_update_height methods should live under client contexts *_update_time and *_update_height should live under client contexts Oct 11, 2023
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Oct 11, 2023
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Oct 12, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.46.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators O: logic Objective: aims for better implementation logic
Projects
None yet
1 participant