-
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/group): Add env bundler to group module #19489
Conversation
WalkthroughWalkthroughThe overarching change integrates a new 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 (
|
@@ -451,7 +450,7 @@ | |||
return errorsmod.Wrap(err, "doTallyAndUpdate") | |||
} | |||
|
|||
if err := k.proposalTable.Update(k.storeService.OpenKVStore(ctx), proposal.Id, &proposal); err != nil { | |||
if err := k.proposalTable.Update(k.environment.KVStoreService.OpenKVStore(ctx), proposal.Id, &proposal); err != nil { |
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop.
@@ -349,7 +348,7 @@ | |||
|
|||
//nolint:gosec // "implicit memory aliasing in the for loop (because of the pointer on &v)" | |||
for _, v := range votes { | |||
err = k.voteTable.Delete(k.storeService.OpenKVStore(ctx), &v) | |||
err = k.voteTable.Delete(k.environment.KVStoreService.OpenKVStore(ctx), &v) |
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop.
@@ -308,7 +307,7 @@ | |||
if proposalInfo.Status == group.PROPOSAL_STATUS_SUBMITTED { | |||
proposalInfo.Status = group.PROPOSAL_STATUS_ABORTED | |||
|
|||
if err := k.proposalTable.Update(k.storeService.OpenKVStore(ctx), proposalInfo.Id, &proposalInfo); err != nil { | |||
if err := k.proposalTable.Update(k.environment.KVStoreService.OpenKVStore(ctx), proposalInfo.Id, &proposalInfo); err != nil { |
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop.
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: 2
Configuration used: .coderabbit.yml
Files selected for processing (18)
- simapp/app.go (1 hunks)
- x/group/CHANGELOG.md (1 hunks)
- x/group/keeper/abci.go (1 hunks)
- x/group/keeper/abci_test.go (23 hunks)
- x/group/keeper/genesis.go (2 hunks)
- x/group/keeper/genesis_test.go (2 hunks)
- x/group/keeper/grpc_query.go (14 hunks)
- x/group/keeper/grpc_query_test.go (1 hunks)
- x/group/keeper/invariants.go (1 hunks)
- x/group/keeper/keeper.go (13 hunks)
- x/group/keeper/keeper_test.go (9 hunks)
- x/group/keeper/migrations.go (1 hunks)
- x/group/keeper/msg_server.go (44 hunks)
- x/group/keeper/msg_server_test.go (2 hunks)
- x/group/keeper/proposal_executor.go (1 hunks)
- x/group/keeper/tally.go (2 hunks)
- x/group/module/depinject.go (3 hunks)
- x/group/module/module.go (1 hunks)
Additional comments: 73
x/group/keeper/abci.go (1)
- 7-15: The implementation of the
EndBlocker
function correctly integrates the new environment bundler setup and follows best practices for error handling and context usage. Good job on ensuring that the function is concise and maintainable.x/group/keeper/migrations.go (1)
- 23-23: The update to use
m.keeper.environment.KVStoreService
in theMigrate1to2
function aligns with the migration to an environment bundler, ensuring that the migration utilizes the new setup correctly.x/group/CHANGELOG.md (1)
- 35-35: The changelog entry is well-documented, clearly stating the new feature related to the integration of
appmodule.Environment
with the Keeper. It follows the established changelog format and principles.x/group/module/depinject.go (1)
- 32-32: The updates in
depinject.go
, specifically the replacement ofStoreService
withEnvironment
in theProvideModule
function, correctly align with the migration to an environment bundler. This ensures proper interaction with external dependencies through the new setup.Also applies to: 48-48
x/group/keeper/tally.go (1)
- 16-16: The updates in the
Tally
function, specifically the replacement ofsdk.Context
withcontext.Context
and the use ofk.environment.KVStoreService
forkvStore
initialization, correctly reflect the migration to an environment bundler and the new context management approach.Also applies to: 26-26
x/group/keeper/genesis.go (1)
- 20-20: The updates in the
InitGenesis
andExportGenesis
functions to usek.environment.KVStoreService
align with the migration to an environment bundler, ensuring that the genesis processes utilize the new setup correctly.Also applies to: 55-55
x/group/keeper/proposal_executor.go (1)
- 19-19: The addition of the comment regarding the future use of
context.Context
andenv bundler service
is clear and provides valuable insight into future development plans. It's a good practice to document such intentions for future reference.x/group/keeper/invariants.go (1)
- 29-29: The update in the
GroupTotalWeightInvariant
function to usekeeper.environment.KVStoreService
aligns with the migration to an environment bundler, ensuring that the invariant check utilizes the new setup correctly.x/group/module/module.go (1)
- 151-151: The simplification of the
EndBlock
method to directly callkeeper.EndBlocker
is a positive change, enhancing the clarity and maintainability of the code.x/group/keeper/genesis_test.go (2)
- 56-56: The introduction of the
runtime.NewEnvironment
call to create anenv
variable is aligned with the PR's objective to integrate theappmodule.Environment
into theKeeper
structure. This change enhances the module's interaction with different application services by providing streamlined access.- 77-77: The modification to include the
env
parameter in thekeeper.NewKeeper
function call is correctly implemented. This change is crucial for utilizing the new environment bundler, enabling more efficient access to application services within thex/group
module.x/group/keeper/grpc_query_test.go (1)
- 72-74: The update to initialize the
groupKeeper
with a newruntime.Environment
instance is correctly implemented and aligns with the PR's objective to enhance the module's architecture by integrating theappmodule.Environment
. This change facilitates improved access to application services.x/group/keeper/grpc_query.go (25)
- 22-22: The change from
sdk.Context
tocontext.Context
in theGroupInfo
function aligns with the PR's objective to utilize the environment bundler. Ensure that all downstream calls within this function and related functions are compatible withcontext.Context
.- 33-35: The
getGroupInfo
function correctly usescontext.Context
. It's important to verify thatk.environment.KVStoreService.OpenKVStore(ctx)
properly handles the standard Gocontext.Context
and that any context cancellation or deadlines are respected.- 40-40: The
GroupPolicyInfo
function's switch tocontext.Context
is consistent with the overall migration strategy. Similar to other functions, ensure compatibility and proper handling of the context throughout the call chain.- 55-57: In
getGroupPolicyInfo
, the use ofcontext.Context
and interaction withk.environment.KVStoreService.OpenKVStore(ctx)
should be checked for correct context handling, especially regarding cancellation and deadlines.- 61-61: The
GroupMembers
function's adaptation tocontext.Context
is part of the module's migration to use an environment bundler. Ensure that the context is correctly propagated through all underlying calls.- 81-82: For
getGroupMembers
, it's crucial to confirm thatk.environment.KVStoreService.OpenKVStore(ctx)
respects thecontext.Context
passed, especially for long-running operations that might be subject to cancellation.- 86-86: The
GroupsByAdmin
function's update to usecontext.Context
is in line with the migration goals. Ensure that all related operations properly handle the context, including any potential error handling or cancellation scenarios.- 109-110: In
getGroupsByAdmin
, verify that the context passed tok.environment.KVStoreService.OpenKVStore(ctx)
is correctly utilized, particularly in terms of handling cancellations or timeouts.- 114-114: The
GroupPoliciesByGroup
function's switch tocontext.Context
is consistent with the migration to an environment bundler. Ensure that the context is properly propagated and handled in all underlying calls.- 134-135: For
getGroupPoliciesByGroup
, it's important to ensure thatk.environment.KVStoreService.OpenKVStore(ctx)
correctly handles thecontext.Context
, especially for operations that might be affected by context cancellation or deadlines.- 140-140: The
GroupPoliciesByAdmin
function's adaptation tocontext.Context
aligns with the module's migration strategy. As with other functions, ensure that the context is correctly handled throughout the call chain.- 163-164: In
getGroupPoliciesByAdmin
, confirm thatk.environment.KVStoreService.OpenKVStore(ctx)
properly utilizes thecontext.Context
, particularly in terms of respecting cancellations and deadlines.- 168-168: The
Proposal
function's update to usecontext.Context
is part of the broader migration to an environment bundler. Ensure that all related operations and downstream calls correctly handle the context.- 179-179: The
ProposalsByGroupPolicy
function's switch tocontext.Context
is consistent with the module's migration goals. Verify that the context is properly propagated and handled in all underlying operations.- 202-203: For
getProposalsByGroupPolicy
, it's crucial to ensure thatk.environment.KVStoreService.OpenKVStore(ctx)
respects thecontext.Context
passed, especially for operations that might be subject to cancellation.- 207-209: The
getProposal
function correctly usescontext.Context
. Verify thatk.environment.KVStoreService.OpenKVStore(ctx)
properly handles the standard Gocontext.Context
, including any context cancellation or deadlines.- 216-216: The
VoteByProposalVoter
function's adaptation tocontext.Context
aligns with the module's migration strategy. Ensure that the context is correctly handled throughout the call chain, especially in error handling or cancellation scenarios.- 232-232: The
VotesByProposal
function's update to usecontext.Context
is part of the broader migration to an environment bundler. Verify that all related operations and downstream calls correctly handle the context.- 252-252: The
VotesByVoter
function's switch tocontext.Context
is consistent with the module's migration goals. Ensure that the context is properly propagated and handled in all underlying operations.- 275-275: The
GroupsByMember
function's adaptation tocontext.Context
aligns with the module's migration strategy. As with other functions, ensure that the context is correctly handled throughout the call chain.- 312-314: In
getVote
, confirm thatk.environment.KVStoreService.OpenKVStore(ctx)
properly utilizes thecontext.Context
, particularly in terms of respecting cancellations and deadlines.- 318-319: For
getVotesByProposal
, it's important to ensure thatk.environment.KVStoreService.OpenKVStore(ctx)
correctly handles thecontext.Context
, especially for operations that might be affected by context cancellation or deadlines.- 323-324: In
getVotesByVoter
, verify that the context passed tok.environment.KVStoreService.OpenKVStore(ctx)
is correctly utilized, particularly in terms of handling cancellations or timeouts.- 328-328: The
TallyResult
function's update to usecontext.Context
is part of the broader migration to an environment bundler. Ensure that all related operations and downstream calls correctly handle the context, including any potential error handling or cancellation scenarios.- 356-356: The
Groups
function's switch tocontext.Context
is consistent with the module's migration goals. Verify that the context is properly propagated and handled in all underlying operations, especially in terms of respecting cancellations and deadlines.x/group/keeper/keeper_test.go (9)
- 12-12: The addition of the
"cosmossdk.io/core/appmodule"
import is necessary for utilizing theappmodule.Environment
type within the test suite. This change aligns with the PR's objective to integrate the environment bundler into thex/group
module.- 49-49: Adding the
environment
field of typeappmodule.Environment
to theTestSuite
struct is a crucial step in enabling the test suite to utilize the new environment bundler. This change is consistent with the PR's goal of enhancing the module's interaction with application services.- 61-62: Initializing the
env
variable withruntime.NewEnvironment(storeService, log.NewNopLogger())
and subsequently assigning it tos.environment
is a correct implementation for setting up the environment bundler within the test suite. This setup is essential for the subsequent use of theenvironment
in keeper and other module functions.- 83-83: The modification to the
keeper.NewKeeper
call to include theenv
parameter is in line with the changes made to the keeper's constructor to accept anappmodule.Environment
parameter. This adjustment ensures that the keeper instance within the test suite is correctly initialized with the environment bundler.- 87-88: Assigning the initialized
env
tos.environment
correctly stores the environment instance for use in subsequent tests. This assignment is necessary for tests that require access to the environment bundler.- 280-280: The modification to the
EndBlocker
call to include thespec.newCtx
parameter is consistent with the updated function signature. However, it's important to note that theEndBlocker
function does not require theenvironment
parameter directly in its arguments. The context provided should already be enriched with necessary environmental data, ensuring that the function operates within the correct context.- 342-342: The modification to the
PruneProposals
call to include thes.environment
parameter is necessary following the changes to the function signature. This ensures that the function has access to the environment bundler, allowing it to perform its operations with the correct context and access to application services.- 465-465: The call to
TallyProposalsAtVPEnd
with thectx
ands.environment
parameters correctly reflects the updated function signature. This change is part of the broader effort to integrate the environment bundler into the module's operations, ensuring that the tallying process has access to the necessary application services.- 532-532: Similar to the previous comment, the call to
TallyProposalsAtVPEnd
in the context of testing group member leaving scenarios is correctly updated to include theenvironment
parameter. This ensures that the function operates with the correct context and access to application services, even in more complex test scenarios.x/group/keeper/abci_test.go (4)
- 1-1: Renaming the package from
module_test
tokeeper_test
aligns with Go conventions for naming test packages. This change improves clarity by explicitly indicating that these tests are for thekeeper
package within thex/group
module.- 166-166: The replacement of
submitProposalAndVote
withsubmitProposalAndVoteHelper
across multiple test cases ensures consistency in how proposals are submitted and voted on within these tests. This change likely reflects an effort to standardize test utilities and improve the readability and maintainability of the test code.Also applies to: 181-181, 196-196, 211-211, 225-225, 245-245, 257-257, 272-272, 288-288, 310-310
- 336-336: Directly calling the
EndBlocker
function ons.groupKeeper
in the test cases for proposal pruning and tallying, instead of through themodule
package, is a significant change. This approach is more direct and likely reflects the architectural changes introduced by the PR, where thex/group
module's interaction with application services is streamlined through theappmodule.Environment
. It's important to ensure that this direct invocation accurately simulates the conditions under whichEndBlocker
would be called in a live environment.Also applies to: 539-539
- 558-558: The introduction of
submitProposalHelper
andsubmitProposalAndVoteHelper
functions at the end of the file is a good practice, as it abstracts the proposal submission and voting logic into reusable components. This not only DRYs up the test code but also makes it easier to understand and maintain. However, it's crucial to ensure that these helper functions accurately represent the proposal submission and voting process as it would occur in the actual application.Also applies to: 576-576
simapp/app.go (1)
- 368-368: The initialization of
app.GroupKeeper
has been updated to include aruntime.NewEnvironment
call, passinglogger
as an argument. This change aligns with the PR's objective to integrate theappmodule.Environment
into theKeeper
structure for thex/group
module, facilitating enhanced access to application services. The modification appears to be correctly implemented, adhering to the described objectives and best practices for maintainability and modularity.However, it's important to ensure that all downstream usages of
app.GroupKeeper
and related components are updated to accommodate this change, especially if they rely on the previous initialization pattern. Additionally, thorough testing should be conducted to verify that the integration of the environment bundler does not introduce any unintended side effects or regressions in the module's functionality.x/group/keeper/msg_server.go (22)
- 29-29: The function
CreateGroup
now correctly usescontext.Context
as its context parameter, aligning with the migration to the new environment bundler. This change is consistent and correctly implemented.- 64-64: The use of
k.environment.KVStoreService.OpenKVStore(ctx)
to obtain the KV store is a significant change that aligns with the new environment bundler approach. This ensures that theKeeper
can interact with the KV store in a context-aware manner, improving modularity and maintainability.- 71-71: The retrieval of the current block time using
k.environment.HeaderService.GetHeaderInfo(ctx).Time
is a good practice, ensuring that the timestamp used inCreateGroup
and other operations is consistent and accurate.- 94-94: The use of
k.environment.EventService.EventManager(ctx).Emit
for emitting events is correctly implemented, leveraging the new environment bundler. This change enhances the modularity and flexibility of event management within the module.- 101-101: The migration of
UpdateGroupMembers
to usecontext.Context
and the new environment bundler is correctly implemented. This change is consistent across the module and improves the maintainability and modularity of the code.- 114-114: The logic for updating group members, including handling member weight updates and deletions, is well-structured and maintains the integrity of group weights. The use of the new environment bundler for accessing the KV store and other services is correctly applied.
- 192-192: The addition of the member's
AddedAt
timestamp usingk.environment.HeaderService.GetHeaderInfo(ctx).Time
during member creation is a good practice, ensuring consistency in the handling of timestamps across the module.- 221-221: The
UpdateGroupAdmin
function's migration to usecontext.Context
and the new environment bundler is correctly implemented, aligning with the module's overall migration strategy. This enhances the maintainability and modularity of the code.- 253-253: The changes in
UpdateGroupMetadata
to usecontext.Context
and leverage the new environment bundler are correctly implemented. This consistency in the migration approach across the module is commendable.- 328-328: The
CreateGroupPolicy
function's adaptation to usecontext.Context
and the new environment bundler, especially for decision policy validation and group policy account creation, is well-executed. This change enhances the module's architecture and maintainability.- 370-370: The loop for handling potential address collisions during group policy account creation is a prudent measure. It ensures the uniqueness of the account address, which is crucial for the integrity of group policies.
- 421-421: The use of
k.environment.EventService.EventManager(ctx).Emit
for emitting theEventCreateGroupPolicy
event is correctly implemented, leveraging the new environment bundler for event management.- 428-428: The migration of
UpdateGroupPolicyAdmin
to usecontext.Context
and the new environment bundler is correctly implemented. This change is consistent with the module's overall migration strategy and improves the code's maintainability.- 451-451: The
UpdateGroupPolicyDecisionPolicy
function's adaptation to usecontext.Context
and the new environment bundler, especially for decision policy validation and update, is well-executed. This enhances the module's architecture and maintainability.- 489-489: The changes in
UpdateGroupPolicyMetadata
to usecontext.Context
and leverage the new environment bundler are correctly implemented. This consistency in the migration approach across the module is commendable.- 511-511: The
SubmitProposal
function's comprehensive update to usecontext.Context
and the new environment bundler, especially for proposal validation and submission, is well-executed. This change significantly enhances the module's functionality and maintainability.- 563-563: The logic for ensuring that only group members can submit proposals and for executing proposal messages is well-structured and correctly leverages the new environment bundler. This maintains the integrity and functionality of the proposal submission process.
- 621-621: The use of
k.environment.EventService.EventManager(ctx).Emit
for emitting theEventSubmitProposal
event is correctly implemented, leveraging the new environment bundler for event management. This enhances the modularity and flexibility of event handling within the module.- 655-655: The migration of
WithdrawProposal
to usecontext.Context
and the new environment bundler is correctly implemented. This change is consistent with the module's overall migration strategy and improves the code's maintainability.- 697-697: The
Vote
function's adaptation to usecontext.Context
and the new environment bundler, especially for vote validation and storage, is well-executed. This enhances the module's architecture and maintainability.- 781-781: The
doTallyAndUpdate
function's logic for performing a tally and updating the proposal status based on the tally result is well-structured and correctly leverages the new environment bundler. This maintains the integrity of the voting and proposal finalization process.- 914-914: The
LeaveGroup
function's comprehensive update to usecontext.Context
and the new environment bundler, especially for group member removal and group weight update, is well-executed. This change significantly enhances the module's functionality and maintainability.
key := storetypes.NewKVStoreKey(group.StoreKey) | ||
env := runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()) | ||
vpe := resp.Proposal.VotingPeriodEnd | ||
timeDiff := vpe.Sub(s.sdkCtx.HeaderInfo().Time) | ||
ctxVPE := sdkCtx.WithHeaderInfo(header.Info{Time: s.sdkCtx.HeaderInfo().Time.Add(timeDiff).Add(time.Second * 1)}) | ||
s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctxVPE)) | ||
s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctxVPE, env)) |
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.
The use of runtime.NewEnvironment
and runtime.NewKVStoreService
within a test case seems unusual and might not reflect a typical usage pattern in Cosmos SDK modules. It's important to ensure that this setup accurately represents the intended environment and store interactions for the functionality being tested. If this pattern is specific to the test environment and does not mirror the actual application's runtime environment, consider adding a comment to clarify its purpose and usage in this context.
// Ensure the use of runtime.NewEnvironment and runtime.NewKVStoreService accurately represents the intended environment for the functionality being tested.
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/group/CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/group/CHANGELOG.md
Description
ref: #19290
Migrate x/group module to use environment bundler
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
EndBlocker
function that updates proposal tallies and prunes expired proposals.appmodule.Environment
.runtime.Environment
.context.Context
for consistency.environment
parameter.