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: adding msg server implementation for ChannelUpgradeAck #3849

Merged
merged 32 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1293330
adding boilerplate skeleton for chanUpgradeAck handler
damiannolan May 31, 2023
c4dce22
Merge branch '04-channel-upgrades' into damian/1620-chan-upgrade-ack
damiannolan Jun 5, 2023
a8c6882
updating msg servers args
damiannolan Jun 5, 2023
c17c8ed
adding test scaffolding and syncing latest changes of feat branch
damiannolan Jun 5, 2023
152a873
Merge branch '04-channel-upgrades' into damian/1620-chan-upgrade-ack
damiannolan Jun 5, 2023
fbacd83
configure both proposed upgrades to use mock.UpgradeVersion
damiannolan Jun 6, 2023
dfaf589
Merge branch '04-channel-upgrades' into damian/1620-chan-upgrade-ack
damiannolan Jun 7, 2023
2834124
Merge branch '04-channel-upgrades' into damian/1620-chan-upgrade-ack
damiannolan Jun 12, 2023
f92edcb
updating chanUpgradeAck test cases
damiannolan Jun 12, 2023
0ae120f
updating var naming for consistency, adding additional testcases
damiannolan Jun 12, 2023
73e9352
rm msg server implementation
damiannolan Jun 12, 2023
2b8b3fe
adding invalid flush status err and rm lint ignore comment
damiannolan Jun 13, 2023
e53d1aa
adding test helpers to endpoint for get/set channel upgrade
damiannolan Jun 13, 2023
f9c2167
lint it
damiannolan Jun 13, 2023
566ab66
adding initial msg server impl skeleton
damiannolan Jun 13, 2023
c241a63
pull in code for WriteUpgradeAckChannel
damiannolan Jun 13, 2023
b1958fd
Merge branch '04-channel-upgrades' into damian/3742-upgrade-ack-msg-s…
damiannolan Jun 14, 2023
ec60447
adding result to MsgChannelUpgradeAckResponse
damiannolan Jun 14, 2023
b82afaf
Merge branch 'damian/update-ack-response-args' into damian/3742-upgra…
damiannolan Jun 14, 2023
7082cd8
add initial test cases
damiannolan Jun 14, 2023
80326a0
adding additional testcases
damiannolan Jun 14, 2023
c870ce1
Merge branch '04-channel-upgrades' into damian/3742-upgrade-ack-msg-s…
damiannolan Jun 14, 2023
cd3d443
Merge branch '04-channel-upgrades' into damian/3742-upgrade-ack-msg-s…
damiannolan Jun 20, 2023
d23c6f1
apply testcase naming review suggestions
damiannolan Jun 20, 2023
4ba1751
apply error return wrapping suggestions from review
damiannolan Jun 20, 2023
a710ee4
fix error to use Wrapf and correct channel id arg, adding success log
damiannolan Jun 20, 2023
e75f1ca
correct testing imports and satisy linter
damiannolan Jun 20, 2023
0169d98
apply self suggestions for testcase context with in-line comments
damiannolan Jun 21, 2023
9d71ee5
updating test func to use path.EndpointA and chainA sender acc
damiannolan Jun 21, 2023
970d006
Merge branch '04-channel-upgrades' into damian/3742-upgrade-ack-msg-s…
damiannolan Jun 21, 2023
d60bf1f
Merge branch '04-channel-upgrades' into damian/3742-upgrade-ack-msg-s…
damiannolan Jun 21, 2023
215280c
Merge branch '04-channel-upgrades' into damian/3742-upgrade-ack-msg-s…
damiannolan Jun 21, 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
60 changes: 34 additions & 26 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,29 +205,6 @@ func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string
return channel, upgrade
}

// WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process.Auxiliary upgrade state is
// also deleted.
func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string, newUpgradeSequence uint64) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel")

upgrade, found := k.GetUpgrade(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find upgrade when updating channel state, channelID: %s, portID: %s", channelID, portID))
}

channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID))
}

previousState := channel.State

k.restoreChannel(ctx, portID, channelID, newUpgradeSequence, channel)

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

// ChanUpgradeAck is called by a module to accept the ACKUPGRADE handshake step of the channel upgrade protocol.
// This method should only be called by the IBC core msg server.
// This method will verify that the counterparty has entered TRYUPGRADE
Expand Down Expand Up @@ -297,7 +274,7 @@ func (k Keeper) ChanUpgradeAck(
func (k Keeper) WriteUpgradeAckChannel(
ctx sdk.Context,
portID, channelID string,
proposedUpgrade types.Upgrade,
upgradeVersion string,
) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-ack")

Expand All @@ -311,10 +288,18 @@ func (k Keeper) WriteUpgradeAckChannel(
channel.FlushStatus = types.FLUSHING

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

upgrade, found := k.GetUpgrade(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("cound not find existing upgrade when updating channel state in successful ChanUpgradeAck step, channelID: %s, portID: %s", channelID, portID))
}

upgrade.Fields.Version = upgradeVersion

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.ACKUPGRADE.String())
emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, proposedUpgrade)
emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, upgrade)
}

// ChanUpgradeCancel is called by a module to cancel a channel upgrade that is in progress.
Expand Down Expand Up @@ -357,6 +342,29 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err
return nil
}

// WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process.Auxiliary upgrade state is
// also deleted.
func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string, newUpgradeSequence uint64) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel")

upgrade, found := k.GetUpgrade(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find upgrade when updating channel state, channelID: %s, portID: %s", channelID, portID))
}

channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID))
}

previousState := channel.State

k.restoreChannel(ctx, portID, channelID, newUpgradeSequence, channel)

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

// ChanUpgradeTimeout times out an outstanding upgrade.
// This should be used by the initialising chain when the counterparty chain has not responded to an upgrade proposal within the specified timeout period.
func (k Keeper) ChanUpgradeTimeout(
Expand Down
50 changes: 49 additions & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,55 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh

// ChannelUpgradeAck defines a rpc handler method for MsgChannelUpgradeAck.
func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeAck) (*channeltypes.MsgChannelUpgradeAckResponse, 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 ack 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")
}

cbs, ok := k.Router.GetRoute(module)
if !ok {
ctx.Logger().Error("channel upgrade ack 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)
}

err = k.ChannelKeeper.ChanUpgradeAck(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyFlushStatus, msg.CounterpartyUpgrade, msg.ProofChannel, msg.ProofUpgrade, msg.ProofHeight)
if err != nil {
ctx.Logger().Error("channel upgrade ack failed", "error", errorsmod.Wrap(err, "channel handshake upgrade ack failed"))
if upgradeErr, ok := err.(*channeltypes.UpgradeError); ok {
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr); err != nil {
return nil, errorsmod.Wrap(err, "channel upgrade ack (abort upgrade) failed")
}
Comment on lines +826 to +828
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets hammer down this flow in a followup and with offline discussion. Some discussion points

  • AbortUpgrade should never fail (AbortUpgrade returns an error despite being an non-failure action #3903)
  • OnChannelUpgradeRestore being handled in one location
  • timeout, cancel, try/ack all do portions of AbortUpgrade, primarily cancel doesn't need to write an upgrade receipt
  • potential caching context around 04-channel handlers if writes ever get moved inside of handlers (as opposed to being called at the end of the msg server handler)


// NOTE: a FAILURE result is returned to the client and an error receipt is written to state.
// This signals to the relayer to begin the cancel upgrade handshake subprotocol.
return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.FAILURE}, nil
}

// NOTE: an error is returned to baseapp and transaction state is not committed.
return nil, errorsmod.Wrap(err, "channel upgrade ack failed")
}

cacheCtx, writeFn := ctx.CacheContext()
err = cbs.OnChanUpgradeAck(cacheCtx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade.Fields.Version)
if err != nil {
ctx.Logger().Error("channel upgrade ack callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error())
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil {
return nil, errorsmod.Wrapf(err, "channel upgrade ack callback (abort upgrade) failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId)
}

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

writeFn()

k.ChannelKeeper.WriteUpgradeAckChannel(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade.Fields.Version)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add:

ctx.Logger().Info("channel upgrade ack succeeded", "channel-id", channelID, "port-id", msg.PortId)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good call. We should replicate the logging and error wrapping for the TRY handler in a follow up PR, looks like it needs it too

ctx.Logger().Info("channel upgrade ack succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)

return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.SUCCESS}, nil
}

// ChannelUpgradeOpen defines a rpc handler method for MsgChannelUpgradeOpen.
Expand Down
148 changes: 148 additions & 0 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,154 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() {
}
}

func (suite *KeeperTestSuite) TestChannelUpgradeAck() {
Copy link
Contributor

Choose a reason for hiding this comment

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

really nice test cases 🥇

var (
path *ibctesting.Path
msg *channeltypes.MsgChannelUpgradeAck
)

cases := []struct {
name string
malleate func()
expResult func(res *channeltypes.MsgChannelUpgradeAckResponse, err error)
}{
{
"success",
func() {},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.SUCCESS, res.Result)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(channeltypes.ACKUPGRADE, channel.State)
suite.Require().Equal(uint64(1), channel.UpgradeSequence)
},
},
{
"module capability not found",
func() {
msg.PortId = ibctesting.InvalidID
msg.ChannelId = ibctesting.InvalidID
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().Error(err)
suite.Require().Nil(res)

suite.Require().ErrorIs(err, capabilitytypes.ErrCapabilityNotFound)
},
},
{
"core handler returns error and no upgrade error receipt is written",
func() {
// force an error by overriding the counterparty flush status to an invalid value
msg.CounterpartyFlushStatus = channeltypes.NOTINFLUSH
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().Error(err)
suite.Require().Nil(res)
suite.Require().ErrorIs(err, channeltypes.ErrInvalidFlushStatus)

errorReceipt, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().Empty(errorReceipt)
suite.Require().False(found)
},
},
{
"core handler returns error and writes upgrade error receipt",
func() {
// force an upgrade error by modifying the channel upgrade ordering to an incompatible value
upgrade := path.EndpointA.GetChannelUpgrade()
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
upgrade.Fields.Ordering = channeltypes.NONE

path.EndpointA.SetChannelUpgrade(upgrade)
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().NoError(err)

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

errorReceipt, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found)
suite.Require().Equal(uint64(1), errorReceipt.Sequence)
},
},
{
"application callback returns error and error receipt is written",
func() {
suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeAck = func(
ctx sdk.Context, portID, channelID, counterpartyVersion string,
) error {
// set arbitrary value in store to mock application state changes
store := ctx.KVStore(suite.chainA.GetSimApp().GetKey(exported.ModuleName))
store.Set([]byte("foo"), []byte("bar"))
return fmt.Errorf("mock app callback failed")
}
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().NoError(err)

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

errorReceipt, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found)
suite.Require().Equal(uint64(1), errorReceipt.Sequence)

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

Choose a reason for hiding this comment

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

Maybe for completeness (and if it's not too difficult to add): should we also have a test where the application callback returns an error and AbortUpgrade also fails and returns an error (and no error receipt is thus written)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like its impossible to force the error here in the test setup. Since deleting the Upgrade or Channel in tc.malleate() will trigger a failure at an earlier point in k.ChannelKeeper.ChanUpgradeAck(), resulting in an error return to the client as opposed to following the abort scenario


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.ChanUpgradeTry()
suite.Require().NoError(err)

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

counterpartyChannel := path.EndpointB.GetChannel()
counterpartyUpgrade := path.EndpointB.GetChannelUpgrade()

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

msg = &channeltypes.MsgChannelUpgradeAck{
PortId: path.EndpointA.ChannelConfig.PortID,
ChannelId: path.EndpointA.ChannelID,
CounterpartyFlushStatus: counterpartyChannel.FlushStatus,
CounterpartyUpgrade: counterpartyUpgrade,
ProofChannel: proofChannel,
ProofUpgrade: proofUpgrade,
ProofHeight: proofHeight,
Signer: suite.chainA.SenderAccount.GetAddress().String(),
}

tc.malleate()

res, err := suite.chainA.GetSimApp().GetIBCKeeper().ChannelUpgradeAck(suite.chainA.GetContext(), msg)

tc.expResult(res, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

the plan was to rename this to assertResultFn / assertFn in a follow up right? (not a huge deal either way)

Copy link
Member Author

Choose a reason for hiding this comment

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

yup sure!

})
}
}

func (suite *KeeperTestSuite) TestChannelUpgradeCancel() {
var (
path *ibctesting.Path
Expand Down