refactor: restructure timeout type #5404

colin-axner · 2023-12-13T12:32:52Z

Description

Applies discussion in #3483. Primarily, HasPassed is renamed to Elapsed. In addition, I added two functions to the timeout type ErrTimeoutElapsed and ErrTimeoutNotReached which will return an error msg based on whether the height or the timestamp is responsible for the error. In the case of both, it defaults to reporting the height first

here are example diffs of when used by the other handlers as well:

diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go
index 3b826dea0..fc3129988 100644
--- a/modules/core/04-channel/keeper/packet.go
+++ b/modules/core/04-channel/keeper/packet.go
@@ -74,25 +74,14 @@ func (k Keeper) SendPacket(
                return 0, errorsmod.Wrapf(clienttypes.ErrClientNotActive, "cannot send packet using client (%s) with status %s", connectionEnd.GetClientID(), status)
        }
 
-       // check if packet is timed out on the receiving chain
-       latestHeight := clientState.GetLatestHeight()
-       if !timeoutHeight.IsZero() && latestHeight.GTE(timeoutHeight) {
-               return 0, errorsmod.Wrapf(
-                       types.ErrPacketTimeout,
-                       "receiving chain block height >= packet timeout height (%s >= %s)", latestHeight, timeoutHeight,
-               )
-       }
-
        latestTimestamp, err := k.connectionKeeper.GetTimestampAtHeight(ctx, connectionEnd, latestHeight)
        if err != nil {
                return 0, err
        }
 
-       if packet.GetTimeoutTimestamp() != 0 && latestTimestamp >= packet.GetTimeoutTimestamp() {
-               return 0, errorsmod.Wrapf(
-                       types.ErrPacketTimeout,
-                       "receiving chain block timestamp >= packet timeout timestamp (%s >= %s)", time.Unix(0, int64(latestTimestamp)).UTC(), time.Unix(0, int64(packet.GetTimeoutTimestamp())).UTC(),
-               )
+       timeout := types.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp()) // could eventually be packet.GetTimeout()
+       if timeout.Elapsed(latestHeight.(clienttypes.Height), latestTimestamp) {
+               return 0, errorsmod.Wrap(timeout.ErrTimeoutElapsed(latestHeight, latestTimestamp), "invalid packet timeout")
        }
 
        commitment := types.CommitPacket(k.cdc, packet)
@@ -172,21 +161,10 @@ func (k Keeper) RecvPacket(
        }
 
        // check if packet timeouted by comparing it with the latest height of the chain
-       selfHeight := clienttypes.GetSelfHeight(ctx)
-       timeoutHeight := packet.GetTimeoutHeight()
-       if !timeoutHeight.IsZero() && selfHeight.GTE(timeoutHeight) {
-               return errorsmod.Wrapf(
-                       types.ErrPacketTimeout,
-                       "block height >= packet timeout height (%s >= %s)", selfHeight, timeoutHeight,
-               )
-       }
-
-       // check if packet timeouted by comparing it with the latest timestamp of the chain
-       if packet.GetTimeoutTimestamp() != 0 && uint64(ctx.BlockTime().UnixNano()) >= packet.GetTimeoutTimestamp() {
-               return errorsmod.Wrapf(
-                       types.ErrPacketTimeout,
-                       "block timestamp >= packet timeout timestamp (%s >= %s)", ctx.BlockTime(), time.Unix(0, int64(packet.GetTimeoutTimestamp())).UTC(),
-               )
+       selfHeight, selfTimestamp := clienttypes.GetSelfHeight(ctx), uint64(ctx.BlockTime().UnixNano())
+       timeout := types.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp())
+       if timeout.Elapsed(selfHeight, selfTimestamp) {
+               return errorsmod.Wrap(timeout.ErrTimeoutElapsed(selfHeight, selfTimestamp), "packet timeout elapsed")
        }
 
        commitment := types.CommitPacket(k.cdc, packet)
diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go
index 929824fed..a19ac05f2 100644
--- a/modules/core/04-channel/keeper/timeout.go
+++ b/modules/core/04-channel/keeper/timeout.go
@@ -68,10 +68,9 @@ func (k Keeper) TimeoutPacket(
                return err
        }
 
-       timeoutHeight := packet.GetTimeoutHeight()
-       if (timeoutHeight.IsZero() || proofHeight.LT(timeoutHeight)) &&
-               (packet.GetTimeoutTimestamp() == 0 || proofTimestamp < packet.GetTimeoutTimestamp()) {
-               return errorsmod.Wrap(types.ErrPacketTimeout, "packet timeout has not been reached for height or timestamp")
+       timeout := types.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp())
+       if !timeout.Elapsed(proofHeight.(clienttypes.Height), proofTimestamp) {
+               return errorsmod.Wrap(timeout.ErrTimeoutNotReached(proofHeight.(clienttypes.Height), proofTimestamp), "packet timeout not reached")
        }

closes: #3483

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

colin-axner · 2023-12-13T12:34:43Z

modules/core/04-channel/types/timeout.go

+
+// ErrTimeoutNotReached returns a timeout not reached error indicating which timeout value
+// has not been reached.
+func (t Timeout) ErrTimeoutNotReached(height clienttypes.Height, timestamp uint64) error {


this can be ErrTimeoutNotElapsed if folks want

colin-axner · 2023-12-13T12:35:15Z

modules/core/04-channel/types/timeout.go

what I like about this now is that we have a generic timeout type rather than a timeout specific to channel upgrades

…go into colin/3483-timeout-type-impl

colin-axner · 2023-12-14T18:18:18Z

modules/core/04-channel/types/timeout.go

+// ErrTimeoutElapsed returns a timeout elapsed error indicating which timeout value
+// has elapsed.
+func (t Timeout) ErrTimeoutElapsed(height clienttypes.Height, timestamp uint64) error {
+	if t.heightElapsed(height) {


happy to add an additional statement if folks want to indicate both height and timestamp elapsed. I just opted for this for now

…go into colin/3483-timeout-type-impl

DimitrisJim

overall lgtm, thanks 💪, left a couple of minor comments

DimitrisJim · 2023-12-18T15:01:08Z

modules/core/04-channel/types/timeout_test.go

+		tc := tc
+		suite.Run(tc.name, func() {
+			err := tc.timeout.ErrTimeoutElapsed(height, timestamp)
+			suite.Require().Equal(err.Error(), tc.expError.Error())


switcharoo on Equal args.

Suggested change

suite.Require().Equal(err.Error(), tc.expError.Error())

suite.Require().Equal(tc.expError.Error(), err.Error())

DimitrisJim · 2023-12-18T15:01:19Z

modules/core/04-channel/types/timeout_test.go

+		tc := tc
+		suite.Run(tc.name, func() {
+			err := tc.timeout.ErrTimeoutNotReached(height, timestamp)
+			suite.Require().Equal(err.Error(), tc.expError.Error())


ditto re switcharoo

DimitrisJim · 2023-12-18T15:49:09Z

modules/core/04-channel/types/timeout.go

+// ErrTimeoutNotReached returns a timeout not reached error indicating which timeout value
+// has not been reached.
+func (t Timeout) ErrTimeoutNotReached(height clienttypes.Height, timestamp uint64) error {
+	if !t.Height.IsZero() && !t.heightElapsed(height) {


isn't this just !t.heightElapsed(height)? Currently would expand to:

!t.Height.IsZero() && !t.Height.IsZero() && height.GTE(t.Height)

No it is: !t.Height.IsZero() && !(!t.Height.IsZero() && height.GTE(t.Height))

When t.Height.IsZero(), the second statement returns !false. This caught me while writing tests as I originally just had !t.heightElapsed(height). It's quite unfortunate and also odd. When height is 0, heightElapsed returning false makes sense, but in the case of ErrTimeoutNotReached it doesn't as the height is not being used to determine the timeout

I could pull out the !IsZero() checks out of heightElapsed and timestampElapsed or I could add a comment to explain that when asserting timeout has not been reached, we must filter out the height if it is empty

aha! indeed. These are slightly tricky to read, thanks for adding a comment here!

…olin/3483-timeout-type-impl

…go into colin/3483-timeout-type-impl

damiannolan

LGTM, I think there's spots in Send/RecvPacket that could make use of the new type also! Glad to see this finally in. Thanks @colin-axner

codecov-commenter · 2023-12-19T21:14:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (26fbb37) 80.67% compared to head (74ebc67) 80.77%.

Additional details and impacted files

@@                   Coverage Diff                   @@
##           04-channel-upgrades    #5404      +/-   ##
=======================================================
+ Coverage                80.67%   80.77%   +0.10%     
=======================================================
  Files                      197      197              
  Lines                    15101    15114      +13     
=======================================================
+ Hits                     12183    12209      +26     
+ Misses                    2453     2440      -13     
  Partials                   465      465

Files	Coverage Δ
modules/core/04-channel/keeper/packet.go	`99.12% <100.00%> (+<0.01%)`	⬆️
modules/core/04-channel/keeper/timeout.go	`95.41% <100.00%> (+0.04%)`	⬆️
modules/core/04-channel/keeper/upgrade.go	`92.12% <100.00%> (+0.04%)`	⬆️
modules/core/04-channel/types/timeout.go	`100.00% <100.00%> (+65.00%)`	⬆️

colin-axner added 2 commits December 13, 2023 13:12

refactor: apply issue design to timeout type

ac8840b

chore: add godocs

7ec14cc

colin-axner commented Dec 13, 2023

View reviewed changes

Merge branch '04-channel-upgrades' into colin/3483-timeout-type-impl

01cf9b6

colin-axner requested review from chatton, DimitrisJim and charleenfei December 13, 2023 12:40

colin-axner added the channel-upgradability Channel upgradability feature label Dec 13, 2023

colin-axner added 5 commits December 13, 2023 18:17

tests: add tests for elapsed and ErrTimeoutElapsed functions

86dc54f

Merge branch 'colin/3483-timeout-type-impl' of github.com:cosmos/ibc-…

adcc031

…go into colin/3483-timeout-type-impl

rm: unfinished tests

b6b65c1

Merge branch '04-channel-upgrades' into colin/3483-timeout-type-impl

5c261e2

test: tests for ErrTimeoutNotReached

638f967

colin-axner commented Dec 14, 2023

View reviewed changes

colin-axner added 3 commits December 14, 2023 19:18

test: add tests for ErrTimeoutNotReached

30ac3b4

Merge branch 'colin/3483-timeout-type-impl' of github.com:cosmos/ibc-…

6c7c9f7

…go into colin/3483-timeout-type-impl

Merge branch '04-channel-upgrades' into colin/3483-timeout-type-impl

e50e3a4

colin-axner marked this pull request as ready for review December 14, 2023 18:20

colin-axner requested review from AdityaSripal and damiannolan as code owners December 14, 2023 18:20

colin-axner requested a review from crodriguezvega December 14, 2023 18:20

DimitrisJim approved these changes Dec 18, 2023

View reviewed changes

colin-axner added 4 commits December 18, 2023 17:55

review suggestion: switcharoo

98f34ea

chore: add in-code comment

63373d1

Merge branch '04-channel-upgrades' of github.com:cosmos/ibc-go into c…

714b405

…olin/3483-timeout-type-impl

Merge branch 'colin/3483-timeout-type-impl' of github.com:cosmos/ibc-…

3924754

…go into colin/3483-timeout-type-impl

damiannolan approved these changes Dec 19, 2023

View reviewed changes

Merge branch '04-channel-upgrades' into colin/3483-timeout-type-impl

74ebc67

crodriguezvega merged commit 5ec221a into 04-channel-upgrades Dec 19, 2023
56 checks passed

crodriguezvega deleted the colin/3483-timeout-type-impl branch December 19, 2023 22:15

crodriguezvega mentioned this pull request Jan 5, 2024

Add Timeout type #3483

Closed

3 tasks

colin-axner mentioned this pull request Jan 10, 2024

refactor: use timeout type when possible #5572

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: restructure timeout type #5404

refactor: restructure timeout type #5404

colin-axner commented Dec 13, 2023 •

edited

Loading

colin-axner Dec 13, 2023

colin-axner Dec 13, 2023

colin-axner Dec 14, 2023

DimitrisJim left a comment

DimitrisJim Dec 18, 2023

DimitrisJim Dec 18, 2023

DimitrisJim Dec 18, 2023

colin-axner Dec 18, 2023

DimitrisJim Dec 18, 2023

damiannolan left a comment

codecov-commenter commented Dec 19, 2023

	suite.Require().Equal(err.Error(), tc.expError.Error())
	suite.Require().Equal(tc.expError.Error(), err.Error())

refactor: restructure timeout type #5404

refactor: restructure timeout type #5404

Conversation

colin-axner commented Dec 13, 2023 • edited Loading

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2023

Codecov Report

colin-axner commented Dec 13, 2023 •

edited

Loading