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.

Sign up for GitHub

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

Jump to bottom

refactor: allow ICA authentication module provided timeout timestamp values #726

Merged
merged 5 commits into from
Jan 13, 2022
Merged

refactor: allow ICA authentication module provided timeout timestamp values #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d8302de
allow ICA authentication module provided timeout timestamp
colin-axner Jan 13, 2022
de9aa19
update ICA docs
colin-axner Jan 13, 2022
7c39cab
fix test case
colin-axner Jan 13, 2022
9634000
Apply suggestions from code review
colin-axner Jan 13, 2022
486b572
Merge branch 'main' into colin/540-ica-timeout-timestamp
colin-axner Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/.vuepress/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ module.exports = {
directory: false,
path: "/app_modules/interchain-accounts/integration.html"
},
{
title: "Authentication module development",
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I noticed this was missing the other day! I have a draft almost ready for review with some more doc updates/improvements :)

directory: false,
path: "/app_modules/interchain-accounts/ica_auth.html"
},
]
},
]
Expand Down
7 changes: 6 additions & 1 deletion docs/app_modules/interchain-accounts/ica_auth.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
20 changes: 12 additions & 8 deletions modules/apps/27-interchain-accounts/controller/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any pros or cons in using an absolute timestamp instead of relative? I am asking because for the transfer cli we have a relative timestamp, right? Maybe it's good to be consistent (if possible) on using either relative or absolute, but not a mix.

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires also a spec change, right? I am keeping track of all the changes so that we don't miss any.

// Check for the active channel
activeChannelID, found := k.GetActiveChannelID(ctx, portID)
if !found {
Expand All @@ -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(
Expand All @@ -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")
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

glad this is gone 😆


packet := channeltypes.NewPacket(
icaPacketData.GetBytes(),
sequence,
Expand Down
36 changes: 31 additions & 5 deletions modules/apps/27-interchain-accounts/controller/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
1 change: 1 addition & 0 deletions modules/apps/27-interchain-accounts/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)