feat(x/accounts): add event emission to accounts module #19636
Conversation
WalkthroughThe changes revolve around the introduction of Changes
Assessment against linked issues
The code changes directly address the primary objective outlined in the linked issue by introducing event emission capabilities in the accounts module, as evidenced by the addition of event handling and emission functionalities in 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 (
|
|
@tac0turtle your pull request is missing a changelog! |
![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 (13)
- x/accounts/account_test.go (6 hunks)
- x/accounts/accountstd/exports.go (2 hunks)
- x/accounts/defaults/base/account.go (4 hunks)
- x/accounts/internal/implementation/account_test.go (2 hunks)
- x/accounts/internal/implementation/api_builder.go (7 hunks)
- x/accounts/internal/implementation/api_builder_test.go (2 hunks)
- x/accounts/internal/implementation/context_test.go (2 hunks)
- x/accounts/internal/implementation/implementation.go (2 hunks)
- x/accounts/internal/implementation/implementation_test.go (1 hunks)
- x/accounts/internal/implementation/protoaccount.go (4 hunks)
- x/accounts/keeper.go (3 hunks)
- x/accounts/testing/account_abstraction/minimal.go (2 hunks)
- x/accounts/testing/counter/counter.go (5 hunks)
Additional comments: 40
x/accounts/internal/implementation/api_builder_test.go (2)
- 14-18: The addition of
env appmodule.Environmentas a parameter inRegisterExecuteHandlerfunctions withinTestRouterDoubleRegistrationaligns with the new requirement for handlers to be aware of the environment context. This change is consistent and correctly implemented.- 36-39: In
TestEmptyQueryExecuteHandler, the introduction ofenv appmodule.Environmentas a parameter for theqhandehfunctions is correctly implemented, ensuring that the environment context is passed to query and execute handlers. This is a necessary adjustment to accommodate the new event emission functionality.x/accounts/internal/implementation/account_test.go (3)
- 29-29: Adding
appmodule.Environmentas a parameter inRegisterInitHandleris correctly implemented, ensuring that initialization handlers can access environmental context. This is crucial for the new event emission functionality.- 35-44: The modifications in
RegisterExecuteHandlersto includeappmodule.Environmentin the function signatures are correctly implemented. This ensures that execute handlers have access to the environment, which is essential for the intended functionality.- 50-53: The changes in
RegisterQueryHandlersto includeappmodule.Environmentin the function signatures are correctly implemented. This adjustment allows query handlers to utilize environmental context, aligning with the new requirements.x/accounts/internal/implementation/implementation_test.go (1)
- 19-19: The creation of
env appmodule.Environmentand its inclusion in method calls withinTestImplementationis correctly implemented. This ensures that the tests accurately reflect the new requirement for environment context inExecute,Init, andQueryfunctions.x/accounts/internal/implementation/protoaccount.go (3)
- 14-23: The addition of
appmodule.Environmentto theRegisterInitHandlerfunction signature is correctly implemented. This change ensures that initialization handlers for smart accounts using protobuf can interact with the environment context.- 41-52: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [35-49]
The update to
RegisterExecuteHandlerto includeappmodule.Environmentin the function signature is correctly implemented. This adjustment allows execution handlers for smart accounts using protobuf to utilize environmental context, which is essential for the new functionality.
- 61-61: The inclusion of
appmodule.Environmentin theRegisterQueryHandlerfunction signature is correctly implemented. This ensures that query handlers for smart accounts using protobuf can access the environment, aligning with the new requirements.x/accounts/testing/account_abstraction/minimal.go (3)
- 39-39: The inclusion of
appmodule.Environmentin theInitmethod signature forMinimalAbstractedAccountis correctly implemented, ensuring that initialization can access environmental context.- 48-54: The update to the
Authenticatemethod to includeappmodule.Environmentand to emit an event upon authentication is correctly implemented. This enhances the method's functionality by integrating event emission and ensuring error handling is in place.- 59-59: The addition of
appmodule.Environmentto theQueryAuthenticateMethodsmethod signature is correctly implemented, aligning with the new requirement for methods to access the environment context.x/accounts/internal/implementation/context_test.go (1)
- 35-36: The creation of
env appmodule.Environmentand its inclusion in theimpl.Executefunction call withinTestMakeAccountContextis correctly implemented. This ensures that the test accurately reflects the new requirement for environment context in execution.x/accounts/account_test.go (3)
- 30-30: Adding
appmodule.Environmentas a parameter inRegisterInitHandleris correctly implemented, ensuring that initialization handlers can access environmental context. This is crucial for the new event emission functionality.- 38-48: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [41-77]
The modifications in
RegisterExecuteHandlersto includeappmodule.Environmentin the function signatures are correctly implemented. This ensures that execute handlers have access to the environment, which is essential for the intended functionality.
- 74-96: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [83-111]
The changes in
RegisterQueryHandlersto includeappmodule.Environmentin the function signatures are correctly implemented. This adjustment allows query handlers to utilize environmental context, aligning with the new requirements.x/accounts/internal/implementation/api_builder.go (3)
- 27-27: The addition of
appmodule.Environmentto thehandlerfunction signature inInitBuilderis correctly implemented. This change ensures that initialization handlers can interact with the environment context.- 54-54: Updating the
handlersmap inExecuteBuilderto includeappmodule.Environmentin function signatures is correctly implemented. This allows execution handlers to utilize environmental context, which is essential for the new functionality.- 101-101: The use of
appmodule.Environmentin theQueryBuilderthrough theExecuteBuilderinstance is a clever design choice, ensuring that query handlers also have access to the environment. This maintains consistency across the implementation.x/accounts/testing/counter/counter.go (5)
- 10-10: The addition of
"cosmossdk.io/core/appmodule"import is necessary for utilizing theappmodule.Environmentparameter in function signatures. This change aligns with the PR's objective to integrate event emission functionality by leveraging the environment context.- 55-55: Adding the
env appmodule.Environmentparameter to theInitfunction signature is a crucial change for incorporating the environment context into the account initialization process. This allows the function to interact with the broader application environment, which is essential for the new event emission feature. However, it's important to ensure that all calls to this function are updated to include the new parameter.- 68-68: The modification to include
env appmodule.Environmentin theIncreaseCounterfunction signature is appropriate for the feature being implemented. It ensures that event emission can be contextually aware of the environment. Similar to theInitfunction, verify that all invocations ofIncreaseCounterhave been updated accordingly.- 91-91: Incorporating the
env appmodule.Environmentparameter into theQueryCounterfunction is consistent with the overall goal of enhancing the module's interactivity and monitoring capabilities. This change allows the function to leverage the application's environment for event emission or other environment-specific logic.- 101-101: The addition of
env appmodule.Environmentto theTestDependenciesfunction signature is a good practice for testing the module's interaction with the application environment. This ensures that the testing framework can simulate different environmental contexts, which is valuable for thorough testing of the event emission functionality.x/accounts/internal/implementation/implementation.go (4)
- 11-11: The addition of
"cosmossdk.io/core/appmodule"import is necessary for utilizing theappmodule.Environmentparameter in function signatures within theImplementationstruct. This change is consistent with the PR's objective to enhance the module's capabilities by integrating the environment context.- 115-115: Adding the
env appmodule.Environmentparameter to theInitfunction within theImplementationstruct is a significant change that aligns with the PR's goal of integrating the environment context into the module's operations. This modification allows theInitfunction to access and interact with the broader application environment, which is crucial for the new event emission functionality.- 117-117: The inclusion of
env appmodule.Environmentin theExecutefunction signature is appropriate and necessary for the feature being implemented. It ensures that the execution process can be contextually aware of the environment, which is essential for event emission and other environment-specific logic.- 119-119: Incorporating the
env appmodule.Environmentparameter into theQueryfunction is consistent with the overall goal of enhancing the module's interactivity and monitoring capabilities. This change allows the function to leverage the application's environment for event emission or other environment-specific logic.x/accounts/accountstd/exports.go (4)
- 9-9: The import statement for
cosmossdk.io/core/appmoduleis correctly added and follows Go conventions.- 38-38: The addition of
env appmodule.Environmentto the function signature ofRegisterExecuteHandleraligns with the PR's objectives. Ensure all calls to this function are updated accordingly.- 46-46: The addition of
env appmodule.Environmentto the function signature ofRegisterQueryHandleris appropriate. Verify that all invocations of this function have been updated to include the new parameter.- 54-54: Including
env appmodule.Environmentin the function signature ofRegisterInitHandleris consistent with the PR's goal. Confirm that all calls to this function are adjusted to pass the new parameter.x/accounts/defaults/base/account.go (5)
- 16-16: The import statement for
cosmossdk.io/core/appmoduleis correctly added, following Go conventions.- 58-58: The inclusion of
_ appmodule.Environmentin theInitfunction signature aligns with the PR's objectives. If the environment parameter is intended for future use, consider documenting its purpose or implementing its usage now.- 62-62: Including
_ appmodule.Environmentin theSwapPubKeyfunction signature is consistent with the PR's goals. As withInit, if the environment parameter is planned for future use, its intended purpose should be documented or implemented.- 79-79: The addition of
_ appmodule.Environmentto theAuthenticatefunction signature is appropriate. Again, if the environment parameter is to be used in the future, consider documenting its intended use or implementing it now.- 199-199: The inclusion of
_ appmodule.Environmentin theQuerySequencefunction signature aligns with the PR's objectives. As with other functions, if the environment parameter is intended for future use, its purpose should be documented or implemented.x/accounts/keeper.go (3)
- 176-176: The inclusion of
k.environmentin the call toimpl.Initwithin theinitfunction correctly aligns with the PR's objectives. Ensure that theimpl.Initfunction properly utilizes the environment parameter.- 237-237: Passing
k.environmenttoimpl.Executein theExecutefunction is correctly implemented. Verify that theimpl.Executefunction makes appropriate use of the environment parameter.- 262-262: Including
k.environmentin the call toimpl.Querywithin theQueryfunction aligns with the PR's objectives. Ensure that theimpl.Queryfunction properly utilizes the environment parameter.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 150c8a8 and e0579f5811bde2be3fa5b11a46d524896e662260.Files selected for processing (1)
- x/accounts/keeper.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/accounts/keeper.go
|
question why dont we just pass the the event service into the |
|
hmm this seems to put it on the app dev to set it. i think its cleaner like the current approach |
seem like accounts Keeper struct already have appmodule Enviroment field, you just need to pass event service to the |
seems i have to go this way, i will amend |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (6)
- go.work.example (1 hunks)
- x/accounts/defaults/base/account.go (5 hunks)
- x/accounts/internal/implementation/context.go (2 hunks)
- x/accounts/internal/implementation/implementation.go (5 hunks)
- x/accounts/keeper.go (4 hunks)
- x/accounts/testing/counter/counter.go (6 hunks)
Files skipped from review as they are similar to previous changes (4)
- x/accounts/defaults/base/account.go
- x/accounts/internal/implementation/implementation.go
- x/accounts/keeper.go
- x/accounts/testing/counter/counter.go
Additional comments: 9
go.work.example (1)
- 3-3: The update of the Go toolchain version from
go1.22.0togo1.22is correct and aligns with Go versioning practices.x/accounts/internal/implementation/context.go (8)
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [11-13]
The definition of
AccountStatePrefixusingcollections.NewPrefix(255)is correct and follows best practices for defining a unique prefix for account state storage. This ensures that account data is properly namespaced within the module's storage.
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [15-22]
The type definitions for
ModuleExecUntypedFunc,ModuleExecFunc, andModuleQueryFuncare clear and well-defined. They provide a strong typing for module execution and query functions, which enhances code readability and maintainability.
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [24-39]
The
contextValuestruct is well-structured and encapsulates all necessary information for account execution context. However, ensure that sensitive information, such assenderandwhoamiaddresses, is handled securely throughout the codebase to prevent any potential security issues.
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [41-48]
The functions
addCtxandgetCtxfor adding and retrievingcontextValuefrom acontext.Contextare correctly implemented. They follow best practices for context management in Go by usingcontext.WithValueand type assertion.
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [50-69]
The
MakeAccountContextfunction is crucial for creating a new account execution context. It correctly initializes acontextValuestruct with all necessary fields and adds it to the provided context. Ensure that thestoreSvcparameter is validated before use to prevent any potential issues with uninitialized or invalid store services.
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [71-76]
The
makeAccountStorefunction correctly creates a prefixed store for the account using the account number. This approach ensures that each account's state is isolated and easily accessible. The use ofbinary.BigEndian.PutUint64for prefix generation is appropriate for ensuring a consistent byte representation across different platforms.
- 5-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [78-109]
The functions
ExecModuleUntyped,ExecModule, andQueryModuleprovide mechanisms for executing and querying module messages. They are well-implemented, leveraging thecontextValuestruct to access necessary context information. Ensure that error handling is robust and that any errors returned bymoduleExecUntyped,moduleExec, andmoduleQueryare appropriately handled and logged.Verification successful
The review comment concerning the functions
ExecModuleUntyped,ExecModule, andQueryModuleand their error handling has been verified. The implementation of these functions incontext.godemonstrates that errors from lower-level calls are appropriately returned to the caller, adhering to Go's error handling conventions. This approach allows callers to handle errors flexibly, including logging them if necessary. Test cases incontext_test.goandaccount_test.gofurther illustrate how these errors can be handled in practice. Based on the provided script output and analysis, the error handling mechanism for these functions appears to be robust and well-implemented.* 5-10: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [111-122]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in the usage of `ExecModuleUntyped`, `ExecModule`, and `QueryModule`. rg --type go 'ExecModuleUntyped|ExecModule|QueryModule'Length of output: 90494
Utility functions like
openKVStore,Sender,Whoami, andFundsprovide convenient access to the account execution context's fields. These functions are correctly implemented and follow best practices for encapsulation and data access within the context.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (6)
- x/accounts/defaults/base/account.go (1 hunks)
- x/accounts/internal/implementation/api_builder_test.go (1 hunks)
- x/accounts/internal/implementation/implementation.go (4 hunks)
- x/accounts/keeper.go (1 hunks)
- x/accounts/testing/account_abstraction/minimal.go (4 hunks)
- x/accounts/testing/counter/counter.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- x/accounts/defaults/base/account.go
- x/accounts/internal/implementation/api_builder_test.go
- x/accounts/internal/implementation/implementation.go
- x/accounts/keeper.go
- x/accounts/testing/account_abstraction/minimal.go
- x/accounts/testing/counter/counter.go
tac0turtle
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/accounts/internal/implementation/context.go (2 hunks)
- x/accounts/internal/implementation/implementation.go (4 hunks)
- x/accounts/keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/accounts/internal/implementation/context.go
- x/accounts/internal/implementation/implementation.go
- x/accounts/keeper.go
likhita-809
changed the title
Description
Closes: #17687
add event emission to accounts module
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
New Features
Chores
appmoduleandevent, facilitating new features and refactors.