-
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: access to msg events result on postHandler #18377
Conversation
WalkthroughThe changes across multiple files primarily revolve around the addition of a new parameter Changes
Poem
TipsChat with CodeRabbit Bot (
|
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: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- baseapp/baseapp.go (1 hunks)
- testutil/mock/types_handler.go (2 hunks)
- types/handler.go (3 hunks)
- types/handler_test.go (2 hunks)
Additional comments: 7
types/handler.go (2)
- 1-14: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [1-21]
The changes to the
PostHandler
andPostDecorator
function signatures look good. They now accept an additional parametertxEvents []abci.Event
which allows passing transaction events to the post-processing logic. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures.
- 69-81: The
ChainPostDecorators
function has been updated to accommodate the newtxEvents
parameter in thePostHandler
function calls. This looks good.testutil/mock/types_handler.go (3)
10-16: The new import statement is correctly added to import the
abci
package. This is necessary for the newtxEvents []abci.Event
parameter in thePostHandle
function.80-84: The
PostHandle
function in theMockPostDecorator
struct now correctly takes an additional parametertxEvents []abci.Event
before thesimulate
parameter. The function also now returns(types.Context, error)
instead of justtypes.Context
, which is a good practice for error handling.88-90: The
PostHandle
method in theMockPostDecoratorMockRecorder
struct has been updated to include thetxEvents
parameter. This is necessary to match the updated function signature ofPostHandle
.baseapp/baseapp.go (1)
- 942-945: The
postHandler
function now takes an additional argumentresult.Events
. Ensure that all implementations ofpostHandler
have been updated to match this new signature. Also, verify that the error handling logic is still correct given the new argument.types/handler_test.go (1)
- 3-10: The import order is fine and follows the Go convention of grouping standard library imports and third-party imports separately.
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.
Hey events shouldn't be depended on like this since they are not part of consensus. If a module or contract changes their events it could cause issues for your code. At this time we may not be able to accept this pr
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 UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 40-46: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [43-47]
The changes in the changelog are clear and concise, providing a good summary of the new features introduced in the pull request. The links to the pull requests provide a way for developers to get more detailed information about each change.
Hey @tac0turtle I see your point, what if we add some warnings that this can be a double edge sword? Without allowing events to be in the PostHandler seems like its functionalities and scope is very limited. What do you think? |
Sleeping over it makes sense the way mention it. Thanks for the fast review and explanation! I'll proceed to close this PR |
When using PostHandler developers may expect to have access to the events from each message execution, that way developers would be able to do operations based on the results of a specific transaction e.g.: feeshare module from Juno could split the fees equally between all smart contracts involved in that tx by iterating the events, getting the type "execute" and splitting the fee between all the _contract_addresses. Pleas let me know your thoughts and if it is a good solution for a future release.
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
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...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit