refactor(core,stf): complete gas service + simplify deps #21166
Conversation
WalkthroughWalkthroughThe changes introduce significant restructuring within the Cosmos SDK, focusing on dependency management and interface simplification. Several indirect dependencies have been removed, streamlining module interactions. Key interfaces related to service registration (such as Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Module
participant ServiceRegistrar as ServiceRegistrar
participant AppModule as AppModule
Module->>ServiceRegistrar: RegisterServices()
ServiceRegistrar-->>Module: Acknowledge registration
AppModule->>Module: Call methods
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 Configuration File (
|
github-actions
bot
added
C:Store
C:collections
C:server/v2
![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
Outside diff range, codebase verification and nitpick comments (3)
core/gas/service.go (1)
Update Required: References to
BlockGasMeterstill exist in the codebase.The removal of
BlockGasMeterfrom theServiceinterface has not been fully managed. The following files still contain references toBlockGasMeter:
types/context.gotypes/context_test.gotests/e2e/baseapp/block_gas_test.goruntime/gas.gostore/types/gas.gobaseapp/baseapp.gobaseapp/baseapp_test.gobaseapp/abci.gobaseapp/abci_test.goPlease update or remove these references to ensure consistency and avoid potential issues.
Analysis chain
Line range hint
15-20:
LGTM! But verify the impact of removingBlockGasMeter.The removal of
BlockGasMeterfrom theServiceinterface simplifies the interface. Ensure that all references toBlockGasMeterare removed or updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `BlockGasMeter` is no longer referenced in the codebase. # Test: Search for any remaining references to `BlockGasMeter`. Expect: No occurrences. rg --type go 'BlockGasMeter'Length of output: 2908
Script:
#!/bin/bash # Description: Extract context around references to `BlockGasMeter` to verify proper handling after its removal from the `Service` interface. # Extract context around each reference to `BlockGasMeter`. rg --type go --context 5 'BlockGasMeter'Length of output: 15884
server/v2/stf/README.md (1)
Line range hint
1-34:
Approve formatting changes but address grammatical issues.The formatting changes improve readability. Address the following grammatical issues:
- Use a hyphen in "read-only" (Line 5).
- Correct "THe" to "The" and "wrappGasMeter" to "wrapGasMeter" (Line 34).
- Consider shortening "in order to" to "to" (Line 34).
- The state transition function receives a read only instance of state. + The state transition function receives a read-only instance of state. - THe wrappGasMeter is used in order to consume gas. + The wrapGasMeter is used to consume gas.Tools
LanguageTool
[style] ~34-~34: Consider a shorter alternative to avoid wordiness.
Context: ...terMap ) ``` THe wrappGasMeter is used in order to consume gas. Application developers can...(IN_ORDER_TO_PREMIUM)
[style] ~34-~34: Consider a shorter alternative to avoid wordiness.
Context: ...gas meter with their own implementation in order to customize consumption of gas.(IN_ORDER_TO_PREMIUM)
core/appmodule/module.go (1)
36-53: References toHasServicesandRegisterServicesstill exist.The removal of
HasServicesandRegisterServicesis incomplete. The following files still contain references to these interfaces and methods:
x/upgrade/module.gox/staking/module.gox/slashing/module.gox/nft/module/module.gox/group/module/module.gox/params/module.gox/gov/module.gox/mint/module.gox/protocolpool/module.gox/feegrant/module/module.gox/epochs/module.gox/evidence/module.gox/bank/module.gox/circuit/module.gox/consensus/module.gox/distribution/module.gox/authz/module/module.gox/accounts/module.gox/auth/module.gotypes/module/mock_appmodule_test.gotypes/module/module_test.gotypes/module/module.gotypes/module/core_module.gotestutil/x/counter/module.gotestutil/mock/types_mock_appmodule.goruntime/v2/builder.goruntime/v2/services/autocli.goruntime/v2/module.goruntime/services/autocli.goruntime/module.goruntime/v2/manager.goruntime/builder.goruntime/app.gosimapp/app_test.gosimapp/app.gocore/appmodule/module.goPlease ensure that all references to
HasServicesandRegisterServicesare removed or updated accordingly.Analysis chain
LGTM! But verify the impact of removing
HasServices.The removal of
HasServicesandRegisterServicesindicates a change in service registration. Ensure that all references toHasServicesandRegisterServicesare removed or updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `HasServices` and `RegisterServices` are no longer referenced in the codebase. # Test: Search for any remaining references to `HasServices` and `RegisterServices`. Expect: No occurrences. rg --type go 'HasServices|RegisterServices'Length of output: 11909
Script:
#!/bin/bash # Description: Gather detailed context for references to `HasServices` and `RegisterServices`. # List all files containing references to `HasServices` and `RegisterServices` with context lines. rg --type go 'HasServices|RegisterServices' -A 5 -B 5Length of output: 47547
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (6)
collections/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumcore/testing/go.sumis excluded by!**/*.sumserver/v2/appmanager/go.sumis excluded by!**/*.sumserver/v2/stf/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sum
Files selected for processing (11)
- collections/go.mod (1 hunks)
- core/appmodule/module.go (2 hunks)
- core/gas/service.go (1 hunks)
- core/go.mod (1 hunks)
- runtime/v2/module.go (1 hunks)
- server/v2/stf/README.md (2 hunks)
- server/v2/stf/core_gas_service.go (2 hunks)
- server/v2/stf/gas/defaults.go (1 hunks)
- server/v2/stf/gas/store.go (3 hunks)
- server/v2/stf/gas/writer_map.go (2 hunks)
- store/v2/go.mod (1 hunks)
Files skipped from review due to trivial changes (4)
- collections/go.mod
- core/go.mod
- runtime/v2/module.go
- store/v2/go.mod
Additional context used
Path-based instructions (7)
server/v2/stf/core_gas_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/gas/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/gas/writer_map.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/gas/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"core/appmodule/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/gas/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
server/v2/stf/README.md
[uncategorized] ~5-~5: When ‘read-only’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...he state transition function receives a read only instance of state. It does not directly...(READ_ONLY_HYPHEN)
[style] ~34-~34: Consider a shorter alternative to avoid wordiness.
Context: ...terMap ) ``` THe wrappGasMeter is used in order to consume gas. Application developers can...(IN_ORDER_TO_PREMIUM)
[style] ~34-~34: Consider a shorter alternative to avoid wordiness.
Context: ...gas meter with their own implementation in order to customize consumption of gas.(IN_ORDER_TO_PREMIUM)
Additional comments not posted (9)
server/v2/stf/core_gas_service.go (1)
29-29: LGTM!The
GasConfigmethod now returns a default configuration from thestfgaspackage, which enhances functionality by providing a valid gas configuration.server/v2/stf/gas/defaults.go (2)
28-28: LGTM!The type of
DefaultConfighas been correctly modified fromStoreConfigtocoregas.GasConfig, enhancing the management of gas costs.
30-30: LGTM!The
DeleteCostFlatfield has been correctly renamed toDeleteCost, reflecting a simplification of the cost structure.server/v2/stf/gas/writer_map.go (2)
10-10: LGTM!The parameter type for
confin theNewMeteredWriterMapfunction has been correctly changed fromStoreConfigtogas.GasConfig, enhancing the handling of gas-related settings.
23-23: LGTM!The internal field
configwithin theMeteredWriterMapstruct has been correctly updated to change its type fromStoreConfigtogas.GasConfig, maintaining consistency in the type of configuration being utilized.server/v2/stf/gas/store.go (4)
23-23: LGTM! Standardization of gas configuration.The change from
StoreConfigtogas.GasConfigaligns with the goal of standardizing gas configuration management.
26-29: LGTM! Consistency in gas configuration management.The
NewStorefunction now acceptsgas.GasConfig, ensuring consistency in gas configuration management across the store functionalities.
73-73: LGTM! Refinement in deletion cost calculation.The
Deletemethod now usesDeleteCostinstead ofDeleteCostFlat, enhancing clarity and consistency in deletion cost calculation.
110-114: LGTM! Alignment in gas cost configuration.The
iteratorstruct andnewIteratorfunction now usegas.GasConfig, ensuring alignment in gas cost configuration and promoting better maintainability and coherence.
github-actions
bot
added
C:x/staking
C:x/gov
C:x/slashing
C:x/distribution
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (11)
x/protocolpool/module.go (1)
Removal of
HasServicesImplementation fromAppModule.The removal of
_ appmodule.HasServices = AppModule{}inx/protocolpool/module.goimpacts the implementation ofAppModule. This change affects other parts of the codebase whereAppModulewas expected to implement theHasServicesinterface.Affected Files:
types/module/core_module.gotypes/module/mock_appmodule_test.gotypes/module/module_test.goruntime/app.goruntime/services/autocli.goPlease review these files to ensure that the removal of the
HasServicesimplementation does not introduce any unintended side effects or break existing functionality.Analysis chain
Line range hint
14-14:
Verify the impact of removingHasServicesimplementation.The removal of
_ appmodule.HasServices = AppModule{}simplifies the module's interface. Ensure that this change does not affect other parts of the codebase that may rely on this interface.
Verify the impact of removing
HasServicesimplementation fromAppModule.The removal of
_ appmodule.HasServices = AppModule{}might affect other parts of the codebase that rely onAppModuleimplementingHasServices. We need to ensure that this change does not break any functionality or tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the removal of `HasServices` implementation affects other parts of the codebase. # Test: Search for the usage of `HasServices` interface. Expect: No occurrences of `HasServices` in the codebase. rg --type go 'HasServices'Length of output: 2372
Script:
#!/bin/bash # Description: Verify if the removal of `HasServices` implementation affects other parts of the codebase. # Search for the definition of `AppModule` and its interaction with `HasServices`. rg --type go 'AppModule' -A 10Length of output: 464768
x/group/module/module.go (1)
Removal of
appmodule.HasServicesinterface implementation is problematic.The
HasServicesinterface is still being referenced in several key files. Removing its implementation fromAppModulewithout addressing these references will likely cause issues. The following files contain references toHasServices:
runtime/services/autocli.goruntime/app.goruntime/module.gosimapp/app_test.gotypes/module/module.gotypes/module/core_module.goPlease ensure that these references are properly handled before removing the
HasServicesinterface implementation.Analysis chain
Line range hint
28-35:
Removal ofappmodule.HasServicesinterface implementation.The
appmodule.HasServicesinterface implementation has been removed. Ensure that this change does not break any functionality or dependencies relying on this interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` interface implementation does not break any dependencies. # Test: Search for the usage of `HasServices` in the codebase. Expect: No broken dependencies. rg --type go 'HasServices'Length of output: 2372
Script:
#!/bin/bash # Description: Verify the impact of removal of `appmodule.HasServices` interface implementation by checking the references in core files. # Check for references in core files. rg --type go 'HasServices' types/module/core_module.go types/module/module.go simapp/app_test.go runtime/module.go runtime/app.go runtime/services/autocli.goLength of output: 790
x/distribution/module.go (1)
Removal of
appmodule.HasServicesThe removal of
appmodule.HasServicesfromAppModuleis likely to impact the module's functionality. TheHasServicesinterface is used extensively across the codebase for registering services, and its absence could break critical functionality.
core/appmodule/module.goruntime/services/autocli.goruntime/app.goruntime/module.gosimapp/app_test.gotestutil/mock/types_module_module.gotypes/module/mock_appmodule_test.gotypes/module/module.gotypes/module/core_module.goPlease ensure that there is a proper replacement or fallback mechanism for
HasServicesto avoid breaking the module's functionality.Analysis chain
Line range hint
14-14:
Removal ofappmodule.HasServicesThe removal of
appmodule.HasServicesfromAppModulesuggests that the module no longer needs to handle services through this interface. Ensure that this change does not break any functionality relying on this interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the removal of `appmodule.HasServices` impacts the module functionality. # Test: Search for the usage of `HasServices` in the codebase. Expect: No critical dependencies. rg --type go 'HasServices'Length of output: 2372
Script:
#!/bin/bash # Extract context around the usage of `HasServices` in the identified files. # Search for `HasServices` in the identified files with context lines. rg --type go -A 5 -B 5 'HasServices' types/module/core_module.go types/module/module_test.go types/module/module.go types/module/mock_appmodule_test.go testutil/mock/types_module_module.go simapp/app_test.go runtime/app.go runtime/services/autocli.go runtime/module.go core/appmodule/module.goLength of output: 11179
x/gov/module.go (1)
Removal of
appmodule.HasServicesThe removal of
appmodule.HasServicesfromAppModuleaffects critical parts of the codebase that rely on this interface to register services. The following files contain dependencies onHasServices:
types/module/module.goruntime/app.goruntime/services/autocli.goPlease ensure that these dependencies are addressed to avoid breaking functionality.
Analysis chain
Line range hint
14-14:
Removal ofappmodule.HasServicesThe removal of
appmodule.HasServicesfromAppModulesuggests that the module no longer needs to handle services through this interface. Ensure that this change does not break any functionality relying on this interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the removal of `appmodule.HasServices` impacts the module functionality. # Test: Search for the usage of `HasServices` in the codebase. Expect: No critical dependencies. rg --type go 'HasServices'Length of output: 2372
Script:
#!/bin/bash # Extract and display the relevant code snippets for `HasServices` usage to understand its impact. # Extract from types/module/module.go rg --type go -A 10 'HasServices' types/module/module.go # Extract from runtime/app.go rg --type go -A 10 'HasServices' runtime/app.go # Extract from runtime/services/autocli.go rg --type go -A 10 'HasServices' runtime/services/autocli.goLength of output: 1334
core/CHANGELOG.md (3)
41-41: Ensure consistent use of tense in changelog entries.The changelog entries should consistently use the past tense for clarity and consistency.
- Comment out `appmodule.HasServices` to simplify dependencies. This interface is however still supported. + Commented out `appmodule.HasServices` to simplify dependencies. This interface is however still supported.
41-41: Clarify the support status ofappmodule.HasServices.The phrase "This interface is however still supported" is ambiguous. Specify how it is supported or in what context.
- Comment out `appmodule.HasServices` to simplify dependencies. This interface is however still supported. + Commented out `appmodule.HasServices` to simplify dependencies. This interface is still supported in legacy modules.
41-41: Fix grammatical error in changelog entry.The phrase "Add branch.ExecuteWithGasLimit" should be consistent with the other entries.
- Add branch.ExecuteWithGasLimit. + Added `branch.ExecuteWithGasLimit`.runtime/app.go (2)
81-81: Ensure the correct interface type inRegisterModulesmethod.The variable name
moduleshould be renamed to avoid shadowing and confusion.- } else if module, ok := appModule.(hasServicesV1); ok { - if err := module.RegisterServices(a.configurator); err != nil { + } else if modV1, ok := appModule.(hasServicesV1); ok { + if err := modV1.RegisterServices(a.configurator); err != nil {
276-280: Clarify the purpose of thehasServicesV1interface.The comment should explain why the interface is defined and how it is intended to be used.
- // hasServicesV1 is the interface for registering service in baseapp Cosmos SDK. - // This API is part of core/appmodule but commented out for dependencies. + // hasServicesV1 is the interface for registering services in baseapp Cosmos SDK. + // This API is part of core/appmodule but is commented out in the main codebase to simplify dependencies.simapp/app_test.go (1)
111-113: Improve readability of inline interface definition.The inline interface definition can be moved to a separate variable to improve readability.
- if mod, ok := mod.(interface { - RegisterServices(grpc.ServiceRegistrar) error - }); ok { + type serviceRegistrar interface { + RegisterServices(grpc.ServiceRegistrar) error + } + if mod, ok := mod.(serviceRegistrar); ok {types/module/module.go (1)
Inconsistency in Interface Usage:
The
RegisterServicesmethod is not consistently using thehasServicesV1interface across all modules. Please ensure that all modules are updated to implement thehasServicesV1interface for consistent behavior.
- Files to update:
x/upgrade/module.gox/staking/module.gox/slashing/module.gox/protocolpool/module.gox/mint/module.gox/nft/module/module.gox/params/module.gox/gov/module.gox/group/module/module.gox/authz/module/module.gox/bank/module.gox/distribution/module.gox/feegrant/module/module.gox/evidence/module.gox/circuit/module.gox/epochs/module.gox/consensus/module.gox/auth/module.gox/accounts/module.gotestutil/x/counter/module.goruntime/v2/module.goAnalysis chain
Line range hint
395-405: LGTM! But verify the method usage in the codebase.The changes to the
RegisterServicesmethod are approved.However, ensure that all modules implementing
RegisterServicesare updated to use the newhasServicesV1interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `RegisterServices` use the new `hasServicesV1` interface. # Test: Search for the `RegisterServices` method. Expect: Only occurrences of the new interface. rg --type go -A 5 $'RegisterServices'Length of output: 21286
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (25)
- core/CHANGELOG.md (1 hunks)
- runtime/app.go (3 hunks)
- runtime/services/autocli.go (1 hunks)
- runtime/v2/manager.go (3 hunks)
- runtime/v2/module.go (3 hunks)
- runtime/v2/services/autocli.go (1 hunks)
- simapp/app_test.go (2 hunks)
- types/module/core_module.go (1 hunks)
- types/module/module.go (3 hunks)
- types/module/module_test.go (1 hunks)
- x/accounts/module.go (1 hunks)
- x/auth/module.go (1 hunks)
- x/authz/module/module.go (1 hunks)
- x/bank/module.go (1 hunks)
- x/circuit/module.go (1 hunks)
- x/distribution/module.go (1 hunks)
- x/feegrant/module/module.go (1 hunks)
- x/gov/module.go (1 hunks)
- x/group/module/module.go (1 hunks)
- x/mint/module.go (1 hunks)
- x/params/module.go (1 hunks)
- x/protocolpool/module.go (1 hunks)
- x/slashing/module.go (1 hunks)
- x/staking/module.go (1 hunks)
- x/upgrade/module.go (1 hunks)
Files skipped from review due to trivial changes (1)
- runtime/v2/module.go
Additional context used
Path-based instructions (24)
x/accounts/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/params/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/circuit/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/services/autocli.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/services/autocli.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/feegrant/module/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/module/core_module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/module/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/group/module/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/staking/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/slashing/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/gov/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"runtime/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_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"runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/module/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"types/module/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (22)
x/accounts/module.go (1)
Line range hint
14-14:
Removal ofappmodule.HasServicesinterface assertion.The assertion
var _ appmodule.HasServices = AppModule{}has been removed. This indicates thatAppModuleno longer provides services as defined by theHasServicesinterface. Ensure that this change aligns with the overall design and does not impact other components relying on this interface.x/params/module.go (1)
Line range hint
14-14:
Removal ofappmodule.HasServicesinterface assertion.The assertion
var _ appmodule.HasServices = AppModule{}has been removed. This indicates thatAppModuleno longer provides services as defined by theHasServicesinterface. Ensure that this change aligns with the overall design and does not impact other components relying on this interface.x/circuit/module.go (1)
Line range hint
14-14:
Removal ofappmodule.HasServicesinterface implementation.The implementation of the
appmodule.HasServicesinterface has been removed fromAppModule. This indicates thatAppModuleno longer provides services as defined by theHasServicesinterface. Ensure that this change aligns with the overall design and does not impact other components relying on this interface.runtime/services/autocli.go (1)
57-59: Improved type safety with explicit method signature.The modification to the type assertion ensures that only types with the specified
RegisterServices(grpc.ServiceRegistrar) errormethod signature can be treated as valid modules for service registration. This enhances type safety and prevents runtime errors related to incompatible types.runtime/v2/services/autocli.go (1)
55-57: Enhanced compatibility and flexibility in service registration.The modification to the
ExtractAutoCLIOptionsfunction broadens the interface check to accommodate modules that implement a custom interface with aRegisterServices(grpc.ServiceRegistrar) errormethod. This improves compatibility and flexibility in handling different module types.x/upgrade/module.go (1)
Line range hint
14-18:
Verify the impact of removingappmodule.HasServicesimplementation.The removal of the
appmodule.HasServicesinterface implementation suggests a shift in design. Ensure that the services previously registered through this interface are either handled differently or are no longer necessary.Verification successful
Verify the impact of removing
appmodule.HasServicesimplementation.The
RegisterServicesmethod is still implemented inx/upgrade/module.goand continues to register the necessary services. This indicates that the removal of theappmodule.HasServicesinterface does not affect the functionality, as the services are still being registered correctly.
x/upgrade/module.go:81-86Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `appmodule.HasServices` implementation. # Test: Search for usages of `appmodule.HasServices` to ensure no breaking changes. rg --type go 'appmodule.HasServices'Length of output: 117
Script:
#!/bin/bash # Description: Verify the implementation and usage of `RegisterServices` method. # Search for the implementation of `RegisterServices` method. ast-grep --lang go --pattern 'func RegisterServices($_, $_) { $$$ }' # Search for the usage of `RegisterServices` method. rg --type go 'RegisterServices'Length of output: 8719
Script:
#!/bin/bash # Description: Verify the implementation of `RegisterServices` in `x/upgrade/module.go`. # Extract the implementation of `RegisterServices` from `x/upgrade/module.go`. ast-grep --lang go --pattern 'func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error { $$$ }' -- x/upgrade/module.goLength of output: 491
x/feegrant/module/module.go (1)
Line range hint
21-25:
Verify the impact of removingappmodule.HasServicesimplementation.The removal of the
appmodule.HasServicesinterface implementation suggests a shift in design. Ensure that the services previously registered through this interface are either handled differently or are no longer necessary.Verification successful
Verified the impact of changing the type assertion to
hasServicesV1.The
hasServicesV1interface maintains the core functionality of registering services, similar to the commented-outHasServicesinterface. Therefore, the change in type assertion is compatible with the expected behavior.
hasServicesV1includesRegisterServices(grpc.ServiceRegistrar) error.- The commented-out
HasServicesalso includesRegisterServicesand extendsAppModule.The modification does not negatively impact the functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing the type assertion to `hasServicesV1`. # Test: Search for the definition and usage of `hasServicesV1` to ensure compatibility. rg --type go 'hasServicesV1'Length of output: 979
Script:
#!/bin/bash # Description: Compare the definitions of `hasServicesV1` and `appmodule.HasServices`. # Search for the definition of `appmodule.HasServices`. rg --type go 'appmodule.HasServices'Length of output: 117
Script:
#!/bin/bash # Description: Search for the definition of `HasServices` within the `appmodule` package to compare it with `hasServicesV1`. # Search for the definition of `HasServices` in the `appmodule` package. rg --type go 'HasServices' | rg 'appmodule'Length of output: 400
Script:
#!/bin/bash # Description: Locate the complete definition of `HasServices` in `core/appmodule/module.go` to compare it with `hasServicesV1`. # Search for the definition of `HasServices` in `core/appmodule/module.go`. rg --type go 'HasServices' core/appmodule/module.goLength of output: 167
Script:
#!/bin/bash # Description: Extract the definition of `hasServicesV1` and compare it with the commented-out `HasServices` interface. # Extract the definition of `hasServicesV1`. rg --type go 'type hasServicesV1 interface' -A 5 # Extract the commented-out definition of `HasServices`. rg --type go 'type HasServices interface' -A 5 core/appmodule/module.goLength of output: 793
x/authz/module/module.go (1)
Line range hint
21-28:
Removal ofappmodule.HasServicesinterface implementation.The
appmodule.HasServicesinterface implementation has been removed. Ensure that this change does not break any functionality or dependencies relying on this interface.x/staking/module.go (1)
Line range hint
26-34:
Removal ofappmodule.HasServicesinterface implementation.The
appmodule.HasServicesinterface implementation has been removed. Ensure that this change does not break any functionality or dependencies relying on this interface.x/bank/module.go (1)
Line range hint
14-14:
Removedappmodule.HasServicesinterface assertion.The removal of the
appmodule.HasServicesinterface assertion indicates that theAppModuleno longer provides services defined by this interface. Ensure that this change does not affect the functionality of the module or its integration with other components that rely on this interface.Verification successful
Verification successful: The removal of the
appmodule.HasServicesinterface assertion does not affect the functionality of the module. No critical dependencies on this interface were found in the core module files.
- The only references to
appmodule.HasServiceswere found in test files, which do not impact the core functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` does not affect the functionality of the module. # Test: Search for the usage of `appmodule.HasServices` in the codebase. Expect: No critical dependencies on this interface. rg --type go -A 5 $'appmodule.HasServices'Length of output: 549
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` does not affect the functionality of the module. # Test: Search for the usage of `appmodule.HasServices` in the core module files, excluding test files. rg --type go --glob '!*_test.go' -A 5 $'appmodule.HasServices'Length of output: 61
x/mint/module.go (1)
Line range hint
14-14:
Removedappmodule.HasServicesinterface assertion.The removal of the
appmodule.HasServicesinterface assertion indicates that theAppModuleno longer provides services defined by this interface. Ensure that this change does not affect the functionality of the module or its integration with other components that rely on this interface.Verification successful
Verification successful: The removal of the
appmodule.HasServicesinterface assertion does not affect the functionality of the module. The only references to this interface were found in test files, indicating no critical dependencies in the main codebase.
- No further action is required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` does not affect the functionality of the module. # Test: Search for the usage of `appmodule.HasServices` in the codebase. Expect: No critical dependencies on this interface. rg --type go -A 5 $'appmodule.HasServices'Length of output: 549
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` does not affect the functionality of the module. # Test: Search for the usage of `appmodule.HasServices` in the main codebase, excluding test files. Expect: No critical dependencies on this interface. rg --type go --glob '!*_test.go' -A 5 $'appmodule.HasServices'Length of output: 61
x/slashing/module.go (1)
Line range hint
14-14:
Removedappmodule.HasServicesinterface assertion.The removal of the
appmodule.HasServicesinterface assertion indicates that theAppModuleno longer provides services defined by this interface. Ensure that this change does not affect the functionality of the module or its integration with other components that rely on this interface.Verification successful
Removal of
appmodule.HasServicesinterface assertion verified.The
appmodule.HasServicesinterface is only referenced in a test file (types/module/module_test.go) and is not used in the core functionality of the module. Therefore, its removal does not impact the functionality or integration of the module.
types/module/module_test.go: Reference found in test implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` does not affect the functionality of the module. # Test: Search for the usage of `appmodule.HasServices` in the codebase. Expect: No critical dependencies on this interface. rg --type go -A 5 $'appmodule.HasServices'Length of output: 549
Script:
#!/bin/bash # Description: Verify that the removal of `appmodule.HasServices` does not affect the functionality of the module. # Test: Search for any additional references to `appmodule.HasServices` in the codebase. Expect: No critical dependencies on this interface. rg --type go $'appmodule.HasServices'Length of output: 117
x/distribution/module.go (1)
Line range hint
1-1:
File OverviewThis file implements the
AppModulefor thex/distributionmodule. It includes various functionalities such as registering services, migrations, and genesis state management.x/auth/module.go (2)
Line range hint
1-1:
File OverviewThis file implements the
AppModulefor thex/authmodule. It includes various functionalities such as registering services, migrations, and genesis state management.
Line range hint
14-14:
Removal ofappmodule.HasServicesThe removal of
appmodule.HasServicesfromAppModulesuggests that the module no longer needs to handle services through this interface. Ensure that this change does not break any functionality relying on this interface.x/gov/module.go (1)
Line range hint
1-1:
File OverviewThis file implements the
AppModulefor thex/govmodule. It includes various functionalities such as registering services, migrations, and genesis state management.runtime/v2/manager.go (3)
714-718: Introduction ofhasServicesV1interface looks good.The new interface
hasServicesV1encapsulates the service registration logic effectively.
Line range hint
428-438: LGTM! But verify the method usage in the codebase.The changes to the
RegisterServicesmethod are approved.However, ensure that all modules implementing
RegisterServicesare updated to use the newhasServicesV1interface.
Line range hint
557-568: LGTM! But verify the function usage in the codebase.The changes to the
registerServicesfunction are approved.However, ensure that all calls to
registerServicesare updated to use the newhasServicesV1interface.Verification successful
Function usage verified!
The
servicesvariable is correctly checked for thehasServicesV1interface before being passed to theregisterServicesfunction.
runtime/v2/manager.go:
servicesis verified to implementhasServicesV1before being used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `registerServices` use the new `hasServicesV1` interface. # Test: Search for the `registerServices` function calls. Expect: Only occurrences of the new interface. rg --type go -A 5 $'registerServices'Length of output: 748
Script:
#!/bin/bash # Description: Verify that the `services` variable implements the `hasServicesV1` interface. # Find the definition of `services` in the runtime/v2/manager.go file. rg --type go --context 10 'services' runtime/v2/manager.goLength of output: 1176
types/module/module.go (2)
96-102: Introduction ofhasServicesV1interface looks good.The new interface
hasServicesV1encapsulates the service registration logic effectively.
33-33: Introduction ofhasServicesV1interface looks good.The new interface
hasServicesV1encapsulates the service registration logic effectively.
|
I have a small question, we are setting gas config in app_config.go, but it seems like |
Read too fast earlier, but those aren't the same. There are the gas limits from the app config and the gas costs from (stf) gas service. One is app wide, the other one can be node wide. |
|
I have to say that the runtime gas config is a bit weird and its config (app.toml) is still not super well implemented. I will do that in a follow-up. |
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)
- runtime/v2/module.go (3 hunks)
- simapp/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- runtime/v2/module.go
Additional comments not posted (2)
simapp/go.mod (2)
51-51: Direct dependency addition looks good.The addition of
google.golang.org/grpc v1.64.1as a direct dependency is correctly formatted and placed.
52-52: Indirect dependency removal looks good.The removal of
google.golang.org/grpc v1.64.1as an indirect dependency is correctly reflected.
Understood. Bit confused with same name. |
Agreed, I'll be renaming the one from runtime. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
store/v2/go.sumis excluded by!**/*.sum
Files selected for processing (2)
- simapp/go.mod (2 hunks)
- store/v2/go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- store/v2/go.mod
Files skipped from review as they are similar to previous changes (1)
- simapp/go.mod
|
Partially backporting this in v0.52 for module changes |
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
(cherry picked from commit 7040177) # Conflicts: # collections/go.mod # collections/go.sum # core/CHANGELOG.md # core/appmodule/module.go # core/gas/service.go # core/go.mod # core/go.sum # core/testing/go.sum # runtime/v2/manager.go # runtime/v2/module.go # runtime/v2/services/autocli.go # server/v2/appmanager/go.sum # server/v2/stf/README.md # server/v2/stf/core_gas_service.go # server/v2/stf/gas/defaults.go # server/v2/stf/gas/store.go # server/v2/stf/gas/writer_map.go # server/v2/stf/go.sum # store/v2/go.mod # store/v2/go.sum
Co-authored-by: Julien Robert <[email protected]>
Description
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation