Update ica simulated proposals to check which modules are instantianted. #6377
Conversation
Taztingo
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughThe changes involve updating the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant SimState
participant AppModule
participant ControllerKeeper
participant HostKeeper
SimState->>AppModule: Call ProposalMsgs(simState)
AppModule->>ControllerKeeper: Check if ControllerKeeper is provided
AppModule->>HostKeeper: Check if HostKeeper is provided
AppModule-->>SimState: Return proposal messages based on available keepers
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 (
|
Taztingo
requested review from
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
217-217: Remove trailing spaces to maintain clean and professional code formatting.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers.
Line range hint
267-267: Remove trailing spaces to maintain clean and professional code formatting.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers.
Line range hint
202-202: Consider replacing the bare URL with a markdown link to enhance readability and maintain consistency with other entries that use markdown links.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- modules/apps/27-interchain-accounts/module.go (1 hunks)
- modules/apps/27-interchain-accounts/simulation/proposals.go (2 hunks)
- modules/apps/27-interchain-accounts/simulation/proposals_test.go (2 hunks)
Additional Context Used
Markdownlint (3)
CHANGELOG.md (3)
217: Expected: 0 or 2; Actual: 1
Trailing spaces
267: Expected: 0 or 2; Actual: 1
Trailing spaces
202: null
Bare URL used
Path-based Instructions (4)
modules/apps/27-interchain-accounts/simulation/proposals.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/27-interchain-accounts/simulation/proposals_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/module.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"
Additional comments not posted (4)
modules/apps/27-interchain-accounts/simulation/proposals.go (1)
11-13: The new imports forcontrollerkeeperandhostkeeperare correctly placed and necessary for the updated function signature.modules/apps/27-interchain-accounts/simulation/proposals_test.go (1)
15-17: The new imports forcontrollerkeeperandhostkeeperare correctly placed and necessary for the updated test function.modules/apps/27-interchain-accounts/module.go (1)
198-199: The modification to theProposalMsgsfunction correctly delegates to the updatedsimulation.ProposalMsgs, ensuring consistency across the module with the new keeper parameters.CHANGELOG.md (1)
71-72: The changelog entry for PR #6377 correctly summarizes the changes made to the ICA simulation proposals. It's good to see that the changelog is being kept up-to-date with significant modifications.
modules/apps/27-interchain-accounts/simulation/proposals_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
57-57: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers.Also applies to: 219-219, 269-269
Line range hint
204-204: Consider replacing the bare URL with a markdown link to enhance the readability and presentation of the document.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [Bump Cosmos SDK to v0.50.6](https://github.com/cosmos/ibc-go/pull/6193).
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Markdownlint (4)
CHANGELOG.md (4)
57: Expected: 0 or 2; Actual: 1
Trailing spaces
219: Expected: 0 or 2; Actual: 1
Trailing spaces
269: Expected: 0 or 2; Actual: 1
Trailing spaces
204: null
Bare URL used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (1)
CHANGELOG.md (1)
73-74: The changelog entry for the ICA simtest proposals update is correctly documented under the "Bug Fixes" section.
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)
- 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
205-205: null
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
74-75: The changelog entry correctly documents the bug fix related to ICA simtest proposals.
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 adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers.
Line range hint
270-270: Remove trailing spaces to adhere to Markdown best practices.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers.
Line range hint
205-205: Avoid using bare URLs. Provide a descriptive link text.- * (apps/27-interchain-accounts) [\#6377](https://github.com/cosmos/ibc-go/pull/6377) Generate ICA simtest proposals only for provided keepers. + * (apps/27-interchain-accounts) [Generate ICA simtest proposals only for provided keepers](https://github.com/cosmos/ibc-go/pull/6377).
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
205-205: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
74-75: The changelog entry for the bug fix is well-documented and correctly links to the PR.
There was a problem hiding this comment.
Thanks @Taztingo for opening a fix!
cc @crodriguezvega I realized we have two options for backport:
- remove sim functionality on backports (allows for patch release)
- add new function (ProposalMsgsWithKeeper, possible in minor release)
| controller *controllerkeeper.Keeper | ||
| host *hostkeeper.Keeper | ||
| proposalMsgs int | ||
| isHost []bool |
There was a problem hiding this comment.
happy with this, but could also maybe do expMsgs []interface where test case 1 has []interface{msgHostUpdateParams, msgControllerUpdateParams} (we can create the expected host and controller msgs at the top of the test since they are predictable)
There was a problem hiding this comment.
That's a great idea. I also updated the tests to match your others so they include a name and run in a goroutine. Let me know if there is anything else.
There was a problem hiding this comment.
Could we use []sdk.Msg or []proto.Message instead of []interface to have stronger typing?
…ch. Add expMsgs since we know the expected messages before hand.
| name string | ||
| controller *controllerkeeper.Keeper | ||
| host *hostkeeper.Keeper | ||
| expMsgs []interface{} |
There was a problem hiding this comment.
I think we can use []sdk.Msg here? or at least proto.Message
There was a problem hiding this comment.
Thank you @Taztingo!
If you could sync the branch one more time please we can try to get this merged asap! And/or edit the PR to check the "allow edits from maintainers" box if possible (not sure if it can be done after the fact), so that we could keep it in sync ourselves without having to bother you! ❤️
Description
Updates the ICA proposals for simtests to only generate the proposals for provided keepers to have it match the functionality in v6. This was first found in v8.2.x that is currently used by Provenance.
closes: #6375
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