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

host and consensus state timestamp should not contain optional time field #1296

Closed
Tracked by #1304
Farhad-Shabani opened this issue Aug 2, 2024 · 0 comments · Fixed by #1307
Closed
Tracked by #1304

host and consensus state timestamp should not contain optional time field #1296

Farhad-Shabani opened this issue Aug 2, 2024 · 0 comments · Fixed by #1307
Assignees
Labels
A: breaking Admin: breaking change that may impact operators O: security Objective: aims to enhance security and improve safety
Milestone

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Aug 2, 2024

Background

The Timestamp definition was primarily designed to handle the packet's timestamp needs. Since a packet’s timestamp can be zero, which means it's absent. Therefore, the struct ended up using a time field with the type of Option<Time>.

Problem Statement

This approach works fine for checking packet expiration. Though, when it comes to consensus states or asking a host for its timestamp, we don't expect them to return None, and the timestamp should always be available. However, this definition allows light client developers and ibc-rs integrators to pass a Timestamp { time: None } to the system.

An an evidence of that optional non-needed:
When examining how the timestamp of Tendermint consensus state is obtained, we never encounter a None timestamp. The conversion path is proto Timestamp → Time → domain Timestamp, so it always results in a Timestamp { time: Some(time) }.

Proposal

To address this, we should at least ensure timestamp availability wherever we call host_timestamp().

A better design would be to create a generic type and trait for the timestamp, so that hosts and light client developers can introduce their own time types. Not being forced to use Timestamp, which can be challenging to conform to. Additionally, this introduces unnecessary tendermint dependency to downstream projects, which could cause resolving conflicts.

Relevant issues

@Farhad-Shabani Farhad-Shabani added the O: security Objective: aims to enhance security and improve safety label Aug 2, 2024
@seanchen1991 seanchen1991 added this to the v1 milestone Aug 5, 2024
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Aug 8, 2024
@Farhad-Shabani Farhad-Shabani self-assigned this Aug 8, 2024
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: security Objective: aims to enhance security and improve safety
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants