modules/apps/27-interchain-accounts/controller/keeper: Keeper.SendTx doesn't seem to respect the zero value for types.MsgSendTx.TimeoutTimestamp which signifies being disabled

[odeke-em]

Summary of Bug

The code in here

ibc-go/modules/apps/27-interchain-accounts/controller/keeper/msg_server.go

Line 65 in a4be561

seq, err := s.Keeper.SendTx(ctx, chanCap, msg.ConnectionId, portID, msg.PacketData, msg.TimeoutTimestamp)

then invokes

ibc-go/modules/apps/27-interchain-accounts/controller/keeper/relay.go

Lines 38 to 40 in a4be561

	if uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp {
	return 0, icatypes.ErrInvalidTimeoutTimestamp
	}

but if we look at the definition of the types.MsgSendTx, it clearly says that a 0 value disables timeouts

ibc-go/modules/apps/27-interchain-accounts/controller/types/tx.pb.go

Lines 125 to 127 in a4be561

	// Timeout timestamp in absolute nanoseconds since unix epoch.
	// The timeout is disabled when set to 0.
	TimeoutTimestamp uint64 `protobuf:"varint,4,opt,name=timeout_timestamp,json=timeoutTimestamp,proto3" json:"timeout_timestamp,omitempty" yaml:"timeout_timestamp"`

Expected Behaviour

I expected that value to be honored and that ctx.BlockTime().UnixNano() wouldn't be blindly compared with what could be a zero value. If that's not the expected behavior, please update the proto definitions.

Version

a4be561

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[crodriguezvega]

Good catch, @odeke-em!

I think we should probably do if timeoutTimestamp != 0 && uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp, right @damiannolan?

Actually, I am wondering now... why do we need this check here at all? For ICS20 we don't have this check in SendTransfer...

[colin-axner]

Hi @odeke-em, the documentation is incorrect. ICS 27 packets set the timeout height to be zero, thus the timeout timestamp must be non-zero