-
Notifications
You must be signed in to change notification settings - Fork 66
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
Refactor DeliverTx hook so that panics can be handled #543
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
+ Coverage 54.88% 54.90% +0.01%
==========================================
Files 631 631
Lines 54904 54920 +16
==========================================
+ Hits 30135 30152 +17
+ Misses 22615 22614 -1
Partials 2154 2154
Flags with carried forward coverage won't be shown. Click here to find out more.
|
baseapp/baseapp.go
Outdated
resultStr := "" | ||
res := app.getDeliverTxResponse(ctx, gInfo, result, &resultStr) | ||
for _, hook := range app.deliverTxHooks { | ||
hook(ctx, tx, checksum, res) |
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.
this will affect the actual gas Used for the tx execution, but will be using the res
with a stale gas used from getDeliverTxResponse. is there a scenario where this difference between the deliverTx response used for hooks and deliver tx response returned via tendermint causes an issue?
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.
hooks (as of right now) don't use gasUsed passed in. I changed the data structure to be one specific to delivertx hooks that exclude gasUsed to avoid future misuse.
} | ||
} | ||
var events []abci.Event = []abci.Event{} | ||
if result != nil { |
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.
don't we also need to handle the case in which result.EvmError
is non-nil? or do we want to index events for that case as well?
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 existing behavior is to index events even if EvmError is non-nil https://github.com/sei-protocol/sei-cosmos/pull/543/files#diff-389d02f089ab55575400f5300d3c5cfe277fde5f04bd0fe6a85456a7252d1099R309-R330
not sure if we want to change that. I can see reasons both ways
Describe your changes and provide context
Panics in tx handler is usually handled by a defer statement containing a
recover()
clause. Previously the call stack looks like:In the above structure, DeliverTx hooks are run on
DeliverTx
level, so they are outside the deferred recover clause which is withinrunTx
level.This PR changes it to be:
so that the hook can be recovered as well
Testing performed to validate your change
unit test