-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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): use router service from env #20003
Conversation
WalkthroughWalkthroughThe recent updates primarily streamline the interaction with routers and services across various modules in the application. The changes involve removing older router interfaces and replacing them with direct calls to new modular services within the Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
@julienrbrt your pull request is missing a changelog! |
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.
Actionable comments posted: 1
@@ -1,29 +1,19 @@ | |||
package 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.
Tip
Codebase Verification
The removed mock functions mockQuery
and mockExec
from x/accounts/keeper_test.go
are indeed referenced in other test files within the repository (x/bank/keeper/grpc_query_test.go
and x/bank/keeper/keeper_test.go
). This indicates that the elimination of these mock functions could potentially affect other test functionalities or dependencies within the test suite.
Analysis chain
Confirm that the elimination of redundant mock functions does not affect other test functionalities or dependencies within the test suite.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to eliminated mock functions in the test files to ensure they are not used elsewhere.
rg --type go --files-with-matches --glob '*_test.go' 'mock'
Length of output: 3896
Script:
#!/bin/bash
# First, identify the specific mock functions that were removed from x/accounts/keeper_test.go
git diff origin/main...HEAD -- x/accounts/keeper_test.go | grep -P '^\-.*mock.*\('
# Then, check if these specific mock functions are referenced in other test files
rg --type go --files-with-matches --glob '*_test.go' 'specific_mock_function_name_here'
Length of output: 673
Script:
#!/bin/bash
# Search for references to 'mockQuery' and 'mockExec' in the test files to ensure they are not used elsewhere.
rg --type go --files-with-matches --glob '*_test.go' 'mockQuery|mockExec'
Length of output: 137
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.
Actionable comments posted: 2
// TODO: Add a way to inject custom accounts. | ||
// Currently only the base account is supported. |
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.
Consider providing more details or a timeline in the TODO comment.
Would you like assistance in defining a more detailed plan or creating a tracking issue for this TODO?
x/accounts/depinject.go
Outdated
in.Cdc, in.Environment, in.AddressCodec, in.Cdc, | ||
in.ExecRouter, in.QueryRouter, in.Registry, account, | ||
) | ||
accountskeeper, err := NewKeeper(in.Cdc, in.Environment, in.AddressCodec, in.Cdc, in.Registry, account) |
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.
Improve error handling by avoiding panic and returning an error instead.
- panic(err)
+ return ModuleOutputs{}, err
This change allows the caller of ProvideModule
to handle the error appropriately, improving the robustness and maintainability of the module.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
accountskeeper, err := NewKeeper(in.Cdc, in.Environment, in.AddressCodec, in.Cdc, in.Registry, account) | |
accountskeeper, err := NewKeeper(in.Cdc, in.Environment, in.AddressCodec, in.Cdc, in.Registry, account) | |
return ModuleOutputs{}, err |
if we can merge this so I can stop fixing merge conflicts that would be great! |
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.
seems like integration test is failing
* main: refactor(x/simulation)!: remove accounts string (cosmos#20056) fix(baseapp): don't share global gas meter in tx execution (cosmos#19616) feat(x/accounts): use router service from env (cosmos#20003) refactor(x): remove Address.String() (cosmos#20048) build(deps): Bump github.com/jhump/protoreflect from 1.15.6 to 1.16.0 in /tests (cosmos#20040) feat(collections): add `NewJSONValueCodec` (cosmos#19861)
Description
Use router service from environment instead of custom x/accounts router service
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
New Features
Refactor
Bug Fixes
Documentation