-
Notifications
You must be signed in to change notification settings - Fork 609
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
Changes from 60 commits
5cd004e
cbc8e32
d01355f
6f7940b
6d50848
795cd05
dfbc5d4
b2a99ca
d429514
4330c51
c1e05b8
619381b
91605c1
d050ee8
eaa2b91
d6bd3bf
cfe59ae
7d2c3d4
099b147
759474f
3916332
700acba
6b2ce2e
811c418
0f0305f
d6bd69c
51e9070
3e0bf6e
4e12bc0
df123ba
098d0a3
853eae2
c2ae441
85afe0f
8ced59b
bf10856
0e34c52
8405547
fb3897c
2f540ce
f470ffd
78fb32b
1550e6f
ffc1b6f
415e66a
cc8cb48
8ffceb8
269d9e0
2717582
51335bd
d023ac0
04ac212
a3bbf80
8c58850
6f9f680
c36673a
0748052
cc45fe5
f2782b3
95c97db
0f3bc84
fa56a67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -820,5 +820,31 @@ 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
||
newUpgradeSequence, err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch! We used to do |
||
if 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, newUpgradeSequence) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming feels a little janky with this If I look at the implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think maybe it does feel a little off, it matches the convention we've been using with the I think |
||
|
||
ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId) | ||
|
||
return &channeltypes.MsgChannelUpgradeCancelResponse{}, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -924,3 +924,125 @@ 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, | ||
}, | ||
} | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The idea behind this was that we can do While not used in this PR, I think we could replace the usage of our failed application callback test cases with this. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, nevermind, I see its exposed with assignment to |
||
|
||
func (e applicationCallbackError) Error() string { | ||
return "mock application callback failed" | ||
} | ||
|
||
// IBCModule implements the ICS26 callbacks for testing/mock. | ||
type IBCModule struct { | ||
appModule *AppModule | ||
|
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
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 torestoreChannel
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.