fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic #6436
Conversation
…e to AllowUnresolvable field
GAtom22
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughThe recent changes in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Application
participant ICA_Host_Keeper
participant ProtoRegistry
Application->>ICA_Host_Keeper: Instantiate
ICA_Host_Keeper->>ProtoRegistry: Register File with AllowUnresolvable = true
ProtoRegistry-->>ICA_Host_Keeper: File Registered
ICA_Host_Keeper-->>Application: Instantiation Successful
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: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/27-interchain-accounts/host/keeper/keeper.go (2 hunks)
Additional context used
Path-based instructions (1)
modules/apps/27-interchain-accounts/host/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
modules/apps/27-interchain-accounts/host/keeper/keeper.go (1)
280-288: RefactornewModuleQuerySafeAllowListto prevent panic by settingAllowUnresolvableto true.This change effectively addresses the issue described in the PR, ensuring that the application does not panic due to unresolved imports. It's a crucial fix for stability, especially during the instantiation of the ICA host keeper.
GAtom22
requested review from
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
CHANGELOG.md (5)
Line range hint
57-57: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.
Line range hint
219-219: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.
Line range hint
269-269: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.
70-70: Remove the extra blank line to maintain a consistent structure in the markdown file.
Line range hint
204-204: Consider replacing the bare URL with a markdown link to improve the readability and presentation of the document.
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
57-57: Expected: 0 or 2; Actual: 1
Trailing spaces
219-219: Expected: 0 or 2; Actual: 1
Trailing spaces
269-269: Expected: 0 or 2; Actual: 1
Trailing spaces
70-70: Expected: 1; Actual: 2
Multiple consecutive blank lines
204-204: null
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
68-69: The changelog entry for PR #6436 correctly documents the changes made to the ICA host keeper instantiation method to prevent panics related to proto files.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
220-220: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.- * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. + * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files.Tools
Markdownlint
71-71: Expected: 1; Actual: 2
Multiple consecutive blank lines
Line range hint
270-270: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.- * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. + * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files.Tools
Markdownlint
71-71: Expected: 1; Actual: 2
Multiple consecutive blank lines
Line range hint
205-205: Consider replacing the bare URL with a markdown link for better readability and to adhere to best practices.- * [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. + * [\#6436](https://github.com/cosmos/ibc-go/pull/6436) [Refactor ICA host keeper instantiation method to avoid panic related to proto files.](https://github.com/cosmos/ibc-go/pull/6436)Tools
Markdownlint
71-71: Expected: 1; Actual: 2
Multiple consecutive blank lines
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
220-220: Expected: 0 or 2; Actual: 1
Trailing spaces
270-270: Expected: 0 or 2; Actual: 1
Trailing spaces
71-71: Expected: 1; Actual: 2
Multiple consecutive blank lines
205-205: null
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
69-70: The changelog entry correctly summarizes the changes made to the ICA host keeper instantiation method to prevent panics related to proto files.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
220-220: Remove trailing spaces to maintain consistent formatting.- * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. + * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files.Tools
Markdownlint
71-71: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
Line range hint
270-270: Remove trailing spaces to maintain consistent formatting.- * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. + * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files.Tools
Markdownlint
71-71: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
Line range hint
205-205: Avoid using bare URLs. Provide a descriptive hyperlink text.- https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.0 + [Cosmos SDK v0.40.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.0)Tools
Markdownlint
71-71: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
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
220-220: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
270-270: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
71-71: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
205-205: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
71-71: Remove extra blank line to maintain consistent spacing in the markdown file.-Likely invalid or redundant comment.
Tools
Markdownlint
71-71: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
There was a problem hiding this comment.
Thanks @GAtom22 for explaining the issue and providing a fix! I'm not sure I fully understand why this issue occurs and if there's other solutions that would avoid it, but I am fine with this solution to move things forward
Co-authored-by: colin axnér <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
222-222: Remove trailing spaces to adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. + * (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files.
Line range hint
272-272: Remove trailing spaces to adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#5785](https://github.com/cosmos/ibc-go/pull/5785) Introduce a new tx message that ICA host submodule can use to query the chain (only those marked with `module_query_safe`) and write the responses to the acknowledgement. + * (apps/27-interchain-accounts) [\#5785](https://github.com/cosmos/ibc-go/pull/5785) Introduce a new tx message that ICA host submodule can use to query the chain (only those marked with `module_query_safe`) and write the responses to the acknowledgement.
Line range hint
207-207: Enclose bare URL in angle brackets to adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#5633](https://github.com/cosmos/ibc-go/pull/5633) Allow new ICA channels to use unordered ordering. + * (apps/27-interchain-accounts) [\#5633](<https://github.com/cosmos/ibc-go/pull/5633>) Allow new ICA channels to use unordered ordering.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper.go (2 hunks)
Additional context used
Path-based instructions (2)
modules/apps/27-interchain-accounts/host/keeper/keeper.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/host/keeper/keeper.go
107-107: func
Keeper.getConnectionIDis unused (unused)
116-116: func
Keeper.setPortis unused (unused)
122-122: func
Keeper.hasCapabilityis unused (unused)
143-143: func
Keeper.getAppMetadatais unused (unused)
Markdownlint
CHANGELOG.md
222-222: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
272-272: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
207-207: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
modules/apps/27-interchain-accounts/host/keeper/keeper.go (1)
280-288: The refactoring ofnewModuleQuerySafeAllowListto useprotodesc.FileOptionswithAllowUnresolvable = trueis a crucial improvement. This change effectively addresses the issue of application panics due to unresolved imports, aligning with the PR objectives.Consider adding a comment explaining why
AllowUnresolvableis set to true, as suggested in a previous review. This will enhance code readability and maintainability.
…6436) * fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic due to AllowUnresolvable field * add changelog entry * Apply suggestions from code review Co-authored-by: colin axnér <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: colin axnér <[email protected]> (cherry picked from commit a16c549) # Conflicts: # CHANGELOG.md
…6436) * fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic due to AllowUnresolvable field * add changelog entry * Apply suggestions from code review Co-authored-by: colin axnér <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: colin axnér <[email protected]> (cherry picked from commit a16c549) # Conflicts: # CHANGELOG.md
…ackport #6436) (#6519) * fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic (#6436) * fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic due to AllowUnresolvable field * add changelog entry * Apply suggestions from code review Co-authored-by: colin axnér <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: colin axnér <[email protected]> (cherry picked from commit a16c549) # Conflicts: # CHANGELOG.md * fix conflicts in changelog. --------- Co-authored-by: Tom <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
…ackport #6436) (#6520) * fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic (#6436) * fix(ica-host): refactor newModuleQuerySafeAllowList to avoid panic due to AllowUnresolvable field * add changelog entry * Apply suggestions from code review Co-authored-by: colin axnér <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: colin axnér <[email protected]> (cherry picked from commit a16c549) # Conflicts: # CHANGELOG.md * fix conflicts in changelog. --------- Co-authored-by: Tom <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
|
It doesn't look fixed in 8.5.0 FYI: ignite/cli#4318 (comment) |
Description
This PR introduces the changes to avoid triggering a panic during ICA host keeper instantiation.
The change gets the protofiles using
AllowUnresolvable = trueinstead of using thegogoproto.MergedRegistry()function that returns the files using theAllowUnresolvable = falsecloses: #6435
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