Change signature of NewKeeper to directly accept a ConsensusHost #6084
Conversation
WalkthroughThe recent update focuses on enhancing the initialization logic of the IBC keeper by replacing the Changes
Assessment against linked issues
Possibly related 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 (
|
chatton
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk,
charleenfei,
DimitrisJim and
crodriguezvega
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
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- modules/apps/callbacks/testing/simapp/app.go (1 hunks)
- modules/core/02-client/keeper/keeper.go (2 hunks)
- modules/core/keeper/keeper.go (2 hunks)
- modules/core/keeper/keeper_test.go (6 hunks)
- modules/light-clients/08-wasm/testing/simapp/app.go (1 hunks)
- testing/simapp/app.go (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional Context Used
GitHub Check Runs (2)
tests (03) failure (5)
modules/core/keeper/keeper_test.go: [failure] 20-20:
ibctm redeclared in this block
modules/core/keeper/keeper_test.go: [failure] 8-8:
other declaration of ibctm
modules/core/keeper/keeper_test.go: [failure] 20-20:
"github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" imported as ibctm and not usedlint failure (6)
modules/core/keeper/keeper_test.go: [failure] 20-20:
ibctm redeclared in this block
modules/core/keeper/keeper_test.go: [failure] 8-8:
other declaration of ibctm
modules/core/keeper/keeper_test.go: [failure] 20-20:
"github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" imported as ibctm and not used (typecheck)
Path-based Instructions (7)
modules/core/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/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/core/02-client/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testing/simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/testing/simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/08-wasm/testing/simapp/app.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 (5)
modules/core/keeper/keeper.go (1)
40-53: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [43-63]
The changes to the
NewKeeperfunction, including the introduction of theconsensusHostparameter and the updated error messages, are correctly implemented and align with the PR objectives.modules/core/keeper/keeper_test.go (1)
74-84: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [66-136]
The updates to the
TestNewKeeperfunction's test cases correctly reflect the changes fromStakingKeepertoConsensusHost.modules/core/02-client/keeper/keeper.go (1)
33-39: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-45]
The changes to the
NewKeeperfunction, including the introduction of theconsensusHostparameter and its assignment in theKeeperstruct, are correctly implemented and align with the PR objectives.testing/simapp/app.go (1)
411-411: Ensure that the broader impact of changing theNewKeeperfunction's signature to accept aConsensusHostdirectly instead of aStakingKeeperis verified across the IBC module and related components.Verification successful
Given the absence of results from the initial verification scripts, it's possible that the search patterns used were too specific or that the scope of the search did not capture the relevant occurrences. To ensure a thorough verification, I recommend generating new verification scripts with adjusted search patterns or expanding the search scope. If the scripts still return no results, it might be necessary to request manual verification from the user or conclude based on the best available evidence.
The verification process has confirmed that the
NewConsensusHostfunction is properly defined and utilized within the IBC module and related components. Additionally, there are no unintended references to the oldStakingKeeperparameter within the IBC module's initialization. The references toStakingKeeperfound in the search are part of other modules' initialization processes and are intended. Therefore, the changes to theNewKeeperfunction's signature to accept aConsensusHostdirectly instead of aStakingKeeperhave been verified across the IBC module and related components without issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the NewConsensusHost function is properly defined and used elsewhere in the IBC module. ast-grep --lang go --pattern $'NewConsensusHost($_)' # Ensure no unintended references to the old StakingKeeper parameter remain in the IBC module initialization. ast-grep --lang go --pattern $'NewKeeper($$$, $_.StakingKeeper, $$$)'Length of output: 124
Script:
#!/bin/bash # Broaden the search for NewConsensusHost to ensure it's properly defined and used within the IBC module or related components. rg 'NewConsensusHost' --type go # Broaden the search to ensure no unintended references to the old StakingKeeper parameter remain in the IBC module initialization or related components. rg 'NewKeeper' --type go | grep 'StakingKeeper'Length of output: 3425
CHANGELOG.md (1)
46-46: Ensure the CHANGELOG entry for theNewKeeperfunction change incore/02-clientincludes details about the motivation and impact of the change.
| @@ -407,7 +407,7 @@ func NewSimApp( | |||
| app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, runtime.NewKVStoreService(keys[upgradetypes.StoreKey]), appCodec, homePath, app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String()) | |||
|
|
|||
| app.IBCKeeper = ibckeeper.NewKeeper( | |||
| appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), | |||
| appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), ibctm.NewConsensusHost(app.StakingKeeper), app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), | |||
There was a problem hiding this comment.
Ensure the NewConsensusHost function properly handles the StakingKeeper argument and any potential errors.
- ibctm.NewConsensusHost(app.StakingKeeper)
+ consensusHost, err := ibctm.NewConsensusHost(app.StakingKeeper)
+ if err != nil {
+ panic(fmt.Errorf("failed to create NewConsensusHost: %w", 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.
| appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), ibctm.NewConsensusHost(app.StakingKeeper), app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), | |
| appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), consensusHost, err := ibctm.NewConsensusHost(app.StakingKeeper) | |
| if err != nil { | |
| panic(fmt.Errorf("failed to create NewConsensusHost: %w", err)) | |
| }, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), |
modules/core/keeper/keeper_test.go
Outdated
| ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" | ||
|
|
||
| testifysuite "github.com/stretchr/testify/suite" | ||
|
|
||
| upgradekeeper "cosmossdk.io/x/upgrade/keeper" | ||
|
|
||
| stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" | ||
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | ||
|
|
||
| capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper" | ||
| clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" | ||
| ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" | ||
| ibckeeper "github.com/cosmos/ibc-go/v8/modules/core/keeper" | ||
| ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" |
There was a problem hiding this comment.
Remove the duplicate import of "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" as ibctm.
- ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"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.
| ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" | |
| testifysuite "github.com/stretchr/testify/suite" | |
| upgradekeeper "cosmossdk.io/x/upgrade/keeper" | |
| stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" | |
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | |
| capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper" | |
| clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" | |
| ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" | |
| ibckeeper "github.com/cosmos/ibc-go/v8/modules/core/keeper" | |
| ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" | |
| testifysuite "github.com/stretchr/testify/suite" | |
| upgradekeeper "cosmossdk.io/x/upgrade/keeper" | |
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | |
| capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper" | |
| clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" | |
| ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" | |
| ibckeeper "github.com/cosmos/ibc-go/v8/modules/core/keeper" | |
| ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/core/keeper/keeper_test.go (6 hunks)
- modules/light-clients/07-tendermint/consensus_host.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/core/keeper/keeper_test.go
Additional Context Used
Path-based Instructions (1)
modules/light-clients/07-tendermint/consensus_host.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/light-clients/07-tendermint/consensus_host.go (1)
37-39: The addition of a nil check forstakingKeeperinNewConsensusHostis a good practice to ensure that essential dependencies are not nil.
…-core-ibc-newkeeper-func
…-core-ibc-newkeeper-func
…-core-ibc-newkeeper-func
…-core-ibc-newkeeper-func
|
crodriguezvega
deleted the
cian/issue#6082-pass-consensushost-as-arg-to-core-ibc-newkeeper-func
branch
Description
closes: #6082
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
ConsensusHostimplementation instead of aStakingKeeper. This change aims to enhance clarity and functionality in the system's consensus mechanism.