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

Implement MsgChannelUpgradeCancel message server handler #3848

Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
5cd004e
wip: adding restoreChannel and writeErrorReceipt functions
chatton Jun 1, 2023
cbc8e32
merge 04-channel-upgrades
chatton Jun 6, 2023
d01355f
emit error receipt events on abort
chatton Jun 6, 2023
6f7940b
remove unused function
chatton Jun 6, 2023
6d50848
added happy path test for abort
chatton Jun 6, 2023
795cd05
fleshed out TestAbortHandshake
chatton Jun 7, 2023
dfbc5d4
corrected docstring and removed unrequired checks in test
chatton Jun 7, 2023
b2a99ca
merge feature branch
chatton Jun 7, 2023
d429514
chore: added implementation of WriteUpgradeCancelChannel
chatton Jun 7, 2023
4330c51
Merge branch '04-channel-upgrades' into cian/issue#3649-implement-res…
chatton Jun 7, 2023
c1e05b8
fix linting
chatton Jun 7, 2023
619381b
Merge branch 'cian/issue#3649-implement-restorechannel' of https://gi…
chatton Jun 7, 2023
91605c1
addressing PR feedback
chatton Jun 7, 2023
d050ee8
merge with restore branch
chatton Jun 7, 2023
eaa2b91
addressing PR feedback
chatton Jun 7, 2023
d6bd3bf
merge 04-channel-upgrades
chatton Jun 7, 2023
cfe59ae
Merge branch 'cian/issue#3649-implement-restorechannel' into cian/iss…
chatton Jun 7, 2023
7d2c3d4
chore: adding implementation of ChanUpgradeCancel
chatton Jun 7, 2023
099b147
Merge branch '04-channel-upgrades' into cian/issue#3649-implement-res…
chatton Jun 7, 2023
759474f
added scaffolding for TestChanUpgradeCancel
chatton Jun 7, 2023
3916332
addressing PR feedback
chatton Jun 12, 2023
700acba
merge 04-channel-upgrades
chatton Jun 12, 2023
6b2ce2e
Merge branch 'cian/issue#3649-implement-restorechannel' into cian/iss…
chatton Jun 12, 2023
811c418
addressing PR feedback
chatton Jun 12, 2023
0f0305f
Merge branch '04-channel-upgrades' into cian/issue#3752-implement-wri…
chatton Jun 12, 2023
d6bd69c
merge 04-channel-upgrades
chatton Jun 14, 2023
51e9070
fix merge conflicts
chatton Jun 14, 2023
3e0bf6e
ran linter
chatton Jun 14, 2023
4e12bc0
merge 04-channel-upgrades
chatton Jun 14, 2023
df123ba
merge 04-channel-upgrades
chatton Jun 14, 2023
098d0a3
happy path test
chatton Jun 14, 2023
853eae2
adding tests for ChanUpgradeCancel
chatton Jun 14, 2023
c2ae441
add ChannelCancelUpgrade message server implementation
chatton Jun 14, 2023
85afe0f
chore: add msg server tests for ChannelUpgradeCancel
chatton Jun 14, 2023
8ced59b
chore: merge 04-channel-upgrades
chatton Jun 14, 2023
bf10856
addresing PR feedback
chatton Jun 16, 2023
0e34c52
Merge branch '04-channel-upgrades' into cian/issue#3753-implement-cha…
chatton Jun 16, 2023
8405547
Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…
chatton Jun 16, 2023
fb3897c
Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…
chatton Jun 16, 2023
2f540ce
corrected assertion arguments
chatton Jun 16, 2023
f470ffd
Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…
chatton Jun 16, 2023
78fb32b
address PR feedback
chatton Jun 16, 2023
1550e6f
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 16, 2023
ffc1b6f
addressing PR feedback
chatton Jun 16, 2023
415e66a
fix linter
chatton Jun 16, 2023
cc8cb48
re-added commented out test
chatton Jun 16, 2023
8ffceb8
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 19, 2023
269d9e0
pass the new upgrade sequence as an argument to restoreChannel
chatton Jun 19, 2023
2717582
rename variable to be more clear
chatton Jun 19, 2023
51335bd
Merge branch 'cian/issue#3754-implement-msgchannelupgradecancel-handl…
chatton Jun 19, 2023
d023ac0
use error receipt Sequence instead of sequence + 1
chatton Jun 19, 2023
04ac212
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 19, 2023
a3bbf80
addressing PR feedback
chatton Jun 19, 2023
8c58850
merge 04-channel-upgrades
chatton Jun 19, 2023
6f9f680
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 20, 2023
c36673a
addressing PR feedback
chatton Jun 20, 2023
0748052
Merge branch 'cian/issue#3754-implement-msgchannelupgradecancel-handl…
chatton Jun 20, 2023
cc45fe5
handle merge conflicts
chatton Jun 20, 2023
f2782b3
appease linter
chatton Jun 20, 2023
95c97db
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 20, 2023
0f3bc84
added test for capability not found
chatton Jun 21, 2023
fa56a67
use msg.ErrorReceipt.Sequence instead of returning from keeper fn
chatton 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
30 changes: 29 additions & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,5 +820,33 @@ func (k Keeper) ChannelUpgradeTimeout(goCtx context.Context, msg *channeltypes.M

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

module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId)
Copy link
Member

Choose a reason for hiding this comment

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

we should add a testcase for module not found here I think!

if err != nil {
ctx.Logger().Error("channel upgrade cancel 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 cancel 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)
}

if err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight); err != nil {
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error())
return nil, errorsmod.Wrap(err, "channel upgrade cancel failed")
}

if err := cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I missed the discussing about the naming, but what about onChanUpgradeCancel instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it makes more sense for the applications to call a restore function, as they are just going to be handling any restoring to previous state, and not cancelling anything like a handshake on the IBC core level.

ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", errorsmod.Wrap(err, "channel upgrade restore callback failed"))
return nil, errorsmod.Wrapf(err, "channel upgrade restore callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId)
}

k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId)

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

return &channeltypes.MsgChannelUpgradeCancelResponse{}, nil
}
136 changes: 136 additions & 0 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -924,3 +924,139 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() {
})
}
}

func (suite *KeeperTestSuite) TestChannelUpgradeCancel() {
var (
path *ibctesting.Path
msg *channeltypes.MsgChannelUpgradeCancel
)

cases := []struct {
name string
malleate func()
expErr error
}{
{
name: "success",
malleate: func() {},
expErr: nil,
},
{
name: "invalid proof",
malleate: func() {
msg.ProofErrorReceipt = []byte("invalid proof")
},
expErr: commitmenttypes.ErrInvalidProof,
},
{
name: "invalid error receipt sequence",
malleate: func() {
const invalidSequence = 0

errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

errorReceipt.Sequence = invalidSequence

// overwrite the error receipt with an invalid sequence.
suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)

// ensure that the error receipt is committed to state.
suite.coordinator.CommitBlock(suite.chainB)
suite.Require().NoError(path.EndpointA.UpdateClient())

// retrieve the error receipt proof and proof height.
upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey)

// provide a valid proof of the error receipt with an invalid sequence.
msg.ErrorReceipt.Sequence = invalidSequence
msg.ProofErrorReceipt = errorReceiptProof
msg.ProofHeight = proofHeight
},
expErr: channeltypes.ErrInvalidUpgradeSequence,
},
{
name: "application callback fails",
malleate: func() {
suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeRestore = func(ctx sdk.Context, portID, channelID string) error {
// return an error type that is not returned in the regular flow.
return upgradetypes.ErrIntOverflowUpgrade
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
},
// error should be what the application callback returned.
expErr: upgradetypes.ErrIntOverflowUpgrade,
},
}
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved

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

suite.Require().NoError(path.EndpointA.ChanUpgradeInit())

// fetch the previous channel when it is in the INITUPGRADE state.
prevChannel := path.EndpointA.GetChannel()

suite.Require().NoError(path.EndpointB.UpdateClient())
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// cause the upgrade to fail on chain b so an error receipt is written.
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")
}

suite.Require().NoError(path.EndpointB.ChanUpgradeTry())

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

upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

msg = &channeltypes.MsgChannelUpgradeCancel{
PortId: path.EndpointA.ChannelConfig.PortID,
ChannelId: path.EndpointA.ChannelID,
ErrorReceipt: errorReceipt,
ProofErrorReceipt: errorReceiptProof,
ProofHeight: proofHeight,
Signer: suite.chainA.SenderAccount.GetAddress().String(),
}

tc.malleate()

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

expPass := tc.expErr == nil
if expPass {
suite.Require().NoError(err)
channel := path.EndpointA.GetChannel()
suite.Require().Equal(prevChannel.Version, channel.Version, "channel version should be reverted")
suite.Require().Equalf(channeltypes.OPEN, channel.State, "channel state should be %s", channeltypes.OPEN.String())
suite.Require().Equalf(channeltypes.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", channeltypes.NOTINFLUSH.String())
suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "channel upgrade sequence should be incremented")
} else {
suite.Require().Nil(res)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().ErrorIs(err, tc.expErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think maybe here we should check that the channel version, state, flushstatus has not been mutated/upgraded sequence has not been incremented


channel := path.EndpointA.GetChannel()

suite.Require().Equal(prevChannel.Version, channel.Version, "channel version should not be changed")
suite.Require().Equalf(prevChannel.State, channel.State, "channel state should be %s", prevChannel.State.String())
suite.Require().Equalf(prevChannel.FlushStatus, channel.FlushStatus, "channel flush status should be %s", prevChannel.FlushStatus.String())
// TODO suite.Require().Equal(prevChannel.UpgradeSequence, channel.UpgradeSequence, "channel upgrade sequence should not incremented")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a tricky one, there is a state update happening in k.ChannelKeeper.ChanUpgradeCancel so when the application callback errors, the upgrade sequence is incremented but this should not actually be the case. This only happens here as we call ChannelUpgradeCancel. In practice this state would still be reverted when an error returns from the message server.

I think this is another reason to implement this

What do people think about moving the upgrade sequence updating logic into the write function?

}
})
}
}