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.

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

feat: implementing SubmitTx gRPC endpoint #2147

Merged
merged 11 commits into from
Sep 1, 2022
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* (apps/27-interchain-accounts) [\#2147](https://github.com/cosmos/ibc-go/pull/2147) Adding a `SubmitTx` gRPC endpoint for the ICS27 Controller module which allows owners of interchain accounts to submit transactions. This replaces the previously existing need for authentication modules to implement this standard functionality.

### Bug Fixes

* (makefile) [\#1785](https://github.com/cosmos/ibc-go/pull/1785) Fetch the correct versions of protocol buffers dependencies from tendermint, cosmos-sdk, and ics23.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ import (
"context"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v5/modules/core/24-host"
)

var _ types.MsgServer = Keeper{}
Expand All @@ -26,5 +30,27 @@ func (k Keeper) RegisterAccount(goCtx context.Context, msg *types.MsgRegisterAcc

// SubmitTx defines a rpc handler for MsgSubmitTx
func (k Keeper) SubmitTx(goCtx context.Context, msg *types.MsgSubmitTx) (*types.MsgSubmitTxResponse, error) {
return &types.MsgSubmitTxResponse{}, nil
ctx := sdk.UnwrapSDKContext(goCtx)

portID, err := icatypes.NewControllerPortID(msg.Owner)
if err != nil {
return nil, err
}

channelID, found := k.GetActiveChannelID(ctx, msg.ConnectionId, portID)
if !found {
return nil, sdkerrors.Wrapf(icatypes.ErrActiveChannelNotFound, "failed to retrieve active channel for port %s", portID)
}
Comment on lines +45 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be taking in the channelID as the message argument? What's the UX benefit here? I'm thinking about a future where we remove the active channel mapping

cc @AdityaSripal

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we could, but i think in that future we will break APIs anyway. So not sure its worth optimizing for that now


chanCap, found := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, channelID))
if !found {
return nil, sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}

seq, err := k.SendTx(ctx, chanCap, msg.ConnectionId, portID, msg.PacketData, msg.TimeoutTimestamp)
if err != nil {
return nil, err
}

return &types.MsgSubmitTxResponse{Sequence: seq}, nil
}
Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
package keeper_test

import (
"time"

sdk "github.com/cosmos/cosmos-sdk/types"
sdktypes "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
controllertypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"
clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v5/modules/core/24-host"
ibctesting "github.com/cosmos/ibc-go/v5/testing"
)

func (suite *KeeperTestSuite) TestRegisterAccount() {
var (
msg *icatypes.MsgRegisterAccount
msg *controllertypes.MsgRegisterAccount
expectedChannelID = "channel-0"
)

Expand Down Expand Up @@ -63,7 +70,7 @@ func (suite *KeeperTestSuite) TestRegisterAccount() {
path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

msg = icatypes.NewMsgRegisterAccount(ibctesting.FirstConnectionID, ibctesting.TestAccAddress, "")
msg = controllertypes.NewMsgRegisterAccount(ibctesting.FirstConnectionID, ibctesting.TestAccAddress, "")

tc.malleate()

Expand All @@ -85,3 +92,108 @@ func (suite *KeeperTestSuite) TestRegisterAccount() {
}
}
}

func (suite *KeeperTestSuite) TestSubmitTx() {
var (
path *ibctesting.Path
owner string
connectionID string
Copy link
Member

Choose a reason for hiding this comment

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

do we need to expose this var to mutate, or can we just use path.EndpointA.ConnectionID and mutate the path in malleate()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine to leave as is 🤝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right actually, I updated it. Thanks

icaMsg sdk.Msg
)

testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success", func() {
owner = TestOwnerAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

set to TestOwnerAddress as default before tc.malleate()

},
true,
},
{
"failure - owner address is empty", func() {
owner = ""
},
false,
},
{
"failure - active channel does not exist for connection ID", func() {
owner = TestOwnerAddress
connectionID = "connection-100"
},
false,
},
{
"failure - active channel does not exist for port ID", func() {
owner = TestAccAddress.String()
},
false,
},
{
"failure - controller module does not own capability for this channel", func() {
owner = TestAccAddress.String()
portID, err := icatypes.NewControllerPortID(owner)
suite.Require().NoError(err)

// set the active channel with the incorrect portID in order to reach the capability check
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), connectionID, portID, path.EndpointA.ChannelID)
},
false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest()

path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

portID, err := icatypes.NewControllerPortID(TestOwnerAddress)
suite.Require().NoError(err)

// get the address of the interchain account stored in state during handshake step
interchainAccountAddr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), path.EndpointA.ConnectionID, portID)
suite.Require().True(found)

// create bank transfer message that will execute on the host chain
icaMsg = &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.chainA.Codec, []sdk.Msg{icaMsg})
suite.Require().NoError(err)

packetData := icatypes.InterchainAccountPacketData{
Type: icatypes.EXECUTE_TX,
Data: data,
Memo: "memo",
}

timeoutTimestamp := suite.chainA.GetContext().BlockTime().Add(time.Minute).UnixNano()
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a suite.chainA.GetTimeoutHeight() function I think

connectionID = path.EndpointA.ConnectionID

tc.malleate() // malleate mutates test data

msg := types.NewMsgSubmitTx(owner, connectionID, clienttypes.NewHeight(0, 0), uint64(timeoutTimestamp), packetData)
Copy link
Contributor

Choose a reason for hiding this comment

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

use clienttypes.ZeroHeight() instead

Copy link
Member

Choose a reason for hiding this comment

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

we could expose the msg as test func level var and mutate the fields - owner, connection, packetData..etc in malleate()

it would remove the need for 3 test func level vars I think

res, err := suite.chainA.GetSimApp().ICAControllerKeeper.SubmitTx(sdk.WrapSDKContext(suite.chainA.GetContext()), msg)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
} else {
suite.Require().Error(err)
suite.Require().Nil(res)
}
})
}
}