refactor: allow ICA authentication module provided timeout timestamp …

…values (#726) * allow ICA authentication module provided timeout timestamp * update ICA docs * fix test case * Apply suggestions from code review Co-authored-by: Sean King <seantking@users.noreply.github.com> Co-authored-by: Sean King <seantking@users.noreply.github.com>
cosmos · Jan 13, 2022 · 5b7d362 · 5b7d362
1 parent 5f4a90c
commit 5b7d362
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 14 deletions.
diff --git a/docs/.vuepress/config.js b/docs/.vuepress/config.js
@@ -121,6 +121,11 @@ module.exports = {
                     directory: false,
                     path: "/app_modules/interchain-accounts/integration.html"
                 },
+                {
+                    title: "Authentication module development",
+                    directory: false,
+                    path: "/app_modules/interchain-accounts/ica_auth.html"
+                },
             ]
             },
           ]

diff --git a/docs/app_modules/interchain-accounts/ica_auth.md b/docs/app_modules/interchain-accounts/ica_auth.md
@@ -210,7 +210,12 @@ packetData := icatypes.InterchainAccountPacketData{
     Data: data,
 }
 
-_, err = keeper.icaControllerKeeper.TrySendTx(ctx, chanCap, p, packetData)
+// Obtain timeout timestamp
+// An appropriate timeout timestamp must be determined based on the usage of the interchain account.
+// If the packet times out, the channel will be closed requiring a new channel to be created 
+timeoutTimestamp := obtainTimeoutTimestamp()
+
+_, err = keeper.icaControllerKeeper.TrySendTx(ctx, chanCap, p, packetData, timeoutTimestamp)
 ```
 
 The data within an `InterchainAccountPacketData` must be serialized using a format supported by the host chain. 

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/relay.go b/modules/apps/27-interchain-accounts/controller/keeper/relay.go
@@ -10,9 +10,12 @@ import (
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 )
 
-// TrySendTx takes in a transaction from an authentication module and attempts to send the packet
-// if the base application has the capability to send on the provided portID
-func (k Keeper) TrySendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, portID string, icaPacketData icatypes.InterchainAccountPacketData) (uint64, error) {
+// TrySendTx takes pre-built packet data containing messages to be executed on the host chain from an authentication module and attempts to send the packet.
+// The packet sequence for the outgoing packet is returned as a result.
+// If the base application has the capability to send on the provided portID. An appropriate
+// absolute timeoutTimestamp must be provided. If the packet is timed out, the channel will be closed.
+// In the case of channel closure, a new channel may be reopened to reconnect to the host chain.
+func (k Keeper) TrySendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, portID string, icaPacketData icatypes.InterchainAccountPacketData, timeoutTimestamp uint64) (uint64, error) {
 	// Check for the active channel
 	activeChannelID, found := k.GetActiveChannelID(ctx, portID)
 	if !found {
@@ -27,7 +30,11 @@ func (k Keeper) TrySendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability,
 	destinationPort := sourceChannelEnd.GetCounterparty().GetPortID()
 	destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID()
 
-	return k.createOutgoingPacket(ctx, portID, activeChannelID, destinationPort, destinationChannel, chanCap, icaPacketData)
+	if uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp {
+		return 0, icatypes.ErrInvalidTimeoutTimestamp
+	}
+
+	return k.createOutgoingPacket(ctx, portID, activeChannelID, destinationPort, destinationChannel, chanCap, icaPacketData, timeoutTimestamp)
 }
 
 func (k Keeper) createOutgoingPacket(
@@ -38,6 +45,7 @@ func (k Keeper) createOutgoingPacket(
 	destinationChannel string,
 	chanCap *capabilitytypes.Capability,
 	icaPacketData icatypes.InterchainAccountPacketData,
+	timeoutTimestamp uint64,
 ) (uint64, error) {
 	if err := icaPacketData.ValidateBasic(); err != nil {
 		return 0, sdkerrors.Wrap(err, "invalid interchain account packet data")
@@ -49,10 +57,6 @@ func (k Keeper) createOutgoingPacket(
 		return 0, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "failed to retrieve next sequence send for channel %s on port %s", sourceChannel, sourcePort)
 	}
 
-	// timeoutTimestamp is set to be a max number here so that we never recieve a timeout
-	// ics-27-1 uses ordered channels which can close upon recieving a timeout, which is an undesired effect
-	const timeoutTimestamp = ^uint64(0) >> 1 // Shift the unsigned bit to satisfy hermes relayer timestamp conversion
-
 	packet := channeltypes.NewPacket(
 		icaPacketData.GetBytes(),
 		sequence,

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/relay_test.go b/modules/apps/27-interchain-accounts/controller/keeper/relay_test.go
@@ -14,9 +14,10 @@ import (
 
 func (suite *KeeperTestSuite) TestTrySendTx() {
 	var (
-		path       *ibctesting.Path
-		packetData icatypes.InterchainAccountPacketData
-		chanCap    *capabilitytypes.Capability
+		path             *ibctesting.Path
+		packetData       icatypes.InterchainAccountPacketData
+		chanCap          *capabilitytypes.Capability
+		timeoutTimestamp uint64
 	)
 
 	testCases := []struct {
@@ -114,13 +115,38 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 			},
 			false,
 		},
+		{
+			"timeout timestamp is not in the future",
+			func() {
+				interchainAccountAddr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
+				suite.Require().True(found)
+
+				msg := &banktypes.MsgSend{
+					FromAddress: interchainAccountAddr,
+					ToAddress:   suite.chainB.SenderAccount.GetAddress().String(),
+					Amount:      sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
+				}
+
+				data, err := icatypes.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []sdk.Msg{msg})
+				suite.Require().NoError(err)
+
+				packetData = icatypes.InterchainAccountPacketData{
+					Type: icatypes.EXECUTE_TX,
+					Data: data,
+				}
+
+				timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().UnixNano())
+			},
+			false,
+		},
 	}
 
 	for _, tc := range testCases {
 		tc := tc
 
 		suite.Run(tc.msg, func() {
-			suite.SetupTest() // reset
+			suite.SetupTest()             // reset
+			timeoutTimestamp = ^uint64(0) // default
 
 			path = NewICAPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupConnections(path)
@@ -134,7 +160,7 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 
 			tc.malleate() // malleate mutates test data
 
-			_, err = suite.chainA.GetSimApp().ICAControllerKeeper.TrySendTx(suite.chainA.GetContext(), chanCap, path.EndpointA.ChannelConfig.PortID, packetData)
+			_, err = suite.chainA.GetSimApp().ICAControllerKeeper.TrySendTx(suite.chainA.GetContext(), chanCap, path.EndpointA.ChannelConfig.PortID, packetData, timeoutTimestamp)
 
 			if tc.expPass {
 				suite.Require().NoError(err)

diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go
@@ -19,4 +19,5 @@ var (
 	ErrUnsupported                 = sdkerrors.Register(ModuleName, 13, "interchain account does not support this action")
 	ErrInvalidControllerPort       = sdkerrors.Register(ModuleName, 14, "invalid controller port")
 	ErrInvalidHostPort             = sdkerrors.Register(ModuleName, 15, "invalid host port")
+	ErrInvalidTimeoutTimestamp     = sdkerrors.Register(ModuleName, 16, "timeout timestamp must be in the future")
 )