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

Remove ValidationContext::host_timestamp() #784

Closed
plafer opened this issue Jul 25, 2023 · 2 comments
Closed

Remove ValidationContext::host_timestamp() #784

plafer opened this issue Jul 25, 2023 · 2 comments
Labels
O: usability Objective: aims to enhance user experience (UX) and streamline product usability

Comments

@plafer
Copy link
Contributor

plafer commented Jul 25, 2023

We should implement this function ourselves in terms of host_height() and host_consensus_state() (see basecoin-rs), since there are no other valid ways to do it. It should no longer be part of the trait.

@plafer plafer added the O: usability Objective: aims to enhance user experience (UX) and streamline product usability label Jul 25, 2023
@plafer plafer added this to ibc-rs Jul 25, 2023
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Jul 25, 2023
@Farhad-Shabani
Copy link
Member

This reminded me of another issue I came across in IBC-go (cosmos/ibc-go#3957), which is kind of related to #728 as well. As in both places, we assume that the timestamp necessarily comes from a ConsensusState definition.

IBC-go does things a bit differently when it comes to getting timestamps - they use a method called GetTimestampAtHeight under the ClientState interface. Given that light clients can have access to the ConsensusState store, a client can choose whether to use ConsensusState for getting the timestamp (e.g. here) or not where the Wasm light clients can be an example.

Anyhow, worth evaluating the IBC-go design too, particularly as I see the host_timestamp in our implementation is only called by the ClientState impls.

@Farhad-Shabani
Copy link
Member

Closing this issue, as with the experience we gained from recent integration projects, specially considering this spec change, the host_consensus_state is not required anymore. Therefore, the host_timestamp does not necessarily comes from host_consensus_state, though if a host want to go that way, still can.

@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: usability Objective: aims to enhance user experience (UX) and streamline product usability
Projects
Status: Done
Development

No branches or pull requests

2 participants