-
Notifications
You must be signed in to change notification settings - Fork 639
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
Changes from 20 commits
ea76c13
c0d923f
a5d997e
d9a8999
fe0095d
fa6598b
0505850
75b52f1
ec3bb0c
89a8ce8
e1a7d62
a3a2ac5
7e0ced8
1a326a5
b732ce3
4462bd7
98ce964
912b8da
b3042e0
ab18e7e
acc6b97
d6e358c
bb422e4
aaee7a7
809ff19
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 |
---|---|---|
|
@@ -180,12 +180,7 @@ func (k Keeper) ChanUpgradeTry( | |
|
||
// WriteUpgradeTryChannel writes a channel which has successfully passed 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) { | ||
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try") | ||
|
||
channel, found := k.GetChannel(ctx, portID, channelID) | ||
|
@@ -195,13 +190,20 @@ 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
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. 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) | ||
} | ||
|
||
func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { | ||
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. Pending #3728 |
||
return nil | ||
} | ||
|
||
// startFlushUpgradeHandshake will verify the counterparty proposed upgrade and the current channel state. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -751,7 +751,54 @@ 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
|
||
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() | ||
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. 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 | ||
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. 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() | ||
|
||
k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, upgradeVersion) | ||
|
||
return &channeltypes.MsgChannelUpgradeTryResponse{ | ||
Result: channeltypes.SUCCESS, | ||
ChannelId: msg.ChannelId, | ||
Upgrade: upgrade, | ||
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. It looks like this will return an upgrade with the passed-in version. Doesn't it make sense to return with the version set by callback? 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. aha, yes indeed! nice catch 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. Agreed. Upgrade set here should have updated version 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. Two potential ways of handling this:
do you guys favour either? 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 could also go for this returning the upgrade and the sequence for the response func (k Keeper) WriteUpgradeTryChannel(...) (Upgrade, uint64) 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. You could return the entire response I think 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, opted for going with returning |
||
}, nil | ||
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. Upgrade sequence is defined in the response but requires a channel lookup in the msg server as its no longer returned from anyone have any thoughts on this? 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. Im ok with removing that from response 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 agree with not doing the lookup here, but maybe we could abstract into a function: return formatUpgradeTrySuccessResponse() or something. No strong preference, happy to remove now and add in the future upon request |
||
} | ||
|
||
// ChannelUpgradeAck defines a rpc handler method for MsgChannelUpgradeAck. | ||
|
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" | ||
|
@@ -774,3 +777,143 @@ 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) | ||
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. [nit] usually I would imagine 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. that's a fair point, I think we have this test setup in some extra places in the codebase using (I know we have inconsistencies 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.
|
||
}{ | ||
{ | ||
"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) { | ||
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) | ||
}, | ||
}, | ||
} | ||
|
||
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(clienttypes.NewHeight(1, 100), 0), | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) | ||
}) | ||
} | ||
} |
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 can open another PR to replicate this on INIT - accepting the
upgradeVersion
as an arg and updating the upgrade inside this method as opposed to in the msg serverThere 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.
+1 for mutations in one function. Docstring could also mention that we set the upgrade in addition to the channel.