Allow host_connection_id in version metadata to be empty on ChanOpenInit #5533
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GNaD13 marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,16 +47,19 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| path *ibctesting.Path | ||
| chanCap *capabilitytypes.Capability | ||
| metadata icatypes.Metadata | ||
| version string | ||
| ) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| malleate func() | ||
| assert func() | ||
|
There was a problem hiding this comment. You shouldn't add an assert function since you can just check the There was a problem hiding this comment. I see that you removed assert but I don't think you are testing that the output version inside the expPass case. Could you please check that the |
||
| expPass bool | ||
| }{ | ||
| { | ||
| "success", | ||
| func() {}, | ||
| func() {}, | ||
| true, | ||
| }, | ||
| { | ||
|
|
@@ -69,7 +72,9 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| suite.Require().NoError(err) | ||
|
|
||
| suite.openAndCloseChannel(path) | ||
| }, | ||
| func() {}, | ||
| true, | ||
| }, | ||
| { | ||
| "success - reopening account with new address", | ||
|
|
@@ -89,7 +94,28 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| // assert interchain account address mapping was deleted | ||
| _, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID) | ||
| suite.Require().False(found) | ||
| }, | ||
| func() {}, | ||
| true, | ||
| }, | ||
| { | ||
| "success - metadata empty host connection id", | ||
| func() { | ||
| metadata.HostConnectionId = "" | ||
|
|
||
| versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) | ||
| suite.Require().NoError(err) | ||
|
|
||
| path.EndpointA.ChannelConfig.Version = string(versionBytes) | ||
| }, | ||
| func() { | ||
| var metadataResponse icatypes.Metadata | ||
| err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadataResponse) | ||
| suite.Require().NoError(err) | ||
|
|
||
| suite.Require().Equal(ibctesting.FirstConnectionID, metadataResponse.HostConnectionId) | ||
| }, | ||
| true, | ||
| }, | ||
| { | ||
| "reopening account fails - no existing account", | ||
|
|
@@ -108,7 +134,9 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| acc := suite.chainB.GetSimApp().AccountKeeper.GetAccount(suite.chainB.GetContext(), sdk.MustAccAddressFromBech32(addr)) | ||
| suite.chainB.GetSimApp().AccountKeeper.RemoveAccount(suite.chainB.GetContext(), acc) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
| "reopening account fails - existing account is not interchain account type", | ||
|
|
@@ -132,7 +160,9 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| // overwrite existing account with only base account type, not intercahin account type | ||
| suite.chainB.GetSimApp().AccountKeeper.SetAccount(suite.chainB.GetContext(), icaAcc.BaseAccount) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
| "account already exists", | ||
|
|
@@ -142,6 +172,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| suite.Require().NoError(err) | ||
| suite.Require().True(suite.chainB.GetSimApp().AccountKeeper.HasAccount(suite.chainB.GetContext(), interchainAccAddr)) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -163,20 +194,24 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| channel.Version = string(versionBytes) | ||
|
|
||
| path.EndpointB.SetChannel(*channel) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
| "invalid order - UNORDERED", | ||
| func() { | ||
| channel.Ordering = channeltypes.UNORDERED | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
| "invalid port ID", | ||
| func() { | ||
| path.EndpointB.ChannelConfig.PortID = "invalid-port-id" //nolint:goconst | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -185,13 +220,15 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| channel.ConnectionHops = []string{"invalid-connnection-id"} | ||
| path.EndpointB.SetChannel(*channel) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
| "invalid metadata bytestring", | ||
| func() { | ||
| path.EndpointA.ChannelConfig.Version = "invalid-metadata-bytestring" | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -204,6 +241,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| path.EndpointA.ChannelConfig.Version = string(versionBytes) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -216,6 +254,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| path.EndpointA.ChannelConfig.Version = string(versionBytes) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -228,6 +267,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| path.EndpointA.ChannelConfig.Version = string(versionBytes) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -240,6 +280,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| path.EndpointA.ChannelConfig.Version = string(versionBytes) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -252,6 +293,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| path.EndpointA.ChannelConfig.Version = string(versionBytes) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -261,6 +303,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| err := suite.chainB.GetSimApp().ScopedICAHostKeeper.ClaimCapability(suite.chainB.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) | ||
| suite.Require().NoError(err) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| { | ||
|
|
@@ -272,7 +315,9 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| // set the active channelID in state | ||
| suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID) | ||
| }, | ||
| func() {}, | ||
| false, | ||
| }, | ||
| } | ||
|
|
||
|
|
@@ -311,7 +356,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
|
|
||
| tc.malleate() // malleate mutates test data | ||
|
|
||
| version, err = suite.chainB.GetSimApp().ICAHostKeeper.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.GetConnectionHops(), | ||
| path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, chanCap, channel.Counterparty, path.EndpointA.ChannelConfig.Version, | ||
| ) | ||
|
|
||
|
|
@@ -327,6 +372,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { | |
| // Check if account is created | ||
| interchainAccount := suite.chainB.GetSimApp().AccountKeeper.GetAccount(suite.chainB.GetContext(), interchainAccAddr) | ||
| suite.Require().Equal(interchainAccount.GetAddress().String(), storedAddr) | ||
| tc.assert() | ||
| } else { | ||
| suite.Require().Error(err) | ||
| suite.Require().Equal("", version) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,9 @@ func validateConnectionParams(metadata Metadata, controllerConnectionID, hostCon | |
| } | ||
|
|
||
| if metadata.HostConnectionId != hostConnectionID { | ||
| if metadata.HostConnectionId == "" { | ||
| return nil | ||
|
There was a problem hiding this comment. This is not how we should do it imo. It is possible that we want to add other checks after this in the future. So, the only There was a problem hiding this comment. The reason I added an if statement here to check whether There was a problem hiding this comment. I think we need to be a bit carefully with the validation of There was a problem hiding this comment. I'd make the following suggestion @GNaD13 :
Note that this is okay because the host is allowed to propose a new version string in There was a problem hiding this comment. If so, I think we will also remove the validation in ChanOpenInit and leave the controller chain validation for ChanOpenAck There was a problem hiding this comment. We can perform the validation if the user provided the version string, but not if we are using the default |
||
| } | ||
| return errorsmod.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", hostConnectionID, metadata.HostConnectionId) | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What is this test case actually testing?
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.
This test is for my old code. I removed it. Thank you sir