-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(x/accounts): add env bundler to accounts module #19418
Conversation
WalkthroughWalkthroughThe recent updates consolidate the initialization and management of various services within the application, particularly focusing on 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 (
|
@likhita-809 your pull request is missing a changelog! |
Not necessary. Since unreleased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- simapp/app.go (1 hunks)
- x/accounts/go.mod (4 hunks)
- x/accounts/go.sum (9 hunks)
- x/accounts/keeper.go (3 hunks)
- x/accounts/utils_test.go (2 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 6
x/accounts/utils_test.go (1)
- 52-53: LGTM. Ensure documentation is updated to reflect the new way of initializing
Keeper
in tests.x/accounts/go.mod (1)
- 10-10: LGTM. Verify compatibility of new dependencies with the rest of the project.
x/accounts/keeper.go (3)
- 73-73: LGTM. Ensure thorough testing of the refactored
NewKeeper
function with the environment parameter.- 82-83: LGTM. Verify that all services accessed through the environment are correctly initialized.
- 105-105: LGTM. Test
MakeAccountsMap
call withenv.HeaderService
andenv.GasService
parameters.simapp/app.go (1)
- 290-290: The use of
runtime.NewEnvironment
simplifies service dependency management by consolidating them into a single environment. Ensure that all services previously accessed individually are now correctly accessed through the environment to maintain functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (2)
- x/accounts/keeper.go (6 hunks)
- x/accounts/keeper_account_abstraction.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/accounts/keeper.go
…ita/env-bundler-accounts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- simapp/app.go (1 hunks)
- x/accounts/go.mod (4 hunks)
- x/accounts/keeper.go (6 hunks)
- x/accounts/utils_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- simapp/app.go
- x/accounts/keeper.go
- x/accounts/utils_test.go
Additional comments: 4
x/accounts/go.mod (4)
- 26-27: Ensure the versions for
cosmossdk.io/x/bank
andcosmossdk.io/x/staking
are correctly set to match the project's requirements and compatibility with other modules.- 81-81: Adding
github.com/google/orderedcode v0.0.1
introduces a new dependency. Verify its usage within the module to ensure it's necessary and there are no lighter alternatives.- 103-103: The addition of
github.com/lib/pq v1.10.7
suggests database interactions. Confirm that this dependency is required for the module's functionality and that it adheres to security best practices for database connections.- 109-109:
github.com/minio/highwayhash v1.0.2
is added as a new dependency. Check its application within the module to ensure its use is justified and there are no performance implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but we should remove the event service as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- simapp/app.go (1 hunks)
- x/accounts/go.mod (4 hunks)
- x/accounts/go.sum (9 hunks)
- x/accounts/keeper.go (6 hunks)
Files skipped from review as they are similar to previous changes (4)
- simapp/app.go
- x/accounts/go.mod
- x/accounts/go.sum
- x/accounts/keeper.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- simapp/app.go (1 hunks)
- x/accounts/keeper.go (6 hunks)
- x/accounts/msg_server.go (1 hunks)
- x/accounts/utils_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- simapp/app.go
- x/accounts/keeper.go
- x/accounts/utils_test.go
Additional comments: 1
x/accounts/msg_server.go (1)
- 45-45: The update to use
environment.EventService
for event management aligns with the PR's objective to integrate an environment bundler into the accounts module. Ensure thatm.k.environment
is properly initialized before this call to avoid potential nil pointer dereferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/accounts/utils_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/accounts/utils_test.go
Description
Closes: #19417
This PR doesn't have much changes since accounts module is mostly migrated to use Environment
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.md
Reviewers 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
accountsKeeper
by using a unified environment approach.NewKeeper
function and various account operations to leverage the new environment system for service dependencies.go.mod
andgo.sum
to include newer versions and additional libraries.Keeper
.