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 all 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
17 changes: 7 additions & 10 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string

// 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) {
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)
Expand All @@ -222,7 +222,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str

previousState := channel.State

k.restoreChannel(ctx, portID, channelID, channel)
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)
Expand Down Expand Up @@ -354,9 +354,6 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err
return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "error receipt sequence (%d) must be greater than or equal to current sequence (%d)", counterpartySequence, currentSequence)
}

channel.UpgradeSequence = errorReceipt.Sequence + 1
k.SetChannel(ctx, portID, channelID, channel)

return nil
}

Expand Down Expand Up @@ -715,7 +712,9 @@ func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err erro
return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

k.restoreChannel(ctx, portID, channelID, channel)
// the channel upgrade sequence has already been updated in ChannelUpgradeTry, so we can pass
// its updated value.
k.restoreChannel(ctx, portID, channelID, channel.UpgradeSequence, channel)

// in the case of application callbacks, the error may not be an upgrade error.
// in this case we need to construct one in order to write the error receipt.
Expand All @@ -728,16 +727,14 @@ func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err erro
return err
}

// TODO: callback execution
// cbs.OnChanUpgradeRestore()

return nil
}

// restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted.
func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string, currentChannel types.Channel) {
func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string, upgradeSequence uint64, currentChannel types.Channel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see 2 patterns for changing the upgrade sequence currently:

  • upon upgradeTRY, if we reach a sequence mismatch and we error on a higher sequence, we update the channel immediately, this change is referenced in restoreChannel
  • upon cancellation, if the counterparty errors on a higher sequence, we update the channel in restore

Both seem like fine options to me, but might be nice to be explicit about which ones we use. I guess the former is a little defensive to update as soon as possible and the latter doesn't really have a chance to be defensive

What do you think? Noting as an observation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinking a bit more, I don't actually like that we are simply referencing channel.UpgradeSequence and passing it to restoreChannel in the case of UpgradeTry. This makes it look like the upgrade sequence is not changing when you look at the restore call in isolation.

I think I'm happy to stick with the current approach, but it is maybe worth adding a comment in the try abort situation to highlight that the upgrade sequence has already been updated.

currentChannel.State = types.OPEN
currentChannel.FlushStatus = types.NOTINFLUSH
currentChannel.UpgradeSequence = upgradeSequence

k.SetChannel(ctx, portID, channelID, currentChannel)

Expand Down
2 changes: 0 additions & 2 deletions modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1183,8 +1183,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
channel := path.EndpointA.GetChannel()
suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "upgrade sequence should be incremented")
} else {
suite.Require().ErrorIs(err, tc.expError)
}
Expand Down
27 changes: 26 additions & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,5 +820,30 @@ 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")
}

cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)

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

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

return &channeltypes.MsgChannelUpgradeCancelResponse{}, nil
}
129 changes: 129 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,132 @@ 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.
errorReceiptProof, proofHeight := path.EndpointB.QueryProof(host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))

// 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: "capability not found",
malleate: func() {
msg.ChannelId = ibctesting.InvalidID
},
expErr: capabilitytypes.ErrCapabilityNotFound,
},
}
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()

// 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 := path.EndpointB.QueryProof(upgradeErrorReceiptKey)

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, channel.UpgradeSequence, "channel upgrade sequence should be set to error receipt sequence")
} 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())
suite.Require().Equal(prevChannel.UpgradeSequence, channel.UpgradeSequence, "channel upgrade sequence should not incremented")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
})
}
}
7 changes: 7 additions & 0 deletions testing/mock/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ import (
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

// applicationCallbackError is a custom error type that will be unique for testing purposes.
type applicationCallbackError struct{}
Copy link
Member

Choose a reason for hiding this comment

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

is this being used anywhere? can't we just override callbacks and return an error with fmt.Errorf?

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 was added because there used to be a testcase that checked the exact error type returned by the OnChanUpgradeRestore application callback. However that function no longer returns an error.

The idea behind this was that we can do suite.Require().ErrorIs(err) to assert a custom error type with fmt.Errorf, it would just be a regular go error and we could run into false positives.

While not used in this PR, I think we could replace the usage of our failed application callback test cases with this. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Aye! makes sense for strict error checking in testcases, happy to keep it around if that is the plan but we would likely need it to be public to do overrides in testcases and make the assertion against this type.

Copy link
Member

Choose a reason for hiding this comment

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

oh, nevermind, I see its exposed with assignment to MockApplicationCallbackError in mock.go 👍


func (e applicationCallbackError) Error() string {
return "mock application callback failed"
}

// IBCModule implements the ICS26 callbacks for testing/mock.
type IBCModule struct {
appModule *AppModule
Expand Down
3 changes: 3 additions & 0 deletions testing/mock/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ var (
MockAckCanaryCapabilityName = "mock acknowledgement canary capability name"
MockTimeoutCanaryCapabilityName = "mock timeout canary capability name"
UpgradeVersion = fmt.Sprintf("%s-v2", Version)
// MockApplicationCallbackError should be returned when an application callback should fail. It is possible to
// test that this error was returned using ErrorIs.
MockApplicationCallbackError error = &applicationCallbackError{}
)

var _ porttypes.IBCModule = IBCModule{}
Expand Down
Loading