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

chore: implement msg server for ChannelUpgradeTry #3791

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ea76c13
refactor: add rest of the handler code
colin-axner Jun 7, 2023
c0d923f
Merge remote-tracking branch 'origin/04-channel-upgrades' into colin/…
colin-axner Jun 7, 2023
a5d997e
fix: update upgrade tests
colin-axner Jun 7, 2023
d9a8999
fix: test fixes for upgradeTry
colin-axner Jun 7, 2023
fe0095d
fix: upgrade tests
colin-axner Jun 7, 2023
fa6598b
imp: add error code checking to tests
colin-axner Jun 7, 2023
0505850
Merge branch '04-channel-upgrades' into colin/3739-upgrade-try-handler
colin-axner Jun 7, 2023
75b52f1
chore: add todo
colin-axner Jun 7, 2023
ec3bb0c
Merge branch 'colin/3739-upgrade-try-handler' of github.com:cosmos/ib…
colin-axner Jun 7, 2023
89a8ce8
Update modules/core/04-channel/keeper/upgrade.go
colin-axner Jun 7, 2023
e1a7d62
adding msg server scaffolding for MsgChannelUpgradeTry
damiannolan Jun 7, 2023
a3a2ac5
updating logging and comments
damiannolan Jun 7, 2023
7e0ced8
Merge remote-tracking branch 'origin/04-channel-upgrades' into damian…
colin-axner Jun 8, 2023
1a326a5
Merge branch '04-channel-upgrades' into damian/3740-chan-upgrade-try-…
damiannolan Jun 8, 2023
b732ce3
removing flushStatus arg from WriteUpgradeTryChannel
damiannolan Jun 8, 2023
4462bd7
move upgradeVersion refreshing to WriteUpgradeTryChannel, tidy code s…
damiannolan Jun 8, 2023
98ce964
adding initial testcases for upgrade try msg server
damiannolan Jun 11, 2023
912b8da
adding testing app overrides for upgrade handshake app callback handlers
damiannolan Jun 11, 2023
b3042e0
adding app callback testcases and updating assertions
damiannolan Jun 11, 2023
ab18e7e
Merge branch '04-channel-upgrades' into damian/3740-chan-upgrade-try-…
damiannolan Jun 11, 2023
acc6b97
updating tests to include application state changes and assert they a…
damiannolan Jun 12, 2023
d6e358c
adding extra context to inline comment for upgrade cancellation subpr…
damiannolan Jun 12, 2023
bb422e4
updating WriteUpgradetryChannel godoc
damiannolan Jun 12, 2023
aaee7a7
add return args to WriteUpgradeTryChannel to fulfil response args
damiannolan Jun 12, 2023
809ff19
Merge branch '04-channel-upgrades' into damian/3740-chan-upgrade-try-…
damiannolan Jun 12, 2023
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
20 changes: 11 additions & 9 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,7 @@ func (k Keeper) ChanUpgradeTry(

// WriteUpgradeTryChannel writes a channel which has successfully passed the UpgradeTry handshake step.
// An event is emitted for the handshake step.
func (k Keeper) WriteUpgradeTryChannel(
ctx sdk.Context,
portID, channelID string,
proposedUpgrade types.Upgrade,
flushStatus types.FlushStatus,
) {
func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeVersion string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I can open another PR to replicate this on INIT - accepting the upgradeVersion as an arg and updating the upgrade inside this method as opposed to in the msg server

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for mutations in one function. Docstring could also mention that we set the upgrade in addition to the channel.

defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try")

channel, found := k.GetChannel(ctx, portID, channelID)
Expand All @@ -195,13 +190,20 @@ func (k Keeper) WriteUpgradeTryChannel(

previousState := channel.State
channel.State = types.TRYUPGRADE
channel.FlushStatus = flushStatus
// TODO: determine flush status
// channel.FlushStatus = flushStatus
Comment on lines +193 to +194
Copy link
Member Author

Choose a reason for hiding this comment

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

should be addressed in a future PR.


upgrade.Fields.Version = upgradeVersion

k.SetChannel(ctx, portID, channelID, channel)
k.SetUpgrade(ctx, portID, channelID, proposedUpgrade)
k.SetUpgrade(ctx, portID, channelID, upgrade)

k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.TRYUPGRADE.String())
emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade)
emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, upgrade)
}

func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Pending #3728

return nil
}

// startFlushUpgradeHandshake will verify the counterparty proposed upgrade and the current channel state.
Expand Down
49 changes: 48 additions & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,54 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC

// ChannelUpgradeTry defines a rpc handler method for MsgChannelUpgradeTry.
func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeTry) (*channeltypes.MsgChannelUpgradeTryResponse, error) {
return nil, nil
ctx := sdk.UnwrapSDKContext(goCtx)

module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId)
if err != nil {
ctx.Logger().Error("channel upgrade try failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id"))
return nil, errorsmod.Wrap(err, "could not retrieve module from port-id")
}
chatton marked this conversation as resolved.
Show resolved Hide resolved

cbs, ok := k.Router.GetRoute(module)
if !ok {
ctx.Logger().Error("channel upgrade try failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module))
return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)
}

upgrade, err := k.ChannelKeeper.ChanUpgradeTry(ctx, msg.PortId, msg.ChannelId, msg.ProposedUpgradeConnectionHops, msg.UpgradeTimeout, msg.CounterpartyProposedUpgrade, msg.CounterpartyUpgradeSequence, msg.ProofChannel, msg.ProofUpgrade, msg.ProofHeight)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
if upgradeErr, ok := err.(*channeltypes.UpgradeError); ok {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr); err != nil {
return nil, err
}

// NOTE: a FAILURE result is returned to the client and an error receipt is written to state.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return &channeltypes.MsgChannelUpgradeTryResponse{Result: channeltypes.FAILURE}, nil
}

// NOTE: an error is returned to baseapp and transaction state is not committed.
return nil, err
}

cacheCtx, writeFn := ctx.CacheContext()
Copy link
Member Author

Choose a reason for hiding this comment

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

state changes made the by app are not committed on error of the app callback, and instead, an error receipt is written for the current upgrade sequence.

upgradeVersion, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version)
if err != nil {
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

guess we should also be logging this error case? Similarly log a successful upgradetry before we return in 796.

}

return &channeltypes.MsgChannelUpgradeTryResponse{Result: channeltypes.FAILURE}, nil
}

writeFn()

k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, upgradeVersion)

return &channeltypes.MsgChannelUpgradeTryResponse{
Result: channeltypes.SUCCESS,
ChannelId: msg.ChannelId,
Upgrade: upgrade,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this will return an upgrade with the passed-in version. Doesn't it make sense to return with the version set by callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

aha, yes indeed! nice catch

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Upgrade set here should have updated version

Copy link
Member Author

Choose a reason for hiding this comment

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

Two potential ways of handling this:

  1. pass Upgrade to the WriteUpgradeTryChannel method as a reference, pointer type.
  2. Return the Upgrade or even possibly the entire response from the WriteUpgradeTryChannel method.

do you guys favour either?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also go for this returning the upgrade and the sequence for the response

func (k Keeper) WriteUpgradeTryChannel(...) (Upgrade, uint64)

Copy link
Member

Choose a reason for hiding this comment

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

You could return the entire response I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, opted for going with returning (Channel, Upgrade) for now, opened #3825

}, nil
Copy link
Member Author

Choose a reason for hiding this comment

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

Upgrade sequence is defined in the response but requires a channel lookup in the msg server as its no longer returned from ChanUpgradeTry. I'm not a massive fan of doing that lookup at this level. Upgrade sequence is still emitted in events.

anyone have any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Im ok with removing that from response

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with not doing the lookup here, but maybe we could abstract into a function:

return formatUpgradeTrySuccessResponse()

or something. No strong preference, happy to remove now and add in the future upon request

}

// ChannelUpgradeAck defines a rpc handler method for MsgChannelUpgradeAck.
Expand Down
143 changes: 143 additions & 0 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package keeper_test

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
Expand Down Expand Up @@ -774,3 +777,143 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
}
}
}

func (suite *KeeperTestSuite) TestChannelUpgradeTry() {
var (
path *ibctesting.Path
msg *channeltypes.MsgChannelUpgradeTry
)

cases := []struct {
name string
malleate func()
expResult func(res *channeltypes.MsgChannelUpgradeTryResponse, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] usually I would imagine expResult is the expected result, but this is an assertion function. Maybe assertionFn?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a fair point, I think we have this test setup in some extra places in the codebase using expResult, so maybe we can do one swoop to change all if people feel its worth it.

(I know we have inconsistencies with expPass, expError and expResult in different places - maybe we can keep some housekeeping work for a rainy day (?))

Copy link
Contributor

Choose a reason for hiding this comment

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

assertionFn sounds nice to me. Agree it should be done in a single swoop

}{
{
"success",
func() {},
func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.SUCCESS, res.Result)

channel := path.EndpointB.GetChannel()
suite.Require().Equal(channeltypes.TRYUPGRADE, channel.State)
suite.Require().Equal(uint64(1), channel.UpgradeSequence)
},
},
{
"module capability not found",
func() {
msg.PortId = "invalid-port"
msg.ChannelId = "invalid-channel"
},
func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) {
suite.Require().Error(err)
suite.Require().Nil(res)

suite.Require().ErrorIs(err, capabilitytypes.ErrCapabilityNotFound)
},
},
{
"elapsed upgrade timeout returns error",
func() {
msg.UpgradeTimeout = channeltypes.NewTimeout(clienttypes.NewHeight(1, 10), 0)
suite.coordinator.CommitNBlocks(suite.chainB, 100)
},
func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) {
suite.Require().Error(err)
suite.Require().Nil(res)
suite.Require().ErrorIs(err, channeltypes.ErrInvalidUpgrade)

errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().Empty(errorReceipt)
suite.Require().False(found)
},
},
{
"unsynchronized upgrade sequence writes upgrade error receipt",
func() {
channel := path.EndpointB.GetChannel()
channel.UpgradeSequence = 100

path.EndpointB.SetChannel(channel)
},
func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) {
suite.Require().NoError(err)

suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.FAILURE, res.Result)

// TODO: assert error receipt exists for the upgrade sequence when RestoreChannel / AbortUpgrade is called
// errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
// suite.Require().True(found)
},
},
{
"application callback error writes upgrade error receipt",
func() {
suite.chainB.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeTry = func(
ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string,
) (string, error) {
return "", fmt.Errorf("mock app callback failed")
}
},
func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) {
suite.Require().NoError(err)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.FAILURE, res.Result)

// TODO: assert error receipt exists for the upgrade sequence when RestoreChannel / AbortUpgrade is called
// errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
// suite.Require().True(found)
},
},
}

for _, tc := range cases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

// configure the channel upgrade version on testing endpoints
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion

err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointB.UpdateClient()
suite.Require().NoError(err)

counterpartySequence := path.EndpointA.GetChannel().UpgradeSequence
counterpartyUpgrade, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found)

proofChannel, proofUpgrade, proofHeight := path.EndpointB.QueryChannelUpgradeProof()

msg = &channeltypes.MsgChannelUpgradeTry{
PortId: path.EndpointB.ChannelConfig.PortID,
ChannelId: path.EndpointB.ChannelID,
ProposedUpgradeConnectionHops: []string{ibctesting.FirstConnectionID},
UpgradeTimeout: channeltypes.NewTimeout(clienttypes.NewHeight(1, 100), 0),
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
CounterpartyUpgradeSequence: counterpartySequence,
CounterpartyProposedUpgrade: counterpartyUpgrade,
ProofChannel: proofChannel,
ProofUpgrade: proofUpgrade,
ProofHeight: proofHeight,
Signer: suite.chainB.SenderAccount.GetAddress().String(),
}

tc.malleate()

res, err := suite.chainB.GetSimApp().GetIBCKeeper().ChannelUpgradeTry(suite.chainB.GetContext(), msg)

tc.expResult(res, err)
})
}
}
36 changes: 36 additions & 0 deletions testing/mock/ibc_app.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,42 @@ type IBCApp struct {
packet channeltypes.Packet,
relayer sdk.AccAddress,
) error

OnChanUpgradeInit func(
ctx sdk.Context,
portID, channelID string,
order channeltypes.Order,
connectionHops []string,
sequence uint64,
version, previousVersion string,
) (string, error)

OnChanUpgradeTry func(
ctx sdk.Context,
portID, channelID string,
order channeltypes.Order,
connectionHops []string,
counterpartyVersion string,
) (string, error)

OnChanUpgradeAck func(
ctx sdk.Context,
portID,
channelID,
counterpartyVersion string,
) error

OnChanUpgradeOpen func(
ctx sdk.Context,
portID,
channelID string,
) error

OnChanUpgradeRestore func(
ctx sdk.Context,
portID,
channelID string,
) error
}

// NewIBCApp returns a IBCApp. An empty PortID indicates the mock app doesn't bind/claim ports.
Expand Down
20 changes: 20 additions & 0 deletions testing/mock/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,26 +164,46 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet,

// OnChanUpgradeInit implements the IBCModule interface
func (im IBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, sequence uint64, version, previousVersion string) (string, error) {
if im.IBCApp.OnChanUpgradeInit != nil {
return im.IBCApp.OnChanUpgradeInit(ctx, portID, channelID, order, connectionHops, sequence, version, previousVersion)
}

return version, nil
}

// OnChanUpgradeTry implements the IBCModule interface
func (im IBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string) (string, error) {
if im.IBCApp.OnChanUpgradeTry != nil {
return im.IBCApp.OnChanUpgradeTry(ctx, portID, channelID, order, connectionHops, counterpartyVersion)
}

return counterpartyVersion, nil
}

// OnChanUpgradeAck implements the IBCModule interface
func (im IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
if im.IBCApp.OnChanUpgradeAck != nil {
return im.IBCApp.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion)
}

return nil
}

// OnChanUpgradeOpen implements the IBCModule interface
func (im IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string) error {
if im.IBCApp.OnChanUpgradeOpen != nil {
return im.IBCApp.OnChanUpgradeOpen(ctx, portID, channelID)
}

return nil
}

// OnChanUpgradeRestore implements the IBCModule interface
func (im IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) error {
if im.IBCApp.OnChanUpgradeRestore != nil {
return im.IBCApp.OnChanUpgradeRestore(ctx, portID, channelID)
}

return nil
}

Expand Down
2 changes: 2 additions & 0 deletions testing/simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ type SimApp struct {

// make IBC modules public for test purposes
// these modules are never directly routed to by the IBC Router
IBCMockModule ibcmock.IBCModule
ICAAuthModule ibcmock.IBCModule
FeeMockModule ibcmock.IBCModule

Expand Down Expand Up @@ -455,6 +456,7 @@ func NewSimApp(

// The mock module is used for testing IBC
mockIBCModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp(ibcmock.ModuleName, scopedIBCMockKeeper))
app.IBCMockModule = mockIBCModule
ibcRouter.AddRoute(ibcmock.ModuleName, mockIBCModule)

// Create Transfer Stack
Expand Down