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

imp(ibc)!: refactor Timestamp type and distinguish timeout timestamp #1307

Merged
merged 15 commits into from
Aug 8, 2024

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Aug 8, 2024

Closes: #180
Closes: #1296
Closes: #1306

Description

This PR introduces following improvements:

  • Separates the Timestamp definitions used by the host from those used for timeout purposes. The host timestamp is mandatory, while a None value for the timeout timestamp indicates the absence of a timeout.
  • Implements a direct, more efficient conversion to/from protobuf timestamps, eliminating the previous redirection through tendermint::Time, which was also intended for tendermint_proto::google::Timestamp instead of ibc_proto::google::Timestamp.
  • Decouples timestamp definition from tendermint::Time, laying the groundwork for fully decoupling ibc_primitive from the tendermint dependency, making it chain-agnostic.
  • Fixes an issue where tendermint::Time could produce negative UNIX timestamps, causing a panic when calling Timestamp::nanoseconds().
  • Unifies all timestamp-related errors under a single TimestampError type
  • Removes the error variant for Timestamp::from_nanoseconds, as it never fails.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@Farhad-Shabani Farhad-Shabani added this to the 0.54.0 milestone Aug 8, 2024
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 76.13636% with 63 lines in your changes missing coverage. Please review.

Project coverage is 67.07%. Comparing base (7e39192) to head (ac1ec73).

Files Patch % Lines
ibc-primitives/src/types/timestamp.rs 76.98% 29 Missing ⚠️
...-core/ics04-channel/types/src/timeout/timestamp.rs 80.00% 13 Missing ⚠️
ibc-clients/ics07-tendermint/types/src/header.rs 0.00% 6 Missing ⚠️
ibc-testkit/src/hosts/tendermint.rs 50.00% 6 Missing ⚠️
ibc-core/ics04-channel/types/src/events/mod.rs 20.00% 4 Missing ⚠️
ibc-apps/ics20-transfer/types/src/msgs/transfer.rs 0.00% 2 Missing ⚠️
...pps/ics721-nft-transfer/types/src/msgs/transfer.rs 0.00% 2 Missing ⚠️
ibc-core/ics04-channel/types/src/timeout/height.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1307      +/-   ##
==========================================
- Coverage   67.08%   67.07%   -0.01%     
==========================================
  Files         226      227       +1     
  Lines       23336    23383      +47     
==========================================
+ Hits        15654    15685      +31     
- Misses       7682     7698      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review August 8, 2024 02:08
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Awesome job putting this PR together 🙌

@seanchen1991 seanchen1991 added this pull request to the merge queue Aug 8, 2024
Merged via the queue into main with commit 966d0de Aug 8, 2024
15 checks passed
@seanchen1991 seanchen1991 deleted the farhad/overhaul-timestamp branch August 8, 2024 20:17
Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
#1307)

* imp: refactor Timestamp type and distinguish timeout timestamp

* fix: timestamp tests

* fix: correct serde impls + tests

* imp: From<u64> nevel fails + tests

* fix: From<u64> for TimeoutTimestamp never fails

* fix: correct from_unix_timestamp to reject large seconds + remove From<u64> for Timestamp

* chore: apply review comments

* fix: cargo doc

* Fix typo in doc comment

* fix: remove redundant negative check in Sub impl

* tests: add timout arithmetic tests

* docs: add a comment for From<u64> for TimeoutTimestamp

* fix: revert client_processed_times signature

* chore: add changelog

---------

Co-authored-by: Sean Chen <[email protected]>
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
Projects
Status: Done
2 participants