-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Baseapp recovery middleware #6053
Conversation
Thanks for contributing to the framework @iTiky! This proposal looks very interesting. Before taking a look at the actual changes, it's clear that this work makes critical changes to the execution model design. That being the case, I'd highly recommend first creating and submitting an ADR (essentially an RFC). This way we have record of the context and what decision was made and why. We should leave this PR open and to be used as a reference in the meantime 👍 |
# Conflicts: # CHANGELOG.md # baseapp/baseapp.go
Codecov Report
@@ Coverage Diff @@
## master #6053 +/- ##
==========================================
+ Coverage 55.66% 55.70% +0.03%
==========================================
Files 448 449 +1
Lines 26983 27006 +23
==========================================
+ Hits 15020 15043 +23
+ Misses 10884 10883 -1
- Partials 1079 1080 +1 |
@alexanderbez As the ADR-022 is approved can we push this implementation? |
Yes, ADR is approved. Let's merge that and then we can further review this 👍 |
@alexanderbez ADR is merged and implementation here is identical to the discussed one. Can we push it forward? I'm really looking forward to use this solution in our project and remove unnecessary |
@iTiky do you mind merging in the latest |
Done |
@alexanderbez Was the local test successful? I'm really curious about that ) |
@iTiky please merge |
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.
Tested, it seems to work pretty well. Though I think we may need some docs here: docs/core/
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.
Approved
@iTiky could merge master |
@Mergifyio refresh |
Command |
Description
PR adds panic recovery middleware which is used in app.runTx() method. Feature add ability for Cosmos SDK based projects to register custom panic handlers for app.Deliver().
Proposed middleware acts as a handling chain for
recover()
object, for example: Custom user-defined handler -> Standard OutOfGas handler -> Default handler. User can assertrecovery object
type and if it matches, add some logging context / emit panic / etc (example).Idea background:
panic
source (critical error) we have to emitpanic()
to start aCONSENSUS FAILURE!!!
mechanism. For that we have to inject (copy-paste) the entirebaseapp
source code to modify it and it makes a lot of migration to newer SDK version problems.defer recover()
sections where the handling customization is required.For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)