refactor(bank): migrate to use env var #19477
Conversation
github-actions
bot
added
C:x/distribution
WalkthroughWalkthroughThe changes involve updating the initialization process and component interaction within the Changes
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.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (19)
- simapp/app.go (1 hunks)
- tests/integration/bank/keeper/deterministic_test.go (1 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (1 hunks)
- tests/integration/evidence/keeper/infraction_test.go (1 hunks)
- tests/integration/gov/keeper/keeper_test.go (1 hunks)
- tests/integration/slashing/keeper/keeper_test.go (1 hunks)
- tests/integration/staking/keeper/common_test.go (1 hunks)
- tests/integration/staking/keeper/deterministic_test.go (1 hunks)
- x/auth/vesting/fuzz_test.go (1 hunks)
- x/bank/CHANGELOG.md (1 hunks)
- x/bank/depinject.go (3 hunks)
- x/bank/keeper/collections_test.go (3 hunks)
- x/bank/keeper/grpc_query.go (1 hunks)
- x/bank/keeper/keeper.go (9 hunks)
- x/bank/keeper/keeper_test.go (3 hunks)
- x/bank/keeper/send.go (9 hunks)
- x/bank/keeper/view.go (4 hunks)
- x/bank/types/events.go (1 hunks)
- x/distribution/migrations/v4/migrate_funds_test.go (1 hunks)
Additional comments: 42
x/bank/types/events.go (1)
- 22-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-21]
The constants for event types and attribute keys are correctly defined and follow the Cosmos SDK's conventions. It's noted that functions for constructing coin events have been removed as part of the refactor.
x/bank/CHANGELOG.md (1)
- 43-43: The changelog entry clearly communicates the API breaking change related to the introduction of
appmodule.Environmentin theNewKeeperfunction. It adheres to the changelog's guiding principles and provides essential information for developers.x/bank/depinject.go (2)
- 30-33: The modifications to
ModuleInputsto useappmodule.Environmentinstead of specific services likeLoggerandStoreServicealign with the PR's objectives to enhance modularity and flexibility. This change should facilitate easier configuration and initialization of thebankmodule.- 81-81: The adjustment in the
ProvideModulefunction to passEnvironmentinstead ofStoreServiceis correctly implemented. This change supports the broader refactor towards using environment variables for configuration and enhances the module's adaptability.x/bank/keeper/collections_test.go (4)
- 33-33: The modification to use a new
Environmentinstance in theTestBankStateCompatibilitytest function aligns with the broader refactor towards utilizing environment variables for configuration. This change ensures that the test accurately reflects the module's operation under the new configuration approach.- 41-41: The adjustment in the
BaseKeeperinitialization to include theEnvironmentinstance is correctly implemented. This change is consistent with the modifications in the main codebase and supports the objective of enhancing the module's flexibility and configurability.- 59-59: The update to access the key-value store service using the
Environmentinstance in the test function is correctly implemented. This ensures that the test remains relevant and accurately reflects the module's operation under the new configuration approach.- 82-82: The verification of the new raw value in the key-value store, using the
Environmentinstance, is correctly implemented. This part of the test checks the updated behavior under the new configuration approach, ensuring that the module functions as expected.x/auth/vesting/fuzz_test.go (1)
- 110-110: The modification in the
FuzzMsgServerCreateVestingAccountfunction to useruntime.NewEnvironmentfor initializingbankKeeperaligns with the broader refactor towards utilizing environment variables for configuration. This change ensures that the fuzz test accurately simulates the module's operation under the new configuration approach.x/distribution/migrations/v4/migrate_funds_test.go (1)
- 60-60: The modification in the
TestFundsMigrationfunction to useruntime.NewEnvironmentfor initializingbankKeeperaligns with the broader refactor towards utilizing environment variables for configuration. This change ensures that the test accurately reflects the module's operation under the new configuration approach.tests/integration/gov/keeper/keeper_test.go (1)
- 87-87: The modification in the
initFixturefunction to useruntime.NewEnvironmentfor initializingbankKeeperaligns with the broader refactor towards utilizing environment variables for configuration. This change ensures that the integration test accurately simulates the module's operation under the new configuration approach.tests/integration/staking/keeper/common_test.go (1)
- 137-137: The modification to initialize
bankKeeperusingruntime.NewEnvironmentwithlog.NewNopLogger()is a significant shift towards leveraging environment variables for configuration. This change aligns with the PR's objectives and should enhance the flexibility and configurability of the tests. Ensure that all dependent tests are updated accordingly to accommodate this new setup.x/bank/keeper/view.go (4)
- 9-9: Replacing the import of
"cosmossdk.io/core/store"with"cosmossdk.io/core/appmodule"aligns with the PR's objective to utilize environment variables for configuration. This change is crucial for enabling theBaseViewKeeperto leverage the new environment setup.- 59-62: The modification of
BaseViewKeeperto useenvironmentinstead ofstoreServiceis a significant change that aligns with the PR's objectives. This adjustment enhances the module's configurability and adaptability to different runtime environments.- 73-74: Updating the
NewBaseViewKeeperfunction signature to includeappmodule.Environmentis a necessary change to accommodate the new environment-driven configuration approach. This change ensures that theBaseViewKeeperis initialized with the correct environment context.- 189-189: Utilizing
k.environment.HeaderService.GetHeaderInfo(ctx).Timein theLockedCoinsfunction instead of the previous method is a direct consequence of the migration towards using environment variables. This change should be carefully tested to ensure it does not affect the functionality negatively.x/bank/keeper/grpc_query.go (1)
- 175-175: Updating the initialization of
kvStorein theDenomsMetadatafunction to usek.environment.KVStoreServiceis a key change that aligns with the PR's objectives. This modification is crucial for ensuring that the gRPC query functionality leverages the new environment setup. Ensure that this change is thoroughly tested to confirm that it does not introduce any regressions in the gRPC query functionality.tests/integration/bank/keeper/deterministic_test.go (2)
- 94-94: The initialization of
bankKeepernow usesruntime.NewEnvironmentwithruntime.NewKVStoreServiceandlog.NewNopLogger(). This change aligns with the PR's objective to migrate towards using environment variables for configuration. However, it's crucial to ensure that this new setup does not introduce any unintended side effects, especially in terms of performance and security. The use oflog.NewNopLogger()in tests is appropriate as it prevents logging output during test execution, which can clutter test results and slow down the testing process. However, ensure that the logger configuration is appropriately adjusted for production environments to capture necessary logs for monitoring and debugging purposes.- 90-98: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2516]
Overall, the integration tests in
deterministic_test.goappear to be well-structured and comprehensive, covering a wide range of scenarios for the bank module. The migration to usingruntime.NewEnvironmentforbankKeeperinitialization is a significant change that aligns with the PR's objectives. It's essential to ensure that these changes are thoroughly tested, especially considering the shift towards environment-variable-driven configuration. The use of property-based testing withrapidis commendable, as it helps uncover edge cases and ensures robustness. However, it's crucial to keep an eye on the performance implications of the new setup and ensure that logging is appropriately handled in different environments.x/bank/keeper/send.go (4)
- 59-62: The introduction of
appmodule.Environmentin theBaseSendKeeperstruct is a significant change that aligns with the PR's objective to utilize environment variables for configuration. This change enhances modularity and configurability, allowing for more flexible runtime environments. Ensure that all instances whereBaseSendKeeperis instantiated have been updated to pass theappmodule.Environmentparameter.- 84-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-90]
The
NewBaseSendKeeperfunction now requires anappmodule.Environmentparameter, which is a good practice for dependency injection, enhancing the testability and modularity of the code. However, ensure that all calls toNewBaseSendKeeperthroughout the codebase have been updated to include the newenvparameter. This is crucial for maintaining compatibility and preventing runtime errors.Verification successful
Given the current verification, it appears that the specific instance of
NewBaseSendKeeperfound in the codebase has been correctly updated to include the newenvparameter. However, to ensure comprehensive verification, it would be prudent to search for any calls toNewBaseSendKeeperwithout specifying the exact number of parameters, to catch all instances regardless of how many parameters they include. This step is crucial for confirming that all necessary updates have been made throughout the codebase.
The verification process has confirmed that the call to
NewBaseSendKeeperin the codebase has been correctly updated to include the newenvparameter. This addresses the concern raised in the review comment about ensuring compatibility and preventing runtime errors by updating all calls toNewBaseSendKeeperto include the new parameter. No additional instances requiring updates were found.* 176-182: The modifications to event emission logic, utilizing `k.environment.EventService.EventManager(ctx).EmitKV`, represent a streamlined approach to handling events within the bank module. This change not only simplifies the event emission process but also aligns with the broader refactor towards using environment variables and services. It's important to ensure that all event-related logic across the bank module has been updated to use this new pattern for consistency and maintainability.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances of NewBaseSendKeeper to ensure they now include the env parameter. ast-grep --lang go --pattern $'NewBaseSendKeeper($_, $_, $_, $_, $_, $_)'Length of output: 196
Script:
#!/bin/bash # Search for instances of NewBaseSendKeeper to ensure they include the env parameter, without specifying the exact number of parameters. ast-grep --lang go --pattern $'NewBaseSendKeeper($$$)'Length of output: 177
Also applies to: 216-220, 267-271, 296-299
- 176-182: The use of
EmitKVfor event emission is correctly implemented. However, ensure that the attributes used in events (types.EventTypeTransfer,types.AttributeKeyRecipient, etc.) are consistently defined and used across the module. This consistency is crucial for downstream services that may rely on these event types and attributes for processing.tests/integration/evidence/keeper/infraction_test.go (1)
- 120-120: The initialization of
bankKeeperhas been updated to useruntime.NewEnvironmentwithlog.NewNopLogger(), replacing the direct creation ofruntime.NewKVStoreService(keys[banktypes.StoreKey]). This change aligns with the PR's objective to migrate towards using environment variables for configuration. It's crucial to ensure that all dependent tests and functionalities are updated to accommodate this new initialization method. Additionally, verify that theruntime.NewEnvironmentmethod properly configures the environment as expected for the integration tests.x/bank/keeper/keeper.go (7)
- 7-8: The addition of
appmoduleandeventpackages is in line with the PR's objective to migrate the bank module to utilize environment variables and enhance event management. Ensure that all references to these packages are correctly updated throughout the module.- 62-62: The introduction of
appmodule.Environmentin theBaseKeeperstruct is a significant change that aligns with the PR's goal of enhancing modularity and configurability. It's crucial to verify that all instances ofBaseKeeperare correctly updated to handle this new dependency.- 86-86: The modification to
NewBaseKeeperto accept anappmodule.Environmentparameter is a key part of the migration towards using environment variables. This change requires careful attention to ensure that all calls toNewBaseKeeperacross the Cosmos SDK are updated accordingly.- 101-104: The initialization of
BaseSendKeeperwithinNewBaseKeepernow correctly passes theappmodule.Environmentparameter. This change is crucial for maintaining consistency in how environment variables are used across the bank module. Ensure that theNewBaseSendKeeperfunction and its usage are updated to reflect this new parameter.Verification successful
The verification process confirms that the
NewBaseSendKeeperfunction withinNewBaseKeeperhas been updated to include theappmodule.Environmentparameter, aligning with the review comment's requirement for consistency in handling environment variables across the bank module.* 160-166: The update to event emission logic to use the environment's event service is a positive change that aligns with the PR's objectives. It's important to ensure that all event emissions within the bank module are updated to use this new approach for consistency and maintainability. * 383-388: The emission of the mint event using the environment's event service is correctly implemented. This change enhances the modularity and configurability of the event management system within the bank module. Ensure that similar updates are made across all event emissions in the module. * 424-427: The update to the burn event emission logic to utilize the environment's event service is consistent with the PR's goal of improving event management. This change should be mirrored across all event-related logic within the bank module to ensure uniformity and ease of maintenance.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances of NewBaseSendKeeper initialization to ensure they now include the environment parameter. ast-grep --lang go --pattern $'NewBaseSendKeeper($_, $_, $_, $_, $_, $_)'Length of output: 196
tests/integration/slashing/keeper/keeper_test.go (1)
- 88-88: The initialization of
bankKeepernow usesruntime.NewEnvironmentwithlog.NewNopLogger(), replacing the direct call toruntime.NewKVStoreService. This change aligns with the PR's objective to migrate towards using environment variables for configuration. However, ensure that the new environment setup is correctly configured and thatlog.NewNopLogger()is appropriate for the test context. Using a no-op logger in tests is generally acceptable as it avoids cluttering test output, but consider if there's any scenario where logging could be beneficial for debugging.simapp/app.go (1)
- 311-311: The modification to initialize
BankKeeperusingruntime.NewEnvironmentand a logger is a significant shift towards a more dynamic and configurable system. This change aligns with the PR's objective to utilize environment variables for configuration. However, it's crucial to ensure that the new environment setup does not introduce any performance overhead or complexity that could affect the maintainability of the code. Additionally, verify that the logger is appropriately configured to handle different log levels and formats to aid in debugging and monitoring.tests/integration/staking/keeper/deterministic_test.go (2)
- 101-101: The change to initialize
bankKeeperusingruntime.NewEnvironmentinstead ofruntime.NewKVStoreServicealigns with the PR's objective to migrate towards using environment variables for configuration. This approach enhances modularity and adaptability to different runtime environments. Ensure that the rest of the test suite and the main application code are updated accordingly to maintain consistency and avoid potential runtime errors due to the change in initialization parameters.- 98-105: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1027]
Given the significant refactor to use environment variables for the
bankKeeperinitialization, it's crucial to verify that all necessary environment variables are correctly set up in the test environment. This includes ensuring that any new environment variables introduced by the refactor are documented and included in the test setup procedures. Additionally, consider adding tests or assertions to verify that thebankKeeperbehaves as expected when initialized with different environment configurations, enhancing the robustness of the integration tests.tests/integration/distribution/keeper/msg_server_test.go (1)
- 100-100: The initialization of
bankKeepernow usesruntime.NewEnvironmentwithlog.NewNopLogger(), which is a significant change from the previous direct use ofruntime.NewKVStoreService. This change aligns with the PR's objective to migrate towards using environment variables for configuration. However, it's crucial to ensure that this new setup does not introduce any unintended side effects, especially in terms of logging and state management. The use oflog.NewNopLogger()means that logs generated by thebankKeeperduring tests will be discarded, which is acceptable for tests but should be carefully considered in a production environment.x/bank/keeper/keeper_test.go (6)
- 131-137: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]
The use of
runtime.NewEnvironmentwithlog.NewNopLogger()in theSetupTestfunction is appropriate for unit testing, as it isolates the test environment from external dependencies and avoids unnecessary logging.
- 303-306: The creation of a new
runtime.Environmentwithin theTestGetAuthorityfunction ensures that each test case runs in a clean environment, which is a good practice for unit testing to avoid side effects between tests.- 131-137: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]
The tests make good use of mocking and isolation techniques, ensuring that each unit test runs in a controlled environment. This approach is crucial for accurate and reliable testing, especially in a complex system like the Cosmos SDK.
Also applies to: 303-306
- 131-137: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]
The tests are well-structured and follow a clear naming convention, making it easy to understand what each test aims to verify. This consistency is beneficial for maintainability and readability.
Also applies to: 303-306
- 131-137: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]
The use of
gomockfor mocking dependencies andtestify/suitefor structuring tests as suites are both industry-standard practices in Go testing. These tools provide powerful features for mocking and organizing tests, contributing to the overall quality of the test suite.Also applies to: 303-306
- 131-137: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]
It's important to ensure that all external dependencies, such as the
runtime.Environmentand mocked interfaces, are correctly cleaned up or reset after each test to prevent any potential interference with other tests. This cleanup process seems to be handled well in the provided tests.Also applies to: 303-306
| ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()}) | ||
| encCfg := moduletestutil.MakeTestEncodingConfig() | ||
|
|
||
| storeService := runtime.NewKVStoreService(key) | ||
| env := runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()) | ||
|
|
||
| // gomock initializations | ||
| ctrl := gomock.NewController(suite.T()) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]
The repeated creation of runtime.Environment instances in both SetupTest and TestGetAuthority functions is consistent and necessary for isolating test environments. However, consider if there's a way to reduce code duplication, perhaps by creating a helper function for initializing the environment that can be reused across tests.
// Suggestion to reduce duplication by creating a helper function.
func newTestEnvironment(key storetypes.StoreKey) runtime.Environment {
return runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger())
}Then replace the direct calls with newTestEnvironment(key) where appropriate.
Also applies to: 303-306
x/bank/keeper/send.go
Outdated
| } | ||
|
|
||
| return BaseSendKeeper{ | ||
| BaseViewKeeper: NewBaseViewKeeper(cdc, storeService, ak, logger), | ||
| BaseViewKeeper: NewBaseViewKeeper(env, cdc, ak, logger), | ||
| cdc: cdc, | ||
| ak: ak, | ||
| storeService: storeService, | ||
| environment: env, | ||
| blockedAddrs: blockedAddrs, | ||
| authority: authority, | ||
| logger: logger, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-90]
In the NewBaseSendKeeper function, there's a panic condition for an invalid bank authority address. While this is a valid approach to handle critical configuration errors, consider providing detailed documentation or error messages to help developers understand the cause of the panic. This can improve the developer experience by making it easier to debug and resolve configuration issues.
if _, err := ak.AddressCodec().StringToBytes(authority); err != nil {
- panic(fmt.Errorf("invalid bank authority address: %w", err))
+ panic(fmt.Errorf("invalid bank authority address '%s': %w", authority, err))
}
x/bank/keeper/keeper.go
Outdated
| @@ -446,8 +448,7 @@ func (k BaseKeeper) trackDelegation(ctx context.Context, addr sdk.AccAddress, ba | |||
| vacc, ok := acc.(types.VestingAccount) | |||
| if ok { | |||
| // TODO: return error on account.TrackDelegation | |||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |||
| vacc.TrackDelegation(sdkCtx.HeaderInfo().Time, balance, amt) | |||
| vacc.TrackDelegation(k.environment.HeaderService.GetHeaderInfo(ctx).Time, balance, amt) | |||
There was a problem hiding this comment.
The comment regarding the potential return of an error from TrackDelegation suggests a future improvement. It's important to track this as a TODO or open an issue to ensure it's addressed, as error handling is crucial for robustness and reliability.
Would you like me to open a GitHub issue to track the enhancement of error handling in TrackDelegation and TrackUndelegation?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (10)
- simapp/app.go (1 hunks)
- tests/integration/bank/keeper/deterministic_test.go (1 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (1 hunks)
- tests/integration/evidence/keeper/infraction_test.go (1 hunks)
- tests/integration/gov/keeper/keeper_test.go (1 hunks)
- tests/integration/slashing/keeper/keeper_test.go (1 hunks)
- tests/integration/staking/keeper/common_test.go (1 hunks)
- tests/integration/staking/keeper/deterministic_test.go (1 hunks)
- x/auth/vesting/fuzz_test.go (1 hunks)
- x/distribution/migrations/v4/migrate_funds_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- simapp/app.go
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/distribution/keeper/msg_server_test.go
- tests/integration/evidence/keeper/infraction_test.go
- tests/integration/gov/keeper/keeper_test.go
- tests/integration/slashing/keeper/keeper_test.go
- tests/integration/staking/keeper/common_test.go
- tests/integration/staking/keeper/deterministic_test.go
- x/auth/vesting/fuzz_test.go
- x/distribution/migrations/v4/migrate_funds_test.go
Description
ref #19290
migrate bank module to use env variable
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.
I have...
Summary by CodeRabbit
Refactor
BankKeeperfor improved environment handling and logging capabilities.BankKeeperinitialization method, optimizing component interactions and event logic.bankKeepersetup in theFuzzMsgServerCreateVestingAccountfunction for better environment and logging integration.Tests
BankKeeperinitialization and environment setup.Chores