-
Notifications
You must be signed in to change notification settings - Fork 636
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
Changes from 7 commits
09c053a
0a83377
5168cfd
c83cb4c
d64f950
06a5bd6
743001a
9bed0f5
084712a
74c9fbc
00c76d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{} | ||
|
@@ -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) { | ||
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) | ||
} | ||
|
||
chanCap, found := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, channelID)) | ||
if !found { | ||
return nil, sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") | ||
} | ||
|
||
_, err = k.SendTx(ctx, chanCap, msg.ConnectionId, portID, msg.PacketData, msg.TimeoutTimestamp) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can include the sequence in the |
||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &types.MsgSubmitTxResponse{}, 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" | ||
icacontrollertypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think we normally alias this as just |
||
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 *icacontrollertypes.MsgRegisterAccount | ||
expectedChannelID = "channel-0" | ||
) | ||
|
||
|
@@ -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 = icacontrollertypes.NewMsgRegisterAccount(ibctesting.FirstConnectionID, ibctesting.TestAccAddress, "") | ||
|
||
tc.malleate() | ||
|
||
|
@@ -85,3 +92,108 @@ func (suite *KeeperTestSuite) TestRegisterAccount() { | |
} | ||
} | ||
} | ||
|
||
func (suite *KeeperTestSuite) TestSubmitTx() { | ||
var ( | ||
path *ibctesting.Path | ||
owner string | ||
connectionID string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine to leave as is 🤝 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. set to |
||
}, | ||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use clienttypes.ZeroHeight() instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could expose the 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) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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