Allow host_connection_id in version metadata to be empty on ChanOpenInit #5533
Conversation
GNaD13
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5533 +/- ##
==========================================
+ Coverage 81.59% 81.62% +0.02%
==========================================
Files 199 198 -1
Lines 15167 12117 -3050
==========================================
- Hits 12375 9890 -2485
+ Misses 2326 1765 -561
+ Partials 466 462 -4
|
There was a problem hiding this comment.
Thanks for the work @GNaD13. I noted some problems I had with the PR.
Also I'm wondering if we should change the NewDefaultMetadata function in controller's OnChanInit (see here) to not require the counterparty_connection_id?
I could also take over the PR if you think these would be too much work for you @GNaD13 @crodriguezvega
| ) | ||
|
|
||
| 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 host_connection_id after every successful handshake anyway.
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 host_connection_id is correct?
| @@ -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 nil returned should be at the end of the function, not inside this if statement
There was a problem hiding this comment.
The reason I added an if statement here to check whether metadata.HostConnectionId is empty or not because I think we still need to keep check and return an error if
metadata.HostConnectionId != hostConnectionID and metadata.HostConnectionId not empty when the controller chain received an OnChanOpenAck package. So that it could check for the handshake. Should we remove this check?
There was a problem hiding this comment.
I think we need to be a bit carefully with the validation of metadata.HostConnectionId because it will be valid to be empty in ChanOpenInit but it should not be empty anymore in ChanOpenAck, right?
There was a problem hiding this comment.
I'd make the following suggestion @GNaD13 :
- Revert your changes to validateConnectionParams.
- In
ChanOpenTry, before sending the metadata to validation, overridemetadata.HostConnectionIdto beconnectionHops[0]regardless of whether or not it is filled. - Send to validation after step 2.
Note that this is okay because the host is allowed to propose a new version string in ChanOpenTry. Does this sound good @crodriguezvega?
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
…ng/host-connection-id
That sounds fine to me, but that change would be API breaking. Do you think it's ok for users to wait until the v9 to release this? Otherwise we could add a new function
That's also fine by me. Is it ok for you if we continue from here, @GNaD13? Thank you very much for opening the PR! |
Yeah, that's also fine to me. It will be great if we could work together |
Agree! Maybe then @srdtrk can comment the changes that would be needed and we can all together continue from there. :) |
|
@crodriguezvega I think we can break API in this PR to main. We can worry about the API if we backport (we might not backport). So I'd say, we should break |
| { | ||
| "success: empty host connection ID", | ||
| func() { | ||
| path.EndpointB.ConnectionID = "" |
There was a problem hiding this comment.
What is this test case actually testing?
There was a problem hiding this comment.
This test is for my old code. I removed it. Thank you sir
modules/apps/27-interchain-accounts/controller/keeper/handshake.go
Outdated
Show resolved
Hide resolved
WalkthroughThe changes focus on enhancing the interchain accounts functionality in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (9)
- e2e/tests/interchain_accounts/base_test.go (2 hunks)
- e2e/tests/interchain_accounts/gov_test.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (3 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/handshake.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (4 hunks)
- modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go (2 hunks)
- modules/apps/27-interchain-accounts/host/keeper/handshake.go (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (9 hunks)
- modules/apps/27-interchain-accounts/types/metadata.go (2 hunks)
Files skipped from review due to trivial changes (2)
- e2e/tests/interchain_accounts/base_test.go
- modules/apps/27-interchain-accounts/controller/keeper/handshake.go
Additional comments: 14
e2e/tests/interchain_accounts/gov_test.go (1)
- 58-58: The addition of a newline character is a minor stylistic change that improves readability. This change is approved as it does not impact the functionality or correctness of the code.
modules/apps/27-interchain-accounts/types/metadata.go (2)
- 36-36: The clarification added to the comment regarding the host connection identifier's ability to be an empty string improves the understanding of the
NewDefaultMetadatafunction's functionality. This change is approved as it enhances clarity.- 50-50: The clarification added to the comment regarding the host connection identifier's ability to be an empty string improves the understanding of the
NewDefaultMetadataStringfunction's functionality. This change is approved as it enhances clarity.modules/apps/27-interchain-accounts/host/keeper/handshake.go (1)
- 42-44: The addition of setting the
HostConnectionIdto the first connection hop if not set by the controller before validating the host metadata aligns with the PR's objectives and ensures correct host metadata validation. This change is approved as it is necessary for the functionality and correctness of the code.modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go (2)
- 31-32: The assignment of a new
versionusingicatypes.NewDefaultMetadataStringfor testing purposes is logically sound and aligns with the changes introduced in the PR. This ensures that the tests accurately reflect the intended behavior of the updated metadata handling.- 43-44: Using the newly assigned
versionfor settingChannelConfig.Versionfor bothEndpointAandEndpointBis appropriate for testing the migration process with the updated metadata functionality. This change is approved as it correctly implements the intended testing functionality.modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (2)
- 73-74: The modifications in
TestOnChanOpenTryintroduce several test cases to validate the handling of emptyhost_connection_idand ensure metadata consistency. Here are some observations and recommendations:
Empty
host_connection_idHandling (Lines 100-107): This test case effectively checks the scenario wherehost_connection_idis intentionally left empty, aligning with the PR's objectives. It's crucial to ensure that other parts of the system gracefully handle this scenario without unintended side effects.Metadata Consistency Checks: Several test cases have been added or modified to test various failure scenarios, such as invalid metadata, unsupported encoding formats, and incorrect connection IDs. These are essential for maintaining robustness in the face of potentially malformed or malicious inputs.
Use of
suite.Require().NoError(err)(Multiple Instances): The consistent use ofsuite.Require().NoError(err)for error handling in test cases is good practice, ensuring that tests fail fast upon encountering an unexpected error.Dynamic Test Case Structure (Lines 314-351): The dynamic structure used for defining test cases, where each case is defined with a name, a malleate function to mutate test data, and an expected pass/fail outcome, enhances readability and maintainability. This pattern is especially useful for testing a wide range of scenarios with varying setup requirements.
Overall, these changes are well-aligned with the PR's objectives and contribute to the robustness and flexibility of the
ibc-goframework's testing suite. It's recommended to continue this pattern of thorough and scenario-specific testing for future changes.Also applies to: 94-96, 100-107, 145-146, 170-171, 271-272, 314-351
- 591-591: The addition of handling the
host_connection_idwithin theTestOnChanUpgradeTrytest case (Line 591) is a critical update that aligns with the PR's objectives. By explicitly setting thehost_connection_idto the connection ID ofpath.EndpointB, this change ensures that the test accurately reflects the intended behavior of allowinghost_connection_idto be optionally empty and filled during version negotiation.It's commendable that the test case also considers various failure scenarios, such as invalid port IDs, unsupported metadata encoding formats, and changes to the interchain account address. These comprehensive tests are crucial for ensuring the robustness of the
ibc-goframework's upgrade mechanism.One minor suggestion for improvement is to ensure that all possible edge cases related to the
host_connection_idhandling are covered in the test scenarios. This includes scenarios where thehost_connection_idmight be incorrectly filled or left empty when it should not be. Adding such test cases, if not already present, would further strengthen the framework's resilience to potential issues.Overall, the changes to
TestOnChanUpgradeTryare well-thought-out and contribute significantly to the testing coverage of the new functionality introduced by the PR.modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (4)
- 19-23: The introduction of the
expectedVersionvariable is a good practice for making tests more dynamic and adaptable to changes. This approach allows for easier maintenance and readability of test cases, especially when dealing with version strings that might change based on the test scenario.- 58-58: The assignment of
expectedVersionusingicatypes.NewDefaultMetadataStringwithin the test case "success: empty channel version returns default metadata JSON string" is a crucial change. It ensures that the test accurately reflects the behavior of the system when the channel version is empty, aligning with the PR's objective to handlehost_connection_idmore flexibly. This change is correctly implemented and directly addresses the issue at hand.- 280-281: Setting the
expectedVersionto the marshaled JSON string ofmetadataat the beginning of the test setup is a good practice. It establishes a baseline expectation for the version string that will be used in most test cases, ensuring consistency and reducing redundancy in test code. This setup is well thought out and contributes to the clarity and maintainability of the test suite.- 303-303: The assertion that checks if the
expectedVersionmatches the actualversionreturned byOnChanOpenInitis a critical part of validating the correctness of the implementation. This assertion directly tests the functionality introduced in the PR, ensuring that the version negotiation behaves as expected. The use ofsuite.Require().Equalfor this assertion is appropriate and follows best practices for writing effective and meaningful tests.modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (2)
- 34-34: The change to
TestVersionusingicatypes.NewDefaultMetadataStringis a significant improvement in terms of code readability and maintainability. By utilizing theNewDefaultMetadataStringfunction, the test setup becomes more aligned with the actual implementation logic, ensuring that the tests are more representative of real-world scenarios. This change also reduces the potential for errors in manually marshaling JSON strings for test version metadata.- 241-241: The assertion
suite.Require().Equal(TestVersion, version)within theTestOnChanOpenInittest case correctly validates that the version returned by theOnChanOpenInitcallback matches the expectedTestVersion. This ensures that the version negotiation process adheres to the expected behavior, particularly in the context of the changes made to handlehost_connection_idmore flexibly. It's crucial for maintaining the integrity of the version negotiation process in the IBC middleware.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
Description
closes: #5437
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit