feat(x/authz): Add env bundler to x/authz #19490
Conversation
WalkthroughWalkthroughThe changes involve updating the initialization of 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 0
Configuration used: .coderabbit.yml
Files selected for processing (14)
- simapp/app.go (1 hunks)
- x/authz/CHANGELOG.md (1 hunks)
- x/authz/keeper/genesis.go (1 hunks)
- x/authz/keeper/genesis_test.go (2 hunks)
- x/authz/keeper/grpc_query.go (3 hunks)
- x/authz/keeper/keeper.go (18 hunks)
- x/authz/keeper/keeper_test.go (2 hunks)
- x/authz/keeper/migrations.go (1 hunks)
- x/authz/keeper/msg_server.go (1 hunks)
- x/authz/migrations/v2/store.go (1 hunks)
- x/authz/migrations/v2/store_test.go (3 hunks)
- x/authz/module/abci_test.go (2 hunks)
- x/authz/module/depinject.go (3 hunks)
- x/authz/testutil/bank_helpers.go (1 hunks)
Additional comments: 37
x/authz/keeper/migrations.go (1)
- 21-21: The update to use
m.keeper.environmentin theMigrate1to2function aligns with the PR's objective of migrating to environment bundler services. Ensure that theenvironmentobject is correctly initialized and passed throughout the module to maintain consistency and functionality.x/authz/testutil/bank_helpers.go (5)
- 11-11: Renaming the context parameter from
goCtxtoctxin theSendfunction improves consistency and readability.- 15-15: Renaming the context parameter from
goCtxtoctxin theBurnfunction improves consistency and readability.- 19-19: Renaming the context parameter from
goCtxtoctxin theMultiSendfunction improves consistency and readability.- 23-23: Renaming the context parameter from
goCtxtoctxin theUpdateParamsfunction improves consistency and readability.- 27-27: Renaming the context parameter from
goCtxtoctxin theSetSendEnabledfunction improves consistency and readability.x/authz/module/depinject.go (2)
- 36-36: Replacing
store.KVStoreServicewithappmodule.Environmentin theModuleInputsstruct aligns with the PR's objective of enhancing the module's architecture. Ensure that theEnvironmentobject is correctly utilized throughout the module.- 47-47: The update in the
ProvideModulefunction to useappmodule.Environmentis consistent with the architectural changes. Verify the correct initialization and usage of theEnvironmentobject to ensure module functionality.x/authz/keeper/genesis.go (1)
- 13-13: The update to source the current time using
k.environment.HeaderService.GetHeaderInfo(ctx).Timealigns with the PR's objective of utilizing environment bundler services. Ensure that theHeaderServiceis correctly initialized and accessible through theenvironment.x/authz/CHANGELOG.md (3)
- 40-40: The changelog entry detailing the receipt of
appmodule.Environmenton the Keeper to access different application services accurately reflects the significant architectural changes made in the PR.- 40-40: The update to the
DequeueAndDeleteExpiredGrantsmethod to include a limit argument for the number of grants to prune is accurately documented in the changelog, highlighting an important improvement.- 40-40: Moving
AcceptResponsetosdk/types/authzand changing theUpdatedfield type is correctly noted in the changelog, providing clarity on API changes.x/authz/migrations/v2/store.go (2)
- 20-21: Updating the
MigrateStorefunction to takeappmodule.Environmentas a parameter aligns with the PR's objective of utilizing environment bundler services. Ensure that theEnvironmentobject is correctly utilized and that context handling is consistent.- 29-37: The change in the
addExpiredGrantsIndexfunction to useappmodule.Environmentand the update in context handling are consistent with the architectural enhancements. Verify the correct utilization of theEnvironmentobject and consistent context handling.x/authz/keeper/genesis_test.go (1)
- 50-50: Creating a new
runtime.Environmentobject in theSetupTestfunction and passing it to theNewKeeperfunction aligns with the PR's objective of utilizing environment bundler services, even in test setups. Ensure that theEnvironmentobject is correctly initialized and passed.x/authz/migrations/v2/store_test.go (1)
- 108-108: Creating a new
runtime.Environmentobject in the test setup and passing it to thev2.MigrateStorefunction aligns with the PR's objective of utilizing environment bundler services in test setups. Ensure that theEnvironmentobject is correctly initialized and passed.x/authz/keeper/msg_server.go (1)
- 120-123: Updating the
PruneExpiredGrantsfunction to use theEventServicefrom the environment for event emission, with explicit error handling, aligns with the PR's objective of utilizing environment bundler services for event management. Ensure that theEventServiceis correctly accessed through theenvironmentand that errors are appropriately handled.x/authz/module/abci_test.go (1)
- 35-35: Creating a new
runtime.Environmentobject in the test setup and passing it to theNewKeeperfunction aligns with the PR's objective of utilizing environment bundler services in test setups. Ensure that theEnvironmentobject is correctly initialized and passed.x/authz/keeper/grpc_query.go (3)
- 61-61: The update to use
runtime.KVStoreAdapter(k.environment.KVStoreService.OpenKVStore(ctx))for accessing the KVStore service through the environment is a positive change, aligning with the PR's objective to integrateappmodule.Environment. This enhances modularity and maintainability by centralizing the interaction with the environment.- 103-103: Using
runtime.KVStoreAdapter(k.environment.KVStoreService.OpenKVStore(ctx))in theGranterGrantsfunction is consistent with the overall goal of migrating to use the environment's services. This change ensures that the module's architecture is more cohesive and flexible.- 154-154: The modification in the
GranteeGrantsfunction to access the KVStore service via the environment (runtime.KVStoreAdapter(k.environment.KVStoreService.OpenKVStore(ctx))) is in line with the PR's architectural enhancements. It's a good practice to utilize the environment for such interactions, improving the module's integration with the Cosmos SDK's runtime environment.x/authz/keeper/keeper.go (13)
- 33-36: The integration of
appmodule.Environmentinto theKeeperstruct is a key architectural enhancement. This change allows theKeeperto interact more effectively with the Cosmos SDK's runtime environment, facilitating access to services like KVStore, logging, and event management through a unified interface.- 40-45: The
NewKeeperfunction correctly initializes theKeeperwith theappmodule.Environment, ensuring that all subsequent operations can leverage the environment's services. This is a crucial step in migrating the module to utilize environment bundler services.- 50-51: Accessing the logger via
k.environment.Logger.With("module", fmt.Sprintf("x/%s", authz.ModuleName))in theLoggermethod is a good practice. It ensures that logging is consistent and leverages the environment's centralized logging service.- 56-56: The
getGrantmethod's adaptation to use the environment's KVStore service (k.environment.KVStoreService.OpenKVStore(ctx)) for accessing grants is in line with the PR's objectives. This change enhances the method's integration with the environment.- 88-88: The
updatemethod's use of the environment's KVStore service for setting grant data is a positive change, aligning with the architectural enhancements aimed at making the module more environment-aware.- 96-96: The
DispatchActionsmethod's modification to source the current time from the environment (k.environment.HeaderService.GetHeaderInfo(ctx).Time) is a notable improvement. It reflects a shift towards using the environment for time sourcing, which is a more flexible and cohesive approach.- 158-158: Converting the context to
sdk.ContextwithinDispatchActions(sdk.UnwrapSDKContext(ctx)) is a temporary measure. It's important to note the TODO comment about removing this conversion once the baseapp's MsgServiceRouter migrates to usecontext.Context. This highlights an area for future improvement and ensures backward compatibility in the meantime.- 167-174: Emitting events using the environment's event service in
DispatchActionsis a good practice. It demonstrates the method's integration with the environment and the shift towards a more modular and environment-aware design.- 187-187: The
SaveGrantmethod's use of the environment's KVStore service for storing grants is consistent with the PR's goal of integratingappmodule.Environment. This change centralizes the interaction with the environment, enhancing modularity and maintainability.- 233-233: In the
DeleteGrantmethod, accessing the KVStore service through the environment for deleting grants aligns with the architectural enhancements of the module. This is another example of the module's migration towards a more cohesive and flexible architecture.- 261-261: The
GetAuthorizationsmethod's adaptation to use the environment's KVStore service for retrieving authorizations is in line with the PR's objectives. This enhances the method's integration with the environment and its services.- 291-291: The
GetAuthorizationmethod's use of the environment for time sourcing (k.environment.HeaderService.GetHeaderInfo(ctx).Time) when checking for grant expiration is a positive change. It reflects a broader adoption of the environment across different aspects of the module.- 310-310: The
IterateGrantsmethod's use of the environment's KVStore service for iterating over grants demonstrates the module's shift towards a more environment-aware design. This is consistent with the PR's architectural enhancements.x/authz/keeper/keeper_test.go (2)
- 55-55: The introduction of
runtime.NewEnvironmentwithstoreServiceandlog.NewNopLogger()as arguments is a significant change aligning with the PR's objectives. This encapsulates the environment setup within a single entity, enhancing modularity and maintainability. Ensure that thelog.NewNopLogger()is appropriate for all test scenarios, as it might suppress valuable debugging information during test failures.- 78-78: The modification to pass the
envobject toauthzkeeper.NewKeeperinstead ofstoreServicedirectly is a key part of integrating the environment bundler. This change centralizes the interaction with the environment, which is a positive step towards improving the module's architecture. However, ensure that all functionalities previously relying onstoreServicedirectly are fully compatible with the newenvabstraction.simapp/app.go (1)
- 358-358: The modification to initialize
AuthzKeeperusingruntime.NewEnvironmentinstead of directly usingruntime.NewKVStoreServicealigns with the PR's objective of integrating theappmodule.Environment. This change enhances the modularity and maintainability of thex/authzmodule by centralizing the interaction with the environment. It's a positive step towards a more cohesive architecture within the Cosmos SDK, facilitating better interaction with the runtime environment. Ensure that corresponding updates are made whereverAuthzKeeperis utilized to maintain consistency across the module.
x/authz/keeper/keeper.go
Outdated
| } | ||
|
|
||
| resp, err := authorization.Accept(sdkCtx, msg) | ||
| resp, err := authorization.Accept(ctx, msg) |
There was a problem hiding this comment.
Now that we pass a context, we need to do like in the x/feegrant PR (break interface, or set environement in context). Otherwise, some authorization will fail at unwrapping the context: https://github.com/cosmos/cosmos-sdk//blob/833547f2aaa67382fbdbd171245760ffcf5765c1/x/staking/types/authz.go#L104
There was a problem hiding this comment.
since we are using only time or height it might be safe. need to talk to @testinginprod
There was a problem hiding this comment.
ill open an issue on this, we will solve before release but lets unblock this since its blocked on a team call
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/authz/keeper/keeper.go (16 hunks)
Additional comments: 9
x/authz/keeper/keeper.go (9)
- 33-36: The
Keeperstruct has been updated to include anappmodule.Environmentfield, replacing the previous direct usage of services likeKVStoreService. This change centralizes the interaction with the Cosmos SDK's runtime environment, enhancing modularity and maintainability. Ensure that all instances and usages of theKeeperstruct throughout the codebase are updated to accommodate this new field.- 50-51: The
Loggermethod now accesses the logger from theappmodule.Environment, which is a positive change towards utilizing the environment's capabilities. This approach allows for more centralized logging management. Ensure that the logger obtained from the environment is correctly configured for thex/authzmodule.- 167-174: The
DispatchActionsmethod now handles events differently, utilizing theEventServicefrom theappmodule.Environment. This change improves the flexibility and modularity of event management. However, ensure that the event names and attributes are consistent and meaningful for downstream consumers. Additionally, consider the performance implications of emitting events within a loop and verify that this does not introduce any significant overhead.- 183-193: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [187-223]
The
SaveGrantmethod has been updated to interact with the environment for event management, specifically using theEventServiceto emit events. This is a good practice as it leverages the environment's capabilities for a more cohesive architecture. However, ensure that the event structure and emitted information are comprehensive and useful for event consumers. Also, verify that the error handling for event emission is adequate.
- 230-236: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [233-252]
The
DeleteGrantmethod's changes are consistent with the overall shift towards using theappmodule.Environment. The method now uses theKVStoreServiceandEventServicefrom the environment. This enhances the method's integration with the broader Cosmos SDK environment. Ensure that the deletion logic is thoroughly tested, especially in scenarios involving concurrent access or failure cases.
- 287-294: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [261-291]
The
GetAuthorizationsandGetAuthorizationmethods have been updated to use theKVStoreServicefrom theappmodule.Environment. This change aligns with the objective of centralizing environment interactions. However, it's important to ensure that the iteration and unmarshalling logic inGetAuthorizationsis efficient and does not introduce unnecessary overhead, especially for large datasets.
- 321-327: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [310-344]
The
IterateGrantsand related methods (getGrantQueueItem,setGrantQueueItem) have been refactored to utilize theKVStoreServicefrom theappmodule.Environment. This is a positive step towards a more environment-aware design. However, given the potential for significant I/O operations, as mentioned in the comment forIterateGrants, ensure that these methods are used judiciously and that appropriate gas costs are accounted for in transaction processing.
- 381-388: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [365-385]
The
removeFromGrantQueuemethod now consumes gas for each iteration over the grant queue items, which is a good practice for accounting for the computational cost of operations. However, ensure that the gas cost per iteration (gasCostPerIteration) is calibrated correctly based on empirical data or benchmarks to prevent either overcharging or undercharging for the operation.
- 406-408: The
DequeueAndDeleteExpiredGrantsmethod has been updated to use theKVStoreServicefrom theappmodule.Environmentfor deleting expired grants. This method's changes are in line with the overall architectural shift. Ensure that the deletion logic, especially the iteration limit to avoid excessive processing time, is well-tested and that the method behaves correctly under various load conditions.
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
lgtm, ill open an issue on the interface stuff
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/authz/keeper/msg_server.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/authz/keeper/msg_server.go
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
As per #19490 (comment), ACK.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- simapp/app.go (1 hunks)
- x/authz/keeper/genesis_test.go (2 hunks)
- x/authz/keeper/keeper_test.go (2 hunks)
- x/authz/migrations/v2/store_test.go (3 hunks)
- x/authz/module/abci_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- simapp/app.go
- x/authz/keeper/genesis_test.go
- x/authz/keeper/keeper_test.go
- x/authz/migrations/v2/store_test.go
- x/authz/module/abci_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/authz/keeper/keeper_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/authz/keeper/keeper_test.go
Description
ref: #19290
Migrate x/authz to use environment bundler services
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
AuthzKeeperto enhance performance and reliability.