-
Notifications
You must be signed in to change notification settings - Fork 620
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
Fix missing events in OnRecvPacket #233
Conversation
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.
Very nice catch!! Just left a suggestion for adding a in code comment for easier verifiability of the code. Tested ACK
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.
On second thought, I think we should emit events in all cases. So we should move the line added above the if statement. This allows failed acks to be emitted as well
Great catch! Agreed with Colin that we should emit events regardless of callback success. We must document to developers that state gets reverted but any emitted events still get emitted even in error case |
e2c5746
to
c607317
Compare
@colin-axner |
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.
LGTM, thanks for the contribution @bluele and the fast response time!
* fix to set events to the original context * Update modules/core/keeper/msg_server.go Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
* fix to set events to the original context * Update modules/core/keeper/msg_server.go Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
* Fix/channel open/close events (#220) * fix: moving event to keeper function instead of rpc handler * refactor: removing unnecessary handler * refactor: delete channel handler file * Apply suggestions from code review Co-authored-by: Aditya <adityasripal@gmail.com> * ibc: fix metrics (#223) * add missing changelog entries (#230) * add missing changelog entries * add missing entry * Fix missing events in OnRecvPacket (#233) * fix to set events to the original context * Update modules/core/keeper/msg_server.go Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * bump SDK dependency to v0.43.0-rc0 (#229) Co-authored-by: Aditya <adityasripal@gmail.com> * Sentinel Root Fix (#234) * fix sentinel value * add godoc and test * fix grammar * add changelog Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * export connection params (#242) * ensure latest height revision number matches chain id revision number (#241) * ensure latest height revision number matches chain id revision number fix tests as well * add changelog * Update modules/light-clients/07-tendermint/types/client_state_test.go * Update modules/light-clients/07-tendermint/types/client_state_test.go * address review suggestions * fix merge conflict Co-authored-by: Sean King <seantking@users.noreply.github.com> Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Jun Kimura <junkxdev@gmail.com>
* fix to set events to the original context * Update modules/core/keeper/msg_server.go Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Restrict querier redispatched gas
Description
The events set in the cache context need to be set in the original context.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes