refactor: remove gov dependency from modules and sdk #18424
Conversation
WalkthroughWalkthroughThe changes primarily involve the introduction of a new Changes
TipsChat with CodeRabbit Bot (
|
github-actions
bot
added
C:x/staking
C:x/slashing
C:x/distribution
github-prbot
requested review from
a team,
alexanderbez and
testinginprod
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (17)
- client/v2/go.mod
- go.mod
- x/accounts/go.mod
- x/auth/go.mod
- x/bank/go.mod
- x/circuit/go.mod
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/mint/go.mod
- x/nft/go.mod
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
Files selected for processing (20)
- baseapp/block_gas_test.go (3 hunks)
- go.work.example (1 hunks)
- testutil/configurator/configurator.go (10 hunks)
- testutil/types.go (1 hunks)
- x/crisis/keeper/msg_server.go (2 hunks)
- x/crisis/module.go (2 hunks)
- x/crisis/types/errors.go (1 hunks)
- x/crisis/types/keys.go (1 hunks)
- x/distribution/types/keys.go (1 hunks)
- x/protocolpool/testutil/app_config.go (2 hunks)
- x/protocolpool/types/keys.go (1 hunks)
- x/slashing/keeper/keeper_test.go (2 hunks)
- x/slashing/keeper/msg_server.go (2 hunks)
- x/slashing/module.go (2 hunks)
- x/slashing/types/errors.go (1 hunks)
- x/slashing/types/keys.go (1 hunks)
- x/upgrade/keeper/grpc_query_test.go (1 hunks)
- x/upgrade/keeper/keeper_test.go (2 hunks)
- x/upgrade/module.go (2 hunks)
- x/upgrade/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (12)
- baseapp/block_gas_test.go
- go.work.example
- testutil/types.go
- x/crisis/types/errors.go
- x/crisis/types/keys.go
- x/distribution/types/keys.go
- x/protocolpool/types/keys.go
- x/slashing/keeper/msg_server.go
- x/slashing/types/errors.go
- x/slashing/types/keys.go
- x/upgrade/module.go
- x/upgrade/types/keys.go
Additional comments: 23
x/upgrade/keeper/keeper_test.go (1)
- 66-71: The change from
govModuleNametotypes.GovModuleNameis correct and aligns with the goal of removing the dependency on thex/govmodule. Ensure that thetypes.GovModuleNameconstant is correctly defined and that it matches the previous value ofgovModuleName.x/upgrade/keeper/grpc_query_test.go (1)
- 41-47: The change from
govModuleNametotypes.GovModuleNameis consistent with the PR's goal of removing the dependency on thex/govmodule. Ensure thattypes.GovModuleNameis correctly defined and imported.x/protocolpool/testutil/app_config.go (2)
5-10: The removal of the
x/govmodule from the import statements is consistent with the goal of decoupling it from the SDK. Ensure that all dependencies on this module have been properly refactored throughout the codebase.24-27: The
x/govmodule is no longer part of the application configuration. This change should be verified across all application setups to ensure consistency.x/crisis/keeper/msg_server.go (2)
4-9: The import of "cosmossdk.io/x/gov/types" has been removed. Ensure that this does not cause any undefined types or functions in the current file.
79-81: The error type has been updated from
govtypes.ErrInvalidSignertotypes.ErrInvalidSigner. Ensure that the new error type is defined in the "types" package and is used consistently across the codebase.- return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority) + return nil, errors.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority)x/slashing/keeper/keeper_test.go (2)
13-19: The import of "cosmossdk.io/x/gov/types" is removed and "cosmossdk.io/x/slashing/types" is added. Ensure that all the references to the "govtypes" are replaced with "types" in the entire codebase.
58-59: The reference to "govtypes.ModuleName" is replaced with "types.GovModuleName". This change is consistent with the removal of the "govtypes" import.
x/slashing/module.go (2)
12-17: The import statements are well organized and follow the best practices of grouping them by standard library, external packages, and internal packages.
217-223: The
ProvideModulefunction has been updated to usetypes.GovModuleNameinstead ofgovtypes.ModuleName. Ensure thattypes.GovModuleNameis correctly defined and that all references togovtypes.ModuleNamehave been replaced throughout the codebase.x/crisis/module.go (2)
16-21: The import of
cosmossdk.io/x/gov/typeshas been removed. Ensure that this does not cause any undefined references in the code.200-206: The usage of
govtypes.ModuleNamehas been replaced withtypes.GovModuleName. Ensure thattypes.GovModuleNameis correctly defined and imported.- authority := authtypes.NewModuleAddress(govtypes.ModuleName) + authority := authtypes.NewModuleAddress(types.GovModuleName)testutil/configurator/configurator.go (11)
8-14: The new module
countermodulev1is imported and used in theCounterModulefunction. Ensure that this module is implemented correctly and that all dependencies are properly managed.42-109: The
defaultConfigfunction has been updated to use constants from thetestutilpackage instead of hard-coded strings. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.138-164: The
BankModuleandAuthModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.167-201: The
ParamsModule,TxModule,StakingModule, andSlashingModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.212-228: The
DistributionModuleandFeegrantModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.239-246: The
GovModulefunction has been updated to use a constant from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.257-264: The
MintModulefunction has been updated to use a constant from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.272-287: The
EvidenceModuleandAuthzModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.291-323: The
GroupModule,NFTModule,CircuitModule, andProtocolPoolModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the chance of typos and makes it easier to manage these values.327-332: The
CounterModulefunction has been added. Ensure that this module is implemented correctly and that all dependencies are properly managed.375-380: The
AuthModuleNameis checked in thecfg.ModuleConfigsand if it exists, a newStoreKeyConfigis appended tooverrides. This is a good practice as it ensures that theAuthModuleNameis properly configured before it is used.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (17)
- client/v2/go.mod
- go.mod
- x/accounts/go.mod
- x/auth/go.mod
- x/bank/go.mod
- x/circuit/go.mod
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/mint/go.mod
- x/nft/go.mod
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
Files selected for processing (20)
- baseapp/block_gas_test.go (3 hunks)
- go.work.example (1 hunks)
- testutil/configurator/configurator.go (10 hunks)
- testutil/types.go (1 hunks)
- x/crisis/keeper/msg_server.go (2 hunks)
- x/crisis/module.go (2 hunks)
- x/crisis/types/errors.go (1 hunks)
- x/crisis/types/keys.go (1 hunks)
- x/distribution/types/keys.go (1 hunks)
- x/protocolpool/testutil/app_config.go (2 hunks)
- x/protocolpool/types/keys.go (1 hunks)
- x/slashing/keeper/keeper_test.go (2 hunks)
- x/slashing/keeper/msg_server.go (2 hunks)
- x/slashing/module.go (2 hunks)
- x/slashing/types/errors.go (1 hunks)
- x/slashing/types/keys.go (1 hunks)
- x/upgrade/keeper/grpc_query_test.go (1 hunks)
- x/upgrade/keeper/keeper_test.go (2 hunks)
- x/upgrade/module.go (2 hunks)
- x/upgrade/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (8)
- go.work.example
- x/crisis/types/keys.go
- x/distribution/types/keys.go
- x/protocolpool/types/keys.go
- x/slashing/types/errors.go
- x/slashing/types/keys.go
- x/upgrade/keeper/keeper_test.go
- x/upgrade/types/keys.go
Additional comments: 27
x/crisis/types/errors.go (1)
- 6-10: The new error
ErrInvalidSigneris correctly registered with a unique error code and a descriptive message. Ensure that this error is handled appropriately in the code where it's expected to be thrown.x/upgrade/keeper/grpc_query_test.go (1)
- 41-47: Ensure that the change from
authtypes.NewModuleAddress(govModuleName)toauthtypes.NewModuleAddress(types.GovModuleName)does not affect the functionality of theUpgradeTestSuitesetup. Verify that thetypes.GovModuleNameis correctly defined and that theauthorityis correctly generated and used in thekeeper.NewKeeperfunction.testutil/types.go (1)
- 1-25: The introduction of the
testutilpackage with constants for module names is a good practice. It helps to avoid hard-coded strings scattered throughout the codebase, which can lead to potential errors and maintainability issues. However, ensure that all instances where these module names are used have been updated to use these constants.baseapp/block_gas_test.go (1)
- 26-32: The new import statement for "github.com/cosmos/cosmos-sdk/testutil" is added correctly.
x/protocolpool/testutil/app_config.go (2)
5-10: The removal of the
x/govimport is consistent with the PR's goal of removing the dependency on thex/govmodule. Ensure that this does not affect any functionality that relies on this import.24-27: The removal of
configurator.GovModule()is consistent with the PR's goal of removing the dependency on thex/govmodule. Ensure that this does not affect any functionality that relies on this module.x/upgrade/module.go (2)
29-34: The
initfunction is used to register the legacy Amino codec. This is a good practice as it ensures that the codec is registered at the package level and is available for use throughout the package.205-211: The
ProvideModulefunction has been updated to usetypes.GovModuleNameinstead of a hardcoded string. This is a good practice as it reduces the risk of typos and makes it easier to update the module name if needed in the future. However, ensure thattypes.GovModuleNameis correctly defined and imported.x/crisis/module.go (2)
16-21: The import statements have been updated to remove the dependency on
x/gov/types. This is in line with the PR's goal of removing the dependency on thex/govmodule.200-206: The
govtypes.ModuleNamehas been replaced withtypes.GovModuleNameto avoid a cyclic dependency. This change affects the determination of the default authority address. Ensure thattypes.GovModuleNameis correctly defined and that this change does not affect the functionality of theProvideModulefunction.x/slashing/module.go (1)
- 12-17: The import statements have been updated to remove the dependency on
x/gov/typesand replace it with the respective types from thex/slashingmodule. This change is in line with the goal of removing the dependency on thex/govmodule from other modules and the SDK itself.x/crisis/keeper/msg_server.go (2)
4-9: The import of "cosmossdk.io/x/gov/types" has been removed and replaced with "types". This change is consistent with the PR's goal of removing the dependency on the
x/govmodule.77-81: The error type
govtypes.ErrInvalidSignerhas been replaced withtypes.ErrInvalidSigner. Ensure that the new error type is defined in thetypespackage and is equivalent to the old one.- return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority) + return nil, errors.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority)x/slashing/keeper/keeper_test.go (1)
- 58-59: The reference to "govtypes.ModuleName" has been replaced with "types.GovModuleName". Ensure that the new reference is correctly defined and imported.
x/slashing/keeper/msg_server.go (2)
4-9: The import of "cosmossdk.io/x/gov/types" has been replaced with "cosmossdk.io/x/slashing/types". Ensure that the new import does not introduce any breaking changes and all the required types are available in the new package.
25-31: The error type has been changed from
govtypes.ErrInvalidSignertotypes.ErrInvalidSigner. Make sure that the new error type is handled correctly wherever this function is called.- return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority) + return nil, errors.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority)testutil/configurator/configurator.go (11)
8-14: The new module
countermodulev1has been added to the import statements. Ensure that this module is correctly implemented and used throughout the codebase.42-109: The
defaultConfigfunction has been updated to use constants from thetestutilpackage instead of hard-coded strings. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.138-164: The
BankModuleandAuthModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.167-201: The
ParamsModule,TxModule,StakingModule, andSlashingModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.212-228: The
DistributionModuleandFeegrantModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.239-246: The
GovModulefunction has been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.257-264: The
MintModulefunction has been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.272-287: The
EvidenceModule,AuthzModule, andGroupModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.300-315: The
NFTModule,CircuitModule, andProtocolPoolModulefunctions have been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.327-333: The
CounterModulefunction has been added. Ensure that this function is correctly implemented and used throughout the codebase.375-380: The
NewAppConfigfunction has been updated to use constants from thetestutilpackage. This is a good practice as it reduces the risk of typos and makes it easier to manage these constants.
| sdkmath "cosmossdk.io/math" | ||
| storetypes "cosmossdk.io/store/types" | ||
| authtypes "cosmossdk.io/x/auth/types" | ||
| govtypes "cosmossdk.io/x/gov/types" | ||
| slashingkeeper "cosmossdk.io/x/slashing/keeper" | ||
| slashingtestutil "cosmossdk.io/x/slashing/testutil" | ||
| "cosmossdk.io/x/slashing/types" | ||
| slashingtypes "cosmossdk.io/x/slashing/types" |
There was a problem hiding this comment.
The import of "cosmossdk.io/x/slashing/types" is duplicated. Remove the duplicate import.
- "cosmossdk.io/x/slashing/types"Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| sdkmath "cosmossdk.io/math" | |
| storetypes "cosmossdk.io/store/types" | |
| authtypes "cosmossdk.io/x/auth/types" | |
| govtypes "cosmossdk.io/x/gov/types" | |
| slashingkeeper "cosmossdk.io/x/slashing/keeper" | |
| slashingtestutil "cosmossdk.io/x/slashing/testutil" | |
| "cosmossdk.io/x/slashing/types" | |
| slashingtypes "cosmossdk.io/x/slashing/types" | |
| sdkmath "cosmossdk.io/math" | |
| storetypes "cosmossdk.io/store/types" | |
| authtypes "cosmossdk.io/x/auth/types" | |
| slashingkeeper "cosmossdk.io/x/slashing/keeper" | |
| slashingtestutil "cosmossdk.io/x/slashing/testutil" | |
| slashingtypes "cosmossdk.io/x/slashing/types" |
|
|
||
| // test account and fund | ||
| priv1, _, addr1 := testdata.KeyTestPubAddr() | ||
| err = bankKeeper.MintCoins(ctx, mintModuleName, feeAmount) | ||
| err = bankKeeper.MintCoins(ctx, testutil.MintModuleName, feeAmount) | ||
| require.NoError(t, err) | ||
| err = bankKeeper.SendCoinsFromModuleToAccount(ctx, mintModuleName, addr1, feeAmount) | ||
| err = bankKeeper.SendCoinsFromModuleToAccount(ctx, testutil.MintModuleName, addr1, feeAmount) | ||
| require.NoError(t, err) | ||
| require.Equal(t, feeCoin.Amount, bankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount) | ||
| seq := accountKeeper.GetAccount(ctx, addr1).GetSequence() |
There was a problem hiding this comment.
The constant "mintModuleName" is replaced with "testutil.MintModuleName" correctly. This change improves maintainability by avoiding hard-coded strings.
- err = bankKeeper.MintCoins(ctx, mintModuleName, feeAmount)
+ err = bankKeeper.MintCoins(ctx, testutil.MintModuleName, feeAmount)
- err = bankKeeper.SendCoinsFromModuleToAccount(ctx, mintModuleName, addr1, feeAmount)
+ err = bankKeeper.SendCoinsFromModuleToAccount(ctx, testutil.MintModuleName, addr1, feeAmount)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| // test account and fund | |
| priv1, _, addr1 := testdata.KeyTestPubAddr() | |
| err = bankKeeper.MintCoins(ctx, mintModuleName, feeAmount) | |
| err = bankKeeper.MintCoins(ctx, testutil.MintModuleName, feeAmount) | |
| require.NoError(t, err) | |
| err = bankKeeper.SendCoinsFromModuleToAccount(ctx, mintModuleName, addr1, feeAmount) | |
| err = bankKeeper.SendCoinsFromModuleToAccount(ctx, testutil.MintModuleName, addr1, feeAmount) | |
| require.NoError(t, err) | |
| require.Equal(t, feeCoin.Amount, bankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount) | |
| seq := accountKeeper.GetAccount(ctx, addr1).GetSequence() | |
| // test account and fund | |
| priv1, _, addr1 := testdata.KeyTestPubAddr() | |
| err = bankKeeper.MintCoins(ctx, testutil.MintModuleName, feeAmount) | |
| require.NoError(t, err) | |
| err = bankKeeper.SendCoinsFromModuleToAccount(ctx, testutil.MintModuleName, addr1, feeAmount) | |
| require.NoError(t, err) | |
| require.Equal(t, feeCoin.Amount, bankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount) | |
| seq := accountKeeper.GetAccount(ctx, addr1).GetSequence() |
|
|
||
| func ProvideModule(in ModuleInputs) ModuleOutputs { | ||
| // default to governance authority if not provided | ||
| authority := authtypes.NewModuleAddress(govtypes.ModuleName) | ||
| authority := authtypes.NewModuleAddress(types.GovModuleName) | ||
| if in.Config.Authority != "" { | ||
| authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) | ||
| } |
There was a problem hiding this comment.
The hardcoded module name "govtypes.ModuleName" has been replaced with "types.GovModuleName". This change is part of the effort to replace hardcoded module names with constants, which improves maintainability and reduces potential errors related to hardcoded strings.
- authority := authtypes.NewModuleAddress(govtypes.ModuleName)
+ authority := authtypes.NewModuleAddress(types.GovModuleName)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| func ProvideModule(in ModuleInputs) ModuleOutputs { | |
| // default to governance authority if not provided | |
| authority := authtypes.NewModuleAddress(govtypes.ModuleName) | |
| authority := authtypes.NewModuleAddress(types.GovModuleName) | |
| if in.Config.Authority != "" { | |
| authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) | |
| } | |
| func ProvideModule(in ModuleInputs) ModuleOutputs { | |
| // default to governance authority if not provided | |
| authority := authtypes.NewModuleAddress(types.GovModuleName) | |
| if in.Config.Authority != "" { | |
| authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) | |
| } |
Description
ref: #18290
Extract a all x/gov changes from #18381 to extract x/gov from the SDK and modules.
Extracting from the original PR because I was getting issues.
Additionally, scoped PR are better :D
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
testutilpackage with constants representing module names.ErrInvalidSignerinx/crisis/typesandx/slashing/typesfor better error handling.testutilpackage across various parts of the code.x/govmodule by introducing new constants inx/crisis/types,x/upgrade/types, andx/slashing/types.x/distribution/typesandx/protocolpool/typesto explain the duplication of theGovModuleName.