imp: use UNORDERED as default ordering for new ICA channels #6434
Conversation
crodriguezvega
requested review from
colin-axner,
AdityaSripal,
charleenfei,
damiannolan,
chatton and
DimitrisJim
as code owners
WalkthroughWalkthroughThe changes primarily involve modifying the Changes
Sequence Diagram(s) (Beta)No sequence diagrams are generated as the changes are primarily parameter additions and default value updates, which do not significantly alter the control flow. Assessment against linked issues
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.
Actionable comments posted: 8
Outside diff range and nitpick comments (3)
docs/docs/05-migrations/13-v8-to-v9.md (3)
Line range hint
35-36: Remove trailing spaces on lines 35 and 36 to adhere to markdown best practices.- The `exported.ConnectionI` and `exported.CounterpartyConnectionI` interfaces have been removed. Please use the concrete types. + The `exported.ConnectionI` and `exported.CounterpartyConnectionI` interfaces have been removed. Please use the concrete types.
Line range hint
80-80: Replace hard tabs with spaces on line 80 to maintain consistency in indentation.- + queryRouter icatypes.QueryRouter, + queryRouter icatypes.QueryRouter,
Line range hint
8-8: Ensure there is only one top-level heading in the document to comply with markdown best practices.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (15)
- CHANGELOG.md (1 hunks)
- docs/docs/02-apps/02-interchain-accounts/09-active-channels.md (1 hunks)
- docs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md (3 hunks)
- docs/docs/05-migrations/13-v8-to-v9.md (1 hunks)
- modules/apps/27-interchain-accounts/controller/client/cli/tx.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/account.go (2 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/account_test.go (3 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (2 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go (4 hunks)
- modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go (2 hunks)
- modules/apps/27-interchain-accounts/host/ibc_module_test.go (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (1 hunks)
- modules/apps/29-fee/ica_test.go (1 hunks)
Additional context used
Path-based instructions (15)
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/27-interchain-accounts/controller/keeper/account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.docs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"modules/apps/27-interchain-accounts/controller/client/cli/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"docs/docs/05-migrations/13-v8-to-v9.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"modules/apps/29-fee/ica_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/ibc_module_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md
[uncategorized] ~16-~16: Possible missing comma found.
Context: ...ED channels is that when a packet times out the channel will be closed. In the cas...
[style] ~35-~35: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... given interchain account, new channels can not be opened for this account until the cu...
Markdownlint
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md
10-10: Expected: 0 or 2; Actual: 1
Trailing spaces
8-8: null
Multiple top-level headings in the same documentdocs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md
9-9: null
Multiple top-level headings in the same documentdocs/docs/05-migrations/13-v8-to-v9.md
35-35: Expected: 0 or 2; Actual: 1
Trailing spaces
36-36: Expected: 0 or 2; Actual: 1
Trailing spaces
80-80: Column: 34
Hard tabs
8-8: null
Multiple top-level headings in the same documentCHANGELOG.md
218-218: Expected: 0 or 2; Actual: 1
Trailing spaces
268-268: Expected: 0 or 2; Actual: 1
Trailing spaces
203-203: null
Bare URL used
GitHub Check: lint
modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go
[failure] 11-11:
redundant-import-alias: Import alias "v6" is redundant (revive)
Additional comments not posted (9)
modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (1)
43-49: Ensure correct default channel ordering.The logic to default the channel ordering to
UNORDEREDwhenNONEis specified is correctly implemented. This aligns with the PR's objectives to makeUNORDEREDthe default state.modules/apps/27-interchain-accounts/controller/keeper/account.go (1)
Line range hint
32-49: Ensure correct default channel ordering in legacy API.The implementation correctly sets the default channel ordering to
UNORDEREDwhenNONEis specified, which is consistent with the PR's objectives.modules/apps/27-interchain-accounts/controller/client/cli/tx.go (1)
74-74: Set default channel ordering in CLI.The CLI now correctly sets the default channel ordering to
UNORDERED, which aligns with the PR's objectives to makeUNORDEREDthe default state.modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go (2)
39-45: The test case for defaulting toUNORDEREDwhenNONEis specified is well-implemented and aligns with the PR objectives.
Line range hint
82-110: The test setup correctly initializes the expected ordering toORDEREDand verifies the channel ordering after theRegisterInterchainAccountcall. This ensures that the channel ordering is set as expected based on the test setup.docs/docs/05-migrations/13-v8-to-v9.md (1)
87-97: The documentation correctly reflects the API changes forRegisterInterchainAccount, including the neworderingparameter.modules/apps/29-fee/ica_test.go (1)
78-78: The test setup correctly uses theORDEREDparameter for theRegisterInterchainAccountfunction, aligning with the expected test configuration.modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go (1)
93-93: The test setup correctly uses theORDEREDparameter for theRegisterInterchainAccountfunction, ensuring that the test configuration is consistent with the expected behavior.CHANGELOG.md (1)
57-58: The changes to the default ordering for new ICA channels and the modification of theUnmarshalPacketDatainterface are correctly documented under the "API Breaking" section. This aligns with the PR's objectives and the modifications made in the code.
| @@ -78,7 +78,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro | |||
|
|
|||
| channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) | |||
|
|
|||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion); err != nil { | |||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, channeltypes.ORDERED); err != nil { | |||
There was a problem hiding this comment.
Change the hardcoded channeltypes.ORDERED to channeltypes.UNORDERED to align with the PR's objective of defaulting to UNORDERED.
- if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, channeltypes.ORDERED); err != nil {
+ if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, channeltypes.UNORDERED); err != nil {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, channeltypes.ORDERED); err != nil { | |
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, channeltypes.UNORDERED); err != nil { |
| @@ -112,7 +112,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro | |||
|
|
|||
| channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) | |||
|
|
|||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version); err != nil { | |||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.ORDERED); err != nil { | |||
There was a problem hiding this comment.
Ensure the RegisterInterchainAccount function uses the correct channel ordering.
The test function RegisterInterchainAccount is hardcoded to use channeltypes.ORDERED, which contradicts the PR's objective to default to UNORDERED when no specific ordering is provided. This needs to be aligned with the new default behavior.
- if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.ORDERED); err != nil {
+ if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.UNORDERED); err != nil {
return err
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.ORDERED); err != nil { | |
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.UNORDERED); err != nil { |
| @@ -11,7 +11,7 @@ The Interchain Accounts module uses either [ORDERED or UNORDERED](https://github | |||
|
|
|||
| When using `ORDERED` channels, the order of transactions when sending packets from a controller to a host chain is maintained. | |||
|
|
|||
| When using `UNORDERED` channels, there is no guarantee that the order of transactions when sending packets from the controller to the host chain is maintained. | |||
| When using `UNORDERED` channels, there is no guarantee that the order of transactions when sending packets from the controller to the host chain is maintained.If no ordering is specified in `MsgRegisterInterchainAccount`, then the default ordering for new ICA channels is `UNORDERED`. | |||
There was a problem hiding this comment.
Clarify the default behavior for channel ordering.
Consider adding a sentence to explicitly state that UNORDERED is the default unless specified otherwise. This could improve clarity for users unfamiliar with the context.
| @@ -11,7 +11,7 @@ | |||
|
|
|||
| When using `ORDERED` channels, the order of transactions when sending packets from a controller to a host chain is maintained. | |||
|
|
|||
| When using `UNORDERED` channels, there is no guarantee that the order of transactions when sending packets from the controller to the host chain is maintained. | |||
| When using `UNORDERED` channels, there is no guarantee that the order of transactions when sending packets from the controller to the host chain is maintained.If no ordering is specified in `MsgRegisterInterchainAccount`, then the default ordering for new ICA channels is `UNORDERED`. | |||
|
|
|||
| > A limitation when using ORDERED channels is that when a packet times out the channel will be closed. | |||
There was a problem hiding this comment.
Add a comma for better readability.
- ...ED channels is that when a packet times out the channel will be closed. In the cas...
+ ...ED channels is that when a packet times out, the channel will be closed. In the cas...Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| > A limitation when using ORDERED channels is that when a packet times out the channel will be closed. | |
| > A limitation when using ORDERED channels is that when a packet times out, the channel will be closed. |
| @@ -72,7 +72,7 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() { | |||
|
|
|||
| tc.malleate() // malleate mutates test data | |||
|
|
|||
| err = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner, TestVersion) | |||
| err = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner, TestVersion, channeltypes.ORDERED) | |||
There was a problem hiding this comment.
Update tests to reflect new default behavior.
The tests explicitly set the channel ordering to ORDERED. Consider adding tests that verify the new default behavior where the ordering defaults to UNORDERED if not specified.
Also applies to: 106-106, 118-118
| @@ -19,7 +19,7 @@ The controller submodule keeper exposes two legacy functions that allow respecti | |||
| The authentication module can begin registering interchain accounts by calling `RegisterInterchainAccount`: | |||
|
|
|||
| ```go | |||
| if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, connectionID, owner.String(), version); err != nil { | |||
| if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, connectionID, owner.String(), version, channeltypes.UNORDERED); err != nil { | |||
There was a problem hiding this comment.
Update documentation to reflect new default behavior.
The documentation examples explicitly set the channel ordering to UNORDERED. Consider mentioning the new default behavior in the documentation to inform users that they do not need to explicitly set UNORDERED unless they want to override another specified behavior.
Also applies to: 47-47, 78-78
| @@ -83,7 +83,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro | |||
|
|
|||
| channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) | |||
|
|
|||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version); err != nil { | |||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.ORDERED); err != nil { | |||
There was a problem hiding this comment.
Change the hardcoded channel ordering to align with PR objectives.
- if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.ORDERED); err != nil {
+ if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.UNORDERED); err != nil {This change ensures that the default ordering for new ICA channels is UNORDERED, as intended by the PR.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.ORDERED); err != nil { | |
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, channeltypes.UNORDERED); err != nil { |
| @@ -8,7 +8,7 @@ | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
|
|
|||
| capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" | |||
| "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6" | |||
| v6 "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6" | |||
There was a problem hiding this comment.
Remove the redundant import alias v6 as it is unnecessary and flagged by the linter.
- import v6 "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6"
+ import "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| v6 "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6" | |
| import "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6" |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md (1)
Line range hint
35-35: Use "cannot" instead of "can not" for better readability and grammatical correctness.- new channels can not be opened for this account until the currently set `Active Channel` is set to `CLOSED`. + new channels cannot be opened for this account until the currently set `Active Channel` is set to `CLOSED`.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/docs/02-apps/02-interchain-accounts/09-active-channels.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md
[style] ~35-~35: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... given interchain account, new channels can not be opened for this account until the cu...
Additional comments not posted (1)
docs/docs/02-apps/02-interchain-accounts/09-active-channels.md (1)
14-14: Clarify the default behavior for channel ordering.Consider adding a sentence to explicitly state that
UNORDEREDis the default unless specified otherwise. This could improve clarity for users unfamiliar with the context.
…t-in-new-ica-channels
| @@ -78,7 +78,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro | |||
|
|
|||
| channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) | |||
|
|
|||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion); err != nil { | |||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, channeltypes.ORDERED); err != nil { | |||
There was a problem hiding this comment.
should we be using UNORDERED here now? or is this kept to maintain some compatibility with existing test cases?
There was a problem hiding this comment.
Good remark, @damiannolan. I have modified in bbba1e5 I think all tests doing something with ICA to test both ordered and unordered channels. If you could take a look today and let me know if the changes look good to you, then I would appreciate. Then I can merge the PR today. 🚀
I am basically adding a loop (for _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} ) and changed the NewICAPath functions to accept an extra argument to specify the ordering.
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
docs/docs/05-migrations/13-v8-to-v9.md (1)
87-97: The legacy functionRegisterInterchainAccountnow takes an extra parameter to specify the ordering of new ICA channels. Consider refining the documentation to clarify the impact of this change on existing implementations.The legacy function `RegisterInterchainAccount` now takes an extra parameter, `ordering`, to specify the ordering of new ICA channels. This change may require updates to existing implementations to accommodate the new parameter.CHANGELOG.md (3)
Line range hint
219-219: Correct the trailing spaces in the markdown file.- * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. + * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able.
Line range hint
269-269: Correct the trailing spaces in the markdown file.- * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. + * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels.
Line range hint
204-204: Avoid using bare URLs in markdown files.- * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. + * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. See [here](https://github.com/cosmos/ibc-go/pull/6251).
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- docs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md (3 hunks)
- docs/docs/05-migrations/13-v8-to-v9.md (1 hunks)
Additional context used
Path-based instructions (3)
docs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"docs/docs/05-migrations/13-v8-to-v9.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md
[grammar] ~29-~29: Did you mean “specifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...packets. Theorderingargument allows to specify the ordering of the channel that is cre...
Markdownlint
docs/docs/02-apps/02-interchain-accounts/10-legacy/03-keeper-api.md
9-9: null (MD025, single-title, single-h1)
Multiple top-level headings in the same documentdocs/docs/05-migrations/13-v8-to-v9.md
35-35: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
36-36: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
80-80: Column: 34 (MD010, no-hard-tabs)
Hard tabs
8-8: null (MD025, single-title, single-h1)
Multiple top-level headings in the same documentCHANGELOG.md
219-219: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
269-269: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
204-204: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
58-58: Correct the trailing spaces in the markdown file.- * (apps/27-interchain-accounts) [\#6433](https://github.com/cosmos/ibc-go/pull/6433) Use UNORDERED as the default ordering for new ICA channels. + * (apps/27-interchain-accounts) [\#6433](https://github.com/cosmos/ibc-go/pull/6433) Use UNORDERED as the default ordering for new ICA channels.Likely invalid or redundant comment.
… involving something with ICA
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (16)
- modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (17 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/account_test.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/genesis_test.go (2 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/grpc_query_test.go (2 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (5 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go (3 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go (4 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/relay_test.go (2 hunks)
- modules/apps/27-interchain-accounts/host/ibc_module_test.go (13 hunks)
- modules/apps/27-interchain-accounts/host/keeper/genesis_test.go (2 hunks)
- modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (5 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (4 hunks)
- modules/apps/27-interchain-accounts/host/keeper/relay_test.go (3 hunks)
- modules/apps/29-fee/ibc_middleware_test.go (2 hunks)
- modules/apps/29-fee/ica_test.go (4 hunks)
Files not summarized due to errors (2)
- modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go: Error: Message exceeds token limit
- modules/apps/27-interchain-accounts/host/ibc_module_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (2)
- modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go
- modules/apps/29-fee/ica_test.go
Additional context used
Path-based instructions (14)
modules/apps/27-interchain-accounts/controller/keeper/grpc_query_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/genesis_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/keeper/genesis_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/relay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/keeper/relay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/ibc_module_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/29-fee/ibc_middleware_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
golangci-lint
modules/apps/27-interchain-accounts/controller/keeper/account_test.go
10-10: undefined: KeeperTestSuite (typecheck)
89-89: undefined: KeeperTestSuite (typecheck)
29-29: undefined: TestPortID (typecheck)
30-30: undefined: TestPortID (typecheck)
45-45: undefined: TestOwnerAddress (typecheck)
56-56: undefined: TestVersion (typecheck)
70-70: undefined: TestOwnerAddress (typecheck)
72-72: undefined: NewICAPath (typecheck)
77-77: undefined: TestVersion (typecheck)
93-93: undefined: TestOwnerAddress (typecheck)
95-95: undefined: NewICAPath (typecheck)
98-98: undefined: NewICAPath (typecheck)
GitHub Check: lint
modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
[failure] 23-23:
(*KeeperTestSuite).openAndCloseChannel-orderingis unused (unparam)
Additional comments not posted (51)
modules/apps/27-interchain-accounts/controller/keeper/grpc_query_test.go (1)
45-77: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change ensures that the functionality works correctly across different channel orderings, which is crucial given the PR's focus on changing default channel behaviors.
modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go (1)
46-78: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change ensures that the migration logic is robust across different channel orderings, aligning with the PR's objectives to handle different channel types effectively.
modules/apps/27-interchain-accounts/controller/keeper/account_test.go (1)
11-85: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change ensures that the registration logic works correctly across different channel orderings, which is crucial given the PR's focus on changing default channel behaviors.
Tools
golangci-lint
29-29: undefined: TestPortID (typecheck)
30-30: undefined: TestPortID (typecheck)
45-45: undefined: TestOwnerAddress (typecheck)
56-56: undefined: TestVersion (typecheck)
70-70: undefined: TestOwnerAddress (typecheck)
72-72: undefined: NewICAPath (typecheck)
77-77: undefined: TestVersion (typecheck)
modules/apps/27-interchain-accounts/controller/keeper/genesis_test.go (1)
99-124: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change ensures that the genesis initialization and export logic are robust across different channel orderings, aligning with the PR's objectives to handle different channel types effectively.
modules/apps/27-interchain-accounts/host/keeper/genesis_test.go (1)
116-140: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change ensures that the genesis initialization and export logic are robust across different channel orderings, aligning with the PR's objectives to handle different channel types effectively.
modules/apps/27-interchain-accounts/controller/keeper/relay_test.go (1)
Line range hint
145-223: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change ensures that the relay logic works correctly across different channel orderings, which is crucial given the PR's focus on changing default channel behaviors.
modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go (7)
55-60: Ensure theNewICAPathfunction correctly handles theorderingparameter.This function now accepts an
orderingparameter and correctly sets it for both endpoints, aligning with the PR's objective to handle channel ordering dynamically.
164-178: Review the test coverage for differentorderingtypes inTestGetAllPorts.The test iterates over both
UNORDEREDandORDEREDtypes, ensuring comprehensive coverage as per the PR objectives.
182-200: Check the robustness ofTestGetInterchainAccountAddressfor different orderings.This test method effectively checks the retrieval of interchain account addresses under different channel orderings, ensuring the system's robustness.
204-237: Assess the implementation ofTestGetAllActiveChannelsfor handling multiple orderings.The test method correctly handles multiple channel orderings and verifies the active channels, aligning with the changes made in the PR.
242-276: Evaluate theTestGetAllInterchainAccountsfor its handling of different channel orderings.The test ensures that interchain accounts are correctly retrieved and listed for both
UNORDEREDandORDEREDchannels, which is crucial for the functionality introduced in this PR.
281-294: Confirm the functionality ofTestIsActiveChannelwith different orderings.This test validates the active status of channels under different orderings, ensuring that the system behaves as expected across different configurations.
93-93: Verify the correct usage of theorderingparameter inRegisterInterchainAccount.Verification successful
The
rgcommand output shows multiple instances of theRegisterInterchainAccountfunction across various files. To verify the correct usage of theorderingparameter, we need to ensure that it is passed correctly in all these instances.Here are some key instances to review:
modules/apps/29-fee/ica_test.gomodules/apps/27-interchain-accounts/host/ibc_module_test.gomodules/apps/27-interchain-accounts/host/keeper/keeper_test.gomodules/apps/27-interchain-accounts/controller/ibc_middleware_test.gomodules/apps/27-interchain-accounts/controller/keeper/account_test.goWe will check these files to ensure that the
orderingparameter is used correctly in theRegisterInterchainAccountfunction calls.
The results from the shell scripts indicate that the
orderingparameter is consistently used in theRegisterInterchainAccountfunction calls across various files. The parameter is passed correctly as per the function definition in all instances reviewed.Here are the key observations:
- In
modules/apps/29-fee/ica_test.go, theorderingparameter is passed asendpoint.ChannelConfig.Order.- In
modules/apps/27-interchain-accounts/host/ibc_module_test.go, theorderingparameter is passed asendpoint.ChannelConfig.Order.- In
modules/apps/27-interchain-accounts/host/keeper/keeper_test.go, theorderingparameter is passed asendpoint.ChannelConfig.Order.- In
modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go, theorderingparameter is passed asendpoint.ChannelConfig.Order.- In
modules/apps/27-interchain-accounts/controller/keeper/account_test.go, theorderingparameter is passed asordering.Based on these observations, the usage of the
orderingparameter in theRegisterInterchainAccountfunction calls is correct and consistent.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `ordering` parameter is used correctly across all calls to `RegisterInterchainAccount`. # Test: Search for the function usage. Expect: Correct usage of the ordering parameter. rg --type go $'RegisterInterchainAccount'Length of output: 29104
Script:
#!/bin/bash # Description: Verify the correct usage of the `ordering` parameter in `RegisterInterchainAccount` function calls. # Check the usage in modules/apps/29-fee/ica_test.go echo "Checking modules/apps/29-fee/ica_test.go" rg --context 5 'RegisterInterchainAccount' modules/apps/29-fee/ica_test.go # Check the usage in modules/apps/27-interchain-accounts/host/ibc_module_test.go echo "Checking modules/apps/27-interchain-accounts/host/ibc_module_test.go" rg --context 5 'RegisterInterchainAccount' modules/apps/27-interchain-accounts/host/ibc_module_test.go # Check the usage in modules/apps/27-interchain-accounts/host/keeper/keeper_test.go echo "Checking modules/apps/27-interchain-accounts/host/keeper/keeper_test.go" rg --context 5 'RegisterInterchainAccount' modules/apps/27-interchain-accounts/host/keeper/keeper_test.go # Check the usage in modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go echo "Checking modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go" rg --context 5 'RegisterInterchainAccount' modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go # Check the usage in modules/apps/27-interchain-accounts/controller/keeper/account_test.go echo "Checking modules/apps/27-interchain-accounts/controller/keeper/account_test.go" rg --context 5 'RegisterInterchainAccount' modules/apps/27-interchain-accounts/controller/keeper/account_test.goLength of output: 12691
modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (6)
Line range hint
66-82: Ensure theNewICAPathfunction correctly handles theorderingparameter with different encodings.This function now accepts both
encodingandorderingparameters, correctly setting them for both endpoints. This is crucial for supporting different data formats and channel behaviors.
230-248: Review the test coverage for differentorderingtypes inTestGetInterchainAccountAddress.The test iterates over both
UNORDEREDandORDEREDtypes, ensuring comprehensive coverage as per the PR objectives.
252-283: Assess the implementation ofTestGetAllActiveChannelsfor handling multiple orderings.The test method correctly handles multiple channel orderings and verifies the active channels, aligning with the changes made in the PR.
288-322: Evaluate theTestGetAllInterchainAccountsfor its handling of different channel orderings.The test ensures that interchain accounts are correctly retrieved and listed for both
UNORDEREDandORDEREDchannels, which is crucial for the functionality introduced in this PR.
327-338: Confirm the functionality ofTestIsActiveChannelwith different orderings.This test validates the active status of channels under different orderings, ensuring that the system behaves as expected across different configurations.
115-115: Verify the correct usage of theorderingparameter inRegisterInterchainAccount.Verification successful
The search results show multiple instances where
RegisterInterchainAccountis called. To verify the correct usage of theorderingparameter, we need to ensure that it is consistently and correctly passed in these calls.Instances to Review:
modules/apps/29-fee/ica_test.gomodules/apps/27-interchain-accounts/host/ibc_module_test.gomodules/apps/27-interchain-accounts/host/keeper/keeper_test.gomodules/apps/27-interchain-accounts/controller/ibc_middleware_test.gomodules/apps/27-interchain-accounts/controller/keeper/account_test.goLet's review these instances to ensure the
orderingparameter is used correctly.Review of Instances:
modules/apps/29-fee/ica_test.goif err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, endpoint.ChannelConfig.Order); err != nil {The
orderingparameter is correctly passed asendpoint.ChannelConfig.Order.
modules/apps/27-interchain-accounts/host/ibc_module_test.goif err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, endpoint.ChannelConfig.Order); err != nil {The
orderingparameter is correctly passed asendpoint.ChannelConfig.Order.
modules/apps/27-interchain-accounts/host/keeper/keeper_test.goif err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version, endpoint.ChannelConfig.Order); err != nil {The
orderingparameter is correctly passed asendpoint.ChannelConfig.Order.
modules/apps/27-interchain-accounts/controller/ibc_middleware_test.goif err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, TestVersion, endpoint.ChannelConfig.Order); err != nil {The
orderingparameter is correctly passed asendpoint.ChannelConfig.Order.
modules/apps/27-interchain-accounts/controller/keeper/account_test.goerr = suite.chainA.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner, TestVersion, ordering)The
orderingparameter is correctly passed asordering.Based on the review of these instances, the
orderingparameter is used correctly in the calls toRegisterInterchainAccount.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `ordering` parameter is used correctly across all calls to `RegisterInterchainAccount`. # Test: Search for the function usage. Expect: Correct usage of the ordering parameter. rg --type go $'RegisterInterchainAccount'Length of output: 29104
modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (4)
46-358: The implementation ofTestOnChanOpenTrycorrectly iterates over bothUNORDEREDandORDEREDchannel types, aligning with the PR's objectives.This comprehensive testing ensures that channel opening logic behaves as expected under various conditions and with both channel orderings.
384-414: The implementation ofTestOnChanOpenConfirmcorrectly iterates over bothUNORDEREDandORDEREDchannel types, aligning with the PR's objectives.This ensures that the channel confirmation logic is robust and behaves as expected under various conditions and with both channel orderings.
431-453: The implementation ofTestOnChanCloseConfirmcorrectly iterates over bothUNORDEREDandORDEREDchannel types, aligning with the PR's objectives.This ensures that the channel close confirmation logic is robust and behaves as expected under various conditions and with both channel orderings.
581-631: The implementation ofTestOnChanUpgradeTrycorrectly iterates over bothUNORDEREDandORDEREDchannel types, aligning with the PR's objectives.This ensures that the channel upgrade logic is robust and behaves as expected under various conditions and with both channel orderings.
modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (4)
18-18: Enhanced test coverage by iterating over bothORDEREDandUNORDEREDchannel types.
428-428: Properly testing bothORDEREDandUNORDEREDchannel types in the acknowledgment phase.
492-492: Ensuring bothORDEREDandUNORDEREDchannel types are handled during the channel close confirmation phase.
655-655: Comprehensive testing of channel upgrade initialization for bothORDEREDandUNORDEREDchannel types.modules/apps/27-interchain-accounts/host/keeper/relay_test.go (2)
28-30: Ensure comprehensive testing by iterating over bothUNORDEREDandORDEREDchannel types.This change aligns with the PR's objective to handle both channel types, enhancing test coverage and ensuring robustness.
881-927: Validate the handling of different channel orderings in JSON-based tests.This segment properly tests the handling of
UNORDEREDandORDEREDchannels using JSON encoding, ensuring that the system behaves as expected under different configurations.modules/apps/27-interchain-accounts/host/ibc_module_test.go (5)
66-71: Ensure theorderingparameter is correctly propagated inNewICAPath.This function correctly sets the
orderingfor both endpoints, aligning with the PR's objective to manage channel order dynamically.
804-892: Validate financial calculations inTestControlAccountAfterChannelClose.Verification successful
The financial calculations in
TestControlAccountAfterChannelCloseare verified through the assertionssuite.assertBalance(icaAddr, expBalAfterFirstSend)andsuite.assertBalance(icaAddr, expBalAfterSecondSend). These assertions ensure that the expected balances are correctly checked after each transaction.
expBalAfterFirstSendis asserted withsuite.assertBalance(icaAddr, expBalAfterFirstSend).expBalAfterSecondSendis asserted withsuite.assertBalance(icaAddr, expBalAfterSecondSend).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that financial calculations in TestControlAccountAfterChannelClose are accurate and consistent. # Test: Check for balance calculations and assertions. rg --type go 'suite.Require().Equal(expBalAfterSecondSend, balance)' 'modules/apps/27-interchain-accounts/host/ibc_module_test.go'Length of output: 130
Script:
#!/bin/bash # Description: Ensure that financial calculations in TestControlAccountAfterChannelClose are accurate and consistent. # Test: Check for balance calculations and assertions in the TestControlAccountAfterChannelClose method. rg --type go 'TestControlAccountAfterChannelClose' -A 50 'modules/apps/27-interchain-accounts/host/ibc_module_test.go' | rg 'suite.Require().Equal|suite.assertBalance|expBalAfterFirstSend|expBalAfterSecondSend'Length of output: 329
Script:
#!/bin/bash # Description: Ensure that financial calculations in TestControlAccountAfterChannelClose are accurate and consistent. # Test: Check for balance calculations and assertions in the entire ibc_module_test.go file. rg --type go 'suite.Require().Equal|suite.assertBalance|expBalAfterFirstSend|expBalAfterSecondSend' 'modules/apps/27-interchain-accounts/host/ibc_module_test.go'Length of output: 389
237-268: Check error handling consistency inTestChanOpenAck.Verification successful
The
TestChanOpenAckfunction does include error handling withsuite.Require().Error(err). This indicates that the error handling is present and consistent within this function.
- Location of error handling in
TestChanOpenAck:
modules/apps/27-interchain-accounts/host/ibc_module_test.go, within theTestChanOpenAckfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent error handling in TestChanOpenAck across different scenarios. # Test: Search for error handling patterns in TestChanOpenAck. rg --type go 'suite.Require().Error(err)' 'modules/apps/27-interchain-accounts/host/ibc_module_test.go'Length of output: 103
Script:
#!/bin/bash # Description: Verify error handling consistency in TestChanOpenAck function. # Step 1: Locate the TestChanOpenAck function definition. ast-grep --lang go --pattern 'func TestChanOpenAck($_) $_' # Step 2: Extract the content of the TestChanOpenAck function to inspect error handling. rg --type go -A 50 'func TestChanOpenAck' modules/apps/27-interchain-accounts/host/ibc_module_test.goLength of output: 164
Script:
#!/bin/bash # Description: Verify error handling consistency in functions related to ChanOpenAck. # Step 1: Locate any function containing ChanOpenAck in its name. ast-grep --lang go --pattern 'func $_ChanOpenAck($_) $_' # Step 2: Extract the content of the located functions to inspect error handling. rg --type go -A 50 'func.*ChanOpenAck' modules/apps/27-interchain-accounts/host/ibc_module_test.goLength of output: 2204
658-684: Ensure error handling aligns with expected outcomes inTestOnChanUpgradeInit.
86-86: Verify error handling inRegisterInterchainAccount.modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (15)
61-66: Ensure theorderingparameter is properly propagated inNewICAPath.This change correctly updates the channel configuration to use the specified
orderingfor both endpoints, aligning with the PR's objective to support configurable channel orderings.
188-250: Review the test case structure inTestOnChanOpenInit.The test cases are well-structured and iterate over both
ORDEREDandUNORDEREDtypes, which is crucial for ensuring that both channel types are supported and behave as expected.
358-395: Assess the implementation ofTestOnChanOpenAck.The implementation correctly handles different scenarios and channel orderings, ensuring comprehensive testing of the channel acknowledgment process.
405-448: Evaluate the logic inTestChanOpenConfirm.This function tests the channel confirmation logic under various conditions and for both channel orderings, which is essential for verifying the IBC channel lifecycle.
497-531: Review the test setup and execution inTestOnChanCloseConfirm.The test setup and execution are appropriate for testing the channel close confirm functionality, covering both
ORDEREDandUNORDEREDchannel types.
546-595: Check the packet handling inTestOnRecvPacket.The test effectively checks the packet reception logic, ensuring that errors are handled correctly and that the system behaves as expected under erroneous conditions.
647-692: Evaluate the acknowledgment packet handling inTestOnAcknowledgementPacket.This function tests the acknowledgment packet handling thoroughly, covering various scenarios and both channel orderings.
744-789: Inspect the timeout packet handling inTestOnTimeoutPacket.The timeout packet handling is correctly implemented, with comprehensive tests that ensure the system's robustness in handling timeouts across different channel orderings.
842-888: Review the channel upgrade initialization logic inTestOnChanUpgradeInit.The test cases are well-structured and cover a variety of scenarios, including error conditions and different channel orderings, ensuring the channel upgrade process is robust.
894-918: Check the channel upgrade try logic inTestOnChanUpgradeTry.This function tests the channel upgrade try logic comprehensively, ensuring that errors are handled correctly and that the system behaves as expected under various conditions.
970-1013: Evaluate the channel upgrade acknowledgment logic inTestOnChanUpgradeAck.The implementation correctly handles different scenarios and channel orderings, ensuring comprehensive testing of the channel upgrade acknowledgment process.
Line range hint
1030-1111: Inspect the channel upgrade open logic inTestOnChanUpgradeOpen.This function tests the channel upgrade open logic under various conditions and for both channel orderings, which is essential for verifying the IBC channel lifecycle.
453-473: Inspect error handling inTestOnChanCloseInit.This verification checks if the error handling in
TestOnChanCloseInitis comprehensive and covers all necessary cases.
81-81: Validate error handling inRegisterInterchainAccount.This verification checks if all potential errors in the
RegisterInterchainAccountfunction are handled appropriately, ensuring robustness.
261-304: Check the error handling inTestChanOpenTry.This script checks for consistent error handling in the
TestChanOpenTryfunction, ensuring that all error paths are covered.Verification successful
The
TestChanOpenTryfunction exists in bothmodules/apps/27-interchain-accounts/controller/ibc_middleware_test.goandmodules/core/04-channel/keeper/handshake_test.go. Both functions include error handling, but the specific implementation details differ.
- In
ibc_middleware_test.go, error handling is done usingsuite.Require().Error(err)andsuite.Require().NoError(err).- In
handshake_test.go, error handling is also done usingsuite.Require().Error(err)andsuite.Require().NoError(err)within various test cases.Given the presence of error handling in both functions, it appears that the error handling is consistent and covers different branches of the test cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error handling in TestChanOpenTry is consistent and covers all branches. # Test: Search for error handling patterns in TestChanOpenTry. rg --type go 'if err != nil {'Length of output: 126
Script:
#!/bin/bash # Description: Verify that the error handling in TestChanOpenTry is consistent and covers all branches. # Test: Search for error handling patterns in TestChanOpenTry function specifically. ast-grep --lang go --pattern $'func TestChanOpenTry($_) { $$$ if err != nil { $$$ } $$$ }'Length of output: 103
Script:
#!/bin/bash # Description: Verify that the error handling in TestChanOpenTry is consistent and covers all branches. # Step 1: Locate the TestChanOpenTry function. ast-grep --lang go --pattern 'func TestChanOpenTry($_) { $$$ }' # Step 2: Search for error handling within the located function. ast-grep --lang go --pattern 'if err != nil { $$$ }' --within 'func TestChanOpenTry($_) { $$$ }'Length of output: 370
Script:
#!/bin/bash # Description: Verify that the error handling in TestChanOpenTry is consistent and covers all branches. # Step 1: Locate the TestChanOpenTry function and output its content. ast-grep --lang go --pattern 'func TestChanOpenTry($_) { $$$ }' > test_chan_open_try_function.txt # Step 2: Search for error handling within the located function. grep -A 20 'func TestChanOpenTry' test_chan_open_try_function.txt | grep 'if err != nil {'Length of output: 160
Script:
#!/bin/bash # Description: Verify the existence and content of the TestChanOpenTry function. # Step 1: Locate the TestChanOpenTry function and output its content. ast-grep --lang go --pattern 'func TestChanOpenTry($_) { $$$ }' > test_chan_open_try_function.txt # Step 2: Output the content of the located function for manual inspection. cat test_chan_open_try_function.txtLength of output: 105
Script:
#!/bin/bash # Description: Verify the existence and content of the TestChanOpenTry function. # Step 1: Locate the TestChanOpenTry function using ripgrep. rg 'func TestChanOpenTry' > test_chan_open_try_function.txt # Step 2: Output the content of the located function for manual inspection. cat test_chan_open_try_function.txtLength of output: 65
Script:
#!/bin/bash # Description: Verify the existence and content of the TestChanOpenTry function. # Step 1: Locate the TestChanOpenTry function using ripgrep. rg 'TestChanOpenTry' > test_chan_open_try_function.txt # Step 2: Output the content of the located function for manual inspection. cat test_chan_open_try_function.txtLength of output: 428
Script:
#!/bin/bash # Description: Extract and inspect the content of the TestChanOpenTry function from both identified files. # Step 1: Extract the content of the TestChanOpenTry function from ibc_middleware_test.go. ast-grep --lang go --pattern 'func (suite *InterchainAccountsTestSuite) TestChanOpenTry() { $$$ }' modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go > ibc_middleware_test_function.txt # Step 2: Extract the content of the TestChanOpenTry function from handshake_test.go. ast-grep --lang go --pattern 'func (suite *KeeperTestSuite) TestChanOpenTry() { $$$ }' modules/core/04-channel/keeper/handshake_test.go > handshake_test_function.txt # Step 3: Output the content of the located functions for manual inspection. cat ibc_middleware_test_function.txt cat handshake_test_function.txtLength of output: 17748
modules/apps/29-fee/ibc_middleware_test.go (2)
76-141: Ensure comprehensive testing of channel ordering inTestOnChanOpenInit.The addition of a loop to test both
UNORDEREDandORDEREDchannel types is a good practice to ensure that the system behaves correctly under different configurations. This change aligns with the PR's objective to handle different channel orderings robustly.
174-230: Ensure comprehensive testing of channel ordering inTestOnChanOpenTry.Similar to
TestOnChanOpenInit, the inclusion of a loop to test bothUNORDEREDandORDEREDchannel types inTestOnChanOpenTryis a positive change. It ensures that the channel opening logic is thoroughly tested across different channel orderings, which is crucial for maintaining the robustness of the system.
| for _, ordering := range testedOrderings { | ||
| for _, encoding := range testedEncodings { | ||
| for _, tc := range testCases { | ||
| tc := tc | ||
|
|
||
| icaAddr, err := sdk.AccAddressFromBech32(storedAddr) | ||
| suite.Require().NoError(err) | ||
| suite.Run(tc.msg, func() { | ||
| suite.SetupTest() // reset | ||
|
|
||
| // Check if account is created | ||
| interchainAccount := suite.chainB.GetSimApp().AccountKeeper.GetAccount(suite.chainB.GetContext(), icaAddr) | ||
| suite.Require().Equal(interchainAccount.GetAddress().String(), storedAddr) | ||
| path = NewICAPath(suite.chainA, suite.chainB, encoding, ordering) | ||
| path.SetupConnections() | ||
|
|
||
| suite.fundICAWallet(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(1000000)))) | ||
|
|
||
| tc.malleate(encoding) // malleate mutates test data | ||
| err := SetupICAPath(path, TestOwnerAddress) | ||
| suite.Require().NoError(err) | ||
|
|
||
| packet := channeltypes.NewPacket( | ||
| packetData, | ||
| suite.chainA.SenderAccount.GetSequence(), | ||
| path.EndpointA.ChannelConfig.PortID, | ||
| path.EndpointA.ChannelID, | ||
| path.EndpointB.ChannelConfig.PortID, | ||
| path.EndpointB.ChannelID, | ||
| suite.chainB.GetTimeoutHeight(), | ||
| 0, | ||
| ) | ||
| portID, err := icatypes.NewControllerPortID(TestOwnerAddress) | ||
| suite.Require().NoError(err) | ||
|
|
||
| txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) | ||
| // Get the address of the interchain account stored in state during handshake step | ||
| storedAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, portID) | ||
| suite.Require().True(found) | ||
|
|
||
| expPass := tc.expErr == nil | ||
| if expPass { | ||
| icaAddr, err := sdk.AccAddressFromBech32(storedAddr) | ||
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(txResponse) | ||
| } else { | ||
| suite.Require().ErrorIs(err, tc.expErr) | ||
| suite.Require().Nil(txResponse) | ||
| } | ||
| }) | ||
|
|
||
| // Check if account is created | ||
| interchainAccount := suite.chainB.GetSimApp().AccountKeeper.GetAccount(suite.chainB.GetContext(), icaAddr) | ||
| suite.Require().Equal(interchainAccount.GetAddress().String(), storedAddr) | ||
|
|
||
| suite.fundICAWallet(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(1000000)))) | ||
|
|
||
| tc.malleate(encoding) // malleate mutates test data | ||
|
|
||
| packet := channeltypes.NewPacket( | ||
| packetData, | ||
| suite.chainA.SenderAccount.GetSequence(), | ||
| path.EndpointA.ChannelConfig.PortID, | ||
| path.EndpointA.ChannelID, | ||
| path.EndpointB.ChannelConfig.PortID, | ||
| path.EndpointB.ChannelID, | ||
| suite.chainB.GetTimeoutHeight(), | ||
| 0, | ||
| ) | ||
|
|
||
| txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) | ||
|
|
||
| expPass := tc.expErr == nil | ||
| if expPass { | ||
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(txResponse) | ||
| } else { | ||
| suite.Require().ErrorIs(err, tc.expErr) | ||
| suite.Require().Nil(txResponse) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Review the nested loop structure for clarity and efficiency.
The nested loops over ordering and encoding types, and then over test cases, could be refactored for better readability and potential performance improvements. Consider separating these concerns or simplifying the control flow.
| @@ -20,7 +20,7 @@ | |||
| ) | |||
|
|
|||
| // open and close channel is a helper function for TestOnChanOpenTry for reopening accounts | |||
| func (suite *KeeperTestSuite) openAndCloseChannel(path *ibctesting.Path) { | |||
| func (suite *KeeperTestSuite) openAndCloseChannel(path *ibctesting.Path, ordering channeltypes.Order) { | |||
There was a problem hiding this comment.
Address the unused parameter ordering in the function openAndCloseChannel.
The parameter ordering is passed to the function but not used within its body. This could be an oversight or a design decision pending implementation. If it's intended for future use, consider documenting this; otherwise, it should be removed to clean up the function signature.
Tools
GitHub Check: lint
[failure] 23-23:
(*KeeperTestSuite).openAndCloseChannel-orderingis unused (unparam)
There was a problem hiding this comment.
Good Cath, rabbit! I will update the code.
There was a problem hiding this comment.
@crodriguezvega, thank you for the update! If there's anything else you need help with as you make the changes, feel free to ask. Happy coding! 🐇
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| for _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} { | ||
| suite.SetupTest() // reset | ||
| path := NewICAPath(suite.chainA, suite.chainB, ordering) | ||
| path.SetupConnections() | ||
|
|
||
| // use chainB (host) for ChanOpenInit | ||
| msg := channeltypes.NewMsgChannelOpenInit(path.EndpointB.ChannelConfig.PortID, icatypes.Version, channeltypes.ORDERED, []string{path.EndpointB.ConnectionID}, path.EndpointA.ChannelConfig.PortID, icatypes.ModuleName) | ||
| handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) | ||
| _, err := handler(suite.chainB.GetContext(), msg) | ||
| // use chainB (host) for ChanOpenInit | ||
| msg := channeltypes.NewMsgChannelOpenInit(path.EndpointB.ChannelConfig.PortID, icatypes.Version, ordering, []string{path.EndpointB.ConnectionID}, path.EndpointA.ChannelConfig.PortID, icatypes.ModuleName) | ||
| handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) | ||
| _, err := handler(suite.chainB.GetContext(), msg) | ||
|
|
||
| suite.Require().Error(err) | ||
| suite.Require().Error(err) | ||
| } |
There was a problem hiding this comment.
Refactor repetitive test setup in TestChanOpenInit.
Consider extracting the repetitive setup logic into a helper function to improve code readability and maintainability.
- suite.SetupTest() // reset
+ suite.resetTestEnvironment()Committable suggestion was skipped due to low confidence.
| "success w/ UNORDERED channel", func() { | ||
| channel.Ordering = channeltypes.UNORDERED | ||
| }, true, | ||
| "success", func() {}, true, |
There was a problem hiding this comment.
Enhance clarity of test case structuring in TestOnChanOpenTry.
The test cases are well-structured, but adding comments or separating sections can enhance readability and maintainability.
+ // Define test scenarios
testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success", func() {}, true,
},
// Add more cases here
}Also applies to: 177-230
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "success", func() {}, true, | |
| + // Define test scenarios | |
| testCases := []struct { | |
| name string | |
| malleate func() | |
| expPass bool | |
| }{ | |
| { | |
| "success", func() {}, true, | |
| }, | |
| // Add more cases here | |
| } |
| for _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} { | ||
| for _, tc := range testCases { | ||
| tc := tc | ||
|
|
||
| suite.Run(tc.name, func() { | ||
| suite.SetupTest() | ||
| path := NewICAPath(suite.chainA, suite.chainB) | ||
| path.SetupConnections() | ||
| suite.Run(tc.name, func() { | ||
| suite.SetupTest() | ||
| path := NewICAPath(suite.chainA, suite.chainB, ordering) | ||
| path.SetupConnections() | ||
|
|
||
| err := RegisterInterchainAccount(path.EndpointA, TestOwnerAddress) | ||
| suite.Require().NoError(err) | ||
| err := RegisterInterchainAccount(path.EndpointA, TestOwnerAddress) | ||
| suite.Require().NoError(err) | ||
|
|
||
| err = path.EndpointB.ChanOpenTry() | ||
| suite.Require().NoError(err) | ||
| err = path.EndpointB.ChanOpenTry() | ||
| suite.Require().NoError(err) | ||
|
|
||
| err = path.EndpointA.ChanOpenAck() | ||
| suite.Require().NoError(err) | ||
| err = path.EndpointA.ChanOpenAck() | ||
| suite.Require().NoError(err) | ||
|
|
||
| tc.malleate() | ||
| tc.malleate() | ||
|
|
||
| module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) | ||
| suite.Require().NoError(err) | ||
| module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) | ||
| suite.Require().NoError(err) | ||
|
|
||
| cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) | ||
| suite.Require().True(ok) | ||
| cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) | ||
| suite.Require().True(ok) | ||
|
|
||
| err = cbs.OnChanOpenConfirm(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) | ||
| err = cbs.OnChanOpenConfirm(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) | ||
|
|
||
| if tc.expPass { | ||
| suite.Require().NoError(err) | ||
| } else { | ||
| suite.Require().Error(err) | ||
| } | ||
| }) | ||
| if tc.expPass { | ||
| suite.Require().NoError(err) | ||
| } else { | ||
| suite.Require().Error(err) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Optimize test case execution in TestOnChanOpenConfirm.
Consider using table-driven tests to optimize the execution and readability of multiple test cases.
+ // Table-driven approach for handling multiple test scenarios
for _, tc := range testCases {
suite.Run(tc.name, func() {
suite.SetupTest()
path := NewICAPath(suite.chainA, suite.chainB, ordering)
path.SetupConnections()
// Test logic here
})
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (5 hunks)
- modules/apps/27-interchain-accounts/host/keeper/handshake_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
Additional context used
Path-based instructions (1)
modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go (2)
18-18: Ensure theorderingparameter is utilized correctly within theopenAndCloseChannelfunction.
427-427: Verify the initialization and correct usage ofexpectedOrderingin assertions within theTestRegisterInterchainAccount_MsgServerfunction.
modules/apps/27-interchain-accounts/controller/keeper/msg_server.go
Outdated
Show resolved
Hide resolved
…er.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
…t-in-new-ica-channels
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (1)
Line range hint
37-37: Address undefined methods and fields inmsgServer.Several methods such as
IsMiddlewareEnabled,SetMiddlewareDisabled,registerInterchainAccount, andLoggerare called onmsgServerbut are not defined within it. This could be due to missing method implementations or incorrect receiver types. Consider defining these methods or ensuring they are accessible through the correct type.Also applies to: 41-41, 49-49, 51-51, 55-55, 75-75
CHANGELOG.md (3)
58-58: Consider removing trailing spaces for consistency and to adhere to Markdown best practices.
Line range hint
204-204: Avoid using bare URLs. Consider using Markdown link syntax to make the text more readable and the document more professional.
Line range hint
219-219: Remove trailing spaces to maintain clean and professional documentation.Also applies to: 269-269
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (2 hunks)
Additional context used
Path-based instructions (2)
modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
golangci-lint
modules/apps/27-interchain-accounts/controller/keeper/msg_server.go
19-19: undefined: Keeper (typecheck)
16-16: cannot use (*msgServer)(nil) (value of type *msgServer) as "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types".MsgServer value in variable declaration: *msgServer does not implement "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types".MsgServer (missing method UpdateParams) (typecheck)
24-24: undefined: Keeper (typecheck)
84-84: undefined: Keeper (typecheck)
37-37: s.IsMiddlewareEnabled undefined (type msgServer has no field or method IsMiddlewareEnabled) (typecheck)
41-41: s.SetMiddlewareDisabled undefined (type msgServer has no field or method SetMiddlewareDisabled) (typecheck)
49-49: s.registerInterchainAccount undefined (type msgServer has no field or method registerInterchainAccount, but does have RegisterInterchainAccount) (typecheck)
51-51: s.Logger undefined (type msgServer has no field or method Logger) (typecheck)
55-55: s.Logger undefined (type msgServer has no field or method Logger) (typecheck)
75-75: s.sendTx undefined (type msgServer has no field or method sendTx, but does have SendTx) (typecheck)
25-25: cannot use &msgServer{…} (value of type *msgServer) as "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types".MsgServer value in return statement: *msgServer does not implement "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types".MsgServer (missing method UpdateParams) (typecheck)
Markdownlint
CHANGELOG.md
219-219: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
269-269: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
204-204: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
modules/apps/27-interchain-accounts/controller/keeper/msg_server.go (1)
43-47: Set default ordering toUNORDEREDifNONEis specified.This change correctly implements the desired behavior as per the PR objectives. Ensure that all dependent functions and methods are correctly defined and accessible within
msgServer.
…t-in-new-ica-channels
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
221-221: Remove trailing spaces to adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#6433](https://github.com/cosmos/ibc-go/pull/6433) Use UNORDERED as the default ordering for new ICA channels. + * (apps/27-interchain-accounts) [\#6433](https://github.com/cosmos/ibc-go/pull/6433) Use UNORDERED as the default ordering for new ICA channels.
Line range hint
271-271: Remove trailing spaces to adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. + * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels.
Line range hint
206-206: Avoid using bare URLs in Markdown files to improve readability and maintainability.- * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. + * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. See [PR #6251](https://github.com/cosmos/ibc-go/pull/6251) for more details.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
CHANGELOG.md
221-221: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
271-271: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
206-206: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
58-58: Ensure the changelog entry for the default ordering change is accurate and complete.Verification successful
The changelog entry for the default ordering change is accurate and complete. The PR #6433 indeed includes changes related to the default ordering for new ICA channels, as described in the changelog.
- The changelog entry correctly references the PR and describes the change made.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changelog entry for the default ordering change. # Test: Search for the changelog entry. Expect: Entry for default ordering change. rg --type markdown $'Use UNORDERED as the default ordering for new ICA channels' CHANGELOG.mdLength of output: 239
Script:
#!/bin/bash # Description: Verify the changes in PR #6433 related to the default ordering for new ICA channels. # Fetch the details of PR #6433 gh pr view 6433 --json title,body,files,commits # Check the files changed in the PR for changes related to default ordering gh pr diff 6433Length of output: 66444
|
DimitrisJim
deleted the
carlos/6264-use-unordered-ordering-by-default-in-new-ica-channels
branch
Description
closes: #6264
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
UnmarshalPacketDatainterface to include additional parameters for context, portID, and channelID.Documentation
UnmarshalPacketDatainterface.