From bc0f5e44301cc34a0f2a0b120902bd668d5b13ae Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 16 Jan 2024 13:27:53 +0100 Subject: [PATCH] remaining review comments from #1613 (backport #5539) (#5617) * remaining review comments from #1613 (#5539) * e2e test: send incentivised packet after upgrade, add extra tests for cbs * update hermes docker image * add prune acknowledgements to successful upgrade test * use correct tx response * getting further with the e2e test, addressing a couple of other review items * refactor test to use sync incentivization instead of async * update hermes image tag * revert change that was breaking a test * Apply suggestions from code review Co-authored-by: Damian Nolan Co-authored-by: DimitrisJim * rename variables for consistency * rename variables for clarification --------- Co-authored-by: Damian Nolan Co-authored-by: DimitrisJim (cherry picked from commit 5d772213da10f17c9be02f7889bc25fb31df371e) # Conflicts: # e2e/README.md # e2e/tests/core/04-channel/upgrades_test.go # e2e/tests/transfer/incentivized_test.go # e2e/testsuite/grpc_query.go # e2e/testsuite/tx.go * remove e2e folder --------- Co-authored-by: Carlos Rodriguez --- .../controller/ibc_middleware_test.go | 27 ++++++++++ .../host/ibc_module_test.go | 52 +++++++++++++++++++ modules/core/04-channel/types/msgs.go | 4 +- 3 files changed, 82 insertions(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index 568edb2b031..fb5d54bf740 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -863,6 +863,33 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { } } +// OnChanUpgradeTry callback returns error on controller chains +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + // call application callback directly + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + app, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) + suite.Require().True(ok) + + version, err := cbs.OnChanUpgradeTry( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + path.EndpointA.ChannelConfig.Order, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.Version, + ) + suite.Require().Error(err) + suite.Require().ErrorIs(err, icatypes.ErrInvalidChannelFlow) + suite.Require().Equal("", version) +} + func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { var ( path *ibctesting.Path diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index f5fdde6c5c1..3491f47d52d 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -604,6 +604,33 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { } } +// OnChanUpgradeInit callback returns error on host chains +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + // call application callback directly + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + app, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) + suite.Require().True(ok) + + version, err := cbs.OnChanUpgradeInit( + suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, + path.EndpointB.ChannelConfig.Order, []string{path.EndpointB.ConnectionID}, path.EndpointB.ChannelConfig.Version, + ) + + suite.Require().Error(err) + suite.Require().ErrorIs(err, icatypes.ErrInvalidChannelFlow) + suite.Require().Equal("", version) +} + func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { testCases := []struct { name string @@ -672,6 +699,31 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { } } +// OnChanUpgradeAck callback returns error on host chains +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + // call application callback directly + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + app, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) + suite.Require().True(ok) + + err = cbs.OnChanUpgradeAck( + suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.ChannelConfig.Version, + ) + + suite.Require().Error(err) + suite.Require().ErrorIs(err, icatypes.ErrInvalidChannelFlow) +} + func (suite *InterchainAccountsTestSuite) fundICAWallet(ctx sdk.Context, portID string, amount sdk.Coins) { interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(ctx, ibctesting.FirstConnectionID, portID) suite.Require().True(found) diff --git a/modules/core/04-channel/types/msgs.go b/modules/core/04-channel/types/msgs.go index 47e0103ec8d..e07d17c5670 100644 --- a/modules/core/04-channel/types/msgs.go +++ b/modules/core/04-channel/types/msgs.go @@ -832,7 +832,9 @@ func NewMsgChannelUpgradeCancel( } } -// ValidateBasic implements sdk.Msg +// ValidateBasic implements sdk.Msg. No checks are done for ErrorReceipt and ProofErrorReceipt +// since they are not required if the current channel state is not in FLUSHCOMPLETE and the signer +// is the designated authority (e.g. the governance module). func (msg MsgChannelUpgradeCancel) ValidateBasic() error { if err := host.PortIdentifierValidator(msg.PortId); err != nil { return errorsmod.Wrap(err, "invalid port ID")