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 all 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
24 changes: 14 additions & 10 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,9 @@ func (k Keeper) ChanUpgradeTry(
return upgrade, nil
}

// WriteUpgradeTryChannel writes a channel which has successfully passed the UpgradeTry handshake step.
// WriteUpgradeTryChannel writes the channel end and upgrade to state after successfully passing 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) (types.Channel, types.Upgrade) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try")

channel, found := k.GetChannel(ctx, portID, channelID)
Expand All @@ -195,13 +190,22 @@ 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
Contributor 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)

return channel, upgrade
}

func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error {
Copy link
Contributor 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
51 changes: 50 additions & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,56 @@ 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
// This signals to the relayer to begin the cancel upgrade handshake subprotocol.
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
Contributor 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()

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

return &channeltypes.MsgChannelUpgradeTryResponse{
Result: channeltypes.SUCCESS,
ChannelId: msg.ChannelId,
Upgrade: upgrade,
UpgradeSequence: channel.UpgradeSequence,
}, nil
}

// ChannelUpgradeAck defines a rpc handler method for MsgChannelUpgradeAck.
Expand Down
150 changes: 150 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,150 @@ 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
Contributor 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) {
// set arbitrary value in store to mock application state changes
store := ctx.KVStore(suite.chainB.GetSimApp().GetKey(exported.ModuleName))
store.Set([]byte("foo"), []byte("bar"))
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)

// assert application state changes are not committed
store := suite.chainB.GetContext().KVStore(suite.chainB.GetSimApp().GetKey(exported.ModuleName))
suite.Require().False(store.Has([]byte("foo")))
},
},
}

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(path.EndpointA.Chain.GetTimeoutHeight(), 0),
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)
})
}
}