ConsensusState::timestamp() can't properly handle possible conversion failures

[Farhad-Shabani]

Summary

The ICS-02 ConsensusState trait forces any new light client implementation to return a Timestamp for timestamp() implementation, without room for failure or error handling. This becomes problematic because each light client can define its own ConsensusState type, where the timestamp might not always match ibc_primitives::Timestamp. A good example is the tendermint ConsensusState, which uses tendermint::Time. Since the conversion to ibc_primitives::Timestamp isn’t guaranteed, there’s a risk of runtime errors.

Proposal

To avoid these issues, it makes more sense for the timestamp() to return a Result<Timestamp, ClientError>. This would give implementations a safe way to handle conversion failures and other potential issues when dealing with different timestamp formats.

Side Note

One way to solve this would be to redesign the trait to use an associated type Timestamp or a generic type, letting developers define their own timestamp type right within the trait. However, if we go that route, for consistency, we’d need to apply the same flexibility to other methods in validation and execution contexts. This would change how ibc-rs is integrated, since each light client would need to be integrated separately. Then we won't be needing to use the AnyClient/ConsensusState enums, and ibc-derive would no longer be needed. While this could be a more flexible and robust solution, it’s a much bigger architectural shift. Something that should be addressed through an ADR and at the right time.