Add MaxTimeout condition #7521
Add MaxTimeout condition #7521
Conversation
sangier
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
| sequence, destChannel, err := k.sendPacket(ctx, msg.SourceChannel, msg.TimeoutTimestamp, msg.Payloads) | ||
| if err != nil { | ||
| sdkCtx.Logger().Error("send packet failed", "source-channel", msg.SourceChannel, "error", errorsmod.Wrap(err, "send packet failed")) | ||
| return nil, errorsmod.Wrapf(err, "send packet failed for source id: %s", msg.SourceChannel) | ||
| } | ||
|
|
||
| // Note, the validate basic function in sendPacket does the TimeoutTimestamp != 0 check and other stateless checks on the packet. | ||
| if uint64(sdkCtx.BlockTime().Unix()) > msg.TimeoutTimestamp || msg.TimeoutTimestamp > uint64(sdkCtx.BlockTime().Unix())+channeltypesv2.MaxTimeout { |
There was a problem hiding this comment.
These are separate conditions. In the first case the msg timeout timestamp is too low. But in the second case, it is too high. But the error case only says it exceeded maximum value. Split these up so you have correct error message in each case
| @@ -124,6 +134,14 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { | |||
| }, | |||
| expError: channeltypesv1.ErrTimeoutElapsed, | |||
| }, | |||
| { | |||
| name: "failure: timeout timestamp exceeds max allowed input", | |||
There was a problem hiding this comment.
Let's also add an error case for when its too low
Co-authored-by: Aditya <14364734+AdityaSripal@users.noreply.github.com>
| // Note, the validate basic function in sendPacket does the timeoutTimestamp != 0 check and other stateless checks on the packet. | ||
| // timeoutTimestamp must be less than current block time + MaxTimeoutDelta | ||
| if msg.TimeoutTimestamp > uint64(sdkCtx.BlockTime().Unix())+channeltypesv2.MaxTimeoutDelta { | ||
| return nil, errorsmod.Wrap(channeltypesv2.ErrMaxTimeoutDeltaExceeded, "timeout exceeds the maximum expected value") | ||
| } | ||
|
|
||
| // timeoutTimestamp must be greater than current block time | ||
| if uint64(sdkCtx.BlockTime().Unix()) > msg.TimeoutTimestamp { | ||
| return nil, errorsmod.Wrap(channeltypesv2.ErrTimeoutTooLow, "timeout is less than the current block timestamp") | ||
| } |
There was a problem hiding this comment.
since sdk block time is a time.Time, we could leverage the time.After() / time.Before() apis from std lib here, by creating a time.Time with the absolute timeout. Would imagine it to be something like this:
// ------------------------------------------------
const timeoutDelta time.Duration = 24 * time.Hour
// ------------------------------------------------
timeout := time.Unix(int64(msg.TimeoutTimestamp), 0)
if timeout.Before(sdkCtx.BlockTime()) {
// err timeout less than current block timestamp
}
if timeout.After(sdkCtx.BlockTime().Add(timeoutDelta)) {
// err timeout exceeds max exp value
}I'm fine with using uint64 absolute values too. But it might look a little less verbose to leverage the stdlib apis. Interested on input from others!
There was a problem hiding this comment.
Thanks @damiannolan! That's interesting. Personally I find it a bit less readable, but happy to use it if folks prefer it!
There was a problem hiding this comment.
i think we should use the time package as well when possible
There was a problem hiding this comment.
Find out that using the time package is the idiomatic choice in go. My bad. Changes applied as suggested!
|
Description
Add MaxTimeout condition in the SendPacket function as specified in 004
closes: #7337
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.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.